From 85e5fd99babc4262654b979f0be66a31925e99de Mon Sep 17 00:00:00 2001 From: LagoLunatic Date: Fri, 3 Jan 2025 15:34:06 -0500 Subject: [PATCH 01/18] Show reloc diff in func view when data content differs --- objdiff-core/src/diff/code.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/objdiff-core/src/diff/code.rs b/objdiff-core/src/diff/code.rs index e645a24..3264040 100644 --- a/objdiff-core/src/diff/code.rs +++ b/objdiff-core/src/diff/code.rs @@ -9,7 +9,10 @@ use crate::{ DiffObjConfig, ObjInsArgDiff, ObjInsBranchFrom, ObjInsBranchTo, ObjInsDiff, ObjInsDiffKind, ObjSymbolDiff, }, - obj::{ObjInfo, ObjInsArg, ObjReloc, ObjSection, ObjSymbol, ObjSymbolFlags, SymbolRef}, + obj::{ + ObjInfo, ObjInsArg, ObjReloc, ObjSection, ObjSymbol, ObjSymbolFlags, ObjSymbolKind, + SymbolRef, + }, }; pub fn process_code_symbol( @@ -234,6 +237,9 @@ fn reloc_eq( // Match if section and name or address match section_name_eq(left_obj, right_obj, *sl, *sr) && (symbol_name_matches || address_eq(left, right)) + && (left.target.kind != ObjSymbolKind::Object + || right.target.name.starts_with("...") + || left.target.bytes == right.target.bytes) } (Some(_), None) => false, (None, Some(_)) => { From 5346d7baada91030979613d7e8d143f20f1ae3d5 Mon Sep 17 00:00:00 2001 From: LagoLunatic Date: Fri, 3 Jan 2025 15:36:34 -0500 Subject: [PATCH 02/18] Add "Relax shifted data diffs" option --- objdiff-core/src/diff/code.rs | 4 +++- objdiff-core/src/diff/mod.rs | 2 ++ objdiff-gui/src/app.rs | 12 ++++++++++++ 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/objdiff-core/src/diff/code.rs b/objdiff-core/src/diff/code.rs index 3264040..7e3ab91 100644 --- a/objdiff-core/src/diff/code.rs +++ b/objdiff-core/src/diff/code.rs @@ -236,7 +236,9 @@ fn reloc_eq( (Some(sl), Some(sr)) => { // Match if section and name or address match section_name_eq(left_obj, right_obj, *sl, *sr) - && (symbol_name_matches || address_eq(left, right)) + && (symbol_name_matches + || address_eq(left, right) + || config.relax_shifted_data_diffs) && (left.target.kind != ObjSymbolKind::Object || right.target.name.starts_with("...") || left.target.bytes == right.target.bytes) diff --git a/objdiff-core/src/diff/mod.rs b/objdiff-core/src/diff/mod.rs index 5a17e90..874d40f 100644 --- a/objdiff-core/src/diff/mod.rs +++ b/objdiff-core/src/diff/mod.rs @@ -160,6 +160,7 @@ const fn default_true() -> bool { true } #[serde(default)] pub struct DiffObjConfig { pub relax_reloc_diffs: bool, + pub relax_shifted_data_diffs: bool, #[serde(default = "default_true")] pub space_between_args: bool, pub combine_data_sections: bool, @@ -184,6 +185,7 @@ impl Default for DiffObjConfig { fn default() -> Self { Self { relax_reloc_diffs: false, + relax_shifted_data_diffs: false, space_between_args: true, combine_data_sections: false, symbol_mappings: Default::default(), diff --git a/objdiff-gui/src/app.rs b/objdiff-gui/src/app.rs index d6f6b66..e4af1ed 100644 --- a/objdiff-gui/src/app.rs +++ b/objdiff-gui/src/app.rs @@ -740,6 +740,18 @@ impl eframe::App for App { { state.queue_reload = true; } + if ui + .checkbox( + &mut state.config.diff_obj_config.relax_shifted_data_diffs, + "Relax shifted data diffs", + ) + .on_hover_text( + "Ignores differences in addresses for symbols with matching data.", + ) + .changed() + { + state.queue_reload = true; + } if ui .checkbox( &mut state.config.diff_obj_config.space_between_args, From a191c1d2aad92df4e68a752d5b32370a93c4440b Mon Sep 17 00:00:00 2001 From: LagoLunatic Date: Fri, 3 Jan 2025 17:04:45 -0500 Subject: [PATCH 03/18] Display fake pool relocations at end of line --- objdiff-core/src/arch/ppc.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/objdiff-core/src/arch/ppc.rs b/objdiff-core/src/arch/ppc.rs index 3768a53..0d78a2f 100644 --- a/objdiff-core/src/arch/ppc.rs +++ b/objdiff-core/src/arch/ppc.rs @@ -143,6 +143,15 @@ impl ObjArch for ObjArchPpc { } } + if reloc.is_none() { + if let Some(fake_pool_reloc) = fake_pool_reloc_for_addr.get(&cur_addr) { + // If this instruction has a fake pool relocation, show it as a fake argument + // at the end of the line. + args.push(ObjInsArg::PlainText(" ".into())); + push_reloc(&mut args, fake_pool_reloc)?; + } + } + ops.push(ins.op as u16); let line = line_info.range(..=cur_addr as u64).last().map(|(_, &b)| b); insts.push(ObjIns { @@ -248,6 +257,12 @@ fn push_reloc(args: &mut Vec, reloc: &ObjReloc) -> Result<()> { elf::R_PPC_ADDR32 | elf::R_PPC_UADDR32 | elf::R_PPC_REL24 | elf::R_PPC_REL14 => { args.push(ObjInsArg::Reloc); } + elf::R_PPC_NONE => { + // Fake pool relocation. + args.push(ObjInsArg::PlainText("<".into())); + args.push(ObjInsArg::Reloc); + args.push(ObjInsArg::PlainText(">".into())); + } _ => bail!("Unsupported ELF PPC relocation type {r_type}"), }, flags => bail!("Unsupported PPC relocation kind: {flags:?}"), From 729f40624aade94b7a17396a1591571ee1f5ce46 Mon Sep 17 00:00:00 2001 From: LagoLunatic Date: Fri, 3 Jan 2025 17:12:21 -0500 Subject: [PATCH 04/18] Diff reloc data by display string instead of raw bytes This is to handle data symbols that contain multiple values in them at once, such as stringBase. If you compare the target symbol's bytes directly, then any part of the symbol having different bytes will cause *all* relocations to that symbol to show as a diff, even if the specific string being accessed is the same. --- objdiff-core/src/arch/mod.rs | 11 +++++++++++ objdiff-core/src/diff/code.rs | 18 +++++++++++------- objdiff-gui/src/views/function_diff.rs | 8 ++------ 3 files changed, 24 insertions(+), 13 deletions(-) diff --git a/objdiff-core/src/arch/mod.rs b/objdiff-core/src/arch/mod.rs index 82579dd..da76ca4 100644 --- a/objdiff-core/src/arch/mod.rs +++ b/objdiff-core/src/arch/mod.rs @@ -156,6 +156,17 @@ pub trait ObjArch: Send + Sync { Some(format!("Bytes: {:#x?}", bytes)) } + fn display_ins_data(&self, ins: &ObjIns) -> Option { + let reloc = ins.reloc.as_ref()?; + if reloc.addend >= 0 && reloc.target.bytes.len() > reloc.addend as usize { + self.guess_data_type(ins).and_then(|ty| { + self.display_data_type(ty, &reloc.target.bytes[reloc.addend as usize..]) + }) + } else { + None + } + } + // Downcast methods #[cfg(feature = "ppc")] fn ppc(&self) -> Option<&ppc::ObjArchPpc> { None } diff --git a/objdiff-core/src/diff/code.rs b/objdiff-core/src/diff/code.rs index 7e3ab91..0d0230d 100644 --- a/objdiff-core/src/diff/code.rs +++ b/objdiff-core/src/diff/code.rs @@ -10,7 +10,7 @@ use crate::{ ObjSymbolDiff, }, obj::{ - ObjInfo, ObjInsArg, ObjReloc, ObjSection, ObjSymbol, ObjSymbolFlags, ObjSymbolKind, + ObjInfo, ObjIns, ObjInsArg, ObjReloc, ObjSection, ObjSymbol, ObjSymbolFlags, ObjSymbolKind, SymbolRef, }, }; @@ -218,10 +218,13 @@ fn reloc_eq( config: &DiffObjConfig, left_obj: &ObjInfo, right_obj: &ObjInfo, - left_reloc: Option<&ObjReloc>, - right_reloc: Option<&ObjReloc>, + left_ins: Option<&ObjIns>, + right_ins: Option<&ObjIns>, ) -> bool { - let (Some(left), Some(right)) = (left_reloc, right_reloc) else { + let (Some(left_ins), Some(right_ins)) = (left_ins, right_ins) else { + return false; + }; + let (Some(left), Some(right)) = (&left_ins.reloc, &right_ins.reloc) else { return false; }; if left.flags != right.flags { @@ -241,7 +244,8 @@ fn reloc_eq( || config.relax_shifted_data_diffs) && (left.target.kind != ObjSymbolKind::Object || right.target.name.starts_with("...") - || left.target.bytes == right.target.bytes) + || left_obj.arch.display_ins_data(left_ins) + == left_obj.arch.display_ins_data(right_ins)) } (Some(_), None) => false, (None, Some(_)) => { @@ -279,8 +283,8 @@ fn arg_eq( config, left_obj, right_obj, - left_diff.ins.as_ref().and_then(|i| i.reloc.as_ref()), - right_diff.ins.as_ref().and_then(|i| i.reloc.as_ref()), + left_diff.ins.as_ref(), + right_diff.ins.as_ref(), ) } ObjInsArg::BranchDest(_) => match right { diff --git a/objdiff-gui/src/views/function_diff.rs b/objdiff-gui/src/views/function_diff.rs index dc44973..b2d3bbd 100644 --- a/objdiff-gui/src/views/function_diff.rs +++ b/objdiff-gui/src/views/function_diff.rs @@ -149,12 +149,8 @@ fn ins_hover_ui( appearance.highlight_color, format!("Size: {:x}", reloc.target.size), ); - if reloc.addend >= 0 && reloc.target.bytes.len() > reloc.addend as usize { - if let Some(s) = obj.arch.guess_data_type(ins).and_then(|ty| { - obj.arch.display_data_type(ty, &reloc.target.bytes[reloc.addend as usize..]) - }) { - ui.colored_label(appearance.highlight_color, s); - } + if let Some(s) = obj.arch.display_ins_data(ins) { + ui.colored_label(appearance.highlight_color, s); } } else { ui.colored_label(appearance.highlight_color, "Extern".to_string()); From a28d1084029e8cd0f6b78b8e8970ea5634419b8b Mon Sep 17 00:00:00 2001 From: LagoLunatic Date: Fri, 3 Jan 2025 18:23:30 -0500 Subject: [PATCH 05/18] Fix weak stripped symbols showing as a false diff Fixed this by showing extern symbols correctly instead of skipping them. --- objdiff-core/src/arch/ppc.rs | 55 ++++++++++++++++++++++-------------- 1 file changed, 34 insertions(+), 21 deletions(-) diff --git a/objdiff-core/src/arch/ppc.rs b/objdiff-core/src/arch/ppc.rs index 0d78a2f..fa52c81 100644 --- a/objdiff-core/src/arch/ppc.rs +++ b/objdiff-core/src/arch/ppc.rs @@ -476,26 +476,39 @@ fn get_offset_and_addr_gpr_for_possible_pool_reference( fn make_fake_pool_reloc(offset: i16, cur_addr: u32, pool_reloc: &ObjReloc) -> Option { let offset_from_pool = pool_reloc.addend + offset as i64; let target_address = pool_reloc.target.address.checked_add_signed(offset_from_pool)?; - let orig_section_index = pool_reloc.target.orig_section_index?; - // We also need to create a fake target symbol to go inside our fake relocation. - // This is because we don't have access to list of all symbols in this section, so we can't find - // the real symbol yet. Instead we make a placeholder that has the correct `orig_section_index` - // and `address` fields, and then later on when this information is displayed to the user, we - // can find the real symbol by searching through the object's section's symbols for one that - // contains this address. - let fake_target_symbol = ObjSymbol { - name: "".to_string(), - demangled_name: None, - address: target_address, - section_address: 0, - size: 0, - size_known: false, - kind: Default::default(), - flags: Default::default(), - orig_section_index: Some(orig_section_index), - virtual_address: None, - original_index: None, - bytes: vec![], + let target_symbol = if pool_reloc.target.orig_section_index.is_some() { + // If the target symbol is within this current object, then we also need to create a fake + // target symbol to go inside our fake relocation. This is because we don't have access to + // list of all symbols in this section, so we can't find the real symbol within the pool + // based on its address yet. Instead we make a placeholder that has the correct + // `orig_section_index` and `address` fields, and then later on when this information is + // displayed to the user, we can find the real symbol by searching through the object's + // section's symbols for one that contains this address. + ObjSymbol { + name: "".to_string(), + demangled_name: None, + address: target_address, + section_address: 0, + size: 0, + size_known: false, + kind: Default::default(), + flags: Default::default(), + orig_section_index: pool_reloc.target.orig_section_index, + virtual_address: None, + original_index: None, + bytes: vec![], + } + } else { + // But if the target symbol is in a different object (extern), then we simply copy the pool + // relocation's target. This is because it won't be possible to locate the actual symbol + // later on based only off of an offset without knowing the object or section it's in. And + // doing that for external symbols would also be unnecessary, because when the compiler + // generates an instruction that accesses an external "pool" plus some offset, that won't be + // a normal pool that contains other symbols within it that we want to display. It will be + // something like a vtable for a class with multiple inheritance (for example, dCcD_Cyl in + // The Wind Waker). So just showing that vtable symbol plus an addend to represent the + // offset into it works fine in this case, no fake symbol to hold an address is necessary. + pool_reloc.target.clone() }; // The addend is also fake because we don't know yet if the `target_address` here is the exact // start of the symbol or if it's in the middle of it. @@ -503,7 +516,7 @@ fn make_fake_pool_reloc(offset: i16, cur_addr: u32, pool_reloc: &ObjReloc) -> Op Some(ObjReloc { flags: RelocationFlags::Elf { r_type: elf::R_PPC_NONE }, address: cur_addr as u64, - target: fake_target_symbol, + target: target_symbol, addend: fake_addend, }) } From 4b78958a35adc746b0a785600b56c27979c092b7 Mon Sep 17 00:00:00 2001 From: LagoLunatic Date: Fri, 3 Jan 2025 18:57:35 -0500 Subject: [PATCH 06/18] Add "Relax shifted data diffs" option to objdiff-cli Includes both a command line argument and a keyboard shortcut (S). --- objdiff-cli/src/cmd/diff.rs | 7 +++++++ objdiff-cli/src/views/function_diff.rs | 6 ++++++ 2 files changed, 13 insertions(+) diff --git a/objdiff-cli/src/cmd/diff.rs b/objdiff-cli/src/cmd/diff.rs index 6ef8ea0..abebf6e 100644 --- a/objdiff-cli/src/cmd/diff.rs +++ b/objdiff-cli/src/cmd/diff.rs @@ -66,6 +66,9 @@ pub struct Args { #[argp(switch, short = 'x')] /// Relax relocation diffs relax_reloc_diffs: bool, + #[argp(switch, short = 's')] + /// Relax shifted data diffs + relax_shifted_data_diffs: bool, #[argp(option, short = 'o')] /// Output file (one-shot mode) ("-" for stdout) output: Option, @@ -202,6 +205,7 @@ fn run_oneshot( let output_format = OutputFormat::from_option(args.format.as_deref())?; let config = diff::DiffObjConfig { relax_reloc_diffs: args.relax_reloc_diffs, + relax_shifted_data_diffs: args.relax_shifted_data_diffs, ..Default::default() // TODO }; let target = target_path @@ -230,6 +234,7 @@ pub struct AppState { pub reload_time: Option, pub time_format: Vec>, pub relax_reloc_diffs: bool, + pub relax_shifted_data_diffs: bool, pub watcher: Option, pub modified: Arc, } @@ -259,6 +264,7 @@ fn create_objdiff_config(state: &AppState) -> ObjDiffConfig { base_path: state.base_path.clone(), diff_obj_config: diff::DiffObjConfig { relax_reloc_diffs: state.relax_reloc_diffs, + relax_shifted_data_diffs: state.relax_shifted_data_diffs, ..Default::default() // TODO }, symbol_mappings: Default::default(), @@ -327,6 +333,7 @@ fn run_interactive( reload_time: None, time_format, relax_reloc_diffs: args.relax_reloc_diffs, + relax_shifted_data_diffs: args.relax_shifted_data_diffs, watcher: None, modified: Default::default(), }; diff --git a/objdiff-cli/src/views/function_diff.rs b/objdiff-cli/src/views/function_diff.rs index 112fb50..b9b310c 100644 --- a/objdiff-cli/src/views/function_diff.rs +++ b/objdiff-cli/src/views/function_diff.rs @@ -374,6 +374,12 @@ impl UiView for FunctionDiffUi { result.redraw = true; return EventControlFlow::Reload; } + // Toggle relax shifted data diffs + KeyCode::Char('s') => { + state.relax_shifted_data_diffs = !state.relax_shifted_data_diffs; + result.redraw = true; + return EventControlFlow::Reload; + } // Toggle three-way diff KeyCode::Char('3') => { self.three_way = !self.three_way; From 72a3f4714026c061261cd27bf3a79b7f877cc5d4 Mon Sep 17 00:00:00 2001 From: LagoLunatic Date: Fri, 3 Jan 2025 19:32:35 -0500 Subject: [PATCH 07/18] Remove addi string data hack and ... pool name hack --- objdiff-core/src/arch/ppc.rs | 7 ------- objdiff-core/src/diff/code.rs | 1 - 2 files changed, 8 deletions(-) diff --git a/objdiff-core/src/arch/ppc.rs b/objdiff-core/src/arch/ppc.rs index fa52c81..eee4ddf 100644 --- a/objdiff-core/src/arch/ppc.rs +++ b/objdiff-core/src/arch/ppc.rs @@ -210,13 +210,6 @@ impl ObjArch for ObjArchPpc { let op = Opcode::from(instruction.op as u8); if let Some(ty) = guess_data_type_from_load_store_inst_op(op) { Some(ty) - } else if op == Opcode::Addi { - // Assume that any addi instruction that references a local symbol is loading a string. - // This hack is not ideal and results in tons of false positives where it will show - // garbage strings (e.g. misinterpreting arrays, float literals, etc). - // But not all strings are in the @stringBase pool, so the condition above that checks - // the target symbol name would miss some. - Some(DataType::String) } else { None } diff --git a/objdiff-core/src/diff/code.rs b/objdiff-core/src/diff/code.rs index 0d0230d..d1cd509 100644 --- a/objdiff-core/src/diff/code.rs +++ b/objdiff-core/src/diff/code.rs @@ -243,7 +243,6 @@ fn reloc_eq( || address_eq(left, right) || config.relax_shifted_data_diffs) && (left.target.kind != ObjSymbolKind::Object - || right.target.name.starts_with("...") || left_obj.arch.display_ins_data(left_ins) == left_obj.arch.display_ins_data(right_ins)) } From 5a086fd97763383c6df8c8a21612f5f877d3ea27 Mon Sep 17 00:00:00 2001 From: LagoLunatic Date: Fri, 3 Jan 2025 19:53:31 -0500 Subject: [PATCH 08/18] Clippy fix --- objdiff-core/src/arch/ppc.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/objdiff-core/src/arch/ppc.rs b/objdiff-core/src/arch/ppc.rs index eee4ddf..a9d4c11 100644 --- a/objdiff-core/src/arch/ppc.rs +++ b/objdiff-core/src/arch/ppc.rs @@ -207,12 +207,7 @@ impl ObjArch for ObjArchPpc { return Some(DataType::String); } - let op = Opcode::from(instruction.op as u8); - if let Some(ty) = guess_data_type_from_load_store_inst_op(op) { - Some(ty) - } else { - None - } + guess_data_type_from_load_store_inst_op(Opcode::from(instruction.op as u8)) } fn display_data_type(&self, ty: DataType, bytes: &[u8]) -> Option { From 0c7df234238fd2420044eed3f7264e4f6aef47e7 Mon Sep 17 00:00:00 2001 From: LagoLunatic Date: Sat, 4 Jan 2025 17:10:25 -0500 Subject: [PATCH 09/18] PPC: Clear relocs from GPRs when overwritten --- objdiff-core/src/arch/ppc.rs | 33 ++++++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/objdiff-core/src/arch/ppc.rs b/objdiff-core/src/arch/ppc.rs index a9d4c11..d1be1a6 100644 --- a/objdiff-core/src/arch/ppc.rs +++ b/objdiff-core/src/arch/ppc.rs @@ -10,7 +10,7 @@ use object::{ elf, File, Object, ObjectSection, ObjectSymbol, Relocation, RelocationFlags, RelocationTarget, Symbol, SymbolKind, }; -use ppc750cl::{Argument, InsIter, Opcode, ParsedIns, GPR}; +use ppc750cl::{Argument, Arguments, Ins, InsIter, Opcode, ParsedIns, GPR}; use crate::{ arch::{DataType, ObjArch, ProcessCodeResult}, @@ -445,16 +445,29 @@ fn get_offset_and_addr_gpr_for_possible_pool_reference( Argument::Simm(simm), ) => Some((simm.0, addr_src_gpr, Some(addr_dst_gpr))), ( + // `mr` or `mr.` Opcode::Or, Argument::GPR(addr_dst_gpr), Argument::GPR(addr_src_gpr), Argument::None, - ) => Some((0, addr_src_gpr, Some(addr_dst_gpr))), // `mr` or `mr.` + ) => Some((0, addr_src_gpr, Some(addr_dst_gpr))), _ => None, } } } +// Remove the relocation we're keeping track of in a particular register when an instruction reuses +// that register to hold some other value, unrelated to pool relocation addresses. +fn clear_overwritten_gprs(ins: Ins, active_pool_relocs: &mut HashMap) { + let mut def_args = Arguments::default(); + ins.parse_defs(&mut def_args); + for arg in def_args { + if let Argument::GPR(gpr) = arg { + active_pool_relocs.remove(&gpr.0); + } + } +} + // We create a fake relocation for an instruction, vaguely simulating what the actual relocation // might have looked like if it wasn't pooled. This is so minimal changes are needed to display // pooled accesses vs non-pooled accesses. We set the relocation type to R_PPC_NONE to indicate that @@ -537,20 +550,22 @@ fn generate_fake_pool_reloc_for_addr_mapping( let args = &simplified.args; match (ins.op, args[0], args[1], args[2]) { ( + // `lis` + `addi` Opcode::Addi, Argument::GPR(addr_dst_gpr), Argument::GPR(_addr_src_gpr), Argument::Simm(_simm), ) => { - active_pool_relocs.insert(addr_dst_gpr.0, reloc.clone()); // `lis` + `addi` + active_pool_relocs.insert(addr_dst_gpr.0, reloc.clone()); } ( + // `lis` + `ori` Opcode::Ori, Argument::GPR(addr_dst_gpr), Argument::GPR(_addr_src_gpr), Argument::Uimm(_uimm), ) => { - active_pool_relocs.insert(addr_dst_gpr.0, reloc.clone()); // `lis` + `ori` + active_pool_relocs.insert(addr_dst_gpr.0, reloc.clone()); } (Opcode::B, _, _, _) => { if simplified.mnemonic == "bl" { @@ -562,7 +577,9 @@ fn generate_fake_pool_reloc_for_addr_mapping( } } } - _ => {} + _ => { + clear_overwritten_gprs(ins, &mut active_pool_relocs); + } } } else if let Some((offset, addr_src_gpr, addr_dst_gpr)) = get_offset_and_addr_gpr_for_possible_pool_reference(ins.op, &simplified) @@ -585,8 +602,14 @@ fn generate_fake_pool_reloc_for_addr_mapping( let mut new_reloc = pool_reloc.clone(); new_reloc.addend += offset as i64; active_pool_relocs.insert(addr_dst_gpr.0, new_reloc); + } else { + clear_overwritten_gprs(ins, &mut active_pool_relocs); } + } else { + clear_overwritten_gprs(ins, &mut active_pool_relocs); } + } else { + clear_overwritten_gprs(ins, &mut active_pool_relocs); } } From 2132946397675c32198a841a71ca84ab66e6240d Mon Sep 17 00:00:00 2001 From: LagoLunatic Date: Sat, 4 Jan 2025 17:33:04 -0500 Subject: [PATCH 10/18] PPC: Follow branches to improve pool detection accuracy --- objdiff-core/src/arch/ppc.rs | 200 ++++++++++++++++++++++------------- 1 file changed, 128 insertions(+), 72 deletions(-) diff --git a/objdiff-core/src/arch/ppc.rs b/objdiff-core/src/arch/ppc.rs index d1be1a6..aa907e3 100644 --- a/objdiff-core/src/arch/ppc.rs +++ b/objdiff-core/src/arch/ppc.rs @@ -1,6 +1,6 @@ use std::{ borrow::Cow, - collections::{BTreeMap, HashMap}, + collections::{BTreeMap, HashMap, HashSet}, }; use anyhow::{bail, ensure, Result}; @@ -458,12 +458,12 @@ fn get_offset_and_addr_gpr_for_possible_pool_reference( // Remove the relocation we're keeping track of in a particular register when an instruction reuses // that register to hold some other value, unrelated to pool relocation addresses. -fn clear_overwritten_gprs(ins: Ins, active_pool_relocs: &mut HashMap) { +fn clear_overwritten_gprs(ins: Ins, gpr_pool_relocs: &mut HashMap) { let mut def_args = Arguments::default(); ins.parse_defs(&mut def_args); for arg in def_args { if let Argument::GPR(gpr) = arg { - active_pool_relocs.remove(&gpr.0); + gpr_pool_relocs.remove(&gpr.0); } } } @@ -526,90 +526,146 @@ fn make_fake_pool_reloc(offset: i16, cur_addr: u32, pool_reloc: &ObjReloc) -> Op // of pooled data relocations in them, finding which instructions load data from those addresses, // and constructing a mapping of the address of that instruction to a "fake pool relocation" that // simulates what that instruction's relocation would look like if data hadn't been pooled. -// Limitations: This method currently only goes through the instructions in a function in linear -// order, from start to finish. It does *not* follow any branches. This means that it could have -// false positives or false negatives in determining which relocation is currently loaded in which -// register at any given point in the function, as control flow is not respected. -// There are currently no known examples of this method producing inaccurate results in reality, but -// if examples are found, it may be possible to update this method to also follow all branches so -// that it produces more accurate results. +// This method tries to follow the function's proper control flow. It keeps track of a queue of +// states it hasn't traversed yet, where each state holds an instruction address and a HashMap of +// which registers hold which pool relocations at that point. +// When a conditional or unconditional branch is encountered, the destination of the branch is added +// to the queue. Conditional branches will traverse both the path where the branch is taken and the +// one where it's not. Unconditional branches only follow the branch, ignoring any code immediately +// after the branch instruction. +// Limitations: This method cannot follow jump tables. This is because the jump table is located in +// the .data section, but ObjArch.process_code only has access to the .text section. This means that +// it will miss most of the cases in a switch statement that uses a jump table. fn generate_fake_pool_reloc_for_addr_mapping( - address: u64, + func_address: u64, code: &[u8], relocations: &[ObjReloc], ) -> HashMap { - let mut active_pool_relocs = HashMap::new(); + let mut visited_ins_addrs = HashSet::new(); let mut pool_reloc_for_addr = HashMap::new(); - for (cur_addr, ins) in InsIter::new(code, address as u32) { - let simplified = ins.simplified(); - let reloc = relocations.iter().find(|r| (r.address as u32 & !3) == cur_addr); - - if let Some(reloc) = reloc { - // This instruction has a real relocation, so it may be a pool load we want to keep - // track of. - let args = &simplified.args; - match (ins.op, args[0], args[1], args[2]) { - ( - // `lis` + `addi` - Opcode::Addi, - Argument::GPR(addr_dst_gpr), - Argument::GPR(_addr_src_gpr), - Argument::Simm(_simm), - ) => { - active_pool_relocs.insert(addr_dst_gpr.0, reloc.clone()); - } - ( - // `lis` + `ori` - Opcode::Ori, - Argument::GPR(addr_dst_gpr), - Argument::GPR(_addr_src_gpr), - Argument::Uimm(_uimm), - ) => { - active_pool_relocs.insert(addr_dst_gpr.0, reloc.clone()); + let mut ins_iters_with_gpr_state = + vec![(InsIter::new(code, func_address as u32), HashMap::new())]; + while let Some((ins_iter, mut gpr_pool_relocs)) = ins_iters_with_gpr_state.pop() { + for (cur_addr, ins) in ins_iter { + if visited_ins_addrs.contains(&cur_addr) { + // Avoid getting stuck in an infinite loop when following looping branches. + break; + } + visited_ins_addrs.insert(cur_addr); + + let simplified = ins.simplified(); + let reloc = relocations.iter().find(|r| (r.address as u32 & !3) == cur_addr); + + let mut branch_dest = None; + for arg in simplified.args_iter() { + if let Argument::BranchDest(dest) = arg { + let dest = cur_addr.wrapping_add_signed(dest.0); + branch_dest = Some(dest); + break; } - (Opcode::B, _, _, _) => { - if simplified.mnemonic == "bl" { - // When encountering a function call, clear any active pool relocations from - // the volatile registers (r0, r3-r12), but not the nonvolatile registers. - active_pool_relocs.remove(&0); - for gpr in 3..12 { - active_pool_relocs.remove(&gpr); + } + if let Some(branch_dest) = branch_dest { + if branch_dest >= func_address as u32 + && (branch_dest - func_address as u32) < code.len() as u32 + { + let dest_offset_into_func = branch_dest - func_address as u32; + let dest_code_slice = &code[dest_offset_into_func as usize..]; + match ins.op { + Opcode::Bc => { + // Conditional branch. + // Add the branch destination to the queue to do later. + ins_iters_with_gpr_state.push(( + InsIter::new(dest_code_slice, branch_dest), + gpr_pool_relocs.clone(), + )); + // Then continue on with the current iterator. } + Opcode::B => { + if simplified.mnemonic != "bl" { + // Unconditional branch. + // Add the branch destination to the queue. + ins_iters_with_gpr_state.push(( + InsIter::new(dest_code_slice, branch_dest), + gpr_pool_relocs.clone(), + )); + // Break out of the current iterator so we can do the newly added one. + break; + } + } + _ => unreachable!(), } } - _ => { - clear_overwritten_gprs(ins, &mut active_pool_relocs); - } } - } else if let Some((offset, addr_src_gpr, addr_dst_gpr)) = - get_offset_and_addr_gpr_for_possible_pool_reference(ins.op, &simplified) - { - // This instruction doesn't have a real relocation, so it may be a reference to one of - // the already-loaded pools. - if let Some(pool_reloc) = active_pool_relocs.get(&addr_src_gpr.0) { - if let Some(fake_pool_reloc) = make_fake_pool_reloc(offset, cur_addr, pool_reloc) { - pool_reloc_for_addr.insert(cur_addr, fake_pool_reloc); + + if let Some(reloc) = reloc { + // This instruction has a real relocation, so it may be a pool load we want to keep + // track of. + let args = &simplified.args; + match (ins.op, args[0], args[1], args[2]) { + ( + // `lis` + `addi` + Opcode::Addi, + Argument::GPR(addr_dst_gpr), + Argument::GPR(_addr_src_gpr), + Argument::Simm(_simm), + ) => { + gpr_pool_relocs.insert(addr_dst_gpr.0, reloc.clone()); + } + ( + // `lis` + `ori` + Opcode::Ori, + Argument::GPR(addr_dst_gpr), + Argument::GPR(_addr_src_gpr), + Argument::Uimm(_uimm), + ) => { + gpr_pool_relocs.insert(addr_dst_gpr.0, reloc.clone()); + } + (Opcode::B, _, _, _) => { + if simplified.mnemonic == "bl" { + // When encountering a function call, clear any active pool relocations from + // the volatile registers (r0, r3-r12), but not the nonvolatile registers. + gpr_pool_relocs.remove(&0); + for gpr in 3..12 { + gpr_pool_relocs.remove(&gpr); + } + } + } + _ => { + clear_overwritten_gprs(ins, &mut gpr_pool_relocs); + } } - if let Some(addr_dst_gpr) = addr_dst_gpr { - // If the address of the pool relocation got copied into another register, we - // need to keep track of it in that register too as future instructions may - // reference the symbol indirectly via this new register, instead of the - // register the symbol's address was originally loaded into. - // For example, the start of the function might `lis` + `addi` the start of the - // ...data pool into r25, and then later the start of a loop will `addi` r25 - // with the offset within the .data section of an array variable into r21. - // Then the body of the loop will `lwzx` one of the array elements from r21. - let mut new_reloc = pool_reloc.clone(); - new_reloc.addend += offset as i64; - active_pool_relocs.insert(addr_dst_gpr.0, new_reloc); + } else if let Some((offset, addr_src_gpr, addr_dst_gpr)) = + get_offset_and_addr_gpr_for_possible_pool_reference(ins.op, &simplified) + { + // This instruction doesn't have a real relocation, so it may be a reference to one of + // the already-loaded pools. + if let Some(pool_reloc) = gpr_pool_relocs.get(&addr_src_gpr.0) { + if let Some(fake_pool_reloc) = + make_fake_pool_reloc(offset, cur_addr, pool_reloc) + { + pool_reloc_for_addr.insert(cur_addr, fake_pool_reloc); + } + if let Some(addr_dst_gpr) = addr_dst_gpr { + // If the address of the pool relocation got copied into another register, we + // need to keep track of it in that register too as future instructions may + // reference the symbol indirectly via this new register, instead of the + // register the symbol's address was originally loaded into. + // For example, the start of the function might `lis` + `addi` the start of the + // ...data pool into r25, and then later the start of a loop will `addi` r25 + // with the offset within the .data section of an array variable into r21. + // Then the body of the loop will `lwzx` one of the array elements from r21. + let mut new_reloc = pool_reloc.clone(); + new_reloc.addend += offset as i64; + gpr_pool_relocs.insert(addr_dst_gpr.0, new_reloc); + } else { + clear_overwritten_gprs(ins, &mut gpr_pool_relocs); + } } else { - clear_overwritten_gprs(ins, &mut active_pool_relocs); + clear_overwritten_gprs(ins, &mut gpr_pool_relocs); } } else { - clear_overwritten_gprs(ins, &mut active_pool_relocs); + clear_overwritten_gprs(ins, &mut gpr_pool_relocs); } - } else { - clear_overwritten_gprs(ins, &mut active_pool_relocs); } } From 4449ff19f285e75ab12ac011b31677edebaefc80 Mon Sep 17 00:00:00 2001 From: LagoLunatic Date: Sat, 4 Jan 2025 19:09:55 -0500 Subject: [PATCH 11/18] PPC: Handle following bctr jump table control flow --- objdiff-core/src/arch/ppc.rs | 51 +++++++++++++++++++++++++++++++++--- 1 file changed, 47 insertions(+), 4 deletions(-) diff --git a/objdiff-core/src/arch/ppc.rs b/objdiff-core/src/arch/ppc.rs index aa907e3..910d059 100644 --- a/objdiff-core/src/arch/ppc.rs +++ b/objdiff-core/src/arch/ppc.rs @@ -533,9 +533,13 @@ fn make_fake_pool_reloc(offset: i16, cur_addr: u32, pool_reloc: &ObjReloc) -> Op // to the queue. Conditional branches will traverse both the path where the branch is taken and the // one where it's not. Unconditional branches only follow the branch, ignoring any code immediately // after the branch instruction. -// Limitations: This method cannot follow jump tables. This is because the jump table is located in -// the .data section, but ObjArch.process_code only has access to the .text section. This means that -// it will miss most of the cases in a switch statement that uses a jump table. +// Limitations: This method cannot read jump tables. This is because the jump tables are located in +// the .data section, but ObjArch.process_code only has access to the .text section. In order to +// work around this limitation and avoid completely missing most code inside switch statements that +// use jump tables, we instead guess that any parts of a function we missed were switch cases, and +// traverse them as if the last `bctr` before that address had branched there. This should be fairly +// accurate in practice - in testing the only instructions it seems to miss are double branches that +// the compiler generates in error which can never be reached during normal execution anyway. fn generate_fake_pool_reloc_for_addr_mapping( func_address: u64, code: &[u8], @@ -545,6 +549,7 @@ fn generate_fake_pool_reloc_for_addr_mapping( let mut pool_reloc_for_addr = HashMap::new(); let mut ins_iters_with_gpr_state = vec![(InsIter::new(code, func_address as u32), HashMap::new())]; + let mut gpr_state_at_bctr = BTreeMap::new(); while let Some((ins_iter, mut gpr_pool_relocs)) = ins_iters_with_gpr_state.pop() { for (cur_addr, ins) in ins_iter { if visited_ins_addrs.contains(&cur_addr) { @@ -554,8 +559,8 @@ fn generate_fake_pool_reloc_for_addr_mapping( visited_ins_addrs.insert(cur_addr); let simplified = ins.simplified(); - let reloc = relocations.iter().find(|r| (r.address as u32 & !3) == cur_addr); + // First handle traversing the function's control flow. let mut branch_dest = None; for arg in simplified.args_iter() { if let Argument::BranchDest(dest) = arg { @@ -596,7 +601,19 @@ fn generate_fake_pool_reloc_for_addr_mapping( } } } + match ins.op { + Opcode::Bcctr => { + if simplified.mnemonic == "bctr" { + // Unconditional branch to count register. + // Likely a jump table. + gpr_state_at_bctr.insert(cur_addr, gpr_pool_relocs.clone()); + } + } + _ => {} + } + // Then handle keeping track of which GPR contains which pool relocation. + let reloc = relocations.iter().find(|r| (r.address as u32 & !3) == cur_addr); if let Some(reloc) = reloc { // This instruction has a real relocation, so it may be a pool load we want to keep // track of. @@ -667,6 +684,32 @@ fn generate_fake_pool_reloc_for_addr_mapping( clear_overwritten_gprs(ins, &mut gpr_pool_relocs); } } + + // Finally, if we're about to finish the outer loop and don't have any more control flow to + // follow, we check if there are any instruction addresses in this function that we missed. + // If so, and if there were any `bctr` instructions before those points in this function, + // then we try to traverse those missing spots as switch cases. + if ins_iters_with_gpr_state.is_empty() { + let unseen_addrs = (func_address as u32..func_address as u32 + code.len() as u32) + .step_by(4) + .filter(|addr| !visited_ins_addrs.contains(&addr)); + for unseen_addr in unseen_addrs { + let prev_bctr_gpr_state = gpr_state_at_bctr + .iter() + .filter(|(&addr, _)| addr < unseen_addr) + .min_by_key(|(&addr, _)| addr) + .and_then(|(_, gpr_state)| Some(gpr_state)); + if let Some(gpr_pool_relocs) = prev_bctr_gpr_state { + let dest_offset_into_func = unseen_addr - func_address as u32; + let dest_code_slice = &code[dest_offset_into_func as usize..]; + ins_iters_with_gpr_state.push(( + InsIter::new(dest_code_slice, unseen_addr), + gpr_pool_relocs.clone(), + )); + break; + } + } + } } pool_reloc_for_addr From e5cba3ec6e005b9892783531066bb55c92341212 Mon Sep 17 00:00:00 2001 From: LagoLunatic Date: Sat, 4 Jan 2025 19:16:10 -0500 Subject: [PATCH 12/18] Clippy fixes --- objdiff-core/src/arch/ppc.rs | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/objdiff-core/src/arch/ppc.rs b/objdiff-core/src/arch/ppc.rs index 910d059..a5b23f8 100644 --- a/objdiff-core/src/arch/ppc.rs +++ b/objdiff-core/src/arch/ppc.rs @@ -601,15 +601,12 @@ fn generate_fake_pool_reloc_for_addr_mapping( } } } - match ins.op { - Opcode::Bcctr => { - if simplified.mnemonic == "bctr" { - // Unconditional branch to count register. - // Likely a jump table. - gpr_state_at_bctr.insert(cur_addr, gpr_pool_relocs.clone()); - } + if let Opcode::Bcctr = ins.op { + if simplified.mnemonic == "bctr" { + // Unconditional branch to count register. + // Likely a jump table. + gpr_state_at_bctr.insert(cur_addr, gpr_pool_relocs.clone()); } - _ => {} } // Then handle keeping track of which GPR contains which pool relocation. @@ -692,13 +689,13 @@ fn generate_fake_pool_reloc_for_addr_mapping( if ins_iters_with_gpr_state.is_empty() { let unseen_addrs = (func_address as u32..func_address as u32 + code.len() as u32) .step_by(4) - .filter(|addr| !visited_ins_addrs.contains(&addr)); + .filter(|addr| !visited_ins_addrs.contains(addr)); for unseen_addr in unseen_addrs { let prev_bctr_gpr_state = gpr_state_at_bctr .iter() .filter(|(&addr, _)| addr < unseen_addr) .min_by_key(|(&addr, _)| addr) - .and_then(|(_, gpr_state)| Some(gpr_state)); + .map(|(_, gpr_state)| gpr_state); if let Some(gpr_pool_relocs) = prev_bctr_gpr_state { let dest_offset_into_func = unseen_addr - func_address as u32; let dest_code_slice = &code[dest_offset_into_func as usize..]; From 1fe16c1c90fe8411e51f2a399d49a3bdb8856c80 Mon Sep 17 00:00:00 2001 From: LagoLunatic Date: Sat, 4 Jan 2025 19:29:31 -0500 Subject: [PATCH 13/18] PPC: Fix extern relocations not having their addend copied --- objdiff-core/src/arch/ppc.rs | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/objdiff-core/src/arch/ppc.rs b/objdiff-core/src/arch/ppc.rs index a5b23f8..ca5978b 100644 --- a/objdiff-core/src/arch/ppc.rs +++ b/objdiff-core/src/arch/ppc.rs @@ -477,7 +477,9 @@ fn clear_overwritten_gprs(ins: Ins, gpr_pool_relocs: &mut HashMap) fn make_fake_pool_reloc(offset: i16, cur_addr: u32, pool_reloc: &ObjReloc) -> Option { let offset_from_pool = pool_reloc.addend + offset as i64; let target_address = pool_reloc.target.address.checked_add_signed(offset_from_pool)?; - let target_symbol = if pool_reloc.target.orig_section_index.is_some() { + let target; + let addend; + if pool_reloc.target.orig_section_index.is_some() { // If the target symbol is within this current object, then we also need to create a fake // target symbol to go inside our fake relocation. This is because we don't have access to // list of all symbols in this section, so we can't find the real symbol within the pool @@ -485,7 +487,7 @@ fn make_fake_pool_reloc(offset: i16, cur_addr: u32, pool_reloc: &ObjReloc) -> Op // `orig_section_index` and `address` fields, and then later on when this information is // displayed to the user, we can find the real symbol by searching through the object's // section's symbols for one that contains this address. - ObjSymbol { + target = ObjSymbol { name: "".to_string(), demangled_name: None, address: target_address, @@ -498,7 +500,10 @@ fn make_fake_pool_reloc(offset: i16, cur_addr: u32, pool_reloc: &ObjReloc) -> Op virtual_address: None, original_index: None, bytes: vec![], - } + }; + // The addend is also fake because we don't know yet if the `target_address` here is the exact + // start of the symbol or if it's in the middle of it. + addend = 0; } else { // But if the target symbol is in a different object (extern), then we simply copy the pool // relocation's target. This is because it won't be possible to locate the actual symbol @@ -509,16 +514,14 @@ fn make_fake_pool_reloc(offset: i16, cur_addr: u32, pool_reloc: &ObjReloc) -> Op // something like a vtable for a class with multiple inheritance (for example, dCcD_Cyl in // The Wind Waker). So just showing that vtable symbol plus an addend to represent the // offset into it works fine in this case, no fake symbol to hold an address is necessary. - pool_reloc.target.clone() + target = pool_reloc.target.clone(); + addend = pool_reloc.addend; }; - // The addend is also fake because we don't know yet if the `target_address` here is the exact - // start of the symbol or if it's in the middle of it. - let fake_addend = 0; Some(ObjReloc { flags: RelocationFlags::Elf { r_type: elf::R_PPC_NONE }, address: cur_addr as u64, - target: target_symbol, - addend: fake_addend, + target, + addend, }) } From c716a817b553cbb79cf8ca6d91ef0ab19a08ac6b Mon Sep 17 00:00:00 2001 From: LagoLunatic Date: Wed, 8 Jan 2025 14:32:04 -0500 Subject: [PATCH 14/18] Add option to disable func data value diffing --- objdiff-cli/src/views/function_diff.rs | 12 ++++++++---- objdiff-core/config-schema.json | 26 ++++++++++++++++++++------ objdiff-core/src/diff/code.rs | 6 ++++-- 3 files changed, 32 insertions(+), 12 deletions(-) diff --git a/objdiff-cli/src/views/function_diff.rs b/objdiff-cli/src/views/function_diff.rs index 2ec6215..c6921a5 100644 --- a/objdiff-cli/src/views/function_diff.rs +++ b/objdiff-cli/src/views/function_diff.rs @@ -3,7 +3,7 @@ use crossterm::event::{Event, KeyCode, KeyEventKind, KeyModifiers, MouseButton, use objdiff_core::{ diff::{ display::{display_diff, DiffText, HighlightKind}, - ObjDiff, ObjInsDiffKind, ObjSymbolDiff, + FunctionDataDiffs, ObjDiff, ObjInsDiffKind, ObjSymbolDiff, }, obj::{ObjInfo, ObjSectionKind, ObjSymbol, SymbolRef}, }; @@ -375,10 +375,14 @@ impl UiView for FunctionDiffUi { result.redraw = true; return EventControlFlow::Reload; } - // Toggle relax shifted data diffs + // Cycle through function data diff mode KeyCode::Char('s') => { - state.diff_obj_config.relax_shifted_data_diffs = - !state.diff_obj_config.relax_shifted_data_diffs; + state.diff_obj_config.function_data_diffs = + match state.diff_obj_config.function_data_diffs { + FunctionDataDiffs::AddressOnly => FunctionDataDiffs::ValueOnly, + FunctionDataDiffs::ValueOnly => FunctionDataDiffs::All, + FunctionDataDiffs::All => FunctionDataDiffs::AddressOnly, + }; result.redraw = true; return EventControlFlow::Reload; } diff --git a/objdiff-core/config-schema.json b/objdiff-core/config-schema.json index 72c0eb4..d1bab40 100644 --- a/objdiff-core/config-schema.json +++ b/objdiff-core/config-schema.json @@ -8,11 +8,25 @@ "description": "Ignores differences in relocation targets. (Address, name, etc)" }, { - "id": "relaxShiftedDataDiffs", - "type": "boolean", - "default": false, - "name": "Relax shifted data diffs", - "description": "Ignores differences in addresses for symbols with matching data." + "id": "functionDataDiffs", + "type": "choice", + "default": "address_only", + "name": "Function data diffs", + "description": "How data relocations will be diffed in the function view.", + "items": [ + { + "value": "address_only", + "name": "Address only" + }, + { + "value": "value_only", + "name": "Value only" + }, + { + "value": "all", + "name": "Value and address" + } + ] }, { "id": "spaceBetweenArgs", @@ -201,7 +215,7 @@ "name": "General", "properties": [ "relaxRelocDiffs", - "relaxShiftedDataDiffs", + "functionDataDiffs", "spaceBetweenArgs", "combineDataSections" ] diff --git a/objdiff-core/src/diff/code.rs b/objdiff-core/src/diff/code.rs index 661ec52..2ba65ae 100644 --- a/objdiff-core/src/diff/code.rs +++ b/objdiff-core/src/diff/code.rs @@ -3,6 +3,7 @@ use std::{cmp::max, collections::BTreeMap}; use anyhow::{anyhow, Result}; use similar::{capture_diff_slices_deadline, Algorithm}; +use super::FunctionDataDiffs; use crate::{ arch::ProcessCodeResult, diff::{ @@ -241,8 +242,9 @@ fn reloc_eq( section_name_eq(left_obj, right_obj, *sl, *sr) && (symbol_name_matches || address_eq(left, right) - || config.relax_shifted_data_diffs) - && (left.target.kind != ObjSymbolKind::Object + || config.function_data_diffs == FunctionDataDiffs::ValueOnly) + && (config.function_data_diffs == FunctionDataDiffs::AddressOnly + || left.target.kind != ObjSymbolKind::Object || left_obj.arch.display_ins_data(left_ins) == left_obj.arch.display_ins_data(right_ins)) } From db9769b46fb36a3e3a848e3a003ecac2080cb767 Mon Sep 17 00:00:00 2001 From: LagoLunatic Date: Wed, 8 Jan 2025 14:53:44 -0500 Subject: [PATCH 15/18] PPC: Handle lmw when clearing GPRs --- objdiff-core/src/arch/ppc.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/objdiff-core/src/arch/ppc.rs b/objdiff-core/src/arch/ppc.rs index ca5978b..092906e 100644 --- a/objdiff-core/src/arch/ppc.rs +++ b/objdiff-core/src/arch/ppc.rs @@ -463,6 +463,14 @@ fn clear_overwritten_gprs(ins: Ins, gpr_pool_relocs: &mut HashMap) ins.parse_defs(&mut def_args); for arg in def_args { if let Argument::GPR(gpr) = arg { + if ins.op == Opcode::Lmw { + // `lmw` overwrites all registers from rd to r31. + // ppc750cl only returns rd itself, so we manually clear the rest of them. + for reg in gpr.0..31 { + gpr_pool_relocs.remove(®); + } + break; + } gpr_pool_relocs.remove(&gpr.0); } } From 5726997ea1cd3e5f3b60c543b3acb5e127f1b47a Mon Sep 17 00:00:00 2001 From: LagoLunatic Date: Fri, 10 Jan 2025 17:12:17 -0500 Subject: [PATCH 16/18] PPC: Handle moving reloc address with `add` inst --- objdiff-core/src/arch/ppc.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/objdiff-core/src/arch/ppc.rs b/objdiff-core/src/arch/ppc.rs index 092906e..4788824 100644 --- a/objdiff-core/src/arch/ppc.rs +++ b/objdiff-core/src/arch/ppc.rs @@ -451,6 +451,12 @@ fn get_offset_and_addr_gpr_for_possible_pool_reference( Argument::GPR(addr_src_gpr), Argument::None, ) => Some((0, addr_src_gpr, Some(addr_dst_gpr))), + ( + Opcode::Add, + Argument::GPR(addr_dst_gpr), + Argument::GPR(addr_src_gpr), + Argument::GPR(_offset_gpr), + ) => Some((0, addr_src_gpr, Some(addr_dst_gpr))), _ => None, } } From 1cb3a7ac3e1fa799284db0f4ce587454a9dd5742 Mon Sep 17 00:00:00 2001 From: LagoLunatic Date: Fri, 10 Jan 2025 17:17:08 -0500 Subject: [PATCH 17/18] Combine "relax reloc diffs" with other reloc diff options --- objdiff-cli/src/cmd/report.rs | 5 +++- objdiff-cli/src/views/function_diff.rs | 22 +++++++----------- objdiff-core/config-schema.json | 32 +++++++++++--------------- objdiff-core/src/diff/code.rs | 16 ++++++------- objdiff-gui/src/app_config.rs | 12 +++++++--- 5 files changed, 43 insertions(+), 44 deletions(-) diff --git a/objdiff-cli/src/cmd/report.rs b/objdiff-cli/src/cmd/report.rs index 701aa0f..ab74c11 100644 --- a/objdiff-cli/src/cmd/report.rs +++ b/objdiff-cli/src/cmd/report.rs @@ -169,7 +169,10 @@ fn report_object( } _ => {} } - let diff_config = diff::DiffObjConfig { relax_reloc_diffs: true, ..Default::default() }; + let diff_config = diff::DiffObjConfig { + function_reloc_diffs: diff::FunctionRelocDiffs::None, + ..Default::default() + }; let mapping_config = diff::MappingConfig::default(); let target = object .target_path diff --git a/objdiff-cli/src/views/function_diff.rs b/objdiff-cli/src/views/function_diff.rs index c6921a5..44dde3e 100644 --- a/objdiff-cli/src/views/function_diff.rs +++ b/objdiff-cli/src/views/function_diff.rs @@ -3,7 +3,7 @@ use crossterm::event::{Event, KeyCode, KeyEventKind, KeyModifiers, MouseButton, use objdiff_core::{ diff::{ display::{display_diff, DiffText, HighlightKind}, - FunctionDataDiffs, ObjDiff, ObjInsDiffKind, ObjSymbolDiff, + FunctionRelocDiffs, ObjDiff, ObjInsDiffKind, ObjSymbolDiff, }, obj::{ObjInfo, ObjSectionKind, ObjSymbol, SymbolRef}, }; @@ -368,20 +368,14 @@ impl UiView for FunctionDiffUi { self.scroll_x = self.scroll_x.saturating_sub(1); result.redraw = true; } - // Toggle relax relocation diffs + // Cycle through function relocation diff mode KeyCode::Char('x') => { - state.diff_obj_config.relax_reloc_diffs = - !state.diff_obj_config.relax_reloc_diffs; - result.redraw = true; - return EventControlFlow::Reload; - } - // Cycle through function data diff mode - KeyCode::Char('s') => { - state.diff_obj_config.function_data_diffs = - match state.diff_obj_config.function_data_diffs { - FunctionDataDiffs::AddressOnly => FunctionDataDiffs::ValueOnly, - FunctionDataDiffs::ValueOnly => FunctionDataDiffs::All, - FunctionDataDiffs::All => FunctionDataDiffs::AddressOnly, + state.diff_obj_config.function_reloc_diffs = + match state.diff_obj_config.function_reloc_diffs { + FunctionRelocDiffs::None => FunctionRelocDiffs::NameAddress, + FunctionRelocDiffs::NameAddress => FunctionRelocDiffs::DataValue, + FunctionRelocDiffs::DataValue => FunctionRelocDiffs::All, + FunctionRelocDiffs::All => FunctionRelocDiffs::None, }; result.redraw = true; return EventControlFlow::Reload; diff --git a/objdiff-core/config-schema.json b/objdiff-core/config-schema.json index d1bab40..1cc24e9 100644 --- a/objdiff-core/config-schema.json +++ b/objdiff-core/config-schema.json @@ -1,30 +1,27 @@ { "properties": [ { - "id": "relaxRelocDiffs", - "type": "boolean", - "default": false, - "name": "Relax relocation diffs", - "description": "Ignores differences in relocation targets. (Address, name, etc)" - }, - { - "id": "functionDataDiffs", + "id": "functionRelocDiffs", "type": "choice", - "default": "address_only", - "name": "Function data diffs", - "description": "How data relocations will be diffed in the function view.", + "default": "name_address", + "name": "Function relocation diffs", + "description": "How relocation targets will be diffed in the function view.", "items": [ { - "value": "address_only", - "name": "Address only" + "value": "none", + "name": "None" + }, + { + "value": "name_address", + "name": "Name or address" }, { - "value": "value_only", - "name": "Value only" + "value": "data_value", + "name": "Data value" }, { "value": "all", - "name": "Value and address" + "name": "Name or address, data value" } ] }, @@ -214,8 +211,7 @@ "id": "general", "name": "General", "properties": [ - "relaxRelocDiffs", - "functionDataDiffs", + "functionRelocDiffs", "spaceBetweenArgs", "combineDataSections" ] diff --git a/objdiff-core/src/diff/code.rs b/objdiff-core/src/diff/code.rs index 2ba65ae..3a14e3c 100644 --- a/objdiff-core/src/diff/code.rs +++ b/objdiff-core/src/diff/code.rs @@ -3,7 +3,7 @@ use std::{cmp::max, collections::BTreeMap}; use anyhow::{anyhow, Result}; use similar::{capture_diff_slices_deadline, Algorithm}; -use super::FunctionDataDiffs; +use super::FunctionRelocDiffs; use crate::{ arch::ProcessCodeResult, diff::{ @@ -231,7 +231,7 @@ fn reloc_eq( if left.flags != right.flags { return false; } - if config.relax_reloc_diffs { + if config.function_reloc_diffs == FunctionRelocDiffs::None { return true; } @@ -240,10 +240,10 @@ fn reloc_eq( (Some(sl), Some(sr)) => { // Match if section and name or address match section_name_eq(left_obj, right_obj, *sl, *sr) - && (symbol_name_matches - || address_eq(left, right) - || config.function_data_diffs == FunctionDataDiffs::ValueOnly) - && (config.function_data_diffs == FunctionDataDiffs::AddressOnly + && (config.function_reloc_diffs == FunctionRelocDiffs::DataValue + || symbol_name_matches + || address_eq(left, right)) + && (config.function_reloc_diffs == FunctionRelocDiffs::NameAddress || left.target.kind != ObjSymbolKind::Object || left_obj.arch.display_ins_data(left_ins) == left_obj.arch.display_ins_data(right_ins)) @@ -275,7 +275,7 @@ fn arg_eq( ObjInsArg::Arg(r) => l.loose_eq(r), // If relocations are relaxed, match if left is a constant and right is a reloc // Useful for instances where the target object is created without relocations - ObjInsArg::Reloc => config.relax_reloc_diffs, + ObjInsArg::Reloc => config.function_reloc_diffs == FunctionRelocDiffs::None, _ => false, }, ObjInsArg::Reloc => { @@ -296,7 +296,7 @@ fn arg_eq( } // If relocations are relaxed, match if left is a constant and right is a reloc // Useful for instances where the target object is created without relocations - ObjInsArg::Reloc => config.relax_reloc_diffs, + ObjInsArg::Reloc => config.function_reloc_diffs == FunctionRelocDiffs::None, _ => false, }, } diff --git a/objdiff-gui/src/app_config.rs b/objdiff-gui/src/app_config.rs index 965324c..6fdf780 100644 --- a/objdiff-gui/src/app_config.rs +++ b/objdiff-gui/src/app_config.rs @@ -4,7 +4,10 @@ use eframe::Storage; use globset::Glob; use objdiff_core::{ config::ScratchConfig, - diff::{ArmArchVersion, ArmR9Usage, DiffObjConfig, MipsAbi, MipsInstrCategory, X86Formatter}, + diff::{ + ArmArchVersion, ArmR9Usage, DiffObjConfig, FunctionRelocDiffs, MipsAbi, MipsInstrCategory, + X86Formatter, + }, }; use crate::app::{AppConfig, ObjectConfig, CONFIG_KEY}; @@ -147,7 +150,11 @@ impl Default for DiffObjConfigV1 { impl DiffObjConfigV1 { fn into_config(self) -> DiffObjConfig { DiffObjConfig { - relax_reloc_diffs: self.relax_reloc_diffs, + function_reloc_diffs: if self.relax_reloc_diffs { + FunctionRelocDiffs::None + } else { + FunctionRelocDiffs::default() + }, space_between_args: self.space_between_args, combine_data_sections: self.combine_data_sections, x86_formatter: self.x86_formatter, @@ -160,7 +167,6 @@ impl DiffObjConfigV1 { arm_sl_usage: self.arm_sl_usage, arm_fp_usage: self.arm_fp_usage, arm_ip_usage: self.arm_ip_usage, - ..Default::default() } } } From b11db433d6a20c2737ebe6e059935e92f10e1ac0 Mon Sep 17 00:00:00 2001 From: Luke Street Date: Sat, 18 Jan 2025 16:13:16 -0700 Subject: [PATCH 18/18] Add v3 config and migrate from v2 --- objdiff-gui/src/app_config.rs | 120 +++++++++++++++++++++++++++++++++- 1 file changed, 117 insertions(+), 3 deletions(-) diff --git a/objdiff-gui/src/app_config.rs b/objdiff-gui/src/app_config.rs index 6fdf780..19d28dc 100644 --- a/objdiff-gui/src/app_config.rs +++ b/objdiff-gui/src/app_config.rs @@ -3,7 +3,7 @@ use std::path::PathBuf; use eframe::Storage; use globset::Glob; use objdiff_core::{ - config::ScratchConfig, + config::{ScratchConfig, SymbolMappings}, diff::{ ArmArchVersion, ArmR9Usage, DiffObjConfig, FunctionRelocDiffs, MipsAbi, MipsInstrCategory, X86Formatter, @@ -18,7 +18,7 @@ pub struct AppConfigVersion { } impl Default for AppConfigVersion { - fn default() -> Self { Self { version: 2 } } + fn default() -> Self { Self { version: 3 } } } /// Deserialize the AppConfig from storage, handling upgrades from older versions. @@ -26,7 +26,8 @@ pub fn deserialize_config(storage: &dyn Storage) -> Option { let str = storage.get_string(CONFIG_KEY)?; match ron::from_str::(&str) { Ok(version) => match version.version { - 2 => from_str::(&str), + 3 => from_str::(&str), + 2 => from_str::(&str).map(|c| c.into_config()), 1 => from_str::(&str).map(|c| c.into_config()), _ => { log::warn!("Unknown config version: {}", version.version); @@ -52,6 +53,119 @@ where T: serde::de::DeserializeOwned { } } +#[derive(serde::Deserialize, serde::Serialize)] +pub struct ScratchConfigV2 { + #[serde(default)] + pub platform: Option, + #[serde(default)] + pub compiler: Option, + #[serde(default)] + pub c_flags: Option, + #[serde(default)] + pub ctx_path: Option, + #[serde(default)] + pub build_ctx: Option, + #[serde(default)] + pub preset_id: Option, +} + +impl ScratchConfigV2 { + fn into_config(self) -> ScratchConfig { + ScratchConfig { + platform: self.platform, + compiler: self.compiler, + c_flags: self.c_flags, + ctx_path: self.ctx_path, + build_ctx: self.build_ctx, + preset_id: self.preset_id, + } + } +} + +#[derive(serde::Deserialize, serde::Serialize)] +pub struct ObjectConfigV2 { + pub name: String, + pub target_path: Option, + pub base_path: Option, + pub reverse_fn_order: Option, + pub complete: Option, + pub scratch: Option, + pub source_path: Option, + #[serde(default)] + pub symbol_mappings: SymbolMappings, +} + +impl ObjectConfigV2 { + fn into_config(self) -> ObjectConfig { + ObjectConfig { + name: self.name, + target_path: self.target_path, + base_path: self.base_path, + reverse_fn_order: self.reverse_fn_order, + complete: self.complete, + scratch: self.scratch.map(|scratch| scratch.into_config()), + source_path: self.source_path, + symbol_mappings: self.symbol_mappings, + } + } +} + +#[derive(serde::Deserialize, serde::Serialize)] +pub struct AppConfigV2 { + pub version: u32, + #[serde(default)] + pub custom_make: Option, + #[serde(default)] + pub custom_args: Option>, + #[serde(default)] + pub selected_wsl_distro: Option, + #[serde(default)] + pub project_dir: Option, + #[serde(default)] + pub target_obj_dir: Option, + #[serde(default)] + pub base_obj_dir: Option, + #[serde(default)] + pub selected_obj: Option, + #[serde(default = "bool_true")] + pub build_base: bool, + #[serde(default)] + pub build_target: bool, + #[serde(default = "bool_true")] + pub rebuild_on_changes: bool, + #[serde(default)] + pub auto_update_check: bool, + #[serde(default)] + pub watch_patterns: Vec, + #[serde(default)] + pub recent_projects: Vec, + #[serde(default)] + pub diff_obj_config: DiffObjConfigV1, +} + +impl AppConfigV2 { + fn into_config(self) -> AppConfig { + log::info!("Upgrading configuration from v2"); + AppConfig { + custom_make: self.custom_make, + custom_args: self.custom_args, + selected_wsl_distro: self.selected_wsl_distro, + project_dir: self.project_dir, + target_obj_dir: self.target_obj_dir, + base_obj_dir: self.base_obj_dir, + selected_obj: self.selected_obj.map(|obj| obj.into_config()), + build_base: self.build_base, + build_target: self.build_target, + rebuild_on_changes: self.rebuild_on_changes, + auto_update_check: self.auto_update_check, + watch_patterns: self.watch_patterns, + recent_projects: self.recent_projects, + diff_obj_config: self.diff_obj_config.into_config(), + ..Default::default() + } + } +} + #[derive(serde::Deserialize, serde::Serialize)] pub struct ScratchConfigV1 { #[serde(default)]