From 4d2044b0109ac1cfa77e6bd9b7f3e287fc7fcacc Mon Sep 17 00:00:00 2001 From: PThorpe92 Date: Mon, 17 Feb 2025 08:15:57 -0500 Subject: [PATCH] Fix ownership semantics in extention value conversions --- core/ext/mod.rs | 4 +- core/lib.rs | 7 +-- core/translate/emitter.rs | 102 ++++++++------------------------------ core/types.rs | 11 ++-- core/vdbe/mod.rs | 14 +++--- 5 files changed, 41 insertions(+), 97 deletions(-) diff --git a/core/ext/mod.rs b/core/ext/mod.rs index a5cfd5909..1402c4098 100644 --- a/core/ext/mod.rs +++ b/core/ext/mod.rs @@ -11,7 +11,7 @@ type ExternAggFunc = (InitAggFunction, StepFunction, FinalizeFunction); #[derive(Clone)] pub struct VTabImpl { - pub module_type: VTabKind, + pub module_kind: VTabKind, pub implementation: Rc, } @@ -104,7 +104,7 @@ impl Database { ) -> ResultCode { let module = Rc::new(module); let vmodule = VTabImpl { - module_type: kind, + module_kind: kind, implementation: module, }; self.syms diff --git a/core/lib.rs b/core/lib.rs index ed45d344e..3583ad5d5 100644 --- a/core/lib.rs +++ b/core/lib.rs @@ -542,7 +542,7 @@ impl VirtualTable { module_name )))?; if let VTabKind::VirtualTable = kind { - if module.module_type != VTabKind::VirtualTable { + if module.module_kind != VTabKind::VirtualTable { return Err(LimboError::ExtensionError(format!( "Virtual table module {} is not a virtual table", module_name @@ -612,10 +612,7 @@ impl VirtualTable { pub fn column(&self, cursor: &VTabOpaqueCursor, column: usize) -> Result { let val = unsafe { (self.implementation.column)(cursor.as_ptr(), column as u32) }; - let res = OwnedValue::from_ffi(&val)?; - unsafe { - val.free(); - } + let res = OwnedValue::from_ffi(val)?; Ok(res) } diff --git a/core/translate/emitter.rs b/core/translate/emitter.rs index 8d095eb9b..ca7162af1 100644 --- a/core/translate/emitter.rs +++ b/core/translate/emitter.rs @@ -308,13 +308,7 @@ fn emit_program_for_delete( &plan.table_references, &plan.where_clause, )?; - if let Some(table) = plan.table_references.first() { - if table.virtual_table().is_some() { - emit_delete_vtable_insns(program, &mut t_ctx, &plan.table_references, &plan.limit)?; - } else { - emit_delete_insns(program, &mut t_ctx, &plan.table_references, &plan.limit)?; - } - } + emit_delete_insns(program, &mut t_ctx, &plan.table_references, &plan.limit)?; // Clean up and close the main execution loop close_loop(program, &mut t_ctx, &plan.table_references)?; @@ -328,77 +322,6 @@ fn emit_program_for_delete( Ok(()) } -fn emit_delete_vtable_insns( - program: &mut ProgramBuilder, - t_ctx: &mut TranslateCtx, - table_references: &[TableReference], - limit: &Option, -) -> Result<()> { - let table_reference = table_references.first().unwrap(); - - let cursor_id = match &table_reference.op { - Operation::Scan { .. } => program.resolve_cursor_id(&table_reference.identifier), - Operation::Search(search) => match search { - Search::RowidEq { .. } | Search::RowidSearch { .. } => { - program.resolve_cursor_id(&table_reference.identifier) - } - Search::IndexSearch { index, .. } => program.resolve_cursor_id(&index.name), - }, - _ => return Ok(()), - }; - - let rowid_reg = program.alloc_register(); - program.emit_insn(Insn::RowId { - cursor_id, - dest: rowid_reg, - }); - // if we have a limit, decrement and check zero - if let Some(limit) = limit { - let limit_reg = program.alloc_register(); - program.emit_insn(Insn::Integer { - value: *limit as i64, - dest: limit_reg, - }); - program.mark_last_insn_constant(); - - program.emit_insn(Insn::DecrJumpZero { - reg: limit_reg, - target_pc: t_ctx.label_main_loop_end.unwrap(), - }); - } - - // we want old_rowid= rowid_reg, new_rowid= NULL, so we pass 2 arguments to VUpdate - // we need a second register for the new rowid = NULL - let new_rowid_reg = program.alloc_register(); - - program.emit_insn(Insn::Null { - dest: new_rowid_reg, - dest_end: None, - }); - - // we'll do VUpdate with arg_count=2: - // argv[0] => old_rowid = rowid_reg - // argv[1] => new_rowid = new_rowid_reg (NULL) - - let Some(virtual_table) = table_reference.virtual_table() else { - return Err(crate::LimboError::ParseError( - "Table is not a virtual table".to_string(), - )); - }; - let conflict_action = 0u16; - let start_reg = rowid_reg; - - program.emit_insn(Insn::VUpdate { - cursor_id, - arg_count: 2, - start_reg, - vtab_ptr: virtual_table.implementation.as_ref().ctx as usize, - conflict_action, - }); - - Ok(()) -} - fn emit_delete_insns( program: &mut ProgramBuilder, t_ctx: &mut TranslateCtx, @@ -423,8 +346,27 @@ fn emit_delete_insns( cursor_id, dest: key_reg, }); - program.emit_insn(Insn::DeleteAsync { cursor_id }); - program.emit_insn(Insn::DeleteAwait { cursor_id }); + + if let Some(vtab) = table_reference.virtual_table() { + let conflict_action = 0u16; + let start_reg = key_reg; + + let new_rowid_reg = program.alloc_register(); + program.emit_insn(Insn::Null { + dest: new_rowid_reg, + dest_end: None, + }); + program.emit_insn(Insn::VUpdate { + cursor_id, + arg_count: 2, + start_reg, + vtab_ptr: vtab.implementation.as_ref().ctx as usize, + conflict_action, + }); + } else { + program.emit_insn(Insn::DeleteAsync { cursor_id }); + program.emit_insn(Insn::DeleteAwait { cursor_id }); + } if let Some(limit) = limit { let limit_reg = program.alloc_register(); program.emit_insn(Insn::Integer { diff --git a/core/types.rs b/core/types.rs index 7eee76e94..f1dafd31e 100644 --- a/core/types.rs +++ b/core/types.rs @@ -223,8 +223,8 @@ impl OwnedValue { } } - pub fn from_ffi(v: &ExtValue) -> Result { - match v.value_type() { + pub fn from_ffi(v: ExtValue) -> Result { + let res = match v.value_type() { ExtValueType::Null => Ok(OwnedValue::Null), ExtValueType::Integer => { let Some(int) = v.to_integer() else { @@ -259,7 +259,11 @@ impl OwnedValue { (code, None) => Err(LimboError::ExtensionError(code.to_string())), } } + }; + unsafe { + v.free(); } + res } } @@ -281,8 +285,7 @@ impl AggContext { if let Self::External(ext_state) = self { if ext_state.finalized_value.is_none() { let final_value = unsafe { (ext_state.finalize_fn)(ext_state.state) }; - ext_state.cache_final_value(OwnedValue::from_ffi(&final_value)?); - unsafe { final_value.free() }; + ext_state.cache_final_value(OwnedValue::from_ffi(final_value)?); } } Ok(()) diff --git a/core/vdbe/mod.rs b/core/vdbe/mod.rs index fc980e85b..a45ea199c 100644 --- a/core/vdbe/mod.rs +++ b/core/vdbe/mod.rs @@ -169,13 +169,11 @@ macro_rules! call_external_function { ) => {{ if $arg_count == 0 { let result_c_value: ExtValue = unsafe { ($func_ptr)(0, std::ptr::null()) }; - match OwnedValue::from_ffi(&result_c_value) { + match OwnedValue::from_ffi(result_c_value) { Ok(result_ov) => { $state.registers[$dest_register] = result_ov; - unsafe { result_c_value.free() }; } Err(e) => { - unsafe { result_c_value.free() }; return Err(e); } } @@ -188,13 +186,14 @@ macro_rules! call_external_function { } let argv_ptr = ext_values.as_ptr(); let result_c_value: ExtValue = unsafe { ($func_ptr)($arg_count as i32, argv_ptr) }; - match OwnedValue::from_ffi(&result_c_value) { + for arg in ext_values { + unsafe { arg.free() }; + } + match OwnedValue::from_ffi(result_c_value) { Ok(result_ov) => { $state.registers[$dest_register] = result_ov; - unsafe { result_c_value.free() }; } Err(e) => { - unsafe { result_c_value.free() }; return Err(e); } } @@ -1858,6 +1857,9 @@ impl Program { } let argv_ptr = ext_values.as_ptr(); unsafe { step_fn(state_ptr, argc as i32, argv_ptr) }; + for ext_value in ext_values { + unsafe { ext_value.free() }; + } } } };