From e97209515ad8c4042f5a3ef32068200366892fc2 Mon Sep 17 00:00:00 2001 From: Austin Clements Date: Mon, 12 Jun 2017 11:12:12 -0400 Subject: [PATCH] runtime: hide methods from call stack The compiler generates wrapper methods to forward interface method calls (which are always pointer-based) to value methods. These wrappers appear in the call stack even though they are an implementation detail. This leaves ugly "" functions in stack traces and can throw off skip counts for stack traces. Fix this by considering these runtime frames in printed stack traces so they will only be printed if runtime frames are being printed, and by eliding them from the call stack expansion used by CallersFrames and Caller. This removes the test for issue 4388 since that was checking that "" appeared in the stack trace instead of something even weirder. We replace it with various runtime package tests. Fixes #16723. Change-Id: Ice3f118c66f254bb71478a664d62ab3fc7125819 Reviewed-on: https://go-review.googlesource.com/45412 Run-TryBot: Austin Clements TryBot-Result: Gobot Gobot Reviewed-by: Ian Lance Taylor --- src/runtime/stack_test.go | 58 +++++++++++++++++++++++++++++++++++++ src/runtime/symtab.go | 8 +++++ src/runtime/traceback.go | 8 ++++- test/fixedbugs/issue4388.go | 56 ----------------------------------- 4 files changed, 73 insertions(+), 57 deletions(-) delete mode 100644 test/fixedbugs/issue4388.go diff --git a/src/runtime/stack_test.go b/src/runtime/stack_test.go index 25e8f77da4..c9b84be066 100644 --- a/src/runtime/stack_test.go +++ b/src/runtime/stack_test.go @@ -5,6 +5,7 @@ package runtime_test import ( + "fmt" . "runtime" "strings" "sync" @@ -627,3 +628,60 @@ func count23(n int) int { } return 1 + count1(n-1) } + +type structWithMethod struct{} + +func (s structWithMethod) caller() string { + _, file, line, ok := Caller(1) + if !ok { + panic("Caller failed") + } + return fmt.Sprintf("%s:%d", file, line) +} + +func (s structWithMethod) callers() []uintptr { + pc := make([]uintptr, 16) + return pc[:Callers(0, pc)] +} + +func (s structWithMethod) stack() string { + buf := make([]byte, 4<<10) + return string(buf[:Stack(buf, false)]) +} + +func TestStackWrapperCaller(t *testing.T) { + var d structWithMethod + // Force the compiler to construct a wrapper method. + wrapper := (*structWithMethod).caller + // Check that the wrapper doesn't affect the stack trace. + if dc, ic := d.caller(), wrapper(&d); dc != ic { + t.Fatalf("direct caller %q != indirect caller %q", dc, ic) + } +} + +func TestStackWrapperCallers(t *testing.T) { + var d structWithMethod + wrapper := (*structWithMethod).callers + // Check that doesn't appear in the stack trace. + pcs := wrapper(&d) + frames := CallersFrames(pcs) + for { + fr, more := frames.Next() + if fr.File == "" { + t.Fatalf(" appears in stack trace: %+v", fr) + } + if !more { + break + } + } +} + +func TestStackWrapperStack(t *testing.T) { + var d structWithMethod + wrapper := (*structWithMethod).stack + // Check that doesn't appear in the stack trace. + stk := wrapper(&d) + if strings.Contains(stk, "") { + t.Fatalf(" appears in stack trace:\n%s", stk) + } +} diff --git a/src/runtime/symtab.go b/src/runtime/symtab.go index 7e001c96b1..0324fb7a1c 100644 --- a/src/runtime/symtab.go +++ b/src/runtime/symtab.go @@ -118,6 +118,7 @@ func (ci *Frames) Next() (frame Frame, more bool) { func (se *stackExpander) next(callers []uintptr) (ncallers []uintptr, frame Frame, more bool) { ncallers = callers +again: if !se.pcExpander.more { // Expand the next PC. if len(ncallers) == 0 { @@ -144,6 +145,13 @@ func (se *stackExpander) next(callers []uintptr) (ncallers []uintptr, frame Fram } frame = se.pcExpander.next() + if frame.File == "" { + // Ignore autogenerated functions such as pointer + // method forwarding functions. These are an + // implementation detail that doesn't reflect the + // source code. + goto again + } return ncallers, frame, se.pcExpander.more || len(ncallers) > 0 } diff --git a/src/runtime/traceback.go b/src/runtime/traceback.go index abd70e1581..2282b2d5c0 100644 --- a/src/runtime/traceback.go +++ b/src/runtime/traceback.go @@ -733,7 +733,13 @@ func showframe(f funcInfo, gp *g, firstFrame bool) bool { return true } level, _, _ := gotraceback() + if level > 1 { + // Show all frames. + return true + } + 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 @@ -744,7 +750,7 @@ func showframe(f funcInfo, gp *g, firstFrame bool) bool { return true } - return level > 1 || f.valid() && contains(name, ".") && (!hasprefix(name, "runtime.") || isExportedRuntime(name)) + return f.valid() && contains(name, ".") && (!hasprefix(name, "runtime.") || isExportedRuntime(name)) && file != "" } // isExportedRuntime reports whether name is an exported runtime function. diff --git a/test/fixedbugs/issue4388.go b/test/fixedbugs/issue4388.go deleted file mode 100644 index 5bb05eb404..0000000000 --- a/test/fixedbugs/issue4388.go +++ /dev/null @@ -1,56 +0,0 @@ -// 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. - -package main - -import ( - "fmt" - "io" - "runtime" -) - -type T struct { - io.Closer -} - -func f1() { - // The 5 here and below depends on the number of internal runtime frames - // that sit between a deferred function called during panic and - // the original frame. If that changes, this test will start failing and - // the number here will need to be updated. - defer checkLine(5) - var t *T - var c io.Closer = t - c.Close() -} - -func f2() { - defer checkLine(5) - var t T - var c io.Closer = t - c.Close() -} - -func main() { - f1() - f2() -} - -func checkLine(n int) { - if err := recover(); err == nil { - panic("did not panic") - } - var file string - var line int - for i := 1; i <= n; i++ { - _, file, line, _ = runtime.Caller(i) - if file != "" || line != 1 { - continue - } - return - } - panic(fmt.Sprintf("expected :1 have %s:%d", file, line)) -} -- 2.50.0