-
Notifications
You must be signed in to change notification settings - Fork 141
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
transmute fatal errors into SYS_ASSERTION_FAILED
exit code.
#548
Conversation
4ba2efb
to
c2bf65f
Compare
aa0efaa
to
168dd9c
Compare
b1f8125
to
fcfc70f
Compare
SYS_ASSERTION_FAILED
exit code.
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.
One nit, but feel free to ignore it.
/// every time an actor returns. That's why `begin()` resets any currently | ||
/// accumulated state, as once an error occurs, we want to track its | ||
/// propagation all the way up. | ||
pub fn begin(&mut self, cause: Cause) { |
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.
So much nicer.
fvm/src/call_manager/backtrace.rs
Outdated
pub struct FatalCause { | ||
/// The alternate-formatted message from the anyhow error. | ||
pub error_msg: String, | ||
/// The backtrace, captured if the relevant | ||
/// [environment variables](https://doc.rust-lang.org/std/backtrace/index.html#environment-variables) are enabled. | ||
pub backtrace: String, | ||
} | ||
|
||
/// The ultimate "cause" of a failed message. | ||
#[derive(Clone, Debug)] | ||
pub enum Cause { | ||
/// The original cause was a syscall error. | ||
Syscall(SyscallCause), | ||
/// The original cause was a fatal error. | ||
Fatal(FatalCause), | ||
} | ||
|
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.
We can collapse these into a single enum.
/// The ultimate "cause" of a failed message.
#[derive(Clone, Debug)]
pub enum Cause {
/// The original cause was a syscall error.
Syscall {
module: &'static str,
/// The syscall function name.
function: &'static str,
/// The exact syscall error.
error: ErrorNumber,
/// The informational syscall message.
message: String,
},
/// The original cause was a fatal error.
Fatal {
/// The alternate-formatted message from the anyhow error.
error_msg: String,
/// The backtrace, captured if the relevant
/// [environment variables](https://doc.rust-lang.org/std/backtrace/index.html#environment-variables) are enabled.
backtrace: String,
},
}
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.
I had inline structs in a previous version, but promoted them to types so I could annotate them with derive
macros. I think I don't need those now, so I can demote them again.
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.
LGTM
Closes #508.
This PR transmutes fatal errors into the
SYS_ASSERTION_FAILED
exit code at the top layer (Executor) to produce a consensus outcome on fatal errors.It also modifies the Backtrace components to generalise them so they can tackle syscall errors as well as fatal errors. The cause is informed at different times for each, so the APIs have been adapted accordingly:
Backtrace#begin()
).Backtrace#set_cause()
).I think capturing backtraces (both FVM backtraces and Rust backtraces) is crucial for debuggability. Because I had no way of generating fatal errors to verify the behaviour, I added an integration test which performs no assertions, but prints the backtrace on stdout.
Backtraces from syscall-motivated aborts look like this:
Whereas fatal error backtraces display the error chain from anyhow, as well as the backtrace if captured: