Skip to content

Commit

Permalink
winch: Gracefully handle compilation errors (#9851)
Browse files Browse the repository at this point in the history
* winch: Gracefully handle compilation errors

Closes: #8096

This commit threads `anyhow::Result`  through most of Winch's
compilation process in order to gracefully handle compilation errors
gracefully instead of panicking.

The error classification is intentionally very granular, to avoid string
allocation which could impact compilation performance.

The errors are largely fit in two categories:

* Unimplemented/Unsupported
* Internal

The firs category signals partial or no support for Wasmtime features
and or Wasm proposals. These errors are meant to be temporary while
such features or proposals are in development.

The second category signals that a compilation invariant was not met.
These errors are considered internal and their presence usually means
a bug in the compiler.

* Include `Result` in the MacroAssembler

This commit updates the MacroAssembler trait to require returning
`Result<T>`  on every method in the interface, making it easier to
detect partial support for Masm instructions.
  • Loading branch information
saulecabrera authored Jan 1, 2025
1 parent 5ca3715 commit b93e1bc
Show file tree
Hide file tree
Showing 17 changed files with 2,556 additions and 1,783 deletions.
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions winch/codegen/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ regalloc2 = { workspace = true }
gimli = { workspace = true }
wasmtime-environ = { workspace = true }
wasmtime-cranelift = { workspace = true }
thiserror = { workspace = true }

[features]
x64 = ["cranelift-codegen/x86"]
Expand Down
64 changes: 35 additions & 29 deletions winch/codegen/src/codegen/bounds.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use crate::{
masm::{IntCmpKind, MacroAssembler, OperandSize, RegImm, TrapCode},
stack::TypedReg,
};
use anyhow::Result;
use wasmtime_environ::Signed;

/// A newtype to represent an immediate offset argument for a heap access.
Expand Down Expand Up @@ -86,30 +87,33 @@ pub(crate) fn load_dynamic_heap_bounds<M>(
masm: &mut M,
heap: &HeapData,
ptr_size: OperandSize,
) -> Bounds
) -> Result<Bounds>
where
M: MacroAssembler,
{
let dst = context.any_gpr(masm);
let dst = context.any_gpr(masm)?;
match heap.memory.static_heap_size() {
// Constant size, no need to perform a load.
Some(size) => masm.mov(writable!(dst), RegImm::i64(size.signed()), ptr_size),
Some(size) => masm.mov(writable!(dst), RegImm::i64(size.signed()), ptr_size)?,

None => {
let scratch = scratch!(M);
let base = if let Some(offset) = heap.import_from {
let addr = masm.address_at_vmctx(offset);
masm.load_ptr(addr, writable!(scratch));
let addr = masm.address_at_vmctx(offset)?;
masm.load_ptr(addr, writable!(scratch))?;
scratch
} else {
vmctx!(M)
};
let addr = masm.address_at_reg(base, heap.current_length_offset);
masm.load_ptr(addr, writable!(dst));
let addr = masm.address_at_reg(base, heap.current_length_offset)?;
masm.load_ptr(addr, writable!(dst))?;
}
}

Bounds::from_typed_reg(TypedReg::new(heap.index_type(), dst))
Ok(Bounds::from_typed_reg(TypedReg::new(
heap.index_type(),
dst,
)))
}

/// This function ensures the following:
Expand All @@ -125,10 +129,10 @@ pub(crate) fn ensure_index_and_offset<M: MacroAssembler>(
index: Index,
offset: u64,
heap_ty_size: OperandSize,
) -> ImmOffset {
) -> Result<ImmOffset> {
match u32::try_from(offset) {
// If the immediate offset fits in a u32, then we simply return.
Ok(offs) => ImmOffset::from_u32(offs),
Ok(offs) => Ok(ImmOffset::from_u32(offs)),
// Else we adjust the index to be index = index + offset, including an
// overflow check, and return 0 as the offset.
Err(_) => {
Expand All @@ -138,9 +142,9 @@ pub(crate) fn ensure_index_and_offset<M: MacroAssembler>(
RegImm::i64(offset as i64),
heap_ty_size,
TrapCode::HEAP_OUT_OF_BOUNDS,
);
)?;

ImmOffset::from_u32(0)
Ok(ImmOffset::from_u32(0))
}
}
}
Expand All @@ -157,28 +161,28 @@ pub(crate) fn load_heap_addr_checked<M, F>(
index: Index,
offset: ImmOffset,
mut emit_check_condition: F,
) -> Reg
) -> Result<Reg>
where
M: MacroAssembler,
F: FnMut(&mut M, Bounds, Index) -> IntCmpKind,
F: FnMut(&mut M, Bounds, Index) -> Result<IntCmpKind>,
{
let cmp_kind = emit_check_condition(masm, bounds, index);
let cmp_kind = emit_check_condition(masm, bounds, index)?;

masm.trapif(cmp_kind, TrapCode::HEAP_OUT_OF_BOUNDS);
let addr = context.any_gpr(masm);
masm.trapif(cmp_kind, TrapCode::HEAP_OUT_OF_BOUNDS)?;
let addr = context.any_gpr(masm)?;

load_heap_addr_unchecked(masm, heap, index, offset, addr, ptr_size);
load_heap_addr_unchecked(masm, heap, index, offset, addr, ptr_size)?;
if !enable_spectre_mitigation {
addr
Ok(addr)
} else {
// Conditionally assign 0 to the register holding the base address if
// the comparison kind is met.
let tmp = context.any_gpr(masm);
masm.mov(writable!(tmp), RegImm::i64(0), ptr_size);
let cmp_kind = emit_check_condition(masm, bounds, index);
masm.cmov(writable!(addr), tmp, cmp_kind, ptr_size);
let tmp = context.any_gpr(masm)?;
masm.mov(writable!(tmp), RegImm::i64(0), ptr_size)?;
let cmp_kind = emit_check_condition(masm, bounds, index)?;
masm.cmov(writable!(addr), tmp, cmp_kind, ptr_size)?;
context.free_reg(tmp);
addr
Ok(addr)
}
}

Expand All @@ -192,14 +196,15 @@ pub(crate) fn load_heap_addr_unchecked<M>(
offset: ImmOffset,
dst: Reg,
ptr_size: OperandSize,
) where
) -> Result<()>
where
M: MacroAssembler,
{
let base = if let Some(offset) = heap.import_from {
// If the WebAssembly memory is imported, load the address into
// the scratch register.
let scratch = scratch!(M);
masm.load_ptr(masm.address_at_vmctx(offset), writable!(scratch));
masm.load_ptr(masm.address_at_vmctx(offset)?, writable!(scratch))?;
scratch
} else {
// Else if the WebAssembly memory is defined in the current module,
Expand All @@ -208,17 +213,18 @@ pub(crate) fn load_heap_addr_unchecked<M>(
};

// Load the base of the memory into the `addr` register.
masm.load_ptr(masm.address_at_reg(base, heap.offset), writable!(dst));
masm.load_ptr(masm.address_at_reg(base, heap.offset)?, writable!(dst))?;
// Start by adding the index to the heap base addr.
let index_reg = index.as_typed_reg().reg;
masm.add(writable!(dst), dst, index_reg.into(), ptr_size);
masm.add(writable!(dst), dst, index_reg.into(), ptr_size)?;

if offset.as_u32() > 0 {
masm.add(
writable!(dst),
dst,
RegImm::i64(offset.as_u32() as i64),
ptr_size,
);
)?;
}
Ok(())
}
Loading

0 comments on commit b93e1bc

Please sign in to comment.