Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 22, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0cfb559
Data reloc hover tooltip: Show relocation source address
LagoLunatic Jan 22, 2025
669e566
Refactor data relocation diffing to improve accuracy and fix bugs
LagoLunatic Jan 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions objdiff-core/src/bindings/diff.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment on lines 44 to +45
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't entirely sure what this section of the code does, but I think it's something for objdiff-cli, and I don't think that can diff data sections anyway? So I don't think leaving data relocation diffs unimplemented here would have any effect, but I might be wrong.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, I’ll be adding this for CLI and VS Code extension eventually.

Self {
name: section.name.to_string(),
kind: SectionKind::from(section.kind) as i32,
Expand Down
149 changes: 52 additions & 97 deletions objdiff-core/src/diff/data.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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},
};

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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),
},
))
}

Expand All @@ -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),
},
))
}
14 changes: 11 additions & 3 deletions objdiff-core/src/diff/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::collections::HashSet;
use std::{collections::HashSet, ops::Range};

use anyhow::Result;

Expand Down Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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)]
Expand Down Expand Up @@ -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,
});
}
Expand Down
6 changes: 3 additions & 3 deletions objdiff-core/src/obj/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down Expand Up @@ -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>,
Expand Down Expand Up @@ -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,
Expand Down
Loading
Loading