Stream: git-wasmtime

Topic: wasmtime / PR #7078 Make `wasmtime::WasmCoreDump` seriali...

Wasmtime GitHub notifications bot (Sep 22 2023 at 17:53):

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 and wasmtime-runtime:

• wasmtime::Instance::module: get the module that a wasmtime::Instance is an instance of. This is public, since it seems generally useful. This involved adding a new return value from ModuleRegistry::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 these pub(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 inside wasmtime-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 the StoreData 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 underlying Stored<T> as a hash key because it isn't stable across duplicate StoreData entries. So we have these new methods. They are only pub(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 deprecated wasmtime::FrameInfo::module_name since you can now instead do frame.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.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Sep 22 2023 at 17:53):

fitzgen requested alexcrichton for a review on PR #7078.

Wasmtime GitHub notifications bot (Sep 22 2023 at 17:53):

fitzgen requested wasmtime-core-reviewers for a review on PR #7078.

Wasmtime GitHub notifications bot (Sep 22 2023 at 18:08):

fitzgen updated PR #7078.

Wasmtime GitHub notifications bot (Sep 25 2023 at 15:04):

alexcrichton submitted PR review:

Nice :+1:

Mind adding a hash_key to Table for consistency, even though it's not used here? Also mind adding some API tests that hash_key is the same value if, for example, a global is extracted twice?

Hm as I say this Func doesn't implement hash_key either, so the "inconsistency" of tables not implementing it may be ok. Could Func implement it too though?

Wasmtime GitHub notifications bot (Sep 25 2023 at 15:04):

alexcrichton submitted PR review:

Nice :+1:

Mind adding a hash_key to Table for consistency, even though it's not used here? Also mind adding some API tests that hash_key is the same value if, for example, a global is extracted twice?

Hm as I say this Func doesn't implement hash_key either, so the "inconsistency" of tables not implementing it may be ok. Could Func implement it too though?

Wasmtime GitHub notifications bot (Sep 25 2023 at 15:04):

alexcrichton created PR review comment:

stray change?

Wasmtime GitHub notifications bot (Sep 25 2023 at 15:04):

alexcrichton created PR review comment:

I think you should be able to s/&'a self/&self/ here

Wasmtime GitHub notifications bot (Sep 25 2023 at 15:04):

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.

Wasmtime GitHub notifications bot (Sep 25 2023 at 15:04):

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 with StoreOpaque

Wasmtime GitHub notifications bot (Sep 25 2023 at 15:04):

alexcrichton created PR review comment:

Could this add documentation for what name is?

Wasmtime GitHub notifications bot (Sep 25 2023 at 15:04):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 25 2023 at 17:02):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Sep 25 2023 at 17:02):

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.

Wasmtime GitHub notifications bot (Sep 25 2023 at 17:06):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Sep 25 2023 at 17:06):

fitzgen created PR review comment:

https://github.com/bytecodealliance/wasmtime/issues/7083

Wasmtime GitHub notifications bot (Sep 25 2023 at 17:06):

fitzgen updated PR #7078.

Wasmtime GitHub notifications bot (Sep 25 2023 at 17:06):

fitzgen has enabled auto merge for PR #7078.

Wasmtime GitHub notifications bot (Sep 25 2023 at 17:10):

fitzgen has disabled auto merge for PR #7078.

Wasmtime GitHub notifications bot (Sep 25 2023 at 17:43):

fitzgen requested wasmtime-default-reviewers for a review on PR #7078.

Wasmtime GitHub notifications bot (Sep 25 2023 at 17:43):

fitzgen updated PR #7078.

Wasmtime GitHub notifications bot (Sep 25 2023 at 17:44):

fitzgen has enabled auto merge for PR #7078.

Wasmtime GitHub notifications bot (Sep 25 2023 at 18:49):

fitzgen updated PR #7078.

Wasmtime GitHub notifications bot (Sep 25 2023 at 18:50):

fitzgen updated PR #7078.

Wasmtime GitHub notifications bot (Sep 25 2023 at 19:30):

fitzgen updated PR #7078.

Wasmtime GitHub notifications bot (Sep 25 2023 at 19:30):

fitzgen has enabled auto merge for PR #7078.

Wasmtime GitHub notifications bot (Sep 25 2023 at 19:42):

fitzgen updated PR #7078.

Wasmtime GitHub notifications bot (Sep 25 2023 at 21:01):

fitzgen merged PR #7078.

Last updated: Nov 22 2024 at 16:03 UTC