Repository navigation
Conversation
AlignmentLowering and I64ToI32Lowering split a store into smaller stores and emitted them from the lowest address up. When the original store is partly out of bounds the low pieces were written before the high piece trapped, while the unlowered store writes nothing. Emit the highest piece first so a trap happens before any memory is changed. In I64ToI32Lowering the low word value has to be evaluated before the high store, since evaluating it is what sets the high bits. The pass requires flat IR, so the low word is always a constant or a local.get and can stay in the low store. Fixes WebAssembly#9186
There was a problem hiding this comment.
Code Review
This pull request modifies the AlignmentLowering and I64ToI32Lowering passes to emit lowered store pieces from the highest offset to the lowest. This ensures that if a store is out of bounds, it traps before any memory is modified, preserving the correct transactional behavior of trapping stores. The changes are accompanied by a new test suite verifying this trap behavior, along with updated expectations for existing tests. I have no feedback to provide as there are no review comments.
This branch has not been deployed
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
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.
Fixes #9186
Root cause
AlignmentLowering splits an unaligned store into byte or halfword stores, and I64ToI32Lowering splits an i64 store into two i32 stores. Both emitted the pieces from the lowest address up. A wasm store that goes out of bounds writes nothing, but after lowering the low pieces were written before the high piece trapped, so memory ended up partly modified.
Fix
Emit the pieces from the highest address down. If the highest piece is in bounds then so are the lower ones, so a trap now happens before anything is written. In I64ToI32Lowering the low word has to be evaluated before the high store, because evaluating it is what sets the high bits local. The pass requires flat IR, so the low word always ends up as a constant or a local.get, which can stay in the low store. The generated expectations for the existing alignment lowering, i64 lowering and wasm2js tests are updated to the new order.
Testing
Windows 11 with MSYS2 ucrt64 GCC 16.2, Release.
--alignment-loweringand--flatten --i64-to-i32-loweringwith--fuzz-exec, then reads back the last word of memory. Every stored value has a different nonzero byte at each address, so any partial write shows up. It also does an in bounds unaligned i64 store and checks both halves. The checks come from update_lit_checks.py --output=fuzz-exec, with separate ALIGN and I64 prefixes because the trap messages differ between the two runs.