consistently create thread and task when entering component instance (#12379)

* consistently create thread and task when entering component instance

Previously, we weren't creating a new thread or task in all cases when entering
a component instance, even when component model async features were enabled.  In
particular, entering an instance via a sync-to-sync, guest-to-guest adapter, via
`Linker::instantiate[_async]`, or via `[Typed]Func::call` all skipped creating a
thread or task, creating panics and/or instance mismatches in certain cases.

This commit addresses all those cases and also adds assertions to all CM async
intrinsics to verify that the caller instance matches the most-recently-pushed
task.  Note that we still skip pushing and popping threads and tasks if no CM
async features are enabled in the `Config`.

In order to populate the `GuestTask::instance` field for tasks created as part
of `Linker::instantiate[_async]` calls, I had to add a
`RuntimeComponentInstanceIndex` field to `GlobalInitializer::InstantiateModule`
and friends so it would be available when needed.

While testing this, I uncovered and fixed a couple of related issues:

- We weren't checking the `may_leave` flag when guest-to-guest calling a resource destructor
- We weren't checking whether a subtask was ready to delete (e.g. that no threads were still running) before attempting to delete it while deleting its supertask

* fix warnings when component-model-async feature disabled

* address review feedback

* push a task when calling a resource dtor host-to-guest

* add tests which call `thread.index` from realloc and post-return functions

...and assert that the indexes match as expected.

Getting the post-return test to pass required moving the call to
`StoreOpaque::exit_sync_call` to after the post-return function is called.

Author: dicej

crates/cranelift/src/compiler/component.rs

@@ -1,6 +1,6 @@
 //! Compilation support for the component model.
 
-use crate::{TRAP_INTERNAL_ASSERT, compiler::Compiler};
+use crate::{TRAP_CANNOT_LEAVE_COMPONENT, TRAP_INTERNAL_ASSERT, compiler::Compiler};
 use cranelift_codegen::ir::condcodes::IntCC;
 use cranelift_codegen::ir::{self, InstBuilder, MemFlags, Value};
 use cranelift_codegen::isa::{CallConv, TargetIsa};
@@ -722,6 +722,22 @@ impl<'a> TrampolineCompiler<'a> {
                     |_, _| {},
                 );
             }
