commit 18840865d22d58ac97b79c6a972dacada3d9215b
author Michael Pratt <mpratt@google.com> Thu Feb 01 12:14:41 2024 -0500
committer Michael Pratt <mpratt@google.com> Thu Feb 01 18:09:36 2024 +0000
tree b1dcd22ac5bd795c76b1c21ead28c39228bda078
parent 5b6cd3d0cbd3f3f0a645e37526fabe35a7e75319

Revert "runtime: preempt more aggressively when panicking"

This reverts CL 546135.

Reason for revert: Causes occasional throw during panic

For #65416.

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

src/runtime/os_windows.go

diff --git a/src/runtime/os_windows.go b/src/runtime/os_windows.go
index 50f61c1..7e9bbd0 100644
--- a/src/runtime/os_windows.go
+++ b/src/runtime/os_windows.go
@@ -1298,7 +1298,7 @@
 	// Does it want a preemption and is it safe to preempt?
 	gp := gFromSP(mp, c.sp())
 	if gp != nil && wantAsyncPreempt(gp) {
-		if ok, newpc := isAsyncSafePoint(gp, c.ip(), c.sp(), c.lr(), panicking.Load() != 0); ok {
+		if ok, newpc := isAsyncSafePoint(gp, c.ip(), c.sp(), c.lr()); ok {
 			// Inject call to asyncPreempt
 			targetPC := abi.FuncPCABI0(asyncPreempt)
 			switch GOARCH {

src/runtime/preempt.go

diff --git a/src/runtime/preempt.go b/src/runtime/preempt.go
index aedf7f3..82d85cd 100644
--- a/src/runtime/preempt.go
+++ b/src/runtime/preempt.go
@@ -359,11 +359,7 @@
 // In some cases the PC is safe for asynchronous preemption but it
 // also needs to adjust the resumption PC. The new PC is returned in
 // the second result.
-//
-// If noResume is true, we know we're not going to resume execution
-// on this goroutine (as we're crashing), and thus we can preempt
-// more aggressively.
-func isAsyncSafePoint(gp *g, pc, sp, lr uintptr, noResume bool) (bool, uintptr) {
+func isAsyncSafePoint(gp *g, pc, sp, lr uintptr) (bool, uintptr) {
 	mp := gp.m
 
 	// Only user Gs can have safe-points. We check this first
@@ -374,7 +370,7 @@
 	}
 
 	// Check M state.
-	if mp.p == 0 || (!canPreemptM(mp) && !noResume) {
+	if mp.p == 0 || !canPreemptM(mp) {
 		return false, 0
 	}
 
@@ -389,13 +385,6 @@
 		// Not Go code.
 		return false, 0
 	}
-	if noResume && f.flag&abi.FuncFlagAsm == 0 {
-		// We're not going to resume execution and not going to scan the
-		// stack for GC, so we don't care whether it is a safe point, and
-		// also don't care the resumption PC.
-		// TODO: maybe we can preempt non-SPWRITE assembly functions?
-		return true, pc
-	}
 	if (GOARCH == "mips" || GOARCH == "mipsle" || GOARCH == "mips64" || GOARCH == "mips64le") && lr == pc+8 && funcspdelta(f, pc) == 0 {
 		// We probably stopped at a half-executed CALL instruction,
 		// where the LR is updated but the PC has not. If we preempt

src/runtime/signal_unix.go

diff --git a/src/runtime/signal_unix.go b/src/runtime/signal_unix.go
index 78e71c9..84391d5 100644
--- a/src/runtime/signal_unix.go
+++ b/src/runtime/signal_unix.go
@@ -342,7 +342,7 @@
 	// Check if this G wants to be preempted and is safe to
 	// preempt.
 	if wantAsyncPreempt(gp) {
-		if ok, newpc := isAsyncSafePoint(gp, ctxt.sigpc(), ctxt.sigsp(), ctxt.siglr(), panicking.Load() != 0); ok {
+		if ok, newpc := isAsyncSafePoint(gp, ctxt.sigpc(), ctxt.sigsp(), ctxt.siglr()); ok {
 			// Adjust the PC and inject a call to asyncPreempt.
 			ctxt.pushCall(abi.FuncPCABI0(asyncPreempt), newpc)
 		}