0001-runtime-use-sched.bp-for-unwinding-trace-events-with.patch

From 08b6b8208c07c3f764da8a8fbbc01d0b1573748a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Felix=20Geisend=C3=B6rfer?=
 <felix.geisendoerfer@datadoghq.com>
Date: Fri, 10 Mar 2023 11:04:19 +0100
Subject: [PATCH] runtime: use sched.bp for unwinding trace events with frame
 pointers

Depends on CL 472195.

Change-Id: Ibe91debf04569d18ccb2eb6497bfebdd1eaeaf74
---
 src/runtime/trace.go                  | 18 +++++++++---------
 src/runtime/trace/trace_stack_test.go | 23 +++++++----------------
 2 files changed, 16 insertions(+), 25 deletions(-)

diff --git a/src/runtime/trace.go b/src/runtime/trace.go
index 0f740607c8..06410d1ca6 100644
--- a/src/runtime/trace.go
+++ b/src/runtime/trace.go
@@ -893,16 +893,13 @@ func traceStackID(mp *m, pcBuf []uintptr, skip int) uint64 {
 			nstk = gcallers(curgp, skip, pcBuf[1:]) + 1
 		}
 	} else {
-		// Fast path: Unwind using frame pointers.
+		// Fast path: Unwind using frame pointers. We skip one less frame than the
+		// slow path because we start unwinding from the current frame rather than
+		// the callers/gcallers callee.
 		if curgp == gp {
 			nstk = fpTracebackPCs(getcallerfp(), skip, pcBuf)
 		} else if curgp != nil {
-			// TODO: unwinding from curgp.sched.bp would be a better approach,
-			// but it's causing a lot of TestTraceSymbolize failures that are
-			// difficult to debug and fix. skip+4 is a workaround to hide g0
-			// frames for now, but it's brittle because the number of frames
-			// leading up to traceStackID is variable, see traceGoSysCall.
-			nstk = fpTracebackPCs(getcallerfp(), skip+4, pcBuf)
+			nstk = fpTracebackPCs(curgp.sched.bp, skip-1, pcBuf)
 		}
 	}
 	if nstk > 0 {
@@ -1564,8 +1561,11 @@ func traceGoSysCall() {
 	if tracefpunwindoff() {
 		traceEvent(traceEvGoSysCall, 1)
 	} else {
-		// TODO: remove this workaround, see traceStackID
-		traceEvent(traceEvGoSysCall, 2)
+		// Skip 3 more frames than the default unwinder which starts from the the
+		// pc/sp captured in entersyscall, so it needs to skip less frames.
+		// TODO: Should we capture bp in entersyscall to keep the skip value
+		// consistent here?
+		traceEvent(traceEvGoSysCall, 4)
 	}
 }
 
diff --git a/src/runtime/trace/trace_stack_test.go b/src/runtime/trace/trace_stack_test.go
index dd684b4c66..521598fad2 100644
--- a/src/runtime/trace/trace_stack_test.go
+++ b/src/runtime/trace/trace_stack_test.go
@@ -228,6 +228,13 @@ func TestTraceSymbolize(t *testing.T) {
 			{"runtime/trace_test.TestTraceSymbolize", 0},
 			{"testing.tRunner", 0},
 		}},
+		{trace.EvGomaxprocs, []frame{
+			{"runtime.startTheWorld", 0}, // this is when the current gomaxprocs is logged.
+			{"runtime.startTheWorldGC", 0},
+			{"runtime.GOMAXPROCS", 0},
+			{"runtime/trace_test.TestTraceSymbolize", 0},
+			{"testing.tRunner", 0},
+		}},
 	}
 
 	if tracefpunwindoff() {
@@ -236,23 +243,7 @@ func TestTraceSymbolize(t *testing.T) {
 				{"runtime/trace_test.TestTraceSymbolize", 112}, // runtime.Gosched call gets inlined and mcall doesn't push frame pointer (TODO: make mcall push fp?)
 				{"testing.tRunner", 0},
 			}},
-			eventDesc{trace.EvGomaxprocs, []frame{
-				{"runtime.startTheWorld", 0}, // this is when the current gomaxprocs is logged.
-				{"runtime.startTheWorldGC", 0},
-				{"runtime.GOMAXPROCS", 0},
-				{"runtime/trace_test.TestTraceSymbolize", 0},
-				{"testing.tRunner", 0},
-			}},
 		)
-	} else {
-		want = append(want, eventDesc{trace.EvGomaxprocs, []frame{
-			{"runtime.startTheWorld.func1", 0}, // this is when the current gomaxprocs is logged.
-			{"runtime.systemstack", 0},         // systemstack doesn't push rbp, so no runtime.startTheWorld frame
-			{"runtime.startTheWorldGC", 0},
-			{"runtime.GOMAXPROCS", 0},
-			{"runtime/trace_test.TestTraceSymbolize", 0},
-			{"testing.tRunner", 0},
-		}})
 	}
 
 	// Stacks for the following events are OS-dependent due to OS-specific code in net package.
-- 
2.39.0