From d79930cb3aefc5f2f5786cad1aa183994b91a3af Mon Sep 17 00:00:00 2001 From: deelawn Date: Sat, 30 Mar 2024 01:00:31 +0000 Subject: [PATCH] fix: fix the scope of `recover()` (#1672) ## High level overview Addresses #1656. This is meant to bring achieve parity between gno and go's `recover` functionality. The new tests help to ensure this. BREAKING CHANGE: this will break any realm code that has relies on the incorrect `recover` implementation ### Changes For the convenience of the reviewer, here are the categories of changes that have been made: - the `recover` function has been modified to determine whether a panic can be recovered; this is based on both the "level" of nested defer statements and the frame from which the panic was initiated - `Frame`s are now stored as pointers to avoid copying frame values as they are tracked by the `Exception`s that have been thrown - a new `Exception` type has been defined to encapsulate both the panic string and the frame in which it was initiated - the machine has two new "scope" member variables for tracking levels of `panic` and `defer` - added safety mechanisms to retrieving frames because we may not always want it to panic when trying to retrieve a call frame at an index that exceeds the stack depth - the gno standard library's `testing` package's `Recover` function no longer works as intended; the tests have been modified to achieve the intended behavior - various new file tests have been added to ensure `recover` behaves as expected in certain edge cases
Contributors' checklist... - [x] Added new tests, or not needed, or not feasible - [x] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [x] Updated the official documentation or not needed - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs - [x] Provided any useful hints for running manual tests - [ ] Added new benchmarks to [generated graphs](https://gnoland.github.io/benchmarks), if any. More info [here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
--- gnovm/cmd/gno/testdata/gno_test/recover.txtar | 18 +++- gnovm/pkg/gnolang/frame.go | 7 ++ gnovm/pkg/gnolang/machine.go | 86 ++++++++++++++++--- gnovm/pkg/gnolang/op_call.go | 38 ++++---- gnovm/pkg/gnolang/op_exec.go | 2 +- gnovm/pkg/gnolang/uverse.go | 26 +++++- gnovm/stdlibs/std/native.go | 2 +- gnovm/stdlibs/testing/testing.gno | 14 ++- gnovm/tests/files/recover10.gno | 10 +++ gnovm/tests/files/recover10a.gno | 12 +++ gnovm/tests/files/recover11.gno | 38 ++++++++ gnovm/tests/files/recover12a.gno | 26 ++++++ gnovm/tests/files/recover12b.gno | 28 ++++++ .../tests/{challenges => files}/recover5b.gno | 1 - gnovm/tests/files/recover8.gno | 24 ++++++ gnovm/tests/files/recover9.gno | 14 +++ gnovm/tests/stdlibs/std/std.go | 2 +- 17 files changed, 304 insertions(+), 44 deletions(-) create mode 100644 gnovm/tests/files/recover10.gno create mode 100644 gnovm/tests/files/recover10a.gno create mode 100644 gnovm/tests/files/recover11.gno create mode 100644 gnovm/tests/files/recover12a.gno create mode 100644 gnovm/tests/files/recover12b.gno rename gnovm/tests/{challenges => files}/recover5b.gno (98%) create mode 100644 gnovm/tests/files/recover8.gno create mode 100644 gnovm/tests/files/recover9.gno diff --git a/gnovm/cmd/gno/testdata/gno_test/recover.txtar b/gnovm/cmd/gno/testdata/gno_test/recover.txtar index 9960ee74a1d..418545f97c9 100644 --- a/gnovm/cmd/gno/testdata/gno_test/recover.txtar +++ b/gnovm/cmd/gno/testdata/gno_test/recover.txtar @@ -32,20 +32,30 @@ package recov import "testing" +type RecoverySetter struct { + value interface{} +} + +func (s *RecoverySetter) Set(v interface{}) { + s.value = v +} + func TestRecover(t *testing.T) { + var setter RecoverySetter defer func() { - err := testing.Recover() - t.Log("recovered", err) + t.Log("recovered", setter.value) }() + defer testing.Recover(&setter) panic("bad panic!") } func TestRecoverSkip(t *testing.T) { + var setter RecoverySetter defer func() { - err := testing.Recover() - t.Log("recovered", err) + t.Log("recovered", setter.value) }() + defer testing.Recover(&setter) t.Skip("skipped") panic("bad panic!") diff --git a/gnovm/pkg/gnolang/frame.go b/gnovm/pkg/gnolang/frame.go index 7f87fa26097..c808fc111b0 100644 --- a/gnovm/pkg/gnolang/frame.go +++ b/gnovm/pkg/gnolang/frame.go @@ -26,6 +26,8 @@ type Frame struct { Defers []Defer // deferred calls LastPackage *PackageValue // previous package context LastRealm *Realm // previous realm context + + Popped bool // true if frame has been popped } func (fr Frame) String() string { @@ -84,4 +86,9 @@ type Defer struct { Args []TypedValue // arguments Source *DeferStmt // source Parent *Block + + // PanicScope is set to the value of the Machine's PanicScope when the + // defer is created. The PanicScope of the Machine is incremented each time + // a panic occurs and is decremented each time a panic is recovered. + PanicScope uint } diff --git a/gnovm/pkg/gnolang/machine.go b/gnovm/pkg/gnolang/machine.go index e9e8eba8adc..ea7be1d1f22 100644 --- a/gnovm/pkg/gnolang/machine.go +++ b/gnovm/pkg/gnolang/machine.go @@ -16,6 +16,19 @@ import ( "github.com/gnolang/gno/tm2/pkg/std" ) +// Exception represents a panic that originates from a gno program. +type Exception struct { + // Value is the value passed to panic. + Value TypedValue + // Frame is used to reference the frame a panic occurred in so that recover() knows if the + // currently executing deferred function is able to recover from the panic. + Frame *Frame +} + +func (e Exception) Sprint(m *Machine) string { + return e.Value.Sprint(m) +} + //---------------------------------------- // Machine @@ -28,13 +41,13 @@ type Machine struct { Exprs []Expr // pending expressions Stmts []Stmt // pending statements Blocks []*Block // block (scope) stack - Frames []Frame // func call stack + Frames []*Frame // func call stack Package *PackageValue // active package Realm *Realm // active realm Alloc *Allocator // memory allocations - Exceptions []*TypedValue // if panic'd unless recovered - NumResults int // number of results returned - Cycles int64 // number of "cpu" cycles + Exceptions []Exception + NumResults int // number of results returned + Cycles int64 // number of "cpu" cycles // Configuration CheckTypes bool // not yet used @@ -44,6 +57,14 @@ type Machine struct { Output io.Writer Store Store Context interface{} + + // PanicScope is incremented each time a panic occurs and is reset to + // zero when it is recovered. + PanicScope uint + // DeferPanicScope is set to the value of the defer's panic scope before + // it is executed. It is reset to zero after the defer functions in the current + // scope have finished executing. + DeferPanicScope uint } // NewMachine initializes a new gno virtual machine, acting as a shorthand @@ -1425,6 +1446,7 @@ func (m *Machine) PushOp(op Op) { copy(newOps, m.Ops) m.Ops = newOps } + m.Ops[m.NumOps] = op m.NumOps++ } @@ -1642,7 +1664,7 @@ func (m *Machine) LastBlock() *Block { // Pushes a frame with one less statement. func (m *Machine) PushFrameBasic(s Stmt) { label := s.GetLabel() - fr := Frame{ + fr := &Frame{ Label: label, Source: s, NumOps: m.NumOps, @@ -1661,7 +1683,7 @@ func (m *Machine) PushFrameBasic(s Stmt) { // ensure the counts are consistent, otherwise we mask // bugs with frame pops. func (m *Machine) PushFrameCall(cx *CallExpr, fv *FuncValue, recv TypedValue) { - fr := Frame{ + fr := &Frame{ Source: cx, NumOps: m.NumOps, NumValues: m.NumValues - cx.NumArgs - 1, @@ -1698,7 +1720,7 @@ func (m *Machine) PushFrameCall(cx *CallExpr, fv *FuncValue, recv TypedValue) { } func (m *Machine) PushFrameGoNative(cx *CallExpr, fv *NativeValue) { - fr := Frame{ + fr := &Frame{ Source: cx, NumOps: m.NumOps, NumValues: m.NumValues - cx.NumArgs - 1, @@ -1724,15 +1746,17 @@ func (m *Machine) PushFrameGoNative(cx *CallExpr, fv *NativeValue) { func (m *Machine) PopFrame() Frame { numFrames := len(m.Frames) f := m.Frames[numFrames-1] + f.Popped = true if debug { m.Printf("-F %#v\n", f) } m.Frames = m.Frames[:numFrames-1] - return f + return *f } func (m *Machine) PopFrameAndReset() { fr := m.PopFrame() + fr.Popped = true m.NumOps = fr.NumOps m.NumValues = fr.NumValues m.Exprs = m.Exprs[:fr.NumExprs] @@ -1744,6 +1768,7 @@ func (m *Machine) PopFrameAndReset() { // TODO: optimize by passing in last frame. func (m *Machine) PopFrameAndReturn() { fr := m.PopFrame() + fr.Popped = true if debug { // TODO: optimize with fr.IsCall if fr.Func == nil && fr.GoFunc == nil { @@ -1799,18 +1824,29 @@ func (m *Machine) NumFrames() int { } func (m *Machine) LastFrame() *Frame { - return &m.Frames[len(m.Frames)-1] + return m.Frames[len(m.Frames)-1] +} + +// MustLastCallFrame returns the last call frame with an offset of n. It panics if the frame is not found. +func (m *Machine) MustLastCallFrame(n int) *Frame { + return m.lastCallFrame(n, true) +} + +// LastCallFrame behaves the same as MustLastCallFrame, but rather than panicking, +// returns nil if the frame is not found. +func (m *Machine) LastCallFrame(n int) *Frame { + return m.lastCallFrame(n, false) } // TODO: this function and PopUntilLastCallFrame() is used in conjunction // spanning two disjoint operations upon return. Optimize. // If n is 1, returns the immediately last call frame. -func (m *Machine) LastCallFrame(n int) *Frame { +func (m *Machine) lastCallFrame(n int, mustBeFound bool) *Frame { if n == 0 { panic("n must be positive") } for i := len(m.Frames) - 1; i >= 0; i-- { - fr := &m.Frames[i] + fr := m.Frames[i] if fr.Func != nil || fr.GoFunc != nil { // TODO: optimize with fr.IsCall if n == 1 { @@ -1820,20 +1856,34 @@ func (m *Machine) LastCallFrame(n int) *Frame { } } } - panic("frame not found") + + if mustBeFound { + panic("frame not found") + } + + return nil } // pops the last non-call (loop) frames // and returns the last call frame (which is left on stack). func (m *Machine) PopUntilLastCallFrame() *Frame { for i := len(m.Frames) - 1; i >= 0; i-- { - fr := &m.Frames[i] + fr := m.Frames[i] if fr.Func != nil || fr.GoFunc != nil { // TODO: optimize with fr.IsCall m.Frames = m.Frames[:i+1] return fr } + + fr.Popped = true } + + // No frames are popped, so revert all the frames' popped flag. + // This is expected to happen infrequently. + for _, frame := range m.Frames { + frame.Popped = false + } + return nil } @@ -1926,7 +1976,15 @@ func (m *Machine) CheckEmpty() error { } func (m *Machine) Panic(ex TypedValue) { - m.Exceptions = append(m.Exceptions, &ex) + m.Exceptions = append( + m.Exceptions, + Exception{ + Value: ex, + Frame: m.MustLastCallFrame(1), + }, + ) + + m.PanicScope++ m.PopUntilLastCallFrame() m.PushOp(OpPanic2) m.PushOp(OpReturnCallDefers) diff --git a/gnovm/pkg/gnolang/op_call.go b/gnovm/pkg/gnolang/op_call.go index c7274dd73af..5479ee6d5ae 100644 --- a/gnovm/pkg/gnolang/op_call.go +++ b/gnovm/pkg/gnolang/op_call.go @@ -247,7 +247,7 @@ func (m *Machine) doOpReturnFromBlock() { // deferred statements can refer to results with name // expressions. func (m *Machine) doOpReturnToBlock() { - cfr := m.LastCallFrame(1) + cfr := m.MustLastCallFrame(1) ft := cfr.Func.GetType(m.Store) numParams := len(ft.Params) numResults := len(ft.Results) @@ -260,10 +260,11 @@ func (m *Machine) doOpReturnToBlock() { } func (m *Machine) doOpReturnCallDefers() { - cfr := m.LastCallFrame(1) + cfr := m.MustLastCallFrame(1) dfr, ok := cfr.PopDefer() if !ok { // Done with defers. + m.DeferPanicScope = 0 m.ForcePopOp() if len(m.Exceptions) > 0 { // In a state of panic (not return). @@ -272,6 +273,9 @@ func (m *Machine) doOpReturnCallDefers() { } return } + + m.DeferPanicScope = dfr.PanicScope + // Call last deferred call. // NOTE: the following logic is largely duplicated in doOpCall(). // Convert if variadic argument. @@ -347,7 +351,7 @@ func (m *Machine) doOpReturnCallDefers() { func (m *Machine) doOpDefer() { lb := m.LastBlock() - cfr := m.LastCallFrame(1) + cfr := m.MustLastCallFrame(1) ds := m.PopStmt().(*DeferStmt) // Pop arguments numArgs := len(ds.Call.Args) @@ -361,10 +365,11 @@ func (m *Machine) doOpDefer() { case *FuncValue: // TODO what if value is NativeValue? cfr.PushDefer(Defer{ - Func: cv, - Args: args, - Source: ds, - Parent: lb, + Func: cv, + Args: args, + Source: ds, + Parent: lb, + PanicScope: m.PanicScope, }) case *BoundMethodValue: if debug { @@ -381,17 +386,19 @@ func (m *Machine) doOpDefer() { args2[0] = cv.Receiver copy(args2[1:], args) cfr.PushDefer(Defer{ - Func: cv.Func, - Args: args2, - Source: ds, - Parent: lb, + Func: cv.Func, + Args: args2, + Source: ds, + Parent: lb, + PanicScope: m.PanicScope, }) case *NativeValue: cfr.PushDefer(Defer{ - GoFunc: cv, - Args: args, - Source: ds, - Parent: lb, + GoFunc: cv, + Args: args, + Source: ds, + Parent: lb, + PanicScope: m.PanicScope, }) default: panic("should not happen") @@ -410,6 +417,7 @@ func (m *Machine) doOpPanic2() { // Recovered from panic m.PushOp(OpReturnFromBlock) m.PushOp(OpReturnCallDefers) + m.PanicScope = 0 } else { // Keep panicking last := m.PopUntilLastCallFrame() diff --git a/gnovm/pkg/gnolang/op_exec.go b/gnovm/pkg/gnolang/op_exec.go index 300303135ad..12e0f9e26e3 100644 --- a/gnovm/pkg/gnolang/op_exec.go +++ b/gnovm/pkg/gnolang/op_exec.go @@ -541,7 +541,7 @@ EXEC_SWITCH: m.PushForPointer(cs.X) case *ReturnStmt: m.PopStmt() - fr := m.LastCallFrame(1) + fr := m.MustLastCallFrame(1) ft := fr.Func.GetType(m.Store) hasDefers := 0 < len(fr.Defers) hasResults := 0 < len(ft.Results) diff --git a/gnovm/pkg/gnolang/uverse.go b/gnovm/pkg/gnolang/uverse.go index a0e9913eaad..9f836c789fe 100644 --- a/gnovm/pkg/gnolang/uverse.go +++ b/gnovm/pkg/gnolang/uverse.go @@ -972,9 +972,29 @@ func UverseNode() *PackageNode { m.PushValue(TypedValue{}) return } - // Just like in go, only the last exception is returned to recover. - m.PushValue(*m.Exceptions[len(m.Exceptions)-1]) - // The remaining exceptions are removed + + // If the exception is out of scope, this recover can't help; return nil. + if m.PanicScope <= m.DeferPanicScope { + m.PushValue(TypedValue{}) + return + } + + exception := &m.Exceptions[len(m.Exceptions)-1] + + // If the frame the exception occurred in is not popped, it's possible that + // the exception is still in scope and can be recovered. + if !exception.Frame.Popped { + // If the frame is not the current frame, the exception is not in scope; return nil. + // This retrieves the second most recent call frame because the first most recent + // is the call to recover itself. + if frame := m.LastCallFrame(2); frame == nil || (frame != nil && frame != exception.Frame) { + m.PushValue(TypedValue{}) + return + } + } + + m.PushValue(exception.Value) + // Recover complete; remove exceptions. m.Exceptions = nil }, ) diff --git a/gnovm/stdlibs/std/native.go b/gnovm/stdlibs/std/native.go index 0965ef74277..26bfe433858 100644 --- a/gnovm/stdlibs/std/native.go +++ b/gnovm/stdlibs/std/native.go @@ -64,7 +64,7 @@ func X_callerAt(m *gno.Machine, n int) string { ctx := m.Context.(ExecContext) return string(ctx.OrigCaller) } - return string(m.LastCallFrame(n).LastPackage.GetPkgAddr().Bech32()) + return string(m.MustLastCallFrame(n).LastPackage.GetPkgAddr().Bech32()) } func X_getRealm(m *gno.Machine, height int) (address string, pkgPath string) { diff --git a/gnovm/stdlibs/testing/testing.gno b/gnovm/stdlibs/testing/testing.gno index 36e8e7a6955..fb13c0f39cd 100644 --- a/gnovm/stdlibs/testing/testing.gno +++ b/gnovm/stdlibs/testing/testing.gno @@ -30,12 +30,18 @@ func (s skipErr) Error() string { // (and Skip* functions). // // NOTE: Recover() is likely to be removed. -func Recover() interface{} { +func Recover(result Setter) { r := recover() - if _, ok := r.(skipErr); ok { - panic(r) + if _, ok := r.(skipErr); !ok { + result.Set(r) + return } - return r + + panic(r) +} + +type Setter interface { + Set(v interface{}) } func Short() bool { diff --git a/gnovm/tests/files/recover10.gno b/gnovm/tests/files/recover10.gno new file mode 100644 index 00000000000..16dff4d4fed --- /dev/null +++ b/gnovm/tests/files/recover10.gno @@ -0,0 +1,10 @@ +package main + +func main() { + defer recover() + + panic("ahhhhh") +} + +// Error: +// ahhhhh diff --git a/gnovm/tests/files/recover10a.gno b/gnovm/tests/files/recover10a.gno new file mode 100644 index 00000000000..4e9aa9301e7 --- /dev/null +++ b/gnovm/tests/files/recover10a.gno @@ -0,0 +1,12 @@ +package main + +func main() { + defer func() { + println(recover()) + }() + + panic("ahhhhh") +} + +// Output: +// ahhhhh diff --git a/gnovm/tests/files/recover11.gno b/gnovm/tests/files/recover11.gno new file mode 100644 index 00000000000..a8adaff2c34 --- /dev/null +++ b/gnovm/tests/files/recover11.gno @@ -0,0 +1,38 @@ +package main + +func level3() { + panic("level3") + defer func() { + if r := recover(); r != nil { + println("recover level3 in level3") + } + }() +} + +func level2() { + defer func() { + if r := recover(); r != nil { + println("recover level3 in level2") + } + }() + defer level3() + panic("level2") +} + +func level1() { + defer func() { + if r := recover(); r != nil { + println("recover level3 in level1") + } + }() + level2() + panic("level1") +} + +func main() { + level1() +} + +// Output: +// recover level3 in level2 +// recover level3 in level1 diff --git a/gnovm/tests/files/recover12a.gno b/gnovm/tests/files/recover12a.gno new file mode 100644 index 00000000000..c8b82511ad0 --- /dev/null +++ b/gnovm/tests/files/recover12a.gno @@ -0,0 +1,26 @@ +package main + +func anotherRecover() { + if r := recover(); r != nil { + println(r) + } +} + +func main() { + defer func() { + if r := recover(); r != nil { + println(r) + } + }() + defer anotherRecover() + defer func() { + if r := recover(); r != nil { + panic("panic in defer func") + } + }() + + panic("panic in main") +} + +// Output: +// panic in defer func diff --git a/gnovm/tests/files/recover12b.gno b/gnovm/tests/files/recover12b.gno new file mode 100644 index 00000000000..4cc0a48b9b0 --- /dev/null +++ b/gnovm/tests/files/recover12b.gno @@ -0,0 +1,28 @@ +package main + +func anotherRecover() { + if r := recover(); r != nil { + println(r) + } +} + +func main() { + defer func() { + if r := recover(); r != nil { + println(r.(string) + ": not another recover") + } + }() + defer func() { + anotherRecover() + }() + defer func() { + if r := recover(); r != nil { + panic("panic in defer func") + } + }() + + panic("panic in main") +} + +// Output: +// panic in defer func: not another recover diff --git a/gnovm/tests/challenges/recover5b.gno b/gnovm/tests/files/recover5b.gno similarity index 98% rename from gnovm/tests/challenges/recover5b.gno rename to gnovm/tests/files/recover5b.gno index 285bc386093..f94c7442efe 100644 --- a/gnovm/tests/challenges/recover5b.gno +++ b/gnovm/tests/files/recover5b.gno @@ -24,4 +24,3 @@ func g() { // Output: // g recover undefined // f recover wtf -// false diff --git a/gnovm/tests/files/recover8.gno b/gnovm/tests/files/recover8.gno new file mode 100644 index 00000000000..53b31f05468 --- /dev/null +++ b/gnovm/tests/files/recover8.gno @@ -0,0 +1,24 @@ +package main + +func doSomething() { + defer func() { + doSomethingElse() + if r := recover(); r != nil { + panic("do something panic") + } + }() + panic("first panic") +} + +func doSomethingElse() { + if r := recover(); r != nil { + panic("do something else panic") + } +} + +func main() { + doSomething() +} + +// Error: +// do something panic diff --git a/gnovm/tests/files/recover9.gno b/gnovm/tests/files/recover9.gno new file mode 100644 index 00000000000..d6c944aeb06 --- /dev/null +++ b/gnovm/tests/files/recover9.gno @@ -0,0 +1,14 @@ +package main + +func rec() { + println(recover()) +} + +func main() { + defer rec() + + panic("ahhhhh") +} + +// Output: +// ahhhhh diff --git a/gnovm/tests/stdlibs/std/std.go b/gnovm/tests/stdlibs/std/std.go index 0e15c8d0241..f7bc001043e 100644 --- a/gnovm/tests/stdlibs/std/std.go +++ b/gnovm/tests/stdlibs/std/std.go @@ -85,7 +85,7 @@ func X_callerAt(m *gno.Machine, n int) string { ctx := m.Context.(std.ExecContext) return string(ctx.OrigCaller) } - return string(m.LastCallFrame(n).LastPackage.GetPkgAddr().Bech32()) + return string(m.MustLastCallFrame(n).LastPackage.GetPkgAddr().Bech32()) } func X_testSetOrigCaller(m *gno.Machine, addr string) {