-
Notifications
You must be signed in to change notification settings - Fork 729
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
[wasm-reduce] Do not crash on non-func element segments #6778
Conversation
src/tools/wasm-reduce.cpp
Outdated
} | ||
// Replace the element if it is different from our first "zero" | ||
// element. | ||
return !ExpressionAnalyzer::equal(first, elem); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks flipped? Before we returned f->func == e->func
which meant we return true when they were equal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
Generalize the code for simplifying element segments to handle more than just null and funcref elements.
2c055b3
to
2969e98
Compare
test/reduce/memory_table.wast
Outdated
(elem (i32.const 0) $f0 $f0 $f1 $f2 $f0 $f3 $f0) | ||
(elem (table 1) (i32.const 0) i31ref (item (ref.i31 (i32.const 0))) (item (ref.i31 (i32.const 1)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you make one of these values different than 0 and 1 then I think it will not be removed, which would show that we keep things that are needed.
(elem (table 1) (i32.const 0) i31ref (item (ref.i31 (i32.const 0))) (item (ref.i31 (i32.const 1)))) | |
(elem (table 1) (i32.const 0) i31ref (item (ref.i31 (i32.const 0))) (item (ref.i31 (i32.const 42)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks!
(func $f5 (result i32) | ||
(i32.add | ||
(i31.get_s (table.get 1 (i32.const 0))) | ||
(i31.get_u (table.get 1 (i32.const 1))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the suggestion above I think one of these two operations will get reduced but not the other.
test/reduce/memory_table.wast
Outdated
(elem (i32.const 0) $f0 $f0 $f1 $f2 $f0 $f3 $f0) | ||
(elem (table 1) (i32.const 0) i31ref (item (ref.i31 (i32.const 0))) (item (ref.i31 (i32.const 42)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, now it isn't removing anything here... maybe add a final item with some value (doesn't matter what)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the iteration here, but now I think this fully tests the change, nice!
Generalize the code for simplifying element segments to handle more than
just null and funcref elements.