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());