This repository has been archived by the owner on Nov 3, 2021. It is now read-only.
[interpreter] Simplify zero-len and drop semantics #126
Conversation
This file contains 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
|
Punting to @eqrion for work on tests :-) |
|
I wrote a commit based on your branch to update the test-generators. I can confirm that it resolved all of the test failures. Feel free to take it and use in this PR, I'm unsure of the best way on GH to handle this situation. |
|
@eqrion, thanks! Can't you simply create a PR against this branch? |
rossberg
changed the title
|
With @eqrion's test generator tweaks, this PR should be complete and ready for review now. |
|
@eqrion, GH doesn't let me request you as a reviewer, but can you please take a look? |
Ah, this is a branch in this repo not a personal one. I suppose it would work either way, though. I just opened the PR.
Yup, I'll take a look at this today. |
aheejin
added a commit
to aheejin/bulk-memory-operations
that referenced
this pull request
Dec 15, 2019
This reflects spec changes in WebAssembly#126 in Overview.md.
aheejin
added a commit
to aheejin/binaryen
that referenced
this pull request
Dec 15, 2019
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.
aheejin
added a commit
to aheejin/binaryen
that referenced
this pull request
Dec 15, 2019
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.
aheejin
added a commit
to aheejin/binaryen
that referenced
this pull request
Dec 15, 2019
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.
aheejin
added a commit
that referenced
this pull request
Dec 16, 2019
After the spec change in #126, byte copying order is not observable.
aheejin
added a commit
to WebAssembly/binaryen
that referenced
this pull request
Dec 17, 2019
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.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
As discussed on #124, this introduces two simplifications, partially as a consequence of #111/#123:
Zero-length out-of-bound access trap again.
This reverts Update overview for the zero-byte OOB case #100, [spec] Update text for zero-byte OOB case #101, [interp] Don't trap on zero-bytes out-of-bounds #102, which turn out to no longer be simplifications but rather complications in the presence of the OOB semantics more recently introduced by memory.copy|fill semantics limit optimizations for short constant lengths #111/[exec] Bounds check bulk-memory before execution #123.
Segment drops are treated like a shrink-to-zero.
This unifies drop check and OOB into a single check and avoids maintaining dropped as an extra state bit in the implementation. The only observable change in behaviour is that it is no longer a trap to (a) drop a segment twice, or (b) perform a 0-length init from a dropped segment at offset 0; both these cases are nops now.
Taken together, these changes also address #124.
Includes:
The spec of OOB checks follows in PR #129, since resp changes for #123 were still missing.