Stream: git-wasmtime

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

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:

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

fitzgen requested alexcrichton for a review on PR #8763.

Wasmtime GitHub notifications bot (Jun 10 2024 at 20:15):

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

Wasmtime GitHub notifications bot (Jun 10 2024 at 20:15):

fitzgen requested elliottt for a review on PR #8763.

Wasmtime GitHub notifications bot (Jun 10 2024 at 20:15):

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

Wasmtime GitHub notifications bot (Jun 10 2024 at 20:15):

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

Wasmtime GitHub notifications bot (Jun 10 2024 at 20:15):

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

Wasmtime GitHub notifications bot (Jun 10 2024 at 20:56):

fitzgen updated PR #8763.

Wasmtime GitHub notifications bot (Jun 10 2024 at 20:58):

fitzgen updated PR #8763.

Wasmtime GitHub notifications bot (Jun 10 2024 at 21:05):

fitzgen updated PR #8763.

Wasmtime GitHub notifications bot (Jun 10 2024 at 22:02):

fitzgen updated PR #8763.

Wasmtime GitHub notifications bot (Jun 10 2024 at 22:29):

alexcrichton submitted PR review:

This looks great to me, thanks!

Wasmtime GitHub notifications bot (Jun 10 2024 at 22:29):

alexcrichton submitted PR review:

This looks great to me, thanks!

Wasmtime GitHub notifications bot (Jun 10 2024 at 22:29):

alexcrichton created PR review comment:

FITZGEN

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)

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

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?

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?

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?

Wasmtime GitHub notifications bot (Jun 10 2024 at 22:29):

alexcrichton created PR review comment:

How come the translation here changed?

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:

• fitzgen: fuzzing

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

Learn more.
</details>

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:

• [ ] If you added a new Config method, you wrote extensive documentation for
  it.

  <details>

  Our documentation should be of the following form:

  ```text
  Short, simple summary sentence.

  More details. These details can be multiple paragraphs. There should be
  information about not just the method, but its parameters and results as
  well.

  Is this method fallible? If so, when can it return an error?

  Can this method panic? If so, when does it panic?

  Example

  Optional example here.
  ```

  </details>

• [ ] If you added a new Config method, or modified an existing one, you
  ensured that this configuration is exercised by the fuzz targets.

  <details>

  For example, if you expose a new strategy for allocating the next instance
  slot inside the pooling allocator, you should ensure that at least one of our
  fuzz targets exercises that new strategy.

  Often, all that is required of you is to ensure that there is a knob for this
  configuration option in [wasmtime_fuzzing::Config][fuzzing-config] (or one
  of its nested structs).

  Rarely, this may require authoring a new fuzz target to specifically test this
  configuration. See [our docs on fuzzing][fuzzing-docs] for more details.

  </details>

• [ ] If you are enabling a configuration option by default, make sure that it
  has been fuzzed for at least two weeks before turning it on by default.

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

Wasmtime GitHub notifications bot (Jun 10 2024 at 23:48):

fitzgen updated PR #8763.

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:

• [x] If you added a new Config method, you wrote extensive documentation for
  it.

  <details>

  Our documentation should be of the following form:

  ```text
  Short, simple summary sentence.

  More details. These details can be multiple paragraphs. There should be
  information about not just the method, but its parameters and results as
  well.

  Is this method fallible? If so, when can it return an error?

  Can this method panic? If so, when does it panic?

  Example

  Optional example here.
  ```

  </details>

• [ ] If you added a new Config method, or modified an existing one, you
  ensured that this configuration is exercised by the fuzz targets.

  <details>

  For example, if you expose a new strategy for allocating the next instance
  slot inside the pooling allocator, you should ensure that at least one of our
  fuzz targets exercises that new strategy.

  Often, all that is required of you is to ensure that there is a knob for this
  configuration option in [wasmtime_fuzzing::Config][fuzzing-config] (or one
  of its nested structs).

  Rarely, this may require authoring a new fuzz target to specifically test this
  configuration. See [our docs on fuzzing][fuzzing-docs] for more details.

  </details>

• [ ] If you are enabling a configuration option by default, make sure that it
  has been fuzzed for at least two weeks before turning it on by default.

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

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:

• [x] If you added a new Config method, you wrote extensive documentation for
  it.

  <details>

  Our documentation should be of the following form:

  ```text
  Short, simple summary sentence.

  More details. These details can be multiple paragraphs. There should be
  information about not just the method, but its parameters and results as
  well.

  Is this method fallible? If so, when can it return an error?

  Can this method panic? If so, when does it panic?

  Example

  Optional example here.
  ```

  </details>

• [x] If you added a new Config method, or modified an existing one, you
  ensured that this configuration is exercised by the fuzz targets.

  <details>

  For example, if you expose a new strategy for allocating the next instance
  slot inside the pooling allocator, you should ensure that at least one of our
  fuzz targets exercises that new strategy.

  Often, all that is required of you is to ensure that there is a knob for this
  configuration option in [wasmtime_fuzzing::Config][fuzzing-config] (or one
  of its nested structs).

  Rarely, this may require authoring a new fuzz target to specifically test this
  configuration. See [our docs on fuzzing][fuzzing-docs] for more details.

  </details>

• [ ] If you are enabling a configuration option by default, make sure that it
  has been fuzzed for at least two weeks before turning it on by default.

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

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:

