diff --git a/objdiff-core/src/diff/code.rs b/objdiff-core/src/diff/code.rs index 2cc14b4..9e8e2ac 100644 --- a/objdiff-core/src/diff/code.rs +++ b/objdiff-core/src/diff/code.rs @@ -8,12 +8,13 @@ use alloc::{ use anyhow::{anyhow, ensure, Context, Result}; use super::{ - DiffObjConfig, FunctionRelocDiffs, InstructionArgDiffIndex, InstructionBranchFrom, - InstructionBranchTo, InstructionDiffKind, InstructionDiffRow, SymbolDiff, + display::display_ins_data_literals, DiffObjConfig, FunctionRelocDiffs, InstructionArgDiffIndex, + InstructionBranchFrom, InstructionBranchTo, InstructionDiffKind, InstructionDiffRow, + SymbolDiff, }; use crate::obj::{ - InstructionArg, InstructionArgValue, InstructionRef, Object, ResolvedRelocation, - ScannedInstruction, SymbolFlag, SymbolKind, + InstructionArg, InstructionArgValue, InstructionRef, Object, ResolvedInstructionRef, + ResolvedRelocation, ScannedInstruction, SymbolFlag, SymbolKind, }; pub fn no_diff_code( @@ -291,12 +292,12 @@ pub(crate) fn section_name_eq( fn reloc_eq( left_obj: &Object, right_obj: &Object, - left_reloc: Option, - right_reloc: Option, + left_ins: ResolvedInstructionRef, + right_ins: ResolvedInstructionRef, diff_config: &DiffObjConfig, ) -> bool { let relax_reloc_diffs = diff_config.function_reloc_diffs == FunctionRelocDiffs::None; - let (left_reloc, right_reloc) = match (left_reloc, right_reloc) { + let (left_reloc, right_reloc) = match (left_ins.relocation, right_ins.relocation) { (Some(left_reloc), Some(right_reloc)) => (left_reloc, right_reloc), // If relocations are relaxed, match if left is missing a reloc (None, Some(_)) => return relax_reloc_diffs, @@ -319,13 +320,10 @@ fn reloc_eq( && (diff_config.function_reloc_diffs == FunctionRelocDiffs::DataValue || symbol_name_addend_matches || address_eq(left_reloc, right_reloc)) - && ( - diff_config.function_reloc_diffs == FunctionRelocDiffs::NameAddress - || left_reloc.symbol.kind != SymbolKind::Object - // TODO - // || left_obj.arch.display_ins_data_labels(left_ins) - // == left_obj.arch.display_ins_data_labels(right_ins)) - ) + && (diff_config.function_reloc_diffs == FunctionRelocDiffs::NameAddress + || left_reloc.symbol.kind != SymbolKind::Object + || display_ins_data_literals(left_obj, left_ins) + == display_ins_data_literals(right_obj, right_ins)) } (Some(_), None) => false, (None, Some(_)) => { @@ -343,8 +341,8 @@ fn arg_eq( right_row: &InstructionDiffRow, left_arg: &InstructionArg, right_arg: &InstructionArg, - left_reloc: Option, - right_reloc: Option, + left_ins: ResolvedInstructionRef, + right_ins: ResolvedInstructionRef, diff_config: &DiffObjConfig, ) -> bool { match left_arg { @@ -357,7 +355,7 @@ fn arg_eq( }, InstructionArg::Reloc => { matches!(right_arg, InstructionArg::Reloc) - && reloc_eq(left_obj, right_obj, left_reloc, right_reloc, diff_config) + && reloc_eq(left_obj, right_obj, left_ins, right_ins, diff_config) } InstructionArg::BranchDest(_) => match right_arg { // Compare dest instruction idx after diffing @@ -434,8 +432,10 @@ fn diff_instruction( .resolve_instruction_ref(right_symbol_idx, r) .context("Failed to resolve right instruction")?; - if left_resolved.code != right_resolved.code { - // If data doesn't match, process instructions and compare args + if left_resolved.code != right_resolved.code + || !reloc_eq(left_obj, right_obj, left_resolved, right_resolved, diff_config) + { + // If either the raw code bytes or relocations don't match, process instructions and compare args let left_ins = left_obj.arch.process_instruction(left_resolved, diff_config)?; let right_ins = left_obj.arch.process_instruction(right_resolved, diff_config)?; if left_ins.args.len() != right_ins.args.len() { @@ -455,8 +455,8 @@ fn diff_instruction( right_row, a, b, - left_resolved.relocation, - right_resolved.relocation, + left_resolved, + right_resolved, diff_config, ) { result.left_args_diff.push(InstructionArgDiffIndex::NONE); @@ -500,19 +500,6 @@ fn diff_instruction( return Ok(result); } - // Compare relocations - if !reloc_eq( - left_obj, - right_obj, - left_resolved.relocation, - right_resolved.relocation, - diff_config, - ) { - state.diff_score += PENALTY_REG_DIFF; - // TODO add relocation diff to args - return Ok(InstructionDiffResult::new(InstructionDiffKind::ArgMismatch)); - } - Ok(InstructionDiffResult::new(InstructionDiffKind::None)) } diff --git a/objdiff-core/src/diff/data.rs b/objdiff-core/src/diff/data.rs index d0b6394..742e38d 100644 --- a/objdiff-core/src/diff/data.rs +++ b/objdiff-core/src/diff/data.rs @@ -63,7 +63,7 @@ fn reloc_eq( } #[inline] -fn resolve_relocation<'obj>( +pub fn resolve_relocation<'obj>( obj: &'obj Object, reloc: &'obj Relocation, ) -> ResolvedRelocation<'obj> { @@ -72,10 +72,6 @@ fn resolve_relocation<'obj>( } /// Compares relocations contained with a certain data range. -/// The DataDiffKind for each diff will either be `None`` (if the relocation matches), -/// or `Replace` (if a relocation was changed, added, or removed). -/// `Insert` and `Delete` are not used when a relocation is added or removed to avoid confusing diffs -/// where it looks like the bytes themselves were changed but actually only the relocations changed. fn diff_data_relocs_for_range<'left, 'right>( left_obj: &'left Object, right_obj: &'right Object, @@ -254,13 +250,21 @@ pub fn diff_data_section( let len = left_obj.arch.data_reloc_size(left_reloc.relocation.flags); let range = left_reloc.relocation.address as usize ..left_reloc.relocation.address as usize + len; - left_reloc_diffs.push(DataRelocationDiff { kind: diff_kind, range }); + left_reloc_diffs.push(DataRelocationDiff { + reloc: left_reloc.relocation.clone(), + kind: diff_kind, + range, + }); } if let Some(right_reloc) = right_reloc { let len = right_obj.arch.data_reloc_size(right_reloc.relocation.flags); let range = right_reloc.relocation.address as usize ..right_reloc.relocation.address as usize + len; - right_reloc_diffs.push(DataRelocationDiff { kind: diff_kind, range }); + right_reloc_diffs.push(DataRelocationDiff { + reloc: right_reloc.relocation.clone(), + kind: diff_kind, + range, + }); } } diff --git a/objdiff-core/src/diff/display.rs b/objdiff-core/src/diff/display.rs index 8a972a5..459a0e6 100644 --- a/objdiff-core/src/diff/display.rs +++ b/objdiff-core/src/diff/display.rs @@ -15,7 +15,7 @@ use crate::{ diff::{DiffObjConfig, InstructionDiffKind, InstructionDiffRow, ObjectDiff, SymbolDiff}, obj::{ InstructionArg, InstructionArgValue, Object, ParsedInstruction, ResolvedInstructionRef, - SectionFlag, SectionKind, Symbol, SymbolFlag, SymbolKind, + ResolvedRelocation, SectionFlag, SectionKind, Symbol, SymbolFlag, SymbolKind, }, }; @@ -424,6 +424,42 @@ pub fn symbol_hover(obj: &Object, symbol_index: usize, addend: i64) -> Vec, +) -> Vec { + let mut out = Vec::new(); + if let Some(ins) = ins { + for literal in display_ins_data_literals(obj, ins) { + out.push(ContextItem::Copy { value: literal, label: None }); + } + out.push(ContextItem::Separator); + } + out.append(&mut symbol_context(obj, reloc.relocation.target_symbol)); + out +} + +pub fn relocation_hover(obj: &Object, reloc: ResolvedRelocation) -> Vec { + let mut out = Vec::new(); + if let Some(name) = obj.arch.reloc_name(reloc.relocation.flags) { + out.push(HoverItem::Text { + label: "Relocation type".into(), + value: name.to_string(), + color: HoverItemColor::Normal, + }); + } else { + out.push(HoverItem::Text { + label: "Relocation type".into(), + value: format!("<{:?}>", reloc.relocation.flags), + color: HoverItemColor::Normal, + }); + } + out.push(HoverItem::Separator); + out.append(&mut symbol_hover(obj, reloc.relocation.target_symbol, reloc.relocation.addend)); + out +} + pub fn instruction_context( obj: &Object, resolved: ResolvedInstructionRef, @@ -458,11 +494,7 @@ pub fn instruction_context( } } if let Some(reloc) = resolved.relocation { - for literal in display_ins_data_literals(obj, resolved) { - out.push(ContextItem::Copy { value: literal, label: None }); - } - out.push(ContextItem::Separator); - out.append(&mut symbol_context(obj, reloc.relocation.target_symbol)); + out.append(&mut relocation_context(obj, reloc, Some(resolved))); } out } @@ -509,21 +541,17 @@ pub fn instruction_hover( } } if let Some(reloc) = resolved.relocation { - if let Some(name) = obj.arch.reloc_name(reloc.relocation.flags) { - out.push(HoverItem::Text { - label: "Relocation type".into(), - value: name.to_string(), - color: HoverItemColor::Normal, - }); - } else { - out.push(HoverItem::Text { - label: "Relocation type".into(), - value: format!("<{:?}>", reloc.relocation.flags), - color: HoverItemColor::Normal, - }); - } + out.append(&mut relocation_hover(obj, reloc)); out.push(HoverItem::Separator); - out.append(&mut symbol_hover(obj, reloc.relocation.target_symbol, reloc.relocation.addend)); + if let Some(ty) = obj.arch.guess_data_type(resolved) { + for literal in display_ins_data_literals(obj, resolved) { + out.push(HoverItem::Text { + label: format!("{}", ty), + value: literal, + color: HoverItemColor::Normal, + }); + } + } } out } diff --git a/objdiff-core/src/diff/mod.rs b/objdiff-core/src/diff/mod.rs index 117d383..edcdac1 100644 --- a/objdiff-core/src/diff/mod.rs +++ b/objdiff-core/src/diff/mod.rs @@ -16,7 +16,7 @@ use crate::{ diff_generic_section, }, }, - obj::{InstructionRef, Object, SectionKind, Symbol, SymbolFlag}, + obj::{InstructionRef, Object, Relocation, SectionKind, Symbol, SymbolFlag}, }; pub mod code; @@ -93,6 +93,7 @@ pub struct DataDiff { #[derive(Debug, Clone)] pub struct DataRelocationDiff { + pub reloc: Relocation, pub kind: DataDiffKind, pub range: Range, } diff --git a/objdiff-gui/src/views/data_diff.rs b/objdiff-gui/src/views/data_diff.rs index c33e87f..df6ef4f 100644 --- a/objdiff-gui/src/views/data_diff.rs +++ b/objdiff-gui/src/views/data_diff.rs @@ -2,99 +2,89 @@ use std::{cmp::min, default::Default, mem::take}; use egui::{text::LayoutJob, Label, Sense, Widget}; use objdiff_core::{ - diff::{DataDiff, DataDiffKind, DataRelocationDiff}, + diff::{ + data::resolve_relocation, + display::{relocation_context, relocation_hover, ContextItem, HoverItem}, + DataDiff, DataDiffKind, DataRelocationDiff, + }, obj::Object, }; +use super::diff::{context_menu_items_ui, hover_items_ui}; use crate::views::{appearance::Appearance, write_text}; pub(crate) const BYTES_PER_ROW: usize = 16; +fn data_row_hover(obj: &Object, diffs: &[(DataDiff, Vec)]) -> Vec { + let mut out = Vec::new(); + let reloc_diffs = diffs.iter().flat_map(|(_, reloc_diffs)| reloc_diffs); + 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: Change hover text color depending on Insert/Delete/Replace kind + // let color = get_color_for_diff_kind(reloc_diff.kind, appearance); + + let reloc = resolve_relocation(obj, reloc); + out.append(&mut relocation_hover(obj, reloc)); + } + out +} + +fn data_row_context( + obj: &Object, + diffs: &[(DataDiff, Vec)], +) -> Vec { + let mut out = Vec::new(); + let reloc_diffs = diffs.iter().flat_map(|(_, reloc_diffs)| reloc_diffs); + 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); + + let reloc = resolve_relocation(obj, reloc); + out.append(&mut relocation_context(obj, reloc, None)); + } + out +} + fn data_row_hover_ui( ui: &mut egui::Ui, - _obj: &Object, - _diffs: &[(DataDiff, Vec)], - _appearance: &Appearance, + obj: &Object, + diffs: &[(DataDiff, Vec)], + appearance: &Appearance, ) { ui.scope(|ui| { ui.style_mut().override_text_style = Some(egui::TextStyle::Monospace); ui.style_mut().wrap_mode = Some(egui::TextWrapMode::Extend); - - // TODO - // let reloc_diffs = diffs.iter().flat_map(|(_, reloc_diffs)| reloc_diffs); - // 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); - // - // let color = get_color_for_diff_kind(reloc_diff.kind, appearance); - // - // // 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), - // _ => "".to_string(), - // }; - // ui.colored_label(color, format!("Name: {}{}", reloc.target.name, addend_str)); - // if let Some(orig_section_index) = reloc.target.orig_section_index { - // if let Some(section) = - // obj.sections.iter().find(|s| s.orig_index == orig_section_index) - // { - // ui.colored_label(color, format!("Section: {}", section.name)); - // } - // ui.colored_label( - // color, - // format!("Address: {:x}{}", reloc.target.address, addend_str), - // ); - // ui.colored_label(color, format!("Size: {:x}", reloc.target.size)); - // if reloc.addend >= 0 && reloc.target.bytes.len() > reloc.addend as usize {} - // } else { - // ui.colored_label(color, "Extern".to_string()); - // } - // } + hover_items_ui(ui, data_row_hover(obj, diffs), appearance); }); } -fn data_row_context_menu(ui: &mut egui::Ui, _diffs: &[(DataDiff, Vec)]) { +fn data_row_context_menu( + ui: &mut egui::Ui, + obj: &Object, + diffs: &[(DataDiff, Vec)], + column: usize, + appearance: &Appearance, +) { ui.scope(|ui| { ui.style_mut().override_text_style = Some(egui::TextStyle::Monospace); ui.style_mut().wrap_mode = Some(egui::TextWrapMode::Extend); - - // TODO - // let reloc_diffs = diffs.iter().flat_map(|(_, reloc_diffs)| reloc_diffs); - // 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. - // if let Some(name) = &reloc.target.demangled_name { - // if ui.button(format!("Copy \"{name}\"")).clicked() { - // ui.output_mut(|output| output.copied_text.clone_from(name)); - // ui.close_menu(); - // } - // } - // if ui.button(format!("Copy \"{}\"", reloc.target.name)).clicked() { - // ui.output_mut(|output| output.copied_text.clone_from(&reloc.target.name)); - // ui.close_menu(); - // } - // } + context_menu_items_ui(ui, data_row_context(obj, diffs), column, appearance); }); } @@ -113,6 +103,7 @@ pub(crate) fn data_row_ui( address: usize, diffs: &[(DataDiff, Vec)], appearance: &Appearance, + column: usize, ) { if diffs.iter().any(|(dd, rds)| { dd.kind != DataDiffKind::None || rds.iter().any(|rd| rd.kind != DataDiffKind::None) @@ -191,9 +182,8 @@ pub(crate) fn data_row_ui( let response = Label::new(job).sense(Sense::click()).ui(ui); if let Some(obj) = obj { - response - .on_hover_ui_at_pointer(|ui| data_row_hover_ui(ui, obj, diffs, appearance)) - .context_menu(|ui| data_row_context_menu(ui, diffs)); + response.context_menu(|ui| data_row_context_menu(ui, obj, diffs, column, appearance)); + response.on_hover_ui_at_pointer(|ui| data_row_hover_ui(ui, obj, diffs, appearance)); } } diff --git a/objdiff-gui/src/views/diff.rs b/objdiff-gui/src/views/diff.rs index 026b6ea..4df91ad 100644 --- a/objdiff-gui/src/views/diff.rs +++ b/objdiff-gui/src/views/diff.rs @@ -525,9 +525,23 @@ pub fn diff_view_ui( let address = i * BYTES_PER_ROW; row.col(|ui| { if column == 0 { - data_row_ui(ui, Some(left_obj), address, &left_diffs[i], appearance); + data_row_ui( + ui, + Some(left_obj), + address, + &left_diffs[i], + appearance, + column, + ); } else if column == 1 { - data_row_ui(ui, Some(right_obj), address, &right_diffs[i], appearance); + data_row_ui( + ui, + Some(right_obj), + address, + &right_diffs[i], + appearance, + column, + ); } }); }, @@ -667,7 +681,7 @@ fn diff_col_ui( let i = row.index(); let address = i * BYTES_PER_ROW; row.col(|ui| { - data_row_ui(ui, Some(obj), address, &diffs[i], appearance); + data_row_ui(ui, Some(obj), address, &diffs[i], appearance, column); }); }, );