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 <danscales@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
Trust: Dan Scales <danscales@google.com>

diff --git a/src/runtime/crash_test.go b/src/runtime/crash_test.go
index 58ad4f3ebac399f37780281850f80e48ff873a8a..e5bd7973b70b781771b40eb734a49e0de7a0d4d6 100644 (file)
--- a/src/runtime/crash_test.go
+++ b/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)

diff --git a/src/runtime/defer_test.go b/src/runtime/defer_test.go
index 5ac08145646063bee6e87c47cf1c48691d8149ad..9a40ea19842faf029d6a2502c38c15fedddb1f52 100644 (file)
--- a/src/runtime/defer_test.go
+++ b/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)
+       }
+}

diff --git a/src/runtime/panic.go b/src/runtime/panic.go
index aed17d6fc62bd5704559477e192ba22468a055e3..5b2ccdd8740f802468227506afc9854459d23e4f 100644 (file)
--- a/src/runtime/panic.go
+++ b/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
                                }
                        }
 

diff --git a/src/runtime/testdata/testprog/deadlock.go b/src/runtime/testdata/testprog/deadlock.go
index 105d6a5faae0cf78d4badfa926e903f7ab20d110..781acbd7706ac68627f3962bfd9d11f6c8953f41 100644 (file)
--- a/src/runtime/testdata/testprog/deadlock.go
+++ b/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() {