Wasmtime: Implement the custom-page-sizes proposal by fitzgen · Pull Request #8763 · bytecodealliance/wasmtime

fitzgen · 2024-06-10T20:15:41Z

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.

These were written while implementing the proposal in Wasmtime: bytecodealliance/wasmtime#8763

alexcrichton

This looks great to me, thanks!

github-actions · 2024-06-10T23:44:44Z

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 .github/subscribe-to-label.json configuration file.

Learn more.

github-actions · 2024-06-10T23:45:14Z

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:

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.

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 (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 for more 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.

Details

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

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

Learn more.

fitzgen · 2024-06-11T16:55:01Z

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

alexcrichton · 2024-06-11T19:20:21Z

    /// [`PoolingAllocationConfig::linear_memory_keep_resident`] except that it
    /// is applicable to tables instead.
    pub fn table_keep_resident(&mut self, size: usize) -> &mut Self {
-        let size = round_up_to_pages(size as u64) as usize;


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.

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.

Propagate errors instead of returning a value that is not actually a rounded up version of the input. Delay rounding up various config sizes until runtime instead of eagerly doing it at config time (which isn't even guaranteed to work, so we already had to have a backup plan to round up at runtime, since we might be cross-compiling wasm or not have the runtime feature enabled).

…onfig methods

…sive_64_bit_still_limited` test The point of the test is to ensure that we hit the limiter, so just cancel the allocation from the limiter, and otherwise avoid MIRI attempting to allocate a bunch of memory after we hit the limiter.

…the `massive_64_bit_still_limited` test" This reverts commit ccfa34a.

It seems that rather than returning a null pointer from `std::alloc::alloc`, miri will sometimes choose to simply crash the whole program.

XinyuZeng · 2024-09-11T09:30:37Z

-/// A linear memory
+/// A linear memory's backing storage.
+///
+/// This does not a full Wasm linear memory, as it may


This comment seems to be incomplete

Thanks for catching this!

fitzgen requested review from a team as code owners June 10, 2024 20:15

fitzgen requested review from alexcrichton and elliottt and removed request for a team June 10, 2024 20:15

fitzgen added a commit to WebAssembly/custom-page-sizes that referenced this pull request Jun 10, 2024

Add spec tests for the custom-page-sizes proposal

60e832b

These were written while implementing the proposal in Wasmtime: bytecodealliance/wasmtime#8763

fitzgen mentioned this pull request Jun 10, 2024

Add spec tests for the custom-page-sizes proposal WebAssembly/custom-page-sizes#23

Merged

fitzgen force-pushed the custom-page-sizes branch 4 times, most recently from 69cfb13 to 8f3bd6b Compare June 10, 2024 22:02

alexcrichton reviewed Jun 10, 2024

View reviewed changes

elliottt reviewed Jun 11, 2024

View reviewed changes

Comment thread crates/wasmtime/src/runtime/vm/vmcontext.rs

Comment thread crates/wasmtime/src/runtime/vm.rs Outdated

alexcrichton approved these changes Jun 11, 2024

View reviewed changes

alexcrichton added this pull request to the merge queue Jun 11, 2024

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 11, 2024

fitzgen enabled auto-merge June 11, 2024 20:14

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 12, 2024

fitzgen enabled auto-merge June 12, 2024 20:45

alexcrichton reviewed Jun 12, 2024

View reviewed changes

Comment thread tests/all/memory.rs Outdated

This comment was marked as resolved.

Sign in to view

fitzgen added 19 commits June 12, 2024 14:30

Update error string expectation

c1e57a9

Remove debug logging

1657a78

Use a right shift instead of a division

25737d9

fix error message expectation again

a48835d

remove page size from VMMemoryDefinition

3b3d883

fix size of VMMemoryDefinition again

6506f27

Only dynamically check for -1 sentinel for 1-byte page sizes

f3c5a01

Import functions that are used a few times

ff8630b

Fix some anyhow and nostd errors

83a5d78

Add missing rounding up to host page size at runtime

621f29d

Add validate feature to wasmparser dep

c9de801

Add some new rounding in a few places, due to no longer rounding in c…

30ed095

…onfig methods

prtest:full

6fe8f3e

Revert "Avoid actually trying to allocate the whole address space in …

67c3506

…the `massive_64_bit_still_limited` test" This reverts commit ccfa34a.

miri: don't attempt to allocate more than 4GiB of memory

68ad4ca

It seems that rather than returning a null pointer from `std::alloc::alloc`, miri will sometimes choose to simply crash the whole program.

remove duplicate prelude import after rebasing

3732ec8

fitzgen force-pushed the custom-page-sizes branch from 0efb7f4 to 3732ec8 Compare June 12, 2024 21:32

fitzgen added this pull request to the merge queue Jun 12, 2024

Merged via the queue into bytecodealliance:main with commit bdd7842 Jun 12, 2024

fitzgen deleted the custom-page-sizes branch June 12, 2024 22:01

fitzgen mentioned this pull request Jun 13, 2024

Discussion of alternative: byte-granularity deterministic cap WebAssembly/custom-page-sizes#13

Closed

XinyuZeng reviewed Sep 11, 2024

View reviewed changes

Conversation

fitzgen commented Jun 10, 2024

Uh oh!

alexcrichton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jun 10, 2024

Subscribe to Label Action

Uh oh!

github-actions Bot commented Jun 10, 2024 • edited by fitzgen Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Label Messager: wasmtime:config

Uh oh!

Uh oh!

Uh oh!

fitzgen commented Jun 11, 2024

Uh oh!

alexcrichton Jun 11, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

XinyuZeng Sep 11, 2024

Choose a reason for hiding this comment

Uh oh!

fitzgen Sep 13, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions Bot commented Jun 10, 2024 •

edited by fitzgen

Loading