From 18840865d22d58ac97b79c6a972dacada3d9215b Mon Sep 17 00:00:00 2001 From: Michael Pratt Date: Thu, 1 Feb 2024 12:14:41 -0500 Subject: [PATCH] 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 Reviewed-by: Bryan Mills LUCI-TryBot-Result: Go LUCI --- src/runtime/os_windows.go | 2 +- src/runtime/preempt.go | 15 ++------------- src/runtime/signal_unix.go | 2 +- 3 files changed, 4 insertions(+), 15 deletions(-) diff --git a/src/runtime/os_windows.go b/src/runtime/os_windows.go index 50f61c1d54..7e9bbd04f2 100644 --- a/src/runtime/os_windows.go +++ b/src/runtime/os_windows.go @@ -1298,7 +1298,7 @@ func preemptM(mp *m) { // 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 { diff --git a/src/runtime/preempt.go b/src/runtime/preempt.go index aedf7f3ff5..82d85cd707 100644 --- a/src/runtime/preempt.go +++ b/src/runtime/preempt.go @@ -359,11 +359,7 @@ func wantAsyncPreempt(gp *g) bool { // 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 @@ func isAsyncSafePoint(gp *g, pc, sp, lr uintptr, noResume bool) (bool, uintptr) } // Check M state. - if mp.p == 0 || (!canPreemptM(mp) && !noResume) { + if mp.p == 0 || !canPreemptM(mp) { return false, 0 } @@ -389,13 +385,6 @@ func isAsyncSafePoint(gp *g, pc, sp, lr uintptr, noResume bool) (bool, uintptr) // 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 diff --git a/src/runtime/signal_unix.go b/src/runtime/signal_unix.go index 78e71c9086..84391d58ed 100644 --- a/src/runtime/signal_unix.go +++ b/src/runtime/signal_unix.go @@ -342,7 +342,7 @@ func doSigPreempt(gp *g, ctxt *sigctxt) { // 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) } -- 2.48.1