-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
add basic coredump generation #5868
Conversation
src/commands/run.rs
Outdated
if let Some(coredump_path) = self.coredump_on_trap.as_ref() { | ||
if coredump_path.contains("%") { | ||
return Err(anyhow!( | ||
"The coredump-on-trap path does not support patterns yet." |
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.
For context, unix kernels allow to template the coredump path (https://sigquit.wordpress.com/2009/03/13/the-core-pattern/) using variables starting with %. We might want to support them in the future.
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.
Thanks!
One thought I have with this is that while placing it in the CLI is perhaps a good starting point we'll likely want this in a more libararified form at some point too because embeddings of Wasmtime don't necessarily all use the CLI. This current integration is also somewhat limited because the native call stack is gone and as a result impossible to ever recover locals and their information. Basically the "ideal" location for a core dump is probably at the place of the trap itself, which means that this wouldn't be possible to do from the CLI but rather would be required to be baked-in as well. Additionally being baked-in I think will be required to get access to non-exported globals/memories/etc.
None of that is to say though that this shouldn't be added here at this time. Moreso that a more "final" integration of core dumps I think will probably look significantly differently architected which isn't a problem but perhaps worth considering.
3d06eda
to
0a1843b
Compare
I changed the coredump generation to gracefully handle errors, as you pointed out. The dependencies are now trimmed down. Could you please have another look @alexcrichton? |
|
||
let coredump = coredump_builder | ||
.serialize() | ||
.map_err(|err| anyhow!("failed to serialize coredump: {}", err))?; |
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.
Could the .map_err
here become .context(..)
?
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.
The error is from wasm-coredump-builder
and is type Box<dyn std::error::Error + Sync + Send>
instead of the usual anyhow::Error
. Is that a problem?
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.
It's a problem insofar as the error's contexet is all lost here, only displaying one error in a possible chain of errors contained within the trait object. Whether your library exercises that I'm not sure, though.
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.
It's not a problem for my library, for now, as it doesn't add context to errors.
db931d7
to
0712e8f
Compare
Required by bytecodealliance#5868
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've added the new versions to #5894
|
||
let coredump = coredump_builder | ||
.serialize() | ||
.map_err(|err| anyhow!("failed to serialize coredump: {}", err))?; |
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.
It's a problem insofar as the error's contexet is all lost here, only displaying one error in a possible chain of errors contained within the trait object. Whether your library exercises that I'm not sure, though.
This change adds a basic coredump generation after a WebAssembly trap was entered. The coredump includes rudimentary stack / process debugging information. A new CLI argument is added to enable coredump generation: ``` wasmtime --coredump-on-trap=/path/to/coredump/file module.wasm ``` See ./docs/examples-coredump.md for a working example. Refs bytecodealliance#5732
0712e8f
to
e9f89f1
Compare
This change adds a basic coredump generation after a WebAssembly trap was entered. The coredump includes rudimentary stack / process debugging information.
A new CLI argument is added to enable coredump generation:
See ./docs/examples-coredump.md for a working example.
Refs #5732