]> Cypherpunks.ru repositories - gostls13.git/commitdiff
runtime: don't elide wrapper functions that call panic or at TOS
authorAustin Clements <austin@google.com>
Thu, 9 Nov 2017 22:55:45 +0000 (17:55 -0500)
committerAustin Clements <austin@google.com>
Mon, 13 Nov 2017 21:43:44 +0000 (21:43 +0000)
CL 45412 started hiding autogenerated wrapper functions from call
stacks so that call stack semantics better matched language semantics.
This is based on the theory that the wrapper function will call the
"real" function and all the programmer knows about is the real
function.

However, this theory breaks down in two cases:

1. If the wrapper is at the top of the stack, then it didn't call
   anything. This can happen, for example, if the "stack" was actually
   synthesized by the user.

2. If the wrapper panics, for example by calling panicwrap or by
   dereferencing a nil pointer, then it didn't call the wrapped
   function and the user needs to see what panicked, even if we can't
   attribute it nicely.

This commit modifies the traceback logic to include the wrapper
function in both of these cases.

Fixes #22231.

Change-Id: I6e4339a652f73038bd8331884320f0b8edd86eb1
Reviewed-on: https://go-review.googlesource.com/76770
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
src/runtime/extern.go
src/runtime/stack_test.go
src/runtime/symtab.go
src/runtime/traceback.go

index 66b952780234c074b4e91e09fe1226df225c0ec3..2c20e0d8afd3c98395d4be7c27647f6deab8fc77 100644 (file)
@@ -178,11 +178,11 @@ func Caller(skip int) (pc uintptr, file string, line int, ok bool) {
        // We asked for one extra, so skip that one. If this is sigpanic,
        // stepping over this frame will set up state in Frames so the
        // next frame is correct.
-       callers, _, ok = stackExpander.next(callers)
+       callers, _, ok = stackExpander.next(callers, true)
        if !ok {
                return
        }
-       _, frame, _ := stackExpander.next(callers)
+       _, frame, _ := stackExpander.next(callers, true)
        pc = frame.PC
        file = frame.File
        line = frame.Line
index 8e7c7d47a8b0aade2bc160dc40eed7747bd2d3b8..cb0e08256b8323c73b5c13bc30393590422be783 100644 (file)
@@ -7,6 +7,7 @@ package runtime_test
 import (
        "bytes"
        "fmt"
+       "reflect"
        . "runtime"
        "strings"
        "sync"
@@ -650,6 +651,8 @@ func (s structWithMethod) stack() string {
        return string(buf[:Stack(buf, false)])
 }
 
+func (s structWithMethod) nop() {}
+
 func TestStackWrapperCaller(t *testing.T) {
        var d structWithMethod
        // Force the compiler to construct a wrapper method.
@@ -687,6 +690,81 @@ func TestStackWrapperStack(t *testing.T) {
        }
 }
 
+type I interface {
+       M()
+}
+
+func TestStackWrapperStackPanic(t *testing.T) {
+       t.Run("sigpanic", func(t *testing.T) {
+               // nil calls to interface methods cause a sigpanic.
+               testStackWrapperPanic(t, func() { I.M(nil) }, "runtime_test.I.M")
+       })
+       t.Run("panicwrap", func(t *testing.T) {
+               // Nil calls to value method wrappers call panicwrap.
+               wrapper := (*structWithMethod).nop
+               testStackWrapperPanic(t, func() { wrapper(nil) }, "runtime_test.(*structWithMethod).nop")
+       })
+}
+
+func testStackWrapperPanic(t *testing.T, cb func(), expect string) {
+       // Test that the stack trace from a panicking wrapper includes
+       // the wrapper, even though elide these when they don't panic.
+       t.Run("CallersFrames", func(t *testing.T) {
+               defer func() {
+                       err := recover()
+                       if err == nil {
+                               t.Fatalf("expected panic")
+                       }
+                       pcs := make([]uintptr, 10)
+                       n := Callers(0, pcs)
+                       frames := CallersFrames(pcs[:n])
+                       for {
+                               frame, more := frames.Next()
+                               t.Log(frame.Function)
+                               if frame.Function == expect {
+                                       return
+                               }
+                               if !more {
+                                       break
+                               }
+                       }
+                       t.Fatalf("panicking wrapper %s missing from stack trace", expect)
+               }()
+               cb()
+       })
+       t.Run("Stack", func(t *testing.T) {
+               defer func() {
+                       err := recover()
+                       if err == nil {
+                               t.Fatalf("expected panic")
+                       }
+                       buf := make([]byte, 4<<10)
+                       stk := string(buf[:Stack(buf, false)])
+                       if !strings.Contains(stk, "\n"+expect) {
+                               t.Fatalf("panicking wrapper %s missing from stack trace:\n%s", expect, stk)
+                       }
+               }()
+               cb()
+       })
+}
+
+func TestCallersFromWrapper(t *testing.T) {
+       // Test that invoking CallersFrames on a stack where the first
+       // PC is an autogenerated wrapper keeps the wrapper in the
+       // trace. Normally we elide these, assuming that the wrapper
+       // calls the thing you actually wanted to see, but in this
+       // case we need to keep it.
+       pc := reflect.ValueOf(I.M).Pointer()
+       frames := CallersFrames([]uintptr{pc})
+       frame, more := frames.Next()
+       if frame.Function != "runtime_test.I.M" {
+               t.Fatalf("want function %s, got %s", "runtime_test.I.M", frame.Function)
+       }
+       if more {
+               t.Fatalf("want 1 frame, got > 1")
+       }
+}
+
 func TestTracebackSystemstack(t *testing.T) {
        if GOARCH == "ppc64" || GOARCH == "ppc64le" {
                t.Skip("systemstack tail call not implemented on ppc64x")
index 135fc1a7ad009d5c36f5697b0b5870f23f45bae6..c6c2b89f69aa6825e0a3e829259a92027e22b9ad 100644 (file)
@@ -19,6 +19,11 @@ type Frames struct {
        // stackExpander expands callers into a sequence of Frames,
        // tracking the necessary state across PCs.
        stackExpander stackExpander
+
+       // elideWrapper indicates that, if the next frame is an
+       // autogenerated wrapper function, it should be elided from
+       // the stack.
+       elideWrapper bool
 }
 
 // Frame is the information returned by Frames for each call frame.
@@ -112,11 +117,12 @@ func (se *stackExpander) init(callers []uintptr) []uintptr {
 // Next returns frame information for the next caller.
 // If more is false, there are no more callers (the Frame value is valid).
 func (ci *Frames) Next() (frame Frame, more bool) {
-       ci.callers, frame, more = ci.stackExpander.next(ci.callers)
+       ci.callers, frame, more = ci.stackExpander.next(ci.callers, ci.elideWrapper)
+       ci.elideWrapper = elideWrapperCalling(frame.Function)
        return
 }
 
-func (se *stackExpander) next(callers []uintptr) (ncallers []uintptr, frame Frame, more bool) {
+func (se *stackExpander) next(callers []uintptr, elideWrapper bool) (ncallers []uintptr, frame Frame, more bool) {
        ncallers = callers
 again:
        if !se.pcExpander.more {
@@ -145,7 +151,7 @@ again:
        }
 
        frame = se.pcExpander.next()
-       if frame.File == "<autogenerated>" {
+       if elideWrapper && frame.File == "<autogenerated>" {
                // Ignore autogenerated functions such as pointer
                // method forwarding functions. These are an
                // implementation detail that doesn't reflect the
index 2282b2d5c0e9e2e2d49bd7770a9866727bca1e23..501ecb0411cf0887065ae438f0e6c99fe7dc02ed 100644 (file)
@@ -184,6 +184,7 @@ func gentraceback(pc0, sp0, lr0 uintptr, gp *g, skip int, pcbuf *uintptr, max in
        cgoCtxt := gp.cgoCtxt
        printing := pcbuf == nil && callback == nil
        _defer := gp._defer
+       elideWrapper := false
 
        for _defer != nil && _defer.sp == _NoArgs {
                _defer = _defer.link
@@ -386,8 +387,15 @@ func gentraceback(pc0, sp0, lr0 uintptr, gp *g, skip int, pcbuf *uintptr, max in
                }
 
                if printing {
-                       // assume skip=0 for printing
-                       if (flags&_TraceRuntimeFrames) != 0 || showframe(f, gp, nprint == 0) {
+                       // assume skip=0 for printing.
+                       //
+                       // Never elide wrappers if we haven't printed
+                       // any frames. And don't elide wrappers that
+                       // called panic rather than the wrapped
+                       // function. Otherwise, leave them out.
+                       name := funcname(f)
+                       nextElideWrapper := elideWrapperCalling(name)
+                       if (flags&_TraceRuntimeFrames) != 0 || showframe(f, gp, nprint == 0, elideWrapper && nprint != 0) {
                                // Print during crash.
                                //      main(0x1, 0x2, 0x3)
                                //              /home/rsc/go/src/runtime/x.go:23 +0xf
@@ -411,7 +419,6 @@ func gentraceback(pc0, sp0, lr0 uintptr, gp *g, skip int, pcbuf *uintptr, max in
                                                ix = inltree[ix].parent
                                        }
                                }
-                               name := funcname(f)
                                if name == "runtime.gopanic" {
                                        name = "panic"
                                }
@@ -438,6 +445,7 @@ func gentraceback(pc0, sp0, lr0 uintptr, gp *g, skip int, pcbuf *uintptr, max in
                                print("\n")
                                nprint++
                        }
+                       elideWrapper = nextElideWrapper
                }
                n++
 
@@ -647,7 +655,7 @@ func printcreatedby(gp *g) {
        // Show what created goroutine, except main goroutine (goid 1).
        pc := gp.gopc
        f := findfunc(pc)
-       if f.valid() && showframe(f, gp, false) && gp.goid != 1 {
+       if f.valid() && showframe(f, gp, false, false) && gp.goid != 1 {
                print("created by ", funcname(f), "\n")
                tracepc := pc // back up to CALL instruction for funcline.
                if pc > f.entry {
@@ -727,7 +735,7 @@ func gcallers(gp *g, skip int, pcbuf []uintptr) int {
        return gentraceback(^uintptr(0), ^uintptr(0), 0, gp, skip, &pcbuf[0], len(pcbuf), nil, nil, 0)
 }
 
-func showframe(f funcInfo, gp *g, firstFrame bool) bool {
+func showframe(f funcInfo, gp *g, firstFrame, elideWrapper bool) bool {
        g := getg()
        if g.m.throwing > 0 && gp != nil && (gp == g.m.curg || gp == g.m.caughtsig.ptr()) {
                return true
@@ -738,8 +746,18 @@ func showframe(f funcInfo, gp *g, firstFrame bool) bool {
                return true
        }
 
+       if !f.valid() {
+               return false
+       }
+
+       if elideWrapper {
+               file, _ := funcline(f, f.entry)
+               if file == "<autogenerated>" {
+                       return false
+               }
+       }
+
        name := funcname(f)
-       file, _ := funcline(f, f.entry)
 
        // Special case: always show runtime.gopanic frame
        // in the middle of a stack trace, so that we can
@@ -750,7 +768,7 @@ func showframe(f funcInfo, gp *g, firstFrame bool) bool {
                return true
        }
 
-       return f.valid() && contains(name, ".") && (!hasprefix(name, "runtime.") || isExportedRuntime(name)) && file != "<autogenerated>"
+       return contains(name, ".") && (!hasprefix(name, "runtime.") || isExportedRuntime(name))
 }
 
 // isExportedRuntime reports whether name is an exported runtime function.
@@ -760,6 +778,14 @@ func isExportedRuntime(name string) bool {
        return len(name) > n && name[:n] == "runtime." && 'A' <= name[n] && name[n] <= 'Z'
 }
 
+// elideWrapperCalling returns whether a wrapper function that called
+// function "name" should be elided from stack traces.
+func elideWrapperCalling(name string) bool {
+       // If the wrapper called a panic function instead of the
+       // wrapped function, we want to include it in stacks.
+       return !(name == "runtime.gopanic" || name == "runtime.sigpanic" || name == "runtime.panicwrap")
+}
+
 var gStatusStrings = [...]string{
        _Gidle:      "idle",
        _Grunnable:  "runnable",