cmd/compile, runtime, reflect: pointers to go:notinheap types must be stored indirectly in interfaces #42076
Comments
|
Would you expect this to also manifest as a segmentation fault? From running |
|
@bartle-stripe I don't think that would be related, no. |
|
Hm, so git bisect is implicating 9698372 and I can repro this pretty readily. |
|
@bartle-stripe Interesting. Can you open a new issue with all the details? |
cagedmantis
added
the
NeedsInvestigation
|
I know this is ugly, but would it make sense to add an annotation, eg, :opaque, to use for marking such opaque pointers and having the reflect package understand it - i.e. a pointer that's ignored by gc, but used by reflect for direct pointer comparison? This would allow the opaque pointer to be wrapped by a go type and a finalizer can be used to free the underlying type in C or whatever other language is being used. I guess it would require an extra field in reflect.Value which is an overhead. |
|
A complication in the runtime. That's writing a pointer to a Not sure why the prohibition on implicit allocation in the runtime didn't catch this. |
|
This kind of implicit allocation isn't caught by the runtime prohibition. I added a check to see what happens, and it seems to trigger quite a bit, all on conversions to the argument of |
|
Assuming they're mostly a few common data types (ints, strings, etc), seems like with some helpers this would go pretty fast. And if it'd catch bugs, seems worthwhile. Alternatively, we could teach the compiler (and runtime backtracing) to use specialized panic handlers for common panic arg types, thereby avoiding the code weight associated with allocations in everyone's code. |
|
Change https://golang.org/cl/264480 mentions this issue: |
|
@gopherbot Please open a backport issue for 1.15. |
|
Backport issue(s) opened: #42169 (for 1.15). Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases. |
|
Change https://golang.org/cl/265757 mentions this issue: |
…to unsafe.Pointer Pretty minor concern, but after auditing the compiler/runtime for conversions from pointers to go:notinheap types to unsafe.Pointer, this is the only remaining one I found. Update #42076 Change-Id: I81d5b893c9ada2fc19a51c2559262f2e9ff71c35 Reviewed-on: https://go-review.googlesource.com/c/go/+/265757 Trust: Keith Randall <khr@golang.org> Run-TryBot: Keith Randall <khr@golang.org> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com>
|
Change https://golang.org/cl/308970 mentions this issue: |
For #42076 Fixes #45451 Change-Id: I69646226d3480d5403205412ddd13c0cfc2c8a53 Reviewed-on: https://go-review.googlesource.com/c/go/+/308970 Trust: Ian Lance Taylor <iant@golang.org> Run-TryBot: Ian Lance Taylor <iant@golang.org> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Keith Randall <khr@golang.org>
|
Change https://golang.org/cl/340950 mentions this issue: |
|
Change https://golang.org/cl/340609 mentions this issue: |
This is the gofrontend version of https://golang.org/cl/264480. For golang/go#42076 Change-Id: Ic8949fe5f052438a2858aae938eb9941dbd7c27c Reviewed-on: https://go-review.googlesource.com/c/gofrontend/+/340609 Trust: Ian Lance Taylor <iant@golang.org> Reviewed-by: Cherry Mui <cherryyz@google.com> Reviewed-by: Than McIntosh <thanm@google.com>
This is the gofrontend version of https://golang.org/cl/264480. For golang/go#42076 Reviewed-on: https://go-review.googlesource.com/c/gofrontend/+/340609
The first version of CL 340609 for gofrontend passed all existing tests, but not this one. For #42076 Change-Id: I6491e2f186091bdae140b7f7befa511806a6478a Reviewed-on: https://go-review.googlesource.com/c/go/+/340950 Trust: Ian Lance Taylor <iant@golang.org> Run-TryBot: Ian Lance Taylor <iant@golang.org> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cherry Mui <cherryyz@google.com> Reviewed-by: Than McIntosh <thanm@google.com>
(This bug is a consequence of the fixes for issue #40954.)
This program panics with:
reflectis confused by a type that claims to be a pointer, but has no pointer bits. It has no pointer bits because pointers togo:notinheaptypes are treated as uintptrs by the compiler.More generally, we can't ever convert
*Ttounsafe.Pointer, as that's the equivalent of converting auintptr, potentially containing a bad pointer value, to a pointer. Particularly, we can't store a*Tin the data field of an interface, or inreflect.Value.ptr. We have to store them indirectly instead.This issue originally came up with the a call to
reflect.DeepEqualwhich ended up callingPointeron one of these types.Note that
reflect.DeepEqualfundamentally doesn't work on incomplete types like this. They are represented asstruct{}, so pointer equality doesn't really work. This program:Will print
trueafter this issue is fixed, although one would probably expect it to returnfalse. Something about incomplete types and deepequal just don't play well together.But it shouldn't panic.
The text was updated successfully, but these errors were encountered: