Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[gnovm] chain Panics #756

Closed
jaekwon opened this issue Apr 19, 2023 · 5 comments · Fixed by #771
Closed

[gnovm] chain Panics #756

jaekwon opened this issue Apr 19, 2023 · 5 comments · Fixed by #771

Comments

@jaekwon
Copy link
Contributor

jaekwon commented Apr 19, 2023

Description

I don't think the current machine.Panic() handles recursive panics as it should.

// machine.go
func (m *Machine) Panic(ex TypedValue) {
        // TODO: chain exceptions if preexisting unrecovered exception.
        m.Exception = &ex
        m.PopUntilLastCallFrame()
        m.PushOp(OpPanic2)
        m.PushOp(OpReturnCallDefers)
}

vs https://groups.google.com/g/golang-nuts/c/HOXNBQu5c-Q/m/ltQ-QHBrw9gJ

The golang nuts link includes examples that should be added to our filetests, and failure there will prove this issue.

Recommend discussing spec before implementation for feedback!

@tbruyelle
Copy link
Contributor

I noticed that and it was on my TODO list :)

About the examples in the golang nuts forum, I noticed this one, is it the one you're talking about ?

func f1() (success bool) {
	defer func() { success = recover() == nil }()
	defer f2()
	panic("panicking")
}

func f2() {
	defer func() { _ = recover() }()
}

func f3() { _ = recover() }

Does f1() return true or false? I'd expect false, since it wouldn't
make much sense for a panic to be caught several levels up the call
stack. But then, if you replace f2 with f3, I'd expect true, since f2
is at the same depth as a closure would be, so it it's a matter of
depth wrt the panicker, it should be equivalent. However, if it's
something that gets wrapped by the closure, I'd expect false.

Essentially, how does pancking propogate upward?

@jaekwon
Copy link
Contributor Author

jaekwon commented Apr 19, 2023

The first post from Rob had a few examples.
https://groups.google.com/g/golang-nuts/c/HOXNBQu5c-Q/m/Je0qo1hbxIsJ

But we should run that too and others to verify the problem and test the solution!

@tbruyelle
Copy link
Contributor

Thanks I'll work on that!

@tbruyelle
Copy link
Contributor

tbruyelle commented Apr 20, 2023

Somehow unrelated with the TODO, but in Rob's examples I found something that isn't supported in Gno :

https://go.dev/play/p/CcMGgY606O-

Basically a recover() that is invoked in a deferred closure that is absent from the stack at the time of the panic(), should return nil. In Gno it returns the panic argument. I'll prepare a PR to add this in the tests/challenges folder, with other stuffs.

tbruyelle added a commit to tbruyelle/gno that referenced this issue Apr 20, 2023
- Add 2 challenges for map and slice literals, as described in gnolang#758
- Add a challenge to handle chained panics, as described in gnolang#756
- Add a challenge to handle a specific rule of recover, as described in
  this [comment][0]

If you manage to solve one of these challenges by updating the gnovm code, move
those files into the `gnovm/test/files` folder (eventually increment the
number suffix in the name according to existing files).

[0]: gnolang#756 (comment)
tbruyelle added a commit to tbruyelle/gno that referenced this issue Apr 20, 2023
- Add 2 challenges for map and slice literals, as described in gnolang#758
- Add a challenge to handle chained panics, as described in gnolang#756
- Add a challenge to handle a specific rule of recover, as described in
  this [comment][0]

If you manage to solve one of these challenges by updating the gnovm code, move
those files into the `gnovm/test/files` folder (eventually increment the
number suffix in the name according to existing files).

[0]: gnolang#756 (comment)
tbruyelle added a commit to tbruyelle/gno that referenced this issue Apr 20, 2023
- Add 2 challenges for map and slice literals, as described in gnolang#758
- Add a challenge to handle chained panics, as described in gnolang#756
- Add a challenge to handle a specific rule of recover, as described in
  this [comment][0]

Also add some working recover cases in `tests/files`, to increase code
coverage.

If you manage to solve one of these challenges by updating the gnovm code, move
those files into the `gnovm/test/files` folder (eventually increment the
number suffix in the name according to existing files).

[0]: gnolang#756 (comment)
tbruyelle added a commit to tbruyelle/gno that referenced this issue Apr 21, 2023
Close gnolang#756

By turning m.Exception into a slice, it became possible to append
exceptions when panics are chained.

`recover` is adapted so it receives only the last panic, just as in go (see
`tests/files/recover7.gno`). The remaining exception are then wiped.

If no recover is met, the exceptions slice is formatted like in go, with
exception concatened with `\n\t` (see `tests/files/panic0b.gno` that was
prevouisly in `tests/challenges`).
tbruyelle added a commit to tbruyelle/gno that referenced this issue Apr 25, 2023
Close gnolang#756

By turning m.Exception into a slice, it became possible to append
exceptions when panics are chained.

`recover` is adapted so it receives only the last panic, just as in go (see
`tests/files/recover7.gno`). The remaining exception are then wiped.

If no recover is met, the exceptions slice is formatted like in go, with
exception concatened with `\n\t` (see `tests/files/panic0b.gno` that was
prevouisly in `tests/challenges`).
@tbruyelle
Copy link
Contributor

@jaekwon FYI the PR that resolves this issue is ready for review : #771

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging a pull request may close this issue.

2 participants