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 #40954

Closed
steeve opened this issue Aug 21, 2020 · 46 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@steeve
Copy link
Contributor

steeve commented Aug 21, 2020

This is a variant of #26213

What version of Go are you using (go version)?

$ go version
go version go1.13.8 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/steeve/Library/Caches/go-build"
GOENV="/Users/steeve/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/steeve/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.14.5/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.14.5/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/steeve/code/github.com/znly/zenly-client-core/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/td/p8xt8d953g37w9f841byty940000gn/T/go-build199341614=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

package blah

/*
#include <jni.h>
*/
import "C"

var blah C.jmethodID = 0
var bleh C.jfieldID = 0

with the Android NDK:

$ CC=/path/to/ndk-standalone-x86_64/bin/clang CGO_ENABLED=1 GOOS=android go build program.go

What did you expect to see?

No error.

What did you see instead?

# command-line-arguments
./program.go:8: cannot use 0 (type int) as type _Ctype_jmethodID in assignment

Notes

This issue is the same as #26213, only with jmethodID and jfieldID. It seems the bug was always there, but for some reason it now reliably crashes on Android 11 with the following:

08-21 16:33:21.842 15438     0 E Go      : runtime: bad pointer in frame github.com/mypkg.func1 at 0x40000bfc70: 0x71
08-21 16:33:21.842 15438     0 E Go      : fatal error: invalid pointer found on stack
08-21 16:33:21.898 15438     0 E Go      : 
08-21 16:33:21.898 15438     0 E Go      : runtime stack:
08-21 16:33:21.899 15438     0 E Go      : runtime.throw(0x7b60f3d00a, 0x1e)
08-21 16:33:21.899 15438     0 E Go      :  GOROOT/src/runtime/panic.go:847 +0x4c fp=0x7b4c4ab3e0 sp=0x7b4c4ab3b0 pc=0x7b612d594c
08-21 16:33:21.899 15438     0 E Go      : runtime.adjustpointers(0x40000bfc68, 0x7b4c4ab4c8, 0x7b4c4ab8b8, 0x7b62c0acd0, 0x7b630d0780)
08-21 16:33:21.899 15438     0 E Go      :  GOROOT/src/runtime/stack.go:591 +0x1dc fp=0x7b4c4ab430 sp=0x7b4c4ab3e0 pc=0x7b612ecc7c
08-21 16:33:21.899 15438     0 E Go      : runtime.adjustframe(0x7b4c4ab7c0, 0x7b4c4ab8b8, 0x7b630d0780)
08-21 16:33:21.899 15438     0 E Go      :  GOROOT/src/runtime/stack.go:633 +0x260 fp=0x7b4c4ab4f0 sp=0x7b4c4ab430 pc=0x7b612ecef0
08-21 16:33:21.899 15438     0 E Go      : runtime.gentraceback(0xffffffffffffffff, 0xffffffffffffffff, 0x0, 0x40000b3500, 0x0, 0x0, 0x7fffffff, 0x7b625ed288, 0x7b4c4ab8b8, 0x0, ...)
08-21 16:33:21.899 15438     0 E Go      :  GOROOT/src/runtime/traceback.go:334 +0xd4c fp=0x7b4c4ab820 sp=0x7b4c4ab4f0 pc=0x7b612f705c
08-21 16:33:21.899 15438     0 E Go      : runtime.copystack(0x40000b3500, 0x1000, 0x7ca8b3bc01)
08-21 16:33:21.899 15438     0 E Go      :  GOROOT/src/runtime/stack.go:886 +0x1b4 fp=0x7b4c4ab9e0 sp=0x7b4c4ab820 pc=0x7b612ed484
08-21 16:33:21.899 15438     0 E Go      : runtime.newstack()
08-21 16:33:21.899 15438     0 E Go      :  GOROOT/src/runtime/stack.go:1055 +0x24c fp=0x7b4c4abb80 sp=0x7b4c4ab9e0 pc=0x7b612ed7bc
08-21 16:33:21.899 15438     0 E Go      : runtime.morestack()
08-21 16:33:21.899 15438     0 E Go      :  src/runtime/asm_arm64.s:310 +0x70 fp=0x7b4c4abb80 sp=0x7b4c4abb80 pc=0x7b613003b0

We are running a modified 1.13.8 and adding jmethodID and jfieldID to the list of badJNI types fixes the issue.

They are defined like so in Android NDK:

struct _jfieldID;                       /* opaque structure */
typedef struct _jfieldID* jfieldID;     /* field IDs */

struct _jmethodID;                      /* opaque structure */
typedef struct _jmethodID* jmethodID;   /* method IDs */

I'll open a PR with the patch.

@gopherbot
Copy link

Change https://golang.org/cl/249744 mentions this issue: cmd/cgo: add JNI's jmethodID/jfieldID to uintptr for Android NDK

@eliasnaur
Copy link
Contributor

Nice find. Consider backporting this to the Go 1.15 (and Go 1.14?) given that it's easily reproduced on Android 11, which is due soon.

This issue is the same as #26213, only with jmethodID and jfieldID. It seems the bug was always there, but for some reason it now reliably crashes on Android 11 with the following:

Perhaps jmethodID/jfieldID values became IDs, not pointers, in Android 11?

@ianlancetaylor we keep having to whack these moles, each time leading to an API change. In particular, comparing with nil must change to comparing with 0. I'm sure there are many other C APIs out there that misuse pointers like this. Can we handle them better in Cgo?

@steeve
Copy link
Contributor Author

steeve commented Aug 21, 2020

I am indeed in strong favor of a backport in Go 1.15 and Go 1.14 since this is a crasher and Android 11 will be out soon (early Sept if we look at the previous versions)

For what it's worth, I just cherry-picked the commit as-is in our Go 1.13 branch.

@ianlancetaylor
Copy link
Contributor

CC @randall77

@ianlancetaylor
Copy link
Contributor

In Go it's absolutely essential that we be able to distinguish a pointer and an integer. I don't see a way around that.

In cgo we do want C names that are typedefs for pointer types to be usable in Go, and it seems natural to use them as pointers. I don't see a way around that.

I don't fully understand the problem but I don't see any available options. Go requires completely strict typing for pointers, and C does not.

@randall77
Copy link
Contributor

randall77 commented Aug 21, 2020

Yes, it looks like we have to add those two types to the list that need to use uintptr representation.

Does anyone have a pointer to code in Android 11 that makes these pointer-not-pointers? I'm curious what changed and why.

Longer term:

It would be nice if we could distinguish types that are intended to be opaque from ones that are not. The JNI headers do things like:

struct S; // opaque
typedef struct S* T; 

If we could detect pointers like this, ones where there's no possible way you could dereference them, or allocate one in Go (what does cgo do with S? Can you do new(C.S)?), we could autotranslate them to uintptr instead of *C.S or unsafe.Pointer or whatever. There would be a one-time pain of lots of nil->0 conversions on the Go side when we did that, but then maybe not many more going forward.

The other option is to make a special cgo type that's known to the compiler. Syntactically it looks like a pointer (initialized with nil, etc.) but doesn't get added to any pointer maps. I'm not sure how that would work, but maybe possible. It has the same problem as the previous idea though, in that once we have one of these special types we have to make sure they can't be allocated in the Go heap.

What about go:notinheap? We have that comment already, which currently just ensures that we never allocate one in the heap (new(T) fails at compile time if T is marked that way). What if, in addition, it means that pointers to go:notinheap types don't get recorded as pointers in pointer bitmaps? Maybe then we can annotate the appropriate cgo types with go:notinheap.

@gopherbot
Copy link

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

@mdempsky
Copy link
Member

I filed #40507 a little bit ago to look at incomplete struct/union types. I think //go:notinheap is a decent approximation of this (and useful as an optimization, since it allows removing write barriers and GC bits), but it won't prevent users from declaring incomplete C types as global variables.

