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

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 =
+    let all_left_relocs_match = left_reloc_diffs.iter().all(|d| d.kind == ObjDataDiffKind::None);
-        left_diff.iter().all(|d| d.kind == ObjDataDiffKind::None || d.reloc.is_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 {
-        ObjSectionDiff { symbols: vec![], data_diff: vec![], match_percent: Some(match_percent) },
+            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 {
-        ObjSectionDiff { symbols: vec![], data_diff: vec![], match_percent: Some(match_percent) },
+            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,6 +36,7 @@ impl DiffObjConfig {
 pub struct ObjSectionDiff {
     pub symbols: Vec<ObjSymbolDiff>,
     pub data_diff: Vec<ObjDataDiff>,
+    pub reloc_diff: Vec<ObjDataRelocDiff>,
     pub match_percent: Option<f32>,
 }
 
@@ -43,6 +44,7 @@ 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,

objdiff-gui/src/views/data_diff.rs

@@ -6,7 +6,7 @@ use std::{
 
 use egui::{text::LayoutJob, Id, Label, RichText, Sense, Widget};
 use objdiff_core::{
-    diff::{ObjDataDiff, ObjDataDiffKind, ObjDiff},
+    diff::{ObjDataDiff, ObjDataDiffKind, ObjDataRelocDiff, ObjDiff},
     obj::ObjInfo,
 };
 use time::format_description;
@@ -30,27 +30,31 @@ fn find_section(obj: &ObjInfo, section_name: &str) -> Option<usize> {
 fn data_row_hover_ui(
     ui: &mut egui::Ui,
     obj: &ObjInfo,
-    diffs: &[ObjDataDiff],
+    diffs: &[(ObjDataDiff, Vec<ObjDataRelocDiff>)],
     appearance: &Appearance,
 ) {
     ui.scope(|ui| {
         ui.style_mut().override_text_style = Some(egui::TextStyle::Monospace);
         ui.style_mut().wrap_mode = Some(egui::TextWrapMode::Extend);
 
-        for diff in diffs {
+        let reloc_diffs = diffs.iter().flat_map(|(_, reloc_diffs)| reloc_diffs);
-            let Some(reloc) = &diff.reloc else {
+        let mut prev_reloc = None;
+        for reloc_diff in reloc_diffs {
+            let reloc = &reloc_diff.reloc;
+            if prev_reloc == Some(reloc) {
+                // Avoid showing consecutive duplicate relocations.
+                // We do this because a single relocation can span across multiple diffs if the
+                // bytes in the relocation changed (e.g. first byte is added, second is unchanged).
                 continue;
-            };
+            }
+            prev_reloc = Some(reloc);
 
-            // Use a slightly different font color for nonmatching relocations so they stand out.
+            let color = get_color_for_diff_kind(reloc_diff.kind, appearance);
-            let color = match diff.kind {
-                ObjDataDiffKind::None => appearance.highlight_color,
-                _ => appearance.replace_color,
-            };
 
             // TODO: Most of this code is copy-pasted from ins_hover_ui.
             // Try to separate this out into a shared function.
             ui.label(format!("Relocation type: {}", obj.arch.display_reloc(reloc.flags)));
+            ui.label(format!("Relocation address: {:x}", reloc.address));
             let addend_str = match reloc.addend.cmp(&0i64) {
                 Ordering::Greater => format!("+{:x}", reloc.addend),
                 Ordering::Less => format!("-{:x}", -reloc.addend),
@@ -76,15 +80,22 @@ fn data_row_hover_ui(
     });
 }
 
-fn data_row_context_menu(ui: &mut egui::Ui, diffs: &[ObjDataDiff]) {
+fn data_row_context_menu(ui: &mut egui::Ui, diffs: &[(ObjDataDiff, Vec<ObjDataRelocDiff>)]) {
     ui.scope(|ui| {
         ui.style_mut().override_text_style = Some(egui::TextStyle::Monospace);
         ui.style_mut().wrap_mode = Some(egui::TextWrapMode::Extend);
 
-        for diff in diffs {
+        let reloc_diffs = diffs.iter().flat_map(|(_, reloc_diffs)| reloc_diffs);
-            let Some(reloc) = &diff.reloc else {
+        let mut prev_reloc = None;
+        for reloc_diff in reloc_diffs {
+            let reloc = &reloc_diff.reloc;
+            if prev_reloc == Some(reloc) {
+                // Avoid showing consecutive duplicate relocations.
+                // We do this because a single relocation can span across multiple diffs if the
+                // bytes in the relocation changed (e.g. first byte is added, second is unchanged).
                 continue;
-            };
+            }
+            prev_reloc = Some(reloc);
 
             // TODO: This code is copy-pasted from ins_context_menu.
             // Try to separate this out into a shared function.
@@ -102,14 +113,25 @@ fn data_row_context_menu(ui: &mut egui::Ui, diffs: &[ObjDataDiff]) {
     });
 }
 
+fn get_color_for_diff_kind(diff_kind: ObjDataDiffKind, appearance: &Appearance) -> egui::Color32 {
+    match diff_kind {
+        ObjDataDiffKind::None => appearance.text_color,
+        ObjDataDiffKind::Replace => appearance.replace_color,
+        ObjDataDiffKind::Delete => appearance.delete_color,
+        ObjDataDiffKind::Insert => appearance.insert_color,
+    }
+}
+
 fn data_row_ui(
     ui: &mut egui::Ui,
     obj: Option<&ObjInfo>,
     address: usize,
-    diffs: &[ObjDataDiff],
+    diffs: &[(ObjDataDiff, Vec<ObjDataRelocDiff>)],
     appearance: &Appearance,
 ) {
-    if diffs.iter().any(|d| d.kind != ObjDataDiffKind::None) {
+    if diffs.iter().any(|(dd, rds)| {
+        dd.kind != ObjDataDiffKind::None || rds.iter().any(|rd| rd.kind != ObjDataDiffKind::None)
+    }) {
         ui.painter().rect_filled(ui.available_rect_before_wrap(), 0.0, ui.visuals().faint_bg_color);
     }
     let mut job = LayoutJob::default();
@@ -119,29 +141,34 @@ fn data_row_ui(
         &mut job,
         appearance.code_font.clone(),
     );
+    // The offset shown on the side of the GUI, shifted by insertions/deletions.
     let mut cur_addr = 0usize;
-    for diff in diffs {
+    // The offset into the actual bytes of the section on this side, ignoring differences.
-        let base_color = match diff.kind {
+    let mut cur_addr_actual = address;
-            ObjDataDiffKind::None => appearance.text_color,
+    for (diff, reloc_diffs) in diffs {
-            ObjDataDiffKind::Replace => appearance.replace_color,
+        let base_color = get_color_for_diff_kind(diff.kind, appearance);
-            ObjDataDiffKind::Delete => appearance.delete_color,
-            ObjDataDiffKind::Insert => appearance.insert_color,
-        };
         if diff.data.is_empty() {
             let mut str = "   ".repeat(diff.len);
             str.push_str(" ".repeat(diff.len / 8).as_str());
             write_text(str.as_str(), base_color, &mut job, appearance.code_font.clone());
             cur_addr += diff.len;
         } else {
-            let mut text = String::new();
             for byte in &diff.data {
-                text.push_str(format!("{byte:02x} ").as_str());
+                let mut byte_color = base_color;
+                if let Some(reloc_diff) = reloc_diffs.iter().find(|reloc_diff| {
+                    reloc_diff.kind != ObjDataDiffKind::None
+                        && reloc_diff.range.contains(&cur_addr_actual)
+                }) {
+                    byte_color = get_color_for_diff_kind(reloc_diff.kind, appearance);
+                }
+                let byte_text = format!("{byte:02x} ");
+                write_text(byte_text.as_str(), byte_color, &mut job, appearance.code_font.clone());
                 cur_addr += 1;
+                cur_addr_actual += 1;
                 if cur_addr % 8 == 0 {
-                    text.push(' ');
+                    write_text(" ", base_color, &mut job, appearance.code_font.clone());
                 }
             }
-            write_text(text.as_str(), base_color, &mut job, appearance.code_font.clone());
         }
     }
     if cur_addr < BYTES_PER_ROW {
@@ -152,13 +179,8 @@ fn data_row_ui(
         write_text(str.as_str(), appearance.text_color, &mut job, appearance.code_font.clone());
     }
     write_text(" ", appearance.text_color, &mut job, appearance.code_font.clone());
-    for diff in diffs {
+    for (diff, _) in diffs {
-        let base_color = match diff.kind {
+        let base_color = get_color_for_diff_kind(diff.kind, appearance);
-            ObjDataDiffKind::None => appearance.text_color,
-            ObjDataDiffKind::Replace => appearance.replace_color,
-            ObjDataDiffKind::Delete => appearance.delete_color,
-            ObjDataDiffKind::Insert => appearance.insert_color,
-        };
         if diff.data.is_empty() {
             write_text(
                 " ".repeat(diff.len).as_str(),
@@ -188,17 +210,24 @@ fn data_row_ui(
     }
 }
 
-fn split_diffs(diffs: &[ObjDataDiff]) -> Vec<Vec<ObjDataDiff>> {
+fn split_diffs(
-    let mut split_diffs = Vec::<Vec<ObjDataDiff>>::new();
+    diffs: &[ObjDataDiff],
-    let mut row_diffs = Vec::<ObjDataDiff>::new();
+    reloc_diffs: &[ObjDataRelocDiff],
+) -> Vec<Vec<(ObjDataDiff, Vec<ObjDataRelocDiff>)>> {
+    let mut split_diffs = Vec::<Vec<(ObjDataDiff, Vec<ObjDataRelocDiff>)>>::new();
+    let mut row_diffs = Vec::<(ObjDataDiff, Vec<ObjDataRelocDiff>)>::new();
+    // The offset shown on the side of the GUI, shifted by insertions/deletions.
     let mut cur_addr = 0usize;
+    // The offset into the actual bytes of the section on this side, ignoring differences.
+    let mut cur_addr_actual = 0usize;
     for diff in diffs {
         let mut cur_len = 0usize;
         while cur_len < diff.len {
             let remaining_len = diff.len - cur_len;
             let mut remaining_in_row = BYTES_PER_ROW - (cur_addr % BYTES_PER_ROW);
             let len = min(remaining_len, remaining_in_row);
-            row_diffs.push(ObjDataDiff {
+
+            let data_diff = ObjDataDiff {
                 data: if diff.data.is_empty() {
                     Vec::new()
                 } else {
@@ -207,8 +236,27 @@ fn split_diffs(diffs: &[ObjDataDiff]) -> Vec<Vec<ObjDataDiff>> {
                 kind: diff.kind,
                 len,
                 symbol: String::new(), // TODO
-                reloc: diff.reloc.clone(),
+            };
-            });
+            let row_reloc_diffs: Vec<ObjDataRelocDiff> = if diff.data.is_empty() {
+                Vec::new()
+            } else {
+                let diff_range = cur_addr_actual + cur_len..cur_addr_actual + cur_len + len;
+                reloc_diffs
+                    .iter()
+                    .filter_map(|reloc_diff| {
+                        if reloc_diff.range.start < diff_range.end
+                            && diff_range.start < reloc_diff.range.end
+                        {
+                            Some(reloc_diff.clone())
+                        } else {
+                            None
+                        }
+                    })
+                    .collect()
+            };
+            let row_diff = (data_diff, row_reloc_diffs);
+
+            row_diffs.push(row_diff);
             remaining_in_row -= len;
             cur_len += len;
             cur_addr += len;
@@ -216,6 +264,7 @@ fn split_diffs(diffs: &[ObjDataDiff]) -> Vec<Vec<ObjDataDiff>> {
                 split_diffs.push(take(&mut row_diffs));
             }
         }
+        cur_addr_actual += diff.data.len();
     }
     if !row_diffs.is_empty() {
         split_diffs.push(take(&mut row_diffs));
@@ -267,8 +316,10 @@ fn data_table_ui(
     }
     let total_rows = (total_bytes - 1) / BYTES_PER_ROW + 1;
 
-    let left_diffs = left_section.map(|(_, section)| split_diffs(&section.data_diff));
+    let left_diffs =
-    let right_diffs = right_section.map(|(_, section)| split_diffs(&section.data_diff));
+        left_section.map(|(_, section)| split_diffs(&section.data_diff, &section.reloc_diff));
+    let right_diffs =
+        right_section.map(|(_, section)| split_diffs(&section.data_diff, &section.reloc_diff));
 
     hotkeys::check_scroll_hotkeys(ui, true);