runtime: account for missing frame pointer in preamble

If a goroutine is synchronously preempted, then taking a
frame-pointer-based stack trace at that preemption will skip PC of the
caller of the function which called into morestack. This happens because
the frame pointer is pushed to the stack after the preamble, leaving the
stack in an odd state for frame pointer unwinding.

Deal with this by marking a goroutine as synchronously preempted and
using that signal to load the missing PC from the stack. On LR platforms
this is available in gp.sched.lr. On non-LR platforms like x86, it's at
gp.sched.sp, because there are no args, no locals, and no frame pointer
pushed to the SP yet.

For #68090.

Change-Id: I73a1206d8b84eecb8a96dbe727195da30088f288
Reviewed-on: https://go-review.googlesource.com/c/go/+/684435
Reviewed-by: Cherry Mui <cherryyz@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Nick Ripley <nick.ripley@datadoghq.com>

diff --git a/src/internal/trace/testdata/testprog/stacks.go b/src/internal/trace/testdata/testprog/stacks.go
index e64bc86844c4c109794f4ba38751d27f88aec43c..478daa0d941b2bc4a8b0336e108b9fd9835ad780 100644 (file)
--- a/src/internal/trace/testdata/testprog/stacks.go
+++ b/src/internal/trace/testdata/testprog/stacks.go
@@ -97,6 +97,11 @@ func main() {
                rp.Read(data[:])
                pipeReadDone <- true
        }()
+       go func() { // func12
+               for {
+                       syncPreemptPoint()
+               }
+       }()
 
        time.Sleep(100 * time.Millisecond)
        runtime.GC()
@@ -127,3 +132,12 @@ func main() {
 
        runtime.GOMAXPROCS(oldGoMaxProcs)
 }
+
+//go:noinline
+func syncPreemptPoint() {
+       if never {
+               syncPreemptPoint()
+       }
+}
+
+var never bool

diff --git a/src/internal/trace/trace_test.go b/src/internal/trace/trace_test.go
index eaf194cf077e76fa79c7ff8ff97f88c8f11c5203..44b70553440242ba2844969191e3b6a3a0675614 100644 (file)
--- a/src/internal/trace/trace_test.go
+++ b/src/internal/trace/trace_test.go
@@ -326,7 +326,8 @@ func TestTraceStacks(t *testing.T) {
                const mainLine = 21
                want := []evDesc{
                        {trace.EventStateTransition, "Goroutine Running->Runnable", []frame{
-                               {"main.main", mainLine + 82},
+                               {"runtime.Gosched", 0},
+                               {"main.main", mainLine + 87},
                        }},
                        {trace.EventStateTransition, "Goroutine NotExist->Runnable", []frame{
                                {"main.main", mainLine + 11},
@@ -349,7 +350,7 @@ func TestTraceStacks(t *testing.T) {
                        }},
                        {trace.EventStateTransition, "Goroutine Waiting->Runnable", []frame{
                                {"runtime.chansend1", 0},
-                               {"main.main", mainLine + 84},
+                               {"main.main", mainLine + 89},
                        }},
                        {trace.EventStateTransition, "Goroutine Running->Waiting", []frame{
                                {"runtime.chansend1", 0},
@@ -357,7 +358,7 @@ func TestTraceStacks(t *testing.T) {
                        }},
                        {trace.EventStateTransition, "Goroutine Waiting->Runnable", []frame{
                                {"runtime.chanrecv1", 0},
-                               {"main.main", mainLine + 85},
+                               {"main.main", mainLine + 90},
                        }},
                        {trace.EventStateTransition, "Goroutine Running->Waiting", []frame{
                                {"runtime.selectgo", 0},
@@ -365,7 +366,7 @@ func TestTraceStacks(t *testing.T) {
                        }},
                        {trace.EventStateTransition, "Goroutine Waiting->Runnable", []frame{
                                {"runtime.selectgo", 0},
-                               {"main.main", mainLine + 86},
+                               {"main.main", mainLine + 91},
                        }},
                        {trace.EventStateTransition, "Goroutine Running->Waiting", []frame{
                                {"sync.(*Mutex).Lock", 0},
@@ -382,7 +383,7 @@ func TestTraceStacks(t *testing.T) {
                        {trace.EventStateTransition, "Goroutine Waiting->Runnable", []frame{
                                {"sync.(*WaitGroup).Add", 0},
                                {"sync.(*WaitGroup).Done", 0},
-                               {"main.main", mainLine + 91},
+                               {"main.main", mainLine + 96},
                        }},
                        {trace.EventStateTransition, "Goroutine Running->Waiting", []frame{
                                {"sync.(*Cond).Wait", 0},
@@ -402,6 +403,10 @@ func TestTraceStacks(t *testing.T) {
                                {"runtime.GOMAXPROCS", 0},
                                {"main.main", 0},
                        }},
+                       {trace.EventStateTransition, "Goroutine Running->Runnable", []frame{
+                               {"main.syncPreemptPoint", 0},
+                               {"main.main.func12", 0},
+                       }},
                }
                if !stress {
                        // Only check for this stack if !stress because traceAdvance alone could

diff --git a/src/runtime/proc.go b/src/runtime/proc.go
index 98173084302a33a32e5a1946dff1bf83ae16d487..0376f7812bdad4a2f385a79f8111839b2948bb0f 100644 (file)
--- a/src/runtime/proc.go
+++ b/src/runtime/proc.go
@@ -3307,10 +3307,10 @@ func execute(gp *g, inheritTime bool) {
                tryRecordGoroutineProfile(gp, nil, osyield)
        }
 
-       // Assign gp.m before entering _Grunning so running Gs have an
-       // M.
+       // Assign gp.m before entering _Grunning so running Gs have an M.
        mp.curg = gp
        gp.m = mp
+       gp.syncSafePoint = false // Clear the flag, which may have been set by morestack.
        casgstatus(gp, _Grunnable, _Grunning)
        gp.waitsince = 0
        gp.preempt = false

diff --git a/src/runtime/runtime2.go b/src/runtime/runtime2.go
index 96720846b24559e544e2149e98102334e907328d..49a2ba27525d73f3662b3ebe9db5470c6c01fd33 100644 (file)
--- a/src/runtime/runtime2.go
+++ b/src/runtime/runtime2.go
@@ -466,6 +466,7 @@ type g struct {
        runnableTime    int64 // the amount of time spent runnable, cleared when running, only used when tracking
        lockedm         muintptr
        fipsIndicator   uint8
+       syncSafePoint   bool // set if g is stopped at a synchronous safe point.
        runningCleanups atomic.Bool
        sig             uint32
        writebuf        []byte

diff --git a/src/runtime/stack.go b/src/runtime/stack.go
index 4b647976f075b18e7933ab2348bb91aea2173d57..a338708d76fca83dcefd268d9200978aeb74fb3e 100644 (file)
--- a/src/runtime/stack.go
+++ b/src/runtime/stack.go
@@ -1115,6 +1115,9 @@ func newstack() {
                        shrinkstack(gp)
                }
 
+               // Set a flag indicated that we've been synchronously preempted.
+               gp.syncSafePoint = true
+
                if gp.preemptStop {
                        preemptPark(gp) // never returns
                }

diff --git a/src/runtime/traceruntime.go b/src/runtime/traceruntime.go
index 39adeb4c07ea37608478c86dc0ef308ab5b04a11..a2775a3427194373f47cd362d75de3495942db85 100644 (file)
--- a/src/runtime/traceruntime.go
+++ b/src/runtime/traceruntime.go
@@ -457,7 +457,7 @@ func (tl traceLocker) GoPreempt() {
 
 // GoStop emits a GoStop event with the provided reason.
 func (tl traceLocker) GoStop(reason traceGoStopReason) {
-       tl.eventWriter(tracev2.GoRunning, tracev2.ProcRunning).event(tracev2.EvGoStop, traceArg(trace.goStopReasons[tl.gen%2][reason]), tl.stack(1))
+       tl.eventWriter(tracev2.GoRunning, tracev2.ProcRunning).event(tracev2.EvGoStop, traceArg(trace.goStopReasons[tl.gen%2][reason]), tl.stack(0))
 }
 
 // GoPark emits a GoBlock event with the provided reason.

diff --git a/src/runtime/tracestack.go b/src/runtime/tracestack.go
index bca2d0a88deec4be42a9549280f29c172d3c9f1f..2ee68c85f0e80c2a19e9afca3eb34f3a5c586769 100644 (file)
--- a/src/runtime/tracestack.go
+++ b/src/runtime/tracestack.go
@@ -109,7 +109,22 @@ func traceStack(skip int, gp *g, gen uintptr) uint64 {
                                nstk += 1 + fpTracebackPCs(unsafe.Pointer(gp.syscallbp), pcBuf[2:])
                        } else {
                                pcBuf[1] = gp.sched.pc
-                               nstk += 1 + fpTracebackPCs(unsafe.Pointer(gp.sched.bp), pcBuf[2:])
+                               if gp.syncSafePoint {
+                                       // We're stopped in morestack, which is an odd state because gp.sched.bp
+                                       // refers to our parent frame, since we haven't had the chance to push our
+                                       // frame pointer to the stack yet. If we just start walking from gp.sched.bp,
+                                       // we'll skip a frame as a result. Luckily, we can find the PC we want right
+                                       // at gp.sched.sp on non-LR platforms, and we have it directly on LR platforms.
+                                       // See issue go.dev/issue/68090.
+                                       if usesLR {
+                                               pcBuf[2] = gp.sched.lr
+                                       } else {
+                                               pcBuf[2] = *(*uintptr)(unsafe.Pointer(gp.sched.sp))
+                                       }
+                                       nstk += 2 + fpTracebackPCs(unsafe.Pointer(gp.sched.bp), pcBuf[3:])
+                               } else {
+                                       nstk += 1 + fpTracebackPCs(unsafe.Pointer(gp.sched.bp), pcBuf[2:])
+                               }
                        }
                }
        }