Refactor data relocation diffing to improve accuracy and fix bugs (#157)

* Data reloc hover tooltip: Show relocation source address

* Refactor data relocation diffing to improve accuracy and fix bugs

Author: LagoLunatic

objdiff-core/src/bindings/diff.rs

@@ -42,6 +42,7 @@ impl SectionDiff {
         let section = &obj.sections[section_index];
         let symbols = section_diff.symbols.iter().map(|d| SymbolDiff::new(obj, d)).collect();
         let data = section_diff.data_diff.iter().map(|d| DataDiff::new(obj, d)).collect();
+        // TODO: section_diff.reloc_diff
         Self {
             name: section.name.to_string(),
             kind: SectionKind::from(section.kind) as i32,

objdiff-core/src/diff/data.rs

@@ -8,7 +8,7 @@ use similar::{capture_diff_slices_deadline, get_diff_ratio, Algorithm};
 
 use super::code::section_name_eq;
 use crate::{
-    diff::{ObjDataDiff, ObjDataDiffKind, ObjSectionDiff, ObjSymbolDiff},
+    diff::{ObjDataDiff, ObjDataDiffKind, ObjDataRelocDiff, ObjSectionDiff, ObjSymbolDiff},
     obj::{ObjInfo, ObjReloc, ObjSection, ObjSymbolFlags, SymbolRef},
 };
 
@@ -103,7 +103,7 @@ fn diff_data_relocs_for_range(
             let right_offset = r.address as usize - right_range.start;
             right_offset == left_offset
         }) else {
-            diffs.push((ObjDataDiffKind::Replace, Some(left_reloc.clone()), None));
+            diffs.push((ObjDataDiffKind::Delete, Some(left_reloc.clone()), None));
             continue;
         };
         if reloc_eq(left_obj, right_obj, left_reloc, right_reloc) {
@@ -132,7 +132,7 @@ fn diff_data_relocs_for_range(
             let left_offset = r.address as usize - left_range.start;
             left_offset == right_offset
         }) else {
-            diffs.push((ObjDataDiffKind::Replace, None, Some(right_reloc.clone())));
+            diffs.push((ObjDataDiffKind::Insert, None, Some(right_reloc.clone())));
             continue;
         };
         // No need to check the cases for relocations being deleted or matching again.
@@ -176,94 +176,6 @@ pub fn diff_data_section(
                 ObjDataDiffKind::Replace
             }
         };
-        if kind == ObjDataDiffKind::None {
-            let mut found_any_relocs = false;
-            let mut left_curr_addr = left_range.start;
-            let mut right_curr_addr = right_range.start;
-            for (diff_kind, left_reloc, right_reloc) in diff_data_relocs_for_range(
-                left_obj,
-                right_obj,
-                left,
-                right,
-                left_range.clone(),
-                right_range.clone(),
-            ) {
-                found_any_relocs = true;
-
-                if let Some(left_reloc) = left_reloc {
-                    let left_reloc_addr = left_reloc.address as usize;
-                    if left_reloc_addr > left_curr_addr {
-                        let len = left_reloc_addr - left_curr_addr;
-                        let left_data = &left.data[left_curr_addr..left_reloc_addr];
-                        left_diff.push(ObjDataDiff {
-                            data: left_data[..min(len, left_data.len())].to_vec(),
-                            kind: ObjDataDiffKind::None,
-                            len,
-                            ..Default::default()
-                        });
-                    }
-                    let reloc_diff_len = left_obj.arch.get_reloc_byte_size(left_reloc.flags);
-                    let left_data = &left.data[left_reloc_addr..left_reloc_addr + reloc_diff_len];
-                    left_diff.push(ObjDataDiff {
-                        data: left_data[..min(reloc_diff_len, left_data.len())].to_vec(),
-                        kind: diff_kind,
-                        len: reloc_diff_len,
-                        reloc: Some(left_reloc.clone()),
-                        ..Default::default()
-                    });
-                    left_curr_addr = left_reloc_addr + reloc_diff_len;
-                }
-
-                if let Some(right_reloc) = right_reloc {
-                    let right_reloc_addr = right_reloc.address as usize;
-                    if right_reloc_addr > right_curr_addr {
-                        let len = right_reloc_addr - right_curr_addr;
-                        let right_data = &right.data[right_curr_addr..right_reloc_addr];
-                        right_diff.push(ObjDataDiff {
-                            data: right_data[..min(len, right_data.len())].to_vec(),
-                            kind: ObjDataDiffKind::None,
-                            len,
-                            ..Default::default()
-                        });
-                    }
-                    let reloc_diff_len = right_obj.arch.get_reloc_byte_size(right_reloc.flags);
-                    let right_data =
-                        &right.data[right_reloc_addr..right_reloc_addr + reloc_diff_len];
-                    right_diff.push(ObjDataDiff {
-                        data: right_data[..min(reloc_diff_len, right_data.len())].to_vec(),
-                        kind: diff_kind,
-                        len: reloc_diff_len,
-                        reloc: Some(right_reloc.clone()),
-                        ..Default::default()
-                    });
-                    right_curr_addr = right_reloc_addr + reloc_diff_len;
-                }
-            }
-
-            if found_any_relocs {
-                if left_curr_addr < left_range.end - 1 {
-                    let len = left_range.end - left_curr_addr;
-                    let left_data = &left.data[left_curr_addr..left_range.end];
-                    left_diff.push(ObjDataDiff {
-                        data: left_data[..min(len, left_data.len())].to_vec(),
-                        kind: ObjDataDiffKind::None,
-                        len,
-                        ..Default::default()
-                    });
-                }
-                if right_curr_addr < right_range.end - 1 {
-                    let len = right_range.end - right_curr_addr;
-                    let right_data = &right.data[right_curr_addr..right_range.end];
-                    right_diff.push(ObjDataDiff {
-                        data: right_data[..min(len, right_data.len())].to_vec(),
-                        kind: ObjDataDiffKind::None,
-                        len,
-                        ..Default::default()
-                    });
-                }
-                continue;
-            }
-        }
         let left_data = &left.data[left_range];
         let right_data = &right.data[right_range];
         left_diff.push(ObjDataDiff {
@@ -315,12 +227,35 @@ pub fn diff_data_section(
         }
     }
 
+    let mut left_reloc_diffs = Vec::new();
+    let mut right_reloc_diffs = Vec::new();
+    for (diff_kind, left_reloc, right_reloc) in diff_data_relocs_for_range(
+        left_obj,
+        right_obj,
+        left,
+        right,
+        0..left_max as usize,
+        0..right_max as usize,
+    ) {
+        if let Some(left_reloc) = left_reloc {
+            let len = left_obj.arch.get_reloc_byte_size(left_reloc.flags);
+            let range = left_reloc.address as usize..left_reloc.address as usize + len;
+            left_reloc_diffs.push(ObjDataRelocDiff { reloc: left_reloc, kind: diff_kind, range });
+        }
+        if let Some(right_reloc) = right_reloc {
+            let len = right_obj.arch.get_reloc_byte_size(right_reloc.flags);
+            let range = right_reloc.address as usize..right_reloc.address as usize + len;
+            right_reloc_diffs.push(ObjDataRelocDiff { reloc: right_reloc, kind: diff_kind, range });
+        }
+    }
+
     let (mut left_section_diff, mut right_section_diff) =
         diff_generic_section(left, right, left_section_diff, right_section_diff)?;
-    let all_left_relocs_match =
-        left_diff.iter().all(|d| d.kind == ObjDataDiffKind::None || d.reloc.is_none());
+    let all_left_relocs_match = left_reloc_diffs.iter().all(|d| d.kind == ObjDataDiffKind::None);
     left_section_diff.data_diff = left_diff;
     right_section_diff.data_diff = right_diff;
+    left_section_diff.reloc_diff = left_reloc_diffs;
+    right_section_diff.reloc_diff = right_reloc_diffs;
     if all_left_relocs_match {
         // Use the highest match percent between two options:
         // - Left symbols matching right symbols by name
@@ -430,8 +365,18 @@ pub fn diff_generic_section(
             / left.size as f32
     };
     Ok((
-        ObjSectionDiff { symbols: vec![], data_diff: vec![], match_percent: Some(match_percent) },
-        ObjSectionDiff { symbols: vec![], data_diff: vec![], match_percent: Some(match_percent) },
+        ObjSectionDiff {
+            symbols: vec![],
+            data_diff: vec![],
+            reloc_diff: vec![],
+            match_percent: Some(match_percent),
+        },
+        ObjSectionDiff {
+            symbols: vec![],
+            data_diff: vec![],
+            reloc_diff: vec![],
+            match_percent: Some(match_percent),
+        },
     ))
 }
 
@@ -456,7 +401,17 @@ pub fn diff_bss_section(
     }
 
     Ok((
-        ObjSectionDiff { symbols: vec![], data_diff: vec![], match_percent: Some(match_percent) },
-        ObjSectionDiff { symbols: vec![], data_diff: vec![], match_percent: Some(match_percent) },
+        ObjSectionDiff {
+            symbols: vec![],
+            data_diff: vec![],
+            reloc_diff: vec![],
+            match_percent: Some(match_percent),
+        },
+        ObjSectionDiff {
+            symbols: vec![],
+            data_diff: vec![],
+            reloc_diff: vec![],
+            match_percent: Some(match_percent),
+        },
     ))
 }

objdiff-core/src/diff/mod.rs

@@ -1,4 +1,4 @@
-use std::collections::HashSet;
+use std::{collections::HashSet, ops::Range};
 
 use anyhow::Result;
 
@@ -36,13 +36,15 @@ impl DiffObjConfig {
 pub struct ObjSectionDiff {
     pub symbols: Vec<ObjSymbolDiff>,
     pub data_diff: Vec<ObjDataDiff>,
+    pub reloc_diff: Vec<ObjDataRelocDiff>,
     pub match_percent: Option<f32>,
 }
 
 impl ObjSectionDiff {
     fn merge(&mut self, other: ObjSectionDiff) {
         // symbols ignored
         self.data_diff = other.data_diff;
+        self.reloc_diff = other.reloc_diff;
         self.match_percent = other.match_percent;
     }
 }
@@ -87,7 +89,13 @@ pub struct ObjDataDiff {
     pub kind: ObjDataDiffKind,
     pub len: usize,
     pub symbol: String,
-    pub reloc: Option<ObjReloc>,
+}
+
+#[derive(Debug, Clone)]
+pub struct ObjDataRelocDiff {
+    pub reloc: ObjReloc,
+    pub kind: ObjDataDiffKind,
+    pub range: Range<usize>,
 }
 
 #[derive(Debug, Copy, Clone, Eq, PartialEq, Default)]
@@ -156,8 +164,8 @@ impl ObjDiff {
                     kind: ObjDataDiffKind::None,
                     len: section.data.len(),
                     symbol: section.name.clone(),
-                    ..Default::default()
                 }],
+                reloc_diff: vec![],
                 match_percent: None,
             });
         }

objdiff-core/src/obj/mod.rs

@@ -28,7 +28,7 @@ flags! {
         HasExtra,
     }
 }
-#[derive(Debug, Copy, Clone, Default)]
+#[derive(Debug, Copy, Clone, Default, PartialEq)]
 pub struct ObjSymbolFlagSet(pub FlagSet<ObjSymbolFlags>);
 
 #[derive(Debug, Clone)]
@@ -132,7 +132,7 @@ pub enum ObjSymbolKind {
     Section,
 }
 
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, PartialEq)]
 pub struct ObjSymbol {
     pub name: String,
     pub demangled_name: Option<String>,
@@ -161,7 +161,7 @@ pub struct ObjInfo {
     pub split_meta: Option<SplitMeta>,
 }
 
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, PartialEq)]
 pub struct ObjReloc {
     pub flags: RelocationFlags,
     pub address: u64,