From: Austin Clements <austin@google.com>
Date: Wed, 3 Jan 2024 15:41:09 +0000 (-0500)
Subject: runtime: simplify freedefer logic
X-Git-Tag: go1.23rc1~1422
X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=5a61d8d36be864c0c87cdad9118c801b2ce17331;p=gostls13.git

runtime: simplify freedefer logic

Currently, freedefer's API forces a subtle and fragile situation. It
requires that the caller unlink the _defer from the G list, but
freedefer itself is responsible for zeroing some _defer fields. In the
window between these two steps, we have to prevent stack growth
because stack growth walks the defer list (which no longer contains
the unlinked defer) to adjust pointers, and would thus leave an
unadjusted and potentially invalid pointer behind in the _defer before
freedefer zeroes it.

This setup puts part of this subtle responsibility on the caller and
also means freedefer must be nosplit, which forces other shenanigans
to avoid nosplit overflows.

We can simplify all of this by replacing freedefer with a new popDefer
function that's responsible for both unlinking and zeroing the _defer,
in addition to freeing it.

Some history: prior to regabi, defer records contained their argument
frame, which deferreturn copied to the stack before freeing the defer
record (and subsequently running the defer). Since that argument frame
didn't have a valid stack map until we ran the deferred function, the
non-preemptible window was much larger and more difficult to isolate.
Now we use normal closure calls to capture defer state and call the
defer, so the non-preemptible window is narrowed to just the unlinking
step.

Change-Id: I7cf95ba18e1e2e7d73f616b9ed9fb38f5e725d72
Reviewed-on: https://go-review.googlesource.com/c/go/+/553696
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Auto-Submit: Austin Clements <austin@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
---

diff --git a/src/runtime/panic.go b/src/runtime/panic.go
index 36d658aa4c..e6d1c5d908 100644
--- a/src/runtime/panic.go
+++ b/src/runtime/panic.go
@@ -420,17 +420,18 @@ func deferprocat(fn func(), frame any) {
 	return0()
 }
 
-// deferconvert converts a rangefunc defer list into an ordinary list.
+// deferconvert converts the rangefunc defer list of d0 into an ordinary list
+// following d0.
 // See the doc comment for deferrangefunc for details.
-func deferconvert(d *_defer) *_defer {
-	head := d.head
+func deferconvert(d0 *_defer) {
+	head := d0.head
 	if raceenabled {
 		racereadpc(unsafe.Pointer(head), getcallerpc(), abi.FuncPCABIInternal(deferconvert))
 	}
-	tail := d.link
-	d.rangefunc = false
-	d0 := d
+	tail := d0.link
+	d0.rangefunc = false
 
+	var d *_defer
 	for {
 		d = head.Load()
 		if head.CompareAndSwap(d, badDefer()) {
@@ -438,8 +439,7 @@ func deferconvert(d *_defer) *_defer {
 		}
 	}
 	if d == nil {
-		freedefer(d0)
-		return tail
+		return
 	}
 	for d1 := d; ; d1 = d1.link {
 		d1.sp = d0.sp
@@ -449,8 +449,8 @@ func deferconvert(d *_defer) *_defer {
 			break
 		}
 	}
-	freedefer(d0)
-	return d
+	d0.link = d
+	return
 }
 
 // deferprocStack queues a new deferred function with a defer record on the stack.
@@ -528,22 +528,18 @@ func newdefer() *_defer {
 	return d
 }
 
-// Free the given defer.
-// The defer cannot be used after this call.
-//
-// This is nosplit because the incoming defer is in a perilous state.
-// It's not on any defer list, so stack copying won't adjust stack
-// pointers in it (namely, d.link). Hence, if we were to copy the
-// stack, d could then contain a stale pointer.
-//
-//go:nosplit
-func freedefer(d *_defer) {
+// popDefer pops the head of gp's defer list and frees it.
+func popDefer(gp *g) {
+	d := gp._defer
+	d.fn = nil // Can in theory point to the stack
+	// We must not copy the stack between the updating gp._defer and setting
+	// d.link to nil. Between these two steps, d is not on any defer list, so
+	// stack copying won't adjust stack pointers in it (namely, d.link). Hence,
+	// if we were to copy the stack, d could then contain a stale pointer.
+	gp._defer = d.link
 	d.link = nil
 	// After this point we can copy the stack.
 
-	if d.fn != nil {
-		freedeferfn()
-	}
 	if !d.heap {
 		return
 	}
@@ -579,13 +575,6 @@ func freedefer(d *_defer) {
 	mp, pp = nil, nil
 }
 
-// Separate function so that it can split stack.
-// Windows otherwise runs out of stack space.
-func freedeferfn() {
-	// fn must be cleared before d is unlinked from gp.
-	throw("freedefer with d.fn != nil")
-}
-
 // deferreturn runs deferred functions for the caller's frame.
 // The compiler inserts a call to this at the end of any
 // function which calls defer.
@@ -876,12 +865,12 @@ func (p *_panic) nextDefer() (func(), bool) {
 	Recheck:
 		if d := gp._defer; d != nil && d.sp == uintptr(p.sp) {
 			if d.rangefunc {
-				gp._defer = deferconvert(d)
+				deferconvert(d)
+				popDefer(gp)
 				goto Recheck
 			}
 
 			fn := d.fn
-			d.fn = nil
 
 			// TODO(mdempsky): Instead of having each deferproc call have
 			// its own "deferreturn(); return" sequence, we should just make
@@ -889,8 +878,7 @@ func (p *_panic) nextDefer() (func(), bool) {
 			p.retpc = d.pc
 
 			// Unlink and free.
-			gp._defer = d.link
-			freedefer(d)
+			popDefer(gp)
 
 			return fn, true
 		}