runtime: make sure to remove open-coded defer entries in all cases after a recover

We add entries to the defer list at panic/goexit time on-the-fly for
frames with open-coded defers. We do this so that we can correctly
process open-coded defers and non-open-coded defers in the correct order
during panics/goexits. But we need to remove entries for open-coded
defers from the defer list when there is a recover, since those entries
may never get removed otherwise and will get stale, since their
corresponding defers may now be processed normally (inline).

This bug here is that we were only removing higher-up stale entries
during a recover if all defers in the current frame were done. But we
could have more defers in the current frame (as the new test case
shows). In this case, we need to leave the current defer entry around
for use by deferreturn, but still remove any stale entries further along
the chain.

For bug 43921, simple change that we should abort the removal loop for
any defer entry that is started (i.e. in process by a still
not-recovered outer panic), even if it is not an open-coded defer.

This change does not fix bug 43920, which looks to be a more complex fix.

Fixes #43882
Fixes #43921

Change-Id: Ie05b2fa26973aa26b25c8899a2abc916090ee4f5
Reviewed-on: https://go-review.googlesource.com/c/go/+/286712
Run-TryBot: Dan Scales <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Reviewed-by: Matthew Dempsky <[email protected]>
Reviewed-by: Keith Randall <[email protected]>
Trust: Dan Scales <[email protected]>

Author: danscales

src/runtime/crash_test.go

@@ -294,6 +294,18 @@ func TestRecursivePanic4(t *testing.T) {
 
 }
 
+func TestRecursivePanic5(t *testing.T) {
+	output := runTestProg(t, "testprog", "RecursivePanic5")
+	want := `first panic
+second panic
+panic: third panic
+`
+	if !strings.HasPrefix(output, want) {
+		t.Fatalf("output does not start with %q:\n%s", want, output)
+	}
+
+}
+
 func TestGoexitCrash(t *testing.T) {
 	// External linking brings in cgo, causing deadlock detection not working.
 	testenv.MustInternalLink(t)

src/runtime/defer_test.go

@@ -410,3 +410,31 @@ func rec1(max int) {
 		rec1(max - 1)
 	}
 }
+
+func TestIssue43921(t *testing.T) {
+	defer func() {
+		expect(t, 1, recover())
+	}()
+	func() {
+		// Prevent open-coded defers
+		for {
+			defer func() {}()
+			break
+		}
+
+		defer func() {
+			defer func() {
+				expect(t, 4, recover())
+			}()
+			panic(4)
+		}()
+		panic(1)
+
+	}()
+}
+
+func expect(t *testing.T, n int, err interface{}) {
+	if n != err {
+		t.Fatalf("have %v, want %v", err, n)
+	}
+}

src/runtime/panic.go

@@ -1000,37 +1000,42 @@ func gopanic(e interface{}) {
 			}
 			atomic.Xadd(&runningPanicDefers, -1)
 
-			if done {
-				// Remove any remaining non-started, open-coded
-				// defer entries after a recover, since the
-				// corresponding defers will be executed normally
-				// (inline). Any such entry will become stale once
-				// we run the corresponding defers inline and exit
-				// the associated stack frame.
-				d := gp._defer
-				var prev *_defer
-				for d != nil {
-					if d.openDefer {
-						if d.started {
-							// This defer is started but we
-							// are in the middle of a
-							// defer-panic-recover inside of
-							// it, so don't remove it or any
-							// further defer entries
-							break
-						}
-						if prev == nil {
-							gp._defer = d.link
-						} else {
-							prev.link = d.link
-						}
-						newd := d.link
-						freedefer(d)
-						d = newd
+			// Remove any remaining non-started, open-coded
+			// defer entries after a recover, since the
+			// corresponding defers will be executed normally
+			// (inline). Any such entry will become stale once
+			// we run the corresponding defers inline and exit
+			// the associated stack frame.
+			d := gp._defer
+			var prev *_defer
+			if !done {
+				// Skip our current frame, if not done. It is
+				// needed to complete any remaining defers in
+				// deferreturn()
+				prev = d
+				d = d.link
+			}
+			for d != nil {
+				if d.started {
+					// This defer is started but we
+					// are in the middle of a
+					// defer-panic-recover inside of
+					// it, so don't remove it or any
+					// further defer entries
+					break
+				}
+				if d.openDefer {
+					if prev == nil {
+						gp._defer = d.link
 					} else {
-						prev = d
-						d = d.link
+						prev.link = d.link
 					}
+					newd := d.link
+					freedefer(d)
+					d = newd
+				} else {
+					prev = d
+					d = d.link
 				}
 			}
 

src/runtime/testdata/testprog/deadlock.go

@@ -25,6 +25,7 @@ func init() {
 	register("RecursivePanic2", RecursivePanic2)
 	register("RecursivePanic3", RecursivePanic3)
 	register("RecursivePanic4", RecursivePanic4)
+	register("RecursivePanic5", RecursivePanic5)
 	register("GoexitExit", GoexitExit)
 	register("GoNil", GoNil)
 	register("MainGoroutineID", MainGoroutineID)
@@ -160,6 +161,44 @@ func RecursivePanic4() {
 	panic("first panic")
 }
 
+// Test case where we have an open-coded defer higher up the stack (in two), and
+// in the current function (three) we recover in a defer while we still have
+// another defer to be processed.
+func RecursivePanic5() {
+	one()
+	panic("third panic")
+}
+
+//go:noinline
+func one() {
+	two()
+}
+
+//go:noinline
+func two() {
+	defer func() {
+	}()
+
+	three()
+}
+
+//go:noinline
+func three() {
+	defer func() {
+	}()
+
+	defer func() {
+		fmt.Println(recover())
+	}()
+
+	defer func() {
+		fmt.Println(recover())
+		panic("second panic")
+	}()
+
+	panic("first panic")
+}
+
 func GoexitExit() {
 	println("t1")
 	go func() {