* [fuzz] Remove some differential fuzz targets
The changes in #4515 do everything the `differential_spec` and
`differential_wasmi` fuzz target already do. These fuzz targets are now
redundant and this PR removes them. It also updates the fuzz
documentation slightly.
* [fuzz] Add `Module` enum, refactor `ModuleConfig`
This change adds a way to create either a single-instruction module or a
regular (big) `wasm-smith` module. It has some slight refactorings in
preparation for the use of this new code.
* [fuzz] Add `DiffValue` for differential evaluation
In order to evaluate functions with randomly-generated values, we needed
a common way to generate these values. Using the Wasmtime `Val` type is
not great because we would like to be able to implement various traits
on the new value type, e.g., to convert `Into` and `From` boxed values
of other engines we differentially fuzz against. This new type,
`DiffValue`, gives us a common ground for all the conversions and
comparisons between the other engine types.
* [fuzz] Add interface for differential engines
In order to randomly choose an engine to fuzz against, we expect all of
the engines to meet a common interface. The traits in this commit allow
us to instantiate a module from its binary form, evaluate exported
functions, and (possibly) hash the exported items of the instance.
This change has some missing pieces, though:
- the `wasm-spec-interpreter` needs some work to be able to create
instances, evaluate a function by name, and expose exported items
- the `v8` engine is not implemented yet due to the complexity of its
Rust lifetimes
* [fuzz] Use `ModuleFeatures` instead of existing configuration
When attempting to use both wasm-smith and single-instruction modules,
there is a mismatch in how we communicate what an engine must be able to
support. In the first case, we could use the `ModuleConfig`, a wrapper
for wasm-smith's `SwarmConfig`, but single-instruction modules do not
have a `SwarmConfig`--the many options simply don't apply. Here, we
instead add `ModuleFeatures` and adapt a `ModuleConfig` to that.
`ModuleFeatures` then becomes the way to communicate what features an
engine must support to evaluate functions in a module.
* [fuzz] Add a new fuzz target using the meta-differential oracle
This change adds the `differential_meta` target to the list of fuzz
targets. I expect that sometime soon this could replace the other
`differential*` targets, as it almost checks all the things those check.
The major missing piece is that currently it only chooses
single-instruction modules instead of also generating arbitrary modules
using `wasm-smith`.
Also, this change adds the concept of an ignorable error: some
differential engines will choke with certain inputs (e.g., `wasmi` might
have an old opcode mapping) which we do not want to flag as fuzz bugs.
Here we wrap those errors in `DiffIgnoreError` and then use a new helper
trait, `DiffIgnorable`, to downcast and inspect the `anyhow` error to
only panic on non-ignorable errors; the ignorable errors are converted
to one of the `arbitrary::Error` variants, which we already ignore.
* [fuzz] Compare `DiffValue` NaNs more leniently
Because arithmetic NaNs can contain arbitrary payload bits, checking
that two differential executions should produce the same result should
relax the comparison of the `F32` and `F64` types (and eventually `V128`
as well... TODO). This change adds several considerations, however, so
that in the future we make the comparison a bit stricter, e.g., re:
canonical NaNs. This change, however, just matches the current logic
used by other fuzz targets.
* review: allow hashing mutate the instance state
@alexcrichton requested that the interface be adapted to accommodate
Wasmtime's API, in which even reading from an instance could trigger
mutation of the store.
* review: refactor where configurations are made compatible
See @alexcrichton's
[suggestion](https://github.com/bytecodealliance/wasmtime/pull/4515#discussion_r928974376).
* review: convert `DiffValueType` using `TryFrom`
See @alexcrichton's
[comment](https://github.com/bytecodealliance/wasmtime/pull/4515#discussion_r928962394).
* review: adapt target implementation to Wasmtime-specific RHS
This change is joint work with @alexcrichton to adapt the structure of
the fuzz target to his comments
[here](https://github.com/bytecodealliance/wasmtime/pull/4515#pullrequestreview-1073247791).
This change:
- removes `ModuleFeatures` and the `Module` enum (for big and small
modules)
- upgrades `SingleInstModule` to filter out cases that are not valid for
a given `ModuleConfig`
- adds `DiffEngine::name()`
- constructs each `DiffEngine` using a `ModuleConfig`, eliminating
`DiffIgnoreError` completely
- prints an execution rate to the `differential_meta` target
Still TODO:
- `get_exported_function_signatures` could be re-written in terms of the
Wasmtime API instead `wasmparser`
- the fuzzer crashes eventually, we think due to the signal handler
interference between OCaml and Wasmtime
- the spec interpreter has several cases that we skip for now but could
be fuzzed with further work
Co-authored-by: Alex Crichton <alex@alexcrichton.com>
* fix: avoid SIGSEGV by explicitly initializing OCaml runtime first
* review: use Wasmtime's API to retrieve exported functions
Co-authored-by: Alex Crichton <alex@alexcrichton.com>
The sse_cmp_op rule had cases that would produce SseOperand values that aren't legal to use with MInst.XmmRmR, and was only used in vector_all_ones when constructing an XmmRmR value. Additionally, vector_all_ones always called sse_cmp_op with the same type, so the other cases were redundant.
The solution in this PR is to remove sse_cmp_op entirely and inline a call to x64_pcmpeqd directly in vector_all_ones, and remove the unused argument from vector_all_ones.
Also, adjust the tests that are executed on that platform. Finally,
fix a bug with obtaining backtraces when back-edge CFI is enabled.
Copyright (c) 2022, Arm Limited.
* Upgrade wasm-tools crates, namely the component model
This commit pulls in the latest versions of all of the `wasm-tools`
family of crates. There were two major changes that happened in
`wasm-tools` in the meantime:
* bytecodealliance/wasm-tools#697 - this commit introduced a new API for
more efficiently reading binary operators from a wasm binary. The old
`Operator`-based reading was left in place, however, and continues to
be what Wasmtime uses. I hope to update Wasmtime in a future PR to use
this new API, but for now the biggest change is...
* bytecodealliance/wasm-tools#703 - this commit was a major update to
the component model AST. This commit almost entirely deals with the
fallout of this change.
The changes made to the component model were:
1. The `unit` type no longer exists. This was generally a simple change
where the `Unit` case in a few different locations were all removed.
2. The `expected` type was renamed to `result`. This similarly was
relatively lightweight and mostly just a renaming on the surface. I
took this opportunity to rename `val::Result` to `val::ResultVal` and
`types::Result` to `types::ResultType` to avoid clashing with the
standard library types. The `Option`-based types were handled with
this as well.
3. The payload type of `variant` and `result` types are now optional.
This affected many locations that calculate flat type
representations, ABI information, etc. The `#[derive(ComponentType)]`
macro now specifically handles Rust-defined `enum` types which have
no payload to the equivalent in the component model.
4. Functions can now return multiple parameters. This changed the
signature of invoking component functions because the return value is
now bound by `ComponentNamedList` (renamed from `ComponentParams`).
This had a large effect in the tests, fuzz test case generation, etc.
5. Function types with 2-or-more parameters/results must uniquely name
all parameters/results. This mostly affected the text format used
throughout the tests.
I haven't added specifically new tests for multi-return but I changed a
number of tests to use it. Additionally I've updated the fuzzers to all
exercise multi-return as well so I think we should get some good
coverage with that.
* Update version numbers
* Use crates.io
When trying to read generated CLIF, it's nice to be able to see at a
glance that some of the operands are defined by `iconst` and similar
instructions, without having to go find each operand's definition
manually.
* Limit the size of functions in the `stacks` fuzzer
The fuzzers recently found a timeout in this fuzz test case related to
the compile time of the generated module. Inspecting the generated
module showed that it had 100k+ opcodes for one function, so this commit
updates the fuzzer to limit the number of operations per-function to a
smaller amount to avoid timeout limits.
* Use `arbitrary_len` for `ops` length
* Fix a max/min flip
All of the `*_imm` instructions are rewritten during legalization to an
explicit `iconst` plus the general form of the operator, so backends
never see them. Therefore these ISLE rules in the x64 backend can never
match anything.
* Fix a compile error on nightly Rust
It looks like Rust nightly has gotten a bit more strict about
attributes-on-expressions and previously accepted code is no longer
accepted. This commit updates the generated code for a macro to a form
which is accepted by rustc.
* Fix a soundness issue with lowering variants
This commit fixes a soundness issue lowering variants in the component
model where host memory could be leaked to the guest module by accident.
In reviewing code recently for `Val::lower` I noticed that the variant
lowering was extending the payload with `ValRaw::u32(0)` to
appropriately fit the size of the variant. In reading this it appeared
incorrect to me due to the fact that it should be `ValRaw::u64(0)` since
up to 64-bits can be read. Additionally this implementation was also
incorrect because the lowered representation of the payload itself was
not possibly zero-extended to 64-bits to accommodate other variants.
It turned out these issues were benign because with the dynamic
surface area to the component model the arguments were all initialized
to 0 anyway. The static version of the API, however, does not initialize
arguments to 0 and I wanted to initially align these two implementations
so I updated the variant implementation of lowering for dynamic values
and removed the zero-ing of arguments.
To test this change I updated the `debug` mode of adapter module
generation to assert that the upper bits of values in wasm are always
zero when the value is casted down (during `stack_get` which only
happens with variants). I then threaded through the `debug` boolean
configuration parameter into the dynamic and static fuzzers.
To my surprise this new assertion tripped even after the fix was
applied. It turns out, though, that there was other leakage of bits
through other means that I was previously unaware of. At the primitive
level lowerings of types like `u32` will have a `Lower` representation
of `ValRaw` and the lowering is simply `dst.write(ValRaw::i32(self))`,
or the equivalent thereof. The problem, that the fuzzers detected, with
this pattern is that the `ValRaw` type is 16-bytes, and
`ValRaw::i32(X)` only initializes the first 4. This meant that all the
lowerings for all primitives were writing up to 12 bytes of garbage from
the host for the wasm module to read.
It turned out that this write of a `ValRaw` was sometimes 16 bytes and
sometimes the appropriate size depending on the number of optimizations
in play. With enough inlining for example `dst.write(ValRaw::i32(self))`
would only write 4 bytes, as expected. In debug mode though without
inlining 16 bytes would be written, including the garbage from the upper
bits.
To solve this issue I ended up taking a somewhat different approach. I
primarily updated the `ValRaw` constructors to simply always extend the
values internally to 64-bits, meaning that the low 8 bytes of a `ValRaw`
is always initialized. This prevents any undefined data from leaking
from the host into a wasm module, and means that values are also
zero-extended even if they're only used in 32-bit contexts outside of a
variant. This felt like the best fix for now, though, in terms of
not really having a performance impact while additionally not requiring
a rewrite of all lowerings.
This solution ended up also neatly removing the "zero out the entire
payload" logic that was previously require. Now after a payload is
lowered only the tail end of the payload, up to the size of the variant,
is zeroed out. This means that each lowered argument is written to at
most once which should hopefully be a small performance boost for
calling into functions as well.
It looks like Rust nightly has gotten a bit more strict about
attributes-on-expressions and previously accepted code is no longer
accepted. This commit updates the generated code for a macro to a form
which is accepted by rustc.
This fixes two problems: minimum symbol alignment for the LARL
instruction, and alignment requirements for LRL/LGRL etc.
The first problem is that the LARL instruction used to load a
symbol address (PC relative) requires that the target symbol
is at least 2-byte aligned. This is always guaranteed for code
symbols (all instructions must be 2-aligned anyway), but not
necessarily for data symbols.
Other s390x compilers fix this problem by ensuring that all
global symbols are always emitted with a minimum 2-byte
alignment. This patch introduces an equivalent mechanism
for cranelift:
- Add a symbol_alignment routine to TargetIsa, similar to the
existing code_section_alignment routine.
- Respect symbol_alignment as minimum alignment for all symbols
emitted in the object backend (code and data).
The second problem is that PC-relative instructions that
directly *access* data (like LRL/LGRL, STRL/STGRL etc.)
not only have the 2-byte requirement like LARL, but actually
require that their memory operand is *naturally* aligned
(i.e. alignment is at least the size of the access).
This property (natural alignment for memory accesses) is
supposed to be provided by the "aligned" flag in MemFlags;
however, this is not implemented correctly at the moment.
To fix this, this patch:
- Only emits PC-relative memory access instructions if the
"aligned" flag is set in the associated MemFlags.
- Fixes a bug in emit_small_memory_copy and emit_small_memset
which currently set the aligned flag unconditionally, ignoring
the actual alignment info passed by their caller.
Tested with wasmtime and cg_clif.
* Add a test for iadd_pairwise with swiden input
* Implement iadd_pairwise for swiden_{low,high} input
* Add a test case for iadd_pairwise with uwiden input
* Implement iadd_pairwise with uwiden
* Optimize flat type representation calculations
Previously calculating the flat type representation would be done
recursively for an entire type tree every time it was visited.
Additionally the flat type representation was entirely built only to be
thrown away if it was too large at the end. This chiefly presented a
source of recursion based on the type structure in the component model
which fuzzing does not like as it reports stack overflows.
This commit overhauls the representation of flat types in Wasmtime by
caching the representation for each type in the compile-time
`ComponentTypesBuilder` structure. This avoids recalculating each time
the flat representation is queried and additionally allows opportunity
to have more short-circuiting to avoid building overly-large vectors.
* Remove duplicate flat count calculation in wasmtime
Roughly share the infrastructure in the `wasmtime-environ` crate, namely
the non-recursive and memoizing nature of the calculation.
* Fix component fuzz build
* Fix example compile
A `GtU` condition needed to actually be `GeU`, as the comment right
above it stated but apparently I forgot to translate the comment to
actual code. This fixes a fuzz bug that arose from oss-fuzz over the
weekend.
* Cranelift: Use bump allocation in `remove_constant_phis` pass
This makes compilation 2-6% faster for Sightglass's bz2 benchmark:
```
compilation :: cycles :: benchmarks/bz2/benchmark.wasm
Δ = 7290648.36 ± 4245152.07 (confidence = 99%)
bump.so is 1.02x to 1.06x faster than main.so!
[166388177 183238542.98 214732518] bump.so
[172836648 190529191.34 217514271] main.so
compilation :: cycles :: benchmarks/pulldown-cmark/benchmark.wasm
No difference in performance.
[182220055 225793551.12 277857575] bump.so
[193212613 227784078.61 277175335] main.so
compilation :: cycles :: benchmarks/spidermonkey/benchmark.wasm
No difference in performance.
[3848442474 4295214144.37 4665127241] bump.so
[3969505457 4262415290.10 4563869974] main.so
```
* Add audit for `bumpalo`
* Add an audit of `arrayvec` version 0.7.2
* Remove unnecessary `collect` into `Vec`
I wasn't able to measure any perf difference here, but its nice to do anyways.
* Use a `SecondaryMap` for keeping track of summaries
* Rename `MmapVec::drain` to `split_off`
As suggested on #4609
* Fix tests
* Make MmapVec::split_off work like Vec::split_off
Co-authored-by: Jamey Sharp <jsharp@fastly.com>
* Cranelift: Remove the `ABICaller` trait
It has only one implementation: the `ABICallerImpl` struct. We can just use that
directly rather than having extra, unnecessary layers of generics and abstractions.
* Cranelift: Rename `ABICallerImpl` to `Caller`
* Cranelift: Remove `ABICallee` trait
It has only one implementation: the `ABICalleeImpl` struct. By using that
directly we can avoid unnecessary layers of generics and abstractions as well as
a couple `Box`es that were previously putting the single implementation into a
`Box<dyn>`.
* Cranelift: Rename `ABICalleeImpl` to `AbiCallee`
* Fix comments as per review
* Rename `AbiCallee` to `Callee`
* Remove recursion building types in `component_api` fuzzer
Sure enough the fuzzers found an input that blows the stack, so the
type-building here was rewritten to use a heap-based stack instead of a
stack-based-stack.
* Review comments
* Add source tarballs to our releases
This commit adds a small script to create a source tarball as part of
the release process. This goes further than requested by #3808 by
vendoring all Rust dependencies as well to be more in line with
"download the source once then build somewhere without a network".
Vendoring the Rust dependencies makes the tarball pretty beefy (67M
compressed, 500M uncompressed). Unfortunately most of this size comes
from vendored crates such as v8, pqcrypto-kyber, winapi, capstone-sys,
plotters, and web-sys. Only `winapi` in this list is actually needed for
`wasmtime`-the-binary and only on Windows as well but for now this is
the state of things related to `cargo vendor`. If this becomes an issue
we could specifically remove the bulky contents of crates in the
`vendor` directory such as `v8` since it's only used for fuzzing.
Closes#3808
* Review feedback
* Review comments
This is the implementation of https://github.com/bytecodealliance/wasmtime/issues/4155, using the "inverted API" approach suggested by @cfallin (thanks!) in Cranelift, and trait object to provide a backend for an all-included experience in Wasmtime.
After the suggestion of Chris, `Function` has been split into mostly two parts:
- on the one hand, `FunctionStencil` contains all the fields required during compilation, and that act as a compilation cache key: if two function stencils are the same, then the result of their compilation (`CompiledCodeBase<Stencil>`) will be the same. This makes caching trivial, as the only thing to cache is the `FunctionStencil`.
- on the other hand, `FunctionParameters` contain the... function parameters that are required to finalize the result of compilation into a `CompiledCode` (aka `CompiledCodeBase<Final>`) with proper final relocations etc., by applying fixups and so on.
Most changes are here to accomodate those requirements, in particular that `FunctionStencil` should be `Hash`able to be used as a key in the cache:
- most source locations are now relative to a base source location in the function, and as such they're encoded as `RelSourceLoc` in the `FunctionStencil`. This required changes so that there's no need to explicitly mark a `SourceLoc` as the base source location, it's automatically detected instead the first time a non-default `SourceLoc` is set.
- user-defined external names in the `FunctionStencil` (aka before this patch `ExternalName::User { namespace, index }`) are now references into an external table of `UserExternalNameRef -> UserExternalName`, present in the `FunctionParameters`, and must be explicitly declared using `Function::declare_imported_user_function`.
- some refactorings have been made for function names:
- `ExternalName` was used as the type for a `Function`'s name; while it thus allowed `ExternalName::Libcall` in this place, this would have been quite confusing to use it there. Instead, a new enum `UserFuncName` is introduced for this name, that's either a user-defined function name (the above `UserExternalName`) or a test case name.
- The future of `ExternalName` is likely to become a full reference into the `FunctionParameters`'s mapping, instead of being "either a handle for user-defined external names, or the thing itself for other variants". I'm running out of time to do this, and this is not trivial as it implies touching ISLE which I'm less familiar with.
The cache computes a sha256 hash of the `FunctionStencil`, and uses this as the cache key. No equality check (using `PartialEq`) is performed in addition to the hash being the same, as we hope that this is sufficient data to avoid collisions.
A basic fuzz target has been introduced that tries to do the bare minimum:
- check that a function successfully compiled and cached will be also successfully reloaded from the cache, and returns the exact same function.
- check that a trivial modification in the external mapping of `UserExternalNameRef -> UserExternalName` hits the cache, and that other modifications don't hit the cache.
- This last check is less efficient and less likely to happen, so probably should be rethought a bit.
Thanks to both @alexcrichton and @cfallin for your very useful feedback on Zulip.
Some numbers show that for a large wasm module we're using internally, this is a 20% compile-time speedup, because so many `FunctionStencil`s are the same, even within a single module. For a group of modules that have a lot of code in common, we get hit rates up to 70% when they're used together. When a single function changes in a wasm module, every other function is reloaded; that's still slower than I expect (between 10% and 50% of the overall compile time), so there's likely room for improvement.
Fixes#4155.
* cranelift: Remove shifts-small-types runtests
These were moved to the main shifts file in #4519 but this file was accidentaly left in tree.
It also fixes the missing sshr_i8_i8 testcase
* cranelift: Add shifts to fuzzer
* cranelift: Add extends to fuzzer
The trait had only one implementation: the `Lower` struct. It is easier to just
use that directly, and not introduce unnecessary layers of generics and
abstractions.
Once upon a time, there was hope that we would have other implementations of the
`LowerCtx` trait, that did things like lower CLIF to SMTLIB for
verification. However, this is not practical these days given the way that the
trait has evolved over time, and our verification efforts are focused on ISLE
now anyways, and we're actually making some progress on that front (much more
than anyone ever did on a second `LowerCtx` trait implementation!)
In #4375 we introduced a code pattern that appears as a warning when
building the `cranelift-interpreter` crate:
```
warning: cannot borrow `*state` as mutable because it is also borrowed as immutable
--> cranelift/interpreter/src/step.rs:412:13
|
47 | let arg = |index: usize| -> Result<V, StepError> {
| -------------------------------------- immutable borrow occurs here
48 | let value_ref = inst_context.args()[index];
49 | state
| ----- first borrow occurs due to use of `*state` in closure
...
412 | state.set_pinned_reg(arg(0)?);
| ^^^^^^^^^^^^^^^^^^^^^---^^^^^
| | |
| | immutable borrow later used here
| mutable borrow occurs here
|
= note: `#[warn(mutable_borrow_reservation_conflict)]` on by default
= warning: this borrowing pattern was not meant to be accepted, and may become a hard error in the future
= note: for more information, see issue #59159 <https://github.com/rust-lang/rust/issues/59159>
```
This change fixes the warning.
* Add a test for the existing behavior of fcvt_from_unit
* Migrate the I8, I16, I32 cases of fcvt_from_uint
* Implement the I64 case of fcvt_from_uint
* Add a test for the existing behavior of fcvt_from_uint.f64x2
* Migrate fcvt_from_uint.f64x2 to ISLE
* Lower the last case of `fcvt_from_uint`
* Add a test for `fcvt_from_uint`
* Finish lowering fcmp_from_uint
* Format
This implements the s390x back-end portion of the solution for
https://github.com/bytecodealliance/wasmtime/issues/4566
We now support both big- and little-endian vector lane order
in code generation. The order used for a function is determined
by the function's ABI: if it uses a Wasmtime ABI, it will use
little-endian lane order, and big-endian lane order otherwise.
(This ensures that all raw_bitcast instructions generated by
both wasmtime and other cranelift frontends can always be
implemented as a no-op.)
Lane order affects the implementation of a number of operations:
- Vector immediates
- Vector memory load / store (in big- and little-endian variants)
- Operations explicitly using lane numbers
(insertlane, extractlane, shuffle, swizzle)
- Operations implicitly using lane numbers
(iadd_pairwise, narrow/widen, promote/demote, fcvt_low, vhigh_bits)
In addition, when calling a function using a different lane order,
we need to lane-swap all vector values passed or returned in registers.
A small number of changes to common code were also needed:
- Ensure we always select a Wasmtime calling convention on s390x
in crates/cranelift (func_signature).
- Fix vector immediates for filetests/runtests. In PR #4427,
I attempted to fix this by byte-swapping the V128 value, but
with the new scheme, we'd instead need to perform a per-lane
byte swap. Since we do not know the actual type in write_to_slice
and read_from_slice, this isn't easily possible.
Revert this part of PR #4427 again, and instead just mark the
memory buffer as little-endian when emitting the trampoline;
the back-end will then emit correct code to load the constant.
- Change a runtest in simd-bitselect-to-vselect.clif to no longer
make little-endian lane order assumptions.
- Remove runtests in simd-swizzle.clif that make little-endian
lane order assumptions by relying on implicit type conversion
when using a non-i16x8 swizzle result type (this feature should
probably be removed anyway).
Tested with both wasmtime and cg_clif.
This commit fixes a build warning on Rust 1.63 when the `memory-init-cow`
feature is disabled in the `wasmtime-runtime` crate. Some "tricks" were
used prior to have the `MemoryImage` type be an empty `enum {}` but that
wreaks havoc with warnings so this commit instead just makes it a unit
struct and makes all methods panic (as they shouldn't be hit anyway).
During differential execution against V8, Wasm values need to be
converted back and forth from JS values. This change documents the
location in the specification where this is defined.
* Limit the type hierarchies in component fuzzing
For now `wasmparser` has a hard limit on the size of tuples and such at
1000 recursive types within the tuple itself. Respect this limit by
limiting the width of recursive types generated for the `component_api`
fuzzer. This commit unifies this new requirement with the preexisting
`TupleArray` and `NonEmptyArray` types into one `VecInRange<T, L, H>`
which allow expressing all of these various requirements in one type.
* Fix a compile error on `main`
* Review comments
