internal/crashmonitor: parse systemstack too Until now, the crashmonitor would only parse the running goroutine's stack from the system traceback, omitting any frames on the system stack, which are usually the frames of interest. This CL extends the parser to parse all the stacks and tie the two parts together. The test artificially triggers a crash in runtime.ReadMemStats on the systemstack by allocating the MemStats in a read-only segment, and checks that the resulting stack counter has the right name. Change-Id: I86bc3d66a5e79dc9dd398ed5914f33977d7fe8c2 Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/789880 LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Cherry Mui <cherryyz@google.com>
diff --git a/internal/crashmonitor/monitor.go b/internal/crashmonitor/monitor.go index ac22f68..f6e6c70 100644 --- a/internal/crashmonitor/monitor.go +++ b/internal/crashmonitor/monitor.go
@@ -202,19 +202,26 @@ return strconv.ParseUint(pcstr, 0, 64) // 0 => allow 0x prefix } + type goroutine struct { + id uint64 + pcs []uintptr + system bool // stack starts with runtime.systemstack_switch + } + var ( - pcs []uintptr + glist []*goroutine parentSentinel uint64 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] + // Parsing state for the current goroutine. + g *goroutine + symLine = true // a symbol (not a filename) is expected on this line + currSymbol string + prevSymbol string + ) + + for _, line := range lines { // Read sentinel value. if parentSentinel == 0 && strings.HasPrefix(line, "sentinel ") { _, err := fmt.Sscanf(line, "sentinel %x", &parentSentinel) @@ -224,27 +231,37 @@ 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") + // Check for a "goroutine GID [STATUS]" header. + if strings.HasPrefix(line, "goroutine ") { + if parentSentinel == 0 { + return nil, fmt.Errorf("no sentinel value in crash report") + } + isG0 := strings.HasPrefix(line, "goroutine 0 ") + isRunning := strings.Contains(line, " [running]:") + if isG0 || isRunning { + var id uint64 + if parts := strings.Fields(line); len(parts) > 1 { + id, _ = strconv.ParseUint(parts[1], 10, 64) } + g = &goroutine{id: id} + glist = append(glist, g) + symLine = true + currSymbol = "" + prevSymbol = "" + } else { + g = nil } continue } - // A blank line marks end of a goroutine stack. - if line == "" { - break + if g == nil { + continue } - // Skip the final "created by SYMBOL in goroutine GID" part. - if strings.HasPrefix(line, "created by ") { - break + // A blank line or "created by " marks the end of the goroutine stack. + if line == "" || strings.HasPrefix(line, "created by ") { + g = nil + continue } // Expect a pair of lines: @@ -316,7 +333,10 @@ pc++ } - pcs = append(pcs, uintptr(pc)) + if len(g.pcs) == 0 && currSymbol == "runtime.systemstack_switch" { + g.system = true + } + g.pcs = append(g.pcs, uintptr(pc)) // Done with this frame. Next line is a new frame. prevSymbol = currSymbol @@ -324,5 +344,24 @@ symLine = true } } - return pcs, nil + + if len(glist) == 0 { + return nil, nil + } + + // The first goroutine in the dump is the one that crashed. + firstG := glist[0] + + // If the first goroutine is g0 (the system stack), we want to find the user + // goroutine that called it, which will start with systemstack_switch. + if firstG.id == 0 { + for _, g := range glist[1:] { + if g.system && len(g.pcs) > 0 { + // Stitch the g0 stack and user stack (skipping systemstack_switch). + return append(firstG.pcs, g.pcs[1:]...), nil + } + } + } + + return firstG.pcs, nil }
diff --git a/internal/crashmonitor/monitor_test.go b/internal/crashmonitor/monitor_test.go index a2033a9..1077437 100644 --- a/internal/crashmonitor/monitor_test.go +++ b/internal/crashmonitor/monitor_test.go
@@ -30,7 +30,7 @@ func TestMain(m *testing.M) { entry := os.Getenv("CRASHMONITOR_TEST_ENTRYPOINT") switch entry { - case "via-stderr.panic", "via-stderr.trap": + case "via-stderr.panic", "via-stderr.trap", "via-stderr.systemstack": // This mode bypasses Start and debug.SetCrashOutput; // the crash is printed to stderr. debug.SetTraceback("system") @@ -38,12 +38,14 @@ if entry == "via-stderr.panic" { childPanic() // this line is "TestMain:+10" - } else { + } else if entry == "via-stderr.trap" { childTrap() // this line is "TestMain:+12" + } else { + childSystemstackCrash() } panic("unreachable") - case "start.panic", "start.trap", "start.exit": + case "start.panic", "start.trap", "start.exit", "start.systemstack": // These modes uses Start and debug.SetCrashOutput. // We stub the actual telemetry by instead writing to a file. crashmonitor.SetIncrementCounter(func(name string) { @@ -67,6 +69,11 @@ childTrap() // this line is "TestMain.func4:+1" }() select {} // deadlocks when reached + case "start.systemstack": + go func() { + childSystemstackCrash() + }() + select {} // deadlocks when reached case "start.exit": os.Exit(42) } @@ -110,7 +117,7 @@ got = sanitize(counter.DecodeStack(got)) wantRE := regexp.MustCompile(`(?m)crash/crash runtime.gopanic:-- -golang.org/x/telemetry/internal/crashmonitor_test\.grandchildPanic:=85,\+0x.* +golang.org/x/telemetry/internal/crashmonitor_test\.grandchildPanic:=92,\+0x.* golang.org/x/telemetry/internal/crashmonitor_test\.childPanic:\+2,\+0x.* golang.org/x/telemetry/internal/crashmonitor_test\.TestMain:\+10,\+0x.* main.main:-- @@ -133,7 +140,7 @@ runtime.gopanic:-- runtime.panicmem:-- runtime.sigpanic:-- -golang.org/x/telemetry/internal/crashmonitor_test.grandchildTrap:=96,\+0x.* +golang.org/x/telemetry/internal/crashmonitor_test.grandchildTrap:=103,\+0x.* golang.org/x/telemetry/internal/crashmonitor_test.childTrap:\+2,\+0x.* golang.org/x/telemetry/internal/crashmonitor_test.TestMain:\+12,\+0x.* main.main:-- @@ -193,7 +200,7 @@ got := sanitize(counter.DecodeStack(string(data))) wantRE := regexp.MustCompile(`(?m)crash/crash runtime.gopanic:-- -golang.org/x/telemetry/internal/crashmonitor_test.grandchildPanic:=85,.* +golang.org/x/telemetry/internal/crashmonitor_test.grandchildPanic:=92,.* golang.org/x/telemetry/internal/crashmonitor_test.childPanic:\+2,.* golang.org/x/telemetry/internal/crashmonitor_test.TestMain.func3:\+1,.* runtime.goexit:--`) @@ -217,7 +224,7 @@ runtime.gopanic:-- runtime.panicmem:-- runtime.sigpanic:-- -golang.org/x/telemetry/internal/crashmonitor_test.grandchildTrap:=96,.* +golang.org/x/telemetry/internal/crashmonitor_test.grandchildTrap:=103,.* golang.org/x/telemetry/internal/crashmonitor_test.childTrap:\+2,.* golang.org/x/telemetry/internal/crashmonitor_test.TestMain.func4:\+1,.* runtime.goexit:--`) @@ -225,8 +232,39 @@ t.Errorf("got counter name <<%s>>, want match for <<%s>>", got, wantRE) } }) + + // Check the name of the incremented counter + // when the child process crashes on systemstack. + t.Run("systemstack", func(t *testing.T) { + if childSystemstackCrash == nil { + t.Skip("systemstack crash not supported on this platform") // unix only for now + } + // Gather a stack trace from executing the systemstack crash above. + telemetryFile, exitFile, _ := runSelf(t, "start.systemstack") + waitForExitFile(t, exitFile) + data, err := os.ReadFile(telemetryFile) + if err != nil { + t.Fatalf("failed to read file: %v", err) + } + got := sanitize(counter.DecodeStack(string(data))) + wantRE := regexp.MustCompile(`(?m)crash/crash +runtime.* +runtime.ReadMemStats.func.*:-- +runtime.systemstack:-- +runtime.ReadMemStats:-- +golang.org/x/telemetry/internal/crashmonitor_test.* +golang.org/x/telemetry/internal/crashmonitor_test.TestMain.func5:\+1,.* +runtime.goexit:--`) + if !wantRE.MatchString(got) { + t.Errorf("got counter name <<%s>>, want match for <<%s>>", got, wantRE) + } + }) } +// On supported platforms (e.g. unix) this var is set to function +// that triggers a crash on the system stack. +var childSystemstackCrash func() + // runSelf fork+exec's this test executable using an alternate entry point. // It returns the child's stderr, the name of the file // to which any incremented counter name will be written, and
diff --git a/internal/crashmonitor/monitor_unix_test.go b/internal/crashmonitor/monitor_unix_test.go new file mode 100644 index 0000000..846aa42 --- /dev/null +++ b/internal/crashmonitor/monitor_unix_test.go
@@ -0,0 +1,33 @@ +// Copyright 2026 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. + +//go:build unix + +package crashmonitor_test + +import ( + "log" + "runtime" + "syscall" + "unsafe" +) + +func init() { + childSystemstackCrash = func() { + // ReadMemStats writes to the supplied variable while + // running on the system stack. We pass it a readonly + // variable to trigger a SIGBUS. + runtime.ReadMemStats(newReadOnly[runtime.MemStats]()) + } +} + +func newReadOnly[T any]() *T { + const PageSize = 4096 + length := (unsafe.Sizeof(*new(T)) + PageSize - 1) &^ (PageSize - 1) + data, err := syscall.Mmap(-1, 0, int(length), syscall.PROT_READ, syscall.MAP_ANON|syscall.MAP_PRIVATE) + if err != nil { + log.Fatalf("mmap: %v", err) + } + return (*T)(unsafe.Pointer(&data[0])) +}