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: //go:uintptrescapes doesn't work with anonymous parameters #23045

Closed
mdempsky opened this issue Dec 8, 2017 · 4 comments
Closed
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@mdempsky
Copy link
Contributor

mdempsky commented Dec 8, 2017

If you remove the a parameter name from F1 and F2 in uintptrescapes2.go (i.e., change them to func F1(uintptr) {} and func F2(...uintptr) {}), the test fails.

I suspect in practice this particular issue doesn't matter in practice, but looking at the code, I can't obviously see why the parameter names should be relevant in either (*EscState).esctag or ordercall. It makes me worry that escape analysis / pragma tags might not be working correctly in some other more realistic scenarios.

/cc @ianlancetaylor

@mdempsky mdempsky added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 8, 2017
@mdempsky mdempsky added this to the Go1.11 milestone Dec 8, 2017
@cherrymui
Copy link
Member

In esctag, the escape tag is overwritten by the loop at the end which is for special handling of unnamed parameters, https://go.googlesource.com/go/+/fdf3ceafc61c0cf9b7b7bd6757f0c51393a7a83c/src/cmd/compile/internal/gc/esc.go#2262.

Making that loop not overwrite those tags fixes the problem. CL coming.

@cherrymui cherrymui self-assigned this Dec 8, 2017
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/82775 mentions this issue: cmd/compile: fix go:uintptrescapes tag for unnamed parameters

@mdempsky
Copy link
Contributor Author

mdempsky commented Dec 8, 2017

@cherrymui Thanks for pointing that out. That settles my worries.

Perhaps this is working as intended then. Since //go:uintptrescapes only applies to functions implemented in Go (as opposed to assembly), and you can't refer to anonymous parameters, there's no need for their arguments to escape or be kept alive?

@ianlancetaylor
Copy link
Member

//go:uintptrescapes really only exists as a patch for syscall.(*Proc).Call and syscall.(*LazyProc).Call to permit them to take uintptr arguments without breaking the Go 1 guarantee. The uintptrescapes directive is not documented and it's not especially important how it is implemented as long as those two methods (and the corresponding ones in golang.org/x/sys/windows) continue to work correctly. See #16035.

That said, theoretically, I think it shouldn't matter whether the parameter is anonymous or not. Either way the object should remain live and should not move. I can imagine cases where one parameter would be the address of a field of another parameter, and that second parameter should remain live even if it happens to be unnamed.

@golang golang locked and limited conversation to collaborators Feb 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants