Remove feature 'unsafe_disable_continuation_linearity_check'. (#231)

Resolves #230.

.github/workflows/main.yml

@@ -740,23 +740,6 @@ jobs:
  env:
  RUST_BACKTRACE: 1
 
- # Crude check for whether
- # `unsafe_disable_continuation_linearity_check` makes the test
- # `cont_twice` fail.
- # We run it with continuation pooling enabled, so that the involved
- # `VMContRef` should still be allocated, meaning that we can reliably detect
- # that the test is resuming an already returned fiber.
- # We expect the test case to trigger an internal assertion failure (rather
- # than a proper Wasm trap), which turns into a SIGILL (signal 4, exit code
- # 132).
- - run: |
- # By default, Github actions run commands with pipefail enabled. We don't want that, as the command below is supposed to fail:
- set +e
- # Let's keep the build step separate from the run step below, because the former should not fail:
- cargo build --features=unsafe_disable_continuation_linearity_check,wasmfx_pooling_allocator
- # We want this to fail and are happy with any non-zero exit code:
- cargo run --features=unsafe_disable_continuation_linearity_check,wasmfx_pooling_allocator -- wast -W=exceptions,function-references,typed-continuations tests/misc_testsuite/typed-continuations/cont_twice.wast ; test $? -eq 132
-
  # NB: the test job here is explicitly lacking in cancellation of this run if
  # something goes wrong. These take the longest anyway and otherwise if
  # Windows fails GitHub Actions will confusingly mark the failed Windows job

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",

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"

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"]
 

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 = []
 

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>(

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);

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<VMContObj>` 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<VMContObj>` 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<VMContObj>` 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