diff --git a/src/runtime/defer_test.go b/src/runtime/defer_test.go index fc96144597..1d5745d60b 100644 --- a/src/runtime/defer_test.go +++ b/src/runtime/defer_test.go @@ -438,3 +438,82 @@ func expect(t *testing.T, n int, err interface{}) { t.Fatalf("have %v, want %v", err, n) } } + +func TestIssue43920(t *testing.T) { + var steps int + + defer func() { + expect(t, 1, recover()) + }() + defer func() { + defer func() { + defer func() { + expect(t, 5, recover()) + }() + defer panic(5) + func() { + panic(4) + }() + }() + defer func() { + expect(t, 3, recover()) + }() + defer panic(3) + }() + func() { + defer step(t, &steps, 1) + panic(1) + }() +} + +func step(t *testing.T, steps *int, want int) { + println("step", want) + *steps++ + if *steps != want { + t.Fatalf("have %v, want %v", *steps, want) + } +} + +func TestIssue43941(t *testing.T) { + var steps int = 7 + defer func() { + step(t, &steps, 14) + expect(t, 4, recover()) + }() + func() { + func() { + defer func() { + defer func() { + expect(t, 3, recover()) + }() + defer panic(3) + panic(2) + }() + defer func() { + expect(t, 1, recover()) + }() + defer panic(1) + }() + defer func() {}() + defer func() {}() + defer step(t, &steps, 10) + defer step(t, &steps, 9) + step(t, &steps, 8) + }() + func() { + defer step(t, &steps, 13) + defer step(t, &steps, 12) + func() { + defer step(t, &steps, 11) + panic(4) + }() + + // Code below isn't executed, + // but removing it breaks the test case. + defer func() {}() + defer panic(-1) + defer step(t, &steps, -1) + defer step(t, &steps, -1) + defer func() {}() + }() +} diff --git a/src/runtime/panic.go b/src/runtime/panic.go index 58135cf8ce..eec69dfdc6 100644 --- a/src/runtime/panic.go +++ b/src/runtime/panic.go @@ -560,14 +560,28 @@ func printpanics(p *_panic) { print("\n") } -// addOneOpenDeferFrame scans the stack for the first frame (if any) with -// open-coded defers and if it finds one, adds a single record to the defer chain -// for that frame. If sp is non-nil, it starts the stack scan from the frame -// specified by sp. If sp is nil, it uses the sp from the current defer record -// (which has just been finished). Hence, it continues the stack scan from the -// frame of the defer that just finished. It skips any frame that already has an -// open-coded _defer record, which would have been created from a previous -// (unrecovered) panic. +// addOneOpenDeferFrame scans the stack (in gentraceback order, from inner frames to +// outer frames) for the first frame (if any) with open-coded defers. If it finds +// one, it adds a single entry to the defer chain for that frame. The entry added +// represents all the defers in the associated open defer frame, and is sorted in +// order with respect to any non-open-coded defers. +// +// addOneOpenDeferFrame stops (possibly without adding a new entry) if it encounters +// an in-progress open defer entry. An in-progress open defer entry means there has +// been a new panic because of a defer in the associated frame. addOneOpenDeferFrame +// does not add an open defer entry past a started entry, because that started entry +// still needs to finished, and addOneOpenDeferFrame will be called when that started +// entry is completed. The defer removal loop in gopanic() similarly stops at an +// in-progress defer entry. Together, addOneOpenDeferFrame and the defer removal loop +// ensure the invariant that there is no open defer entry further up the stack than +// an in-progress defer, and also that the defer removal loop is guaranteed to remove +// all not-in-progress open defer entries from the defer chain. +// +// If sp is non-nil, addOneOpenDeferFrame starts the stack scan from the frame +// specified by sp. If sp is nil, it uses the sp from the current defer record (which +// has just been finished). Hence, it continues the stack scan from the frame of the +// defer that just finished. It skips any frame that already has a (not-in-progress) +// open-coded _defer record in the defer chain. // // Note: All entries of the defer chain (including this new open-coded entry) have // their pointers (including sp) adjusted properly if the stack moves while @@ -608,6 +622,16 @@ func addOneOpenDeferFrame(gp *g, pc uintptr, sp unsafe.Pointer) { if !d.openDefer { throw("duplicated defer entry") } + // Don't add any record past an + // in-progress defer entry. We don't + // need it, and more importantly, we + // want to keep the invariant that + // there is no open defer entry + // passed an in-progress entry (see + // header comment). + if d.started { + return false + } return true } prev = d @@ -849,12 +873,15 @@ func gopanic(e interface{}) { } atomic.Xadd(&runningPanicDefers, -1) - // 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. + // After a recover, remove any remaining non-started, + // open-coded defer entries, 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. We only remove up to + // the first started (in-progress) open defer entry, not + // including the current frame, since any higher entries will + // be from a higher panic in progress, and will still be + // needed. d := gp._defer var prev *_defer if !done { diff --git a/test/fixedbugs/issue48898.go b/test/fixedbugs/issue48898.go new file mode 100644 index 0000000000..c3af16480f --- /dev/null +++ b/test/fixedbugs/issue48898.go @@ -0,0 +1,40 @@ +// run + +// Copyright 2021 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main + +func main() { + defer func() { + println(recover().(int)) + }() + func() { + func() (_ [2]int) { type _ int; return }() + func() { + defer func() { + defer func() { + recover() + }() + defer panic(3) + panic(2) + }() + defer func() { + recover() + }() + panic(1) + }() + defer func() {}() + }() + + var x = 123 + func() { + // in the original issue, this defer was not executed (which is incorrect) + defer print(x) + func() { + defer func() {}() + panic(4) + }() + }() +} diff --git a/test/fixedbugs/issue48898.out b/test/fixedbugs/issue48898.out new file mode 100644 index 0000000000..81c545efeb --- /dev/null +++ b/test/fixedbugs/issue48898.out @@ -0,0 +1 @@ +1234