馃 Non-trapping mode for wasm-smith #659
Conversation
|
putting this up for some initial review as I work on the todos |
There was a problem hiding this comment.
This is looking awesome, thanks for this!
One thing I might recommend adding, could a fuzzer (or a runner like crates/fuzz-stats) be added to test out that there are no traps? I presume that at this moment in time it'd quickly find the todo! panics but it might be useful to have something in-tree which tests the no-trap mode and asserts that traps in fact do not come out.
crates/wasm-smith/src/notrap.rs
Outdated
| // there is an `unreachable` on the stack. This would allow | ||
| // us to keep executing the rest of the code following the | ||
| // `unreachable`, but also is a ton more work, and it isn't | ||
| // clear that it would pay for itself. |
There was a problem hiding this comment.
Personally I agree with this comment, and the more prudent thing would probably be to simply disable generation of the unreachable instruction in the first place if we really want to lean into it.
There was a problem hiding this comment.
I think this is a question we can kick down the road to a different PR. Personally I don't think we should completely 100% disable unreachable, so I think this pass would have to handle it somehow anyways.
crates/wasm-smith/src/notrap.rs
Outdated
| // When we can, avoid emitting `drop`s to consume the | ||
| // arguments when possible, since dead code isn't | ||
| // usually an interesting thing to give to a Wasm | ||
| // compiler. Instead, prefer writing them to the first | ||
| // page of the first memory if possible. |
There was a problem hiding this comment.
This definitely makes sense to me, but this may not be the best place to put this perhaps? For example during general code generation we already generate a lot of drop instructions to fixup block/function types. Given the discussion on #655, I wonder if it might be a good idea to (eventually) replace this with drop and then have a later "pass", similar to this one, where it replaces all drop instructions with stores to memory or fold-the-value-into-a-global. That way this could be simpler by "just drop" and then all drop instructions could be uniformly handled no matter where they're generated.
itsrainy
force-pushed
the
wasm-smith-no-traps-mode
branch
from
197a2fb
to
d21a79d
Compare
Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
itsrainy
force-pushed
the
wasm-smith-no-traps-mode
branch
from
a73570d
to
7da2519
Compare
itsrainy
force-pushed
the
wasm-smith-no-traps-mode
branch
from
7da2519
to
5116066
Compare
itsrainy
force-pushed
the
wasm-smith-no-traps-mode
branch
from
878c55c
to
d320f3a
Compare
itsrainy
changed the title
I think this is also something I will push to a later PR if that's cool (likely once the simd and table instructions are handled, since that's the point when such a fuzzer will be informative). |
Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
* Introduce non-trapping mode for wasm-smith * Ensure integer division and remainder operations don't trap * Ensure `store` operations don't trap * Avoid trapping on float -> int conversion for NaN, and Infinities * Move notrap.rs under wasm-smith core * Fixed some bugs with the wasm we were emitting * Fixed the last few bugs with call_indirect rewrites Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com> * Clean up unused variables for now * cargo fmt * revert validator changes and remove unused import * fixup comments Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com> Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
Partially addresses #266. This inserts instructions around or in place of instructions that have the potential to trap.
Completed:
unreachables with dummy-valuereturnscall_indirectcallsNaN,Infinity, and out of range values (ref)Not completed, but punting to a later PR:
memory.{init,copy,fill}(ref)