]> Cypherpunks repositories - gostls13.git/commitdiff
runtime: test trap panic parsing in TestTracebackSystem
authorMichael Pratt <mpratt@google.com>
Fri, 20 Dec 2024 01:58:19 +0000 (20:58 -0500)
committerMichael Pratt <mpratt@google.com>
Fri, 20 Dec 2024 03:19:50 +0000 (19:19 -0800)
This mirrors https://go.dev/cl/637755, as x/telemetry is now aware of
sigpanic preceding trap frames.

For #70637.

Change-Id: I13a775f25e89047702d4f2d463ce3210bcf192d9
Reviewed-on: https://go-review.googlesource.com/c/go/+/638015
Reviewed-by: Cherry Mui <cherryyz@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

src/runtime/crash_test.go
src/runtime/traceback_system_test.go

index 268ddb59c9e4c1e6cb36ecc48935d6bfd53baba0..236c32ea34f7dea7443e65229ee4ca6e54b79157 100644 (file)
@@ -32,8 +32,11 @@ const entrypointVar = "RUNTIME_TEST_ENTRYPOINT"
 
 func TestMain(m *testing.M) {
        switch entrypoint := os.Getenv(entrypointVar); entrypoint {
-       case "crash":
-               crash()
+       case "panic":
+               crashViaPanic()
+               panic("unreachable")
+       case "trap":
+               crashViaTrap()
                panic("unreachable")
        default:
                log.Fatalf("invalid %s: %q", entrypointVar, entrypoint)
index ece58e806d51c1f7638d9e03a2e7f46db9be8355..af20f54a0946fcbaef69add7e55c6ff3d019715c 100644 (file)
@@ -23,8 +23,8 @@ import (
 )
 
 // This is the entrypoint of the child process used by
-// TestTracebackSystem. It prints a crash report to stdout.
-func crash() {
+// TestTracebackSystem/panic. It prints a crash report to stdout.
+func crashViaPanic() {
        // Ensure that we get pc=0x%x values in the traceback.
        debug.SetTraceback("system")
        writeSentinel(os.Stdout)
@@ -37,6 +37,21 @@ func crash() {
        select {}
 }
 
+// This is the entrypoint of the child process used by
+// TestTracebackSystem/trap. It prints a crash report to stdout.
+func crashViaTrap() {
+       // Ensure that we get pc=0x%x values in the traceback.
+       debug.SetTraceback("system")
+       writeSentinel(os.Stdout)
+       debug.SetCrashOutput(os.Stdout, debug.CrashOptions{})
+
+       go func() {
+               // This call is typically inlined.
+               trap1()
+       }()
+       select {}
+}
+
 func child1() {
        child2()
 }
@@ -85,6 +100,20 @@ func child7() {
        panic("oops")
 }
 
+func trap1() {
+       trap2()
+}
+
+var sinkPtr *int
+
+func trap2() {
+       trap3(sinkPtr)
+}
+
+func trap3(i *int) {
+       *i = 42
+}
+
 // 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
@@ -100,46 +129,75 @@ func TestTracebackSystem(t *testing.T) {
                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")
-       var stdout, stderr bytes.Buffer
-       cmd.Stdout = &stdout
-       cmd.Stderr = &stderr
-       cmd.Run() // expected to crash
-       t.Logf("stderr:\n%s\nstdout: %s\n", stderr.Bytes(), stdout.Bytes())
-       crash := stdout.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")
+       tests := []struct{
+               name string
+               want string
+       }{
+               {
+                       name: "panic",
+                       want: `redacted.go:0: runtime.gopanic
+traceback_system_test.go:100: runtime_test.child7:     panic("oops")
+traceback_system_test.go:83: runtime_test.child6:      child7() // appears in stack trace
+traceback_system_test.go:74: runtime_test.child5:      child6() // appears in stack trace
+traceback_system_test.go:68: runtime_test.child4:      child5()
+traceback_system_test.go:64: runtime_test.child3:      child4()
+traceback_system_test.go:60: runtime_test.child2:      child3()
+traceback_system_test.go:56: runtime_test.child1:      child2()
+traceback_system_test.go:35: runtime_test.crashViaPanic.func1:                 child1()
+redacted.go:0: runtime.goexit
+`,
+               },
+               {
+                       // Test panic via trap. x/telemetry is aware that trap
+                       // PCs follow runtime.sigpanic and need to be
+                       // incremented to offset the decrement done by
+                       // CallersFrames.
+                       name: "trap",
+                       want: `redacted.go:0: runtime.gopanic
+redacted.go:0: runtime.panicmem
+redacted.go:0: runtime.sigpanic
+traceback_system_test.go:114: runtime_test.trap3:      *i = 42
+traceback_system_test.go:110: runtime_test.trap2:      trap3(sinkPtr)
+traceback_system_test.go:104: runtime_test.trap1:      trap2()
+traceback_system_test.go:50: runtime_test.crashViaTrap.func1:          trap1()
+redacted.go:0: runtime.goexit
+`,
+               },
        }
 
-       // Parse the PCs out of the child's crash report.
-       pcs, err := parseStackPCs(crash)
-       if err != nil {
-               t.Fatal(err)
-       }
+       for _, tc := range tests {
+               t.Run(tc.name, func(t *testing.T) {
+                       // 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+"="+tc.name)
+                       var stdout, stderr bytes.Buffer
+                       cmd.Stdout = &stdout
+                       cmd.Stderr = &stderr
+                       cmd.Run() // expected to crash
+                       t.Logf("stderr:\n%s\nstdout: %s\n", stderr.Bytes(), stdout.Bytes())
+                       crash := stdout.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")
+                       }
 
-       // Unwind the stack using this executable's symbol table.
-       got := formatStack(pcs)
-       want := `redacted.go:0: runtime.gopanic
-traceback_system_test.go:85: runtime_test.child7:      panic("oops")
-traceback_system_test.go:68: runtime_test.child6:      child7() // appears in stack trace
-traceback_system_test.go:59: runtime_test.child5:      child6() // appears in stack trace
-traceback_system_test.go:53: runtime_test.child4:      child5()
-traceback_system_test.go:49: runtime_test.child3:      child4()
-traceback_system_test.go:45: runtime_test.child2:      child3()
-traceback_system_test.go:41: runtime_test.child1:      child2()
-traceback_system_test.go:35: runtime_test.crash.func1:                 child1()
-redacted.go:0: runtime.goexit
-`
-       if strings.TrimSpace(got) != strings.TrimSpace(want) {
-               t.Errorf("got:\n%swant:\n%s", got, want)
+                       // 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)
+                       if strings.TrimSpace(got) != strings.TrimSpace(tc.want) {
+                               t.Errorf("got:\n%swant:\n%s", got, tc.want)
+                       }
+               })
        }
 }
 
@@ -154,6 +212,35 @@ redacted.go:0: runtime.goexit
 //
 // (Copied from golang.org/x/telemetry/crashmonitor.parseStackPCs.)
 func parseStackPCs(crash string) ([]uintptr, error) {
+       // getSymbol parses the symbol name out of a line of the form:
+       // SYMBOL(ARGS)
+       //
+       // Note: SYMBOL may contain parens "pkg.(*T).method". However, type
+       // parameters are always replaced with ..., so they cannot introduce
+       // more parens. e.g., "pkg.(*T[...]).method".
+       //
+       // ARGS can contain parens. We want the first paren that is not
+       // immediately preceded by a ".".
+       //
+       // TODO(prattmic): This is mildly complicated and is only used to find
+       // runtime.sigpanic, so perhaps simplify this by checking explicitly
+       // for sigpanic.
+       getSymbol := func(line string) (string, error) {
+               var prev rune
+               for i, c := range line {
+                       if line[i] != '(' {
+                               prev = c
+                               continue
+                       }
+                       if prev == '.' {
+                               prev = c
+                               continue
+                       }
+                       return line[:i], nil
+               }
+               return "", fmt.Errorf("no symbol for stack frame: %s", line)
+       }
+
        // getPC parses the PC out of a line of the form:
        //     \tFILE:LINE +0xRELPC sp=... fp=... pc=...
        getPC := func(line string) (uint64, error) {
@@ -170,6 +257,9 @@ func parseStackPCs(crash string) ([]uintptr, error) {
                childSentinel  = sentinel()
                on             = false // are we in the first running goroutine?
                lines          = strings.Split(crash, "\n")
+               symLine        = true // within a goroutine, every other line is a symbol or file/line/pc location, starting with symbol.
+               currSymbol     string
+               prevSymbol     string // symbol of the most recent previous frame with a PC.
        )
        for i := 0; i < len(lines); i++ {
                line := lines[i]
@@ -212,21 +302,76 @@ func parseStackPCs(crash string) ([]uintptr, error) {
                // 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]
+               if symLine {
+                       var err error
+                       currSymbol, err = getSymbol(line)
+                       if err != nil {
+                               return nil, fmt.Errorf("error extracting symbol: %v", err)
+                       }
 
-               // 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
+                       symLine = false // Next line is FILE:LINE.
+               } else {
+                       // 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.
+
+                               // Done with this frame. Next line is a new frame.
+                               //
+                               // Don't update prevSymbol; we only want to
+                               // track frames with a PC.
+                               currSymbol = ""
+                               symLine = true
+                               continue
+                       }
+
+                       pc = pc-parentSentinel+childSentinel
+
+                       // If the previous frame was sigpanic, then this frame
+                       // was a trap (e.g., SIGSEGV).
+                       //
+                       // Typically all middle frames are calls, and report
+                       // the "return PC". That is, the instruction following
+                       // the CALL where the callee will eventually return to.
+                       //
+                       // runtime.CallersFrames is aware of this property and
+                       // will decrement each PC by 1 to "back up" to the
+                       // location of the CALL, which is the actual line
+                       // number the user expects.
+                       //
+                       // This does not work for traps, as a trap is not a
+                       // call, so the reported PC is not the return PC, but
+                       // the actual PC of the trap.
+                       //
+                       // runtime.Callers is aware of this and will
+                       // intentionally increment trap PCs in order to correct
+                       // for the decrement performed by
+                       // runtime.CallersFrames. See runtime.tracebackPCs and
+                       // runtume.(*unwinder).symPC.
+                       //
+                       // We must emulate the same behavior, otherwise we will
+                       // report the location of the instruction immediately
+                       // prior to the trap, which may be on a different line,
+                       // or even a different inlined functions.
+                       //
+                       // TODO(prattmic): The runtime applies the same trap
+                       // behavior for other "injected calls", see injectCall
+                       // in runtime.(*unwinder).next. Do we want to handle
+                       // those as well? I don't believe we'd ever see
+                       // runtime.asyncPreempt or runtime.debugCallV2 in a
+                       // typical crash.
+                       if prevSymbol == "runtime.sigpanic" {
+                               pc++
+                       }
+
+                       pcs = append(pcs, uintptr(pc))
+
+                       // Done with this frame. Next line is a new frame.
+                       prevSymbol = currSymbol
+                       currSymbol = ""
+                       symLine = true
                }
-               pcs = append(pcs, uintptr(pc-parentSentinel+childSentinel))
        }
        return pcs, nil
 }