From 386dcf4c93bfc8af232a12086d73da4bb3558af9 Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Thu, 14 Mar 2024 15:30:13 -0400 Subject: [PATCH] runtime: revert "traceback: include pc=0x%x for inline frames" This reverts commit 643d816c8b43 (CL 561635). Reason for revert: This works for telemetry but broke various other properties of the tracebacks as well as some programs that read tracebacks. We should figure out a solution that works for all uses, and in the interim we should not be making telemetry work at the cost of breaking other, existing valid uses. See #65761 for details. Change-Id: I467993ae778887e5bd3cca4c0fb54e9d44802ee1 Reviewed-on: https://go-review.googlesource.com/c/go/+/571797 Auto-Submit: Russ Cox LUCI-TryBot-Result: Go LUCI Reviewed-by: Austin Clements Reviewed-by: Cherry Mui --- src/runtime/crash_test.go | 13 -- src/runtime/traceback.go | 24 +-- src/runtime/traceback_system_test.go | 238 --------------------------- 3 files changed, 6 insertions(+), 269 deletions(-) delete mode 100644 src/runtime/traceback_system_test.go diff --git a/src/runtime/crash_test.go b/src/runtime/crash_test.go index 9ba45b8f2a..33edb4652b 100644 --- a/src/runtime/crash_test.go +++ b/src/runtime/crash_test.go @@ -14,7 +14,6 @@ import ( "internal/testenv" tracev2 "internal/trace/v2" "io" - "log" "os" "os/exec" "path/filepath" @@ -29,19 +28,7 @@ import ( var toRemove []string -const entrypointVar = "RUNTIME_TEST_ENTRYPOINT" - func TestMain(m *testing.M) { - switch entrypoint := os.Getenv(entrypointVar); entrypoint { - case "crash": - crash() - panic("unreachable") - default: - log.Fatalf("invalid %s: %q", entrypointVar, entrypoint) - case "": - // fall through to normal behavior - } - _, coreErrBefore := os.Stat("core") status := m.Run() diff --git a/src/runtime/traceback.go b/src/runtime/traceback.go index 61027ea89a..1c75c447d2 100644 --- a/src/runtime/traceback.go +++ b/src/runtime/traceback.go @@ -993,24 +993,12 @@ func traceback2(u *unwinder, showRuntime bool, skip, max int) (n, lastN int) { } print(")\n") print("\t", file, ":", line) - // The contract between Callers and CallersFrames uses - // return addresses, which are +1 relative to the CALL - // instruction. Follow that convention. - pc := uf.pc + 1 - if !iu.isInlined(uf) && pc > f.entry() { - // Func-relative PCs make no sense for inlined - // frames because there is no actual entry. - print(" +", hex(pc-f.entry())) - } - if gp.m != nil && gp.m.throwing >= throwTypeRuntime && gp == gp.m.curg || level >= 2 { - if !iu.isInlined(uf) { - // The stack information makes no sense for inline frames. - print(" fp=", hex(u.frame.fp), " sp=", hex(u.frame.sp), " pc=", hex(pc)) - } else { - // The PC for an inlined frame is a special marker NOP, - // but crash monitoring tools may still parse the PCs - // and feed them to CallersFrames. - print(" pc=", hex(pc)) + if !iu.isInlined(uf) { + if u.frame.pc > f.entry() { + print(" +", hex(u.frame.pc-f.entry())) + } + if gp.m != nil && gp.m.throwing >= throwTypeRuntime && gp == gp.m.curg || level >= 2 { + print(" fp=", hex(u.frame.fp), " sp=", hex(u.frame.sp), " pc=", hex(u.frame.pc)) } } print("\n") diff --git a/src/runtime/traceback_system_test.go b/src/runtime/traceback_system_test.go deleted file mode 100644 index 223d78a808..0000000000 --- a/src/runtime/traceback_system_test.go +++ /dev/null @@ -1,238 +0,0 @@ -// Copyright 2024 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 runtime_test - -// This test of GOTRACEBACK=system has its own file, -// to minimize line-number perturbation. - -import ( - "bytes" - "fmt" - "internal/testenv" - "io" - "os" - "path/filepath" - "reflect" - "runtime" - "runtime/debug" - "strconv" - "strings" - "testing" -) - -// This is the entrypoint of the child process used by -// TestTracebackSystem. It prints a crash report to stdout. -func crash() { - // Ensure that we get pc=0x%x values in the traceback. - debug.SetTraceback("system") - writeSentinel(os.Stdout) - debug.SetCrashOutput(os.Stdout) - - go func() { - // This call is typically inlined. - child() - }() - select {} -} - -func child() { - grandchild() -} - -func grandchild() { - // Write runtime.Caller's view of the stack to stderr, for debugging. - var pcs [16]uintptr - n := runtime.Callers(1, pcs[:]) - io.WriteString(os.Stderr, formatStack(pcs[:n])) - - // Cause the crash report to be written to stdout. - panic("oops") -} - -// TestTracebackSystem tests that the syntax of crash reports produced -// by GOTRACEBACK=system (see traceback2) contains a complete, -// parseable list of program counters for the running goroutine that -// can be parsed and fed to runtime.CallersFrames to obtain accurate -// information about the logical call stack, even in the presence of -// inlining. -// -// The test is a distillation of the crash monitor in -// golang.org/x/telemetry/crashmonitor. -func TestTracebackSystem(t *testing.T) { - testenv.MustHaveExec(t) - if runtime.GOOS == "android" { - t.Skip("Can't read source code for this file on Android") - } - - // Fork+exec the crashing process. - exe, err := os.Executable() - if err != nil { - t.Fatal(err) - } - cmd := testenv.Command(t, exe) - cmd.Env = append(cmd.Environ(), entrypointVar+"=crash") - cmd.Stdout = new(strings.Builder) - // cmd.Stderr = os.Stderr // uncomment to debug, e.g. to see runtime.Caller's view - cmd.Run() // expected to crash - crash := cmd.Stdout.(*strings.Builder).String() - - // If the only line is the sentinel, it wasn't a crash. - if strings.Count(crash, "\n") < 2 { - t.Fatalf("child process did not produce a crash report") - } - - // Parse the PCs out of the child's crash report. - pcs, err := parseStackPCs(crash) - if err != nil { - t.Fatal(err) - } - - // Unwind the stack using this executable's symbol table. - got := formatStack(pcs) - want := `redacted.go:0: runtime.gopanic -traceback_system_test.go:51: runtime_test.grandchild: panic("oops") -traceback_system_test.go:41: runtime_test.child: grandchild() -traceback_system_test.go:35: runtime_test.crash.func1: child() -redacted.go:0: runtime.goexit` - if strings.TrimSpace(got) != strings.TrimSpace(want) { - t.Errorf("got:\n%swant:\n%s", got, want) - } -} - -// parseStackPCs parses the parent process's program counters for the -// first running goroutine out of a GOTRACEBACK=system traceback, -// adjusting them so that they are valid for the child process's text -// segment. -// -// This function returns only program counter values, ensuring that -// there is no possibility of strings from the crash report (which may -// contain PII) leaking into the telemetry system. -// -// (Copied from golang.org/x/telemetry/crashmonitor.parseStackPCs.) -func parseStackPCs(crash string) ([]uintptr, error) { - // getPC parses the PC out of a line of the form: - // \tFILE:LINE +0xRELPC sp=... fp=... pc=... - getPC := func(line string) (uint64, error) { - _, pcstr, ok := strings.Cut(line, " pc=") // e.g. pc=0x%x - if !ok { - return 0, fmt.Errorf("no pc= for stack frame: %s", line) - } - return strconv.ParseUint(pcstr, 0, 64) // 0 => allow 0x prefix - } - - var ( - pcs []uintptr - parentSentinel uint64 - childSentinel = sentinel() - on = false // are we in the first running goroutine? - lines = strings.Split(crash, "\n") - ) - for i := 0; i < len(lines); i++ { - line := lines[i] - - // Read sentinel value. - if parentSentinel == 0 && strings.HasPrefix(line, "sentinel ") { - _, err := fmt.Sscanf(line, "sentinel %x", &parentSentinel) - if err != nil { - return nil, fmt.Errorf("can't read sentinel line") - } - continue - } - - // Search for "goroutine GID [STATUS]" - if !on { - if strings.HasPrefix(line, "goroutine ") && - strings.Contains(line, " [running]:") { - on = true - - if parentSentinel == 0 { - return nil, fmt.Errorf("no sentinel value in crash report") - } - } - continue - } - - // A blank line marks end of a goroutine stack. - if line == "" { - break - } - - // Skip the final "created by SYMBOL in goroutine GID" part. - if strings.HasPrefix(line, "created by ") { - break - } - - // Expect a pair of lines: - // SYMBOL(ARGS) - // \tFILE:LINE +0xRELPC sp=0x%x fp=0x%x pc=0x%x - // Note: SYMBOL may contain parens "pkg.(*T).method" - // The RELPC is sometimes missing. - - // Skip the symbol(args) line. - i++ - if i == len(lines) { - break - } - line = lines[i] - - // Parse the PC, and correct for the parent and child's - // different mappings of the text section. - pc, err := getPC(line) - if err != nil { - // Inlined frame, perhaps; skip it. - continue - } - pcs = append(pcs, uintptr(pc-parentSentinel+childSentinel)) - } - return pcs, nil -} - -// The sentinel function returns its address. The difference between -// this value as observed by calls in two different processes of the -// same executable tells us the relative offset of their text segments. -// -// It would be nice if SetCrashOutput took care of this as it's fiddly -// and likely to confuse every user at first. -func sentinel() uint64 { - return uint64(reflect.ValueOf(sentinel).Pointer()) -} - -func writeSentinel(out io.Writer) { - fmt.Fprintf(out, "sentinel %x\n", sentinel()) -} - -// formatStack formats a stack of PC values using the symbol table, -// redacting information that cannot be relied upon in the test. -func formatStack(pcs []uintptr) string { - // When debugging, show file/line/content of files other than this one. - const debug = false - - var buf strings.Builder - i := 0 - frames := runtime.CallersFrames(pcs) - for { - fr, more := frames.Next() - if debug { - fmt.Fprintf(&buf, "pc=%x ", pcs[i]) - i++ - } - if base := filepath.Base(fr.File); base == "traceback_system_test.go" || debug { - content, err := os.ReadFile(fr.File) - if err != nil { - panic(err) - } - lines := bytes.Split(content, []byte("\n")) - fmt.Fprintf(&buf, "%s:%d: %s: %s\n", base, fr.Line, fr.Function, lines[fr.Line-1]) - } else { - // For robustness, don't show file/line for functions from other files. - fmt.Fprintf(&buf, "redacted.go:0: %s\n", fr.Function) - } - - if !more { - break - } - } - return buf.String() -} -- 2.50.0