Stream: git-wasmtime

Topic: wasmtime / PR #8763 Wasmtime: Implement the custom-page-s...


view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2024 at 20:15):

fitzgen opened PR #8763 from fitzgen:custom-page-sizes to bytecodealliance:main:

This commit adds support for the custom-page-sizes proposal to Wasmtime: https://github.com/WebAssembly/custom-page-sizes

I've migrated, fixed some bugs within, and extended the *.wast tests for this proposal from the wasm-tools repository. I intend to upstream them into the proposal shortly.

There is a new wasmtime::Config::wasm_custom_page_sizes_proposal method to enable or disable the proposal. It is disabled by default.

Our fuzzing config has been updated to turn this feature on/off as dictated by the arbitrary input given to us from the fuzzer.

Additionally, there were getting to be so many constructors for wasmtime::MemoryType that I added a builder rather than add yet another constructor.

In general, we store the log2(page_size) rather than the page size directly. This helps cut down on invalid states and properties we need to assert.

I've also intentionally written this code such that supporting any power of two page size (rather than just the exact values 1 and 65536 that are currently valid) will essentially just involve updating wasmparser's validation and removing some debug asserts in Wasmtime.

<!--
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 (Jun 10 2024 at 20:15):

fitzgen requested alexcrichton for a review on PR #8763.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2024 at 20:15):

fitzgen requested wasmtime-fuzz-reviewers for a review on PR #8763.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2024 at 20:15):

fitzgen requested elliottt for a review on PR #8763.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2024 at 20:15):

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

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2024 at 20:15):

fitzgen requested wasmtime-compiler-reviewers for a review on PR #8763.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2024 at 20:15):

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

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2024 at 20:56):

fitzgen updated PR #8763.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2024 at 20:58):

fitzgen updated PR #8763.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2024 at 21:05):

fitzgen updated PR #8763.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2024 at 22:02):

fitzgen updated PR #8763.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2024 at 22:29):

alexcrichton submitted PR review:

This looks great to me, thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2024 at 22:29):

alexcrichton submitted PR review:

This looks great to me, thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2024 at 22:29):

alexcrichton created PR review comment:

FITZGEN

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2024 at 22:29):

alexcrichton created PR review comment:

How come this is stored here? This should be statically known from ambient context elsewhere I think? (e.g. this just stores the runtime state of the base/current byte length)

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2024 at 22:29):

alexcrichton created PR review comment:

(and/or if it's just for 1-byte pages could this have special handling to handle that case with an icmp/select combo specifically?)

(and/or if you feel it's not worth it to specialize here mind leaving a comment?)

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2024 at 22:29):

alexcrichton created PR review comment:

Mind importing this function since it's called a few times in succession here?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2024 at 22:29):

alexcrichton created PR review comment:

Since we're already storing the log2 and cranelift doesn't optimize div, want to go ahead and change this to a shift-right?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2024 at 22:29):

alexcrichton created PR review comment:

Mind importing this function as well since it's used a few times?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2024 at 22:29):

alexcrichton created PR review comment:

How come the translation here changed?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2024 at 23:44):

github-actions[bot] commented on PR #8763:

Subscribe to Label Action

cc @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:x64", "cranelift:wasm", "fuzzing", "wasmtime:api", "wasmtime:config"

Thus the following users have been cc'd because of the following labels:

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2024 at 23:45):

github-actions[bot] commented on PR #8763:

Label Messager: wasmtime:config

It looks like you are changing Wasmtime's configuration options. Make sure to
complete this check list:

[fuzzing-config]: https://github.com/bytecodealliance/wasmtime/blob/ca0e8d0a1d8cefc0496dba2f77a670571d8fdcab/crates/fuzzing/src/generators.rs#L182-L194
[fuzzing-docs]: https://docs.wasmtime.dev/contributing-fuzzing.html


<details>

To modify this label's message, edit the <code>.github/label-messager/wasmtime-config.md</code> file.

To add new label messages or remove existing label messages, edit the
<code>.github/label-messager.json</code> configuration file.

Learn more.

</details>

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2024 at 23:48):

fitzgen updated PR #8763.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2024 at 23:49):

fitzgen edited a comment on PR #8763:

