Fix an issue in adapter module partitioning (#4622)

When an adapter module depends on a particular core wasm instance this
means that it actually depends on not only that instance but all prior
core wasm instances as well. This is because core wasm instances must be
instantiated in the specified order within a component and that cannot
change depending on the dataflow between adapters. This commit fixes a
possible panic from linearizing the component dfg where an adapter
module tried to depend on an instance that hadn't been instantiated yet
because the ordering dependency between core wasm instances hadn't been
modeled.
This commit is contained in:
Alex Crichton
2022-08-04 20:32:39 -05:00
committed by GitHub
parent 412fa04911
commit 1ce9e8aa5f
3 changed files with 71 additions and 7 deletions

View File

@@ -352,6 +352,7 @@ enum RuntimeInstance {
impl LinearizeDfg<'_> { impl LinearizeDfg<'_> {
fn instantiate(&mut self, instance: InstanceId, args: &Instance) { fn instantiate(&mut self, instance: InstanceId, args: &Instance) {
log::trace!("creating instance {instance:?}");
let instantiation = match args { let instantiation = match args {
Instance::Static(index, args) => InstantiateModule::Static( Instance::Static(index, args) => InstantiateModule::Static(
*index, *index,
@@ -500,8 +501,10 @@ impl LinearizeDfg<'_> {
where where
T: Clone, T: Clone,
{ {
let instance = export.instance;
log::trace!("referencing export of {instance:?}");
info::CoreExport { info::CoreExport {
instance: self.runtime_instances[&RuntimeInstance::Normal(export.instance)], instance: self.runtime_instances[&RuntimeInstance::Normal(instance)],
item: export.item.clone(), item: export.item.clone(),
} }
} }

View File

@@ -296,7 +296,7 @@ impl PartitionAdapterModules {
// didn't depend on anything in that module itself or it will be added // didn't depend on anything in that module itself or it will be added
// to a fresh module if this adapter depended on something that the // to a fresh module if this adapter depended on something that the
// current adapter module created. // current adapter module created.
log::debug!("adding {id:?} to adapter module {adapter:#?}"); log::debug!("adding {id:?} to adapter module");
self.next_module.adapters.push(id); self.next_module.adapters.push(id);
} }
@@ -319,9 +319,12 @@ impl PartitionAdapterModules {
// If this adapter is already defined then we can safely depend // If this adapter is already defined then we can safely depend
// on it with no consequences. // on it with no consequences.
if self.defined_items.contains(&Def::Adapter(*id)) { if self.defined_items.contains(&Def::Adapter(*id)) {
log::debug!("using existing adapter {id:?} ");
return; return;
} }
log::debug!("splitting module needing {id:?} ");
// .. otherwise we found a case of an adapter depending on an // .. otherwise we found a case of an adapter depending on an
// adapter-module-in-progress meaning that the current adapter // adapter-module-in-progress meaning that the current adapter
// module must be completed and then a new one is started. // module must be completed and then a new one is started.
@@ -337,16 +340,33 @@ impl PartitionAdapterModules {
} }
fn core_export<T>(&mut self, dfg: &dfg::ComponentDfg, export: &dfg::CoreExport<T>) { fn core_export<T>(&mut self, dfg: &dfg::ComponentDfg, export: &dfg::CoreExport<T>) {
// If this instance has already been visited that means it can already // When an adapter depends on an exported item it actually depends on
// be defined for this adapter module, so nothing else needs to be done. // the instance of that exported item. The caveat here is that the
if !self.defined_items.insert(Def::Instance(export.instance)) { // adapter not only depends on that particular instance, but also all
return; // prior instances to that instance as well because instance
// instantiation order is fixed and cannot change.
//
// To model this the instance index space is looped over here and while
// an instance hasn't been visited it's visited. Note that if an
// instance has already been visited then all prior instances have
// already been visited so there's no need to continue.
let mut instance = export.instance;
while self.defined_items.insert(Def::Instance(instance)) {
self.instance(dfg, instance);
if instance.as_u32() == 0 {
break;
}
instance = dfg::InstanceId::from_u32(instance.as_u32() - 1);
} }
}
fn instance(&mut self, dfg: &dfg::ComponentDfg, instance: dfg::InstanceId) {
log::debug!("visiting instance {instance:?}");
// ... otherwise if this is the first timet he instance has been seen // ... otherwise if this is the first timet he instance has been seen
// then the instances own arguments are recursively visited to find // then the instances own arguments are recursively visited to find
// transitive dependencies on adapters. // transitive dependencies on adapters.
match &dfg.instances[export.instance] { match &dfg.instances[instance] {
dfg::Instance::Static(_, args) => { dfg::Instance::Static(_, args) => {
for arg in args.iter() { for arg in args.iter() {
self.core_def(dfg, arg); self.core_def(dfg, arg);

View File

@@ -1393,3 +1393,44 @@
) )
(instance (instantiate $c2 (with "" (instance $c1)))) (instance (instantiate $c2 (with "" (instance $c1))))
) )
;; Adapters are used slightly out-of-order here to stress the internals of
;; dependencies between adapters.
(component
(core module $m
(func (export "execute"))
(func (export "realloc") (param i32 i32 i32 i32) (result i32) unreachable)
(memory (export "memory") 1)
)
(component $root
(core instance $m (instantiate $m))
(func (export "execute")
(canon lift (core func $m "execute"))
)
)
(component $c
(import "backend" (instance $i
(export "execute" (func))
))
(core module $shim2 (import "" "0" (func)))
(core instance $m (instantiate $m))
;; This adapter, when fused with itself on the second instantiation of this
;; component, will dependend on the prior instance `$m` so it which means
;; that the adapter module containing this must be placed in the right
;; location.
(core func $execute
(canon lower (func $i "execute") (memory $m "memory") (realloc (func $m "realloc")))
)
(core instance (instantiate $shim2
(with "" (instance
(export "0" (func $execute))
))
))
(func (export "execute") (canon lift (core func $m "execute")))
)
(instance $root (instantiate $root))
(instance $c1 (instantiate $c (with "backend" (instance $root))))
(instance $c2 (instantiate $c (with "backend" (instance $c1))))
)