From 666c2554ea0e1728c35aa41178cf235920db888a Mon Sep 17 00:00:00 2001 From: Chris Fallin Date: Thu, 31 Mar 2022 14:26:01 -0700 Subject: [PATCH] Merge pull request from GHSA-gwc9-348x-qwv2 * Run the GC smoketest with epoch support enabled as well. * Handle safepoints in cold blocks properly. Currently, the way that we find safepoint slots for a given instruction relies on the instruction index order in the safepoint list matching the order of instruction emission. Previous to the introduction of cold-block support, this was trivially satisfied by sorting the safepoint list: we emit instructions 0, 1, 2, 3, 4, ..., and so if we have safepoints at instructions 1 and 4, we will encounter them in that order. However, cold blocks are supported by swizzling the emission order at the last moment (to avoid having to renumber instructions partway through the compilation pipeline), so we actually emit instructions out of index order when cold blocks are present. Reference-type support in Wasm in particular uses cold blocks for slowpaths, and has live refs and safepoints in these slowpaths, so we can reliably "skip" a safepoint (not emit any metadata for it) in the presence of reftype usage. This PR fixes the emission code by building a map from instruction index to safepoint index first, then doing lookups through this map, rather than following along in-order as it emits instructions. --- cranelift/codegen/src/machinst/vcode.rs | 31 +++++++++++++++---------- tests/all/funcref.rs | 5 +++- tests/all/gc.rs | 16 ++++++++++++- tests/all/main.rs | 9 ++++++- 4 files changed, 46 insertions(+), 15 deletions(-) diff --git a/cranelift/codegen/src/machinst/vcode.rs b/cranelift/codegen/src/machinst/vcode.rs index e31c1d8ec1..d1e585c609 100644 --- a/cranelift/codegen/src/machinst/vcode.rs +++ b/cranelift/codegen/src/machinst/vcode.rs @@ -17,6 +17,7 @@ //! See the main module comment in `mod.rs` for more details on the VCode-based //! backend pipeline. +use crate::fx::FxHashMap; use crate::ir::{self, types, Constant, ConstantData, SourceLoc}; use crate::machinst::*; use crate::settings; @@ -478,6 +479,19 @@ impl VCode { let mut inst_end_offsets = vec![0; self.insts.len()]; let mut label_inst_indices = vec![0; self.num_blocks()]; + // Map from instruction index to index in + // `safepoint_slots`. We need this because we emit + // instructions out-of-order, while the safepoint_insns / + // safepoint_slots data structures are sorted in instruction + // order. + let mut safepoint_indices: FxHashMap = FxHashMap::default(); + for (safepoint_idx, iix) in self.safepoint_insns.iter().enumerate() { + // Disregard safepoints that ended up having no live refs. + if self.safepoint_slots[safepoint_idx].len() > 0 { + safepoint_indices.insert(*iix, safepoint_idx); + } + } + // Construct the final order we emit code in: cold blocks at the end. let mut final_order: SmallVec<[BlockIndex; 16]> = smallvec![]; let mut cold_blocks: SmallVec<[BlockIndex; 16]> = smallvec![]; @@ -493,7 +507,6 @@ impl VCode { final_order.extend(cold_blocks.clone()); // Emit blocks. - let mut safepoint_idx = 0; let mut cur_srcloc = None; let mut last_offset = None; let mut start_of_cold_code = None; @@ -541,17 +554,11 @@ impl VCode { } state.pre_sourceloc(cur_srcloc.unwrap_or(SourceLoc::default())); - if safepoint_idx < self.safepoint_insns.len() - && self.safepoint_insns[safepoint_idx] == iix - { - if self.safepoint_slots[safepoint_idx].len() > 0 { - let stack_map = self.abi.spillslots_to_stack_map( - &self.safepoint_slots[safepoint_idx][..], - &state, - ); - state.pre_safepoint(stack_map); - } - safepoint_idx += 1; + if let Some(safepoint_idx) = safepoint_indices.get(&iix) { + let stack_map = self + .abi + .spillslots_to_stack_map(&self.safepoint_slots[*safepoint_idx][..], &state); + state.pre_safepoint(stack_map); } self.insts[iix as usize].emit(&mut buffer, &self.emit_info, &mut state); diff --git a/tests/all/funcref.rs b/tests/all/funcref.rs index 73dd21319d..74980bb21c 100644 --- a/tests/all/funcref.rs +++ b/tests/all/funcref.rs @@ -6,6 +6,7 @@ use wasmtime::*; #[test] fn pass_funcref_in_and_out_of_wasm() -> anyhow::Result<()> { let (mut store, module) = ref_types_module( + false, r#" (module (func (export "func") (param funcref) (result funcref) @@ -60,7 +61,8 @@ fn pass_funcref_in_and_out_of_wasm() -> anyhow::Result<()> { // Passing in a `funcref` from another store fails. { - let (mut other_store, other_module) = ref_types_module(r#"(module (func (export "f")))"#)?; + let (mut other_store, other_module) = + ref_types_module(false, r#"(module (func (export "f")))"#)?; let other_store_instance = Instance::new(&mut other_store, &other_module, &[])?; let f = other_store_instance .get_func(&mut other_store, "f") @@ -77,6 +79,7 @@ fn pass_funcref_in_and_out_of_wasm() -> anyhow::Result<()> { #[test] fn receive_null_funcref_from_wasm() -> anyhow::Result<()> { let (mut store, module) = ref_types_module( + false, r#" (module (func (export "get-null") (result funcref) diff --git a/tests/all/gc.rs b/tests/all/gc.rs index 6de09f1c53..4730f79418 100644 --- a/tests/all/gc.rs +++ b/tests/all/gc.rs @@ -14,7 +14,17 @@ impl Drop for SetFlagOnDrop { #[test] fn smoke_test_gc() -> anyhow::Result<()> { + smoke_test_gc_impl(false) +} + +#[test] +fn smoke_test_gc_epochs() -> anyhow::Result<()> { + smoke_test_gc_impl(true) +} + +fn smoke_test_gc_impl(use_epochs: bool) -> anyhow::Result<()> { let (mut store, module) = ref_types_module( + use_epochs, r#" (module (import "" "" (func $do_gc)) @@ -69,6 +79,7 @@ fn smoke_test_gc() -> anyhow::Result<()> { #[test] fn wasm_dropping_refs() -> anyhow::Result<()> { let (mut store, module) = ref_types_module( + false, r#" (module (func (export "drop_ref") (param externref) @@ -145,7 +156,7 @@ fn many_live_refs() -> anyhow::Result<()> { ", ); - let (mut store, module) = ref_types_module(&wat)?; + let (mut store, module) = ref_types_module(false, &wat)?; let live_refs = Arc::new(AtomicUsize::new(0)); @@ -191,6 +202,7 @@ fn many_live_refs() -> anyhow::Result<()> { #[test] fn drop_externref_via_table_set() -> anyhow::Result<()> { let (mut store, module) = ref_types_module( + false, r#" (module (table $t 1 externref) @@ -400,6 +412,7 @@ fn gee_i_sure_hope_refcounting_is_atomic() -> anyhow::Result<()> { #[test] fn global_init_no_leak() -> anyhow::Result<()> { let (mut store, module) = ref_types_module( + false, r#" (module (import "" "" (global externref)) @@ -424,6 +437,7 @@ fn global_init_no_leak() -> anyhow::Result<()> { #[test] fn no_gc_middle_of_args() -> anyhow::Result<()> { let (mut store, module) = ref_types_module( + false, r#" (module (import "" "return_some" (func $return (result externref externref externref))) diff --git a/tests/all/main.rs b/tests/all/main.rs index 461bf8d84f..a04ced711e 100644 --- a/tests/all/main.rs +++ b/tests/all/main.rs @@ -33,6 +33,7 @@ mod wast; /// A helper to compile a module in a new store with reference types enabled. pub(crate) fn ref_types_module( + use_epochs: bool, source: &str, ) -> anyhow::Result<(wasmtime::Store<()>, wasmtime::Module)> { use wasmtime::*; @@ -41,9 +42,15 @@ pub(crate) fn ref_types_module( let mut config = Config::new(); config.wasm_reference_types(true); + if use_epochs { + config.epoch_interruption(true); + } let engine = Engine::new(&config)?; - let store = Store::new(&engine, ()); + let mut store = Store::new(&engine, ()); + if use_epochs { + store.set_epoch_deadline(1); + } let module = Module::new(&engine, source)?;