• [x] If you added a new Config method, you wrote extensive documentation for
  it.

  <details>

  Our documentation should be of the following form:

  ```text
  Short, simple summary sentence.

  More details. These details can be multiple paragraphs. There should be
  information about not just the method, but its parameters and results as
  well.

  Is this method fallible? If so, when can it return an error?

  Can this method panic? If so, when does it panic?

  Example

  Optional example here.
  ```

  </details>

• [x] If you added a new Config method, or modified an existing one, you
  ensured that this configuration is exercised by the fuzz targets.

  <details>

  For example, if you expose a new strategy for allocating the next instance
  slot inside the pooling allocator, you should ensure that at least one of our
  fuzz targets exercises that new strategy.

  Often, all that is required of you is to ensure that there is a knob for this
  configuration option in [wasmtime_fuzzing::Config][fuzzing-config] (or one
  of its nested structs).

  Rarely, this may require authoring a new fuzz target to specifically test this
  configuration. See [our docs on fuzzing][fuzzing-docs] for more details.

  </details>

• [x] If you are enabling a configuration option by default, make sure that it
  has been fuzzed for at least two weeks before turning it on by default.

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

Wasmtime GitHub notifications bot (Jun 10 2024 at 23:55):

fitzgen submitted PR review.

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.

Wasmtime GitHub notifications bot (Jun 10 2024 at 23:56):

fitzgen updated PR #8763.

Wasmtime GitHub notifications bot (Jun 10 2024 at 23:59):

fitzgen updated PR #8763.

Wasmtime GitHub notifications bot (Jun 11 2024 at 00:12):

elliottt submitted PR review.

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?

Wasmtime GitHub notifications bot (Jun 11 2024 at 00:12):

elliottt submitted PR review.

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?

Wasmtime GitHub notifications bot (Jun 11 2024 at 16:06):

fitzgen submitted PR review.

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.

Wasmtime GitHub notifications bot (Jun 11 2024 at 16:18):

alexcrichton submitted PR review.

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?

Wasmtime GitHub notifications bot (Jun 11 2024 at 16:22):

fitzgen submitted PR review.

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.

Wasmtime GitHub notifications bot (Jun 11 2024 at 16:27):

alexcrichton submitted PR review.

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?

Wasmtime GitHub notifications bot (Jun 11 2024 at 16:32):

fitzgen submitted PR review.

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.

Wasmtime GitHub notifications bot (Jun 11 2024 at 16:34):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jun 11 2024 at 16:34):

fitzgen created PR review comment:

Just wrapped to 80 characters.

Wasmtime GitHub notifications bot (Jun 11 2024 at 16:35):

fitzgen submitted PR review.

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.

Wasmtime GitHub notifications bot (Jun 11 2024 at 16:54):

fitzgen updated PR #8763.

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.

Wasmtime GitHub notifications bot (Jun 11 2024 at 18:24):

fitzgen updated PR #8763.

Wasmtime GitHub notifications bot (Jun 11 2024 at 18:39):

fitzgen updated PR #8763.

Wasmtime GitHub notifications bot (Jun 11 2024 at 19:18):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 11 2024 at 19:18):

alexcrichton created PR review comment:

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

Wasmtime GitHub notifications bot (Jun 11 2024 at 19:22):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 11 2024 at 19:22):

alexcrichton submitted PR review.

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.

Wasmtime GitHub notifications bot (Jun 11 2024 at 20:14):

fitzgen updated PR #8763.

Wasmtime GitHub notifications bot (Jun 11 2024 at 20:14):

fitzgen has enabled auto merge for PR #8763.

Wasmtime GitHub notifications bot (Jun 12 2024 at 17:28):

fitzgen updated PR #8763.

Wasmtime GitHub notifications bot (Jun 12 2024 at 17:28):

fitzgen has enabled auto merge for PR #8763.

Wasmtime GitHub notifications bot (Jun 12 2024 at 20:45):

fitzgen updated PR #8763.

Wasmtime GitHub notifications bot (Jun 12 2024 at 20:45):

fitzgen has enabled auto merge for PR #8763.

Wasmtime GitHub notifications bot (Jun 12 2024 at 21:03):

fitzgen updated PR #8763.

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

Wasmtime GitHub notifications bot (Jun 12 2024 at 21:05):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 12 2024 at 21:08):

fitzgen submitted PR review.

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.

Wasmtime GitHub notifications bot (Jun 12 2024 at 21:11):

alexcrichton submitted PR review.

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.

Wasmtime GitHub notifications bot (Jun 12 2024 at 21:13):

fitzgen updated PR #8763.

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.

Wasmtime GitHub notifications bot (Jun 12 2024 at 21:32):

fitzgen updated PR #8763.

Wasmtime GitHub notifications bot (Jun 12 2024 at 22:01):

fitzgen merged PR #8763.

Wasmtime GitHub notifications bot (Sep 11 2024 at 09:30):

XinyuZeng submitted PR review.

Wasmtime GitHub notifications bot (Sep 11 2024 at 09:30):

XinyuZeng created PR review comment:

This comment seems to be incomplete

Wasmtime GitHub notifications bot (Sep 13 2024 at 15:40):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Sep 13 2024 at 15:40):

fitzgen created PR review comment:

Thanks for catching this!

Last updated: Oct 23 2024 at 20:03 UTC