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 <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>

Author: adonovan

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 }

internal/counter/stackcounter.go

@@ -93,12 +93,22 @@ func EncodeStack(pcs []uintptr, prefix string) string {
 			// 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 {

internal/crashmonitor/crash_go123.go

@@ -21,20 +21,13 @@ import (
 	"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 @@ func parseStackPCs(crash string) ([]uintptr, error) {
 				continue
 			}
 
-			pc = pc-parentSentinel+childSentinel
+			pc = pc - parentSentinel + childSentinel
 
 			// If the previous frame was sigpanic, then this frame
 			// was a trap (e.g., SIGSEGV).

internal/crashmonitor/monitor.go

@@ -10,6 +10,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"regexp"
 	"runtime/debug"
 	"strings"
 	"testing"
@@ -21,6 +22,11 @@ import (
 	"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 childPanic() {
 }
 
 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 childTrap() {
 }
 
 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 @@ func TestViaStderr(t *testing.T) {
 			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 @@ func TestViaStderr(t *testing.T) {
 			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 @@ func waitForExitFile(t *testing.T, exitFile string) {
 }
 
 // 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 @@ func TestStart(t *testing.T) {
 			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 @@ func TestStart(t *testing.T) {
 			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)
 		}
 	})
 }

internal/crashmonitor/monitor_test.go

@@ -166,7 +166,7 @@ func parent(config Config) *StartResult {
 	}
 
 	childShouldUpload := config.Upload && acquireUploadToken()
-	reportCrashes := config.ReportCrashes && crashmonitor.Supported()
+	reportCrashes := config.ReportCrashes
 
 	if reportCrashes || childShouldUpload {
 		startChild(reportCrashes, childShouldUpload, result)
@@ -267,10 +267,6 @@ func child(config Config) {
 	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,15 +276,15 @@ func child(config Config) {
 	// upload to finish before exiting
 	var g errgroup.Group
 
-	if reportCrashes {
+	if config.ReportCrashes {
 		g.Go(func() error {
 			crashmonitor.Child()
 			return nil
 		})
 	}
 	if upload {
 		g.Go(func() error {
-			uploaderChild(uploadStartTime, uploadURL)
+			uploaderChild(config.UploadStartTime, config.UploadURL)
 			return nil
 		})
 	}

start.go

@@ -21,7 +21,6 @@ import (
 	"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 @@ func TestStart(t *testing.T) {
 	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.