Wasmtime GitHub notifications bot (Feb 23 2023 at 19:02): Wasmtime GitHub notifications bot (Feb 23 2023 at 19:02):xtuc opened PR #5868 from sven/basic-coredump to main:
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/directory module.wasmSee ./docs/examples-coredump.md for a working example.
Refs https://github.com/bytecodealliance/wasmtime/issues/5732
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[x] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (Feb 23 2023 at 19:10):xtuc submitted PR review.
Wasmtime GitHub notifications bot (Feb 23 2023 at 19:10):xtuc created PR review comment:
For context, unix kernels allows 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.
Wasmtime GitHub notifications bot (Feb 23 2023 at 19:10):xtuc edited PR review comment.
Wasmtime GitHub notifications bot (Feb 23 2023 at 19:50):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Feb 23 2023 at 19:50):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Feb 23 2023 at 19:50):alexcrichton created PR review comment:
This is also visible on CI, but we're stricter-than-average about the dependencies we pull in for Wasmtime. Relying on something external here is fine but this specifically pulls in a number of things which I think we'll want to change/address:
colored- in theory this shouldn't be necessary for just building core dumpscore-wasm-ast- this looks like it may be duplicating the functionality of the already-in-usewasmparsercrate perhaps? If that's the case we'd prefer to only have one definition of parsing wasm within wasmtime.hermit-abi- I think this is due to thenum_cpusupdate, but is the update required?threadpool- I think in theory this shouldn't be necessary for a core dump builderwasm-printer- does this perhaps duplicate the functionality already present inwasmprinter? If so likewasmparserit would be best to only have one implementation of this rather than multiple.I'll also note that we're also already using the
wasm-encodercrate for creating core wasm modules, so if core dump functionality could be added to that or somehow leverage that I think that would be ideal.
Wasmtime GitHub notifications bot (Feb 23 2023 at 19:50):alexcrichton created PR review comment:
You can shorten this a bit with just
bail!("...")if you'd like
Wasmtime GitHub notifications bot (Feb 23 2023 at 19:50):alexcrichton created PR review comment:
Handling errors here is a bit interesting because while these operations are fallible I don't think that the error here wants to necessarily "overwrite" the trap error. I also don't know of a great way to "fuse" errors either. Perhaps for tnow this could log a warning message or similar?
Wasmtime GitHub notifications bot (Feb 23 2023 at 19:50):alexcrichton created PR review comment:
This'll want to be a fallible conversion, only emitting a backtrace in the core dump if
WasmBacktraceis available I think. (as opposed to returning a different kind of error if the backtrace is missing)
Wasmtime GitHub notifications bot (Feb 23 2023 at 19:50):alexcrichton created PR review comment:
The backtrace frames here should also have offset information to get to the precise opcode if you'd like to add that.
Wasmtime GitHub notifications bot (Feb 23 2023 at 19:50):alexcrichton created PR review comment:
I think this logic below may want to be conditional on a
err.is::<wasmtime::Trap>()condition since there's a number of other unrelated things that could go wrong during execution.
Wasmtime GitHub notifications bot (Feb 23 2023 at 20:25):xtuc submitted PR review.
Wasmtime GitHub notifications bot (Feb 23 2023 at 20:25):xtuc created PR review comment:
colored,threadpoolandnum_cpusare dependencies not directly used bywasm-coredump-builder, I'll make sure to remove them.
core-wasm-ast(in theory, just defines the stucts to represent coredump) andwasm-printer(maybe a bad name, it's used to serialize the coredump to binary Wasm) are required bywasm-coredump-builder.I'll also note that we're also already using the wasm-encoder crate for creating core wasm modules, so if core dump functionality could be added to that or somehow leverage that I think that would be ideal.
That's indeed duplicated features with my crates. I believe I can rewrite
wasm-coredump-builderto only usewasm-encoder. A disadvantage of that approach on my side is that it won't integrate well with my tools / crates, but could be specific to wasmtime.
Wasmtime GitHub notifications bot (Feb 23 2023 at 20:29):xtuc submitted PR review.
Wasmtime GitHub notifications bot (Feb 23 2023 at 20:29):xtuc created PR review comment:
Good point. Same with the errors bellow, I'll change the code to allow coredump generation to fail gracefully.
Wasmtime GitHub notifications bot (Feb 23 2023 at 20:30):xtuc submitted PR review.
Wasmtime GitHub notifications bot (Feb 23 2023 at 20:30):xtuc created PR review comment:
Yeah, we'll probably want that in the future. I haven't changed (or decided) the coredump format to allow
funcidxin addition tocodeoffsetyet.
Wasmtime GitHub notifications bot (Feb 23 2023 at 20:33):xtuc submitted PR review.
Wasmtime GitHub notifications bot (Feb 23 2023 at 20:33):xtuc created PR review comment:
With the libararified / modular CLI approach, dependecies wouldn't matter as much right?
Wasmtime GitHub notifications bot (Feb 23 2023 at 21:06):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Feb 23 2023 at 21:06):alexcrichton created PR review comment:
To clarify I don't mean to pass judgement on which one should be used per se, but within the context of Wasmtime's dependency graph we'd prefer to keep only one implementation of things rather than bringing together various islands of ecosystems. I don't mean to say that you should rewrite crates on top of the crates in the
wasm-toolsrepository (although feedback on those is always appreciated), but I believe thatwasm-encoderis high-level enough and the core dump format is simple enough that doing it "inline" as opposed to using an external crate wouldn't be the end of the world (not that using an external crate is wrong, again just trying to keep the dependencies in Wasmtime under control)
Wasmtime GitHub notifications bot (Feb 23 2023 at 21:50):xtuc submitted PR review.
Wasmtime GitHub notifications bot (Feb 23 2023 at 21:50):xtuc created PR review comment:
yeah I understand, I wasn't trying to argue against. I'm planning to rewrite
wasm-coredump-builderto depend only on a new coredump specific encoding crate +wasm-encoder. That should address the concern.
Wasmtime GitHub notifications bot (Feb 23 2023 at 22:49):xtuc updated PR #5868 from sven/basic-coredump to main.
Wasmtime GitHub notifications bot (Feb 23 2023 at 22:53):xtuc submitted PR review.
Wasmtime GitHub notifications bot (Feb 23 2023 at 22:53):xtuc created PR review comment:
I published a new version of
wasm-coredump-builderthat depends onwasm-encoderand two coredump specific crates:
- https://github.com/xtuc/wasm-coredump/tree/main/lib/coredump-encoder
- https://github.com/xtuc/wasm-coredump/tree/main/lib/coredump-types
I'm not sure why
num_cpushas been updated. Is it still related to my change?
Wasmtime GitHub notifications bot (Feb 24 2023 at 16:19):xtuc updated PR #5868 from sven/basic-coredump to main.
Wasmtime GitHub notifications bot (Feb 24 2023 at 16:22):xtuc requested alexcrichton for a review on PR #5868.
Wasmtime GitHub notifications bot (Feb 24 2023 at 17:06):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Feb 24 2023 at 17:06):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Feb 24 2023 at 17:06):alexcrichton created PR review comment:
I think this
erris the wrong one being returned here (but I think it's just a small mistake due to name shadowing)
Wasmtime GitHub notifications bot (Feb 24 2023 at 17:06):alexcrichton created PR review comment:
For now we're enforcing that the edits to the
supply-chain/*folder are only done by "trusted reviewers" of the project since this affects our supply-chain security (and others who import these audits). To that end thanks for adding these here, but our current process (which isn't well documented) is that if a contributor needs new vet entries we post a separate PR which the contributor's PR is then rebased on top of.Once that PR lands, though, you should be able to rebase this PR on top of that one and back out the changes to the
supply-chain/*directory.
Wasmtime GitHub notifications bot (Feb 24 2023 at 17:06):alexcrichton created PR review comment:
Since this will be a user-facing error message I think it might be better to reword this as "no wasm backtrace found to generate core dump with" or something along those lines.
Wasmtime GitHub notifications bot (Feb 24 2023 at 17:06):alexcrichton created PR review comment:
Sorry the context is lost on the prior comment with the update here, but again while this isn't a blocker not including the function offset seems like it could hinder the usefulness of backtraces since you don't get where you are in a function, just that you're in a certain function. If you're exploring a certian
unreachablecondition, for example, there'd be no way to figure out which one in a function was hit.
Wasmtime GitHub notifications bot (Feb 24 2023 at 17:06):alexcrichton created PR review comment:
Could the
.map_errhere become.context(..)?
Wasmtime GitHub notifications bot (Feb 24 2023 at 17:06):alexcrichton created PR review comment:
As a minor nit stylistically our error messages start with lowercase letters (including the "Core dumped at ..." message below)
Wasmtime GitHub notifications bot (Feb 24 2023 at 17:06):alexcrichton created PR review comment:
Could this error also contain the path name?
Wasmtime GitHub notifications bot (Feb 24 2023 at 17:43):xtuc submitted PR review.
Wasmtime GitHub notifications bot (Feb 24 2023 at 17:43):xtuc created PR review comment:
Yes, good catch!
Wasmtime GitHub notifications bot (Feb 24 2023 at 17:45):xtuc submitted PR review.
Wasmtime GitHub notifications bot (Feb 24 2023 at 17:45):xtuc created PR review comment:
Ok, thanks.
Wasmtime GitHub notifications bot (Feb 24 2023 at 17:52):xtuc submitted PR review.
Wasmtime GitHub notifications bot (Feb 24 2023 at 17:52):xtuc created PR review comment:
The error is from
wasm-coredump-builderand is typeBox<dyn std::error::Error + Sync + Send>instead of the usualanyhow::Error. Is that a problem?
Wasmtime GitHub notifications bot (Feb 24 2023 at 17:54):xtuc updated PR #5868 from sven/basic-coredump to main.
Wasmtime GitHub notifications bot (Feb 24 2023 at 17:55):xtuc edited PR review comment.
Wasmtime GitHub notifications bot (Feb 24 2023 at 17:56):xtuc edited PR review comment.
Wasmtime GitHub notifications bot (Feb 24 2023 at 18:12):xtuc created PR review comment:
I opened https://github.com/WebAssembly/tool-conventions/pull/202 to add
funcidxto the coredump format.
Wasmtime GitHub notifications bot (Feb 24 2023 at 18:12):xtuc submitted PR review.
Wasmtime GitHub notifications bot (Feb 24 2023 at 18:27):xtuc submitted PR review.
Wasmtime GitHub notifications bot (Feb 24 2023 at 18:27):xtuc created PR review comment:
unfortunately, I don't see a way to retrieve the instruction offset in https://docs.rs/wasmtime/6.0.0/wasmtime/struct.FrameInfo.html.
There's a method to retrieve the func offset in the module: https://docs.rs/wasmtime/6.0.0/wasmtime/struct.FrameInfo.html#method.func_offset.
Wasmtime GitHub notifications bot (Feb 24 2023 at 18:33):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Feb 24 2023 at 18:33):cfallin created PR review comment:
It looks like
func_offsetis actually what is needed? The documentation reads "Returns the offset from the original wasm module’s function to this frame’s program counter." i.e., it encodesPC - function_start, the offset of the instruction. (The name is somewhat confusing though -- "offset in function" rather than "offset of function"!)
Wasmtime GitHub notifications bot (Feb 24 2023 at 18:38):xtuc submitted PR review.
Wasmtime GitHub notifications bot (Feb 24 2023 at 18:38):xtuc created PR review comment:
Ah great, I misunderstood. Thanks
Wasmtime GitHub notifications bot (Feb 24 2023 at 20:05):xtuc updated PR #5868 from sven/basic-coredump to main.
Wasmtime GitHub notifications bot (Feb 24 2023 at 20:06):xtuc submitted PR review.
Wasmtime GitHub notifications bot (Feb 24 2023 at 20:06):xtuc created PR review comment:
Added
codeoffset.
Wasmtime GitHub notifications bot (Feb 24 2023 at 20:59):xtuc edited PR #5868 from sven/basic-coredump to main:
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.wasmSee ./docs/examples-coredump.md for a working example.
Refs https://github.com/bytecodealliance/wasmtime/issues/5732
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[x] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (Feb 27 2023 at 21:19):xtuc updated PR #5868 from sven/basic-coredump to main.
Wasmtime GitHub notifications bot (Feb 28 2023 at 15:31):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Feb 28 2023 at 15:31):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Feb 28 2023 at 15:31):alexcrichton created PR review comment:
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.
Wasmtime GitHub notifications bot (Feb 28 2023 at 15:31):alexcrichton created PR review comment:
Should this perhaps be a conditional invocation of
.codeoffsetdue to it not always being available?
Wasmtime GitHub notifications bot (Feb 28 2023 at 15:39):xtuc submitted PR review.
Wasmtime GitHub notifications bot (Feb 28 2023 at 15:39):xtuc created PR review comment:
It's not a problem for my library, for now, as it doesn't add context to errors.
Wasmtime GitHub notifications bot (Feb 28 2023 at 15:41):xtuc submitted PR review.
Wasmtime GitHub notifications bot (Feb 28 2023 at 15:41):xtuc created PR review comment:
The result would be the same;
codeoffsethas to be specified in coredump and can default to 0 if unknown. At least now, it's explicit.
Wasmtime GitHub notifications bot (Feb 28 2023 at 20:03):xtuc updated PR #5868 from sven/basic-coredump to main.
Wasmtime GitHub notifications bot (Feb 28 2023 at 21:06):alexcrichton merged PR #5868.
Last updated: Nov 22 2024 at 16:03 UTC