-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Implement the memory64 proposal in Wasmtime #3153
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
Merged
Merged
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
c903381
Implement the memory64 proposal in Wasmtime
alexcrichton cece496
Fix some tests
alexcrichton dcbd2b0
More test fixes
alexcrichton 6c1d860
Fix wasmtime tests
alexcrichton 8d0dbd9
Fix doctests
alexcrichton ff3f994
Revert to 32-bit immediate offsets in `heap_addr`
alexcrichton 6be0d97
Disable memory64 for spectest fuzzing
alexcrichton 84e835e
Fix wrong offset being added to heap addr
alexcrichton 8bd2ea6
More comments!
alexcrichton 3ab09ab
Clarify bytes/pages
alexcrichton File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2164,10 +2164,6 @@ fn prepare_addr<FE: FuncEnvironment + ?Sized>( | |
| environ: &mut FE, | ||
| ) -> WasmResult<(MemFlags, Value, Offset32)> { | ||
| let addr = state.pop1(); | ||
| // This function will need updates for 64-bit memories | ||
| debug_assert_eq!(builder.func.dfg.value_type(addr), I32); | ||
| let offset = u32::try_from(memarg.offset).unwrap(); | ||
|
|
||
| let heap = state.get_heap(builder.func, memarg.memory, environ)?; | ||
| let offset_guard_size: u64 = builder.func.heaps[heap].offset_guard_size.into(); | ||
|
|
||
|
|
@@ -2222,25 +2218,65 @@ fn prepare_addr<FE: FuncEnvironment + ?Sized>( | |
| // offsets we're checking here are zero. This means that we'll hit the fast | ||
| // path and emit zero conditional traps for bounds checks | ||
| let adjusted_offset = if offset_guard_size == 0 { | ||
| u64::from(offset) + u64::from(access_size) | ||
| memarg.offset.saturating_add(u64::from(access_size)) | ||
| } else { | ||
| assert!(access_size < 1024); | ||
| cmp::max(u64::from(offset) / offset_guard_size * offset_guard_size, 1) | ||
| cmp::max(memarg.offset / offset_guard_size * offset_guard_size, 1) | ||
| }; | ||
| debug_assert!(adjusted_offset > 0); // want to bounds check at least 1 byte | ||
| let check_size = u32::try_from(adjusted_offset).unwrap_or(u32::MAX); | ||
| let base = builder | ||
| .ins() | ||
| .heap_addr(environ.pointer_type(), heap, addr, check_size); | ||
|
|
||
| // Native load/store instructions take a signed `Offset32` immediate, so adjust the base | ||
| // pointer if necessary. | ||
| let (addr, offset) = if offset > i32::MAX as u32 { | ||
| // Offset doesn't fit in the load/store instruction. | ||
| let adj = builder.ins().iadd_imm(base, i64::from(i32::MAX) + 1); | ||
| (adj, (offset - (i32::MAX as u32 + 1)) as i32) | ||
| } else { | ||
| (base, offset as i32) | ||
| let (addr, offset) = match u32::try_from(adjusted_offset) { | ||
| // If our adjusted offset fits within a u32, then we can place the | ||
| // entire offset into the offset of the `heap_addr` instruction. After | ||
| // the `heap_addr` instruction, though, we need to factor the the offset | ||
| // into the returned address. This is either an immediate if the offset | ||
| // further fits within `i32`, or a manual add instruction otherwise. | ||
| // | ||
| // Note that native instructions take a signed offset hence the switch | ||
| // to i32. Note also the lack of overflow checking in the offset | ||
| // addition, which should be ok since if `heap_addr` passed we're | ||
| // guaranteed that this won't overflow. | ||
| Ok(adjusted_offset) => { | ||
| let base = builder | ||
| .ins() | ||
| .heap_addr(environ.pointer_type(), heap, addr, adjusted_offset); | ||
| match i32::try_from(memarg.offset) { | ||
| Ok(val) => (base, val), | ||
| Err(_) => { | ||
| let adj = builder.ins().iadd_imm(base, memarg.offset as i64); | ||
| (adj, 0) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // If the adjusted offset doesn't fit within a u32, then this gets | ||
| // pessimized a fair amount. We can't pass the fixed-sized offset to | ||
| // the `heap_addr` instruction, so we need to fold the offset into the | ||
| // address itself. In doing so though we need to check for overflow | ||
| // because that would mean the address is out-of-bounds. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A comment here about why we decided not to expand |
||
| // | ||
| // Once we have the effective address, offset already folded in, then | ||
| // `heap_addr` is used to verify that the address is indeed in-bounds. | ||
| // The access size of the `heap_addr` is what we were passed in from | ||
| // above. | ||
| // | ||
| // Note that this is generating what's likely to be at least two | ||
| // branches, one for the overflow and one for the bounds check itself. | ||
| // For now though that should hopefully be ok since 4gb+ offsets are | ||
| // relatively odd/rare. | ||
| Err(_) => { | ||
| let index_type = builder.func.heaps[heap].index_type; | ||
| let offset = builder.ins().iconst(index_type, memarg.offset as i64); | ||
| let (addr, overflow) = builder.ins().iadd_ifcout(addr, offset); | ||
| builder.ins().trapif( | ||
| environ.unsigned_add_overflow_condition(), | ||
| overflow, | ||
| ir::TrapCode::HeapOutOfBounds, | ||
| ); | ||
| let base = builder | ||
| .ins() | ||
| .heap_addr(environ.pointer_type(), heap, addr, access_size); | ||
| (base, 0) | ||
| } | ||
| }; | ||
|
|
||
| // Note that we don't set `is_aligned` here, even if the load instruction's | ||
|
|
||
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
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
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
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
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
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
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
Oops, something went wrong.
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.
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.
Good catch (avoiding wraparound). Could you add a comment re: the saturation here to note this is protecting against offset + access-size overflowing?