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.
