fitzgen opened PR #7078 from fitzgen:serializable-core-dumps
to bytecodealliance:main
:
This commit makes it so that the library type for core dumps is serializable into the standard binary format for core dumps.
Additionally, this commit makes it so that we use the library type for generating core dumps in the CLI. We previously were using a one-off implementation of core dump generation that only had backtrace information and no instances, modules, globals, or memories included. The library type has all that information, so the core dumps produced by our CLI will both be more featureful and be generated by shared code paths going forward.
Along the way, implementing all this required some new helper methods sprinkled throughout
wasmtime
andwasmtime-runtime
:
wasmtime::Instance::module
: get the module that awasmtime::Instance
is an instance of. This is public, since it seems generally useful. This involved adding a new return value fromModuleRegistry::register_module
that is an identifier that can be used to recover a reference to the registered module.
wasmtime::Instance::all_{globals,memories}
: get the full global/memory index space. I made thesepub(crate)
out of caution. I don't think we want to commit to exposing non-exported things in the public API, even if we internally need them for debugging-related features like core dumps. These also needed corresponding methods insidewasmtime-runtime
.
wasmtime::{Global,Memory}::hash_key
: this was needed to work around the fact that each time you call{Global,Memory}::from_wasmtime
, it creates a new entry in theStoreData
and so you can get duplicates. But we need to key some hash maps on globals and memories when constructing core dumps, so we can't treat the underlyingStored<T>
as a hash key because it isn't stable across duplicateStoreData
entries. So we have these new methods. They are onlypub(crate)
, are definitely implementation details, and aren't exposed in the public API.
wasmtime::FrameInfo::module
: Each frame in a backtrace now keeps a handle to its associated module instead of just the name. This is publicly exposed because it seems generally useful. This means I also deprecatedwasmtime::FrameInfo::module_name
since you can now instead doframe.module().name()
to get that exact same info. I updated callers inside the repo.<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
fitzgen requested alexcrichton for a review on PR #7078.
fitzgen requested wasmtime-core-reviewers for a review on PR #7078.
fitzgen updated PR #7078.
alexcrichton submitted PR review:
Nice :+1:
Mind adding a
hash_key
toTable
for consistency, even though it's not used here? Also mind adding some API tests thathash_key
is the same value if, for example, a global is extracted twice?Hm as I say this
Func
doesn't implementhash_key
either, so the "inconsistency" of tables not implementing it may be ok. CouldFunc
implement it too though?
alexcrichton submitted PR review:
Nice :+1:
Mind adding a
hash_key
toTable
for consistency, even though it's not used here? Also mind adding some API tests thathash_key
is the same value if, for example, a global is extracted twice?Hm as I say this
Func
doesn't implementhash_key
either, so the "inconsistency" of tables not implementing it may be ok. CouldFunc
implement it too though?
alexcrichton created PR review comment:
stray change?
alexcrichton created PR review comment:
I think you should be able to
s/&'a self/&self/
here
alexcrichton created PR review comment:
FWIW we haven't done this much in the past with
#[deprecated]
methods so I think it'd be ok to remove this as well.
alexcrichton created PR review comment:
If you're feeling sufficiently motivated I think the
T
here could be dropped in theory but would require some internal refactoring as it seems like it's required or calling public APIs which don't have private equivalents working withStoreOpaque
alexcrichton created PR review comment:
Could this add documentation for what
name
is?
alexcrichton submitted PR review.
fitzgen submitted PR review.
fitzgen created PR review comment:
it seems like it's required or calling public APIs which don't have private equivalents working with
StoreOpaque
Yeah, this is why the
T
is propagated. A bit unfortunate, but also something I don't want to deal with at this very moment. I can file a follow up issue.
fitzgen submitted PR review.
fitzgen created PR review comment:
fitzgen updated PR #7078.
fitzgen has enabled auto merge for PR #7078.
fitzgen has disabled auto merge for PR #7078.
fitzgen requested wasmtime-default-reviewers for a review on PR #7078.
fitzgen updated PR #7078.
fitzgen has enabled auto merge for PR #7078.
fitzgen updated PR #7078.
fitzgen updated PR #7078.
fitzgen updated PR #7078.
fitzgen has enabled auto merge for PR #7078.
fitzgen updated PR #7078.
fitzgen merged PR #7078.
Last updated: Dec 23 2024 at 13:07 UTC