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: more fine-grained mechanism for escape analysis of assembly functions #31525
Comments
Looks like arm64, ppc64, and ppc64le are broken too. |
The problem is all of these functions implement Loadp in assembly and use stubs like:
However, go:noescape is too strong here: it says that ptr doesn't escape at all, but we actually flow *ptr to the result parameter. Easy fix is to remove the go:noescape annotation. |
Change https://golang.org/cl/172578 mentions this issue: |
Related, in package reflect:
Edit: Actually there's a bunch of technically incorrect go:noescape annotations. |
The //go:noescape directive says that arguments don't leak at all, which is too aggressive of a claim for functions that return pointers derived from their parameters. Remove the directive for now. Long term fix will require a new directive that allows more fine-grained control over escape analysis information supplied for functions implemented in assembly. Also, update the BAD comments in the test cases for Loadp: we really want that *ptr leaks to the result parameter, not that *ptr leaks to the heap. Updates #31525. Change-Id: Ibfa61f2b70daa7ed3223056b57eeee777eef2e31 Reviewed-on: https://go-review.googlesource.com/c/go/+/172578 Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Austin Clements <austin@google.com>
Repurposing this issue for tracking the need for a more fine-grained control of escape analysis of external functions. Right now we have |
Change https://golang.org/cl/172602 mentions this issue: |
Casp1 is implemented in Go on js/wasm, so escape analysis correctly determines that the "old" parameter does not escape (which is good). Unfortunately, test/run.go doesn't have a way to indicate that ERROR messages are optional, and cmd/compile only emits diagnostics for "var x int" when it's moved to the heap; not when it stays on the stack. To accomodate that this test currently passes on some GOARCHes but not others, rewrite the Casp1 test to use "x := new(int)" and allow both "new(int) escapes to heap" or "new(int) does not escape". Updates #31525. Change-Id: I40150a7ff9042f184386ccdb2d4d428f63e8ba4f Reviewed-on: https://go-review.googlesource.com/c/go/+/172602 Run-TryBot: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
Can't we make Or are there too many distinctions to make even then (arg->res, *arg->res, **arg->res, etc.)? |
Yeah, I was thinking that might be a good idea to do anyway, but mostly just to make go:noescape a little less prone to accidental misuse. Almost all of the currently annotated functions either don't return anything or just return non-pointer values, so they wouldn't be affected. But that still wouldn't precisely describe the semantics of LoadPointer (return the pointed-to value, not the pointer itself), or StorePointer or CompareAndSwapPointer (one pointer leaks to heap, but the others are noescape). One idea I had was to have a directive like //go:pseudocode that allows you to provide a Go function body for the purposes of escape analysis, but then the function body is discarded and the compiler/linker expects the function to be provided externally (e.g., by assembly). Then you could write:
This would give us an easy way to largely isolate users from escape analysis internals, but still give fine grained details. I also expect it would be pretty non-invasive to implement. (Open to alternative names of course.) |
We could also go for a more invasive language change along these lines. We could allow a user to provide a "default implementation" of a function stub, which is used by cmd/go instead of assembly routines when no assembly routine is available. Then we wouldn't need forwarding stubs, we could use it for escape analysis, we could autotest/autofuzz such routines, etc. This is where https://golang.org/wiki/TargetSpecific is headed anyway; formalizing it might be useful. |
test/escape_runtime_atomic.go is failing on linux/arm. E.g., https://build.golang.org/log/b9cb939c683aed32bfa9ce69dbf44df63e291507
The text was updated successfully, but these errors were encountered: