Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge with upstream #64

Merged
merged 27 commits into from
Dec 4, 2023
Merged

Merge with upstream #64

merged 27 commits into from
Dec 4, 2023

Conversation

dhil
Copy link
Member

@dhil dhil commented Dec 4, 2023

No description provided.

abrown and others added 26 commits November 28, 2023 15:03
* mpk: allow forcing MPK during tests

For testing on machines on which we know MPK is enabled, we want to be
able to force-enable MPK, ensuring we get coverage of MPK-related code.
This change adds a `WASMTIME_TEST_FORCE_MPK` environment variable which,
when set, sets the pooling allocator configuration to force-enable MPK.
This variable, like the `WASMTIME_TEST_NO_HOG_MEMORY` variable it is
styled from, could be used in CI workflows on which we know MPK should
be available.

* review: only check `WASMTIME_TEST_FORCE_MPK` in tests

Checking the environment variable at runtime is too invasive and could
lead to unexpected behavior. This limits the use of
`WASMTIME_TEST_FORCE_MPK` to the `wast` tests and any tests that use the
`small_pool_config`.
…ce#7590)

* Winch: cleanup stack in br_if in non-fallthrough case

* Remove unnecessary refetch of sp_offsets

* Refactoring based on PR feedback

* Have SPOffset implement Ord
* Change `ms` to `duration`

* Make `future-trailers.get` only return the trailers once

* `duration` is in nanoseconds
…ance#7596)

* Replace the preview2 table's HashMap storage with a Vec

* Forgot to reserve index `2`

* Stop reserving 0, 1, and 2 in the table

* Exercise the free queue

* Remove unnecessary `_` suffix

* Remove push_child_

* Review feedback - switch free node tracking from a queue to a list

* Add `with_capacity` back in

* Fix comments

* Simplify pop_free_list

* Simplify iter_entries
* add clang-format

We chose WebKit style because out of all the builtin styles it seems the
closest to what already exists in wasmtime.

Signed-off-by: Tyler Rockwood <[email protected]>

* c-api: don't reorder headers

The order here matters

Signed-off-by: Tyler Rockwood <[email protected]>

* c-api: apply clang-format

Signed-off-by: Tyler Rockwood <[email protected]>

* fiber: apply clang-format

Signed-off-by: Tyler Rockwood <[email protected]>

* runtime: apply clang-format

Signed-off-by: Tyler Rockwood <[email protected]>

* examples: apply clang format

Signed-off-by: Tyler Rockwood <[email protected]>

* tests: apply clang-format

Signed-off-by: Tyler Rockwood <[email protected]>

* ci: add clang-format checks

Signed-off-by: Tyler Rockwood <[email protected]>

* clang-format: keep braces on the same line

This is more the existing style

Signed-off-by: Tyler Rockwood <[email protected]>

* remove clang-format

Just use the tool defaults (LLVM)

Signed-off-by: Tyler Rockwood <[email protected]>

* Fix ci name

Signed-off-by: Tyler Rockwood <[email protected]>

* manually reformat a couple of comments

prtest:full

Signed-off-by: Tyler Rockwood <[email protected]>

* disable formatting for doc-wasm.h

Signed-off-by: Tyler Rockwood <[email protected]>

* manually reformat wasmtime.h

Signed-off-by: Tyler Rockwood <[email protected]>

* disable formatting

To prevent a link from being broken

Signed-off-by: Tyler Rockwood <[email protected]>

* examples: fixing build commands

Signed-off-by: Tyler Rockwood <[email protected]>

* fix parameter comment

Signed-off-by: Tyler Rockwood <[email protected]>

---------

Signed-off-by: Tyler Rockwood <[email protected]>
This fixes a fuzz-bug found from the last update where GC types were
accidentally leaking through validation.
…codealliance#7570)

* Implement timeout on all blocking utility methods.

* Expand documentation of compatibility fixes
This commit aims to address a discrepancy in Wasmtime where the world
supported by `wasmtime serve` is too large today. This includes
WIT interfaces which are not specified in `wasi:http/proxy` such as
`wasi:filesystem/types`, aka access to a filesystem.

This commit slims down `wasmtime serve` to, by default, only supporting
the `wasi:http/proxy` world. Like with `wasmtime run` various CLI flags
can be passed to enable more interfaces, however:

* `-Scommon` - this enables "common" interfaces such as
  `wasi:filesystem`, `wasi:sockets`, and `wasi:cli/*`.
* `-Snn` - this enables wasi-nn

It's expected that more will get extended here over time too.

This change is enabled by a third build of the adapter, a "proxy" mode.
In this mode most functions are cfg'd to return `ERRNO_NOTSUP` to
indicate that the runtime does not support it. Notably this includes the
filesystem, arguments, and environment variables.

This change is tested by updating all `api_proxy*` tests to use this new
adapter which is now required that many previous interfaces are no
longer present by default in the proxy world.
Follow up to:

bytecodealliance#7547

In which I overlooked this change and the fuzzer found an issue with the
following program:

```wat
(module
  (func (export "") (result i32)
    block (result i32)
       i32.const 0
    end
    i32.const 0
    i32.const 0
    br_table 0
  )
)
```

This commit ensures that the stack pointer is correctly positioned when
emitting br_table.

We can't know for sure which branch will be taken, but since all
branches must share the same type information, we can be certain that
the expectations regarding the stack pointer are the same and thus can
we use the default target in order to ensure the correct placement.
* mpk: optimize layout of protected stripes

While experimenting with the limits of MPK-protected memory pools,
@alexcrichton and I discovered that the current slab layout calculations
were too conservative. This meant that the memory pool could not pack in
as many memories as it should have been able: we were expecting, but not
seeing, ~15x more memory slots over non-MPK memory pools.

The fix ends up being simpler than the original: we must maintain the
codegen constraints that expect a static memory to be inaccessible for
OOB access within a `static_memory_maximum_size +
static_memory_guard_size` region (called `expected_slot_bytes +
guard_bytes` in `memory_pool.rs`). By dividing up that region between
the stripes, we still guarantee that the region is inaccessible by
packing in other MPK-protected stripes. And we still need to make sure
that the `post_slab_guard_bytes` add up to that region. These changes
fix the memory inefficiency issues we were seeing.

Co-authored-by: Alex Crichton <[email protected]>

* mpk: eliminate extra stripe

@alexcrichton pointed out that we know that `slot_bytes /
max_memory_bytes` will at least be 1 due to a `max` comparison above.
Knowing this, we can remove a `+ 1` intended for the case when
`needed_num_stripes == 0`, which should be impossible.

* review: replace `checked_*` with `saturating_*`

This style change is a readability improvement; no calculations should
change.

Co-authored-by: Alex Crichton <[email protected]>

---------

Co-authored-by: Alex Crichton <[email protected]>
…iance#7610)

This commit adds some more information to `wasmtime --version` which
includes the git commit plus the git commit's date. This matches `rustc
-V` for example which was additionally copied to `wasm-tools` and
mirrored as `wasm-tools -V`.

Personally I've found this useful since it can help point to exact
commits and additionally quickly get a sense of how old a version is
based on its commit date presented.
)" (bytecodealliance#7611)

This reverts commit 043e4ce. The
following fails when running locally on an MPK-enabled machine:

```
WASMTIME_TEST_FORCE_MPK=1 cargo test
```
…tecodealliance#7613)

I believe these were omitted by mistake.

TODO: We should definitely use the `wasmtime-wit-bindgen`-generated
`add_to_linker` function for the `command` world if possible, which would avoid
such mistakes in the future.

Signed-off-by: Joel Dice <[email protected]>
…#7617)

This followed bytecodealliance#7613 with a test to ensure that the interface isn't
accidentally left out in the future too.
)

* Cranelift: additional `icmp` & `select` ISLE opts

* Don't include an invalid i8-to-i8 extend in the egraph

Tests covering both widths here (added in previous commit) still pass.
This commit tightens the fuzzing criteria for Winch. The previous
implementation only accounted for unsupported instructions. However,
unsupported types can also cause the fuzzer to crash.

Winch currently doesn't support `v128` and most of the `Ref` types.
* mpk: optimize layout of protected stripes, again

This is another attempt at bytecodealliance#7603, attempting reduce the slab layout
sizes of MPK-protected stripes. While experimenting with the limits of
MPK-protected memory pools, @alexcrichton and I discovered that the
current slab layout calculations were too conservative. This meant that
the memory pool could not pack in as many memories as it should have
been able: we were expecting, but not seeing, ~15x more memory slots
over non-MPK memory pools.

This change brings together several fixes:
- it more aggressively divides up the stripes (as in b212152)
- it eliminates an extra stripe (as in 8813a30)
- it replaces some uses of `checked_*` with `saturating_*` (as in
  fb22a20)
- it improves some documentation
- and, crucially, it reports back a larger value for
  `memory_and_guard_size`

The failures observed with bytecodealliance#7603 when run with MPK
(`WASMTIME_TEST_FORCE_MPK=1 cargo test`) were due to `Store::wasm_fault`
not being able to identify which memory an OOB address belonged to.
This is because the `MemoryPool` was underreporting the size of the
region in which OOB accesses would fault. The correct value is provided
by the new `SlabLayout::bytes_to_next_stripe_slot`: any OOB access
within that larger region must fault because (1) the other stripes have
different protection keys and (2) a `Store` can only use one protection
key. We also use (2) to guarantee that `Store::wasm_fault` will be able
to calculate the Wasm address from the raw address.

This change also provides a new `traps` test that will reproduce the
failures from bytecodealliance#7603; if we observe `SIGABRT` from that test, it will be
a regression.

* fix: make test x86-specific
* mpk: allow checking for MPK without a config instance

It is inconvenient to have to construct a `PoolingAllocationConfig` in
order to check if memory protection keys are available. This removes
the unused `&self` restriction.

* mpk: improve logging of calculated slab layout

When double-checking the slab layout calculations it is quite convenient
to see the total slab size. This helps in correlating with mapped
regions.

* mpk: add an example testing the memory limits

This adds an example that can be run with `cargo run --example mpk`. Not
only does the example demonstrate how to build a pool-allocated engine
that uses MPK, it performs an exponential search to find the maximum
number of slots the system can support, with and without MPK.

* review: document Linux requirement

* review: `env_logger::init`

* review: replace `proc-maps` with manual parsing

* vet: audit `bytesize`

* fix: provide `main` for non-Linux systems

* fix: move `cfg` to avoid unused code
@dhil
Copy link
Member Author

dhil commented Dec 4, 2023

The failure is due to our custom into_iter_err_on_gc_types function in wasm-tools. I am tempted to pull in the upstream version and rename our custom function. I think our custom implementation is only required in one place in wasmtime, so long-term it may be most stable solution. I will think about it some more.

@dhil dhil merged commit 95aa433 into wasmfx:main Dec 4, 2023
@dhil dhil deleted the wasmfx-merge branch December 4, 2023 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.