Fix ExpressionRunner issues found by the fuzzer#2790
Conversation
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
This sounds good to me, thanks! The interpreter isn't meant to be super-fast anyhow, optimizing for moves and such is not really the point.
Can we add a testcase in test/passes/?
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! |
| ) | ||
| (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? :)
| @@ -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.
| @@ -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 ! |
4 participants
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.