runtime: allow omitting virtual PCs from runtime.CallersFrames input

This makes CL 561635's test pass without any changes to the
traceback textual format.

The test in this CL is copied identically from CL 561635.

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

src/runtime/crash_test.go

@@ -14,6 +14,7 @@ import (
 	"internal/testenv"
 	tracev2 "internal/trace/v2"
 	"io"
+	"log"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -28,7 +29,19 @@ import (
 
 var toRemove []string
 
+const entrypointVar = "RUNTIME_TEST_ENTRYPOINT"
+
 func TestMain(m *testing.M) {
+	switch entrypoint := os.Getenv(entrypointVar); entrypoint {
+	case "crash":
+		crash()
+		panic("unreachable")
+	default:
+		log.Fatalf("invalid %s: %q", entrypointVar, entrypoint)
+	case "":
+		// fall through to normal behavior
+	}
+
 	_, coreErrBefore := os.Stat("core")
 
 	status := m.Run()

src/runtime/symtab.go

@@ -18,6 +18,9 @@ type Frames struct {
 	// callers is a slice of PCs that have not yet been expanded to frames.
 	callers []uintptr
 
+	// nextPC is a next PC to expand ahead of processing callers.
+	nextPC uintptr
+
 	// frames is a slice of Frames that have yet to be returned.
 	frames     []Frame
 	frameStore [2]Frame
@@ -96,8 +99,12 @@ func (ci *Frames) Next() (frame Frame, more bool) {
 		if len(ci.callers) == 0 {
 			break
 		}
-		pc := ci.callers[0]
-		ci.callers = ci.callers[1:]
+		var pc uintptr
+		if ci.nextPC != 0 {
+			pc, ci.nextPC = ci.nextPC, 0
+		} else {
+			pc, ci.callers = ci.callers[0], ci.callers[1:]
+		}
 		funcInfo := findfunc(pc)
 		if !funcInfo.valid() {
 			if cgoSymbolizer != nil {
@@ -125,6 +132,29 @@ func (ci *Frames) Next() (frame Frame, more bool) {
 			// Note: entry is not modified. It always refers to a real frame, not an inlined one.
 			// File/line from funcline1 below are already correct.
 			f = nil
+
+			// When CallersFrame is invoked using the PC list returned by Callers,
+			// the PC list includes virtual PCs corresponding to each outer frame
+			// around an innermost real inlined PC.
+			// We also want to support code passing in a PC list extracted from a
+			// stack trace, and there only the real PCs are printed, not the virtual ones.
+			// So check to see if the implied virtual PC for this PC (obtained from the
+			// unwinder itself) is the next PC in ci.callers. If not, insert it.
+			// The +1 here correspond to the pc-- above: the output of Callers
+			// and therefore the input to CallersFrames is return PCs from the stack;
+			// The pc-- backs up into the CALL instruction (not the first byte of the CALL
+			// instruction, but good enough to find it nonetheless).
+			// There are no cycles in implied virtual PCs (some number of frames were
+			// inlined, but that number is finite), so this unpacking cannot cause an infinite loop.
+			for unext := u.next(uf); unext.valid() && len(ci.callers) > 0 && ci.callers[0] != unext.pc+1; unext = u.next(unext) {
+				snext := u.srcFunc(unext)
+				if snext.funcID == abi.FuncIDWrapper && elideWrapperCalling(sf.funcID) {
+					// Skip, because tracebackPCs (inside runtime.Callers) would too.
+					continue
+				}
+				ci.nextPC = unext.pc + 1
+				break
+			}
 		}
 		ci.frames = append(ci.frames, Frame{
 			PC:        pc,

src/runtime/traceback.go

@@ -632,8 +632,11 @@ func tracebackPCs(u *unwinder, skip int, pcBuf []uintptr) int {
 				skip--
 			} else {
 				// Callers expect the pc buffer to contain return addresses
-				// and do the -1 themselves, so we add 1 to the call PC to
-				// create a return PC.
+				// and do the -1 themselves, so we add 1 to the call pc to
+				// create a "return pc". Since there is no actual call, here
+				// "return pc" just means a pc you subtract 1 from to get
+				// the pc of the "call". The actual no-op we insert may or
+				// may not be 1 byte.
 				pcBuf[n] = uf.pc + 1
 				n++
 			}

src/runtime/traceback_system_test.go

@@ -0,0 +1,238 @@
+// 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.
+
+package runtime_test
+
+// This test of GOTRACEBACK=system has its own file,
+// to minimize line-number perturbation.
+
+import (
+	"bytes"
+	"fmt"
+	"internal/testenv"
+	"io"
+	"os"
+	"path/filepath"
+	"reflect"
+	"runtime"
+	"runtime/debug"
+	"strconv"
+	"strings"
+	"testing"
+)
+
+// This is the entrypoint of the child process used by
+// TestTracebackSystem. It prints a crash report to stdout.
+func crash() {
+	// Ensure that we get pc=0x%x values in the traceback.
+	debug.SetTraceback("system")
+	writeSentinel(os.Stdout)
+	debug.SetCrashOutput(os.Stdout)
+
+	go func() {
+		// This call is typically inlined.
+		child()
+	}()
+	select {}
+}
+
+func child() {
+	grandchild()
+}
+
+func grandchild() {
+	// Write runtime.Caller's view of the stack to stderr, for debugging.
+	var pcs [16]uintptr
+	n := runtime.Callers(1, pcs[:])
+	io.WriteString(os.Stderr, formatStack(pcs[:n]))
+
+	// Cause the crash report to be written to stdout.
+	panic("oops")
+}
+
+// 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
+// can be parsed and fed to runtime.CallersFrames to obtain accurate
+// information about the logical call stack, even in the presence of
+// inlining.
+//
+// The test is a distillation of the crash monitor in
+// golang.org/x/telemetry/crashmonitor.
+func TestTracebackSystem(t *testing.T) {
+	testenv.MustHaveExec(t)
+	if runtime.GOOS == "android" {
+		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")
+	cmd.Stdout = new(strings.Builder)
+	// cmd.Stderr = os.Stderr // uncomment to debug, e.g. to see runtime.Caller's view
+	cmd.Run() // expected to crash
+	crash := cmd.Stdout.(*strings.Builder).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")
+	}
+
+	// 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)
+	want := `redacted.go:0: runtime.gopanic
+traceback_system_test.go:51: runtime_test.grandchild: 	panic("oops")
+traceback_system_test.go:41: runtime_test.child: 	grandchild()
+traceback_system_test.go:35: runtime_test.crash.func1: 		child()
+redacted.go:0: runtime.goexit`
+	if strings.TrimSpace(got) != strings.TrimSpace(want) {
+		t.Errorf("got:\n%swant:\n%s", got, want)
+	}
+}
+
+// parseStackPCs parses the parent process's program counters for the
+// first running goroutine out of a GOTRACEBACK=system traceback,
+// adjusting them so that they are valid for the child process's text
+// segment.
+//
+// This function returns only program counter values, ensuring that
+// there is no possibility of strings from the crash report (which may
+// contain PII) leaking into the telemetry system.
+//
+// (Copied from golang.org/x/telemetry/crashmonitor.parseStackPCs.)
+func parseStackPCs(crash string) ([]uintptr, error) {
+	// getPC parses the PC out of a line of the form:
+	//     \tFILE:LINE +0xRELPC sp=... fp=... pc=...
+	getPC := func(line string) (uint64, error) {
+		_, pcstr, ok := strings.Cut(line, " pc=") // e.g. pc=0x%x
+		if !ok {
+			return 0, fmt.Errorf("no pc= for stack frame: %s", line)
+		}
+		return strconv.ParseUint(pcstr, 0, 64) // 0 => allow 0x prefix
+	}
+
+	var (
+		pcs            []uintptr
+		parentSentinel uint64
+		childSentinel  = sentinel()
+		on             = false // are we in the first running goroutine?
+		lines          = strings.Split(crash, "\n")
+	)
+	for i := 0; i < len(lines); i++ {
+		line := lines[i]
+
+		// Read sentinel value.
+		if parentSentinel == 0 && strings.HasPrefix(line, "sentinel ") {
+			_, err := fmt.Sscanf(line, "sentinel %x", &parentSentinel)
+			if err != nil {
+				return nil, fmt.Errorf("can't read sentinel line")
+			}
+			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")
+				}
+			}
+			continue
+		}
+
+		// A blank line marks end of a goroutine stack.
+		if line == "" {
+			break
+		}
+
+		// Skip the final "created by SYMBOL in goroutine GID" part.
+		if strings.HasPrefix(line, "created by ") {
+			break
+		}
+
+		// Expect a pair of lines:
+		//   SYMBOL(ARGS)
+		//   \tFILE:LINE +0xRELPC sp=0x%x fp=0x%x pc=0x%x
+		// 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]
+
+		// 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
+		}
+		pcs = append(pcs, uintptr(pc-parentSentinel+childSentinel))
+	}
+	return pcs, nil
+}
+
+// The sentinel function returns its address. The difference between
+// this value as observed by calls in two different processes of the
+// same executable tells us the relative offset of their text segments.
+//
+// It would be nice if SetCrashOutput took care of this as it's fiddly
+// and likely to confuse every user at first.
+func sentinel() uint64 {
+	return uint64(reflect.ValueOf(sentinel).Pointer())
+}
+
+func writeSentinel(out io.Writer) {
+	fmt.Fprintf(out, "sentinel %x\n", sentinel())
+}
+
+// formatStack formats a stack of PC values using the symbol table,
+// redacting information that cannot be relied upon in the test.
+func formatStack(pcs []uintptr) string {
+	// When debugging, show file/line/content of files other than this one.
+	const debug = false
+
+	var buf strings.Builder
+	i := 0
+	frames := runtime.CallersFrames(pcs)
+	for {
+		fr, more := frames.Next()
+		if debug {
+			fmt.Fprintf(&buf, "pc=%x ", pcs[i])
+			i++
+		}
+		if base := filepath.Base(fr.File); base == "traceback_system_test.go" || debug {
+			content, err := os.ReadFile(fr.File)
+			if err != nil {
+				panic(err)
+			}
+			lines := bytes.Split(content, []byte("\n"))
+			fmt.Fprintf(&buf, "%s:%d: %s: %s\n", base, fr.Line, fr.Function, lines[fr.Line-1])
+		} else {
+			// For robustness, don't show file/line for functions from other files.
+			fmt.Fprintf(&buf, "redacted.go:0: %s\n", fr.Function)
+		}
+
+		if !more {
+			break
+		}
+	}
+	return buf.String()
+}