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

fix: catch runtime panic #2484

Closed
wants to merge 3 commits into from
Closed

fix: catch runtime panic #2484

wants to merge 3 commits into from

Conversation

omarsy
Copy link
Contributor

@omarsy omarsy commented Jul 2, 2024

Closes #2146

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Jul 2, 2024
Copy link

codecov bot commented Jul 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 55.03%. Comparing base (6432573) to head (1308adc).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2484      +/-   ##
==========================================
+ Coverage   55.02%   55.03%   +0.01%     
==========================================
  Files         595      595              
  Lines       79645    79655      +10     
==========================================
+ Hits        43821    43838      +17     
+ Misses      32504    32498       -6     
+ Partials     3320     3319       -1     
Flag Coverage Δ
contribs/gnodev 26.00% <ø> (+0.34%) ⬆️
contribs/gnofaucet 14.46% <ø> (ø)
contribs/gnokeykc 0.00% <ø> (ø)
contribs/gnomd 0.00% <ø> (ø)
gno.land 64.39% <ø> (ø)
gnovm 60.32% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@deelawn deelawn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is the best way to solve the problem. My main concern is this: what might happen if there is a bug or unexpected case in VM that generates a panic? A real panic because it doesn't know what to do? Not a go runtime panic.

I'd prefer to see a series a PRs that identifies each of the ways the go runtime can cause panics. Then check the VM code and see which are not handled, such as dividing by zero. Instead of letting the runtime panic, go through the same steps the VM does when it encounters a panic statement -- you should be familiar with how that works with the good work you've done creating the stacktrace.

Let's wait for a second opinion before you dive into that though.

Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, agreed with @deelawn. If there is one thing I'm grateful for, is that in the GnoVM there is a distinction between panics at the VM level (that can be caught with a recover()) and panic as a result of programming errors.

The fix for #2146 is handling the case where the the right expression in an integer division is zero, and doing a m.Panic() as a consequence.

@thehowl
Copy link
Member

thehowl commented Jul 24, 2024

Please open a new PR, closing this one so it's documented we don't want indiscriminate recovers at the machine level like this.

@thehowl thehowl closed this Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

recover not working correctly with runtime panics in Gnolang
3 participants