From 56c644ca03eb75e007d25ddefa8819e0286c4f9b Mon Sep 17 00:00:00 2001 From: snek Date: Tue, 18 Feb 2025 07:41:01 -0800 Subject: [PATCH] fix: handle all values for buffers in turbocall codegen --- cli/lib/util/logger.rs | 2 + ext/ffi/call.rs | 2 +- ext/ffi/dlfcn.rs | 14 +- ext/ffi/ir.rs | 54 ++++---- ext/ffi/symbol.rs | 1 + ext/ffi/turbocall.rs | 284 ++++++++++++++++++++++++++++++---------- tests/ffi/tests/test.js | 9 +- 7 files changed, 264 insertions(+), 102 deletions(-) diff --git a/cli/lib/util/logger.rs b/cli/lib/util/logger.rs index 0fa5c27f78f481..dafb216555980d 100644 --- a/cli/lib/util/logger.rs +++ b/cli/lib/util/logger.rs @@ -95,6 +95,8 @@ pub fn init< .filter_module("opentelemetry_sdk", log::LevelFilter::Off) // for deno_compile, this is too verbose .filter_module("editpe", log::LevelFilter::Error) + // too verbose + .filter_module("cranelift_codegen", log::LevelFilter::Off) .format(|buf, record| { let mut target = record.target().to_string(); if let Some(line_no) = record.line() { diff --git a/ext/ffi/call.rs b/ext/ffi/call.rs index 4f9a057d01379a..d1fbcdd379bb0f 100644 --- a/ext/ffi/call.rs +++ b/ext/ffi/call.rs @@ -125,7 +125,7 @@ where ffi_args.push(ffi_parse_f64_arg(value)?); } NativeType::Buffer => { - ffi_args.push(ffi_parse_buffer_arg(scope, value)?); + ffi_args.push(ffi_parse_buffer_arg(value)?); } NativeType::Struct(_) => { ffi_args.push(ffi_parse_struct_arg(scope, value)?); diff --git a/ext/ffi/dlfcn.rs b/ext/ffi/dlfcn.rs index da5a85e7e3025e..008ca9a1619c06 100644 --- a/ext/ffi/dlfcn.rs +++ b/ext/ffi/dlfcn.rs @@ -212,6 +212,7 @@ where let func_key = v8::String::new(scope, &symbol_key).unwrap(); let sym = Box::new(Symbol { + name: symbol_key.clone(), cif, ptr, parameter_types: foreign_fn.parameters, @@ -259,9 +260,16 @@ fn make_sync_fn<'s>( symbol: Box, ) -> v8::Local<'s, v8::Function> { let turbocall = if turbocall::is_compatible(&symbol) { - let trampoline = turbocall::compile_trampoline(&symbol); - let turbocall = turbocall::make_template(&symbol, trampoline); - Some(turbocall) + match turbocall::compile_trampoline(&symbol) { + Ok(trampoline) => { + let turbocall = turbocall::make_template(&symbol, trampoline); + Some(turbocall) + } + Err(e) => { + log::warn!("Failed to compile FFI turbocall: {e}"); + None + } + } } else { None }; diff --git a/ext/ffi/ir.rs b/ext/ffi/ir.rs index a1877b4a2bb9b2..21af3ddb2e7d37 100644 --- a/ext/ffi/ir.rs +++ b/ext/ffi/ir.rs @@ -350,39 +350,47 @@ pub fn ffi_parse_pointer_arg( } #[inline] -pub fn ffi_parse_buffer_arg( - scope: &mut v8::HandleScope, +pub fn parse_buffer_arg( arg: v8::Local, -) -> Result { +) -> Result<*mut c_void, IRError> { // Order of checking: // 1. ArrayBuffer: Fairly common and not supported by Fast API, optimise this case. // 2. ArrayBufferView: Common and supported by Fast API // 5. Null: Very uncommon / can be represented by a 0. - let pointer = if let Ok(value) = v8::Local::::try_from(arg) { - if let Some(non_null) = value.data() { - non_null.as_ptr() - } else { - ptr::null_mut() - } + if let Ok(value) = v8::Local::::try_from(arg) { + Ok(value.data().map(|p| p.as_ptr()).unwrap_or(ptr::null_mut())) } else if let Ok(value) = v8::Local::::try_from(arg) { - let byte_offset = value.byte_offset(); - let pointer = value - .buffer(scope) - .ok_or(IRError::InvalidArrayBufferView)? - .data(); - if let Some(non_null) = pointer { - // SAFETY: Pointer is non-null, and V8 guarantees that the byte_offset - // is within the buffer backing store. - unsafe { non_null.as_ptr().add(byte_offset) } + const { + // We don't keep `buffer` around when this function returns, + // so assert that it will be unused. + assert!(deno_core::v8::TYPED_ARRAY_MAX_SIZE_IN_HEAP == 0); + } + let mut buffer = [0; deno_core::v8::TYPED_ARRAY_MAX_SIZE_IN_HEAP]; + // SAFETY: `buffer` is unused due to above, returned pointer is not + // dereferenced by rust code, and we keep it alive at least as long + // as the turbocall. + let (ptr, len) = unsafe { value.get_contents_raw_parts(&mut buffer) }; + if ptr == buffer.as_mut_ptr() { + // Empty TypedArray instances can hit this path because their base pointer + // isn't cleared properly: https://issues.chromium.org/issues/40643872 + debug_assert_eq!(len, 0); + Ok(ptr::null_mut()) } else { - ptr::null_mut() + Ok(ptr as _) } } else if arg.is_null() { - ptr::null_mut() + Ok(ptr::null_mut()) } else { - return Err(IRError::InvalidBufferType); - }; + Err(IRError::InvalidBufferType) + } +} + +#[inline] +pub fn ffi_parse_buffer_arg( + arg: v8::Local, +) -> Result { + let pointer = parse_buffer_arg(arg)?; Ok(NativeValue { pointer }) } @@ -493,7 +501,7 @@ where ffi_args.push(ffi_parse_f64_arg(value)?); } NativeType::Buffer => { - ffi_args.push(ffi_parse_buffer_arg(scope, value)?); + ffi_args.push(ffi_parse_buffer_arg(value)?); } NativeType::Struct(_) => { ffi_args.push(ffi_parse_struct_arg(scope, value)?); diff --git a/ext/ffi/symbol.rs b/ext/ffi/symbol.rs index 5bca5be6d2adae..bef06eda49364d 100644 --- a/ext/ffi/symbol.rs +++ b/ext/ffi/symbol.rs @@ -67,6 +67,7 @@ impl TryFrom for libffi::middle::Type { #[derive(Clone)] pub struct Symbol { + pub name: String, pub cif: libffi::middle::Cif, pub ptr: libffi::middle::CodePtr, pub parameter_types: Vec, diff --git a/ext/ffi/turbocall.rs b/ext/ffi/turbocall.rs index acf88a31789fe9..1bce0f9d8ce934 100644 --- a/ext/ffi/turbocall.rs +++ b/ext/ffi/turbocall.rs @@ -7,6 +7,33 @@ use deno_core::v8::fast_api; use crate::NativeType; use crate::Symbol; +#[derive(Debug, thiserror::Error, deno_error::JsError)] +pub enum TurbocallError { + #[class(generic)] + #[error(transparent)] + SetError(#[from] cranelift::prelude::settings::SetError), + + #[class(generic)] + #[error("Cranelift ISA error: {0}")] + IsaError(&'static str), + + #[class(generic)] + #[error(transparent)] + CodegenError(#[from] cranelift::codegen::CodegenError), + + #[class(generic)] + #[error(transparent)] + VerifierError(#[from] cranelift::codegen::verifier::VerifierErrors), + + #[class(generic)] + #[error("{0}")] + CompileError(String), + + #[class(generic)] + #[error(transparent)] + Stdio(#[from] std::io::Error), +} + pub(crate) fn is_compatible(sym: &Symbol) -> bool { !matches!(sym.result_type, NativeType::Struct(_)) && !sym @@ -27,35 +54,69 @@ impl Trampoline { } #[allow(unused)] -pub(crate) fn compile_trampoline(sym: &Symbol) -> Trampoline { +pub(crate) fn compile_trampoline( + sym: &Symbol, +) -> Result { use cranelift::prelude::*; let mut flag_builder = settings::builder(); - flag_builder.set("is_pic", "true").unwrap(); - flag_builder.set("opt_level", "speed_and_size").unwrap(); + flag_builder.set("is_pic", "true")?; + flag_builder.set("opt_level", "speed_and_size")?; let flags = settings::Flags::new(flag_builder); - let isa = cranelift_native::builder().unwrap().finish(flags).unwrap(); + let isa = cranelift_native::builder() + .map_err(TurbocallError::IsaError)? + .finish(flags)?; let mut wrapper_sig = cranelift::codegen::ir::Signature::new(isa.default_call_conv()); let mut target_sig = cranelift::codegen::ir::Signature::new(isa.default_call_conv()); + let mut raise_sig = + cranelift::codegen::ir::Signature::new(isa.default_call_conv()); #[cfg(target_pointer_width = "32")] const ISIZE: Type = types::I32; #[cfg(target_pointer_width = "64")] const ISIZE: Type = types::I64; - // Local receiver - wrapper_sig.params.push(AbiParam::new(ISIZE)); - - fn convert(t: &NativeType) -> AbiParam { + fn convert(t: &NativeType, wrapper: bool) -> AbiParam { match t { - NativeType::U8 => AbiParam::new(types::I8).uext(), - NativeType::I8 => AbiParam::new(types::I8).sext(), - NativeType::U16 => AbiParam::new(types::I16).uext(), - NativeType::I16 => AbiParam::new(types::I16).sext(), + NativeType::U8 => { + if wrapper { + AbiParam::new(types::I32) + } else { + AbiParam::new(types::I8).uext() + } + } + NativeType::I8 => { + if wrapper { + AbiParam::new(types::I32) + } else { + AbiParam::new(types::I8).sext() + } + } + NativeType::U16 => { + if wrapper { + AbiParam::new(types::I32) + } else { + AbiParam::new(types::I16).uext() + } + } + NativeType::I16 => { + if wrapper { + AbiParam::new(types::I32) + } else { + AbiParam::new(types::I16).sext() + } + } + NativeType::Bool => { + if wrapper { + AbiParam::new(types::I32) + } else { + AbiParam::new(types::I8).uext() + } + } NativeType::U32 => AbiParam::new(types::I32), NativeType::I32 => AbiParam::new(types::I32), NativeType::U64 => AbiParam::new(types::I64), @@ -64,7 +125,6 @@ pub(crate) fn compile_trampoline(sym: &Symbol) -> Trampoline { NativeType::ISize => AbiParam::new(ISIZE), NativeType::F32 => AbiParam::new(types::F32), NativeType::F64 => AbiParam::new(types::F64), - NativeType::Bool => AbiParam::new(types::I8).uext(), NativeType::Pointer => AbiParam::new(ISIZE), NativeType::Buffer => AbiParam::new(ISIZE), NativeType::Function => AbiParam::new(ISIZE), @@ -73,27 +133,23 @@ pub(crate) fn compile_trampoline(sym: &Symbol) -> Trampoline { } } - for pty in &sym.parameter_types { - let param = convert(pty); + // *const FastApiCallbackOptions + raise_sig.params.push(AbiParam::new(ISIZE)); - target_sig.params.push(param); + // Local receiver + wrapper_sig.params.push(AbiParam::new(ISIZE)); - if param.value_type == types::I8 || param.value_type == types::I16 { - wrapper_sig.params.push(AbiParam::new(types::I32)); - } else { - wrapper_sig.params.push(param); - } + for pty in &sym.parameter_types { + target_sig.params.push(convert(pty, false)); + wrapper_sig.params.push(convert(pty, true)); } - let param = convert(&sym.result_type); - if param.value_type != types::INVALID { - target_sig.returns.push(param); + // const FastApiCallbackOptions& options + wrapper_sig.params.push(AbiParam::new(ISIZE)); - if param.value_type == types::I8 || param.value_type == types::I16 { - wrapper_sig.returns.push(AbiParam::new(types::I32)); - } else { - wrapper_sig.returns.push(param); - } + if !matches!(sym.result_type, NativeType::Struct(_) | NativeType::Void) { + target_sig.returns.push(convert(&sym.result_type, false)); + wrapper_sig.returns.push(convert(&sym.result_type, true)); } let mut ab_sig = @@ -105,7 +161,10 @@ pub(crate) fn compile_trampoline(sym: &Symbol) -> Trampoline { let mut fn_builder_ctx = FunctionBuilderContext::new(); ctx.func = cranelift::codegen::ir::Function::with_name_signature( - cranelift::codegen::ir::UserFuncName::user(0, 0), + cranelift::codegen::ir::UserFuncName::testcase(format!( + "{}_wrapper", + sym.name + )), wrapper_sig, ); @@ -113,37 +172,101 @@ pub(crate) fn compile_trampoline(sym: &Symbol) -> Trampoline { let target_sig = f.import_signature(target_sig); let ab_sig = f.import_signature(ab_sig); + let raise_sig = f.import_signature(raise_sig); { - let block = f.create_block(); - f.append_block_params_for_function_params(block); - f.switch_to_block(block); + // Define blocks + + let entry = f.create_block(); + f.append_block_params_for_function_params(entry); + + let error = f.create_block(); + f.set_cold_block(error); - let args = f.block_params(block); - let mut args = (args[1..]).to_owned(); + // Define variables + + let mut vidx = 0; + for pt in &sym.parameter_types { + let target_v = Variable::new(vidx); + vidx += 1; + + let wrapper_v = Variable::new(vidx); + vidx += 1; + + f.declare_var(target_v, convert(pt, false).value_type); + f.declare_var(wrapper_v, convert(pt, true).value_type); + } + + let options_v = Variable::new(vidx); + vidx += 1; + f.declare_var(options_v, ISIZE); + + // Go! + + f.switch_to_block(entry); + f.seal_block(entry); + + let args = f.block_params(entry).to_owned(); + + let mut vidx = 1; + let mut argx = 1; + for _ in &sym.parameter_types { + f.def_var(Variable::new(vidx), args[argx]); + argx += 1; + vidx += 2; + } + + f.def_var(options_v, args[argx]); + + let mut next = f.create_block(); + + let mut vidx = 0; + for nty in &sym.parameter_types { + let target_v = Variable::new(vidx); + vidx += 1; + let wrapper_v = Variable::new(vidx); + vidx += 1; + + let arg = f.use_var(wrapper_v); - for (arg, nty) in args.iter_mut().zip(&sym.parameter_types) { match nty { NativeType::U8 | NativeType::I8 | NativeType::Bool => { - *arg = f.ins().ireduce(types::I8, *arg); + let v = f.ins().ireduce(types::I8, arg); + f.def_var(target_v, v); } NativeType::U16 | NativeType::I16 => { - *arg = f.ins().ireduce(types::I16, *arg); + let v = f.ins().ireduce(types::I16, arg); + f.def_var(target_v, v); } NativeType::Buffer => { - let callee = f.ins().iconst( - ISIZE, - Imm64::new(turbocall_ab_contents as usize as isize as i64), - ); - let call = f.ins().call_indirect(ab_sig, callee, &[*arg]); - let results = f.inst_results(call); - *arg = results[0]; + let callee = + f.ins().iconst(ISIZE, turbocall_ab_contents as usize as i64); + let call = f.ins().call_indirect(ab_sig, callee, &[arg]); + let result = f.inst_results(call)[0]; + f.def_var(target_v, result); + + let sentinel = f.ins().iconst(ISIZE, isize::MAX as i64); + let condition = f.ins().icmp(IntCC::Equal, result, sentinel); + f.ins().brif(condition, error, &[], next, &[]); + + // switch to new block + f.switch_to_block(next); + f.seal_block(next); + next = f.create_block(); + } + _ => { + f.def_var(target_v, arg); } - _ => {} } } - let callee = f.ins().iconst(ISIZE, Imm64::new(sym.ptr.as_ptr() as i64)); + let mut args = Vec::with_capacity(sym.parameter_types.len()); + let mut vidx = 0; + for _ in &sym.parameter_types { + args.push(f.use_var(Variable::new(vidx))); + vidx += 2; // skip wrapper arg + } + let callee = f.ins().iconst(ISIZE, sym.ptr.as_ptr() as i64); let call = f.ins().call_indirect(target_sig, callee, &args); let mut results = f.inst_results(call).to_owned(); @@ -158,22 +281,48 @@ pub(crate) fn compile_trampoline(sym: &Symbol) -> Trampoline { } f.ins().return_(&results); + + f.switch_to_block(error); + f.seal_block(error); + if !f.is_unreachable() { + let options = f.use_var(options_v); + let callee = f.ins().iconst(ISIZE, turbocall_raise as usize as i64); + f.ins().call_indirect(raise_sig, callee, &[options]); + let rty = convert(&sym.result_type, true); + if rty.value_type.is_invalid() { + f.ins().return_(&[]); + } else { + let zero = if rty.value_type == types::F32 { + f.ins().f32const(0.0) + } else if rty.value_type == types::F64 { + f.ins().f64const(0.0) + } else { + f.ins().iconst(rty.value_type, 0) + }; + f.ins().return_(&[zero]); + } + } } - f.seal_all_blocks(); f.finalize(); - cranelift::codegen::verifier::verify_function(&ctx.func, isa.flags()) - .unwrap(); + cranelift::codegen::verifier::verify_function(&ctx.func, isa.flags())?; - let code_info = ctx.compile(&*isa, &mut Default::default()).unwrap(); + let mut ctrl_plane = Default::default(); + ctx.optimize(&*isa, &mut ctrl_plane)?; + + log::trace!("Turbocall IR:\n{}", ctx.func.display()); + + let code_info = ctx + .compile(&*isa, &mut ctrl_plane) + .map_err(|e| TurbocallError::CompileError(format!("{e:?}")))?; let data = code_info.buffer.data(); - let mut mutable = memmap2::MmapMut::map_anon(data.len()).unwrap(); + let mut mutable = memmap2::MmapMut::map_anon(data.len())?; mutable.copy_from_slice(data); - let buffer = mutable.make_exec().unwrap(); + let buffer = mutable.make_exec()?; - Trampoline(buffer) + Ok(Trampoline(buffer)) } pub(crate) struct Turbocall { @@ -189,6 +338,7 @@ pub(crate) struct Turbocall { pub(crate) fn make_template(sym: &Symbol, trampoline: Trampoline) -> Turbocall { let param_info = std::iter::once(fast_api::Type::V8Value.as_info()) // Receiver .chain(sym.parameter_types.iter().map(|t| t.into())) + .chain(std::iter::once(fast_api::Type::CallbackOptions.as_info())) .collect::>(); let ret = if sym.result_type == NativeType::Buffer { @@ -239,17 +389,17 @@ impl From<&NativeType> for fast_api::CTypeInfo { extern "C" fn turbocall_ab_contents( v: deno_core::v8::Local, -) -> *mut u8 { - let v = v.cast::(); - const { - // We don't keep `buffer` around when this function returns, - // so assert that it will be unused. - assert!(deno_core::v8::TYPED_ARRAY_MAX_SIZE_IN_HEAP == 0); - } - let mut buffer = [0; deno_core::v8::TYPED_ARRAY_MAX_SIZE_IN_HEAP]; - // SAFETY: `buffer` is unused due to above, returned pointer is not - // dereferenced by rust code, and we keep it alive at least as long - // as the turbocall. - let (data, _) = unsafe { v.get_contents_raw_parts(&mut buffer) }; - data +) -> *mut c_void { + super::ir::parse_buffer_arg(v).unwrap_or(isize::MAX as _) +} + +unsafe extern "C" fn turbocall_raise( + options: *const deno_core::v8::fast_api::FastApiCallbackOptions, +) { + let mut scope = deno_core::v8::CallbackScope::new(&*options); + let exception = deno_core::error::to_v8_error( + &mut scope, + &crate::IRError::InvalidBufferType, + ); + scope.throw_exception(exception); } diff --git a/tests/ffi/tests/test.js b/tests/ffi/tests/test.js index b767757df3abb0..c46b7c6d9a0457 100644 --- a/tests/ffi/tests/test.js +++ b/tests/ffi/tests/test.js @@ -364,16 +364,9 @@ assertEquals(isNullBuffer(emptyBuffer), true, "isNullBuffer(emptyBuffer) !== tru assertEquals(isNullBufferDeopt(emptyBuffer), true, "isNullBufferDeopt(emptyBuffer) !== true"); assertEquals(isNullBuffer(emptySlice), false, "isNullBuffer(emptySlice) !== false"); assertEquals(isNullBufferDeopt(emptySlice), false, "isNullBufferDeopt(emptySlice) !== false"); +assertEquals(isNullBuffer(new Uint8Array()), true, "isNullBuffer(new Uint8Array()) !== false"); assertEquals(isNullBufferDeopt(new Uint8Array()), true, "isNullBufferDeopt(new Uint8Array()) !== true"); -// ==== V8 ZERO LENGTH BUFFER ANOMALIES ==== - -// V8 bug: inline Uint8Array creation to fast call sees non-null pointer -// https://bugs.chromium.org/p/v8/issues/detail?id=13489 -if (Deno.build.os != "linux" || Deno.build.arch != "aarch64") { - assertEquals(isNullBuffer(new Uint8Array()), false, "isNullBuffer(new Uint8Array()) !== false"); -} - // Externally backed ArrayBuffer has a non-null data pointer, even though its length is zero. const externalZeroBuffer = new Uint8Array(Deno.UnsafePointerView.getArrayBuffer(ptr0, 0)); // V8 Fast calls used to get null pointers for all zero-sized buffers no matter their external backing.