From 2876be37a3808f3e2e25858dc4813e73c214578d Mon Sep 17 00:00:00 2001 From: LagoLunatic Date: Sat, 18 Jan 2025 18:18:05 -0500 Subject: [PATCH] Show relocation diffs in function view when the data's content differs (#153) * Show reloc diff in func view when data content differs * Add "Relax shifted data diffs" option * Display fake pool relocations at end of line * 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. * Fix weak stripped symbols showing as a false diff Fixed this by showing extern symbols correctly instead of skipping them. * Add "Relax shifted data diffs" option to objdiff-cli Includes both a command line argument and a keyboard shortcut (S). * Remove addi string data hack and ... pool name hack * Clippy fix * PPC: Clear relocs from GPRs when overwritten * PPC: Follow branches to improve pool detection accuracy * PPC: Handle following bctr jump table control flow * Clippy fixes * PPC: Fix extern relocations not having their addend copied * Add option to disable func data value diffing * PPC: Handle lmw when clearing GPRs * PPC: Handle moving reloc address with `add` inst * Combine "relax reloc diffs" with other reloc diff options * Add v3 config and migrate from v2 --------- Co-authored-by: Luke Street --- objdiff-cli/src/cmd/report.rs | 5 +- objdiff-cli/src/views/function_diff.rs | 13 +- objdiff-core/config-schema.json | 30 ++- objdiff-core/src/arch/mod.rs | 11 + objdiff-core/src/arch/ppc.rs | 354 ++++++++++++++++++------- objdiff-core/src/diff/code.rs | 33 ++- objdiff-gui/src/app_config.rs | 131 ++++++++- objdiff-gui/src/views/function_diff.rs | 8 +- 8 files changed, 452 insertions(+), 133 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 0595084..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}, - ObjDiff, ObjInsDiffKind, ObjSymbolDiff, + FunctionRelocDiffs, ObjDiff, ObjInsDiffKind, ObjSymbolDiff, }, obj::{ObjInfo, ObjSectionKind, ObjSymbol, SymbolRef}, }; @@ -368,10 +368,15 @@ 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; + 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 c4fa69d..1cc24e9 100644 --- a/objdiff-core/config-schema.json +++ b/objdiff-core/config-schema.json @@ -1,11 +1,29 @@ { "properties": [ { - "id": "relaxRelocDiffs", - "type": "boolean", - "default": false, - "name": "Relax relocation diffs", - "description": "Ignores differences in relocation targets. (Address, name, etc)" + "id": "functionRelocDiffs", + "type": "choice", + "default": "name_address", + "name": "Function relocation diffs", + "description": "How relocation targets will be diffed in the function view.", + "items": [ + { + "value": "none", + "name": "None" + }, + { + "value": "name_address", + "name": "Name or address" + }, + { + "value": "data_value", + "name": "Data value" + }, + { + "value": "all", + "name": "Name or address, data value" + } + ] }, { "id": "spaceBetweenArgs", @@ -193,7 +211,7 @@ "id": "general", "name": "General", "properties": [ - "relaxRelocDiffs", + "functionRelocDiffs", "spaceBetweenArgs", "combineDataSections" ] 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/arch/ppc.rs b/objdiff-core/src/arch/ppc.rs index 3768a53..4788824 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}; @@ -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}, @@ -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 { @@ -198,19 +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 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 - } + guess_data_type_from_load_store_inst_op(Opcode::from(instruction.op as u8)) } fn display_data_type(&self, ty: DataType, bytes: &[u8]) -> Option { @@ -248,6 +245,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:?}"), @@ -442,16 +445,43 @@ 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))), + ( + 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, } } } +// 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, 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 { + 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); + } + } +} + // 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 @@ -461,35 +491,51 @@ 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; + 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 + // 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. + target = 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![], + }; + // 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 + // 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. + 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: fake_target_symbol, - addend: fake_addend, + target, + addend, }) } @@ -497,78 +543,184 @@ 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 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( - 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); + 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) { + // Avoid getting stuck in an infinite loop when following looping branches. + break; + } + visited_ins_addrs.insert(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]) { - ( - 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` + let simplified = ins.simplified(); + + // 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 { + let dest = cur_addr.wrapping_add_signed(dest.0); + branch_dest = Some(dest); + break; } - ( - 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` - } - (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!(), } } - _ => {} } - } 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 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()); } - 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); + } + + // 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. + 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); + } + } + } 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 gpr_pool_relocs); + } + } else { + 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) + .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..]; + ins_iters_with_gpr_state.push(( + InsIter::new(dest_code_slice, unseen_addr), + gpr_pool_relocs.clone(), + )); + break; } } } diff --git a/objdiff-core/src/diff/code.rs b/objdiff-core/src/diff/code.rs index 4e1d6ce..3a14e3c 100644 --- a/objdiff-core/src/diff/code.rs +++ b/objdiff-core/src/diff/code.rs @@ -3,13 +3,17 @@ use std::{cmp::max, collections::BTreeMap}; use anyhow::{anyhow, Result}; use similar::{capture_diff_slices_deadline, Algorithm}; +use super::FunctionRelocDiffs; use crate::{ arch::ProcessCodeResult, diff::{ DiffObjConfig, ObjInsArgDiff, ObjInsBranchFrom, ObjInsBranchTo, ObjInsDiff, ObjInsDiffKind, ObjSymbolDiff, }, - obj::{ObjInfo, ObjInsArg, ObjReloc, ObjSection, ObjSymbol, ObjSymbolFlags, SymbolRef}, + obj::{ + ObjInfo, ObjIns, ObjInsArg, ObjReloc, ObjSection, ObjSymbol, ObjSymbolFlags, ObjSymbolKind, + SymbolRef, + }, }; pub fn process_code_symbol( @@ -215,16 +219,19 @@ 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 { return false; } - if config.relax_reloc_diffs { + if config.function_reloc_diffs == FunctionRelocDiffs::None { return true; } @@ -233,7 +240,13 @@ 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_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)) } (Some(_), None) => false, (None, Some(_)) => { @@ -262,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 => { @@ -271,8 +284,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 { @@ -283,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 2647949..19d28dc 100644 --- a/objdiff-gui/src/app_config.rs +++ b/objdiff-gui/src/app_config.rs @@ -3,8 +3,11 @@ use std::path::PathBuf; use eframe::Storage; use globset::Glob; use objdiff_core::{ - config::ScratchConfig, - diff::{ArmArchVersion, ArmR9Usage, DiffObjConfig, MipsAbi, MipsInstrCategory, X86Formatter}, + config::{ScratchConfig, SymbolMappings}, + diff::{ + ArmArchVersion, ArmR9Usage, DiffObjConfig, FunctionRelocDiffs, MipsAbi, MipsInstrCategory, + X86Formatter, + }, }; use crate::app::{AppConfig, ObjectConfig, CONFIG_KEY}; @@ -15,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. @@ -23,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); @@ -49,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)] @@ -147,7 +264,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, 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());