golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
121.21k stars 17.37k forks source link

cmd/compile: more fine-grained mechanism for escape analysis of assembly functions #31525

Open mdempsky opened 5 years ago

mdempsky commented 5 years ago

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

mdempsky commented 5 years ago

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

mdempsky commented 5 years ago

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 commented 5 years ago

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

mdempsky commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

randall77 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

gopherbot commented 1 month ago

Change https://go.dev/cl/586875 mentions this issue: sync/atomic: call internal/runtime/atomic function directly