-
Notifications
You must be signed in to change notification settings - Fork 29
Make assert information more precise #1009
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
base: main
Are you sure you want to change the base?
Conversation
f5a4bbc to
a240212
Compare
| // I don't know in which other cases it can be `None`. | ||
| assert!(target.is_none()); | ||
| // We ignore the arguments | ||
| // TODO: shouldn't we do something with the unwind edge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, Abort should have an unwind edge actually. I'm really not sure that this particular reconstruction we're doing here is useful honestly, this is just a function call that can be caught by tools.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep it for now, Abort is a useful primitive for when we'll turn Assert into a plain if.
| /// A non-diverging runtime check for a condition. This can be either: | ||
| /// - Emitted for inlined "assumes" (which cause UB on failure) | ||
| /// - Reconstructed from `if b { panic() }` if `--reconstruct-asserts` is set. | ||
| /// This statement comes with the effect that happens when the check fails | ||
| /// (rather than representing it as an unwinding edge). | ||
| Assert { | ||
| assert: Assert, | ||
| on_failure: AbortKind, | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, having assert at all felt like a hack because it's a glorified if. Having it both as a statement and a terminator feels even less motivated. Long-term I'd like to remove both and just use if instead honestly. Not asking you to do this here tho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Big fan of BuiltinAssertKind, this will make reconstruction more robust. Overall like the shape of this but we shouldn't be changing the llbc output too much yet. I will want to remove ullbc::StatementKind::Assert eventually; you may find this commit instructive xp
4c279d5 to
efde658
Compare
|
sounds good! and yeah agreed having both edit: forgot about aeneas and eurydice :P will do these tomorrow |
7b361c3 to
4ac1747
Compare
|
Eurydice and Aeneas are ready :) |
4ac1747 to
8e9f54e
Compare
Head branch was pushed to by a user without write access
8e9f54e to
2419dcb
Compare
|
oops, rebased |
(waiting for a review before taking care of Aeneas / Eurydice)
As mentioned in #1000, relying on
reconstruct_fallible_operationsis risky and error-prone. However not having this pass enabled currently provided too little information to provide satisfactory user feedback for assertion failures.We first translate the message associated with each MIR assert, to allow tools to explain what a failed assert may correspond to. This will also allow us to detect what asserts correspond to fallible operations.
Next we translate terminator asserts from MIR into a new
TerminatorKind::Assert. We also removeon_failurefromAssert; instead,StatementKind::Assertis now{ assert: Assert, on_failure: AbortKind }, andTerminatorKind::Assertis now{ assert: Assert, target, on_unwind }.Currently, we initially only translate
StatementKind::Assertfor theAssumenon-diverging intrinsic; later passes then may lower aTerminatorKind::Assertinto aStatementKind, e.g. for dynamic checks if--reconstruct-fallible-operationsis set.This mostly closes #868! The last couple cases where we lose edges right now is when converting calls to the language items
panic,panic_fmt,begin_panicor to the functionscore::panicking::assert_failedandcore::panicking::panic_explicitinto aTerminatorKind::Abort(AbortKind::Panic). The same happens ininline_local_panic_functions.rs. These function calls have an unwind edge, that is needed e.g. to drop any remaining locals after starting the panic. An option is adding an optionalon_unwindtoTerminatorKind::Abort.Small sidenote but I also find it weird we always convert some of these function calls to aborts in
translate_bodies; supposedly this should be done as a micro-pass along withinline_local_panic_functions.rsif a specific flag is set (something like--simplify-panics). Technically the only 'magic' associated with panics should be in__rust_start_panicci: use AeneasVerif/aeneas#745
ci: use AeneasVerif/eurydice#376