Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
gopherbot opened this issue Sep 16, 2020 · 24 comments

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 16, 2020

@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
Copy link
Contributor

@randall77 randall77 commented Sep 16, 2020

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
Copy link
Author

@gopherbot gopherbot commented Sep 16, 2020

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

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Sep 16, 2020

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

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Sep 16, 2020

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

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Sep 16, 2020

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

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Sep 16, 2020

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

@bcmills

This comment was marked as off-topic.

@bcmills

This comment was marked as off-topic.

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Sep 17, 2020

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

gopherbot pushed a commit that referenced this issue Sep 17, 2020
//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
Copy link
Author

@gopherbot gopherbot commented Sep 17, 2020

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

@eliasnaur
Copy link
Contributor

@eliasnaur eliasnaur commented Sep 24, 2020

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

@toothrot
Copy link
Contributor

@toothrot toothrot commented Sep 24, 2020

@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
Copy link
Member

@aclements aclements commented Sep 24, 2020

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
Copy link
Member

@andybons andybons commented Oct 2, 2020

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
Copy link
Author

@gopherbot gopherbot commented Oct 2, 2020

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

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Oct 6, 2020

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 #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
Copy link
Member

@dmitshur dmitshur commented Oct 8, 2020

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

This comment has been minimized.

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Oct 9, 2020

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

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Oct 9, 2020

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

@gopherbot gopherbot closed this Oct 9, 2020
gopherbot pushed a commit that referenced this issue Oct 9, 2020
…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>
gopherbot pushed a commit that referenced this issue Oct 9, 2020
//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
Copy link
Member

@dmitshur dmitshur commented Oct 9, 2020

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

@dmitshur dmitshur reopened this Oct 9, 2020
@gopherbot gopherbot closed this Oct 9, 2020
@gopherbot

This comment has been minimized.

@dmitshur dmitshur reopened this Oct 9, 2020
gopherbot pushed a commit that referenced this issue Oct 12, 2020
…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
Copy link
Member

@dmitshur dmitshur commented Oct 12, 2020

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

@dmitshur dmitshur closed this Oct 12, 2020
@aykevl
Copy link

@aykevl aykevl commented Oct 16, 2020

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
9 participants
You can’t perform that action at this time.