@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 24, 2020
@cagedmantis cagedmantis added this to the Go1.16 milestone Aug 24, 2020
@randall77 randall77 reopened this Aug 25, 2020
@ianlancetaylor
Copy link
Contributor

@randall77 Nice idea, by the way.

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@randall77
Copy link
Contributor

I think my series of CLs solves this issue, and maybe future such issues going forward as well.
The only thing I worry about is that these CLs are a bit subtle, so backporting them is a nontrivial risk. Do we need to backport? Is there another possible workaround? Can Android 11 wait for 1.16 to come out?

@steeve
Copy link
Contributor Author

steeve commented Aug 27, 2020

I would advise against waiting for 1.16, as Android 11 is due in a few weeks and existing programs may be crashing (at least ours does).

Also, CL 249744 could be easily backported in 1.15 and 1.14 while the go:notinheap fixes it for good in 1.16. WDYT ?

@randall77
Copy link
Contributor

The problem with CL 249744 is that the initializers of jfieldID and jmethodID need to be rewritten from nil to 0. Then for 1.16 they'd need to be rewritten back to nil. That's the kind of code churn we really don't want to put users through.

@eliasnaur
Copy link
Contributor

We could do both in Go 1.16 (convert jfieldID and jmethodID to uintptr, and your notinheap fix). That's once churn (but still in a point release).

However, I'd love for all the special cases (JNI types, CFTypes, EGL* types) to go away now that the longer term fix is in. How about doing CL 249744 for Go 1.15.1 and then remove all special cases at once in Go 1.16? That's more churn, but at least we're getting rid of a Cgo wart. If that's too drastic, only remove the special cases when "go 1.16" or later is in go.mod.

Note that code may not need to be rewritten, if you're careful. For example, I'm using var nilEGLDisplay C.EGLDisplay to compare with the zero value, not 0 or nil, to be compatible across the release that changed the types for EGLDisplay. See https://git.sr.ht/~eliasnaur/gio/tree/master/app/internal/egl/egl.go#L38.

@mdempsky
Copy link
Member

The problem with CL 249744 is that the initializers of jfieldID and jmethodID need to be rewritten from nil to 0. Then for 1.16 they'd need to be rewritten back to nil. That's the kind of code churn we really don't want to put users through.

We could make 0 (the untyped constant) assignable to pointer types (like in C) or make nil assignable to uintptr, as needed. This would be most robust in the compiler, but we could probably recognize some common cases even just within cgo.

