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: mid-stack inlining in a method wrapper can cause recover to fail #23557

Closed
aclements opened this issue Jan 25, 2018 · 5 comments

Comments

Projects
None yet
4 participants
@aclements
Copy link
Member

commented Jan 25, 2018

If a method is inlined into a compiler-generated wrapper, calls to recover from that method will return nil even if they should return a panic value. This is only possible with mid-stack inlining enabled because otherwise the call to recover inhibits inlining. For example:

package main

type T struct{}

func (T) M() {
	x := recover()
	if x == nil {
		panic("recover failed")
	}
}

func main() {
	m := (*T).M // Compiler-generated wrapper
	defer m(&T{})
	panic("test panic")
}

When built without -l=4, the deferred method successfully recovers the panic and the program exits silently. When built with -l=4, T.M gets inlined into the generated wrapper (*T).M. However, since (*T).M is marked with the WRAPPER flag, obj generates prologue code that adjusts the gp.panic.argp to point to where the arguments would be for a call from the wrapper. But because T.M was inlined, it doesn't make that call and recover sees the wrapper's own argp, which doesn't match the warpper-adjusted gp.panic.argp. Hence, recover thinks it was called from the wrong frame and returns nil.

@cherrymui and I discovered this while trying to tease apart the wrapper prologue. We thought of a few possible fixes. One approach would be to teach wrappers and inlining about each other. Inlining could be disabled inside a wrapper. Or, if inlining happens, it could clear the wrapper flag. Another approach would be to handle this in the runtime. The compiler could record the WRAPPER flag in the runtime's funcInfo and runtime.gorecover could walk the stack to find the top-most wrapper's argp (there could be more than one in a row because of reflection). While this makes recover somewhat more complex, it would let us eliminate hundreds of lines of hand-written Progs from all of the obj backends, reducing overall complexity and simpifying a very subtle interaction. It also wouldn't hurt the performance of typical recover uses.

/cc @randall77 @rsc

@aclements aclements added this to the Go1.11 milestone Jan 25, 2018

@aclements

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2018

@rsc points out that recover needs to block inlining of its caller, even if wrappers aren't involved.

@gopherbot

This comment has been minimized.

Copy link

commented Jan 25, 2018

Change https://golang.org/cl/90035 mentions this issue: cmd/compile: don't inline functions that call recover

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2018

For what it's worth, I solved the similar problem in gccgo by marking these thunks, and indeed every function that calls recover, as not inlinable.

@gopherbot gopherbot closed this in b5b35be Jan 25, 2018

@odeke-em

This comment has been minimized.

Copy link
Member

commented Jan 26, 2018

/cc @khr @rsc

@aclements I believe Keith's Github handle is @randall77 :) so he probably missed this tag.

@aclements

This comment has been minimized.

Copy link
Member Author

commented Jan 26, 2018

Thanks @odeke-em :)

@golang golang locked and limited conversation to collaborators Jan 26, 2019

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