From 54357b3b84538e0f26b6501fd91bb98170995ff5 Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Fri, 23 Jun 2017 13:45:36 +0000 Subject: [PATCH] runtime: improve handling of panic during deferred function When a panic occurs while processing a deferred function that recovered an earlier panic, we shouldn't report the recovered panic in the panic stack trace. Stop doing so by keeping track of the panic that triggered a defer, marking it as aborted if we see the defer again, and discarding aborted panics when a panic is recovered. This is what the gc runtime does. The test for this is TestRecursivePanic in runtime/crash_test.go. We don't run that test yet, but we will soon. Reviewed-on: https://go-review.googlesource.com/46461 From-SVN: r249590 --- gcc/go/gofrontend/MERGE | 2 +- libgo/go/runtime/panic.go | 108 +++++++++++++++++++++++++---------- libgo/go/runtime/runtime2.go | 8 +++ 3 files changed, 86 insertions(+), 32 deletions(-) diff --git a/gcc/go/gofrontend/MERGE b/gcc/go/gofrontend/MERGE index b6037a68150..d82e012767c 100644 --- a/gcc/go/gofrontend/MERGE +++ b/gcc/go/gofrontend/MERGE @@ -1,4 +1,4 @@ -385efb8947af70b8425c833a1ab68ba5f357dfae +c4adba240f9d5af8ab0534316d6b05bd988c432c The first line of this file holds the git revision number of the last merge done from the gofrontend repository. diff --git a/libgo/go/runtime/panic.go b/libgo/go/runtime/panic.go index de3c79fe6bf..e3d03580de5 100644 --- a/libgo/go/runtime/panic.go +++ b/libgo/go/runtime/panic.go @@ -91,6 +91,9 @@ func throwinit() { // arg is a value to pass to pfn. func deferproc(frame *bool, pfn uintptr, arg unsafe.Pointer) { d := newdefer() + if d._panic != nil { + throw("deferproc: d.panic != nil after newdefer") + } d.frame = frame d.panicStack = getg()._panic d.pfn = pfn @@ -338,17 +341,28 @@ func Goexit() { if d == nil { break } - gp._defer = d.link pfn := d.pfn + if pfn == 0 { + if d._panic != nil { + d._panic.aborted = true + d._panic = nil + } + gp._defer = d.link + freedefer(d) + continue + } d.pfn = 0 - if pfn != 0 { - var fn func(unsafe.Pointer) - *(*uintptr)(unsafe.Pointer(&fn)) = uintptr(unsafe.Pointer(&pfn)) - fn(d.arg) - } + var fn func(unsafe.Pointer) + *(*uintptr)(unsafe.Pointer(&fn)) = uintptr(unsafe.Pointer(&pfn)) + fn(d.arg) + if gp._defer != d { + throw("bad defer entry in Goexit") + } + d._panic = nil + gp._defer = d.link freedefer(d) // Note: we ignore recovers here because Goexit isn't a panic } @@ -442,39 +456,71 @@ func gopanic(e interface{}) { } pfn := d.pfn + + // If defer was started by earlier panic or Goexit (and, since we're back here, that triggered a new panic), + // take defer off list. The earlier panic or Goexit will not continue running. + if pfn == 0 { + if d._panic != nil { + d._panic.aborted = true + } + d._panic = nil + gp._defer = d.link + freedefer(d) + continue + } d.pfn = 0 - if pfn != 0 { - var fn func(unsafe.Pointer) - *(*uintptr)(unsafe.Pointer(&fn)) = uintptr(unsafe.Pointer(&pfn)) - fn(d.arg) + // Record the panic that is running the defer. + // If there is a new panic during the deferred call, that panic + // will find d in the list and will mark d._panic (this panic) aborted. + d._panic = p - if p.recovered { - // Some deferred function called recover. - // Stop running this panic. - gp._panic = p.link - - // Unwind the stack by throwing an exception. - // The compiler has arranged to create - // exception handlers in each function - // that uses a defer statement. These - // exception handlers will check whether - // the entry on the top of the defer stack - // is from the current function. If it is, - // we have unwound the stack far enough. - unwindStack() - - throw("unwindStack returned") + var fn func(unsafe.Pointer) + *(*uintptr)(unsafe.Pointer(&fn)) = uintptr(unsafe.Pointer(&pfn)) + fn(d.arg) + + if gp._defer != d { + throw("bad defer entry in panic") + } + d._panic = nil + + if p.recovered { + // Some deferred function called recover. + // Stop running this panic. + gp._panic = p.link + + // Aborted panics are marked but remain on the g.panic list. + // Remove them from the list. + for gp._panic != nil && gp._panic.aborted { + gp._panic = gp._panic.link + } + if gp._panic == nil { // must be done with signal + gp.sig = 0 } - // Because we executed that defer function by a panic, - // and it did not call recover, we know that we are - // not returning from the calling function--we are - // panicking through it. - *d.frame = false + // Unwind the stack by throwing an exception. + // The compiler has arranged to create + // exception handlers in each function + // that uses a defer statement. These + // exception handlers will check whether + // the entry on the top of the defer stack + // is from the current function. If it is, + // we have unwound the stack far enough. + unwindStack() + + throw("unwindStack returned") } + // Because we executed that defer function by a panic, + // and it did not call recover, we know that we are + // not returning from the calling function--we are + // panicking through it. + *d.frame = false + + // Deferred function did not panic. Remove d. + // In the p.recovered case, d will be removed by checkdefer. gp._defer = d.link + freedefer(d) } diff --git a/libgo/go/runtime/runtime2.go b/libgo/go/runtime/runtime2.go index 96a4edb83d4..cdd3fcc7911 100644 --- a/libgo/go/runtime/runtime2.go +++ b/libgo/go/runtime/runtime2.go @@ -700,6 +700,10 @@ type _defer struct { // has a defer statement itself. panicStack *_panic + // The panic that caused the defer to run. This is used to + // discard panics that have already been handled. + _panic *_panic + // The function to call. pfn uintptr @@ -733,6 +737,10 @@ type _panic struct { // Whether this panic was pushed on the stack because of an // exception thrown in some other language. isforeign bool + + // Whether this panic was already seen by a deferred function + // which called panic again. + aborted bool } const ( -- 2.30.2