-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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/compile: memory corruption from //go:notinheap
interface method wrappers
#46903
Comments
Change https://golang.org/cl/330569 mentions this issue: |
This CL extends unified IR to handle creating wrapper methods. There's relatively little about this code that's actually specific to unified IR, but rewriting this logic allows a few benefits: 1. It decouples unified IR from reflectdata.methodWrapper, so the latter code can evolve freely for -G=3's needs. This will also allow the new code to evolve to unified IR's wrapper needs, which I anticipate will operate slightly differently. 2. It provided an opportunity to revisit a lot of the code and simplify/update it to current style. E.g., in the process, I discovered #46903, which unified IR now gets correctly. (I have not yet attempted to fix reflectdata.methodWrapper.) 3. It gives a convenient way for unified IR to ensure all of the wrapper methods it needs are generated correctly. For now, the wrapper generation is specific to non-quirks mode. Change-Id: I5798de6b141f29e8eb6a5c563e7049627ff2868a Reviewed-on: https://go-review.googlesource.com/c/go/+/330569 Trust: Matthew Dempsky <mdempsky@google.com> Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
@mdempsky This looks like it could be fixed, but I haven't been following the unified IR work so I'm not sure if that's been enabled by default. Please close if so. Thanks! |
The program in the initial comment now causes a compiler crash:
This only happens when usin the |
Maybe we can disallow putting notinheap types in interfaces. Do we need notinheap types in interfaces? |
I just tested hacking unified IR to reject converting notinheap types to interface, and the only error building the standard library is this code in netpoll.go for getting the
It's possible end users put cgo |
Thanks. These two don't need methods. So maybe we can make it that notinheap type doesn't satisfy any interface methods. So (only) empty interface is fine. |
I like the idea of forbidding methods on notinheap types. |
It probably suffices to just make it not satisfy interface methods. We can still have methods on them, and calling them directly is still fine. (We currently have methods on notinheap types, e.g. |
I'm a bit worried about subtle desyncs between static and dynamic interface satisfiability. How do folks feel about instead disallowing conversion of The alternative is to implicitly treat all methods on a notinheap type as nointerface. I think that should work fine, but currently nointerface is only used for GOEXPERIMENT=fieldtrack, so it's a bit less exercised than other compiler code paths. |
Yeah, this is what I thought. It is determined statically. The methods are not in the type descriptor's method table, so it cannot be put into an interface with method dynamically (and also statically). |
Okay, that SGTM. I'll double check the nointerface logic. |
It's unfortunately not as simple as implicitly marking them all as nointerface methods, because we have to worry about promoted methods too. But we only want methods promoted to a NIH type's method set to be marked nointerface, whereas currently the It might be a little more robust to require all methods (including promoted methods) on //go:notinheap types to be explicitly marked as //go:nointerface. That's perhaps more tedious for the runtime team, but would be more obvious what's happening too. |
Rolling milestone forward to 1.20. |
Do I understand correctly that the symptom of this is now a compiler crash rather than memory corruption? If so, we should retitle the issue. |
|
If
T
is a//go:notinheap
type, then*T
is treated likeuintptr
rather than pointer-shaped. One consequence of this is that the interface method wrappers forT
need to use**T
instead of*T
.However, reflectdata.methodWrapper doesn't handle this correctly. For example, this program panics, whereas it succeeds if you remove the
//go:notinheap
directive: https://play.golang.org/p/p7TiaXlyJzXDiscovered while reimplementing wrapper generation for unified IR.
The text was updated successfully, but these errors were encountered: