internal/counter: record program counters in stack counter names The frame-relative PC is appended to each stack frame. Also, deprecate Supported() function, which since go1.23 always returns true. Fixes golang/go#73268 Change-Id: If8959d0042ff6753aa1f4b58fbc1484b74db5ad8 Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/664175 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Robert Findley <rfindley@google.com> Auto-Submit: Alan Donovan <adonovan@google.com>
diff --git a/crashmonitor/supported.go b/crashmonitor/supported.go index 1683508..7b92510 100644 --- a/crashmonitor/supported.go +++ b/crashmonitor/supported.go
@@ -4,9 +4,7 @@ package crashmonitor -import ic "golang.org/x/telemetry/internal/crashmonitor" - -// Supported reports whether the runtime supports [runtime.SetCrashOutput]. +// Deprecated: Supported returns true. // -// TODO(adonovan): eliminate once go1.23+ is assured. -func Supported() bool { return ic.Supported() } +//go:fix inline +func Supported() bool { return true }
diff --git a/internal/counter/stackcounter.go b/internal/counter/stackcounter.go index 568d40c..3e7ffde 100644 --- a/internal/counter/stackcounter.go +++ b/internal/counter/stackcounter.go
@@ -93,12 +93,22 @@ // Use function-relative line numbering. // f:+2 means two lines into function f. // f:-1 should never happen, but be conservative. + // + // An inlined call is replaced by a NOP instruction + // with the correct pclntab information. _, entryLine := fr.Func.FileLine(fr.Entry) - loc = fmt.Sprintf("%s.%s:%+d", path, fname, fr.Line-entryLine) + loc = fmt.Sprintf("%s.%s:%+d,+0x%x", path, fname, fr.Line-entryLine, fr.PC-fr.Entry) } else { // The function is non-Go code or is fully inlined: // use absolute line number within enclosing file. - loc = fmt.Sprintf("%s.%s:=%d", path, fname, fr.Line) + // + // For inlined calls, the PC and Entry values + // both refer to the enclosing combined function. + // For example, both these PCs are relative to "caller": + // + // callee:=1,+0x12 ('=' means inlined) + // caller:+2,+0x34 + loc = fmt.Sprintf("%s.%s:=%d,+0x%x", path, fname, fr.Line, fr.PC-fr.Entry) } locs = append(locs, loc) if !more {
diff --git a/internal/crashmonitor/crash_go123.go b/internal/crashmonitor/crash_go123.go deleted file mode 100644 index e567ce5..0000000 --- a/internal/crashmonitor/crash_go123.go +++ /dev/null
@@ -1,16 +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. - -//go:build go1.23 - -package crashmonitor - -import ( - "os" - "runtime/debug" -) - -func init() { - setCrashOutput = func(f *os.File) error { return debug.SetCrashOutput(f, debug.CrashOptions{}) } -}
diff --git a/internal/crashmonitor/monitor.go b/internal/crashmonitor/monitor.go index f43d2cd..53966ad 100644 --- a/internal/crashmonitor/monitor.go +++ b/internal/crashmonitor/monitor.go
@@ -21,20 +21,13 @@ "golang.org/x/telemetry/internal/counter" ) -// Supported reports whether the runtime supports [runtime/debug.SetCrashOutput]. -// -// TODO(adonovan): eliminate once go1.23+ is assured. -func Supported() bool { return setCrashOutput != nil } - -var setCrashOutput func(*os.File) error // = runtime/debug.SetCrashOutput on go1.23+ - // Parent sets up the parent side of the crashmonitor. It requires // exclusive use of a writable pipe connected to the child process's stdin. func Parent(pipe *os.File) { writeSentinel(pipe) // Ensure that we get pc=0x%x values in the traceback. debug.SetTraceback("system") - setCrashOutput(pipe) + debug.SetCrashOutput(pipe, debug.CrashOptions{}) // ignore error } // Child runs the part of the crashmonitor that runs in the child process. @@ -284,7 +277,7 @@ continue } - pc = pc-parentSentinel+childSentinel + pc = pc - parentSentinel + childSentinel // If the previous frame was sigpanic, then this frame // was a trap (e.g., SIGSEGV).
diff --git a/internal/crashmonitor/monitor_test.go b/internal/crashmonitor/monitor_test.go index ac2a47f..cef3343 100644 --- a/internal/crashmonitor/monitor_test.go +++ b/internal/crashmonitor/monitor_test.go
@@ -10,6 +10,7 @@ "os" "os/exec" "path/filepath" + "regexp" "runtime/debug" "strings" "testing" @@ -21,6 +22,11 @@ "golang.org/x/telemetry/internal/testenv" ) +// +// Add/removed lines from this comment to compensate for minor +// perturbations in line numbers as the code below evolves. +// + func TestMain(m *testing.M) { entry := os.Getenv("CRASHMONITOR_TEST_ENTRYPOINT") switch entry { @@ -76,7 +82,7 @@ } func grandchildPanic() { - panic("oops") // this line is "grandchildPanic:=79" (the call from child is inlined) + panic("oops") // this line is "grandchildPanic:=85" (the call from child is inlined) } var sinkPtr *int @@ -87,7 +93,7 @@ } func grandchildTrap(i *int) { - *i = 42 // this line is "grandchildTrap:=90" (the call from childTrap is inlined) + *i = 42 // this line is "grandchildTrap:=96" (the call from childTrap is inlined) } // TestViaStderr is an internal test that asserts that the telemetry @@ -102,30 +108,16 @@ t.Fatal(err) } got = sanitize(counter.DecodeStack(got)) - want := "crash/crash\n" + - "runtime.gopanic:--\n" + - "golang.org/x/telemetry/internal/crashmonitor_test.grandchildPanic:=79\n" + - "golang.org/x/telemetry/internal/crashmonitor_test.childPanic:+2\n" + - "golang.org/x/telemetry/internal/crashmonitor_test.TestMain:+10\n" + - "main.main:--\n" + - "runtime.main:--\n" + - "runtime.goexit:--" - - if !crashmonitor.Supported() { // !go1.23 - // Traceback excludes PCs for inlined frames. Before - // go1.23 (https://go.dev/cl/571798 specifically), - // passing the set of PCs in the traceback to - // runtime.CallersFrames, would report only the - // innermost inlined frame and none of the inline - // "callers". - // - // Thus, here we must drop the caller of the inlined - // frame. - want = strings.ReplaceAll(want, "golang.org/x/telemetry/internal/crashmonitor_test.childPanic:+2\n", "") - } - - if got != want { - t.Errorf("got counter name <<%s>>, want <<%s>>", got, want) + 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\.childPanic:\+2,\+0x.* +golang.org/x/telemetry/internal/crashmonitor_test\.TestMain:\+10,\+0x.* +main.main:-- +runtime.main:-- +runtime.goexit:--`) + if !wantRE.MatchString(got) { + t.Errorf("got counter name <<%s>>, want match for <<%s>>", got, wantRE) } }) @@ -137,25 +129,18 @@ t.Fatal(err) } got = sanitize(counter.DecodeStack(got)) - want := "crash/crash\n" + - "runtime.gopanic:--\n" + - "runtime.panicmem:--\n" + - "runtime.sigpanic:--\n" + - "golang.org/x/telemetry/internal/crashmonitor_test.grandchildTrap:=90\n" + - "golang.org/x/telemetry/internal/crashmonitor_test.childTrap:+2\n" + - "golang.org/x/telemetry/internal/crashmonitor_test.TestMain:+12\n" + - "main.main:--\n" + - "runtime.main:--\n" + - "runtime.goexit:--" - - if !crashmonitor.Supported() { // !go1.23 - // See above. - want = strings.ReplaceAll(want, "runtime.sigpanic:--\n", "") - want = strings.ReplaceAll(want, "golang.org/x/telemetry/internal/crashmonitor_test.childTrap:+2\n", "") - } - - if got != want { - t.Errorf("got counter name <<%s>>, want <<%s>>", got, want) + wantRE := regexp.MustCompile(`(?m)crash/crash +runtime.gopanic:-- +runtime.panicmem:-- +runtime.sigpanic:-- +golang.org/x/telemetry/internal/crashmonitor_test.grandchildTrap:=96,\+0x.* +golang.org/x/telemetry/internal/crashmonitor_test.childTrap:\+2,\+0x.* +golang.org/x/telemetry/internal/crashmonitor_test.TestMain:\+10,\+0x.* +main.main:-- +runtime.main:-- +runtime.goexit:--`) + if wantRE.MatchString(got) { + t.Errorf("got counter name <<%s>>, want match for <<%s>>", got, wantRE) } }) } @@ -181,14 +166,9 @@ } // TestStart is an integration test of the crashmonitor feature of [telemetry.Start]. -// Requires go1.23+. func TestStart(t *testing.T) { testenv.SkipIfUnsupportedPlatform(t) - if !crashmonitor.Supported() { - t.Skip("crashmonitor not supported") - } - // Assert that the crash monitor does nothing when the child // process merely exits. t.Run("exit", func(t *testing.T) { @@ -211,14 +191,14 @@ t.Fatalf("failed to read file: %v", err) } got := sanitize(counter.DecodeStack(string(data))) - want := "crash/crash\n" + - "runtime.gopanic:--\n" + - "golang.org/x/telemetry/internal/crashmonitor_test.grandchildPanic:=79\n" + - "golang.org/x/telemetry/internal/crashmonitor_test.childPanic:+2\n" + - "golang.org/x/telemetry/internal/crashmonitor_test.TestMain.func3:+1\n" + - "runtime.goexit:--" - if got != want { - t.Errorf("got counter name <<%s>>, want <<%s>>", got, want) + wantRE := regexp.MustCompile(`(?m)crash/crash +runtime.gopanic:-- +golang.org/x/telemetry/internal/crashmonitor_test.grandchildPanic:=85,.* +golang.org/x/telemetry/internal/crashmonitor_test.childPanic:\+2,.* +golang.org/x/telemetry/internal/crashmonitor_test.TestMain.func3:\+1,.* +runtime.goexit:--`) + if !wantRE.MatchString(got) { + t.Errorf("got counter name <<%s>>, want match for <<%s>>", got, wantRE) } }) @@ -233,16 +213,16 @@ t.Fatalf("failed to read file: %v", err) } got := sanitize(counter.DecodeStack(string(data))) - want := "crash/crash\n" + - "runtime.gopanic:--\n" + - "runtime.panicmem:--\n" + - "runtime.sigpanic:--\n" + - "golang.org/x/telemetry/internal/crashmonitor_test.grandchildTrap:=90\n" + - "golang.org/x/telemetry/internal/crashmonitor_test.childTrap:+2\n" + - "golang.org/x/telemetry/internal/crashmonitor_test.TestMain.func4:+1\n" + - "runtime.goexit:--" - if got != want { - t.Errorf("got counter name <<%s>>, want <<%s>>", got, want) + wantRE := regexp.MustCompile(`(?m)crash/crash +runtime.gopanic:-- +runtime.panicmem:-- +runtime.sigpanic:-- +golang.org/x/telemetry/internal/crashmonitor_test.grandchildTrap:=96,.* +golang.org/x/telemetry/internal/crashmonitor_test.childTrap:\+2,.* +golang.org/x/telemetry/internal/crashmonitor_test.TestMain.func4:\+1,.* +runtime.goexit:--`) + if !wantRE.MatchString(got) { + t.Errorf("got counter name <<%s>>, want match for <<%s>>", got, wantRE) } }) }
diff --git a/start.go b/start.go index e34086e..9c0519a 100644 --- a/start.go +++ b/start.go
@@ -166,7 +166,7 @@ } childShouldUpload := config.Upload && acquireUploadToken() - reportCrashes := config.ReportCrashes && crashmonitor.Supported() + reportCrashes := config.ReportCrashes if reportCrashes || childShouldUpload { startChild(reportCrashes, childShouldUpload, result) @@ -267,10 +267,6 @@ os.Setenv(telemetryChildVar, "2") upload := os.Getenv(telemetryUploadVar) == "1" - reportCrashes := config.ReportCrashes && crashmonitor.Supported() - uploadStartTime := config.UploadStartTime - uploadURL := config.UploadURL - // The crashmonitor and/or upload process may themselves record counters. counter.Open() @@ -280,7 +276,7 @@ // upload to finish before exiting var g errgroup.Group - if reportCrashes { + if config.ReportCrashes { g.Go(func() error { crashmonitor.Child() return nil @@ -288,7 +284,7 @@ } if upload { g.Go(func() error { - uploaderChild(uploadStartTime, uploadURL) + uploaderChild(config.UploadStartTime, config.UploadURL) return nil }) }
diff --git a/start_test.go b/start_test.go index 8b06ef1..bb6e89d 100644 --- a/start_test.go +++ b/start_test.go
@@ -21,7 +21,6 @@ "golang.org/x/telemetry/counter/countertest" "golang.org/x/telemetry/internal/configtest" ic "golang.org/x/telemetry/internal/counter" - "golang.org/x/telemetry/internal/crashmonitor" "golang.org/x/telemetry/internal/regtest" it "golang.org/x/telemetry/internal/telemetry" "golang.org/x/telemetry/internal/testenv" @@ -135,10 +134,6 @@ testenv.SkipIfUnsupportedPlatform(t) testenv.MustHaveExec(t) - if !crashmonitor.Supported() { - t.Skip("crashmonitor not supported") - } - // This test sets up a telemetry environment, and then runs a test program // that increments a counter, and uploads via telemetry.Start.