Implement 0-len/drop spec changes in bulk memory #2529
Conversation
This implements recent bulk memory spec changes (WebAssembly/bulk-memory-operations#126) in Binaryen. Now `data.drop` is equivalent to shrinking a segment size to 0, and dropping already dropped segments or active segments (which are thought to be dropped in the beginning) is treated as a no-op. And all bounds checking is performed in advance, so partial copying/filling/initializing does not occur. I tried to implement `visitDataDrop` in the interpreter as `segment.data.clear();`, which is exactly what the revised spec says. I didn't end up doing that because this also deletes all contents from active segments, and there are cases we shouldn't do that: - `wasm-ctor-eval` shouldn't delete active segments, because it will store the changed contents back into segments - When `--fuzz-exec` is given to `wasm-opt`, it runs the module and compare the execution call results before and after transformations. But if running a module will nullify all active segments, applying any transformation to the module or re-running it does not make any sense. Fixes WebAssembly#2528.
| ) | ||
| (func $bar | ||
| (drop | ||
| (loop (result i32) | ||
| (data.drop 0) |
There was a problem hiding this comment.
I'm not sure this function tests anything useful without the data.drop. Should we just leave the data.drops in these tests?
There was a problem hiding this comment.
After memory packing optimization, this module does not contain any segments left, so this becomes a validation error. The same for the one more removed (data.drop 0) above. Before, it was OK because we converted these data.drops to unreachables, because they tried to drop already dropped segments. But dropping the same segments repeatedly is not a trap anymore, so we shouldn't do that. (So segment 0 existed in the beginning and dropped because it was an active segment, but after transformation it does not exist anymore).
There was a problem hiding this comment.
Oh, come to think of it, then it'd be better to just delete this function, now that it does not test anything meaningful... I already merged this :( But I'll post a follow-up patch.
There was a problem hiding this comment.
Wait, are you saying that the memory packing removes the segment but keeps the data.drop around, leading to a validation error? That sounds like a bug in the memory packing pass. It should convert the data.drop to a nop or remove it instead.
There was a problem hiding this comment.
Hmm, yeah, that sounds like a new bug introduced... Is it worth fixing given that you are gonna completely overhaul the pass soon?
There was a problem hiding this comment.
Filed a bug report anyway: #2535
Oh no it was not blocking me. I fixed this because I just thought it was a simple thing to do, and this isn't your bug either :) |
- Remove a function from memory-packing_all-features.wast, because it does not test anything meaningful after WebAssembly#2529. - Rename a test file to use `--all-features`; it started failing I guess because `exnref` requires also reference type features, but not sure why it was OK so far.
- Remove a function from memory-packing_all-features.wast, because it does not test anything meaningful after #2529. - Rename a test file to use `--all-features`; it started failing I guess because `exnref` requires also reference type features, but not sure why it was OK so far.
This does two things: - Restore `visitDataDrop` handler deleted in WebAssembly#2529, but now we convert invalid `data.drop`s to not `unreachable` but `nop`. This conforms to the revised spec that `data.drop` on the active segment can be treated as a nop. - Now `visitMemoryInit` does the same thing; its handling was missing in WebAssembly#2529. It drops all its arguments. Fixes WebAssembly#2535.
This does two things: - Restore `visitDataDrop` handler deleted in WebAssembly#2529, but now we convert invalid `data.drop`s to not `unreachable` but `nop`. This conforms to the revised spec that `data.drop` on the active segment can be treated as a nop. - Now `visitMemoryInit` does the same thing; its handling was missing in WebAssembly#2529. It drops all its arguments. Fixes WebAssembly#2535.
This does two things: - Restore `visitDataDrop` handler deleted in #2529, but now we convert invalid `data.drop`s to not `unreachable` but `nop`. This conforms to the revised spec that `data.drop` on the active segment can be treated as a nop. - Make `visitMemoryInit` trap if offset or size are not equal to 0 or if the dest address is out of bounds. Otherwise drop all its argument. Fixes #2535.
This implements recent bulk memory spec changes
(WebAssembly/bulk-memory-operations#126) in Binaryen. Now
data.dropisequivalent to shrinking a segment size to 0, and dropping already
dropped segments or active segments (which are thought to be dropped in
the beginning) is treated as a no-op. And all bounds checking is
performed in advance, so partial copying/filling/initializing does not
occur.
I tried to implement
visitDataDropin the interpreter assegment.data.clear();, which is exactly what the revised spec says. Ididn't end up doing that because this also deletes all contents from
active segments, and there are cases we shouldn't do that:
wasm-ctor-evalshouldn't delete active segments, because it willstore the changed contents back into segments
--fuzz-execis given towasm-opt, it runs the module andcompare the execution call results before and after transformations.
But if running a module will nullify all active segments, applying
any transformation to the module or re-running it does not make any
sense.
Fixes #2528.