runtime: fix correctness test at end of traceback

We were requiring that the defer stack and the panic stack
be completely processed, thinking that if any were left over
the stack scan and the defer stack/panic stack must be out
of sync. It turns out that the panic stack may well have
leftover entries in some situations, and that's okay.

Fixes #8132.

LGTM=minux, r
R=golang-codereviews, minux, r
CC=golang-codereviews, iant, khr
https://golang.org/cl/100900044

diff --git a/src/pkg/runtime/traceback_arm.c b/src/pkg/runtime/traceback_arm.c
index 8acd143a5c764864aa84cbf35376ff160d9eb5a3..8d1fc542663e07564761fe1c43cf1e0acdd72818 100644 (file)
--- a/src/pkg/runtime/traceback_arm.c
+++ b/src/pkg/runtime/traceback_arm.c
@@ -261,11 +261,20 @@ runtime·gentraceback(uintptr pc0, uintptr sp0, uintptr lr0, G *gp, int32 skip,
        if(pcbuf == nil && callback == nil)
                n = nprint;
 
-       if(callback != nil && n < max && (defer != nil || panic != nil && panic->defer != nil)) {
+       // For rationale, see long comment in traceback_x86.c.
+       if(callback != nil && n < max && defer != nil) {
                if(defer != nil)
                        runtime·printf("runtime: g%D: leftover defer argp=%p pc=%p\n", gp->goid, defer->argp, defer->pc);
-               if(panic != nil && panic->defer != nil)
+               if(panic != nil)
                        runtime·printf("runtime: g%D: leftover panic argp=%p pc=%p\n", gp->goid, panic->defer->argp, panic->defer->pc);
+               for(defer = gp->defer; defer != nil; defer = defer->link)
+                       runtime·printf("\tdefer %p argp=%p pc=%p\n", defer, defer->argp, defer->pc);
+               for(panic = gp->panic; panic != nil; panic = panic->link) {
+                       runtime·printf("\tpanic %p defer %p", panic, panic->defer);
+                       if(panic->defer != nil)
+                               runtime·printf(" argp=%p pc=%p", panic->defer->argp, panic->defer->pc);
+                       runtime·printf("\n");
+               }
                runtime·throw("traceback has leftover defers or panics");
        }
 

diff --git a/src/pkg/runtime/traceback_x86.c b/src/pkg/runtime/traceback_x86.c
index 0ecaa0fd77756f5d47b01b14ca60877f5b88f8d1..851504f529e21ee3d3156847f73583eb615efd93 100644 (file)
--- a/src/pkg/runtime/traceback_x86.c
+++ b/src/pkg/runtime/traceback_x86.c
@@ -304,11 +304,68 @@ runtime·gentraceback(uintptr pc0, uintptr sp0, uintptr lr0, G *gp, int32 skip,
        if(pcbuf == nil && callback == nil)
                n = nprint;
 
-       if(callback != nil && n < max && (defer != nil || panic != nil)) {
+       // If callback != nil, we're being called to gather stack information during
+       // garbage collection or stack growth. In that context, require that we used
+       // up the entire defer stack. If not, then there is a bug somewhere and the
+       // garbage collection or stack growth may not have seen the correct picture
+       // of the stack. Crash now instead of silently executing the garbage collection
+       // or stack copy incorrectly and setting up for a mysterious crash later.
+       //
+       // Note that panic != nil is okay here: there can be leftover panics,
+       // because the defers on the panic stack do not nest in frame order as
+       // they do on the defer stack. If you have:
+       //
+       //      frame 1 defers d1
+       //      frame 2 defers d2
+       //      frame 3 defers d3
+       //      frame 4 panics
+       //      frame 4's panic starts running defers
+       //      frame 5, running d3, defers d4
+       //      frame 5 panics
+       //      frame 5's panic starts running defers
+       //      frame 6, running d4, garbage collects
+       //      frame 6, running d2, garbage collects
+       //
+       // During the execution of d4, the panic stack is d4 -> d3, which
+       // is nested properly, and we'll treat frame 3 as resumable, because we
+       // can find d3. (And in fact frame 3 is resumable. If d4 recovers
+       // and frame 5 continues running, d3, d3 can recover and we'll
+       // resume execution in (returning from) frame 3.)
+       //
+       // During the execution of d2, however, the panic stack is d2 -> d3,
+       // which is inverted. The scan will match d2 to frame 2 but having
+       // d2 on the stack until then means it will not match d3 to frame 3.
+       // This is okay: if we're running d2, then all the defers after d2 have
+       // completed and their corresponding frames are dead. Not finding d3
+       // for frame 3 means we'll set frame 3's continpc == 0, which is correct
+       // (frame 3 is dead). At the end of the walk the panic stack can thus
+       // contain defers (d3 in this case) for dead frames. The inversion here
+       // always indicates a dead frame, and the effect of the inversion on the
+       // scan is to hide those dead frames, so the scan is still okay:
+       // what's left on the panic stack are exactly (and only) the dead frames.
+       //
+       // We require callback != nil here because only when callback != nil
+       // do we know that gentraceback is being called in a "must be correct"
+       // context as opposed to a "best effort" context. The tracebacks with
+       // callbacks only happen when everything is stopped nicely.
+       // At other times, such as when gathering a stack for a profiling signal
+       // or when printing a traceback during a crash, everything may not be
+       // stopped nicely, and the stack walk may not be able to complete.
+       // It's okay in those situations not to use up the entire defer stack:
+       // incomplete information then is still better than nothing.
+       if(callback != nil && n < max && defer != nil) {
                if(defer != nil)
                        runtime·printf("runtime: g%D: leftover defer argp=%p pc=%p\n", gp->goid, defer->argp, defer->pc);
                if(panic != nil)
                        runtime·printf("runtime: g%D: leftover panic argp=%p pc=%p\n", gp->goid, panic->defer->argp, panic->defer->pc);
+               for(defer = gp->defer; defer != nil; defer = defer->link)
+                       runtime·printf("\tdefer %p argp=%p pc=%p\n", defer, defer->argp, defer->pc);
+               for(panic = gp->panic; panic != nil; panic = panic->link) {
+                       runtime·printf("\tpanic %p defer %p", panic, panic->defer);
+                       if(panic->defer != nil)
+                               runtime·printf(" argp=%p pc=%p", panic->defer->argp, panic->defer->pc);
+                       runtime·printf("\n");
+               }
                runtime·throw("traceback has leftover defers or panics");
        }
 

diff --git a/test/fixedbugs/issue8132.go b/test/fixedbugs/issue8132.go
new file mode 100644 (file)
index 0000000..52f5d39
--- /dev/null
+++ b/test/fixedbugs/issue8132.go
@@ -0,0 +1,32 @@
+// run
+
+// Copyright 2014 The Go Authors.  All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// issue 8132. stack walk handling of panic stack was confused
+// about what was legal.
+
+package main
+
+import "runtime"
+
+var p *int
+
+func main() {
+       func() {
+               defer func() {
+                       runtime.GC()
+                       recover()
+               }()
+               var x [8192]byte
+               func(x [8192]byte) {
+                       defer func() {
+                               if err := recover(); err != nil {
+                                       println(*p)
+                               }
+                       }()
+                       println(*p)
+               }(x)
+       }()
+}