From 6e3e9d2daea73652d3aa9c3ede1da3de8d89ee1a Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 1 Aug 2019 11:56:57 -0700 Subject: [PATCH 1/4] Correctly hook up the anyref table initialization This functionality got lost in recent refactorings for WebIDL bindings unfortunately, so this commit touches things up to ensure that the anyref table initialization in anyref-mode is hooked up correctly, even when tests are enabled. This invovled moving injection of the start function to the webidl processing pass and ensuring its intrinsic is registered in the internal maps of wasm-bindgen. --- crates/anyref-xform/src/lib.rs | 31 ------------- crates/cli-support/src/intrinsic.rs | 2 +- crates/cli-support/src/js/mod.rs | 10 ++--- crates/cli-support/src/lib.rs | 8 +--- crates/cli-support/src/webidl/mod.rs | 66 +++++++++++++++++++++++----- 5 files changed, 63 insertions(+), 54 deletions(-) diff --git a/crates/anyref-xform/src/lib.rs b/crates/anyref-xform/src/lib.rs index 4b7a5dc1..747deba6 100644 --- a/crates/anyref-xform/src/lib.rs +++ b/crates/anyref-xform/src/lib.rs @@ -254,10 +254,6 @@ impl Transform<'_> { // functions and make sure everything is still hooked up right. self.rewrite_calls(module); - // Inject initialization routine to set up default slots in the table - // (things like null/undefined/true/false) - self.inject_initialization(module); - Ok(()) } @@ -669,31 +665,4 @@ impl Transform<'_> { } } } - - // Ensure that the `start` function for this module calls the - // `__wbindgen_init_anyref_table` function. This'll ensure that all - // instances of this module have the initial slots of the anyref table - // initialized correctly. - fn inject_initialization(&mut self, module: &mut Module) { - let ty = module.types.add(&[], &[]); - let (import, _) = module.add_import_func( - "__wbindgen_placeholder__", - "__wbindgen_init_anyref_table", - ty, - ); - - let prev_start = match module.start { - Some(f) => f, - None => { - module.start = Some(import); - return; - } - }; - - let mut builder = walrus::FunctionBuilder::new(); - let call_init = builder.call(import, Box::new([])); - let call_prev = builder.call(prev_start, Box::new([])); - let new_start = builder.finish(ty, Vec::new(), vec![call_init, call_prev], module); - module.start = Some(new_start); - } } diff --git a/crates/cli-support/src/intrinsic.rs b/crates/cli-support/src/intrinsic.rs index 33d74e86..c00ac3b8 100644 --- a/crates/cli-support/src/intrinsic.rs +++ b/crates/cli-support/src/intrinsic.rs @@ -145,7 +145,7 @@ intrinsics! { #[symbol = "__wbindgen_anyref_heap_live_count"] #[signature = fn() -> I32] AnyrefHeapLiveCount, - #[symbol = "__wbindgen_init_nyref_table"] + #[symbol = "__wbindgen_init_anyref_table"] #[signature = fn() -> Unit] InitAnyrefTable, } diff --git a/crates/cli-support/src/js/mod.rs b/crates/cli-support/src/js/mod.rs index 5e7dd0be..dcd9e997 100644 --- a/crates/cli-support/src/js/mod.rs +++ b/crates/cli-support/src/js/mod.rs @@ -194,10 +194,7 @@ impl<'a> Context<'a> { // up we always remove the `start` function if one is present. The JS // bindings glue then manually calls the start function (if it was // previously present). - let mut needs_manual_start = false; - if self.config.emit_start { - needs_manual_start = self.unstart_start_function(); - } + let needs_manual_start = self.unstart_start_function(); // After all we've done, especially // `unexport_unused_internal_exports()`, we probably have a bunch of @@ -517,7 +514,10 @@ impl<'a> Context<'a> { for (i, extra) in extra_modules.iter().enumerate() { let imports = match &mut imports { Some(list) => list, - None => bail!("cannot import from modules (`{}`) with `--no-modules`", extra), + None => bail!( + "cannot import from modules (`{}`) with `--no-modules`", + extra + ), }; imports.push_str(&format!("import * as __wbg_star{} from '{}';\n", i, extra)); imports_init.push_str(&format!("imports['{}'] = __wbg_star{};\n", extra, i)); diff --git a/crates/cli-support/src/lib.rs b/crates/cli-support/src/lib.rs index 0f332f10..2d940519 100755 --- a/crates/cli-support/src/lib.rs +++ b/crates/cli-support/src/lib.rs @@ -313,7 +313,7 @@ impl Bindgen { // the webidl bindings proposal) as well as an auxiliary section for all // sorts of miscellaneous information and features #[wasm_bindgen] // supports that aren't covered by WebIDL bindings. - webidl::process(&mut module)?; + webidl::process(&mut module, self.anyref, self.emit_start)?; // Now that we've got type information from the webidl processing pass, // touch up the output of rustc to insert anyref shims where necessary. @@ -324,12 +324,6 @@ impl Bindgen { anyref::process(&mut module)?; } - // If we're in a testing mode then remove the start function since we - // shouldn't execute it. - if !self.emit_start { - module.start = None; - } - let aux = module .customs .delete_typed::() diff --git a/crates/cli-support/src/webidl/mod.rs b/crates/cli-support/src/webidl/mod.rs index d0dcc00e..a28d07d2 100644 --- a/crates/cli-support/src/webidl/mod.rs +++ b/crates/cli-support/src/webidl/mod.rs @@ -473,10 +473,14 @@ struct Context<'a> { vendor_prefixes: HashMap>, unique_crate_identifier: &'a str, descriptors: HashMap, + anyref_enabled: bool, + support_start: bool, } pub fn process( module: &mut Module, + anyref_enabled: bool, + support_start: bool, ) -> Result<(NonstandardWebidlSectionId, WasmBindgenAuxId), Error> { let mut storage = Vec::new(); let programs = extract_programs(module, &mut storage)?; @@ -491,6 +495,8 @@ pub fn process( unique_crate_identifier: "", module, start_found: false, + anyref_enabled, + support_start, }; cx.init()?; @@ -532,18 +538,11 @@ impl<'a> Context<'a> { } } for (id, intrinsic) in intrinsics { - bindings::register_import( - self.module, - &mut self.bindings, - id, - intrinsic.binding(), - ast::WebidlFunctionKind::Static, - )?; - self.aux - .import_map - .insert(id, AuxImport::Intrinsic(intrinsic)); + self.bind_intrinsic(id, intrinsic)?; } + self.inject_anyref_initialization()?; + if let Some(custom) = self .module .customs @@ -612,6 +611,48 @@ impl<'a> Context<'a> { Ok(()) } + // Ensure that the `start` function for this module calls the + // `__wbindgen_init_anyref_table` function. This'll ensure that all + // instances of this module have the initial slots of the anyref table + // initialized correctly. + fn inject_anyref_initialization(&mut self) -> Result<(), Error> { + if !self.anyref_enabled { + return Ok(()); + } + let ty = self.module.types.add(&[], &[]); + let (import, import_id) = self.module.add_import_func( + PLACEHOLDER_MODULE, + "__wbindgen_init_anyref_table", + ty, + ); + + self.module.start = Some(match self.module.start { + Some(prev_start) => { + let mut builder = walrus::FunctionBuilder::new(); + let call_init = builder.call(import, Box::new([])); + let call_prev = builder.call(prev_start, Box::new([])); + builder.finish(ty, Vec::new(), vec![call_init, call_prev], self.module) + } + None => import, + }); + self.bind_intrinsic(import_id, Intrinsic::InitAnyrefTable)?; + Ok(()) + } + + fn bind_intrinsic(&mut self, id: ImportId, intrinsic: Intrinsic) -> Result<(), Error> { + bindings::register_import( + self.module, + &mut self.bindings, + id, + intrinsic.binding(), + ast::WebidlFunctionKind::Static, + )?; + self.aux + .import_map + .insert(id, AuxImport::Intrinsic(intrinsic)); + Ok(()) + } + fn program(&mut self, program: decode::Program<'a>) -> Result<(), Error> { self.unique_crate_identifier = program.unique_crate_identifier; let decode::Program { @@ -752,6 +793,11 @@ impl<'a> Context<'a> { } self.start_found = true; + // Skip this when we're generating tests + if !self.support_start { + return Ok(()); + } + let prev_start = match self.module.start { Some(f) => f, None => { From 5aee2f9c6a2e120f0ddfa6e3501ac180800422f3 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 1 Aug 2019 11:58:23 -0700 Subject: [PATCH 2/4] Fix an off-by-one in anyref stack manipulation With more than two anyref stack arguments we were accidentally storing the anyref values one higher in the stack than intended, so fix this off-by-one by switching up some addition logic. --- crates/anyref-xform/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/anyref-xform/src/lib.rs b/crates/anyref-xform/src/lib.rs index 747deba6..886002a9 100644 --- a/crates/anyref-xform/src/lib.rs +++ b/crates/anyref-xform/src/lib.rs @@ -510,14 +510,14 @@ impl Transform<'_> { // Store an anyref at an offset from our function's stack // pointer frame. let get_fp = builder.local_get(fp); - next_stack_offset += 1; - let (index, idx_local) = if next_stack_offset == 1 { + let (index, idx_local) = if next_stack_offset == 0 { (get_fp, fp) } else { let rhs = builder.i32_const(next_stack_offset); let add = builder.binop(BinaryOp::I32Add, get_fp, rhs); (builder.local_tee(scratch_i32, add), scratch_i32) }; + next_stack_offset += 1; let store = builder.table_set(self.table, index, local); let get = builder.local_get(idx_local); builder.with_side_effects(vec![store], get, Vec::new()) From 6cc7e3dadf22a69548918292d5a917f063ffce06 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 1 Aug 2019 11:59:05 -0700 Subject: [PATCH 3/4] Ensure the 0th slot of anyref table is `undefined` This is currently required by our ABI for wasm-bindgen where `None` js values going out have an index of 0 and are intended to be `undefined`. This also refactors initialization a bit to be slightly more generic over the constants we already have defined in this module. --- crates/cli-support/src/js/mod.rs | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/crates/cli-support/src/js/mod.rs b/crates/cli-support/src/js/mod.rs index dcd9e997..863285fc 100644 --- a/crates/cli-support/src/js/mod.rs +++ b/crates/cli-support/src/js/mod.rs @@ -2641,16 +2641,23 @@ impl<'a> Context<'a> { Intrinsic::InitAnyrefTable => { self.expose_anyref_table(); - String::from( + + // Grow the table to insert our initial values, and then also + // set the 0th slot to `undefined` since that's what we've + // historically used for our ABI which is that the index of 0 + // returns `undefined` for types like `None` going out. + let mut base = format!( " const table = wasm.__wbg_anyref_table; - const offset = table.grow(4); - table.set(offset + 0, undefined); - table.set(offset + 1, null); - table.set(offset + 2, true); - table.set(offset + 3, false); + const offset = table.grow({}); + table.set(0, undefined); ", - ) + INITIAL_HEAP_VALUES.len(), + ); + for (i, value) in INITIAL_HEAP_VALUES.iter().enumerate() { + base.push_str(&format!("table.set(offset + {}, {});\n", i, value)); + } + base } }; Ok(expr) From adde6c2da7b1e45f7a2de71c151d690b9c52ebf9 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 1 Aug 2019 12:00:33 -0700 Subject: [PATCH 4/4] Correct base calculation in anyref allocator This `base` value is the raw value coming out of the first call to `table.grow`. Throwing in the addition of `JSIDX_RESERVED` just wasn't right! --- src/anyref.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/anyref.rs b/src/anyref.rs index 47d8502a..7edff7db 100644 --- a/src/anyref.rs +++ b/src/anyref.rs @@ -39,7 +39,7 @@ impl Slab { internal_error("table grow failure") } if self.base == 0 { - self.base = r as usize + (super::JSIDX_RESERVED as usize); + self.base = r as usize; } else if self.base + self.data.len() != r as usize { internal_error("someone else allocated table entires?") }