Label Messager: wasmtime:config

It looks like you are changing Wasmtime's configuration options. Make sure to
complete this check list:

[fuzzing-config]: https://github.com/bytecodealliance/wasmtime/blob/ca0e8d0a1d8cefc0496dba2f77a670571d8fdcab/crates/fuzzing/src/generators.rs#L182-L194
[fuzzing-docs]: https://docs.wasmtime.dev/contributing-fuzzing.html


<details>

To modify this label's message, edit the <code>.github/label-messager/wasmtime-config.md</code> file.

To add new label messages or remove existing label messages, edit the
<code>.github/label-messager.json</code> configuration file.

Learn more.

</details>

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2024 at 23:49):

fitzgen edited a comment on PR #8763:

Label Messager: wasmtime:config

It looks like you are changing Wasmtime's configuration options. Make sure to
complete this check list:

[fuzzing-config]: https://github.com/bytecodealliance/wasmtime/blob/ca0e8d0a1d8cefc0496dba2f77a670571d8fdcab/crates/fuzzing/src/generators.rs#L182-L194
[fuzzing-docs]: https://docs.wasmtime.dev/contributing-fuzzing.html


<details>

To modify this label's message, edit the <code>.github/label-messager/wasmtime-config.md</code> file.

To add new label messages or remove existing label messages, edit the
<code>.github/label-messager.json</code> configuration file.

Learn more.

</details>

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2024 at 23:49):

fitzgen edited a comment on PR #8763:

Label Messager: wasmtime:config

It looks like you are changing Wasmtime's configuration options. Make sure to
complete this check list:

[fuzzing-config]: https://github.com/bytecodealliance/wasmtime/blob/ca0e8d0a1d8cefc0496dba2f77a670571d8fdcab/crates/fuzzing/src/generators.rs#L182-L194
[fuzzing-docs]: https://docs.wasmtime.dev/contributing-fuzzing.html


<details>

To modify this label's message, edit the <code>.github/label-messager/wasmtime-config.md</code> file.

To add new label messages or remove existing label messages, edit the
<code>.github/label-messager.json</code> configuration file.

Learn more.

</details>

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2024 at 23:55):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2024 at 23:55):

fitzgen created PR review comment:

I think I was using it for an approach that I ultimately abandoned.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2024 at 23:56):

fitzgen updated PR #8763.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2024 at 23:59):

fitzgen updated PR #8763.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 11 2024 at 00:12):

elliottt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 11 2024 at 00:12):

elliottt created PR review comment:

Did this comment change at all, or was it just reformatted?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 11 2024 at 00:12):

elliottt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 11 2024 at 00:12):

elliottt created PR review comment:

Should this be an unwrap_or_else to avoid always computing the default?

        .unwrap_or_else(|| u64::MAX / page_size + 1)

Also, could you add a comment that explains the default?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 11 2024 at 16:06):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 11 2024 at 16:06):

fitzgen created PR review comment:

Yeah with single-byte pages -1 is now a valid return from a memory.{grow,size} in certain cases and so in this scenario we can't blindly sign extend. I can factor this out to another case for only when the memory has a 1-byte page size.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 11 2024 at 16:18):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 11 2024 at 16:18):

alexcrichton created PR review comment:

I think sign extension still works since you can't actually consume the entire address space for wasm though, right?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 11 2024 at 16:22):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 11 2024 at 16:22):

fitzgen created PR review comment:

But you could consume ~half of it and therefore have the sign bit set, which if we sign extend will result in a negative value / wrong unsigned value.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 11 2024 at 16:27):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 11 2024 at 16:27):

alexcrichton created PR review comment:

The only "memory growth failed" value though is -1, not any negative number?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 11 2024 at 16:32):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 11 2024 at 16:32):

fitzgen created PR review comment:

Yes, but you could have a valid memory size that has the sign bit set now, where that was impossible before because the maximum number of wasm pages that could fit in the address space wasn't large enough. That is, (u32::MAX / 2) + 1 is a valid memory size with single-byte page sizes, and it has its high bit set, so if we sign-extend that value then we end up giving an incorrect memory size to the Wasm.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 11 2024 at 16:34):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 11 2024 at 16:34):

