Stream: git-wasmtime

Topic: wasmtime / PR #8203 Compile out wmemcheck-related libcall...


view this post on Zulip Wasmtime GitHub notifications bot (Mar 21 2024 at 01:43):

alexcrichton requested wasmtime-core-reviewers for a review on PR #8203.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 21 2024 at 01:43):

alexcrichton requested fitzgen for a review on PR #8203.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 21 2024 at 01:43):

alexcrichton opened PR #8203 from alexcrichton:no-wmemcheck-libcalls to bytecodealliance:main:

Currently even when the wmemcheck Cargo feature is disabled the various related libcalls are still compiled into wasmtime-runtime. Additionally their signatures are translated when lowering functions, although the signatures are never used. This commit adds #[cfg] annotations to compile these all out when they're not enabled.

Applying this change, however, uncovered a subtle bug in our libcalls. Libcalls are numbered in-order as-listed in the macro ignoring #[cfg], but they're assigned a runtime slot in a VMBuiltinFunctionsArray structure which does respect #[cfg]. This meant, for example, that if gc was enabled and wmemcheck was disabled, as is the default for our tests, then there was a hole in the numbering where libcall numbers were mismatched at runtime and compile time.

To fix this I've first added a const assertion that the runtime-number of libcalls equals the build-time number of libcalls. I then updated the macro a bit to plumb the #[cfg] differently and now libcalls are unconditionally defined regardless of cfgs but the implementation is std::process::abort() if it's compiled out.

This ended up having a large-ish impact on the disas test suite. Lots of functions have fewer signatures translation because wmemcheck, even when disabled, was translating a few signatures. This also had some assembly changes, too, because I believe functions are considered leaves based on whether they declare a signature or not, so declaring an unused signature was preventing all wasm functions from being considered leaves.

<!--
Please make sure you include the following information:

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 21 2024 at 01:58):

alexcrichton updated PR #8203.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 21 2024 at 14:53):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 21 2024 at 14:53):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 21 2024 at 14:53):

fitzgen created PR review comment:

Why continue to define the function, but just have it abort? Is it really easier than not defining the function?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 21 2024 at 14:53):

fitzgen created PR review comment:

Nice.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 21 2024 at 14:53):

fitzgen created PR review comment:

It seems nicer to allow(unused_doc_comments) in a few places than to lose the doc comments themselves, since they show up not only in rustdoc but also in editors using rust-analyzer/LSP and all that on hover and such.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 21 2024 at 15:24):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 21 2024 at 15:24):

alexcrichton created PR review comment:

Good question! The answer actually relates to your below comment of why to define panicking stubs rather than omitting the functions entirely. I actually first went down the route of entirely omitting the libcalls from both numbering and runtime, but I stopped after implementing that and realized that it could create a confusing situation:

These two machines have different libcall numbering schemes without knowing it. I ended up realizing this would be a pretty confusing issue to run into, so I wanted to instead have all the libcalls always have the same number whether they're actually needed or not at runtime.

That then gave rise to needing to have something deal with #[cfg] and #[cfg(not)]. This is below in the trampoline where it uses #[cfg(not(..))] and that changes the pattern of the macro because it no longer matches all attributes but only #[cfg] attributes on each of these items.

More-or-less I couldn't find a nice and easy way to make a macro pattern that would separate doc comments and cfg attributes, so I figured the loss of some minor rustdoc wasn't the end of the world and I demoted these to normal comments instead of doc comments.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 21 2024 at 16:37):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 21 2024 at 16:37):

fitzgen created PR review comment:

Makes sense, reasonable tradeoff. Thanks for explaining!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 21 2024 at 17:30):

alexcrichton merged PR #8203.


Last updated: Dec 23 2024 at 12:05 UTC