BTW, what should checkptr semantics be for notinheap types? (Didn't have to worry about this before when it was runtime only.) I think converting *nih to unsafe.Pointer needs to be an error if it points into the heap. What about the reverse?

@randall77
Copy link
Contributor

BTW, what should checkptr semantics be for notinheap types? (Didn't have to worry about this before when it was runtime only.) I think converting *nih to unsafe.Pointer needs to be an error if it points into the heap. What about the reverse?

Converting *nih to unsafe.Pointer, sounds bad regardless of the target. Just because it doesn't point into the heap currently, doesn't mean it won't in the future (if the heap grows). But we probably want the exception that uintptr(unsafe.Pointer(x)) where x is *nih should be ok. (Not sure if anyone would use that, but maybe.)

Going from unsafe.Pointer to *nih should be ok, if the unsafe.Pointer wasn't pointing to the heap (e.g. it pointed to a global).

@mdempsky
Copy link
Member

Just because it doesn't point into the heap currently, doesn't mean it won't in the future (if the heap grows).

Yeah, though that's a concern for uintptr to unsafe.Pointer conversions too, which checkptr doesn't disallow currently. I think disallowing values that point into the currently reserved heap arena would be reasonable; proactively guarding in case of heap growth seems like it's going to have false positives though.

I think if we want to detect pointers becoming heap pointers due to heap growth, it would be more robust to detect that during heap growth itself. E.g., allocate the new heap memory, but flag it as not quite ready yet; trigger a full GC run; and during heap marking, crash if we find any pointers that point into this newly allocated memory.

@gopherbot
Copy link

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

@randall77
Copy link
Contributor

Yeah, though that's a concern for uintptr to unsafe.Pointer conversions too, which checkptr doesn't disallow currently. I think disallowing values that point into the currently reserved heap arena would be reasonable; proactively guarding in case of heap growth seems like it's going to have false positives though.

Yes, I guess that makes sense. The false negative I was worried about seems very rare.

@eliasnaur
Copy link
Contributor

Gentle ping. What to do for Go 1.15, and can we get rid of the special cases in Go 1.16 with go:notinheap?

@eliasnaur
Copy link
Contributor

FWIW, Android 11 is out: https://blog.google/products/android/android-11.

@steeve
Copy link
Contributor Author

steeve commented Sep 10, 2020

Gentle ping too. I'm in favour of @eliasnaur's plan: merge this one for 1.15 and go:notinheap for 1.16. Go 1.15.2 just got released and it would have been pretty easy to have this one in it, even 1.15.1.
It's also pretty easy to back port on 1.14.

After all it's fixing a very real crash, and Android 11 is out now.

@ianlancetaylor
Copy link
Contributor

I think the suggested plan is:

  1. change cgo to treat jmethodID and jfieldID like jobject (https://golang.org/cl/249744)
  2. backport that change to the 1.15 release branch and possibly the 1.14 release branch
  3. change cgo (in 1.16 only) to mark empty structs as notinheap (https://golang.org/cl/250940)

Does that sound right to everyone?

@randall77 Any objections to this plan?

@randall77
Copy link
Contributor

I was kind of hoping that we could backport #3 (consisting of CLs 250939, 250940, and maybe 251158). That way no user would have to change their code (nil -> 0 initializer is the main pain point).
If we're uncomfortable backporting those CLs, then your suggested plan is fine. It would mean jmethodID and jfieldID would need to be handled as "legacy" bad pointers instead of "improved" bad pointers.

@gopherbot
Copy link

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

@steeve
Copy link
Contributor Author

steeve commented Sep 19, 2020

Thank again folks!

gopherbot pushed a commit that referenced this issue Oct 9, 2020
…heap or stack

Right now we just prevent such types from being on the heap. This CL
makes it so they cannot appear on the stack either. The distinction
between heap and stack is pretty vague at the language level (e.g. it
is affected by -N), and we don't need the flexibility anyway.

Once go:notinheap types cannot be in either place, we don't need to
consider pointers to such types to be pointers, at least according to
the garbage collector and stack copying. (This is the big win of this
CL, in my opinion.)

The distinction between HasPointers and HasHeapPointer no longer
exists. There is only HasPointers.

This CL is cleanup before possible use of go:notinheap to fix #40954.

Update #13386

Change-Id: Ibd895aadf001c0385078a6d4809c3f374991231a
Reviewed-on: https://go-review.googlesource.com/c/go/+/255320
Trust: Keith Randall <khr@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
gopherbot pushed a commit that referenced this issue Oct 9, 2020
The alias doesn't need to be marked go:notinheap. It gets its
notinheap-ness from the target type.

Without this change, the type alias test in the notinheap.go file
generates these two errors:

notinheap.go:62: misplaced compiler directive
notinheap.go:63: type nih must be go:notinheap

The first is a result of go:notinheap pragmas not applying
to type alias declarations.
The second is the result of then trying to match the notinheap-ness
of the alias and the target type.

Add a few more go:notinheap tests while we are here.

Update #40954

Change-Id: I067ec47698df6e9e593e080d67796fd05a1d480f
Reviewed-on: https://go-review.googlesource.com/c/go/+/250939
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Trust: Keith Randall <khr@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/go/+/255337
Trust: Emmanuel Odeke <emm.odeke@gmail.com>
Reviewed-by: Austin Clements <austin@google.com>
gopherbot pushed a commit that referenced this issue Oct 9, 2020
They can't reasonably be allocated on the heap. Not a huge deal, but
it has an interesting and useful side effect.

After CL 249917, the compiler and runtime treat pointers to
go:notinheap types as uintptrs instead of real pointers (no write
barrier, not processed during stack scanning, ...). That feature is
exactly what we want for cgo to fix #40954. All the cases we have of
pointers declared in C, but which might actually be filled with
non-pointer data, are of this form (JNI's jobject heirarch, Darwin's
CFType heirarchy, ...).

Fixes #40954

Change-Id: I44a3b9bc2513d4287107e39d0cbbd0efd46a3aae
Reviewed-on: https://go-review.googlesource.com/c/go/+/250940
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Keith Randall <khr@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/go/+/255321
gopherbot pushed a commit that referenced this issue Oct 9, 2020
…friendlier for cgo

Update #40954

Change-Id: Ifaab7349631ccb12fc892882bbdf7f0ebf3d845f
Reviewed-on: https://go-review.googlesource.com/c/go/+/251158
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Keith Randall <khr@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/go/+/255338
Reviewed-by: Austin Clements <austin@google.com>
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Oct 10, 2020
@cosnicolaou
Copy link
Contributor

I think this change, at least the if dt.Kind == "struct" { ... tt.NotInHeap = true ... } in cgo/gcc.go L2437, part, has an unexpected consequence. It is now in 1.15.3 and is leading to panics in code that uses reflect.DeepEqual on structs that contain pointers created by cgo code. ptrdata is nil and hence the reflect package (value.go:95) will panic when called upon to compare them. From reading this and the code I don't think you were expecting this to cause these panics. I bigger warning in the release notes seems warranted:-)

@randall77
Copy link
Contributor

@cosnicolaou Yes, it looks like we need to relax the condition in reflect.pointer to handle the situation that something has type pointer but has an empty pointer map.

@randall77
Copy link
Contributor

package main

import "reflect"

//go:notinheap
type T struct {
	x int
}

func main() {
	reflect.ValueOf((*T)(nil)).Pointer()
}

This is an example of a program triggering this failure. It panics with panic: can't call pointer on a non-pointer Value.
I think we want this program to panic, as it is unsafe to convert such a pointer into an unsafe.Pointer. Even if the error message is a bit strange, as it is a pointer, just not one that can be manipulated generally.

However, we should do something for uses of pointer() that are ok, like deepequal.

@randall77
Copy link
Contributor

Hm, actually just putting a *T in a reflect.Value is bad, as it is stored internally as an unsafe.Pointer. We may need to teach reflect to understand notinheap and store them as uintptrs.
Maybe force *T when T is notinheap to be indirect?

@cosnicolaou
Copy link
Contributor

cosnicolaou commented Oct 16, 2020 via email

cosnicolaou added a commit to vanadium/core that referenced this issue Oct 16, 2020
- update go.mod with a specific version for the logger submodule
- integration tests with ssl need go 15.1.2, not .3, see golang/go#40954
@randall77
Copy link
Contributor

Your usage of your allocator + finalizer seems fine.

I can probably pull this out into a standalone example/test if you wish,

Thanks, but I don't think that will be necessary. I think I understand what is going on. Not sure what the right fix is yet.

A separate question is why the size information is not available, and I'd
guess that's because the debug dwarf sections are not in the library, but I
don't know enough about dwarf to be sure.

I don't think this has anything to do with dwarf. Cgo uses .h files to find declarations of types and examines those declarations to decide whether they should be notinheap. Can you find the declaration of EC_KEY? You might be able to patch that declaration somehow.

@cosnicolaou
Copy link
Contributor

cosnicolaou commented Oct 17, 2020 via email

@bartle-stripe
Copy link

Should this bug be reopened (or a separate one created) to track this regression?

@randall77
Copy link
Contributor

Yes, I'll open a new issue.

claucece pushed a commit to claucece/go that referenced this issue Oct 22, 2020
…heap or stack

Right now we just prevent such types from being on the heap. This CL
makes it so they cannot appear on the stack either. The distinction
between heap and stack is pretty vague at the language level (e.g. it
is affected by -N), and we don't need the flexibility anyway.

Once go:notinheap types cannot be in either place, we don't need to
consider pointers to such types to be pointers, at least according to
the garbage collector and stack copying. (This is the big win of this
CL, in my opinion.)

The distinction between HasPointers and HasHeapPointer no longer
exists. There is only HasPointers.

This CL is cleanup before possible use of go:notinheap to fix golang#40954.

Update golang#13386

Change-Id: Ibd895aadf001c0385078a6d4809c3f374991231a
Reviewed-on: https://go-review.googlesource.com/c/go/+/255320
Trust: Keith Randall <khr@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
claucece pushed a commit to claucece/go that referenced this issue Oct 22, 2020
The alias doesn't need to be marked go:notinheap. It gets its
notinheap-ness from the target type.

Without this change, the type alias test in the notinheap.go file
generates these two errors:

notinheap.go:62: misplaced compiler directive
notinheap.go:63: type nih must be go:notinheap

The first is a result of go:notinheap pragmas not applying
to type alias declarations.
The second is the result of then trying to match the notinheap-ness
of the alias and the target type.

Add a few more go:notinheap tests while we are here.

Update golang#40954

Change-Id: I067ec47698df6e9e593e080d67796fd05a1d480f
Reviewed-on: https://go-review.googlesource.com/c/go/+/250939
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Trust: Keith Randall <khr@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/go/+/255337
Trust: Emmanuel Odeke <emm.odeke@gmail.com>
Reviewed-by: Austin Clements <austin@google.com>
claucece pushed a commit to claucece/go that referenced this issue Oct 22, 2020
They can't reasonably be allocated on the heap. Not a huge deal, but
it has an interesting and useful side effect.

After CL 249917, the compiler and runtime treat pointers to
go:notinheap types as uintptrs instead of real pointers (no write
barrier, not processed during stack scanning, ...). That feature is
exactly what we want for cgo to fix golang#40954. All the cases we have of
pointers declared in C, but which might actually be filled with
non-pointer data, are of this form (JNI's jobject heirarch, Darwin's
CFType heirarchy, ...).

Fixes golang#40954

Change-Id: I44a3b9bc2513d4287107e39d0cbbd0efd46a3aae
Reviewed-on: https://go-review.googlesource.com/c/go/+/250940
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Keith Randall <khr@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/go/+/255321
claucece pushed a commit to claucece/go that referenced this issue Oct 22, 2020
…friendlier for cgo

Update golang#40954

Change-Id: Ifaab7349631ccb12fc892882bbdf7f0ebf3d845f
Reviewed-on: https://go-review.googlesource.com/c/go/+/251158
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Keith Randall <khr@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/go/+/255338
Reviewed-by: Austin Clements <austin@google.com>
@gopherbot
Copy link

Change https://golang.org/cl/277432 mentions this issue: doc/go1.15: mention 1.15.3 cgo restriction on empty structs

gopherbot pushed a commit that referenced this issue Dec 15, 2020
For #40954

Change-Id: I6a30aed31a16e820817f4ca5c7f591222e922946
Reviewed-on: https://go-review.googlesource.com/c/go/+/277432
Trust: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/278573 mentions this issue: [release-branch.go1.15] doc/go1.15: mention 1.15.3 cgo restriction on empty structs

gopherbot pushed a commit that referenced this issue Dec 16, 2020
… empty structs

For #40954

Change-Id: I6a30aed31a16e820817f4ca5c7f591222e922946
Reviewed-on: https://go-review.googlesource.com/c/go/+/277432
Trust: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
(cherry picked from commit 129bb19)
Reviewed-on: https://go-review.googlesource.com/c/go/+/278573
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@golang golang locked and limited conversation to collaborators Dec 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants