cmd/cgo: jmethodID/jfieldID is not mapped to uintptr if building with the Android NDK [1.15 backport] #41432

gopherbot · 2020-09-16T17:29:20Z

@randall77 requested issue #40954 to be considered for backport to the next 1.15 minor release.

@gopherbot please open a backport issue for 1.15.

randall77 · 2020-09-16T17:32:33Z

This issue prevents Go programs from running correctly on Android 11 (random, but somewhat reliable crashing).

I'm proposing we just backport to 1.15. Although this is a problem in earlier releases as well, we can require 1.15 for Android 11 support, which was just released.

gopherbot · 2020-09-16T17:55:34Z

Change https://golang.org/cl/255318 mentions this issue: cmd/compile: make Haspointers a method instead of a function

gopherbot · 2020-09-16T17:56:46Z

Change https://golang.org/cl/255320 mentions this issue: cmd/compile: don't allow go:notinheap on the heap or stack

gopherbot · 2020-09-16T17:57:44Z

Change https://golang.org/cl/255337 mentions this issue: cmd/compile: allow aliases to go:notinheap types

gopherbot · 2020-09-16T17:59:03Z

Change https://golang.org/cl/255321 mentions this issue: cmd/cgo: use go:notinheap for anonymous structs

gopherbot · 2020-09-16T17:59:55Z

Change https://golang.org/cl/255338 mentions this issue: cmd/compile: make go:notinheap error message friendlier for cgo

gopherbot · 2020-09-17T17:00:34Z

Change https://golang.org/cl/255637 mentions this issue: cmd/compile: propagate go:notinheap implicitly

//go:notinheap type T int type U T We already correctly propagate the notinheap-ness of T to U. But we have an assertion in the typechecker that if there's no explicit //go:notinheap associated with U, then report an error. Get rid of that error so that implicit propagation is allowed. Adjust the tests so that we make sure that uses of types like U do correctly report an error when U is used in a context that might cause a Go heap allocation. Fixes #41451 Update #40954 Update #41432 Change-Id: I1692bc7cceff21ebb3f557f3748812a40887118d Reviewed-on: https://go-review.googlesource.com/c/go/+/255637 Run-TryBot: Keith Randall <khr@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com> Reviewed-by: Ian Lance Taylor <iant@golang.org> Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com> TryBot-Result: Go Bot <gobot@golang.org>

gopherbot · 2020-09-17T19:38:00Z

Change https://golang.org/cl/255697 mentions this issue: cmd/compile: propagate go:notinheap implicitly

eliasnaur · 2020-09-24T08:56:05Z

Gentle ping. I hope this backport can make the Go 1.15.3 release.

toothrot · 2020-09-24T18:23:26Z

@eliasnaur We are looking into balancing the complexity of the changes, and hopefully will weigh in soon. I'm sorry for the delay and lack of comments, but I want you to know that we are actively discussing this.

aclements · 2020-09-24T20:53:57Z

I've read over all the CLs and I'm happy with them. The one outstanding concern, I believe, is whether this breaks any existing cgo-using code. The tip commits are currently working their way through global testing inside Google. If they pass that without introducing new build failures, then I'm good with the cherry-pick.

andybons · 2020-10-02T19:46:59Z

We’re going to run tests on our internal corpus of code before we proceed. That should happen by next week, but no promises :) /cc @neild

gopherbot · 2020-10-02T23:31:38Z

Change https://golang.org/cl/259300 mentions this issue: [release-branch.go1.15] cmd/compile: export notinheap annotation to object file

dmitshur · 2020-10-06T18:11:14Z

The tip commits are currently working their way through global testing inside Google. If they pass that without introducing new build failures, then I'm good with the cherry-pick.

For (my) reference, the tip commit hashes that correspond to the backport CLs are (latest on top):

