Fix ExpressionRunner issues found by the fuzzer #2790
Conversation
I can try to reduce the original Edit: Can't be reduced more using |
|
That seems fine to me. I would just add a comment to the test pointing back to the issue to give some context. |
|
Guess who broke something again. This time: CI |
|
Guess who fixed something again! |
test/passes/issue2788.txt
Outdated
| ) | ||
| (func $2 | ||
| (nop) | ||
| ) |
There was a problem hiding this comment.
There are an awful lot of junk functions in here that are nothing but nops or unreachable. My guess is that the problem will still reproduce if we remove them, and that would make the test much clearer. Even then, I'd almost rather not have a test than check in this beast 😬 @kripken wdyt?
There was a problem hiding this comment.
Let me try to reduce it by hand :)
There was a problem hiding this comment.
Managed to isolate the problematic expression tree. Also tried to reduce that tree, but the first two attempts made the error go away, so I guess it's best to preserve its full beauty? :)
test/passes/issue2788.wast
Outdated
| @@ -0,0 +1,171 @@ | |||
| ;; Regression test for 'std::move'-related issues in ExpressionRunner during precompute-propagate | |||
There was a problem hiding this comment.
Can we possibly reduce this test case? Do we need all this code to reproduce this bug?
There was a problem hiding this comment.
Attempts to far didn't work so well (see also #2790 (review)), but I can try a little more if you'd like. My current guess is that this depends on the implementation of std::unordered_map a bit, i.e. how and when it shuffles its buffers around, and reducing it scares off the bug.
There was a problem hiding this comment.
Interesting. I think in that case I'd prefer to not check in the testcase, if it's not going to deterministically hit the bug anyhow.
I think we're ok without a testcase since the fuzzer found it easily multiple times for me.
test/passes/issue2788.wast
Outdated
| @@ -0,0 +1,171 @@ | |||
| ;; Regression test for 'std::move'-related issues in ExpressionRunner during precompute-propagate | |||
There was a problem hiding this comment.
Interesting. I think in that case I'd prefer to not check in the testcase, if it's not going to deterministically hit the bug anyhow.
I think we're ok without a testcase since the fuzzer found it easily multiple times for me.
|
Great, thanks @dcodeIO ! |
Fixes #2788 found by the fuzzer, introduced in #2702, which turned out to be incorrect usage of
std::move, by removing anystd::moves introduced in that PR to be better safe than sorry. Also fixes problems with WASM_INTERPRETER_DEBUG spotted during debugging.