From 60f3b1dad39594fcda47248c1ca9b3eddd2f73d3 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 23 Jul 2020 13:21:04 -0500 Subject: [PATCH] Pass actual stack pointers around instead of address 8 (#2249) * Pass actual stack pointers around instead of address 8 This commit updates the implementation of passing return pointers from JS to wasm to actually modify the wasm's shadow stack pointer instead of manufacturing the arbitrary address of 8. The purpose of this is to correctly handle threaded scenarios where each thread will write to its own area instead of everyone trying to compete at address 8. The implementation here will lazily, if necessary, export the stack pointer we find the module and modify it as necessary. Closes #2218 * Fix benchmarks build --- azure-pipelines.yml | 9 +++++---- crates/cli-support/src/js/binding.rs | 24 +++++++++++++++--------- crates/cli-support/src/multivalue.rs | 2 +- crates/cli-support/src/wit/mod.rs | 17 ++++++++++++++++- crates/cli-support/src/wit/section.rs | 2 +- crates/cli-support/src/wit/standard.rs | 7 +++++-- crates/cli/Cargo.toml | 1 + 7 files changed, 44 insertions(+), 18 deletions(-) diff --git a/azure-pipelines.yml b/azure-pipelines.yml index a9015e582a4..73d9396962b 100644 --- a/azure-pipelines.yml +++ b/azure-pipelines.yml @@ -211,11 +211,12 @@ jobs: displayName: "Build benchmarks" steps: - template: ci/azure-install-rust.yml - - template: ci/azure-install-wasm-pack.yml - - script: wasm-pack build --target web benchmarks + - script: rustup target add wasm32-unknown-unknown + displayName: "add target" + - script: cargo build --manifest-path benchmarks/Cargo.toml --release --target wasm32-unknown-unknown displayName: "build benchmarks" - - script: rm -f benchmarks/pkg/.gitignore - displayName: "remove stray gitignore" + - script: cargo run -p wasm-bindgen-cli target/wasm32-unknown-unknown/release/wasm_bindgen_benchmark.wasm --out-dir benchmarks/pkg --target web + displayName: "run wasm-bindgen" - task: PublishPipelineArtifact@0 inputs: artifactName: benchmarks diff --git a/crates/cli-support/src/js/binding.rs b/crates/cli-support/src/js/binding.rs index 8a86bd1282b..572a8359542 100644 --- a/crates/cli-support/src/js/binding.rs +++ b/crates/cli-support/src/js/binding.rs @@ -463,13 +463,6 @@ impl<'a, 'b> JsBuilder<'a, 'b> { } fn instruction(js: &mut JsBuilder, instr: &Instruction, log_error: &mut bool) -> Result<(), Error> { - // Here first properly aligned nonzero address is chosen to be the - // out-pointer. We use the address for a BigInt64Array sometimes which - // means it needs to be 8-byte aligned. Otherwise valid code is - // unlikely to ever be working around address 8, so this should be a - // safe address to use for returning data through. - let retptr_val = 8; - match instr { Instruction::Standard(wit_walrus::Instruction::ArgGet(n)) => { let arg = js.arg(*n).to_string(); @@ -566,7 +559,20 @@ fn instruction(js: &mut JsBuilder, instr: &Instruction, log_error: &mut bool) -> js.string_to_memory(*mem, *malloc, *realloc)?; } - Instruction::Retptr => js.stack.push(retptr_val.to_string()), + Instruction::Retptr { size } => { + let sp = match js.cx.aux.shadow_stack_pointer { + Some(s) => js.cx.export_name_of(s), + // In theory this shouldn't happen since malloc is included in + // most wasm binaries (and may be gc'd out) and that almost + // always pulls in a stack pointer. We can try to synthesize + // something here later if necessary. + None => bail!("failed to find shadow stack pointer"), + }; + js.prelude(&format!("const retptr = wasm.{}.value - {};", sp, size)); + js.prelude(&format!("wasm.{}.value = retptr;", sp)); + js.finally(&format!("wasm.{}.value += {};", sp, size)); + js.stack.push("retptr".to_string()); + } Instruction::StoreRetptr { ty, offset, mem } => { let (mem, size) = match ty { @@ -599,7 +605,7 @@ fn instruction(js: &mut JsBuilder, instr: &Instruction, log_error: &mut bool) -> // If we're loading from the return pointer then we must have pushed // it earlier, and we always push the same value, so load that value // here - let expr = format!("{}()[{} / {} + {}]", mem, retptr_val, size, offset); + let expr = format!("{}()[retptr / {} + {}]", mem, size, offset); js.prelude(&format!("var r{} = {};", offset, expr)); js.push(format!("r{}", offset)); } diff --git a/crates/cli-support/src/multivalue.rs b/crates/cli-support/src/multivalue.rs index 43e5206d379..601b2bf5a51 100644 --- a/crates/cli-support/src/multivalue.rs +++ b/crates/cli-support/src/multivalue.rs @@ -64,7 +64,7 @@ fn extract_xform<'a>( // If the first instruction is a `Retptr`, then this must be an exported // adapter which calls a wasm-defined function. Something we'd like to // adapt to multi-value! - if let Some(Instruction::Retptr) = instructions.first().map(|e| &e.instr) { + if let Some(Instruction::Retptr { .. }) = instructions.first().map(|e| &e.instr) { instructions.remove(0); let mut types = Vec::new(); instructions.retain(|instruction| match &instruction.instr { diff --git a/crates/cli-support/src/wit/mod.rs b/crates/cli-support/src/wit/mod.rs index 99b11a5760b..d9ad7dcea0b 100644 --- a/crates/cli-support/src/wit/mod.rs +++ b/crates/cli-support/src/wit/mod.rs @@ -1292,8 +1292,23 @@ impl<'a> Context<'a> { // everything into the outgoing arguments. let mut instructions = Vec::new(); if uses_retptr { + let size = ret.input.iter().fold(0, |sum, ty| { + let size = match ty { + AdapterType::I32 => 4, + AdapterType::I64 => 8, + AdapterType::F32 => 4, + AdapterType::F64 => 8, + _ => panic!("unsupported type in retptr {:?}", ty), + }; + let sum_rounded_up = (sum + (size - 1)) & (!(size - 1)); + sum_rounded_up + size + }); + // Round the number of bytes up to a 16-byte alignment to ensure the + // stack pointer is always 16-byte aligned (which LLVM currently + // requires). + let size = (size + 15) & (!15); instructions.push(InstructionData { - instr: Instruction::Retptr, + instr: Instruction::Retptr { size }, stack_change: StackChange::Modified { pushed: 1, popped: 0, diff --git a/crates/cli-support/src/wit/section.rs b/crates/cli-support/src/wit/section.rs index 88d9892a3db..55042f12d98 100644 --- a/crates/cli-support/src/wit/section.rs +++ b/crates/cli-support/src/wit/section.rs @@ -234,7 +234,7 @@ fn translate_instruction( mem: *mem, malloc: *malloc, }), - StoreRetptr { .. } | LoadRetptr { .. } | Retptr => { + StoreRetptr { .. } | LoadRetptr { .. } | Retptr { .. } => { bail!("return pointers aren't supported in wasm interface types"); } I32FromBool | BoolFromI32 => { diff --git a/crates/cli-support/src/wit/standard.rs b/crates/cli-support/src/wit/standard.rs index 0ab0d34ec59..d9c6807620d 100644 --- a/crates/cli-support/src/wit/standard.rs +++ b/crates/cli-support/src/wit/standard.rs @@ -113,8 +113,11 @@ pub enum Instruction { offset: usize, mem: walrus::MemoryId, }, - /// An instruction which pushes the return pointer onto the stack. - Retptr, + /// An instruction which pushes the return pointer onto the stack, reserving + /// `size` bytes of space. + Retptr { + size: u32, + }, /// Pops a `bool` from the stack and pushes an `i32` equivalent I32FromBool, diff --git a/crates/cli/Cargo.toml b/crates/cli/Cargo.toml index c8369119b38..048b8d91f55 100644 --- a/crates/cli/Cargo.toml +++ b/crates/cli/Cargo.toml @@ -12,6 +12,7 @@ Command line interface of the `#[wasm_bindgen]` attribute and project. For more information see https://github.com/rustwasm/wasm-bindgen. """ edition = '2018' +default-run = 'wasm-bindgen' [dependencies] curl = "0.4.13"