fitzgen created PR review comment:

Just wrapped to 80 characters.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 11 2024 at 16:35):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 11 2024 at 16:35):

fitzgen created PR review comment:

I think it probably doesn't matter much either way in this case, since there isn't anything here that could prevent LLVM from cleaning this up itself (like a function call) but happy to make the change so that future readers don't ahve to reason through that.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 11 2024 at 16:54):

fitzgen updated PR #8763.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 11 2024 at 16:55):

fitzgen commented on PR #8763:

I believe all review feedback has been addressed at this point.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 11 2024 at 18:24):

fitzgen updated PR #8763.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 11 2024 at 18:39):

fitzgen updated PR #8763.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 11 2024 at 19:18):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 11 2024 at 19:18):

alexcrichton created PR review comment:

Right! I'm apparently forgetting how negative numbers work...

view this post on Zulip Wasmtime GitHub notifications bot (Jun 11 2024 at 19:22):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 11 2024 at 19:22):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 11 2024 at 19:22):

alexcrichton created PR review comment:

This change is fine by me, but this might reveal some fuzz bugs after this lands as we trip various assertions that previously assumed things were page aligned. I'm not sure how many of these options we frob during fuzzing though...

In any case it's fine to fix them as they arise, just wanted to give a heads up.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 11 2024 at 20:14):

fitzgen updated PR #8763.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 11 2024 at 20:14):

fitzgen has enabled auto merge for PR #8763.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 12 2024 at 17:28):

fitzgen updated PR #8763.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 12 2024 at 17:28):

fitzgen has enabled auto merge for PR #8763.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 12 2024 at 20:45):

fitzgen updated PR #8763.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 12 2024 at 20:45):

fitzgen has enabled auto merge for PR #8763.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 12 2024 at 21:03):

fitzgen updated PR #8763.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 12 2024 at 21:05):

alexcrichton created PR review comment:

I think this'll want to stay as true as it's trying to exercise the path where the OS says "no you can't have that much memory" as opposed to the store limiting memory, and if false is returned here we won't ever hit the OS. I'm not sure why this test previously passed on MIRI though and it's failing on this PR

view this post on Zulip Wasmtime GitHub notifications bot (Jun 12 2024 at 21:05):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 12 2024 at 21:08):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 12 2024 at 21:08):

fitzgen created PR review comment:

I'm not sure why this test previously passed on MIRI though and it's failing on this PR

I'm not sure either, AFAICT we should be doing the same thing as before.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 12 2024 at 21:11):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 12 2024 at 21:11):

alexcrichton created PR review comment:

Testing locally it looks like an error was returned before we hit MIRI before. I changed this line to a panic and the test passed.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 12 2024 at 21:13):

fitzgen updated PR #8763.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 12 2024 at 21:21):

fitzgen commented on PR #8763:

oh my god this CI failure is driving me crazy:

https://github.com/bytecodealliance/wasmtime/actions/runs/9489825551/job/26152011334?pr=8763#step:5:243

 error: unused import: `prelude::*`
  --> crates\wasmtime\src\runtime\vm.rs:14:5
   |
14 |     prelude::*, DefinedFuncIndex, DefinedMemoryIndex, HostPtr, ModuleInternedTypeIndex, VMOffsets,
   |     ^^^^^^^^^^
   |
   = note: `-D unused-imports` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(unused_imports)]`

If I remove that import, then all the err2anyhow() calls break. I've tried with all sorts of different cfgs and I can't reproduce locally, even when I copy the ./ci/run-tests.sh invocation from CI.

I have no idea why it thinks this is unused, AFAICT, it is definitely used.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 12 2024 at 21:32):

fitzgen updated PR #8763.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 12 2024 at 22:01):

fitzgen merged PR #8763.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 11 2024 at 09:30):

XinyuZeng submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 11 2024 at 09:30):

XinyuZeng created PR review comment:

This comment seems to be incomplete

view this post on Zulip Wasmtime GitHub notifications bot (Sep 13 2024 at 15:40):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 13 2024 at 15:40):

fitzgen created PR review comment:

Thanks for catching this!


Last updated: Dec 23 2024 at 12:05 UTC