a9c75ec - cmd/compile: export notinheap annotation to object file (newly added; see cmd/cgo: exported cgo types not marked as go:notinheap [1.15 backport] #41766 (comment))
2205379 - cmd/compile: propagate go:notinheap implicitly
37f2610 - cmd/compile: make go:notinheap error message friendlier for cgo
42b023d - cmd/cgo: use go:notinheap for anonymous structs
4f91591 - cmd/compile: allow aliases to go:notinheap types
d9a6bdf - cmd/compile: don't allow go:notinheap on the heap or stack
623652e - cmd/compile: make Haspointers a method instead of a function
c060260 - runtime: implement StorepNoWB for wasm in assembly (newly added; see CL comment)

dmitshur · 2020-10-08T20:49:03Z

The global testing inside Google has completed and no new issues were identified connected to the aforementioned commits.

Approving for Go 1.15 per discussion in a release meeting, this is a serious issue without a workaround where we needed to balance multiple factors going into the decision.

gopherbot · 2020-10-09T15:40:58Z

Change https://golang.org/cl/260878 mentions this issue: [release-branch.go1.15] runtime: implement StorepNoWB for wasm in assembly

gopherbot · 2020-10-09T17:23:37Z

Closed by merging 0b80775 to release-branch.go1.15.

…embly The second argument of StorepNoWB must be forced to escape. The current Go code does not explicitly enforce that property. By implementing in assembly, and not using go:noescape, we force the issue. Test is in CL 249761. Issue #40975. This CL is needed for CL 249917, which changes how go:notinheap works and breaks the previous StorepNoWB wasm code. I checked for other possible errors like this. This is the only go:notinheap that isn't in the runtime itself. Included test from CL 249761. Update #41432 Change-Id: I43400a806662655727c4a3baa8902b63bdc9fa57 Reviewed-on: https://go-review.googlesource.com/c/go/+/249962 Run-TryBot: Keith Randall <khr@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com> (cherry picked from commit c060260) Reviewed-on: https://go-review.googlesource.com/c/go/+/260878 Trust: Keith Randall <khr@golang.org> Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com> Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>

//go:notinheap type T int type U T We already correctly propagate the notinheap-ness of T to U. But we have an assertion in the typechecker that if there's no explicit //go:notinheap associated with U, then report an error. Get rid of that error so that implicit propagation is allowed. Adjust the tests so that we make sure that uses of types like U do correctly report an error when U is used in a context that might cause a Go heap allocation. Update #41432 Change-Id: I1692bc7cceff21ebb3f557f3748812a40887118d Reviewed-on: https://go-review.googlesource.com/c/go/+/255637 Run-TryBot: Keith Randall <khr@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com> Reviewed-by: Ian Lance Taylor <iant@golang.org> Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com> TryBot-Result: Go Bot <gobot@golang.org> (cherry picked from commit 2205379) Reviewed-on: https://go-review.googlesource.com/c/go/+/255697 Trust: Keith Randall <khr@golang.org> Reviewed-by: Austin Clements <austin@google.com>

dmitshur · 2020-10-09T18:16:23Z

There's one more CL in the stack that needs to be submitted before this is fully resolved. Reopening to track that.

…bject file In the rare case when a cgo type makes it into an object file, we need the go:notinheap annotation to go with it. Fixes #41432. Change-Id: Ie2ef241ee49661792e0d8c8c46c51b2fe5c6fa7c Reviewed-on: https://go-review.googlesource.com/c/go/+/259300 Trust: Keith Randall <khr@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Cherry Zhang <cherryyz@google.com>

dmitshur · 2020-10-12T15:21:16Z

Closed by merging 76a2c87 to release-branch.go1.15.

aykevl · 2020-10-16T23:39:56Z

The one outstanding concern, I believe, is whether this breaks any existing cgo-using code.

I'm afraid it did break something, see #42032.

…embly The second argument of StorepNoWB must be forced to escape. The current Go code does not explicitly enforce that property. By implementing in assembly, and not using go:noescape, we force the issue. Test is in CL 249761. Issue golang#40975. This CL is needed for CL 249917, which changes how go:notinheap works and breaks the previous StorepNoWB wasm code. I checked for other possible errors like this. This is the only go:notinheap that isn't in the runtime itself. Included test from CL 249761. Update golang#41432 Change-Id: I43400a806662655727c4a3baa8902b63bdc9fa57 Reviewed-on: https://go-review.googlesource.com/c/go/+/249962 Run-TryBot: Keith Randall <khr@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com> (cherry picked from commit c060260) Reviewed-on: https://go-review.googlesource.com/c/go/+/260878 Trust: Keith Randall <khr@golang.org> Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com> Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>

//go:notinheap type T int type U T We already correctly propagate the notinheap-ness of T to U. But we have an assertion in the typechecker that if there's no explicit //go:notinheap associated with U, then report an error. Get rid of that error so that implicit propagation is allowed. Adjust the tests so that we make sure that uses of types like U do correctly report an error when U is used in a context that might cause a Go heap allocation. Update golang#41432 Change-Id: I1692bc7cceff21ebb3f557f3748812a40887118d Reviewed-on: https://go-review.googlesource.com/c/go/+/255637 Run-TryBot: Keith Randall <khr@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com> Reviewed-by: Ian Lance Taylor <iant@golang.org> Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com> TryBot-Result: Go Bot <gobot@golang.org> (cherry picked from commit 2205379) Reviewed-on: https://go-review.googlesource.com/c/go/+/255697 Trust: Keith Randall <khr@golang.org> Reviewed-by: Austin Clements <austin@google.com>

…bject file In the rare case when a cgo type makes it into an object file, we need the go:notinheap annotation to go with it. Fixes golang#41432. Change-Id: Ie2ef241ee49661792e0d8c8c46c51b2fe5c6fa7c Reviewed-on: https://go-review.googlesource.com/c/go/+/259300 Trust: Keith Randall <khr@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Cherry Zhang <cherryyz@google.com>

gopherbot added the CherryPickCandidate label Sep 16, 2020

gopherbot mentioned this issue Sep 16, 2020

cmd/cgo: jmethodID/jfieldID is not mapped to uintptr if building with the Android NDK #40954

Closed

gopherbot added this to the Go1.15.3 milestone Sep 16, 2020

randall77 added the release-blocker label Sep 16, 2020

This comment has been minimized.

Sign in to view

bcmills mentioned this issue Sep 17, 2020

cmd/compile: x/perf build broken by go:notinheap changes in Go 1.16 #41451

Closed

randall77 mentioned this issue Oct 2, 2020

cmd/cgo: exported cgo types not marked as go:notinheap [1.15 backport] #41766

Closed

dmitshur added CherryPickApproved and removed CherryPickCandidate labels Oct 8, 2020

This comment has been minimized.

Sign in to view

gopherbot closed this as completed Oct 9, 2020

dmitshur reopened this Oct 9, 2020

gopherbot closed this as completed Oct 9, 2020

This comment has been minimized.

Sign in to view

dmitshur reopened this Oct 9, 2020

dmitshur closed this as completed Oct 12, 2020

aykevl mentioned this issue Oct 16, 2020

cmd/cgo: opaque struct pointers are broken since Go 1.15.3 #42032

Closed

golang locked and limited conversation to collaborators Oct 16, 2021

gopherbot added the FrozenDueToAge label Oct 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/cgo: jmethodID/jfieldID is not mapped to uintptr if building with the Android NDK [1.15 backport] #41432

cmd/cgo: jmethodID/jfieldID is not mapped to uintptr if building with the Android NDK [1.15 backport] #41432

gopherbot commented Sep 16, 2020

randall77 commented Sep 16, 2020

gopherbot commented Sep 16, 2020

gopherbot commented Sep 16, 2020

gopherbot commented Sep 16, 2020

gopherbot commented Sep 16, 2020

gopherbot commented Sep 16, 2020

This comment has been minimized.

This comment has been minimized.

gopherbot commented Sep 17, 2020

gopherbot commented Sep 17, 2020

eliasnaur commented Sep 24, 2020

toothrot commented Sep 24, 2020

aclements commented Sep 24, 2020

andybons commented Oct 2, 2020

gopherbot commented Oct 2, 2020

dmitshur commented Oct 6, 2020 •

edited

Loading

dmitshur commented Oct 8, 2020

This comment has been minimized.

gopherbot commented Oct 9, 2020

gopherbot commented Oct 9, 2020

dmitshur commented Oct 9, 2020

This comment has been minimized.

dmitshur commented Oct 12, 2020

aykevl commented Oct 16, 2020 •

edited

Loading

cmd/cgo: jmethodID/jfieldID is not mapped to uintptr if building with the Android NDK [1.15 backport] #41432

cmd/cgo: jmethodID/jfieldID is not mapped to uintptr if building with the Android NDK [1.15 backport] #41432

Comments

gopherbot commented Sep 16, 2020

randall77 commented Sep 16, 2020

gopherbot commented Sep 16, 2020

gopherbot commented Sep 16, 2020

gopherbot commented Sep 16, 2020

gopherbot commented Sep 16, 2020

gopherbot commented Sep 16, 2020

This comment has been minimized.

This comment has been minimized.

gopherbot commented Sep 17, 2020

gopherbot commented Sep 17, 2020

eliasnaur commented Sep 24, 2020

toothrot commented Sep 24, 2020

aclements commented Sep 24, 2020

andybons commented Oct 2, 2020

gopherbot commented Oct 2, 2020

dmitshur commented Oct 6, 2020 • edited Loading

dmitshur commented Oct 8, 2020

This comment has been minimized.

gopherbot commented Oct 9, 2020

gopherbot commented Oct 9, 2020

dmitshur commented Oct 9, 2020

This comment has been minimized.

dmitshur commented Oct 12, 2020

aykevl commented Oct 16, 2020 • edited Loading

dmitshur commented Oct 6, 2020 •

edited

Loading

aykevl commented Oct 16, 2020 •

edited

Loading