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

Comments

Projects
None yet
5 participants
@mdempsky
Copy link
Member

commented Apr 17, 2019

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

@bradfitz bradfitz added the NeedsFix label Apr 17, 2019

@bradfitz bradfitz added this to the Go1.13 milestone Apr 17, 2019

@mdempsky

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

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

@mdempsky

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

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

This comment has been minimized.

Copy link

commented Apr 17, 2019

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

@mdempsky

This comment has been minimized.

Copy link
Member Author

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

runtime/internal/atomic: remove bad go:noescape annotations on Loadp
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

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

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

This comment has been minimized.

Copy link

commented Apr 17, 2019

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

test: fix escape_runtime_atomic.go
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

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.