From ba2adc21e8c416c47dec5fbce76286758f15b177 Mon Sep 17 00:00:00 2001 From: Austin Clements Date: Fri, 20 Nov 2020 17:09:23 -0500 Subject: runtime/testdata/testprogcgo: refactor CrashTraceback This moves the C part of the CrashTraceback test into its own file in preparation for adding a test that transitions back into Go. Change-Id: I9560dcfd80bf8a1d30809fd360f958f5261ebb01 Reviewed-on: https://go-review.googlesource.com/c/go/+/272130 Trust: Austin Clements Run-TryBot: Austin Clements TryBot-Result: Go Bot Reviewed-by: Ian Lance Taylor Reviewed-by: Cherry Zhang --- src/runtime/testdata/testprogcgo/traceback.go | 58 +++------------------------ 1 file changed, 5 insertions(+), 53 deletions(-) (limited to 'src/runtime/testdata/testprogcgo/traceback.go') diff --git a/src/runtime/testdata/testprogcgo/traceback.go b/src/runtime/testdata/testprogcgo/traceback.go index 2a023f66ca..03de894c89 100644 --- a/src/runtime/testdata/testprogcgo/traceback.go +++ b/src/runtime/testdata/testprogcgo/traceback.go @@ -11,58 +11,10 @@ package main /* #cgo CFLAGS: -g -O0 -#include - -char *p; - -static int f3(void) { - *p = 0; - return 0; -} - -static int f2(void) { - return f3(); -} - -static int f1(void) { - return f2(); -} - -struct cgoTracebackArg { - uintptr_t context; - uintptr_t sigContext; - uintptr_t* buf; - uintptr_t max; -}; - -struct cgoSymbolizerArg { - uintptr_t pc; - const char* file; - uintptr_t lineno; - const char* func; - uintptr_t entry; - uintptr_t more; - uintptr_t data; -}; - -void cgoTraceback(void* parg) { - struct cgoTracebackArg* arg = (struct cgoTracebackArg*)(parg); - arg->buf[0] = 1; - arg->buf[1] = 2; - arg->buf[2] = 3; - arg->buf[3] = 0; -} - -void cgoSymbolizer(void* parg) { - struct cgoSymbolizerArg* arg = (struct cgoSymbolizerArg*)(parg); - if (arg->pc != arg->data + 1) { - arg->file = "unexpected data"; - } else { - arg->file = "cgo symbolizer"; - } - arg->lineno = arg->data + 1; - arg->data++; -} +// Defined in traceback_c.c. +int tracebackF1(void); +void cgoTraceback(void* parg); +void cgoSymbolizer(void* parg); */ import "C" @@ -77,5 +29,5 @@ func init() { func CrashTraceback() { runtime.SetCgoTraceback(0, unsafe.Pointer(C.cgoTraceback), nil, unsafe.Pointer(C.cgoSymbolizer)) - C.f1() + C.tracebackF1() } -- cgit v1.3 From e8de596f04d0ea7fb6fb68b036760bf088a9c6c2 Mon Sep 17 00:00:00 2001 From: Austin Clements Date: Fri, 20 Nov 2020 17:32:46 -0500 Subject: runtime: use inlined function name for traceback elision Currently, gentraceback decides which frames to print or elide when unwinding inlined frames using only the name of the outermost function. If the outermost function should be elided, then inlined functions will also be elided, even if they shouldn't be. This happens in practice in at least one situation. As of CL 258938, exported Go functions (and functions they call) can now be inlined into the generated _cgoexp_HASH_FN function. The runtime elides _cgoexp_HASH_FN from tracebacks because it doesn't contain a ".". Because of this bug, it also elides anything that was inlined into it. This CL fixes this by synthesizing a funcInfo for the inlined functions to pass to showframe. Fixes #42754. Change-Id: Ie6c663a4a1ac7f0d4beb1aa60bc26fc8cddd0f9d Reviewed-on: https://go-review.googlesource.com/c/go/+/272131 Trust: Austin Clements Run-TryBot: Austin Clements TryBot-Result: Go Bot Reviewed-by: Cherry Zhang --- src/runtime/crash_cgo_test.go | 18 +++++++++++ src/runtime/stack_test.go | 41 ++++++++++++++++++++++++++ src/runtime/testdata/testprogcgo/traceback.go | 21 +++++++++++++ src/runtime/testdata/testprogcgo/traceback_c.c | 11 +++++-- src/runtime/traceback.go | 15 ++++++++-- 5 files changed, 102 insertions(+), 4 deletions(-) (limited to 'src/runtime/testdata/testprogcgo/traceback.go') diff --git a/src/runtime/crash_cgo_test.go b/src/runtime/crash_cgo_test.go index 0680d07a32..140c170ddc 100644 --- a/src/runtime/crash_cgo_test.go +++ b/src/runtime/crash_cgo_test.go @@ -254,6 +254,24 @@ func TestCgoCrashTraceback(t *testing.T) { } } +func TestCgoCrashTracebackGo(t *testing.T) { + t.Parallel() + switch platform := runtime.GOOS + "/" + runtime.GOARCH; platform { + case "darwin/amd64": + case "linux/amd64": + case "linux/ppc64le": + default: + t.Skipf("not yet supported on %s", platform) + } + got := runTestProg(t, "testprogcgo", "CrashTracebackGo") + for i := 1; i <= 3; i++ { + want := fmt.Sprintf("main.h%d", i) + if !strings.Contains(got, want) { + t.Errorf("missing %s", want) + } + } +} + func TestCgoTracebackContext(t *testing.T) { t.Parallel() got := runTestProg(t, "testprogcgo", "TracebackContext") diff --git a/src/runtime/stack_test.go b/src/runtime/stack_test.go index adfc65384a..43fc5cac55 100644 --- a/src/runtime/stack_test.go +++ b/src/runtime/stack_test.go @@ -17,6 +17,7 @@ import ( "sync/atomic" "testing" "time" + _ "unsafe" // for go:linkname ) // TestStackMem measures per-thread stack segment cache behavior. @@ -851,3 +852,43 @@ func deferHeapAndStack(n int) (r int) { // Pass a value to escapeMe to force it to escape. var escapeMe = func(x interface{}) {} + +// Test that when F -> G is inlined and F is excluded from stack +// traces, G still appears. +func TestTracebackInlineExcluded(t *testing.T) { + defer func() { + recover() + buf := make([]byte, 4<<10) + stk := string(buf[:Stack(buf, false)]) + + t.Log(stk) + + if not := "tracebackExcluded"; strings.Contains(stk, not) { + t.Errorf("found but did not expect %q", not) + } + if want := "tracebackNotExcluded"; !strings.Contains(stk, want) { + t.Errorf("expected %q in stack", want) + } + }() + tracebackExcluded() +} + +// tracebackExcluded should be excluded from tracebacks. There are +// various ways this could come up. Linking it to a "runtime." name is +// rather synthetic, but it's easy and reliable. See issue #42754 for +// one way this happened in real code. +// +//go:linkname tracebackExcluded runtime.tracebackExcluded +//go:noinline +func tracebackExcluded() { + // Call an inlined function that should not itself be excluded + // from tracebacks. + tracebackNotExcluded() +} + +// tracebackNotExcluded should be inlined into tracebackExcluded, but +// should not itself be excluded from the traceback. +func tracebackNotExcluded() { + var x *int + *x = 0 +} diff --git a/src/runtime/testdata/testprogcgo/traceback.go b/src/runtime/testdata/testprogcgo/traceback.go index 03de894c89..e2d7599131 100644 --- a/src/runtime/testdata/testprogcgo/traceback.go +++ b/src/runtime/testdata/testprogcgo/traceback.go @@ -12,6 +12,7 @@ package main #cgo CFLAGS: -g -O0 // Defined in traceback_c.c. +extern int crashInGo; int tracebackF1(void); void cgoTraceback(void* parg); void cgoSymbolizer(void* parg); @@ -25,9 +26,29 @@ import ( func init() { register("CrashTraceback", CrashTraceback) + register("CrashTracebackGo", CrashTracebackGo) } func CrashTraceback() { runtime.SetCgoTraceback(0, unsafe.Pointer(C.cgoTraceback), nil, unsafe.Pointer(C.cgoSymbolizer)) C.tracebackF1() } + +func CrashTracebackGo() { + C.crashInGo = 1 + CrashTraceback() +} + +//export h1 +func h1() { + h2() +} + +func h2() { + h3() +} + +func h3() { + var x *int + *x = 0 +} diff --git a/src/runtime/testdata/testprogcgo/traceback_c.c b/src/runtime/testdata/testprogcgo/traceback_c.c index 54f44e11fc..56eda8fa8c 100644 --- a/src/runtime/testdata/testprogcgo/traceback_c.c +++ b/src/runtime/testdata/testprogcgo/traceback_c.c @@ -2,14 +2,21 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -// The C definitions for traceback.go. +// The C definitions for traceback.go. That file uses //export so +// it can't put function definitions in the "C" import comment. #include char *p; +int crashInGo; +extern void h1(void); + int tracebackF3(void) { - *p = 0; + if (crashInGo) + h1(); + else + *p = 0; return 0; } diff --git a/src/runtime/traceback.go b/src/runtime/traceback.go index f3df152535..0825e9e707 100644 --- a/src/runtime/traceback.go +++ b/src/runtime/traceback.go @@ -396,13 +396,21 @@ func gentraceback(pc0, sp0, lr0 uintptr, gp *g, skip int, pcbuf *uintptr, max in // If there is inlining info, print the inner frames. if inldata := funcdata(f, _FUNCDATA_InlTree); inldata != nil { inltree := (*[1 << 20]inlinedCall)(inldata) + var inlFunc _func + inlFuncInfo := funcInfo{&inlFunc, f.datap} for { ix := pcdatavalue(f, _PCDATA_InlTreeIndex, tracepc, nil) if ix < 0 { break } - if (flags&_TraceRuntimeFrames) != 0 || showframe(f, gp, nprint == 0, inltree[ix].funcID, lastFuncID) { - name := funcnameFromNameoff(f, inltree[ix].func_) + + // Create a fake _func for the + // inlined function. + inlFunc.nameoff = inltree[ix].func_ + inlFunc.funcID = inltree[ix].funcID + + if (flags&_TraceRuntimeFrames) != 0 || showframe(inlFuncInfo, gp, nprint == 0, inlFuncInfo.funcID, lastFuncID) { + name := funcname(inlFuncInfo) file, line := funcline(f, tracepc) print(name, "(...)\n") print("\t", file, ":", line, "\n") @@ -811,6 +819,9 @@ func showframe(f funcInfo, gp *g, firstFrame bool, funcID, childID funcID) bool // showfuncinfo reports whether a function with the given characteristics should // be printed during a traceback. func showfuncinfo(f funcInfo, firstFrame bool, funcID, childID funcID) bool { + // Note that f may be a synthesized funcInfo for an inlined + // function, in which case only nameoff and funcID are set. + level, _, _ := gotraceback() if level > 1 { // Show all frames. -- cgit v1.3