Skip to content

Commit

Permalink
fix: fix the scope of recover() (#1672)
Browse files Browse the repository at this point in the history
<!-- please provide a detailed description of the changes made in this
pull request. -->
## 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

<details><summary>Contributors' checklist...</summary>

- [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).
</details>
  • Loading branch information
deelawn authored Mar 30, 2024
1 parent 6afab42 commit d79930c
Show file tree
Hide file tree
Showing 17 changed files with 304 additions and 44 deletions.
18 changes: 14 additions & 4 deletions gnovm/cmd/gno/testdata/gno_test/recover.txtar
Original file line number Diff line number Diff line change
Expand Up @@ -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!")
Expand Down
7 changes: 7 additions & 0 deletions gnovm/pkg/gnolang/frame.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
86 changes: 72 additions & 14 deletions gnovm/pkg/gnolang/machine.go
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -1425,6 +1446,7 @@ func (m *Machine) PushOp(op Op) {
copy(newOps, m.Ops)
m.Ops = newOps
}

m.Ops[m.NumOps] = op
m.NumOps++
}
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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]
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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
}

Expand Down Expand Up @@ -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)
Expand Down
38 changes: 23 additions & 15 deletions gnovm/pkg/gnolang/op_call.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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).
Expand All @@ -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.
Expand Down Expand Up @@ -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)
Expand All @@ -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 {
Expand All @@ -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")
Expand All @@ -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()
Expand Down
2 changes: 1 addition & 1 deletion gnovm/pkg/gnolang/op_exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
26 changes: 23 additions & 3 deletions gnovm/pkg/gnolang/uverse.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
},
)
Expand Down
2 changes: 1 addition & 1 deletion gnovm/stdlibs/std/native.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Loading

0 comments on commit d79930c

Please sign in to comment.