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/compile: memory corruption from //go:notinheap interface method wrappers #46903

Closed
mdempsky opened this issue Jun 24, 2021 · 15 comments
Closed
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@mdempsky
Copy link
Member

If T is a //go:notinheap type, then *T is treated like uintptr rather than pointer-shaped. One consequence of this is that the interface method wrappers for T 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/p7TiaXlyJzX

Discovered while reimplementing wrapper generation for unified IR.

@mdempsky mdempsky added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 24, 2021
@mdempsky mdempsky added this to the Go1.18 milestone Jun 24, 2021
@gopherbot
Copy link

Change https://golang.org/cl/330569 mentions this issue: [dev.typeparams] cmd/compile: generate wrappers within unified IR

gopherbot pushed a commit that referenced this issue Jun 25, 2021
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>
@mknyszek
Copy link
Contributor

@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!

@ianlancetaylor
Copy link
Contributor

The program in the initial comment now causes a compiler crash:

# command-line-arguments
esc
.   CALL tc(1) # <autogenerated>:1:0
.   .   DOTPTR main.M tc(1) # <autogenerated>:1:0
.   .   .   NAME-main..this ld(1) Class:PPARAM Offset:0 OnStack Used PTR-**A tc(1) # <autogenerated>:1:0
<autogenerated>:1: .this.M undefined (type **A has no field or method M)

This only happens when usin the go:notinheap directive, so this doesn't seem critical. The go:notinheap directive isn't even documented. So changing milestone to 1.19.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.18, Go1.19 Jan 28, 2022
@cherrymui
Copy link
Member

Maybe we can disallow putting notinheap types in interfaces. Do we need notinheap types in interfaces?

@mdempsky
Copy link
Member Author

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 *_type for *pollDesc:

netpoll.go:     pdEface any    = (*pollDesc)(nil)
netpoll.go:     pdType  *_type = efaceOf(&pdEface)._type

It's possible end users put cgo C.foo and *C.foo types into interfaces too, though cgo types don't have methods.

@cherrymui
Copy link
Member

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.

@randall77
Copy link
Contributor

I like the idea of forbidding methods on notinheap types.
Or just that their methods don't satisfy otherwise corresponding interface methods?

@cherrymui
Copy link
Member

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. mheap).

@mdempsky
Copy link
Member Author

I'm a bit worried about subtle desyncs between static and dynamic interface satisfiability.

How do folks feel about instead disallowing conversion of T and *T to any interface type (including empty interface), when T is notinheap and has methods?

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.

@cherrymui
Copy link
Member

The alternative is to implicitly treat all methods on a notinheap type as nointerface

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).

@mdempsky
Copy link
Member Author

Okay, that SGTM. I'll double check the nointerface logic.

@mdempsky
Copy link
Member Author

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 //go:nointerface bit is shared across the base method and all promotions.

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.

@ianlancetaylor
Copy link
Contributor

Rolling milestone forward to 1.20.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.19, Go1.20 Jun 24, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
@aclements
Copy link
Member

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.

@cherrymui
Copy link
Member

//go:notinheap is now sys.NotInHeap, and at tip it doesn't seem to have problems with sys.NotInHeap. I think we can close this now.

@golang golang locked and limited conversation to collaborators Jan 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
Development

No branches or pull requests

7 participants