[release-branch.go1.21] runtime: ignore SPWrite on innermost tracebac…

…k frame

Prior to CL 458218, gentraceback ignored the SPWrite function flag on
the innermost frame when doing a precise traceback on the assumption
that precise tracebacks could only be started from the morestack
prologue, and that meant that the innermost function could not have
modified SP yet.

CL 458218 rearranged this logic a bit and unintentionally lost this
particular case. As a result, if traceback starts in an assembly
function that modifies SP (either as a result of stack growth or stack
scanning during a GC preemption), traceback stop at the SPWrite
function and then crash with "traceback did not unwind completely".

Fix this by restoring the earlier special case for when the innermost
frame is SPWrite.

This is a fairly minimal change that should be easy to backport. I
think a more robust change would be to encode this per-PC in the
spdelta table, so it would be clear that we're unwinding from the
morestack prologue and wouldn't rely on a complicated and potentially
fragile set of conditions.

Fixes #62464.

Change-Id: I34f38157631890d33a79d0bd32e32c0fcc2574e4
Reviewed-on: https://go-review.googlesource.com/c/go/+/526100
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Michael Pratt <[email protected]>
Auto-Submit: Heschi Kreinick <[email protected]>

src/runtime/import_test.go

@@ -10,7 +10,7 @@
 //
 // There are a few limitations on runtime package tests that this bridges:
 //
-// 1. Tests use the signature "XTest<name>(t T)". Since runtime can't import
+// 1. Tests use the signature "XTest<name>(t TestingT)". Since runtime can't import
 // testing, test functions can't use testing.T, so instead we have the T
 // interface, which *testing.T satisfies. And we start names with "XTest"
 // because otherwise go test will complain about Test functions with the wrong
@@ -39,3 +39,7 @@ func init() {
 func TestInlineUnwinder(t *testing.T) {
 	runtime.XTestInlineUnwinder(t)
 }
+
+func TestSPWrite(t *testing.T) {
+	runtime.XTestSPWrite(t)
+}

src/runtime/test_amd64.go

@@ -0,0 +1,7 @@
+// Copyright 2023 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
+
+func testSPWrite()

src/runtime/test_amd64.s

@@ -0,0 +1,7 @@
+// Create a large frame to force stack growth. See #62326.
+TEXT ·testSPWrite(SB),0,$16384-0
+	// Write to SP
+	MOVQ SP, AX
+	ANDQ $~0xf, SP
+	MOVQ AX, SP
+	RET

src/runtime/test_stubs.go

@@ -0,0 +1,9 @@
+// Copyright 2023 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 !amd64
+
+package runtime
+
+func testSPWrite() {}

src/runtime/traceback.go

@@ -336,30 +336,40 @@ func (u *unwinder) resolveInternal(innermost, isSyscall bool) {
 	if flag&abi.FuncFlagTopFrame != 0 {
 		// This function marks the top of the stack. Stop the traceback.
 		frame.lr = 0
-	} else if flag&abi.FuncFlagSPWrite != 0 {
+	} else if flag&abi.FuncFlagSPWrite != 0 && (!innermost || u.flags&(unwindPrintErrors|unwindSilentErrors) != 0) {
 		// The function we are in does a write to SP that we don't know
 		// how to encode in the spdelta table. Examples include context
 		// switch routines like runtime.gogo but also any code that switches
 		// to the g0 stack to run host C code.
-		if u.flags&(unwindPrintErrors|unwindSilentErrors) != 0 {
-			// We can't reliably unwind the SP (we might
-			// not even be on the stack we think we are),
-			// so stop the traceback here.
-			frame.lr = 0
-		} else {
-			// For a GC stack traversal, we should only see
-			// an SPWRITE function when it has voluntarily preempted itself on entry
-			// during the stack growth check. In that case, the function has
-			// not yet had a chance to do any writes to SP and is safe to unwind.
-			// isAsyncSafePoint does not allow assembly functions to be async preempted,
-			// and preemptPark double-checks that SPWRITE functions are not async preempted.
-			// So for GC stack traversal, we can safely ignore SPWRITE for the innermost frame,
-			// but farther up the stack we'd better not find any.
-			if !innermost {
-				println("traceback: unexpected SPWRITE function", funcname(f))
+		// We can't reliably unwind the SP (we might not even be on
+		// the stack we think we are), so stop the traceback here.
+		//
+		// The one exception (encoded in the complex condition above) is that
+		// we assume if we're doing a precise traceback, and this is the
+		// innermost frame, that the SPWRITE function voluntarily preempted itself on entry
+		// during the stack growth check. In that case, the function has
+		// not yet had a chance to do any writes to SP and is safe to unwind.
+		// isAsyncSafePoint does not allow assembly functions to be async preempted,
+		// and preemptPark double-checks that SPWRITE functions are not async preempted.
+		// So for GC stack traversal, we can safely ignore SPWRITE for the innermost frame,
+		// but farther up the stack we'd better not find any.
+		// This is somewhat imprecise because we're just guessing that we're in the stack
+		// growth check. It would be better if SPWRITE were encoded in the spdelta
+		// table so we would know for sure that we were still in safe code.
+		//
+		// uSE uPE inn | action
+		//  T   _   _  | frame.lr = 0
+		//  F   T   F  | frame.lr = 0; print
+		//  F   T   T  | frame.lr = 0
+		//  F   F   F  | print; panic
+		//  F   F   T  | ignore SPWrite
+		if u.flags&unwindSilentErrors == 0 && !innermost {
+			println("traceback: unexpected SPWRITE function", funcname(f))
+			if u.flags&unwindPrintErrors == 0 {
 				throw("traceback")
 			}
 		}
+		frame.lr = 0
 	} else {
 		var lrPtr uintptr
 		if usesLR {

src/runtime/tracebackx_test.go

@@ -0,0 +1,18 @@
+// Copyright 2023 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
+
+func XTestSPWrite(t TestingT) {
+	// Test that we can traceback from the stack check prologue of a function
+	// that writes to SP. See #62326.
+
+	// Start a goroutine to minimize the initial stack and ensure we grow the stack.
+	done := make(chan bool)
+	go func() {
+		testSPWrite() // Defined in assembly
+		done <- true
+	}()
+	<-done
+}