Lots of improvements to stack variable location calculations in DWARF

4 files changed, 135 insertions, 14 deletions

diff --git a/rust/examples/dwarf/dwarf_import/src/dwarfdebuginfo.rs b/rust/examples/dwarf/dwarf_import/src/dwarfdebuginfo.rs
index 2a71b38a..03a237db 100644
--- a/rust/examples/dwarf/dwarf_import/src/dwarfdebuginfo.rs
+++ b/rust/examples/dwarf/dwarf_import/src/dwarfdebuginfo.rs
@@ -50,6 +50,7 @@ pub(crate) struct FunctionInfoBuilder {
     pub(crate) platform: Option<Ref<Platform>>,
     pub(crate) variable_arguments: bool,
     pub(crate) stack_variables: Vec<NamedTypedVariable>,
+    pub(crate) use_cfa: bool, //TODO actually store more info about the frame base
 }
 
 impl FunctionInfoBuilder {
@@ -229,6 +230,7 @@ impl DebugInfoBuilder {
         address: Option<u64>,
         parameters: &Vec<Option<(String, TypeUID)>>,
         variable_arguments: bool,
+        use_cfa: bool,
     ) -> Option<usize> {
         // Returns the index of the function
         // Raw names should be the primary key, but if they don't exist, use the full name
@@ -296,6 +298,7 @@ impl DebugInfoBuilder {
             platform: None,
             variable_arguments,
             stack_variables: vec![],
+            use_cfa,
         };
 
         if let Some(n) = &function.full_name {
@@ -362,6 +365,7 @@ impl DebugInfoBuilder {
         offset: i64,
         name: Option<String>,
         type_uid: Option<TypeUID>,
+        lexical_block: Option<&iset::IntervalSet<u64>>,
     ) {
         let name = match name {
             Some(x) => {
@@ -400,18 +404,39 @@ impl DebugInfoBuilder {
             return;
         };
 
-        let Some(offset_adjustment) = self.range_data_offsets.values_overlap(func_addr).next() else {
+        let adjusted_offset;
+        let Some(adjustment_at_variable_lifetime_start) = lexical_block.and_then(|block_ranges| {
+            block_ranges
+            .unsorted_iter()
+            .find_map(|x| self.range_data_offsets.values_overlap(x.start).next())
+        }).or_else(|| {
+            self.range_data_offsets.values_overlap(func_addr).next()
+        }) else {
             // Unknown why, but this is happening with MachO + external dSYM
             debug!("Refusing to add a local variable ({}@{}) to function at {} without a known CIE offset.", name, offset, func_addr);
             return;
         };
 
-        // TODO: offset should be calculated based off reference address and not function start
-        let adjusted_offset = offset - offset_adjustment;
+        // TODO: handle non-sp frame bases
+        // TODO: if not in a lexical block these can be wrong, see https://github.com/Vector35/binaryninja-api/issues/5882#issuecomment-2406065057
+        if function.use_cfa {
+            // Apply CFA offset to variable storage offset if DW_AT_frame_base is frame base is CFA
+            adjusted_offset = offset + adjustment_at_variable_lifetime_start;
+        }
+        else {
+            // If it's using SP, we know the SP offset is <SP offset> + (<entry SP CFA offset> - <SP CFA offset>)
+            let Some(adjustment_at_entry) = self.range_data_offsets.values_overlap(func_addr).next() else {
+                // Unknown why, but this is happening with MachO + external dSYM
+                debug!("Refusing to add a local variable ({}@{}) to function at {} without a known CIE offset for function start.", name, offset, func_addr);
+                return;
+            };
+
+            adjusted_offset = offset + (adjustment_at_entry - adjustment_at_variable_lifetime_start);
+        }
 
         if adjusted_offset > 0 {
             // If we somehow end up with a positive sp offset
-            error!("Trying to add a local variable at positive storage offset {}. Please report this issue.", adjusted_offset);
+            error!("Trying to add a local variable \"{}\" in function at {:#x} at positive storage offset {}. Please report this issue.", name, func_addr, adjusted_offset);
             return;
         }
 
diff --git a/rust/examples/dwarf/dwarf_import/src/functions.rs b/rust/examples/dwarf/dwarf_import/src/functions.rs
index 9ca01a2c..43b69ca3 100644
--- a/rust/examples/dwarf/dwarf_import/src/functions.rs
+++ b/rust/examples/dwarf/dwarf_import/src/functions.rs
@@ -20,8 +20,8 @@ use crate::types::get_type;
 
 use binaryninja::templatesimplifier::simplify_str_to_str;
 use cpp_demangle::DemangleOptions;
-use gimli::{constants, DebuggingInformationEntry, Dwarf, Unit};
-use log::debug;
+use gimli::{constants, AttributeValue, DebuggingInformationEntry, Dwarf, Operation, Unit};
+use log::{debug, error};
 use regex::Regex;
 
 fn get_parameters<R: ReaderType>(
@@ -126,5 +126,79 @@ pub(crate) fn parse_function_entry<R: ReaderType>(
         return None;
     }
 
-    debug_info_builder.insert_function(full_name, raw_name, return_type, address, &parameters, variable_arguments)
+    let use_cfa;
+    if let Ok(Some(AttributeValue::Exprloc(mut expression))) = entry.attr_value(constants::DW_AT_frame_base) {
+        use_cfa = match Operation::parse(&mut expression.0, unit.encoding()) {
+            Ok(Operation::Register { register: _ }) => false, // TODO: handle register-relative encodings later
+            Ok(Operation::CallFrameCFA) => true,
+            _ => false
+        };
+    }
+    else {
+        use_cfa = false;
+    }
+
+    debug_info_builder.insert_function(full_name, raw_name, return_type, address, &parameters, variable_arguments, use_cfa)
+}
+
+
+pub(crate) fn parse_lexical_block<R: ReaderType>(
+    dwarf: &Dwarf<R>,
+    unit: &Unit<R>,
+    entry: &DebuggingInformationEntry<R>,
+) -> Option<iset::IntervalSet<u64>> {
+    // Return lexical block ranges
+    // Must have either DW_AT_ranges or DW_AT_low_pc and DW_AT_high_pc
+    let mut result = iset::IntervalSet::new();
+    if let Ok(Some(attr_value)) = entry.attr_value(constants::DW_AT_ranges) {
+        if let Ok(Some(ranges_offset)) = dwarf.attr_ranges_offset(unit, attr_value)
+        {
+            if let Ok(mut ranges) = dwarf.ranges(unit, ranges_offset)
+            {
+                while let Ok(Some(range)) = ranges.next() {
+                    // Ranges where start == end may be ignored (DWARFv5 spec, 2.17.3 line 17)
+                    if range.begin == range.end {
+                        continue
+                    }
+                    result.insert(range.begin..range.end);
+                }
+            }
+        }
+    }
+    else if let Ok(Some(low_pc_value)) = entry.attr_value(constants::DW_AT_low_pc) {
+        let Ok(Some(low_pc)) = dwarf.attr_address(unit, low_pc_value.clone()) else {
+            let unit_base: usize = unit.header.offset().as_debug_info_offset().unwrap().0;
+            error!("Failed to read lexical block low_pc for entry {:#x}, please report this bug.", unit_base + entry.offset().0);
+            return None;
+        };
+
+        let Ok(Some(high_pc_value)) = entry.attr_value(constants::DW_AT_high_pc) else {
+            let unit_base: usize = unit.header.offset().as_debug_info_offset().unwrap().0;
+            error!("Failed to read lexical block high_pc attribute for entry {:#x}, please report this bug.", unit_base + entry.offset().0);
+            return None;
+        };
+
+        let Some(high_pc) = high_pc_value
+            .udata_value()
+            .and_then(|x| Some(low_pc + x))
+            .or_else(|| dwarf.attr_address(unit, high_pc_value).unwrap_or(None))
+        else {
+            let unit_base: usize = unit.header.offset().as_debug_info_offset().unwrap().0;
+            error!("Failed to read lexical block high_pc for entry {:#x}, please report this bug.", unit_base + entry.offset().0);
+            return None;
+        };
+
+        if low_pc < high_pc {
+            result.insert(low_pc..high_pc);
+        }
+        else {
+            error!("Invalid lexical block range: {:#x} -> {:#x}", low_pc, high_pc);
+        }
+    }
+    else {
+        // If neither case is hit the lexical block doesn't define any ranges and we should ignore it
+        return None;
+    }
+
+    Some(result)
 }
diff --git a/rust/examples/dwarf/dwarf_import/src/lib.rs b/rust/examples/dwarf/dwarf_import/src/lib.rs
index 94503856..076586f1 100644
--- a/rust/examples/dwarf/dwarf_import/src/lib.rs
+++ b/rust/examples/dwarf/dwarf_import/src/lib.rs
@@ -37,6 +37,7 @@ use dwarfreader::{
     create_section_reader, get_endian, is_dwo_dwarf, is_non_dwo_dwarf, is_raw_dwo_dwarf,
 };
 
+use functions::parse_lexical_block;
 use gimli::{constants, CfaRule, DebuggingInformationEntry, Dwarf, DwarfFileType, Reader, Section, SectionId, Unit, UnwindContext, UnwindSection};
 
 use helpers::{get_build_id, load_debug_info_for_build_id};
@@ -222,6 +223,7 @@ fn parse_unit<R: ReaderType>(
 
     let mut current_depth: isize = 0;
     let mut functions_by_depth: Vec<(Option<usize>, isize)> = vec![];
+    let mut lexical_blocks_by_depth: Vec<(iset::IntervalSet<u64>, isize)> = vec![];
 
     // Really all we care about as we iterate the entries in a given unit is how they modify state (our perception of the file)
     // There's a lot of junk we don't care about in DWARF info, so we choose a couple DIEs and mutate state (add functions (which adds the types it uses) and keep track of what namespace we're in)
@@ -250,6 +252,18 @@ fn parse_unit<R: ReaderType>(
             else {
                 break;
             }
+
+            if let Some((_lexical_block, depth)) = lexical_blocks_by_depth.last() {
+                if current_depth <= *depth {
+                    lexical_blocks_by_depth.pop();
+                }
+                else {
+                    break
+                }
+            }
+            else {
+                break;
+            }
         }
 
         match entry.tag() {
@@ -257,9 +271,15 @@ fn parse_unit<R: ReaderType>(
                 let fn_idx = parse_function_entry(dwarf, unit, entry, debug_info_builder_context, debug_info_builder);
                 functions_by_depth.push((fn_idx, current_depth));
             },
+            constants::DW_TAG_lexical_block => {
+                if let Some(block_ranges) = parse_lexical_block(dwarf, unit, entry) {
+                    lexical_blocks_by_depth.push((block_ranges, current_depth));
+                }
+            },
             constants::DW_TAG_variable => {
                 let current_fn_idx = functions_by_depth.last().and_then(|x| x.0);
-                parse_variable(dwarf, unit, entry, debug_info_builder_context, debug_info_builder, current_fn_idx)
+                let current_lexical_block = lexical_blocks_by_depth.last().and_then(|x| Some(&x.0));
+                parse_variable(dwarf, unit, entry, debug_info_builder_context, debug_info_builder, current_fn_idx, current_lexical_block)
             },
             constants::DW_TAG_class_type |
             constants::DW_TAG_enumeration_type |
@@ -300,13 +320,13 @@ where <U as UnwindSection<R>>::Offset: std::hash::Hash {
     }
 
     let mut cies = HashMap::new();
-    let mut cie_data_offsets = iset::IntervalMap::new();
+    let mut cfa_offsets = iset::IntervalMap::new();
 
     let mut entries = unwind_section.entries(&bases);
     let mut unwind_context = UnwindContext::new();
     loop {
         match entries.next()? {
-            None => return Ok(cie_data_offsets),
+            None => return Ok(cfa_offsets),
             Some(gimli::CieOrFde::Cie(_cie)) => {
                 // TODO: do we want to do anything with standalone CIEs?
             }
@@ -325,7 +345,7 @@ where <U as UnwindSection<R>>::Offset: std::hash::Hash {
 
                 if fde.len() == 0 {
                     // This FDE is a terminator
-                    return Ok(cie_data_offsets);
+                    return Ok(cfa_offsets);
                 }
 
                 if fde.initial_address().overflowing_add(fde.len()).1 {
@@ -333,11 +353,12 @@ where <U as UnwindSection<R>>::Offset: std::hash::Hash {
                 } else {
                     // Walk the FDE table rows and store their CFA
                     let mut fde_table = fde.rows(&unwind_section, &bases, &mut unwind_context)?;
+
                     while let Some(row) = fde_table.next_row()? {
                         match row.cfa() {
                             CfaRule::RegisterAndOffset {register: _, offset} => {
-                                // TODO: this offset could be wrong because register might be something wacky
-                                cie_data_offsets.insert(
+                                // TODO: we should store offsets by register
+                                cfa_offsets.insert(
                                     row.start_address()..row.end_address(),
                                     *offset,
                                 );
diff --git a/rust/examples/dwarf/dwarf_import/src/types.rs b/rust/examples/dwarf/dwarf_import/src/types.rs
index 19b38a90..bfc7b17d 100644
--- a/rust/examples/dwarf/dwarf_import/src/types.rs
+++ b/rust/examples/dwarf/dwarf_import/src/types.rs
@@ -34,6 +34,7 @@ pub(crate) fn parse_variable<R: ReaderType>(
     debug_info_builder_context: &DebugInfoBuilderContext<R>,
     debug_info_builder: &mut DebugInfoBuilder,
     function_index: Option<usize>,
+    lexical_block: Option<&iset::IntervalSet<u64>>,
 ) {
     let full_name = debug_info_builder_context.get_name(dwarf, unit, entry);
     let type_uid = get_type(dwarf, unit, entry, debug_info_builder_context, debug_info_builder);
@@ -48,7 +49,7 @@ pub(crate) fn parse_variable<R: ReaderType>(
 
     match Operation::parse(&mut expression.0, unit.encoding()) {
         Ok(Operation::FrameOffset { offset }) => {
-            debug_info_builder.add_stack_variable(function_index, offset, full_name, type_uid);
+            debug_info_builder.add_stack_variable(function_index, offset, full_name, type_uid, lexical_block);
         },
         //Ok(Operation::RegisterOffset { register: _, offset: _, base_type: _ }) => {
         //    //TODO: look up register by index (binja register indexes don't match processor indexes?)