Skip to content

Commit

Permalink
Plumb memory type further down in Cranelift
Browse files Browse the repository at this point in the history 
This commit removes the `HeapStyle` structure from Cranelift and instead
plumbs the `wasmtime_environ::Memory` type further down the stack
through in `HeapData` (same as Winch before this commit). This removes
redundant fields in `MemoryType` and continues to push down the
`MemoryStyle` structure even further.

This commit additionally and unconditionally defines a `GlobalValue` for
the heap limit of memory. This is unused most of the time for 32-bit
wasm and is conditionally used depending on how bounds checks are
generated. This is a small amount of bloat to each function since
previously functions that didn't need this `GlobalValue` elided it. A
future refactoring, however, will make it a bit more clear how this is
used even for "static" memories.
  • Loading branch information
@alexcrichton
alexcrichton committed Nov 2, 2024
1 parent 809a1cf commit ecdd3ee
Show file tree
Hide file tree
Showing 5 changed files with 70 additions and 123 deletions.
57 changes: 14 additions & 43 deletions crates/cranelift/src/func_environ.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::translate::{
FuncEnvironment as _, FuncTranslationState, GlobalVariable, Heap, HeapData, HeapStyle,
StructFieldsVec, TableData, TableSize, TargetEnvironment,
FuncEnvironment as _, FuncTranslationState, GlobalVariable, Heap, HeapData, StructFieldsVec,
TableData, TableSize, TargetEnvironment,
};
use crate::{gc, BuiltinFunctionSignatures, TRAP_INTERNAL_ASSERT};
use cranelift_codegen::cursor::FuncCursor;
Expand Down Expand Up @@ -2309,18 +2309,6 @@ impl<'module_environment> crate::translate::FuncEnvironment
let memory = self.module.memories[index];
let is_shared = memory.shared;

let min_size = memory.minimum_byte_size().unwrap_or_else(|_| {
// The only valid Wasm memory size that won't fit in a 64-bit
// integer is the maximum memory64 size (2^64) which is one
// larger than `u64::MAX` (2^64 - 1). In this case, just say the
// minimum heap size is `u64::MAX`.
debug_assert_eq!(memory.limits.min, 1 << 48);
debug_assert_eq!(memory.page_size(), 1 << 16);
u64::MAX
});

let max_size = memory.maximum_byte_size().ok();

let (ptr, base_offset, current_length_offset, ptr_memtype) = {
let vmctx = self.vmctx(func);
if let Some(def_index) = self.module.defined_memory_index(index) {
Expand Down Expand Up @@ -2373,20 +2361,18 @@ impl<'module_environment> crate::translate::FuncEnvironment
}
};

let page_size_log2 = memory.page_size_log2;
let heap_bound = func.create_global_value(ir::GlobalValueData::Load {
base: ptr,
offset: Offset32::new(current_length_offset),
global_type: pointer_type,
flags: MemFlags::trusted(),
});

