Fix sdk assertion macros #660
Conversation
Codecov Report
@@ Coverage Diff @@
## master #660 +/- ##
==========================================
- Coverage 51.06% 51.05% -0.02%
==========================================
Files 120 120
Lines 9694 9694
==========================================
- Hits 4950 4949 -1
- Misses 4744 4745 +1
|
|
We solved this in the builtin actors with https://github.com/filecoin-project/builtin-actors/blob/344ea72ed5b297bfb1291ee6fe8383cb6b587ea4/runtime/src/runtime/fvm.rs#L552-L554. |
|
Basically:
|
sdk/src/testing.rs
Outdated
| let res = std::panic::catch_unwind(|| { | ||
| core::assert!($cond); | ||
| }); | ||
| if res.is_err() { | ||
| let panic_msg = match res.err() { | ||
| Some(err) => match err.downcast::<String>() { | ||
| Ok(panic_msg_box) => Some(panic_msg_box.as_str()), | ||
| Err(err) => None, | ||
| }, | ||
| None => unreachable!(), | ||
| }; | ||
|
|
||
| $crate::vm::abort( | ||
| fvm_shared::error::ExitCode::USR_ASSERTION_FAILED.value(), | ||
| panic_msg, | ||
| ); | ||
| } | ||
| $crate::testing::handle_assert_err(res); |
There was a problem hiding this comment.
Is there any reason we need to assert than catch it? Can't we just re-implement assert and abort directly? The implementation should be pretty simple.
Alternatively, just drop the wrapping entirely. I.e.:
|
|
Thanks for the tips @Stebalien ! Used them to fix it 👍 |
|
Conflicts developed :-( |
raulk
force-pushed
the
feature/sdk-macro-naming
branch
from
ae9a426 to
9eb38ca
Compare
|
I think there's some confusion here. We shouldn't be setting a panic hook than immediately calling assert.
But the former would be better. However, setting a hook a hook, then immediately asserting, is a very round-about way of going about this. Does that make sense? |
Does that mean we carry unwind information within actor bundles? |
Not the deployed ones, no. However, asserts will still give you nice errors even without actual backtraces. |
This replaces the custom assert macros. Instead of overriding the assert macros, the user would: 1. Call `fvm_sdk::initialize()` at the top of their actor/tests. 2. Use the default `assert` macros. replaces #660
|
See #896. |
This replaces the custom assert macros. Instead of overriding the assert macros, the user would: 1. Call `fvm_sdk::initialize()` at the top of their actor/tests. 2. Use the default `assert` macros. replaces #660
While working on the High level Rust SDK I found out that low level macro were not working as expected. I started to fix their behavior but also found a problem in how things were handled.
At the time when we developed the macros, it seemed relevant to @raulk and myself to use
catch_unwindto prevent a panic on an assert. But it seems that catch_unwind doesn't work in in WASM as it's behavior is panic=abort (rust-lang/rust#58874) which will throw an error before we have time to call theabortsyscall.I do not really see an easy way of solving this, opening a PR to share it with the team and (maybe) get some start of a solution.