Skip to content
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.

Sign up for GitHub

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

pulley: Ungate memory64 feature #9780

Merged
merged 2 commits into from
Dec 11, 2024
Merged

pulley: Ungate memory64 feature #9780

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
01093d5
pulley: Ungate memory64 feature
alexcrichton Dec 10, 2024
3d22b3d
Add notes about spectre speculation
alexcrichton Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions cranelift/codegen/src/isa/pulley_shared/lower.isle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,30 @@
(rule (lower (has_type $I32 (sdiv a b)))
(pulley_xdiv32_s a b))

;;;; Rules for `ishl` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

(rule (lower (has_type $I32 (ishl a b)))
(pulley_xshl32 a b))

(rule (lower (has_type $I64 (ishl a b)))
(pulley_xshl64 a b))

;;;; Rules for `ushr` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

(rule (lower (has_type $I32 (ushr a b)))
(pulley_xshr32_u a b))

(rule (lower (has_type $I64 (ushr a b)))
(pulley_xshr64_u a b))

;;;; Rules for `sshr` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

(rule (lower (has_type $I32 (sshr a b)))
(pulley_xshr32_s a b))

(rule (lower (has_type $I64 (sshr a b)))
(pulley_xshr64_s a b))

;;;; Rules for `band` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

(rule (lower (has_type $I32 (band a b)))
Expand Down Expand Up @@ -338,6 +362,11 @@
(rule (lower (has_type (fits_in_64 _) (sextend val @ (value_type $I32))))
(pulley_sext32 val))

;;;; Rules for `ireduce` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

(rule (lower (has_type (fits_in_64 _ty) (ireduce src)))
src)

;;;; Rules for `uadd_overflow_trap` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

(rule (lower (has_type $I32 (uadd_overflow_trap a b tc)))
Expand Down
28 changes: 23 additions & 5 deletions crates/cranelift/src/translate/code_translator/bounds_checks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,11 +491,29 @@ fn cast_index_to_pointer_ty(
if index_ty == pointer_ty {
return index;
}
// Note that using 64-bit heaps on a 32-bit host is not currently supported,
// would require at least a bounds check here to ensure that the truncation
// from 64-to-32 bits doesn't lose any upper bits. For now though we're
// mostly interested in the 32-bit-heaps-on-64-bit-hosts cast.
assert!(index_ty.bits() < pointer_ty.bits());

// If the index size is larger than the pointer, that means that this is a
// 32-bit host platform with a 64-bit wasm linear memory. If the index is
// larger than 2**32 then that's guranteed to be out-of-bounds, otherwise we
// `ireduce` the index.
//
// Also note that at this time this branch doesn't support pcc nor the
// value-label-ranges of the below path.
//
// Finally, note that the returned `low_bits` here are still subject to an
// explicit bounds check in wasm so in terms of Spectre speculation on
// either side of the `trapnz` should be ok.
if index_ty.bits() > pointer_ty.bits() {
assert_eq!(index_ty, ir::types::I64);
assert_eq!(pointer_ty, ir::types::I32);
let low_bits = pos.ins().ireduce(pointer_ty, index);
let c32 = pos.ins().iconst(pointer_ty, 32);
let high_bits = pos.ins().ushr(index, c32);
let high_bits = pos.ins().ireduce(pointer_ty, high_bits);
pos.ins()
.trapnz(high_bits, ir::TrapCode::HEAP_OUT_OF_BOUNDS);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should make a note re: Spectre here -- trapnz internally uses a branch and so one might expect there to be some exposure here. I think we're actually safe because the worst that happens is that the guest accesses an OOB address like 0x1_0000_1000 where 0x1000 is in-bounds, and gets its own in-bounds data; in other words, this check is layered on top of the actual bounds-check on the lower 32 bits, so there is still not any visibility outside the sandbox in the misspeculated path. But it's... worth noting, if only because the guest might in turn rely on OOBs not to speculatively read valid data (niche but possible).

return low_bits;
}

// Convert `index` to `addr_ty`.
let extended_index = pos.ins().uextend(pointer_ty, index);
Expand Down
4 changes: 1 addition & 3 deletions crates/wasmtime/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1965,9 +1965,7 @@ impl Config {
// errors are panics though due to unimplemented bits in ABI
// code and those causes are listed here.
if self.compiler_target().is_pulley() {
return WasmFeatures::TAIL_CALL
| WasmFeatures::MEMORY64
| WasmFeatures::GC_TYPES;
return WasmFeatures::TAIL_CALL | WasmFeatures::GC_TYPES;
}

// Other Cranelift backends are either 100% missing or complete
Expand Down
16 changes: 0 additions & 16 deletions crates/wasmtime/src/runtime/vm/memory.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,22 +290,6 @@ impl Memory {
// overkill for this purpose.
let absolute_max = 0usize.wrapping_sub(page_size);

// Sanity-check what should already be true from wasm module validation.
// Note that for 32-bit targets the absolute maximum is `1<<32` during
// compilation, not one-page-less-than-u32::MAX, so need to handle that
// specially here.
let absolute_max64 = if cfg!(target_pointer_width = "32") {
1 << 32
} else {
u64::try_from(absolute_max).unwrap()
};
if let Ok(size) = ty.minimum_byte_size() {
assert!(size <= absolute_max64);
}
if let Ok(max) = ty.maximum_byte_size() {
assert!(max <= absolute_max64);
}
Comment on lines -293 to -307
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why were these asserts removed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They were basically a pain to maintain on 32-bit because I already had to modify the minimum check to use 1<<32 rather than absolute_max (due to absolute_max being a usize and not being able to represent 1<<32) and then this PR uncovered an assertion in the next one where a 64-bit linear memory's maximum size far exceeds 1<<32 as well. Given that these are just debug asserts that are already basically checked in many other places it seemed best to just remove them instead of trying to contort ourselves to keep them in all portable conditions.


// If the minimum memory size overflows the size of our own address
// space, then we can't satisfy this request, but defer the error to
// later so the `store` can be informed that an effective oom is
Expand Down
46 changes: 28 additions & 18 deletions crates/wast-util/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,7 @@ impl WastTest {
"misc_testsuite/component-model/nested.wast",
"misc_testsuite/component-model/types.wast",
"misc_testsuite/control-flow.wast",
"misc_testsuite/custom-page-sizes/custom-page-sizes.wast",
"misc_testsuite/elem-ref-null.wast",
"misc_testsuite/elem_drop.wast",
"misc_testsuite/empty.wast",
Expand All @@ -426,12 +427,23 @@ impl WastTest {
"misc_testsuite/imported-memory-copy.wast",
"misc_testsuite/issue4857.wast",
"misc_testsuite/memory-copy.wast",
"misc_testsuite/memory64/bounds.wast",
"misc_testsuite/memory64/linking-errors.wast",
"misc_testsuite/memory64/linking.wast",
"misc_testsuite/memory64/multi-memory.wast",
"misc_testsuite/memory64/offsets.wast",
"misc_testsuite/multi-memory/simple.wast",
"misc_testsuite/partial-init-memory-segment.wast",
"misc_testsuite/rs2wasm-add-func.wast",
"misc_testsuite/stack_overflow.wast",
"misc_testsuite/table_grow_with_funcref.wast",
"misc_testsuite/threads/atomics_notify.wast",
"misc_testsuite/threads/atomics_wait_address.wast",
"misc_testsuite/threads/wait_notify.wast",
"misc_testsuite/winch/_simd_linking.wast",
"misc_testsuite/winch/misc.wast",
"misc_testsuite/winch/oob.wast",
"misc_testsuite/winch/table_grow.wast",
"spec_testsuite/address.wast",
"spec_testsuite/binary-leb128.wast",
"spec_testsuite/binary.wast",
Expand All @@ -445,11 +457,14 @@ impl WastTest {
"spec_testsuite/memory_copy.wast",
"spec_testsuite/memory_fill.wast",
"spec_testsuite/memory_init.wast",
"spec_testsuite/memory_size.wast",
"spec_testsuite/memory_trap.wast",
"spec_testsuite/names.wast",
"spec_testsuite/obsolete-keywords.wast",
"spec_testsuite/proposals/annotations/annotations.wast",
"spec_testsuite/proposals/annotations/id.wast",
"spec_testsuite/proposals/annotations/token.wast",
"spec_testsuite/proposals/custom-page-sizes/custom-page-sizes.wast",
"spec_testsuite/proposals/exception-handling/binary.wast",
"spec_testsuite/proposals/multi-memory/address0.wast",
"spec_testsuite/proposals/multi-memory/address1.wast",
Expand All @@ -462,21 +477,33 @@ impl WastTest {
"spec_testsuite/proposals/multi-memory/exports0.wast",
"spec_testsuite/proposals/multi-memory/imports1.wast",
"spec_testsuite/proposals/multi-memory/imports2.wast",
"spec_testsuite/proposals/multi-memory/imports4.wast",
"spec_testsuite/proposals/multi-memory/linking1.wast",
"spec_testsuite/proposals/multi-memory/linking2.wast",
"spec_testsuite/proposals/multi-memory/load0.wast",
"spec_testsuite/proposals/multi-memory/load1.wast",
"spec_testsuite/proposals/multi-memory/memory-multi.wast",
"spec_testsuite/proposals/multi-memory/memory_copy0.wast",
"spec_testsuite/proposals/multi-memory/memory_copy1.wast",
"spec_testsuite/proposals/multi-memory/memory_fill0.wast",
"spec_testsuite/proposals/multi-memory/memory_init0.wast",
"spec_testsuite/proposals/multi-memory/memory_size.wast",
"spec_testsuite/proposals/multi-memory/memory_size0.wast",
"spec_testsuite/proposals/multi-memory/memory_size1.wast",
"spec_testsuite/proposals/multi-memory/memory_size2.wast",
"spec_testsuite/proposals/multi-memory/memory_size3.wast",
"spec_testsuite/proposals/multi-memory/memory_trap0.wast",
"spec_testsuite/proposals/multi-memory/memory_trap1.wast",
"spec_testsuite/proposals/multi-memory/start0.wast",
"spec_testsuite/proposals/multi-memory/store.wast",
"spec_testsuite/proposals/multi-memory/store0.wast",
"spec_testsuite/proposals/multi-memory/store1.wast",
"spec_testsuite/proposals/multi-memory/trap0.wast",
"spec_testsuite/proposals/multi-memory/traps0.wast",
"spec_testsuite/proposals/threads/atomics_notify.wast",
"spec_testsuite/proposals/threads/atomics_wait_address.wast",
"spec_testsuite/proposals/threads/exports.wast",
"spec_testsuite/proposals/threads/wait_notify.wast",
"spec_testsuite/simd_linking.wast",
"spec_testsuite/skip-stack-guard-page.wast",
"spec_testsuite/start.wast",
Expand All @@ -493,30 +520,13 @@ impl WastTest {
"spec_testsuite/utf8-import-field.wast",
"spec_testsuite/utf8-import-module.wast",
"spec_testsuite/utf8-invalid-encoding.wast",
"threads/exports.wast",
"misc_testsuite/memory64/more-than-4gb.wast",
];

if supported.iter().any(|part| self.path.ends_with(part)) {
return false;
}

// FIXME: once the backend has enough instruction support move these
// into the above tests since they should pass on 64-bit platforms
// as well.
let supported32bit = [
"misc_testsuite/winch/table_grow.wast",
"misc_testsuite/table_grow_with_funcref.wast",
"spec_testsuite/proposals/multi-memory/trap0.wast",
"spec_testsuite/proposals/multi-memory/memory_trap0.wast",
"spec_testsuite/proposals/multi-memory/linking2.wast",
"spec_testsuite/memory_trap.wast",
];
if cfg!(target_pointer_width = "32") {
if supported32bit.iter().any(|part| self.path.ends_with(part)) {
return false;
}
}

return true;
}

Expand Down
42 changes: 42 additions & 0 deletions pulley/src/interp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1131,6 +1131,48 @@ impl OpVisitor for Interpreter<'_> {
ControlFlow::Continue(())
}

fn xshl32(&mut self, operands: BinaryOperands<XReg>) -> ControlFlow<Done> {
let a = self.state[operands.src1].get_u32();
let b = self.state[operands.src2].get_u32();
self.state[operands.dst].set_u32(a.wrapping_shl(b));
ControlFlow::Continue(())
}

fn xshr32_u(&mut self, operands: BinaryOperands<XReg>) -> ControlFlow<Done> {
let a = self.state[operands.src1].get_u32();
let b = self.state[operands.src2].get_u32();
self.state[operands.dst].set_u32(a.wrapping_shr(b));
ControlFlow::Continue(())
}

fn xshr32_s(&mut self, operands: BinaryOperands<XReg>) -> ControlFlow<Done> {
let a = self.state[operands.src1].get_i32();
let b = self.state[operands.src2].get_u32();
self.state[operands.dst].set_i32(a.wrapping_shr(b));
ControlFlow::Continue(())
}

fn xshl64(&mut self, operands: BinaryOperands<XReg>) -> ControlFlow<Done> {
let a = self.state[operands.src1].get_u64();
let b = self.state[operands.src2].get_u32();
self.state[operands.dst].set_u64(a.wrapping_shl(b));
ControlFlow::Continue(())
}

fn xshr64_u(&mut self, operands: BinaryOperands<XReg>) -> ControlFlow<Done> {
let a = self.state[operands.src1].get_u64();
let b = self.state[operands.src2].get_u32();
self.state[operands.dst].set_u64(a.wrapping_shr(b));
ControlFlow::Continue(())
}

fn xshr64_s(&mut self, operands: BinaryOperands<XReg>) -> ControlFlow<Done> {
let a = self.state[operands.src1].get_i64();
let b = self.state[operands.src2].get_u32();
self.state[operands.dst].set_i64(a.wrapping_shr(b));
ControlFlow::Continue(())
}

fn xeq64(&mut self, operands: BinaryOperands<XReg>) -> ControlFlow<Done> {
let a = self.state[operands.src1].get_u64();
let b = self.state[operands.src2].get_u64();
Expand Down
13 changes: 13 additions & 0 deletions pulley/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,19 @@ macro_rules! for_each_op {
/// 64-bit wrapping subtraction: `dst = src1 - src2`.
xsub64 = Xsub64 { operands: BinaryOperands<XReg> };

/// `low32(dst) = low32(src1) << low5(src2)`
xshl32 = Xshl32 { operands: BinaryOperands<XReg> };
/// `low32(dst) = low32(src1) >> low5(src2)`
xshr32_s = Xshr32S { operands: BinaryOperands<XReg> };
/// `low32(dst) = low32(src1) >> low5(src2)`
xshr32_u = Xshr32U { operands: BinaryOperands<XReg> };
/// `dst = src1 << low5(src2)`
xshl64 = Xshl64 { operands: BinaryOperands<XReg> };
/// `dst = src1 >> low6(src2)`
xshr64_s = Xshr64S { operands: BinaryOperands<XReg> };
/// `dst = src1 >> low6(src2)`
xshr64_u = Xshr64U { operands: BinaryOperands<XReg> };

/// 64-bit equality.
xeq64 = Xeq64 { operands: BinaryOperands<XReg> };
/// 64-bit inequality.
Expand Down
Loading