[wasm-reduce] Do not crash on non-func element segments #6778
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kripken
reviewed
Jul 22, 2024
src/tools/wasm-reduce.cpp
Outdated
| } | ||
| // Replace the element if it is different from our first "zero" | ||
| // element. | ||
| return !ExpressionAnalyzer::equal(first, elem); |
Member
There was a problem hiding this comment.
This looks flipped? Before we returned f->func == e->func which meant we return true when they were equal.
Generalize the code for simplifying element segments to handle more than just null and funcref elements.
tlively
force-pushed
the
fix-reduce-segment-cast
branch
from
2c055b3 to
2969e98
Compare
kripken
reviewed
Jul 23, 2024
test/reduce/memory_table.wast
Outdated
| (table 481 481 funcref) | ||
| (table 354 354 i31ref) | ||
| (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)))) |
Member
There was a problem hiding this comment.
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.
Suggested change
| (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)))) |
kripken
reviewed
Jul 23, 2024
| (func $f5 (result i32) | ||
| (i32.add | ||
| (i31.get_s (table.get 1 (i32.const 0))) | ||
| (i31.get_u (table.get 1 (i32.const 1))) |
Member
There was a problem hiding this comment.
With the suggestion above I think one of these two operations will get reduced but not the other.
kripken
reviewed
Jul 23, 2024
test/reduce/memory_table.wast
Outdated
| (table 481 481 funcref) | ||
| (table 354 354 i31ref) | ||
| (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)))) |
Member
There was a problem hiding this comment.
Hmm, now it isn't removing anything here... maybe add a final item with some value (doesn't matter what)?
kripken
approved these changes
Jul 23, 2024
Member
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
Sorry for the iteration here, but now I think this fully tests the change, nice!
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Generalize the code for simplifying element segments to handle more than
just null and funcref elements.