diff --git a/Cargo.toml b/Cargo.toml index 76ffce7026f6..5bd6924608c6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -409,18 +409,6 @@ memory-protection-keys = ["wasmtime-cli-flags/memory-protection-keys"] # throughout Wasmtime and its dependencies. disable-logging = ["log/max_level_off", "tracing/max_level_off"] -# Until we implement proper reference counting for `VMContObj` objects, -# we may use this flag to bypass the creation of VMContObj objects, -# directly using the corresponding VMContRef instead. -# This is to allow running benchmarks that may create a lot of such objects and -# may otherwise run out of memory. -# Note that enabling this is highly unsafe, as it makes it impossible to detect -# at runtime when an already taken continuation is used again. -unsafe_disable_continuation_linearity_check = [ - "wasmtime-cranelift/unsafe_disable_continuation_linearity_check", - "wasmtime/unsafe_disable_continuation_linearity_check" -] - # The following toggles the baseline implementation of typed continuations. wasmfx_baseline = [ "wasmtime-cranelift/wasmfx_baseline", diff --git a/crates/c-api/Cargo.toml b/crates/c-api/Cargo.toml index 4c07b80f8462..72a07863482a 100644 --- a/crates/c-api/Cargo.toml +++ b/crates/c-api/Cargo.toml @@ -56,10 +56,6 @@ gc = ["wasmtime/gc"] cranelift = ['wasmtime/cranelift'] winch = ['wasmtime/winch'] -# Enable unsafe mutable continuations (temporary hack to workaround -# the garbage generated by continuation objects). -unsafe_disable_continuation_linearity_check = ["wasmtime/unsafe_disable_continuation_linearity_check"] - # Toggle the baseline implementation of WasmFX wasmfx_baseline = [ "wasmtime/wasmfx_baseline" diff --git a/crates/c-api/artifact/Cargo.toml b/crates/c-api/artifact/Cargo.toml index 85ad03717e8e..5b6fb04892c2 100644 --- a/crates/c-api/artifact/Cargo.toml +++ b/crates/c-api/artifact/Cargo.toml @@ -55,12 +55,6 @@ gc = ["wasmtime-c-api/gc"] cranelift = ["wasmtime-c-api/cranelift"] winch = ["wasmtime-c-api/winch"] -# Enable unsafe mutable continuations (temporary hack to workaround -# the garbage generated by continuation objects). -unsafe_disable_continuation_linearity_check = [ - "wasmtime-c-api/unsafe_disable_continuation_linearity_check" -] - # Toggle the baseline implementation of WasmFX wasmfx_baseline = ["wasmtime-c-api/wasmfx_baseline"] diff --git a/crates/cranelift/Cargo.toml b/crates/cranelift/Cargo.toml index 700b096e269d..9d7b3a0c6928 100644 --- a/crates/cranelift/Cargo.toml +++ b/crates/cranelift/Cargo.toml @@ -43,6 +43,5 @@ incremental-cache = ["cranelift-codegen/incremental-cache"] wmemcheck = ["wasmtime-environ/wmemcheck"] gc = ["wasmtime-environ/gc"] threads = ["wasmtime-environ/threads"] -unsafe_disable_continuation_linearity_check = [] wasmfx_baseline = [] diff --git a/crates/cranelift/src/wasmfx/baseline.rs b/crates/cranelift/src/wasmfx/baseline.rs index 89c1b520f664..35714efa246d 100644 --- a/crates/cranelift/src/wasmfx/baseline.rs +++ b/crates/cranelift/src/wasmfx/baseline.rs @@ -18,12 +18,8 @@ fn get_revision<'a>( builder: &mut FunctionBuilder, contref: ir::Value, ) -> ir::Value { - if cfg!(feature = "unsafe_disable_continuation_linearity_check") { - builder.ins().iconst(I64, 0) - } else { - let mem_flags = ir::MemFlags::trusted(); - builder.ins().load(I64, mem_flags, contref, 0) - } + let mem_flags = ir::MemFlags::trusted(); + builder.ins().load(I64, mem_flags, contref, 0) } fn compare_revision_and_increment<'a>( @@ -32,21 +28,17 @@ fn compare_revision_and_increment<'a>( contref: ir::Value, witness: ir::Value, ) -> ir::Value { - if cfg!(feature = "unsafe_disable_continuation_linearity_check") { - builder.ins().iconst(I64, 0) - } else { - let mem_flags = ir::MemFlags::trusted(); - let revision = get_revision(env, builder, contref); + let mem_flags = ir::MemFlags::trusted(); + let revision = get_revision(env, builder, contref); - let evidence = builder.ins().icmp(IntCC::Equal, witness, revision); - builder - .ins() - .trapz(evidence, ir::TrapCode::ContinuationAlreadyConsumed); + let evidence = builder.ins().icmp(IntCC::Equal, witness, revision); + builder + .ins() + .trapz(evidence, ir::TrapCode::ContinuationAlreadyConsumed); - let revision_plus1 = builder.ins().iadd_imm(revision, 1); - builder.ins().store(mem_flags, revision_plus1, contref, 0); - revision_plus1 - } + let revision_plus1 = builder.ins().iadd_imm(revision, 1); + builder.ins().store(mem_flags, revision_plus1, contref, 0); + revision_plus1 } fn typed_continuations_load_payloads<'a>( diff --git a/crates/cranelift/src/wasmfx/optimized.rs b/crates/cranelift/src/wasmfx/optimized.rs index b46436604e3d..2c3779529bb1 100644 --- a/crates/cranelift/src/wasmfx/optimized.rs +++ b/crates/cranelift/src/wasmfx/optimized.rs @@ -400,14 +400,10 @@ pub(crate) mod typed_continuation_helpers { _env: &mut crate::func_environ::FuncEnvironment<'a>, builder: &mut FunctionBuilder, ) -> ir::Value { - if cfg!(feature = "unsafe_disable_continuation_linearity_check") { - builder.ins().iconst(I64, 0) - } else { - let mem_flags = ir::MemFlags::trusted(); - let offset = wasmtime_continuations::offsets::vm_cont_ref::REVISION as i32; - let revision = builder.ins().load(I64, mem_flags, self.address, offset); - revision - } + let mem_flags = ir::MemFlags::trusted(); + let offset = wasmtime_continuations::offsets::vm_cont_ref::REVISION as i32; + let revision = builder.ins().load(I64, mem_flags, self.address, offset); + revision } /// Sets the revision counter on the given continuation @@ -418,27 +414,23 @@ pub(crate) mod typed_continuation_helpers { builder: &mut FunctionBuilder, revision: ir::Value, ) -> ir::Value { - if cfg!(feature = "unsafe_disable_continuation_linearity_check") { - builder.ins().iconst(I64, 0) - } else { - if cfg!(debug_assertions) { - let actual_revision = self.get_revision(env, builder); - emit_debug_assert_eq!(env, builder, revision, actual_revision); - } - let mem_flags = ir::MemFlags::trusted(); - let offset = wasmtime_continuations::offsets::vm_cont_ref::REVISION as i32; - let revision_plus1 = builder.ins().iadd_imm(revision, 1); - builder - .ins() - .store(mem_flags, revision_plus1, self.address, offset); - if cfg!(debug_assertions) { - let new_revision = self.get_revision(env, builder); - emit_debug_assert_eq!(env, builder, revision_plus1, new_revision); - // Check for overflow: - emit_debug_assert_ule!(env, builder, revision, revision_plus1); - } - revision_plus1 + if cfg!(debug_assertions) { + let actual_revision = self.get_revision(env, builder); + emit_debug_assert_eq!(env, builder, revision, actual_revision); } + let mem_flags = ir::MemFlags::trusted(); + let offset = wasmtime_continuations::offsets::vm_cont_ref::REVISION as i32; + let revision_plus1 = builder.ins().iadd_imm(revision, 1); + builder + .ins() + .store(mem_flags, revision_plus1, self.address, offset); + if cfg!(debug_assertions) { + let new_revision = self.get_revision(env, builder); + emit_debug_assert_eq!(env, builder, revision_plus1, new_revision); + // Check for overflow: + emit_debug_assert_ule!(env, builder, revision, revision_plus1); + } + revision_plus1 } pub fn get_fiber_stack<'a>( @@ -1574,28 +1566,24 @@ pub(crate) fn translate_resume<'a>( let mut vmcontref = tc::VMContRef::new(resume_contref, env.pointer_type()); let revision = vmcontref.get_revision(env, builder); - if cfg!(not(feature = "unsafe_disable_continuation_linearity_check")) { - let evidence = builder.ins().icmp(IntCC::Equal, revision, witness); - emit_debug_println!( - env, - builder, - "[resume] resume_contref = {:p} witness = {}, revision = {}, evidence = {}", - resume_contref, - witness, - revision, - evidence - ); - builder - .ins() - .trapz(evidence, ir::TrapCode::ContinuationAlreadyConsumed); - } + let evidence = builder.ins().icmp(IntCC::Equal, revision, witness); + emit_debug_println!( + env, + builder, + "[resume] resume_contref = {:p} witness = {}, revision = {}, evidence = {}", + resume_contref, + witness, + revision, + evidence + ); + builder + .ins() + .trapz(evidence, ir::TrapCode::ContinuationAlreadyConsumed); let next_revision = vmcontref.incr_revision(env, builder, revision); emit_debug_println!(env, builder, "[resume] new revision = {}", next_revision); if cfg!(debug_assertions) { // This should be impossible due to the linearity check. - // We keep this check mostly for the test that runs a continuation - // twice with `unsafe_disable_continuation_linearity_check` enabled. let zero = builder.ins().iconst(I8, 0); let csi = vmcontref.common_stack_information(env, builder); let has_returned = csi.has_state(env, builder, wasmtime_continuations::State::Returned); diff --git a/crates/cranelift/src/wasmfx/shared.rs b/crates/cranelift/src/wasmfx/shared.rs index 964d41590dec..3873aec696b4 100644 --- a/crates/cranelift/src/wasmfx/shared.rs +++ b/crates/cranelift/src/wasmfx/shared.rs @@ -28,47 +28,39 @@ pub(crate) use call_builtin; /// The Cranelfift type used to represent all of the following: /// - wasm values of type `(ref null $ct)` and `(ref $ct)` /// - equivalenty: runtime values of type `Option` and `VMContObj` -pub(crate) fn vm_contobj_type(pointer_type: ir::Type) -> ir::Type { - if cfg!(feature = "unsafe_disable_continuation_linearity_check") { - // If linearity checks are disabled, a `VMContObj` is just a pointer - // to the underlying `VMContRef`. - // For consistency with the fat pointer case, we use I32/I64 here - // instead of RI32/I64 (which are used for other reference types) - pointer_type - } else { - // If linearity checks are enabled, a `VMContObj` is a fat pointer - // consisting of a pointer to `VMContRef` and a 64 bit sequence - // counter. - - // Naturally, you may wonder why we don't use any of the following - // types instead: - // - // - I128: We can't use this type, because cranelift only allows - // using this type for parameters/return values if the setting - // `enable_llvm_abi_extensions` is enabled, which is not allowed - // when using cranelift for wasmtime. - // - // - I64X2: If we have to use a 128 bit vector type for our - // continuations in Cranelift, the most reasonable choice would be - // I64X2. After all, our fat pointers consist of an (up to) 64bit - // pointer and a 64 bit counter. The reason why we can't use this - // type is that wasmtime assumes that all wasm SIMD values have the - // same Cranelift type, namely I8X16. As a result, - // [cranelift_wasm::code_translator] liberally inserts `bitcast` - // instructions to turn all vector types it sees into the canonical - // type I8X16. Thus, if we used I64X2 for our continuation values - // in wasm, this canonicalization, intended for actual SIMD wasm - // values, would break our code. `bitcast`-ing between I64X2 and - // I16X8 is a noop, so this has no performance impact. - - // NOTE(frank-emrich) We currently only care about little endian - // platforms. The internal layout of the vector is reflected by - // this, it is identical to what happens if you do a 128bit vector - // load of a `Optional` on a little endian platform: Its - // 64 LSBs contain the revision counter, its 64MSBs contain the - // `VMContRef` pointer. - ir::types::I8X16 - } +pub(crate) fn vm_contobj_type(_pointer_type: ir::Type) -> ir::Type { + // A `VMContObj` is a fat pointer + // consisting of a pointer to `VMContRef` and a 64 bit sequence + // counter. + + // Naturally, you may wonder why we don't use any of the following + // types instead: + // + // - I128: We can't use this type, because cranelift only allows + // using this type for parameters/return values if the setting + // `enable_llvm_abi_extensions` is enabled, which is not allowed + // when using cranelift for wasmtime. + // + // - I64X2: If we have to use a 128 bit vector type for our + // continuations in Cranelift, the most reasonable choice would be + // I64X2. After all, our fat pointers consist of an (up to) 64bit + // pointer and a 64 bit counter. The reason why we can't use this + // type is that wasmtime assumes that all wasm SIMD values have the + // same Cranelift type, namely I8X16. As a result, + // [cranelift_wasm::code_translator] liberally inserts `bitcast` + // instructions to turn all vector types it sees into the canonical + // type I8X16. Thus, if we used I64X2 for our continuation values + // in wasm, this canonicalization, intended for actual SIMD wasm + // values, would break our code. `bitcast`-ing between I64X2 and + // I16X8 is a noop, so this has no performance impact. + + // NOTE(frank-emrich) We currently only care about little endian + // platforms. The internal layout of the vector is reflected by + // this, it is identical to what happens if you do a 128bit vector + // load of a `Optional` on a little endian platform: Its + // 64 LSBs contain the revision counter, its 64MSBs contain the + // `VMContRef` pointer. + ir::types::I8X16 } /// Unless linearity checks disabled, turns a (possibly null reference to a) @@ -79,26 +71,21 @@ pub(crate) fn disassemble_contobj<'a>( builder: &mut FunctionBuilder, contobj: ir::Value, ) -> (ir::Value, ir::Value) { - if cfg!(feature = "unsafe_disable_continuation_linearity_check") { - let zero = builder.ins().iconst(cranelift_codegen::ir::types::I64, 0); - (zero, contobj) - } else { - debug_assert_eq!( - builder.func.dfg.value_type(contobj), - vm_contobj_type(env.pointer_type()) - ); - let flags = ir::MemFlags::new().with_endianness(ir::Endianness::Little); - let contobj = builder.ins().bitcast(ir::types::I64X2, flags, contobj); - let revision_counter = builder.ins().extractlane(contobj, 0); - let contref = builder.ins().extractlane(contobj, 1); - debug_assert_eq!(builder.func.dfg.value_type(contref), ir::types::I64); - debug_assert_eq!( - builder.func.dfg.value_type(revision_counter), - ir::types::I64 - ); - // TODO(frank-emrich) On 32bit platforms, need to ireduce contref to env.pointer_type() - (revision_counter, contref) - } + debug_assert_eq!( + builder.func.dfg.value_type(contobj), + vm_contobj_type(env.pointer_type()) + ); + let flags = ir::MemFlags::new().with_endianness(ir::Endianness::Little); + let contobj = builder.ins().bitcast(ir::types::I64X2, flags, contobj); + let revision_counter = builder.ins().extractlane(contobj, 0); + let contref = builder.ins().extractlane(contobj, 1); + debug_assert_eq!(builder.func.dfg.value_type(contref), ir::types::I64); + debug_assert_eq!( + builder.func.dfg.value_type(revision_counter), + ir::types::I64 + ); + // TODO(frank-emrich) On 32bit platforms, need to ireduce contref to env.pointer_type() + (revision_counter, contref) } /// Constructs a continuation object from a given contref and revision pointer. @@ -109,27 +96,23 @@ pub(crate) fn assemble_contobj<'a>( revision_counter: ir::Value, contref_addr: ir::Value, ) -> ir::Value { - if cfg!(feature = "unsafe_disable_continuation_linearity_check") { - contref_addr - } else { - // TODO(frank-emrich) This check assumes env.pointer_type() == I64 - debug_assert_eq!(builder.func.dfg.value_type(contref_addr), ir::types::I64); - debug_assert_eq!( - builder.func.dfg.value_type(revision_counter), - ir::types::I64 - ); - - let lower = builder - .ins() - .scalar_to_vector(ir::types::I64X2, revision_counter); - let contobj = builder.ins().insertlane(lower, contref_addr, 1); - - let flags = ir::MemFlags::new().with_endianness(ir::Endianness::Little); - let contobj = builder - .ins() - .bitcast(vm_contobj_type(env.pointer_type()), flags, contobj); - contobj - } + // TODO(frank-emrich) This check assumes env.pointer_type() == I64 + debug_assert_eq!(builder.func.dfg.value_type(contref_addr), ir::types::I64); + debug_assert_eq!( + builder.func.dfg.value_type(revision_counter), + ir::types::I64 + ); + + let lower = builder + .ins() + .scalar_to_vector(ir::types::I64X2, revision_counter); + let contobj = builder.ins().insertlane(lower, contref_addr, 1); + + let flags = ir::MemFlags::new().with_endianness(ir::Endianness::Little); + let contobj = builder + .ins() + .bitcast(vm_contobj_type(env.pointer_type()), flags, contobj); + contobj } /// TODO diff --git a/crates/environ/src/builtin.rs b/crates/environ/src/builtin.rs index 010b9b812b5b..989104ef6a89 100644 --- a/crates/environ/src/builtin.rs +++ b/crates/environ/src/builtin.rs @@ -162,22 +162,25 @@ macro_rules! foreach_builtin_function { // General-purpose printing functions. // - // Prints a string. Note that we transfer the string not as C strings, but as 'static str, - // represented as a pointer and a length. + // Prints a string. Note that we transfer the string not + // as C strings, but as 'static str, represented as a + // pointer and a length. tc_print_str(vmctx: vmctx, s: pointer, len : i64); // TODO tc_print_int(vmctx: vmctx, arg : i64); // TODO tc_print_pointer(vmctx: vmctx, arg : pointer); - // Returns an index for Wasm's `table.grow` instruction for `contobj`s. - // Note that the initial Option (i.e., the value to fill - // the new slots with) is split into two arguments: The underlying - // continuation reference and the revision count. - // If `unsafe_disable_continuation_linearity_check` is enabled, the revision value is arbitrary. - // To denote the continuation being `None`, `init_contref` may be 0. + // Returns an index for Wasm's `table.grow` instruction + // for `contobj`s. Note that the initial + // Option (i.e., the value to fill the new + // slots with) is split into two arguments: The underlying + // continuation reference and the revision count. To + // denote the continuation being `None`, `init_contref` + // may be 0. table_grow_cont_obj(vmctx: vmctx, table: i32, delta: i64, init_contref: pointer, init_revision : i64) -> pointer; - // `value_contref` and `value_revision` together encode the Option, as in previous libcall. + // `value_contref` and `value_revision` together encode + // the Option, as in previous libcall. table_fill_cont_obj(vmctx: vmctx, table: i32, dst: i64, value_contref: pointer, value_revision : i64, len: i64); } }; diff --git a/crates/wasmtime/Cargo.toml b/crates/wasmtime/Cargo.toml index 9bd10c1a6d66..9fbd272c7d71 100644 --- a/crates/wasmtime/Cargo.toml +++ b/crates/wasmtime/Cargo.toml @@ -280,12 +280,6 @@ call-hook = [] # with the pooling allocator on x64 to compact linear memory allocations. memory-protection-keys = ["pooling-allocator"] -# Enable unsafe mutable continuations (temporary hack to workaround -# the garbage generated by continuation objects). -unsafe_disable_continuation_linearity_check = [ - "wasmtime-cranelift?/unsafe_disable_continuation_linearity_check" -] - # Toggle the baseline implementation of WasmFX wasmfx_baseline = [ "async", diff --git a/crates/wasmtime/src/runtime/vm/continuation.rs b/crates/wasmtime/src/runtime/vm/continuation.rs index 585a4af140ab..1c5f3063e04d 100644 --- a/crates/wasmtime/src/runtime/vm/continuation.rs +++ b/crates/wasmtime/src/runtime/vm/continuation.rs @@ -13,10 +13,6 @@ cfg_if::cfg_if! { /// once. The linearity is checked dynamically in the generated code /// by comparing the revision witness embedded in the pointer to the /// actual revision counter on the continuation reference. -#[cfg_attr( - feature = "unsafe_disable_continuation_linearity_check", - allow(dead_code) -)] pub mod safe_vm_contobj { use super::imp::VMContRef; use core::ptr::NonNull; @@ -37,35 +33,7 @@ pub mod safe_vm_contobj { } } -/// This version of `VMContObj` does not actually store a revision counter. It is -/// used when we opt out of the linearity check using the -/// `unsafe_disable_continuation_linearity_check` feature -#[cfg_attr( - not(feature = "unsafe_disable_continuation_linearity_check"), - allow(dead_code) -)] -pub mod unsafe_vm_contobj { - use super::imp::VMContRef; - use core::ptr::NonNull; - - #[repr(transparent)] - #[derive(Debug, Clone, Copy)] - pub struct VMContObj(NonNull); - - impl VMContObj { - pub fn new(contref: NonNull, _revision: u64) -> Self { - Self(contref) - } - } -} - -cfg_if::cfg_if! { - if #[cfg(feature = "unsafe_disable_continuation_linearity_check")] { - pub use unsafe_vm_contobj::*; - } else { - pub use safe_vm_contobj::*; - } -} +pub use safe_vm_contobj::*; unsafe impl Send for VMContObj {} unsafe impl Sync for VMContObj {} diff --git a/tests/all/typed_continuations.rs b/tests/all/typed_continuations.rs index 38baef4ec239..9d3e94c6c726 100644 --- a/tests/all/typed_continuations.rs +++ b/tests/all/typed_continuations.rs @@ -1173,17 +1173,14 @@ mod misc { "#; let runner = Runner::new(); - cfg_if::cfg_if! { - if #[cfg(feature = "unsafe_disable_continuation_linearity_check")] { - let result = runner.run_test::<()>(wat, &[])?; - assert_eq!(result, ()) - } else { - let error = runner.run_test::<()>(wat, &[]) - .expect_err("expected an overflow"); - assert!(error.root_cause().is::()); - assert_eq!(*error.downcast_ref::().unwrap(), Trap::IntegerOverflow); - } - } + let error = runner + .run_test::<()>(wat, &[]) + .expect_err("expected an overflow"); + assert!(error.root_cause().is::()); + assert_eq!( + *error.downcast_ref::().unwrap(), + Trap::IntegerOverflow + ); Ok(()) } } diff --git a/tests/wast.rs b/tests/wast.rs index ff10fc126637..e09746680a38 100644 --- a/tests/wast.rs +++ b/tests/wast.rs @@ -263,13 +263,6 @@ fn should_fail(test: &Path, strategy: Strategy) -> bool { } if part == "typed-continuations" { - // This test specifically checks that we catch a continuation being - // resumed twice, which we cannot detect in this mode. - if cfg!(feature = "unsafe_disable_continuation_linearity_check") - && test.ends_with("cont_twice.wast") - { - return true; - } // Tag linking is broken in the baseline implementation. if cfg!(feature = "wasmfx_baseline") && test.ends_with("linking_tags.wast") { return true;