// If we have a declared maximum, we can make this a "static" heap, which is
// allocated up front and never moved.
let style = MemoryStyle::for_memory(memory, self.tunables);
let (heap_style, readonly_base, base_fact, memory_type) = match style {
let (readonly_base, base_fact, memory_type) = match style {
MemoryStyle::Dynamic { .. } => {
let heap_bound = func.create_global_value(ir::GlobalValueData::Load {
base: ptr,
offset: Offset32::new(current_length_offset),
global_type: pointer_type,
flags: MemFlags::trusted(),
});

let (base_fact, data_mt) = if let Some(ptr_memtype) = ptr_memtype {
// Create a memtype representing the untyped memory region.
let data_mt = func.create_memory_type(ir::MemoryTypeData::DynamicMemory {
Expand Down Expand Up @@ -2442,14 +2428,7 @@ impl<'module_environment> crate::translate::FuncEnvironment
(None, None)
};

(
HeapStyle::Dynamic {
bound_gv: heap_bound,
},
false,
base_fact,
data_mt,
)
(false, base_fact, data_mt)
}
MemoryStyle::Static {
byte_reservation: bound_bytes,
Expand Down Expand Up @@ -2497,12 +2476,7 @@ impl<'module_environment> crate::translate::FuncEnvironment
} else {
(None, None)
};
(
HeapStyle::Static { bound: bound_bytes },
true,
base_fact,
data_mt,
)
(true, base_fact, data_mt)
}
};

Expand All @@ -2520,12 +2494,9 @@ impl<'module_environment> crate::translate::FuncEnvironment

Ok(self.heaps.push(HeapData {
base: heap_base,
min_size,
max_size,
style: heap_style,
index_type: index_type_to_ir_type(self.memory(index).idx_type),
memory_type,
page_size_log2,
bound: heap_bound,
pcc_memory_type: memory_type,
memory,
}))
}

Expand Down
10 changes: 6 additions & 4 deletions crates/cranelift/src/translate/code_translator.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1254,7 +1254,7 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
let effective_addr = if memarg.offset == 0 {
addr
} else {
let index_type = environ.heaps()[heap].index_type;
let index_type = environ.heaps()[heap].index_type();
let offset = builder.ins().iconst(index_type, memarg.offset as i64);
environ.uadd_overflow_trap(builder, addr, offset, ir::TrapCode::HEAP_OUT_OF_BOUNDS)
};
Expand All @@ -1278,7 +1278,7 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
let effective_addr = if memarg.offset == 0 {
addr
} else {
let index_type = environ.heaps()[heap].index_type;
let index_type = environ.heaps()[heap].index_type();
let offset = builder.ins().iconst(index_type, memarg.offset as i64);
environ.uadd_overflow_trap(builder, addr, offset, ir::TrapCode::HEAP_OUT_OF_BOUNDS)
};
Expand Down Expand Up @@ -3222,7 +3222,9 @@ where
// relatively odd/rare. In the future if needed we can look into
// optimizing this more.
Err(_) => {
let offset = builder.ins().iconst(heap.index_type, memarg.offset as i64);
let offset = builder
.ins()
.iconst(heap.index_type(), memarg.offset.signed());
let adjusted_index = environ.uadd_overflow_trap(
builder,
index,
Expand Down Expand Up @@ -3251,7 +3253,7 @@ where
let mut flags = MemFlags::new();
flags.set_endianness(ir::Endianness::Little);

if heap.memory_type.is_some() {
if heap.pcc_memory_type.is_some() {
// Proof-carrying code is enabled; check this memory access.
flags.set_checked();
}
Expand Down
75 changes: 35 additions & 40 deletions crates/cranelift/src/translate/code_translator/bounds_checks.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,14 @@
//! !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

use super::Reachability;
use crate::translate::{FuncEnvironment, HeapData, HeapStyle};
use crate::translate::{FuncEnvironment, HeapData};
use cranelift_codegen::{
cursor::{Cursor, FuncCursor},
ir::{self, condcodes::IntCC, InstBuilder, RelSourceLoc},
ir::{Expr, Fact},
};
use cranelift_frontend::FunctionBuilder;
use wasmtime_environ::WasmResult;
use wasmtime_environ::{MemoryStyle, WasmResult};
use Reachability::*;

/// Helper used to emit bounds checks (as necessary) and compute the native
Expand All @@ -50,12 +50,13 @@ where
Env: FuncEnvironment + ?Sized,
{
let pointer_bit_width = u16::try_from(env.pointer_type().bits()).unwrap();
let bound_gv = heap.bound;
let orig_index = index;
let index = cast_index_to_pointer_ty(
index,
heap.index_type,
heap.index_type(),
env.pointer_type(),
heap.memory_type.is_some(),
heap.pcc_memory_type.is_some(),
&mut builder.cursor(),
);
let offset_and_size = offset_plus_size(offset, access_size);
Expand All @@ -64,7 +65,7 @@ where

let host_page_size_log2 = env.target_config().page_size_align_log2;
let can_use_virtual_memory =
heap.page_size_log2 >= host_page_size_log2 && env.signals_based_traps();
heap.memory.page_size_log2 >= host_page_size_log2 && env.signals_based_traps();
let memory_guard_size = env.tunables().memory_guard_size;

let make_compare = |builder: &mut FunctionBuilder,
Expand Down Expand Up @@ -138,14 +139,15 @@ where
// different bounds checks and optimizations of those bounds checks. It is
// intentionally written in a straightforward case-matching style that will
// hopefully make it easy to port to ISLE one day.
Ok(match heap.style {
let style = MemoryStyle::for_memory(heap.memory, env.tunables());
Ok(match style {
// ====== Dynamic Memories ======
//
// 1. First special case for when `offset + access_size == 1`:
//
// index + 1 > bound
// ==> index >= bound
HeapStyle::Dynamic { bound_gv } if offset_and_size == 1 => {
MemoryStyle::Dynamic { .. } if offset_and_size == 1 => {
let bound = get_dynamic_heap_bound(builder, env, heap);
let oob = make_compare(
builder,
Expand All @@ -163,7 +165,7 @@ where
offset,
access_size,
spectre_mitigations_enabled,
AddrPcc::dynamic(heap.memory_type, bound_gv),
AddrPcc::dynamic(heap.pcc_memory_type, bound_gv),
oob,
))
}
Expand Down Expand Up @@ -193,7 +195,7 @@ where
// offset immediates -- which is a common code pattern when accessing
// multiple fields in the same struct that is in linear memory --
// will all emit the same `index > bound` check, which we can GVN.
HeapStyle::Dynamic { bound_gv }
MemoryStyle::Dynamic { .. }
if can_use_virtual_memory && offset_and_size <= memory_guard_size =>
{
let bound = get_dynamic_heap_bound(builder, env, heap);
Expand All @@ -213,7 +215,7 @@ where
offset,
access_size,
spectre_mitigations_enabled,
AddrPcc::dynamic(heap.memory_type, bound_gv),
AddrPcc::dynamic(heap.pcc_memory_type, bound_gv),
oob,
))
}
Expand All @@ -225,7 +227,9 @@ where
//
// index + offset + access_size > bound
// ==> index > bound - (offset + access_size)
HeapStyle::Dynamic { bound_gv } if offset_and_size <= heap.min_size.into() => {
MemoryStyle::Dynamic { .. }
if offset_and_size <= heap.memory.minimum_byte_size().unwrap_or(u64::MAX) =>
{
let bound = get_dynamic_heap_bound(builder, env, heap);
let adjustment = offset_and_size as i64;
let adjustment_value = builder.ins().iconst(env.pointer_type(), adjustment);
Expand Down Expand Up @@ -257,7 +261,7 @@ where
offset,
access_size,
spectre_mitigations_enabled,
AddrPcc::dynamic(heap.memory_type, bound_gv),
AddrPcc::dynamic(heap.pcc_memory_type, bound_gv),
oob,
))
}
Expand All @@ -267,7 +271,7 @@ where
// index + offset + access_size > bound
//
// And we have to handle the overflow case in the left-hand side.
HeapStyle::Dynamic { bound_gv } => {
MemoryStyle::Dynamic { .. } => {
let access_size_val = builder
.ins()
// Explicit cast from u64 to i64: we just want the raw
Expand Down Expand Up @@ -307,7 +311,7 @@ where
offset,
access_size,
spectre_mitigations_enabled,
AddrPcc::dynamic(heap.memory_type, bound_gv),
AddrPcc::dynamic(heap.pcc_memory_type, bound_gv),
oob,
))
}
Expand All @@ -320,7 +324,7 @@ where
// 1. First special case: trap immediately if `offset + access_size >
// bound`, since we will end up being out-of-bounds regardless of the
// given `index`.
HeapStyle::Static { bound } if offset_and_size > bound.into() => {
MemoryStyle::Static { byte_reservation } if offset_and_size > byte_reservation => {
assert!(
can_use_virtual_memory,
"static memories require the ability to use virtual memory"
Expand Down Expand Up @@ -368,11 +372,11 @@ where
// `u32::MAX`. This means that `index` is always either in bounds or
// within the guard page region, neither of which require emitting an
// explicit bounds check.
HeapStyle::Static { bound }
MemoryStyle::Static { byte_reservation }
if can_use_virtual_memory
&& heap.index_type == ir::types::I32
&& heap.index_type() == ir::types::I32
&& u64::from(u32::MAX)
<= u64::from(bound) + u64::from(memory_guard_size) - offset_and_size =>
<= byte_reservation + memory_guard_size - offset_and_size =>
{
assert!(
can_use_virtual_memory,
Expand All @@ -384,10 +388,7 @@ where
env.pointer_type(),
index,
offset,
AddrPcc::static32(
heap.memory_type,
u64::from(bound) + u64::from(memory_guard_size),
),
AddrPcc::static32(heap.pcc_memory_type, byte_reservation + memory_guard_size),
))
}

Expand All @@ -402,14 +403,14 @@ where
// Since we have to emit explicit bounds checks, we might as well be
// precise, not rely on the virtual memory subsystem at all, and not
// factor in the guard pages here.
HeapStyle::Static { bound } => {
MemoryStyle::Static { byte_reservation } => {
assert!(
can_use_virtual_memory,
"static memories require the ability to use virtual memory"
);
// NB: this subtraction cannot wrap because we didn't hit the first
// special case.
let adjusted_bound = u64::from(bound) - offset_and_size;
let adjusted_bound = byte_reservation - offset_and_size;
let adjusted_bound_value = builder
.ins()
.iconst(env.pointer_type(), adjusted_bound as i64);
Expand All @@ -433,7 +434,7 @@ where
offset,
access_size,
spectre_mitigations_enabled,
AddrPcc::static32(heap.memory_type, u64::from(bound)),
AddrPcc::static32(heap.pcc_memory_type, byte_reservation),
oob,
))
}
Expand All @@ -449,9 +450,9 @@ fn get_dynamic_heap_bound<Env>(
where
Env: FuncEnvironment + ?Sized,
{
let enable_pcc = heap.memory_type.is_some();
let enable_pcc = heap.pcc_memory_type.is_some();

let (value, gv) = match (heap.max_size, &heap.style) {
let (value, gv) = match heap.memory.static_heap_size() {
// The heap has a constant size, no need to actually load the
// bound. TODO: this is currently disabled for PCC because we
// can't easily prove that the GV load indeed results in a
Expand All @@ -460,22 +461,16 @@ where
// in the GV, then re-enable this opt. (Or, alternately,
// compile such memories with a static-bound memtype and
// facts.)
(Some(max_size), HeapStyle::Dynamic { bound_gv })
if heap.min_size == max_size && !enable_pcc =>
{
(
builder.ins().iconst(env.pointer_type(), max_size as i64),
*bound_gv,
)
}
Some(max_size) if !enable_pcc => (
builder.ins().iconst(env.pointer_type(), max_size as i64),
heap.bound,
),

// Load the heap bound from its global variable.
(_, HeapStyle::Dynamic { bound_gv }) => (
builder.ins().global_value(env.pointer_type(), *bound_gv),
*bound_gv,
_ => (
builder.ins().global_value(env.pointer_type(), heap.bound),
heap.bound,
),

(_, HeapStyle::Static { .. }) => unreachable!("not a dynamic heap"),
};

// If proof-carrying code is enabled, apply a fact to the range to
Expand Down
Loading

0 comments on commit ecdd3ee

Please sign in to comment.