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,runtime: avoid multiple returns from deferproc #19466

Open
randall77 opened this issue Mar 8, 2017 · 0 comments
Open

cmd/compile,runtime: avoid multiple returns from deferproc #19466

randall77 opened this issue Mar 8, 2017 · 0 comments
Milestone

Comments

@randall77
Copy link
Contributor

@randall77 randall77 commented Mar 8, 2017

We currently compile a defer to this:

x := deferproc(...)
if x != 0 { goto deferreturn }
...
deferreturn:
  deferreturn()

Normally deferproc returns 0. However, when a subsequent panic is caught and the defer is run by the panic, and that defer recovers, then the runtime arranges the defer to return 1 at the same deferproc callsite. The generated code then immediately jumps to deferreturn().

We should just make a recover'd panic return to the deferreturn callsite. That may involve passing the pc of the deferreturn call to deferproc.

It would mean stack traces during panic point to the end of the function instead of to the site of the defer call. That's arguably more correct (and matches the behavior on non-panic running of defers).

Part of the reason for this fix is that the code past deferproc() but before the branch gets executed twice in a defer/panic/recover scenario. The SSA backend has been known to insert non-idempotent code in spots like that (See #19201. That was for selectrecv not deferproc, but the same issue applies.)

The generated code may have originally been written this way because there was a push/pop sequence around the deferproc call, and the pops were required on the second return as well. Those push/pop sequences were removed a long time ago.

@rsc @mdempsky

See #19331

@randall77 randall77 added this to the Go1.9Maybe milestone Mar 8, 2017
@bradfitz bradfitz removed this from the Go1.9Maybe milestone Jul 20, 2017
@bradfitz bradfitz added this to the Go1.10 milestone Jul 20, 2017
@bradfitz bradfitz added this to the Go1.10 milestone Jul 20, 2017
@bradfitz bradfitz removed this from the Go1.9Maybe milestone Jul 20, 2017
@rsc rsc removed this from the Go1.10 milestone Nov 22, 2017
@rsc rsc added this to the Go1.11 milestone Nov 22, 2017
@gopherbot gopherbot removed this from the Go1.11 milestone May 23, 2018
@gopherbot gopherbot added this to the Unplanned milestone May 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants