wasmtime / PR #6513 Add core dump support to the runtime · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #6513 Add core dump support to the runtime

Wasmtime GitHub notifications bot (Jun 02 2023 at 22:37):

itsrainy opened PR #6513 from bytecodealliance:rainy/coredumps to bytecodealliance:main:

This adds a configuration option and machinery to capture a core dump when
a trap occurs and attach it to the resulting anyhow::Error that gets bubbled up to
the caller. I've created a CoreDumpStack structure in the runtime, which is
currently just a backtrace until we design a way to recover the locals and
stack values when a trap occurs. When that CoreDumpStack gets converted to
a wasmtime::WasmCoreDump, we add additional information from the Store such
as globals, memories, and instance information.

A lot of this is mechanistically similar to how backtraces
are captured and attached to errors. Given that they both are attached as
context to anyhow::Errors, setting coredump_on_trap to true will supersede
any setting for wasm_backtrace.

Wasmtime GitHub notifications bot (Jun 02 2023 at 22:37):

itsrainy requested pchickey for a review on PR #6513.

Wasmtime GitHub notifications bot (Jun 02 2023 at 22:37):

itsrainy requested wasmtime-core-reviewers for a review on PR #6513.

Wasmtime GitHub notifications bot (Jun 02 2023 at 22:37):

itsrainy requested wasmtime-default-reviewers for a review on PR #6513.

Wasmtime GitHub notifications bot (Jun 02 2023 at 23:13):

itsrainy updated PR #6513.

Wasmtime GitHub notifications bot (Jun 08 2023 at 18:18):

fitzgen submitted PR review:

This is looking good!

Before this can land, we still need some smoke tests under tests/all/coredump.rs though. Should test that we get the expected stack, modules, instances, globals, and memories in the resulting coredumps.

To be explicit, mostly for folks following along at home, I'm okay with waiting for follow up PRs to implement

serialization into the Wasm coredump format for wastime::WasmCoreDump

implementing wasmtime --coredump-on-trap=... in the CLI on top of wasmtime::WasmCoreDump.

A few nitpicks inline below.

Thanks!

Wasmtime GitHub notifications bot (Jun 08 2023 at 18:18):

fitzgen submitted PR review:

This is looking good!

Before this can land, we still need some smoke tests under tests/all/coredump.rs though. Should test that we get the expected stack, modules, instances, globals, and memories in the resulting coredumps.

To be explicit, mostly for folks following along at home, I'm okay with waiting for follow up PRs to implement

serialization into the Wasm coredump format for wastime::WasmCoreDump

implementing wasmtime --coredump-on-trap=... in the CLI on top of wasmtime::WasmCoreDump.

A few nitpicks inline below.

Thanks!

Wasmtime GitHub notifications bot (Jun 08 2023 at 18:18):

fitzgen created PR review comment:

We can make this a timeless comment rather than a TODO, since it won't ever be the case that we always recover all Wasm state, regardless of the compiler in use and debug flags and all that:
/// Note that some state, such as Wasm locals or values on the operand stack,
/// may be optimized away by the compiler or otherwise not recovered in the
/// coredump.

Wasmtime GitHub notifications bot (Jun 08 2023 at 18:18):

fitzgen created PR review comment:

This doesn't actually override the wasm_backtrace option internally, and if someone tries to downcast the anyhow::Error to a WasmBacktrace they won't get one if they only set this option and not the other, so I think these docs are slightly misleading/confusing. I think we can remove the whole second sentence here.

Also, and this is like the ultimate nitpick, we try to follow the Rust/rustdoc idioms for doc comments (especially for Config). rustdoc will inline the first paragraph of a doc comment into the "index" view of types/methods/etc so having a single, short, and to the point sentence for the first paragraph is good. Then there can be any number of paragraphs (separated by newlines so rustdoc recognizes them as paragraphs) providing more details for when users are on the methods details doc page. See the bot's comment (you have to expand the <details>) for an example.

Wasmtime GitHub notifications bot (Jun 08 2023 at 18:18):

fitzgen created PR review comment:

Can you double check that these links all work in the rendered html docs? IIRC using types as URLs requires [text][the::Type] not [text](the::Type) and I sort of remember needing crate:: prefixes for local-to-the-crate types (eg [Trap][crate::Trap]) but I'm not 100% sure about it. As long as everything works, I'm happy!

Wasmtime GitHub notifications bot (Jun 08 2023 at 18:18):

fitzgen created PR review comment:

Is there a particular reason this is a Display impl and not a Debug impl? Or why we can't just derive(Debug)?

Wasmtime GitHub notifications bot (Jun 08 2023 at 18:18):

fitzgen created PR review comment:

I think if we have a backtrace and a coredump, we want to attach them both as context to the error, so I think it would be better to do something like this:

let mut error = ...;

if let Some(bt) = backtrace {
    let bt = WasmBacktrace::from_captured(store, bt, pc);
    if !bt.wasm_trace.is_empty() {
        error = error.context(bt);
    }
}

if let Some(cd) = coredump {
    let bt = WasmBacktrace::from_captured(store, cd.bt, pc);
    let cd = WasmCoreDump::new(store, bt);
    error = error.context(cd);
}

error

Wasmtime GitHub notifications bot (Jun 08 2023 at 18:18):

fitzgen created PR review comment:

Nitpick: as written, this first sentence suggests that coredumps are always attached to the errors, so I think we should add this suffix:

... when the Config::coredump_on_trap option is enabled.

Wasmtime GitHub notifications bot (Jun 21 2023 at 18:39):

itsrainy created PR review comment:

Is there a particular reason this is a Display impl and not a Debug impl?

I think anyhow context requires it to implement Display iirc. I did try to derive(Debug) but it contains a vec of Module which doesn't derive(Debug) (and transitively contains other things that also don't derive it).

Wasmtime GitHub notifications bot (Jun 21 2023 at 19:34):

itsrainy updated PR #6513.

Wasmtime GitHub notifications bot (Jun 21 2023 at 19:52):

itsrainy updated PR #6513.

Wasmtime GitHub notifications bot (Jun 21 2023 at 19:54):

itsrainy created PR review comment:

Apparently it accepts both () and []! I switched it to [] to be consistent. Also, yep it did need the crate prefix!

Wasmtime GitHub notifications bot (Jun 29 2023 at 20:09):

itsrainy updated PR #6513.

Wasmtime GitHub notifications bot (Jun 29 2023 at 22:30):

itsrainy updated PR #6513.

Wasmtime GitHub notifications bot (Jun 29 2023 at 23:04):

itsrainy updated PR #6513.

Wasmtime GitHub notifications bot (Jun 29 2023 at 23:22):

itsrainy has marked PR #6513 as ready for review.

Wasmtime GitHub notifications bot (Jun 29 2023 at 23:33):

itsrainy updated PR #6513.

Wasmtime GitHub notifications bot (Jun 30 2023 at 18:16):

itsrainy updated PR #6513.

Wasmtime GitHub notifications bot (Jul 07 2023 at 15:36):

itsrainy updated PR #6513.

Wasmtime GitHub notifications bot (Jul 19 2023 at 20:57):

itsrainy updated PR #6513.

Wasmtime GitHub notifications bot (Jul 19 2023 at 21:10):

itsrainy updated PR #6513.

Wasmtime GitHub notifications bot (Jul 26 2023 at 20:50):

fitzgen submitted PR review:

LGTM! Feel free to enqueue to merge when this is rebased and conflicts are resolved.

Wasmtime GitHub notifications bot (Jul 27 2023 at 15:13):

itsrainy updated PR #6513.

Wasmtime GitHub notifications bot (Aug 01 2023 at 14:05):

itsrainy updated PR #6513.

Wasmtime GitHub notifications bot (Aug 01 2023 at 16:39):

itsrainy merged PR #6513.

Last updated: Nov 22 2024 at 16:03 UTC