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.wasm
See ./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.
-->
xtuc submitted PR review.
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.
xtuc edited PR review comment.
alexcrichton submitted PR review.
alexcrichton submitted PR review.
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-usewasmparser
crate 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_cpus
update, 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 likewasmparser
it 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-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.
alexcrichton created PR review comment:
You can shorten this a bit with just
bail!("...")
if you'd like
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?
alexcrichton created PR review comment:
This'll want to be a fallible conversion, only emitting a backtrace in the core dump if
WasmBacktrace
is available I think. (as opposed to returning a different kind of error if the backtrace is missing)
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.
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.
xtuc submitted PR review.
xtuc created PR review comment:
colored
,threadpool
andnum_cpus
are 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-builder
to 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.
xtuc submitted PR review.
xtuc created PR review comment:
Good point. Same with the errors bellow, I'll change the code to allow coredump generation to fail gracefully.
xtuc submitted PR review.
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
funcidx
in addition tocodeoffset
yet.
xtuc submitted PR review.
xtuc created PR review comment:
With the libararified / modular CLI approach, dependecies wouldn't matter as much right?
alexcrichton submitted PR review.
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-tools
repository (although feedback on those is always appreciated), but I believe thatwasm-encoder
is 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)
xtuc submitted PR review.
xtuc created PR review comment:
yeah I understand, I wasn't trying to argue against. I'm planning to rewrite
wasm-coredump-builder
to depend only on a new coredump specific encoding crate +wasm-encoder
. That should address the concern.
xtuc updated PR #5868 from sven/basic-coredump
to main
.
xtuc submitted PR review.
xtuc created PR review comment:
I published a new version of
wasm-coredump-builder
that depends onwasm-encoder
and 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_cpus
has been updated. Is it still related to my change?
xtuc updated PR #5868 from sven/basic-coredump
to main
.
xtuc requested alexcrichton for a review on PR #5868.
alexcrichton submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
I think this
err
is the wrong one being returned here (but I think it's just a small mistake due to name shadowing)
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.
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.
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
unreachable
condition, for example, there'd be no way to figure out which one in a function was hit.
alexcrichton created PR review comment:
Could the
.map_err
here become.context(..)
?
alexcrichton created PR review comment:
As a minor nit stylistically our error messages start with lowercase letters (including the "Core dumped at ..." message below)
alexcrichton created PR review comment:
Could this error also contain the path name?
xtuc submitted PR review.
xtuc created PR review comment:
Yes, good catch!
xtuc submitted PR review.
xtuc created PR review comment:
Ok, thanks.
xtuc submitted PR review.
xtuc created PR review comment:
The error is from
wasm-coredump-builder
and is typeBox<dyn std::error::Error + Sync + Send>
instead of the usualanyhow::Error
. Is that a problem?
xtuc updated PR #5868 from sven/basic-coredump
to main
.
xtuc edited PR review comment.
xtuc edited PR review comment.
xtuc created PR review comment:
I opened https://github.com/WebAssembly/tool-conventions/pull/202 to add
funcidx
to the coredump format.
xtuc submitted PR review.
xtuc submitted PR review.
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.
cfallin submitted PR review.
cfallin created PR review comment:
It looks like
func_offset
is 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"!)
xtuc submitted PR review.
xtuc created PR review comment:
Ah great, I misunderstood. Thanks
xtuc updated PR #5868 from sven/basic-coredump
to main
.
xtuc submitted PR review.
xtuc created PR review comment:
Added
codeoffset
.
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.wasm
See ./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.
-->
xtuc updated PR #5868 from sven/basic-coredump
to main
.
alexcrichton submitted PR review.
alexcrichton submitted PR review.
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.
alexcrichton created PR review comment:
Should this perhaps be a conditional invocation of
.codeoffset
due to it not always being available?
xtuc submitted PR review.
xtuc created PR review comment:
It's not a problem for my library, for now, as it doesn't add context to errors.
xtuc submitted PR review.
xtuc created PR review comment:
The result would be the same;
codeoffset
has to be specified in coredump and can default to 0 if unknown. At least now, it's explicit.
xtuc updated PR #5868 from sven/basic-coredump
to main
.
alexcrichton merged PR #5868.
Last updated: Dec 23 2024 at 12:05 UTC