Wasmtime GitHub notifications bot (Mar 21 2024 at 01:43): Wasmtime GitHub notifications bot (Mar 21 2024 at 01:43):alexcrichton requested wasmtime-core-reviewers for a review on PR #8203.
Wasmtime GitHub notifications bot (Mar 21 2024 at 01:43):alexcrichton requested fitzgen for a review on PR #8203.
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
wmemcheckCargo feature is disabled the various related libcalls are still compiled intowasmtime-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 aVMBuiltinFunctionsArraystructure which does respect#[cfg]. This meant, for example, that ifgcwas enabled andwmemcheckwas 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 isstd::process::abort()if it's compiled out.This ended up having a large-ish impact on the
disastest 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:
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
-->
Wasmtime GitHub notifications bot (Mar 21 2024 at 01:58):alexcrichton updated PR #8203.
Wasmtime GitHub notifications bot (Mar 21 2024 at 14:53):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Mar 21 2024 at 14:53):fitzgen submitted PR review.
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?
Wasmtime GitHub notifications bot (Mar 21 2024 at 14:53):fitzgen created PR review comment:
Nice.
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.
Wasmtime GitHub notifications bot (Mar 21 2024 at 15:24):alexcrichton submitted PR review.
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:
- One machine, with wasmtime features enabled, compiles a wasm module.
- Another machine, with minimal features enabled, loads this module.
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.
Wasmtime GitHub notifications bot (Mar 21 2024 at 16:37):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Mar 21 2024 at 16:37):fitzgen created PR review comment:
Makes sense, reasonable tradeoff. Thanks for explaining!
Wasmtime GitHub notifications bot (Mar 21 2024 at 17:30):alexcrichton merged PR #8203.
Last updated: Oct 23 2024 at 20:03 UTC