+            Trampoline::EnterSyncCall => {
+                self.translate_libcall(
+                    host::enter_sync_call,
+                    TrapSentinel::Falsy,
+                    WasmArgs::InRegisters,
+                    |_, _| {},
+                );
+            }
+            Trampoline::ExitSyncCall => {
+                self.translate_libcall(
+                    host::exit_sync_call,
+                    TrapSentinel::Falsy,
+                    WasmArgs::InRegisters,
+                    |_, _| {},
+                );
+            }
             Trampoline::ContextGet { instance, slot } => {
                 self.translate_libcall(
                     host::context_get,
@@ -1112,11 +1128,22 @@ impl<'a> TrampolineCompiler<'a> {
         //      brif should_run_destructor, run_destructor_block, return_block
         //
         //    run_destructor_block:
+        //      ;; test may_leave, but only if the component instances
+        //      ;; differ
+        //      flags = load.i32 vmctx+$instance_flags_offset
+        //      masked = band flags, $FLAG_MAY_LEAVE
+        //      trapz masked, $TRAP_CANNOT_LEAVE_COMPONENT
+        //
         //      ;; set may_block to false, saving the old value to restore
-        //      ;; later, but only if the component instances differ
+        //      ;; later, but only if the component instances differ and
+        //      ;; concurrency is enabled
         //      old_may_block = load.i32 vmctx+$may_block_offset
         //      store 0, vmctx+$may_block_offset
         //
+        //      ;; call enter_sync_call, but only if the component instances
+        //      ;; differ and concurrency is enabled
+        //      ...
+        //
         //      ;; ============================================================
         //      ;; this is conditionally emitted based on whether the resource
         //      ;; has a destructor or not, and can be statically omitted
@@ -1132,6 +1159,12 @@ impl<'a> TrampolineCompiler<'a> {
         //      ;; restore old value of may_block
         //      store old_may_block, vmctx+$may_block_offset
         //
+        //      ;; if needed, call exit_sync_call
+        //      ...
+        //
+        //      ;; if needed, restore the old value of may_block
+        //      store old_may_block, vmctx+$may_block_offset
+        //
         //      jump return_block
         //
         //    return_block:
@@ -1161,29 +1194,69 @@ impl<'a> TrampolineCompiler<'a> {
 
         self.builder.switch_to_block(run_destructor_block);
 
-        // If this is a defined resource within the component itself then the
-        // `may_block` field must be updated. Note though that this update can
-        // be elided if the resource table resides in the same component
-        // instance that defined the resource as the component is calling
-        // itself.
+        // If this is a component-defined resource, the `may_leave` flag must be
+        // checked.  Additionally, if concurrency is enabled, the `may_block`
+        // field must be updated and `enter_sync_call` called. Note though that
+        // all of that may be elided if the resource table resides in the same
+        // component instance that defined the resource as the component is
+        // calling itself.
         let old_may_block = if let Some(def) = resource_def {
             if self.types[resource].unwrap_concrete_instance() != def.instance {
-                // Stash the old value of `may_block` and then set it to false.
-                let old_may_block = self.builder.ins().load(
+                let flags = self.builder.ins().load(
                     ir::types::I32,
                     trusted,
                     vmctx,
-                    i32::try_from(self.offsets.task_may_block()).unwrap(),
-                );
-                let zero = self.builder.ins().iconst(ir::types::I32, i64::from(0));
-                self.builder.ins().store(
-                    ir::MemFlags::trusted(),
-                    zero,
-                    vmctx,
-                    i32::try_from(self.offsets.task_may_block()).unwrap(),
+                    i32::try_from(
+                        self.offsets
+                            .instance_flags(self.types[resource].unwrap_concrete_instance()),
+                    )
+                    .unwrap(),
                 );
+                let masked = self
+                    .builder
+                    .ins()
+                    .band_imm(flags, i64::from(FLAG_MAY_LEAVE));
+                self.builder
+                    .ins()
+                    .trapz(masked, TRAP_CANNOT_LEAVE_COMPONENT);
+
+                if self.compiler.tunables.component_model_concurrency {
+                    // Stash the old value of `may_block` and then set it to false.
+                    let old_may_block = self.builder.ins().load(
+                        ir::types::I32,
+                        trusted,
+                        vmctx,
+                        i32::try_from(self.offsets.task_may_block()).unwrap(),
+                    );
+                    let zero = self.builder.ins().iconst(ir::types::I32, i64::from(0));
+                    self.builder.ins().store(
+                        ir::MemFlags::trusted(),
+                        zero,
+                        vmctx,
+                        i32::try_from(self.offsets.task_may_block()).unwrap(),
+                    );
 
-                Some(old_may_block)
+                    // Call `enter_sync_call`
+                    //
+                    // FIXME: Apply the optimizations described in #12311.
+                    let host_args = vec![
+                        vmctx,
+                        self.builder
+                            .ins()
+                            .iconst(ir::types::I32, i64::from(instance.as_u32())),
+                        self.builder.ins().iconst(ir::types::I32, i64::from(0)),
+                        self.builder
+                            .ins()
+                            .iconst(ir::types::I32, i64::from(def.instance.as_u32())),
+                    ];
+                    let call = self.call_libcall(vmctx, host::enter_sync_call, &host_args);
+                    let result = self.builder.func.dfg.inst_results(call).get(0).copied();
+                    self.raise_if_host_trapped(result.unwrap());
+
+                    Some(old_may_block)
+                } else {
+                    None
+                }
             } else {
                 None
             }
@@ -1240,6 +1313,14 @@ impl<'a> TrampolineCompiler<'a> {
         }
 
         if let Some(old_may_block) = old_may_block {
+            // Call `exit_sync_call`
+            //
+            // FIXME: Apply the optimizations described in #12311.
+            let call = self.call_libcall(vmctx, host::exit_sync_call, &[vmctx]);
+            let result = self.builder.func.dfg.inst_results(call).get(0).copied();
+            self.raise_if_host_trapped(result.unwrap());
+
+            // Restore the old value of `may_block`
             self.builder.ins().store(
                 ir::MemFlags::trusted(),
                 old_may_block,

crates/cranelift/src/lib.rs

@@ -48,6 +48,8 @@ use self::compiler::Compiler;
 
 const TRAP_INTERNAL_ASSERT: TrapCode = TrapCode::unwrap_user(1);
 const TRAP_OFFSET: u8 = 2;
+pub const TRAP_CANNOT_LEAVE_COMPONENT: TrapCode =
+    TrapCode::unwrap_user(Trap::CannotLeaveComponent as u8 + TRAP_OFFSET);
 pub const TRAP_INDIRECT_CALL_TO_NULL: TrapCode =
     TrapCode::unwrap_user(Trap::IndirectCallToNull as u8 + TRAP_OFFSET);
 pub const TRAP_BAD_SIGNATURE: TrapCode =

crates/environ/src/component.rs

@@ -99,6 +99,11 @@ macro_rules! foreach_builtin_component_function {
             resource_enter_call(vmctx: vmctx);
             resource_exit_call(vmctx: vmctx) -> bool;
 
+            #[cfg(feature = "component-model-async")]
+            enter_sync_call(vmctx: vmctx, caller_instance: u32, callee_async: u32, callee_instance: u32) -> bool;
+            #[cfg(feature = "component-model-async")]
+            exit_sync_call(vmctx: vmctx) -> bool;
+
             #[cfg(feature = "component-model-async")]
             backpressure_modify(vmctx: vmctx, caller_instance: u32, increment: u8) -> bool;
             #[cfg(feature = "component-model-async")]

crates/environ/src/component/dfg.rs

@@ -160,7 +160,7 @@ pub enum SideEffect {
     /// as traps and the core wasm `start` function which may call component
     /// imports. Instantiation order from the original component must be done in
     /// the same order.
-    Instance(InstanceId),
+    Instance(InstanceId, RuntimeComponentInstanceIndex),
 
     /// A resource was declared in this component.
     ///
@@ -476,6 +476,8 @@ pub enum Trampoline {
     StreamTransfer,
     ErrorContextTransfer,
     Trap,
+    EnterSyncCall,
+    ExitSyncCall,
     ContextGet {
         instance: RuntimeComponentInstanceIndex,
         slot: u32,
@@ -721,16 +723,21 @@ enum RuntimeInstance {
 impl LinearizeDfg<'_> {
     fn side_effect(&mut self, effect: &SideEffect) {
         match effect {
-            SideEffect::Instance(i) => {
-                self.instantiate(*i, &self.dfg.instances[*i]);
+            SideEffect::Instance(i, ci) => {
+                self.instantiate(*i, &self.dfg.instances[*i], *ci);
             }
             SideEffect::Resource(i) => {
                 self.resource(*i, &self.dfg.resources[*i]);
             }
         }
     }
 
-    fn instantiate(&mut self, instance: InstanceId, args: &Instance) {
+    fn instantiate(
+        &mut self,
+        instance: InstanceId,
+        args: &Instance,
+        component_instance: RuntimeComponentInstanceIndex,
+    ) {
         log::trace!("creating instance {instance:?}");
         let instantiation = match args {
             Instance::Static(index, args) => InstantiateModule::Static(
@@ -751,8 +758,10 @@ impl LinearizeDfg<'_> {
             ),
         };
         let index = RuntimeInstanceIndex::new(self.runtime_instances.len());
-        self.initializers
-            .push(GlobalInitializer::InstantiateModule(instantiation));
+        self.initializers.push(GlobalInitializer::InstantiateModule(
+            instantiation,
+            Some(component_instance),
+        ));
         let prev = self
             .runtime_instances
             .insert(RuntimeInstance::Normal(instance), index);
@@ -1156,6 +1165,8 @@ impl LinearizeDfg<'_> {
             Trampoline::StreamTransfer => info::Trampoline::StreamTransfer,
             Trampoline::ErrorContextTransfer => info::Trampoline::ErrorContextTransfer,
             Trampoline::Trap => info::Trampoline::Trap,
+            Trampoline::EnterSyncCall => info::Trampoline::EnterSyncCall,
+            Trampoline::ExitSyncCall => info::Trampoline::ExitSyncCall,
             Trampoline::ContextGet { instance, slot } => info::Trampoline::ContextGet {
                 instance: *instance,
                 slot: *slot,
@@ -1242,7 +1253,7 @@ impl LinearizeDfg<'_> {
                 let (module_index, args) = &me.dfg.adapter_modules[adapter_module];
                 let args = args.iter().map(|arg| me.core_def(arg)).collect();
                 let instantiate = InstantiateModule::Static(*module_index, args);
-                GlobalInitializer::InstantiateModule(instantiate)
+                GlobalInitializer::InstantiateModule(instantiate, None)
             },
             |_, init| init,
         )

crates/environ/src/component/info.rs

@@ -243,7 +243,10 @@ pub enum GlobalInitializer {
     /// involve running the `start` function of the instance as well if it's
     /// specified. This largely delegates to the same standard instantiation
     /// process as the rest of the core wasm machinery already uses.
-    InstantiateModule(InstantiateModule),
+    ///
+    /// The second field represents the component instance to which the module
+    /// belongs, if applicable.  This will be `None` for adapter modules.
+    InstantiateModule(InstantiateModule, Option<RuntimeComponentInstanceIndex>),
 
     /// A host function is being lowered, creating a core wasm function.
     ///
@@ -1109,6 +1112,13 @@ pub enum Trampoline {
     /// code.
     Trap,
 
+    /// An intrinsic used by FACT-generated modules to push a task onto the
+    /// stack for a sync-to-sync, guest-to-guest call.
+    EnterSyncCall,
+    /// An intrinsic used by FACT-generated modules to pop the task previously
+    /// pushed by `EnterSyncCall`.
+    ExitSyncCall,
+
     /// Intrinsic used to implement the `context.get` component model builtin.
     ///
     /// The payload here represents that this is accessing the Nth slot of local
@@ -1238,6 +1248,8 @@ impl Trampoline {
             StreamTransfer => format!("stream-transfer"),
             ErrorContextTransfer => format!("error-context-transfer"),
             Trap => format!("trap"),
+            EnterSyncCall => format!("enter-sync-call"),
+            ExitSyncCall => format!("exit-sync-call"),
             ContextGet { .. } => format!("context-get"),
             ContextSet { .. } => format!("context-set"),
             ThreadIndex => format!("thread-index"),

crates/environ/src/component/translate.rs

@@ -551,7 +551,7 @@ impl<'a, 'data> Translator<'a, 'data> {
             PrimaryMap::<RuntimeInstanceIndex, PackedOption<StaticModuleIndex>>::new();
         for init in &translation.component.initializers {
             match init {
-                GlobalInitializer::InstantiateModule(instantiation) => match instantiation {
+                GlobalInitializer::InstantiateModule(instantiation, _) => match instantiation {
                     InstantiateModule::Static(module, args) => {
                         instantiations[*module].join(AbstractInstantiations::One(&*args));
                         instance_to_module.push(Some(*module).into());

crates/environ/src/component/translate/adapt.rs

@@ -208,8 +208,7 @@ impl<'data> Translator<'_, 'data> {
         // the module using standard core wasm translation, and then fills out
         // the dfg metadata for each adapter.
         for (module_id, adapter_module) in state.adapter_modules.iter() {
-            let mut module =
-                fact::Module::new(self.types.types(), self.tunables.debug_adapter_modules);
+            let mut module = fact::Module::new(self.types.types(), self.tunables);
             let mut names = Vec::with_capacity(adapter_module.adapters.len());
             for adapter in adapter_module.adapters.iter() {
                 let name = format!("adapter{}", adapter.as_u32());
@@ -349,6 +348,8 @@ fn fact_import_to_core_def(
             simple_intrinsic(dfg::Trampoline::ErrorContextTransfer)
         }
         fact::Import::Trap => simple_intrinsic(dfg::Trampoline::Trap),
+        fact::Import::EnterSyncCall => simple_intrinsic(dfg::Trampoline::EnterSyncCall),
+        fact::Import::ExitSyncCall => simple_intrinsic(dfg::Trampoline::ExitSyncCall),
     }
 }
 

crates/environ/src/component/translate/inline.rs

@@ -1216,7 +1216,7 @@ impl<'a> Inliner<'a> {
 
                 self.result
                     .side_effects
-                    .push(dfg::SideEffect::Instance(instance));
+                    .push(dfg::SideEffect::Instance(instance, frame.instance));
 
                 frame
                     .module_instances