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: more fine-grained mechanism for escape analysis of assembly functions #31525

Open
mdempsky opened this issue Apr 17, 2019 · 9 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@mdempsky
Copy link
Member

test/escape_runtime_atomic.go is failing on linux/arm. E.g., https://build.golang.org/log/b9cb939c683aed32bfa9ce69dbf44df63e291507

@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Apr 17, 2019
@bradfitz bradfitz added this to the Go1.13 milestone Apr 17, 2019
@mdempsky
Copy link
Member Author

Looks like arm64, ppc64, and ppc64le are broken too.

@mdempsky
Copy link
Member Author

The problem is all of these functions implement Loadp in assembly and use stubs like:

//go:noescape
func Loadp(ptr unsafe.Pointer) unsafe.Pointer 

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.

@gopherbot
Copy link

Change https://golang.org/cl/172578 mentions this issue: runtime: remove bad go:noescape annotations on atomic.Loadp

@mdempsky
Copy link
Member Author

mdempsky commented Apr 17, 2019

Related, in package reflect:

// m escapes into the return value, but the caller of mapiterinit
// doesn't let the return value escape.
//go:noescape
func mapiterinit(t *rtype, m unsafe.Pointer) unsafe.Pointer

Edit: Actually there's a bunch of technically incorrect go:noescape annotations.

@mdempsky mdempsky changed the title runtime/internal/atomic: bad escape analysis of Loadp on linux/arm cmd/compile: more fine-grained mechanism for escape analysis of assembly functions Apr 17, 2019
gopherbot pushed a commit that referenced this issue Apr 17, 2019
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>
@mdempsky
Copy link
Member Author

Repurposing this issue for tracking the need for a more fine-grained control of escape analysis of external functions. Right now we have //go:noescape (arguments don't flow anywhere outside of the function call) and no directive (arguments flow directly to heap), but there's a lot of useful middle ground.

@gopherbot
Copy link

Change https://golang.org/cl/172602 mentions this issue: test: fix escape_runtime_atomic.go

gopherbot pushed a commit that referenced this issue Apr 17, 2019
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>
@randall77
Copy link
Contributor

Can't we make go:noescape mean that nothing escapes to the heap, but may escape to the results? I can't think of any reason that wouldn't cover all current uses.

Or are there too many distinctions to make even then (arg->res, *arg->res, **arg->res, etc.)?

@mdempsky
Copy link
Member Author

Can't we make go:noescape mean that nothing escapes to the heap, but may escape to the results? I can't think of any reason that wouldn't cover all current uses.

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:

 //go:pseudocode
 func LoadPointer(ptr *unsafe.Pointer) unsafe.Pointer { return *ptr }

 //go:pseudocode
 func StorePointer(ptr *unsafe.Pointer, new unsafe.Pointer) { *ptr = new }

 //go:pseudocode
 func CompareAndSwapPointer(ptr *unsafe.Pointer, old, new unsafe.Pointer) bool {
     ok := *ptr == old
     if ok {
          *ptr = new
     }
     return ok
 }

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

@josharian
Copy link
Contributor

//go:pseudocode is a nice idea.

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.

@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Projects
Status: Triage Backlog
Development

No branches or pull requests

7 participants