From 018a0e59d5f597df6a81695b14f066fe9ea4ddee Mon Sep 17 00:00:00 2001 From: swananan Date: Fri, 22 May 2026 21:18:31 +0800 Subject: [PATCH] refactor: replace compiler unwraps with fallible errors --- ghostscope-compiler/src/ebpf/codegen.rs | 12 +-- ghostscope-compiler/src/ebpf/context.rs | 27 ++++--- ghostscope-compiler/src/ebpf/dwarf_bridge.rs | 12 ++- ghostscope-compiler/src/ebpf/expression.rs | 27 +++---- .../src/ebpf/helper_functions.rs | 69 +++++------------ .../src/script/format_validator.rs | 74 +++++-------------- 6 files changed, 70 insertions(+), 151 deletions(-) diff --git a/ghostscope-compiler/src/ebpf/codegen.rs b/ghostscope-compiler/src/ebpf/codegen.rs index d240f141..b353bea1 100644 --- a/ghostscope-compiler/src/ebpf/codegen.rs +++ b/ghostscope-compiler/src/ebpf/codegen.rs @@ -2729,8 +2729,7 @@ impl<'ctx, 'dw> EbpfContext<'ctx, 'dw> { .builder .build_and(ok_pred, offsets_found, "md_ok_with_offsets") .map_err(|e| CodeGenError::LLVMError(e.to_string()))?; - let curr = self.builder.get_insert_block().unwrap(); - let func = curr.get_parent().unwrap(); + let func = self.current_function("compile memdump status branch")?; let ok_b = self.context.append_basic_block(func, "md_ok"); let err_b = self.context.append_basic_block(func, "md_err"); let cont_b = self.context.append_basic_block(func, "md_cont"); @@ -2861,8 +2860,7 @@ impl<'ctx, 'dw> EbpfContext<'ctx, 'dw> { .into_int_value(); // If effective length is zero, mark status and skip read. - let curr = self.builder.get_insert_block().unwrap(); - let func = curr.get_parent().unwrap(); + let func = self.current_function("compile memdump dynamic length branch")?; let zero_b = self.context.append_basic_block(func, "mdd_len_zero"); let read_b = self.context.append_basic_block(func, "mdd_len_read"); let cont_b = self.context.append_basic_block(func, "mdd_cont"); @@ -3229,8 +3227,7 @@ impl<'ctx, 'dw> EbpfContext<'ctx, 'dw> { module_for_offsets.as_deref(), )?; let offsets_found = src_addr.offsets_found; - let current_block = self.builder.get_insert_block().unwrap(); - let current_fn = current_block.get_parent().unwrap(); + let current_fn = self.current_function("compile complex variable read")?; let cont2_block = self.context.append_basic_block(current_fn, "after_read"); let skip_block = self.context.append_basic_block(current_fn, "offsets_skip"); let found_block = self.context.append_basic_block(current_fn, "offsets_found"); @@ -4651,8 +4648,7 @@ impl<'ctx, 'dw> EbpfContext<'ctx, 'dw> { .build_int_to_ptr(src_addr.value, ptr_type, "src_ptr") .map_err(|e| CodeGenError::LLVMError(e.to_string()))?; let offsets_found = src_addr.offsets_found; - let current_block = self.builder.get_insert_block().unwrap(); - let current_fn = current_block.get_parent().unwrap(); + let current_fn = self.current_function("generate print complex variable runtime")?; let cont_block = self.context.append_basic_block(current_fn, "after_read"); let skip_block = self.context.append_basic_block(current_fn, "offsets_skip"); let found_block = self.context.append_basic_block(current_fn, "offsets_found"); diff --git a/ghostscope-compiler/src/ebpf/context.rs b/ghostscope-compiler/src/ebpf/context.rs index 849892ff..52947bf8 100644 --- a/ghostscope-compiler/src/ebpf/context.rs +++ b/ghostscope-compiler/src/ebpf/context.rs @@ -6,6 +6,7 @@ use super::maps::MapManager; use crate::script::{VarType, VariableContext}; use ghostscope_dwarf::DwarfAnalyzer; +use inkwell::basic_block::BasicBlock; use inkwell::builder::Builder; use inkwell::context::Context; use inkwell::debug_info::DebugInfoBuilder; @@ -424,14 +425,21 @@ impl<'ctx, 'dw> EbpfContext<'ctx, 'dw> { self.trace_context.clone() } - /// Get pt_regs parameter from current function - pub fn get_pt_regs_parameter(&self) -> Result> { - let current_function = self - .builder + pub(crate) fn current_insert_block(&self, op: &str) -> Result> { + self.builder .get_insert_block() - .ok_or_else(|| CodeGenError::Builder("No current basic block".to_string()))? + .ok_or_else(|| CodeGenError::Builder(format!("{op} requires an active insert block"))) + } + + pub(crate) fn current_function(&self, op: &str) -> Result> { + self.current_insert_block(op)? .get_parent() - .ok_or_else(|| CodeGenError::Builder("No parent function".to_string()))?; + .ok_or_else(|| CodeGenError::Builder(format!("{op} requires a parent function"))) + } + + /// Get pt_regs parameter from current function + pub fn get_pt_regs_parameter(&self) -> Result> { + let current_function = self.current_function("get pt_regs parameter")?; let pt_regs_param = current_function .get_first_param() @@ -660,12 +668,7 @@ impl<'ctx, 'dw> EbpfContext<'ctx, 'dw> { info!("Adding host TGID filter for filter PID: {}", filter_pid); // Get current function and entry block - let current_fn = self - .builder - .get_insert_block() - .unwrap() - .get_parent() - .unwrap(); + let current_fn = self.current_function("add host pid filter")?; // Create basic blocks for control flow let continue_block = self diff --git a/ghostscope-compiler/src/ebpf/dwarf_bridge.rs b/ghostscope-compiler/src/ebpf/dwarf_bridge.rs index 09458faa..01741a40 100644 --- a/ghostscope-compiler/src/ebpf/dwarf_bridge.rs +++ b/ghostscope-compiler/src/ebpf/dwarf_bridge.rs @@ -955,12 +955,7 @@ impl<'ctx, 'dw> EbpfContext<'ctx, 'dw> { ) .map_err(|e| CodeGenError::LLVMError(e.to_string()))?; - let cur_fn = self - .builder - .get_insert_block() - .unwrap() - .get_parent() - .unwrap(); + let cur_fn = self.current_function("generate dereference runtime check")?; let null_bb = self.context.append_basic_block(cur_fn, "deref_null"); let read_bb = self.context.append_basic_block(cur_fn, "deref_read"); let cont_bb = self.context.append_basic_block(cur_fn, "deref_cont"); @@ -1142,7 +1137,10 @@ impl<'ctx, 'dw> EbpfContext<'ctx, 'dw> { } if stack.len() == 1 { - Ok(stack.pop().unwrap()) + let value = stack.pop().ok_or_else(|| { + CodeGenError::LLVMError("Stack underflow after runtime computation".to_string()) + })?; + Ok(value) } else { Err(CodeGenError::LLVMError(format!( "Invalid stack state after computation: {} elements remaining", diff --git a/ghostscope-compiler/src/ebpf/expression.rs b/ghostscope-compiler/src/ebpf/expression.rs index 4bbf3ac6..d1d526d7 100644 --- a/ghostscope-compiler/src/ebpf/expression.rs +++ b/ghostscope-compiler/src/ebpf/expression.rs @@ -968,8 +968,7 @@ impl<'ctx, 'dw> EbpfContext<'ctx, 'dw> { "memcmp_len_is_zero", ) .map_err(|e| CodeGenError::Builder(e.to_string()))?; - let curr_block = self.builder.get_insert_block().unwrap(); - let func = curr_block.get_parent().unwrap(); + let func = self.current_function("compile memcmp length branch")?; let zero_b = self.context.append_basic_block(func, "memcmp_len_zero"); let nz_b = self.context.append_basic_block(func, "memcmp_len_nz"); let cont_b = self.context.append_basic_block(func, "memcmp_len_cont"); @@ -983,7 +982,7 @@ impl<'ctx, 'dw> EbpfContext<'ctx, 'dw> { self.builder .build_unconditional_branch(cont_b) .map_err(|e| CodeGenError::Builder(e.to_string()))?; - let zero_block = self.builder.get_insert_block().unwrap(); + let zero_block = self.current_insert_block("finish memcmp zero-length block")?; // Non-zero branch: perform reads and compare self.builder.position_at_end(nz_b); @@ -1192,8 +1191,7 @@ impl<'ctx, 'dw> EbpfContext<'ctx, 'dw> { .builder .build_or(not_a, not_b, "memcmp_any_fail") .map_err(|e| CodeGenError::Builder(e.to_string()))?; - let cur_block = self.builder.get_insert_block().unwrap(); - let func = cur_block.get_parent().unwrap(); + let func = self.current_function("compile memcmp condition error branch")?; let set_b = self.context.append_basic_block(func, "memcmp_set_err"); let cont_b = self.context.append_basic_block(func, "memcmp_cont"); self.builder @@ -1211,12 +1209,7 @@ impl<'ctx, 'dw> EbpfContext<'ctx, 'dw> { .builder .build_not(ok_b, "memcmp_fail_b_val") .map_err(|e| CodeGenError::Builder(e.to_string()))?; - let cur_fn = self - .builder - .get_insert_block() - .unwrap() - .get_parent() - .unwrap(); + let cur_fn = self.current_function("compile memcmp failure address branch")?; let set_a_bb = self.context.append_basic_block(cur_fn, "set_addr_a"); let check_b_bb = self.context.append_basic_block(cur_fn, "check_fail_b"); let set_b_bb = self.context.append_basic_block(cur_fn, "set_addr_b"); @@ -1394,7 +1387,7 @@ impl<'ctx, 'dw> EbpfContext<'ctx, 'dw> { self.builder .build_unconditional_branch(cont_b) .map_err(|e| CodeGenError::Builder(e.to_string()))?; - let nz_block = self.builder.get_insert_block().unwrap(); + let nz_block = self.current_insert_block("finish memcmp non-zero block")?; // Merge self.builder.position_at_end(cont_b); @@ -1639,8 +1632,7 @@ impl<'ctx, 'dw> EbpfContext<'ctx, 'dw> { let (bounded_len, len_is_zero) = self.compile_bounded_compare_len_i32(n_expr, cmp_bound, "strncmp")?; - let curr_block = self.builder.get_insert_block().unwrap(); - let func = curr_block.get_parent().unwrap(); + let func = self.current_function("compile strncmp length branch")?; let zero_b = self.context.append_basic_block(func, "strncmp_len_zero"); let nz_b = self.context.append_basic_block(func, "strncmp_len_nz"); let final_b = self.context.append_basic_block(func, "strncmp_len_cont"); @@ -1653,7 +1645,7 @@ impl<'ctx, 'dw> EbpfContext<'ctx, 'dw> { self.builder .build_unconditional_branch(final_b) .map_err(|e| CodeGenError::Builder(e.to_string()))?; - let zero_block = self.builder.get_insert_block().unwrap(); + let zero_block = self.current_insert_block("finish strncmp zero-length block")?; self.builder.position_at_end(nz_b); @@ -1711,8 +1703,7 @@ impl<'ctx, 'dw> EbpfContext<'ctx, 'dw> { // If in condition context and read failed, set condition error code = 1 (ProbeReadFailed) if self.condition_context_active { - let cur_block = self.builder.get_insert_block().unwrap(); - let func = cur_block.get_parent().unwrap(); + let func = self.current_function("compile strncmp condition error branch")?; let set_b = self.context.append_basic_block(func, "strncmp_set_err"); let cont_b = self.context.append_basic_block(func, "strncmp_cont"); let not_ok = self @@ -1800,7 +1791,7 @@ impl<'ctx, 'dw> EbpfContext<'ctx, 'dw> { self.builder .build_unconditional_branch(final_b) .map_err(|e| CodeGenError::Builder(e.to_string()))?; - let nz_block = self.builder.get_insert_block().unwrap(); + let nz_block = self.current_insert_block("finish strncmp non-zero block")?; self.builder.position_at_end(final_b); let result_phi = self diff --git a/ghostscope-compiler/src/ebpf/helper_functions.rs b/ghostscope-compiler/src/ebpf/helper_functions.rs index 6ac9395b..5608ff04 100644 --- a/ghostscope-compiler/src/ebpf/helper_functions.rs +++ b/ghostscope-compiler/src/ebpf/helper_functions.rs @@ -173,12 +173,7 @@ impl<'ctx, 'dw> EbpfContext<'ctx, 'dw> { } }; - let helper_fn = self - .builder - .get_insert_block() - .unwrap() - .get_parent() - .unwrap(); + let helper_fn = self.current_function("lookup proc pid alias")?; let alias_hit_block = self .context .append_basic_block(helper_fn, &format!("{name_prefix}_alias_hit")); @@ -220,13 +215,13 @@ impl<'ctx, 'dw> EbpfContext<'ctx, 'dw> { self.builder .build_unconditional_branch(alias_cont_block) .map_err(|e| CodeGenError::Builder(e.to_string()))?; - let alias_hit_end = self.builder.get_insert_block().unwrap(); + let alias_hit_end = self.current_insert_block("finish pid alias hit block")?; self.builder.position_at_end(alias_miss_block); self.builder .build_unconditional_branch(alias_cont_block) .map_err(|e| CodeGenError::Builder(e.to_string()))?; - let alias_miss_end = self.builder.get_insert_block().unwrap(); + let alias_miss_end = self.current_insert_block("finish pid alias miss block")?; self.builder.position_at_end(alias_cont_block); let alias_phi = self @@ -637,30 +632,10 @@ impl<'ctx, 'dw> EbpfContext<'ctx, 'dw> { .ok_or_else(|| CodeGenError::LLVMError("map_lookup_elem returned void".to_string()))?; let null_ptr = ptr_type.const_null(); - let found_block = self.context.append_basic_block( - self.builder - .get_insert_block() - .unwrap() - .get_parent() - .unwrap(), - "found_offsets", - ); - let miss_block = self.context.append_basic_block( - self.builder - .get_insert_block() - .unwrap() - .get_parent() - .unwrap(), - "miss_offsets", - ); - let cont_block = self.context.append_basic_block( - self.builder - .get_insert_block() - .unwrap() - .get_parent() - .unwrap(), - "cont_offsets", - ); + let current_fn = self.current_function("generate module offset lookup")?; + let found_block = self.context.append_basic_block(current_fn, "found_offsets"); + let miss_block = self.context.append_basic_block(current_fn, "miss_offsets"); + let cont_block = self.context.append_basic_block(current_fn, "cont_offsets"); // Compare against NULL let val_ptr = if let BasicValueEnum::PointerValue(p) = val_ptr_any { @@ -1107,8 +1082,7 @@ impl<'ctx, 'dw> EbpfContext<'ctx, 'dw> { .. } = self.probe_read_user_core(address, size, "probe_read_user_cf")?; - let cur_block = self.builder.get_insert_block().unwrap(); - let func = cur_block.get_parent().unwrap(); + let func = self.current_function("generate memory read with status")?; let set_block = self.context.append_basic_block(func, "set_cond_err"); let cont_block = self.context.append_basic_block(func, "read_cont"); self.builder @@ -1135,25 +1109,20 @@ impl<'ctx, 'dw> EbpfContext<'ctx, 'dw> { /// Map DWARF register number to pt_regs offset (simplified) pub fn dwarf_reg_to_pt_regs_offset(&self, dwarf_reg: u16) -> Result { // Use platform-specific register mapping to get byte offset - let byte_offset = register_mapping::dwarf_reg_to_pt_regs_byte_offset(dwarf_reg).ok_or_else( - || { - let reg_name = register_mapping::dwarf_reg_to_name(dwarf_reg); - if matches!(reg_name, Some(name) if name.starts_with("XMM")) { + let byte_offset = register_mapping::dwarf_reg_to_pt_regs_byte_offset(dwarf_reg) + .ok_or_else(|| match register_mapping::dwarf_reg_to_name(dwarf_reg) { + Some(reg_name) if reg_name.starts_with("XMM") => { CodeGenError::RegisterMappingError(format!( - "Unsupported DWARF register: {dwarf_reg} ({}) is a SIMD/FP register; uprobe pt_regs does not expose XMM register values, so optimized float by-value parameters are unavailable unless the compiler spills them to memory", - reg_name.unwrap() - )) - } else if let Some(reg_name) = reg_name { - CodeGenError::RegisterMappingError(format!( - "Unsupported DWARF register: {dwarf_reg} ({reg_name})" - )) - } else { - CodeGenError::RegisterMappingError(format!( - "Unsupported DWARF register: {dwarf_reg}" + "Unsupported DWARF register: {dwarf_reg} ({reg_name}) is a SIMD/FP register; uprobe pt_regs does not expose XMM register values, so optimized float by-value parameters are unavailable unless the compiler spills them to memory" )) } - }, - )?; + Some(reg_name) => CodeGenError::RegisterMappingError(format!( + "Unsupported DWARF register: {dwarf_reg} ({reg_name})" + )), + None => CodeGenError::RegisterMappingError(format!( + "Unsupported DWARF register: {dwarf_reg}" + )), + })?; // Convert byte offset to u64 array index for pt_regs access let u64_index = byte_offset / core::mem::size_of::(); diff --git a/ghostscope-compiler/src/script/format_validator.rs b/ghostscope-compiler/src/script/format_validator.rs index a3ef7c1b..a345445d 100644 --- a/ghostscope-compiler/src/script/format_validator.rs +++ b/ghostscope-compiler/src/script/format_validator.rs @@ -153,32 +153,23 @@ mod tests { use crate::script::ast::Expr; #[test] - fn test_count_placeholders() { + fn test_count_placeholders() -> Result<(), ParseError> { // Basic cases + assert_eq!(FormatValidator::count_required_args("hello world")?, (0, 0)); + assert_eq!(FormatValidator::count_required_args("hello {}")?, (1, 0)); + assert_eq!(FormatValidator::count_required_args("{} {}")?, (2, 0)); assert_eq!( - FormatValidator::count_required_args("hello world").unwrap(), - (0, 0) - ); - assert_eq!( - FormatValidator::count_required_args("hello {}").unwrap(), - (1, 0) - ); - assert_eq!( - FormatValidator::count_required_args("{} {}").unwrap(), - (2, 0) - ); - assert_eq!( - FormatValidator::count_required_args("pid: {}, name: {}").unwrap(), + FormatValidator::count_required_args("pid: {}, name: {}")?, (2, 0) ); // Escape sequences assert_eq!( - FormatValidator::count_required_args("use {{}} for braces").unwrap(), + FormatValidator::count_required_args("use {{}} for braces")?, (0, 0) ); assert_eq!( - FormatValidator::count_required_args("value: {}, braces: {{}}").unwrap(), + FormatValidator::count_required_args("value: {}, braces: {{}}")?, (1, 0) ); @@ -187,48 +178,19 @@ mod tests { assert!(FormatValidator::count_required_args("unmatched }").is_err()); // Extended specifiers - assert_eq!( - FormatValidator::count_required_args("{:x}").unwrap(), - (1, 0) - ); - assert_eq!( - FormatValidator::count_required_args("{:X}").unwrap(), - (1, 0) - ); - assert_eq!( - FormatValidator::count_required_args("{:p}").unwrap(), - (1, 0) - ); - assert_eq!( - FormatValidator::count_required_args("{:s}").unwrap(), - (1, 0) - ); - assert_eq!( - FormatValidator::count_required_args("{:x.16}").unwrap(), - (1, 0) - ); - assert_eq!( - FormatValidator::count_required_args("{:s.*}").unwrap(), - (1, 1) - ); - assert_eq!( - FormatValidator::count_required_args("{:x.len$}").unwrap(), - (1, 0) - ); + assert_eq!(FormatValidator::count_required_args("{:x}")?, (1, 0)); + assert_eq!(FormatValidator::count_required_args("{:X}")?, (1, 0)); + assert_eq!(FormatValidator::count_required_args("{:p}")?, (1, 0)); + assert_eq!(FormatValidator::count_required_args("{:s}")?, (1, 0)); + assert_eq!(FormatValidator::count_required_args("{:x.16}")?, (1, 0)); + assert_eq!(FormatValidator::count_required_args("{:s.*}")?, (1, 1)); + assert_eq!(FormatValidator::count_required_args("{:x.len$}")?, (1, 0)); // Static length with hex/oct/bin - assert_eq!( - FormatValidator::count_required_args("{:x.0x10}").unwrap(), - (1, 0) - ); - assert_eq!( - FormatValidator::count_required_args("{:s.0o20}").unwrap(), - (1, 0) - ); - assert_eq!( - FormatValidator::count_required_args("{:X.0b1000}").unwrap(), - (1, 0) - ); + assert_eq!(FormatValidator::count_required_args("{:x.0x10}")?, (1, 0)); + assert_eq!(FormatValidator::count_required_args("{:s.0o20}")?, (1, 0)); + assert_eq!(FormatValidator::count_required_args("{:X.0b1000}")?, (1, 0)); assert!(FormatValidator::count_required_args("{:x.1a$}").is_err()); + Ok(()) } #[test]