From 1572f93eeda7b6f8d01ab7a47dd241bb984b6e16 Mon Sep 17 00:00:00 2001 From: RunDevelopment Date: Fri, 11 Oct 2024 17:48:51 +0200 Subject: [PATCH 01/13] `f64` ABI for 32-bit primitives --- crates/cli-support/src/js/binding.rs | 18 ++++++++++ crates/cli-support/src/wit/incoming.rs | 30 +++++++++++----- crates/cli-support/src/wit/outgoing.rs | 24 ++++++++----- crates/cli-support/src/wit/standard.rs | 8 +++++ src/convert/impls.rs | 49 ++++++++++++++++++++++++-- 5 files changed, 110 insertions(+), 19 deletions(-) diff --git a/crates/cli-support/src/js/binding.rs b/crates/cli-support/src/js/binding.rs index 27d58549ba6..0e291ba9112 100644 --- a/crates/cli-support/src/js/binding.rs +++ b/crates/cli-support/src/js/binding.rs @@ -905,6 +905,19 @@ fn instruction( js.push(format!("isLikeNone({0}) ? {1} : {0}", val, hole)); } + Instruction::F64FromOptionSentinelInt => { + let val = js.pop(); + js.cx.expose_is_like_none(); + js.assert_optional_number(&val); + js.push(format!("isLikeNone({0}) ? 0x100000000 : {0}", val)); + } + Instruction::F64FromOptionSentinelF32 => { + let val = js.pop(); + js.cx.expose_is_like_none(); + js.assert_optional_number(&val); + js.push(format!("isLikeNone({0}) ? 0x100000000 : ({0} === 0x100000000 ? 0x100000001 : {0})", val)); + } + Instruction::FromOptionNative { ty } => { let val = js.pop(); js.cx.expose_is_like_none(); @@ -1262,6 +1275,11 @@ fn instruction( )); } + Instruction::OptionF64Sentinel => { + let val = js.pop(); + js.push(format!("{0} === 0x100000000 ? undefined : {0}", val)); + } + Instruction::OptionU32Sentinel => { let val = js.pop(); js.push(format!("{0} === 0xFFFFFF ? undefined : {0}", val)); diff --git a/crates/cli-support/src/wit/incoming.rs b/crates/cli-support/src/wit/incoming.rs index 06c7160cb63..be682a9c468 100644 --- a/crates/cli-support/src/wit/incoming.rs +++ b/crates/cli-support/src/wit/incoming.rs @@ -276,13 +276,13 @@ impl InstructionBuilder<'_, '_> { &[AdapterType::I32], ); } - Descriptor::I8 => self.in_option_sentinel(AdapterType::S8), - Descriptor::U8 => self.in_option_sentinel(AdapterType::U8), - Descriptor::I16 => self.in_option_sentinel(AdapterType::S16), - Descriptor::U16 => self.in_option_sentinel(AdapterType::U16), - Descriptor::I32 => self.in_option_native(ValType::I32), - Descriptor::U32 => self.in_option_native(ValType::I32), - Descriptor::F32 => self.in_option_native(ValType::F32), + Descriptor::I8 => self.in_option_sentinel32(AdapterType::S8), + Descriptor::U8 => self.in_option_sentinel32(AdapterType::U8), + Descriptor::I16 => self.in_option_sentinel32(AdapterType::S16), + Descriptor::U16 => self.in_option_sentinel32(AdapterType::U16), + Descriptor::I32 => self.in_option_sentinel64_int(AdapterType::I32), + Descriptor::U32 => self.in_option_sentinel64_int(AdapterType::U32), + Descriptor::F32 => self.in_option_sentinel64_f32(AdapterType::F32), Descriptor::F64 => self.in_option_native(ValType::F64), Descriptor::I64 | Descriptor::U64 => self.in_option_native(ValType::I64), Descriptor::Boolean => { @@ -459,11 +459,25 @@ impl InstructionBuilder<'_, '_> { ); } - fn in_option_sentinel(&mut self, ty: AdapterType) { + fn in_option_sentinel32(&mut self, ty: AdapterType) { self.instruction( &[ty.option()], Instruction::I32FromOptionU32Sentinel, &[AdapterType::I32], ); } + fn in_option_sentinel64_int(&mut self, ty: AdapterType) { + self.instruction( + &[ty.option()], + Instruction::F64FromOptionSentinelInt, + &[AdapterType::F64], + ); + } + fn in_option_sentinel64_f32(&mut self, ty: AdapterType) { + self.instruction( + &[ty.option()], + Instruction::F64FromOptionSentinelF32, + &[AdapterType::F64], + ); + } } diff --git a/crates/cli-support/src/wit/outgoing.rs b/crates/cli-support/src/wit/outgoing.rs index a3563551bb5..3c929b034b9 100644 --- a/crates/cli-support/src/wit/outgoing.rs +++ b/crates/cli-support/src/wit/outgoing.rs @@ -257,15 +257,15 @@ impl InstructionBuilder<'_, '_> { &[AdapterType::NamedExternref(name.clone()).option()], ); } - Descriptor::I8 => self.out_option_sentinel(AdapterType::S8), - Descriptor::U8 => self.out_option_sentinel(AdapterType::U8), - Descriptor::I16 => self.out_option_sentinel(AdapterType::S16), - Descriptor::U16 => self.out_option_sentinel(AdapterType::U16), - Descriptor::I32 => self.option_native(true, ValType::I32), - Descriptor::U32 => self.option_native(false, ValType::I32), + Descriptor::I8 => self.out_option_sentinel32(AdapterType::S8), + Descriptor::U8 => self.out_option_sentinel32(AdapterType::U8), + Descriptor::I16 => self.out_option_sentinel32(AdapterType::S16), + Descriptor::U16 => self.out_option_sentinel32(AdapterType::U16), + Descriptor::I32 => self.out_option_sentinel64(AdapterType::S32), + Descriptor::U32 => self.out_option_sentinel64(AdapterType::U32), Descriptor::I64 => self.option_native(true, ValType::I64), Descriptor::U64 => self.option_native(false, ValType::I64), - Descriptor::F32 => self.option_native(true, ValType::F32), + Descriptor::F32 => self.out_option_sentinel64(AdapterType::F32), Descriptor::F64 => self.option_native(true, ValType::F64), Descriptor::Boolean => { self.instruction( @@ -576,11 +576,19 @@ impl InstructionBuilder<'_, '_> { ); } - fn out_option_sentinel(&mut self, ty: AdapterType) { + fn out_option_sentinel32(&mut self, ty: AdapterType) { self.instruction( &[AdapterType::I32], Instruction::OptionU32Sentinel, &[ty.option()], ); } + + fn out_option_sentinel64(&mut self, ty: AdapterType) { + self.instruction( + &[AdapterType::F64], + Instruction::OptionF64Sentinel, + &[ty.option()], + ); + } } diff --git a/crates/cli-support/src/wit/standard.rs b/crates/cli-support/src/wit/standard.rs index aee0e0845e9..7f95a0e8108 100644 --- a/crates/cli-support/src/wit/standard.rs +++ b/crates/cli-support/src/wit/standard.rs @@ -210,6 +210,12 @@ pub enum Instruction { I32FromOptionEnum { hole: u32, }, + /// Pops an `externref` from the stack, pushes either a sentinel value if it's + /// "none" or the integer value of it if it's "some" + F64FromOptionSentinelInt, + /// Pops an `externref` from the stack, pushes either a sentinel value if it's + /// "none" or the f32 value of it if it's "some" + F64FromOptionSentinelF32, /// Pops any externref from the stack and then pushes two values. First is a /// 0/1 if it's none/some and second is `ty` value if it was there or 0 if /// it wasn't there. @@ -320,6 +326,8 @@ pub enum Instruction { kind: VectorKind, mem: walrus::MemoryId, }, + /// pops f64, pushes it viewed as an optional value with a known sentinel + OptionF64Sentinel, /// pops i32, pushes it viewed as an optional value with a known sentinel OptionU32Sentinel, /// pops an i32, then `ty`, then pushes externref diff --git a/src/convert/impls.rs b/src/convert/impls.rs index e54f0669c5a..3087b9e10a6 100644 --- a/src/convert/impls.rs +++ b/src/convert/impls.rs @@ -99,14 +99,57 @@ macro_rules! type_wasm_native { } type_wasm_native!( + i64 as i64 + u64 as u64 + f64 as f64 +); + +macro_rules! type_wasm_native_f64_option { + ($($t:tt as $c:tt)*) => ($( + impl IntoWasmAbi for $t { + type Abi = $c; + + #[inline] + fn into_abi(self) -> $c { self as $c } + } + + impl FromWasmAbi for $t { + type Abi = $c; + + #[inline] + unsafe fn from_abi(js: $c) -> Self { js as $t } + } + + impl IntoWasmAbi for Option<$t> { + type Abi = f64; + + #[inline] + fn into_abi(self) -> Self::Abi { + self.map(|v| v as $c as f64).unwrap_or(4294967296_f64) + } + } + + impl FromWasmAbi for Option<$t> { + type Abi = f64; + + #[inline] + unsafe fn from_abi(js: Self::Abi) -> Self { + if js == 4294967296_f64 { + None + } else { + Some(js as $c as $t) + } + } + } + )*) +} + +type_wasm_native_f64_option!( i32 as i32 isize as i32 u32 as u32 usize as u32 - i64 as i64 - u64 as u64 f32 as f32 - f64 as f64 ); macro_rules! type_abi_as_u32 { From 134811ca8e6bde54e81eddecb44eae43b4f16759 Mon Sep 17 00:00:00 2001 From: RunDevelopment Date: Fri, 11 Oct 2024 18:03:59 +0200 Subject: [PATCH 02/13] Fixed sentinel --- crates/cli-support/src/js/binding.rs | 6 +++--- src/convert/impls.rs | 6 ++++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/crates/cli-support/src/js/binding.rs b/crates/cli-support/src/js/binding.rs index 0e291ba9112..049794f909d 100644 --- a/crates/cli-support/src/js/binding.rs +++ b/crates/cli-support/src/js/binding.rs @@ -909,13 +909,13 @@ fn instruction( let val = js.pop(); js.cx.expose_is_like_none(); js.assert_optional_number(&val); - js.push(format!("isLikeNone({0}) ? 0x100000000 : {0}", val)); + js.push(format!("isLikeNone({0}) ? 0x100000001 : {0}", val)); } Instruction::F64FromOptionSentinelF32 => { let val = js.pop(); js.cx.expose_is_like_none(); js.assert_optional_number(&val); - js.push(format!("isLikeNone({0}) ? 0x100000000 : ({0} === 0x100000000 ? 0x100000001 : {0})", val)); + js.push(format!("isLikeNone({0}) ? 0x100000001 : ({0} === 0x100000001 ? 0x100000002 : {0})", val)); } Instruction::FromOptionNative { ty } => { @@ -1277,7 +1277,7 @@ fn instruction( Instruction::OptionF64Sentinel => { let val = js.pop(); - js.push(format!("{0} === 0x100000000 ? undefined : {0}", val)); + js.push(format!("{0} === 0x100000001 ? undefined : {0}", val)); } Instruction::OptionU32Sentinel => { diff --git a/src/convert/impls.rs b/src/convert/impls.rs index 3087b9e10a6..f21f5938857 100644 --- a/src/convert/impls.rs +++ b/src/convert/impls.rs @@ -104,6 +104,8 @@ type_wasm_native!( f64 as f64 ); +const F64_SENTINEL: f64 = 4294967297_f64; // 2^32 + 1 + macro_rules! type_wasm_native_f64_option { ($($t:tt as $c:tt)*) => ($( impl IntoWasmAbi for $t { @@ -125,7 +127,7 @@ macro_rules! type_wasm_native_f64_option { #[inline] fn into_abi(self) -> Self::Abi { - self.map(|v| v as $c as f64).unwrap_or(4294967296_f64) + self.map(|v| v as $c as f64).unwrap_or(F64_SENTINEL) } } @@ -134,7 +136,7 @@ macro_rules! type_wasm_native_f64_option { #[inline] unsafe fn from_abi(js: Self::Abi) -> Self { - if js == 4294967296_f64 { + if js == F64_SENTINEL { None } else { Some(js as $c as $t) From 6138cd5b082defcbcebc7dfb3eccee7d5815eca5 Mon Sep 17 00:00:00 2001 From: RunDevelopment Date: Mon, 14 Oct 2024 15:30:14 +0200 Subject: [PATCH 03/13] Added docs for option sentinel values --- src/convert/impls.rs | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/src/convert/impls.rs b/src/convert/impls.rs index f21f5938857..1d38475600e 100644 --- a/src/convert/impls.rs +++ b/src/convert/impls.rs @@ -104,7 +104,14 @@ type_wasm_native!( f64 as f64 ); -const F64_SENTINEL: f64 = 4294967297_f64; // 2^32 + 1 +/// The sentinel value is 2^32 + 1 for 32-bit primitive types. +/// +/// 2^32 + 1 is used, because it's the smallest positive integer that cannot be +/// represented by any 32-bit primitive. While any value >= 2^32 works as a +/// sentinel value for 32-bit integers, it's a bit more tricky for `f32`. `f32` +/// can represent all powers of 2 up to 2^127 exactly. And between 2^32 and 2^33, +/// `f32` can represent all integers 2^32+512*k exactly. +const F64_ABI_OPTION_SENTINEL: f64 = 4294967297_f64; macro_rules! type_wasm_native_f64_option { ($($t:tt as $c:tt)*) => ($( @@ -127,7 +134,7 @@ macro_rules! type_wasm_native_f64_option { #[inline] fn into_abi(self) -> Self::Abi { - self.map(|v| v as $c as f64).unwrap_or(F64_SENTINEL) + self.map(|v| v as $c as f64).unwrap_or(F64_ABI_OPTION_SENTINEL) } } @@ -136,7 +143,7 @@ macro_rules! type_wasm_native_f64_option { #[inline] unsafe fn from_abi(js: Self::Abi) -> Self { - if js == F64_SENTINEL { + if js == F64_ABI_OPTION_SENTINEL { None } else { Some(js as $c as $t) @@ -154,6 +161,13 @@ type_wasm_native_f64_option!( f32 as f32 ); +/// The sentinel value is 0xFF_FFFF for primitives with less than 32 bits. +/// +/// This value is used, so all small primitive types (`bool`, `i8`, `u8`, +/// `i16`, `u16`, `char`) can use the same JS glue code. `char::MAX` is +/// 0x10_FFFF btw. +const U32_ABI_OPTION_SENTINEL: u32 = 0x00FF_FFFFu32; + macro_rules! type_abi_as_u32 { ($($t:tt)*) => ($( impl IntoWasmAbi for $t { @@ -172,12 +186,12 @@ macro_rules! type_abi_as_u32 { impl OptionIntoWasmAbi for $t { #[inline] - fn none() -> u32 { 0x00FF_FFFFu32 } + fn none() -> u32 { U32_ABI_OPTION_SENTINEL } } impl OptionFromWasmAbi for $t { #[inline] - fn is_none(js: &u32) -> bool { *js == 0x00FF_FFFFu32 } + fn is_none(js: &u32) -> bool { *js == U32_ABI_OPTION_SENTINEL } } )*) } @@ -205,14 +219,14 @@ impl FromWasmAbi for bool { impl OptionIntoWasmAbi for bool { #[inline] fn none() -> u32 { - 0x00FF_FFFFu32 + U32_ABI_OPTION_SENTINEL } } impl OptionFromWasmAbi for bool { #[inline] fn is_none(js: &u32) -> bool { - *js == 0x00FF_FFFFu32 + *js == U32_ABI_OPTION_SENTINEL } } @@ -238,14 +252,14 @@ impl FromWasmAbi for char { impl OptionIntoWasmAbi for char { #[inline] fn none() -> u32 { - 0x00FF_FFFFu32 + U32_ABI_OPTION_SENTINEL } } impl OptionFromWasmAbi for char { #[inline] fn is_none(js: &u32) -> bool { - *js == 0x00FF_FFFFu32 + *js == U32_ABI_OPTION_SENTINEL } } From 18282677bd1e4996c1450541a823b6e602e9b250 Mon Sep 17 00:00:00 2001 From: RunDevelopment Date: Mon, 14 Oct 2024 16:09:36 +0200 Subject: [PATCH 04/13] Cover edge cases --- crates/cli-support/src/js/binding.rs | 26 +++++++++++++++++++++++--- crates/cli-support/src/wit/incoming.rs | 8 ++++---- crates/cli-support/src/wit/standard.rs | 4 +++- 3 files changed, 30 insertions(+), 8 deletions(-) diff --git a/crates/cli-support/src/js/binding.rs b/crates/cli-support/src/js/binding.rs index 211faf755cf..7b47d5b63c1 100644 --- a/crates/cli-support/src/js/binding.rs +++ b/crates/cli-support/src/js/binding.rs @@ -920,17 +920,37 @@ fn instruction( js.push(format!("isLikeNone({0}) ? {1} : {0}", val, hole)); } - Instruction::F64FromOptionSentinelInt => { + Instruction::F64FromOptionSentinelInt { signed } => { let val = js.pop(); js.cx.expose_is_like_none(); js.assert_optional_number(&val); - js.push(format!("isLikeNone({0}) ? 0x100000001 : {0}", val)); + + // We need to convert the given number to a 32-bit integer before + // passing it to the ABI for 2 reasons: + // 1. Rust's behavior for `value_f64 as i32/u32` is different from + // the WebAssembly behavior for values outside the 32-bit range. + // We could implement this behavior is Rust too, but it's easier + // to do it in JS. + // 2. If we allowed values outside the 32-bit range, the sentinel + // value itself would be allowed. This would make it impossible + // to distinguish between the sentinel value and a valid value. + + let op = if *signed { ">>" } else { ">>>" }; + js.push(format!("isLikeNone({val}) ? 0x100000001 : ({val}) {op} 0")); } Instruction::F64FromOptionSentinelF32 => { let val = js.pop(); js.cx.expose_is_like_none(); js.assert_optional_number(&val); - js.push(format!("isLikeNone({0}) ? 0x100000001 : ({0} === 0x100000001 ? 0x100000002 : {0})", val)); + + // Similar to the above 32-bit integer variant, we convert the + // number to a 32-bit *float* before passing it to the ABI. This + // ensures consistent behavior with WebAssembly and makes it + // possible to use a sentinel value. + + js.push(format!( + "isLikeNone({val}) ? 0x100000001 : Math.fround({val})" + )); } Instruction::FromOptionNative { ty } => { diff --git a/crates/cli-support/src/wit/incoming.rs b/crates/cli-support/src/wit/incoming.rs index 16faf05b3a4..5c2a2c4874e 100644 --- a/crates/cli-support/src/wit/incoming.rs +++ b/crates/cli-support/src/wit/incoming.rs @@ -280,8 +280,8 @@ impl InstructionBuilder<'_, '_> { Descriptor::U8 => self.in_option_sentinel32(AdapterType::U8), Descriptor::I16 => self.in_option_sentinel32(AdapterType::S16), Descriptor::U16 => self.in_option_sentinel32(AdapterType::U16), - Descriptor::I32 => self.in_option_sentinel64_int(AdapterType::I32), - Descriptor::U32 => self.in_option_sentinel64_int(AdapterType::U32), + Descriptor::I32 => self.in_option_sentinel64_int(AdapterType::I32, true), + Descriptor::U32 => self.in_option_sentinel64_int(AdapterType::U32, false), Descriptor::F32 => self.in_option_sentinel64_f32(AdapterType::F32), Descriptor::F64 => self.in_option_native(ValType::F64), Descriptor::I64 | Descriptor::U64 => self.in_option_native(ValType::I64), @@ -466,10 +466,10 @@ impl InstructionBuilder<'_, '_> { &[AdapterType::I32], ); } - fn in_option_sentinel64_int(&mut self, ty: AdapterType) { + fn in_option_sentinel64_int(&mut self, ty: AdapterType, signed: bool) { self.instruction( &[ty.option()], - Instruction::F64FromOptionSentinelInt, + Instruction::F64FromOptionSentinelInt { signed }, &[AdapterType::F64], ); } diff --git a/crates/cli-support/src/wit/standard.rs b/crates/cli-support/src/wit/standard.rs index 5d656f95666..8d8c7bc99d7 100644 --- a/crates/cli-support/src/wit/standard.rs +++ b/crates/cli-support/src/wit/standard.rs @@ -216,7 +216,9 @@ pub enum Instruction { }, /// Pops an `externref` from the stack, pushes either a sentinel value if it's /// "none" or the integer value of it if it's "some" - F64FromOptionSentinelInt, + F64FromOptionSentinelInt { + signed: bool, + }, /// Pops an `externref` from the stack, pushes either a sentinel value if it's /// "none" or the f32 value of it if it's "some" F64FromOptionSentinelF32, From 140a08d5e509e58a3406510b77ce71fc73bdcbfa Mon Sep 17 00:00:00 2001 From: RunDevelopment Date: Mon, 14 Oct 2024 16:41:13 +0200 Subject: [PATCH 05/13] Add debug name to error message to aid debugging --- crates/cli-support/src/js/binding.rs | 21 +++++++++++++++++---- crates/cli-support/src/js/mod.rs | 27 ++++++++++++--------------- 2 files changed, 29 insertions(+), 19 deletions(-) diff --git a/crates/cli-support/src/js/binding.rs b/crates/cli-support/src/js/binding.rs index 7b47d5b63c1..546d86fa27b 100644 --- a/crates/cli-support/src/js/binding.rs +++ b/crates/cli-support/src/js/binding.rs @@ -37,6 +37,9 @@ pub struct JsBuilder<'a, 'b> { /// JS functions, etc. cx: &'a mut Context<'b>, + /// A debug name for the function being generated, used for error messages + debug_name: &'a str, + /// The "prelude" of the function, or largely just the JS function we've /// built so far. prelude: String, @@ -121,6 +124,7 @@ impl<'a, 'b> Builder<'a, 'b> { asyncness: bool, variadic: bool, generate_jsdoc: bool, + debug_name: &str, ) -> Result { if self .cx @@ -138,7 +142,7 @@ impl<'a, 'b> Builder<'a, 'b> { // If this is a method then we're generating this as part of a class // method, so the leading parameter is the this pointer stored on // the JS object, so synthesize that here. - let mut js = JsBuilder::new(self.cx); + let mut js = JsBuilder::new(self.cx, debug_name); if let Some(consumes_self) = self.method { let _ = params.next(); if js.cx.config.debug { @@ -184,7 +188,12 @@ impl<'a, 'b> Builder<'a, 'b> { )?; } - assert_eq!(js.stack.len(), adapter.results.len()); + assert_eq!( + js.stack.len(), + adapter.results.len(), + "stack size mismatch for {}", + debug_name + ); match js.stack.len() { 0 => {} 1 => { @@ -422,9 +431,10 @@ impl<'a, 'b> Builder<'a, 'b> { } impl<'a, 'b> JsBuilder<'a, 'b> { - pub fn new(cx: &'a mut Context<'b>) -> JsBuilder<'a, 'b> { + pub fn new(cx: &'a mut Context<'b>, debug_name: &'a str) -> JsBuilder<'a, 'b> { JsBuilder { cx, + debug_name, args: Vec::new(), tmp: 0, pre_try: String::new(), @@ -463,7 +473,10 @@ impl<'a, 'b> JsBuilder<'a, 'b> { } fn pop(&mut self) -> String { - self.stack.pop().unwrap() + match self.stack.pop() { + Some(s) => s, + None => panic!("popping an empty stack in {}", self.debug_name), + } } fn push(&mut self, arg: String) { diff --git a/crates/cli-support/src/js/mod.rs b/crates/cli-support/src/js/mod.rs index 8b8cc9aaf50..92a0de3df7c 100644 --- a/crates/cli-support/src/js/mod.rs +++ b/crates/cli-support/src/js/mod.rs @@ -2655,6 +2655,16 @@ __wbg_set_wasm(wasm);" ContextAdapterKind::Adapter => {} } + // an internal debug name to help with error messages + let debug_name = match kind { + ContextAdapterKind::Import(i) => { + let i = builder.cx.module.imports.get(i); + format!("import of `{}::{}`", i.module, i.name) + } + ContextAdapterKind::Export(e) => format!("`{}`", e.debug_name), + ContextAdapterKind::Adapter => format!("adapter {}", id.0), + }; + // Process the `binding` and generate a bunch of JS/TypeScript/etc. let binding::JsFunction { ts_sig, @@ -2674,22 +2684,9 @@ __wbg_set_wasm(wasm);" asyncness, variadic, generate_jsdoc, + &debug_name, ) - .with_context(|| match kind { - ContextAdapterKind::Export(e) => { - format!("failed to generate bindings for `{}`", e.debug_name) - } - ContextAdapterKind::Import(i) => { - let i = builder.cx.module.imports.get(i); - format!( - "failed to generate bindings for import of `{}::{}`", - i.module, i.name - ) - } - ContextAdapterKind::Adapter => { - "failed to generates bindings for adapter".to_string() - } - })?; + .with_context(|| "failed to generates bindings for ".to_string() + &debug_name)?; self.typescript_refs.extend(ts_refs); From 1aaa40b220840671269c162d746daf156c8ee0d8 Mon Sep 17 00:00:00 2001 From: RunDevelopment Date: Tue, 15 Oct 2024 21:47:04 +0200 Subject: [PATCH 06/13] Option needs to have the same ABI as Option --- src/convert/impls.rs | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/src/convert/impls.rs b/src/convert/impls.rs index 1d38475600e..28dac9cfd83 100644 --- a/src/convert/impls.rs +++ b/src/convert/impls.rs @@ -282,20 +282,25 @@ impl FromWasmAbi for *const T { } impl IntoWasmAbi for Option<*const T> { - type Abi = Option; + type Abi = f64; #[inline] - fn into_abi(self) -> Option { - self.map(|ptr| ptr as u32) + fn into_abi(self) -> f64 { + self.map(|ptr| ptr as u32 as f64) + .unwrap_or(F64_ABI_OPTION_SENTINEL) } } impl FromWasmAbi for Option<*const T> { - type Abi = Option; + type Abi = f64; #[inline] - unsafe fn from_abi(js: Option) -> Option<*const T> { - js.map(|ptr| ptr as *const T) + unsafe fn from_abi(js: f64) -> Option<*const T> { + if js == F64_ABI_OPTION_SENTINEL { + None + } else { + Some(js as u32 as *const T) + } } } @@ -318,20 +323,25 @@ impl FromWasmAbi for *mut T { } impl IntoWasmAbi for Option<*mut T> { - type Abi = Option; + type Abi = f64; #[inline] - fn into_abi(self) -> Option { - self.map(|ptr| ptr as u32) + fn into_abi(self) -> f64 { + self.map(|ptr| ptr as u32 as f64) + .unwrap_or(F64_ABI_OPTION_SENTINEL) } } impl FromWasmAbi for Option<*mut T> { - type Abi = Option; + type Abi = f64; #[inline] - unsafe fn from_abi(js: Option) -> Option<*mut T> { - js.map(|ptr| ptr as *mut T) + unsafe fn from_abi(js: f64) -> Option<*mut T> { + if js == F64_ABI_OPTION_SENTINEL { + None + } else { + Some(js as u32 as *mut T) + } } } From e802d6b9f8fca2a594735bf2a421027d2e07e2c7 Mon Sep 17 00:00:00 2001 From: RunDevelopment Date: Tue, 15 Oct 2024 22:09:55 +0200 Subject: [PATCH 07/13] Updated changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2593781fb3c..d4725e2a651 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,9 @@ * Remove unnecessary JSDoc type annotations from generated `.d.ts` files [#4187](https://github.com/rustwasm/wasm-bindgen/pull/4187) +* Optimized ABI for `Option<{i32,u32,isize,usize,f32,*const T,*mut T}>`. + [#4183](https://github.com/rustwasm/wasm-bindgen/pull/4183) + ### Fixed * Fixed methods with `self: &Self` consuming the object. From f0563a77ecda1f1847692cf537b890190a7cbed3 Mon Sep 17 00:00:00 2001 From: RunDevelopment Date: Tue, 15 Oct 2024 22:13:31 +0200 Subject: [PATCH 08/13] Added tests for the sentinel value --- tests/wasm/optional_primitives.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/wasm/optional_primitives.js b/tests/wasm/optional_primitives.js index 89bf4df7eab..18b3f8f7ef7 100644 --- a/tests/wasm/optional_primitives.js +++ b/tests/wasm/optional_primitives.js @@ -23,12 +23,16 @@ exports.js_works = () => { assert.strictEqual(wasm.optional_i32_identity(wasm.optional_i32_neg_one()), -1); assert.strictEqual(wasm.optional_i32_identity(wasm.optional_i32_min()), -2147483648); assert.strictEqual(wasm.optional_i32_identity(wasm.optional_i32_max()), 2147483647); + assert.strictEqual(wasm.optional_i32_identity(2 ** 32), 0); + assert.strictEqual(wasm.optional_i32_identity(2 ** 32 + 1), 1); assert.strictEqual(wasm.optional_u32_identity(wasm.optional_u32_none()), undefined); assert.strictEqual(wasm.optional_u32_identity(wasm.optional_u32_zero()), 0); assert.strictEqual(wasm.optional_u32_identity(wasm.optional_u32_one()), 1); assert.strictEqual(wasm.optional_u32_identity(wasm.optional_u32_min()), 0); assert.strictEqual(wasm.optional_u32_identity(wasm.optional_u32_max()), 4294967295); + assert.strictEqual(wasm.optional_u32_identity(2 ** 32), 0); + assert.strictEqual(wasm.optional_u32_identity(2 ** 32 + 1), 1); assert.strictEqual(wasm.optional_isize_identity(wasm.optional_isize_none()), undefined); assert.strictEqual(wasm.optional_isize_identity(wasm.optional_isize_zero()), 0); @@ -36,17 +40,23 @@ exports.js_works = () => { assert.strictEqual(wasm.optional_isize_identity(wasm.optional_isize_neg_one()), -1); assert.strictEqual(wasm.optional_isize_identity(wasm.optional_isize_min()), -2147483648); assert.strictEqual(wasm.optional_isize_identity(wasm.optional_isize_max()), 2147483647); + assert.strictEqual(wasm.optional_isize_identity(2 ** 32), 0); // isize is 32 bit + assert.strictEqual(wasm.optional_isize_identity(2 ** 32 + 1), 1); assert.strictEqual(wasm.optional_usize_identity(wasm.optional_usize_none()), undefined); assert.strictEqual(wasm.optional_usize_identity(wasm.optional_usize_zero()), 0); assert.strictEqual(wasm.optional_usize_identity(wasm.optional_usize_one()), 1); assert.strictEqual(wasm.optional_usize_identity(wasm.optional_usize_min()), 0); assert.strictEqual(wasm.optional_usize_identity(wasm.optional_usize_max()), 4294967295); + assert.strictEqual(wasm.optional_usize_identity(2 ** 32), 0); // usize is 32 bit + assert.strictEqual(wasm.optional_usize_identity(2 ** 32 + 1), 1); assert.strictEqual(wasm.optional_f32_identity(wasm.optional_f32_none()), undefined); assert.strictEqual(wasm.optional_f32_identity(wasm.optional_f32_zero()), 0); assert.strictEqual(wasm.optional_f32_identity(wasm.optional_f32_one()), 1); assert.strictEqual(wasm.optional_f32_identity(wasm.optional_f32_neg_one()), -1); + assert.strictEqual(wasm.optional_f32_identity(2 ** 32), 2 ** 32); + assert.strictEqual(wasm.optional_f32_identity(2 ** 32 + 1), 2 ** 32); assert.strictEqual(wasm.optional_f64_identity(wasm.optional_f64_none()), undefined); assert.strictEqual(wasm.optional_f64_identity(wasm.optional_f64_zero()), 0); From aa034a96646476ab24a8b157b117aca5af2b13a5 Mon Sep 17 00:00:00 2001 From: RunDevelopment Date: Tue, 15 Oct 2024 22:39:12 +0200 Subject: [PATCH 09/13] Trigger CI From 53d653b781173671f208e81481fb7bfdc2019306 Mon Sep 17 00:00:00 2001 From: Michael Schmidt Date: Wed, 16 Oct 2024 00:43:46 +0200 Subject: [PATCH 10/13] Update crates/cli-support/src/js/binding.rs Co-authored-by: daxpedda --- crates/cli-support/src/js/binding.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/cli-support/src/js/binding.rs b/crates/cli-support/src/js/binding.rs index 546d86fa27b..f22f95c70c0 100644 --- a/crates/cli-support/src/js/binding.rs +++ b/crates/cli-support/src/js/binding.rs @@ -942,7 +942,7 @@ fn instruction( // passing it to the ABI for 2 reasons: // 1. Rust's behavior for `value_f64 as i32/u32` is different from // the WebAssembly behavior for values outside the 32-bit range. - // We could implement this behavior is Rust too, but it's easier + // We could implement this behavior in Rust too, but it's easier // to do it in JS. // 2. If we allowed values outside the 32-bit range, the sentinel // value itself would be allowed. This would make it impossible From d7a735f434877250fe4c948b5d39ece9adcce25c Mon Sep 17 00:00:00 2001 From: Michael Schmidt Date: Wed, 16 Oct 2024 00:53:26 +0200 Subject: [PATCH 11/13] Update CHANGELOG.md Co-authored-by: daxpedda --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d4725e2a651..5ba9f07e7ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ * Remove unnecessary JSDoc type annotations from generated `.d.ts` files [#4187](https://github.com/rustwasm/wasm-bindgen/pull/4187) -* Optimized ABI for `Option<{i32,u32,isize,usize,f32,*const T,*mut T}>`. +* Optimized ABI performance for `Option<{i32,u32,isize,usize,f32,*const T,*mut T}>`. [#4183](https://github.com/rustwasm/wasm-bindgen/pull/4183) ### Fixed From 270ba8e7fe34d4880d868d78f5b703ee84b74c44 Mon Sep 17 00:00:00 2001 From: RunDevelopment Date: Wed, 16 Oct 2024 00:54:54 +0200 Subject: [PATCH 12/13] Add comments explaining test results --- tests/wasm/optional_primitives.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/wasm/optional_primitives.js b/tests/wasm/optional_primitives.js index 18b3f8f7ef7..d6755144a20 100644 --- a/tests/wasm/optional_primitives.js +++ b/tests/wasm/optional_primitives.js @@ -23,16 +23,16 @@ exports.js_works = () => { assert.strictEqual(wasm.optional_i32_identity(wasm.optional_i32_neg_one()), -1); assert.strictEqual(wasm.optional_i32_identity(wasm.optional_i32_min()), -2147483648); assert.strictEqual(wasm.optional_i32_identity(wasm.optional_i32_max()), 2147483647); - assert.strictEqual(wasm.optional_i32_identity(2 ** 32), 0); - assert.strictEqual(wasm.optional_i32_identity(2 ** 32 + 1), 1); + assert.strictEqual(wasm.optional_i32_identity(2 ** 32), 0); // overflows 32 bits + assert.strictEqual(wasm.optional_i32_identity(2 ** 32 + 1), 1); // overflows and wraps back around assert.strictEqual(wasm.optional_u32_identity(wasm.optional_u32_none()), undefined); assert.strictEqual(wasm.optional_u32_identity(wasm.optional_u32_zero()), 0); assert.strictEqual(wasm.optional_u32_identity(wasm.optional_u32_one()), 1); assert.strictEqual(wasm.optional_u32_identity(wasm.optional_u32_min()), 0); assert.strictEqual(wasm.optional_u32_identity(wasm.optional_u32_max()), 4294967295); - assert.strictEqual(wasm.optional_u32_identity(2 ** 32), 0); - assert.strictEqual(wasm.optional_u32_identity(2 ** 32 + 1), 1); + assert.strictEqual(wasm.optional_u32_identity(2 ** 32), 0); // overflows 32 bits + assert.strictEqual(wasm.optional_u32_identity(2 ** 32 + 1), 1); // overflows and wraps back around assert.strictEqual(wasm.optional_isize_identity(wasm.optional_isize_none()), undefined); assert.strictEqual(wasm.optional_isize_identity(wasm.optional_isize_zero()), 0); @@ -40,7 +40,7 @@ exports.js_works = () => { assert.strictEqual(wasm.optional_isize_identity(wasm.optional_isize_neg_one()), -1); assert.strictEqual(wasm.optional_isize_identity(wasm.optional_isize_min()), -2147483648); assert.strictEqual(wasm.optional_isize_identity(wasm.optional_isize_max()), 2147483647); - assert.strictEqual(wasm.optional_isize_identity(2 ** 32), 0); // isize is 32 bit + assert.strictEqual(wasm.optional_isize_identity(2 ** 32), 0); // isize is 32 bit, so it behaves the same as i32 assert.strictEqual(wasm.optional_isize_identity(2 ** 32 + 1), 1); assert.strictEqual(wasm.optional_usize_identity(wasm.optional_usize_none()), undefined); @@ -48,15 +48,15 @@ exports.js_works = () => { assert.strictEqual(wasm.optional_usize_identity(wasm.optional_usize_one()), 1); assert.strictEqual(wasm.optional_usize_identity(wasm.optional_usize_min()), 0); assert.strictEqual(wasm.optional_usize_identity(wasm.optional_usize_max()), 4294967295); - assert.strictEqual(wasm.optional_usize_identity(2 ** 32), 0); // usize is 32 bit + assert.strictEqual(wasm.optional_usize_identity(2 ** 32), 0); // usize is 32 bit, so it behaves the same as i32 assert.strictEqual(wasm.optional_usize_identity(2 ** 32 + 1), 1); assert.strictEqual(wasm.optional_f32_identity(wasm.optional_f32_none()), undefined); assert.strictEqual(wasm.optional_f32_identity(wasm.optional_f32_zero()), 0); assert.strictEqual(wasm.optional_f32_identity(wasm.optional_f32_one()), 1); assert.strictEqual(wasm.optional_f32_identity(wasm.optional_f32_neg_one()), -1); - assert.strictEqual(wasm.optional_f32_identity(2 ** 32), 2 ** 32); - assert.strictEqual(wasm.optional_f32_identity(2 ** 32 + 1), 2 ** 32); + assert.strictEqual(wasm.optional_f32_identity(2 ** 32), 2 ** 32); // 2^32 is exactly representable + assert.strictEqual(wasm.optional_f32_identity(2 ** 32 + 1), 2 ** 32); // 2^32 + 1 is not and gets rounded assert.strictEqual(wasm.optional_f64_identity(wasm.optional_f64_none()), undefined); assert.strictEqual(wasm.optional_f64_identity(wasm.optional_f64_zero()), 0); From 7ff168a368f065049aa0ade2b6f50bf89949fd1a Mon Sep 17 00:00:00 2001 From: RunDevelopment Date: Wed, 16 Oct 2024 01:01:42 +0200 Subject: [PATCH 13/13] Add comment explaining the conversion --- crates/cli-support/src/js/binding.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/crates/cli-support/src/js/binding.rs b/crates/cli-support/src/js/binding.rs index f22f95c70c0..000634297d3 100644 --- a/crates/cli-support/src/js/binding.rs +++ b/crates/cli-support/src/js/binding.rs @@ -947,6 +947,11 @@ fn instruction( // 2. If we allowed values outside the 32-bit range, the sentinel // value itself would be allowed. This would make it impossible // to distinguish between the sentinel value and a valid value. + // + // To perform the actual conversion, we use JS bit shifts. Handily, + // >> and >>> perform a conversion to i32 and u32 respectively + // to apply the bit shift, so we can use e.g. x >>> 0 to convert to + // u32. let op = if *signed { ">>" } else { ">>>" }; js.push(format!("isLikeNone({val}) ? 0x100000001 : ({val}) {op} 0"));