Improve local branch relocation handling

Reworks the local-branch handling logic to be more
unified: scan_instructions does all the work up front,
and process_instruction / display_instruction can
simply use the calculated branch destination instead
of performing their own is-relocation-target-
function-local checks.

(Hopefully) Fixes #192

Author: encounter

objdiff-core/src/arch/arm.rs

@@ -15,7 +15,7 @@ use crate::{
     diff::{ArmArchVersion, ArmR9Usage, DiffObjConfig, display::InstructionPart},
     obj::{
         InstructionRef, Relocation, RelocationFlags, ResolvedInstructionRef, ResolvedRelocation,
-        ScannedInstruction, Section, SectionKind, Symbol, SymbolFlag, SymbolFlagSet, SymbolKind,
+        Section, SectionKind, Symbol, SymbolFlag, SymbolFlagSet, SymbolKind,
     },
 };
 
@@ -187,14 +187,14 @@ impl Arch for ArchArm {
         self.disasm_modes = Self::get_mapping_symbols(sections, symbols);
     }
 
-    fn scan_instructions(
+    fn scan_instructions_internal(
         &self,
         address: u64,
         code: &[u8],
         section_index: usize,
         _relocations: &[Relocation],
         diff_config: &DiffObjConfig,
-    ) -> Result<Vec<ScannedInstruction>> {
+    ) -> Result<Vec<InstructionRef>> {
         let start_addr = address as u32;
         let end_addr = start_addr + code.len() as u32;
 
@@ -219,7 +219,7 @@ impl Arch for ArchArm {
         let mut next_mapping = mappings_iter.next();
 
         let ins_count = code.len() / mode.instruction_size(start_addr);
-        let mut ops = Vec::<ScannedInstruction>::with_capacity(ins_count);
+        let mut ops = Vec::<InstructionRef>::with_capacity(ins_count);
 
         let parse_flags = self.parse_flags(diff_config);
 
@@ -235,12 +235,10 @@ impl Arch for ArchArm {
             let data = &code[(address - start_addr) as usize..];
             if data.len() < ins_size {
                 // Push the remainder as data
-                ops.push(ScannedInstruction {
-                    ins_ref: InstructionRef {
-                        address: address as u64,
-                        size: data.len() as u8,
-                        opcode: u16::MAX,
-                    },
+                ops.push(InstructionRef {
+                    address: address as u64,
+                    size: data.len() as u8,
+                    opcode: u16::MAX,
                     branch_dest: None,
                 });
                 break;
@@ -256,12 +254,10 @@ impl Arch for ArchArm {
                 }
                 _ => {
                     // Invalid instruction size
-                    ops.push(ScannedInstruction {
-                        ins_ref: InstructionRef {
-                            address: address as u64,
-                            size: ins_size as u8,
-                            opcode: u16::MAX,
-                        },
+                    ops.push(InstructionRef {
+                        address: address as u64,
+                        size: ins_size as u8,
+                        opcode: u16::MAX,
                         branch_dest: None,
                     });
                     address += ins_size as u32;
@@ -325,8 +321,10 @@ impl Arch for ArchArm {
                 unarm::ParseMode::Data => (u16::MAX, None),
             };
 
-            ops.push(ScannedInstruction {
-                ins_ref: InstructionRef { address: address as u64, size: ins_size as u8, opcode },
+            ops.push(InstructionRef {
+                address: address as u64,
+                size: ins_size as u8,
+                opcode,
                 branch_dest: branch_dest.map(|x| x as u64),
             });
             address += ins_size as u32;

objdiff-core/src/arch/arm64.rs

@@ -18,7 +18,6 @@ use crate::{
     diff::{DiffObjConfig, display::InstructionPart},
     obj::{
         InstructionRef, Relocation, RelocationFlags, ResolvedInstructionRef, ResolvedRelocation,
-        ScannedInstruction,
     },
 };
 
@@ -30,16 +29,16 @@ impl ArchArm64 {
 }
 
 impl Arch for ArchArm64 {
-    fn scan_instructions(
+    fn scan_instructions_internal(
         &self,
         address: u64,
         code: &[u8],
         _section_index: usize,
         _relocations: &[Relocation],
         _diff_config: &DiffObjConfig,
-    ) -> Result<Vec<ScannedInstruction>> {
+    ) -> Result<Vec<InstructionRef>> {
         let start_address = address;
-        let mut ops = Vec::<ScannedInstruction>::with_capacity(code.len() / 4);
+        let mut ops = Vec::<InstructionRef>::with_capacity(code.len() / 4);
 
         let mut reader = U8Reader::new(code);
         let decoder = InstDecoder::default();
@@ -58,8 +57,10 @@ impl Arch for ArchArm64 {
                     DecodeError::InvalidOpcode
                     | DecodeError::InvalidOperand
                     | DecodeError::IncompleteDecoder => {
-                        ops.push(ScannedInstruction {
-                            ins_ref: InstructionRef { address, size: 4, opcode: u16::MAX },
+                        ops.push(InstructionRef {
+                            address,
+                            size: 4,
+                            opcode: u16::MAX,
                             branch_dest: None,
                         });
                         continue;
@@ -68,9 +69,9 @@ impl Arch for ArchArm64 {
             }
 
             let opcode = opcode_to_u16(ins.opcode);
-            let ins_ref = InstructionRef { address, size: 4, opcode };
-            let branch_dest = branch_dest(ins_ref, &code[offset as usize..offset as usize + 4]);
-            ops.push(ScannedInstruction { ins_ref, branch_dest });
+            let branch_dest =
+                branch_dest(opcode, address, &code[offset as usize..offset as usize + 4]);
+            ops.push(InstructionRef { address, size: 4, opcode, branch_dest });
         }
 
         Ok(ops)
@@ -163,7 +164,7 @@ impl Arch for ArchArm64 {
     }
 }
 
-fn branch_dest(ins_ref: InstructionRef, code: &[u8]) -> Option<u64> {
+fn branch_dest(opcode: u16, address: u64, code: &[u8]) -> Option<u64> {
     const OPCODE_B: u16 = opcode_to_u16(Opcode::B);
     const OPCODE_BL: u16 = opcode_to_u16(Opcode::BL);
     const OPCODE_BCC: u16 = opcode_to_u16(Opcode::Bcc(0));
@@ -173,21 +174,21 @@ fn branch_dest(ins_ref: InstructionRef, code: &[u8]) -> Option<u64> {
     const OPCODE_TBNZ: u16 = opcode_to_u16(Opcode::TBNZ);
 
     let word = u32::from_le_bytes(code.try_into().ok()?);
-    match ins_ref.opcode {
+    match opcode {
         OPCODE_B | OPCODE_BL => {
             let offset = ((word & 0x03ff_ffff) << 2) as i32;
             let extended_offset = (offset << 4) >> 4;
-            ins_ref.address.checked_add_signed(extended_offset as i64)
+            address.checked_add_signed(extended_offset as i64)
         }
         OPCODE_BCC | OPCODE_CBZ | OPCODE_CBNZ => {
             let offset = (word as i32 & 0x00ff_ffe0) >> 3;
             let extended_offset = (offset << 11) >> 11;
-            ins_ref.address.checked_add_signed(extended_offset as i64)
+            address.checked_add_signed(extended_offset as i64)
         }
         OPCODE_TBZ | OPCODE_TBNZ => {
             let offset = (word as i32 & 0x0007_ffe0) >> 3;
             let extended_offset = (offset << 16) >> 16;
-            ins_ref.address.checked_add_signed(extended_offset as i64)
+            address.checked_add_signed(extended_offset as i64)
         }
         _ => None,
     }

objdiff-core/src/arch/mips.rs

@@ -3,7 +3,6 @@ use alloc::{
     string::{String, ToString},
     vec::Vec,
 };
-use core::ops::Range;
 
 use anyhow::{Result, bail};
 use object::{Endian as _, Object as _, ObjectSection as _, ObjectSymbol as _, elf};
@@ -19,7 +18,7 @@ use crate::{
     diff::{DiffObjConfig, MipsAbi, MipsInstrCategory, display::InstructionPart},
     obj::{
         InstructionArg, InstructionArgValue, InstructionRef, Relocation, RelocationFlags,
-        ResolvedInstructionRef, ResolvedRelocation, ScannedInstruction, SymbolFlag, SymbolFlagSet,
+        ResolvedInstructionRef, ResolvedRelocation, SymbolFlag, SymbolFlagSet,
     },
 };
 
@@ -189,27 +188,24 @@ impl ArchMips {
 }
 
 impl Arch for ArchMips {
-    fn scan_instructions(
+    fn scan_instructions_internal(
         &self,
         address: u64,
         code: &[u8],
         _section_index: usize,
         _relocations: &[Relocation],
         diff_config: &DiffObjConfig,
-    ) -> Result<Vec<ScannedInstruction>> {
+    ) -> Result<Vec<InstructionRef>> {
         let instruction_flags = self.instruction_flags(diff_config);
-        let mut ops = Vec::<ScannedInstruction>::with_capacity(code.len() / 4);
+        let mut ops = Vec::<InstructionRef>::with_capacity(code.len() / 4);
         let mut cur_addr = address as u32;
         for chunk in code.chunks_exact(4) {
             let code = self.endianness.read_u32_bytes(chunk.try_into()?);
             let instruction =
                 rabbitizer::Instruction::new(code, Vram::new(cur_addr), instruction_flags);
             let opcode = instruction.opcode() as u16;
             let branch_dest = instruction.get_branch_vram_generic().map(|v| v.inner() as u64);
-            ops.push(ScannedInstruction {
-                ins_ref: InstructionRef { address: cur_addr as u64, size: 4, opcode },
-                branch_dest,
-            });
+            ops.push(InstructionRef { address: cur_addr as u64, size: 4, opcode, branch_dest });
             cur_addr += 4;
         }
         Ok(ops)
@@ -225,16 +221,7 @@ impl Arch for ArchMips {
         let display_flags = self.instruction_display_flags(diff_config);
         let opcode = instruction.opcode();
         cb(InstructionPart::opcode(opcode.name(), opcode as u16))?;
-        let start_address = resolved.symbol.address;
-        let function_range = start_address..start_address + resolved.symbol.size;
-        push_args(
-            &instruction,
-            resolved.relocation,
-            function_range,
-            resolved.section_index,
-            &display_flags,
-            cb,
-        )?;
+        push_args(&instruction, resolved.relocation, &display_flags, cb)?;
         Ok(())
     }
 
@@ -338,8 +325,6 @@ impl Arch for ArchMips {
 fn push_args(
     instruction: &rabbitizer::Instruction,
     relocation: Option<ResolvedRelocation>,
-    function_range: Range<u64>,
-    section_index: usize,
     display_flags: &rabbitizer::InstructionDisplayFlags,
     mut arg_cb: impl FnMut(InstructionPart) -> Result<()>,
 ) -> Result<()> {
@@ -362,23 +347,7 @@ fn push_args(
             }
             ValuedOperand::core_label(..) | ValuedOperand::core_branch_target_label(..) => {
                 if let Some(resolved) = relocation {
-                    // If the relocation target is within the current function, we can
-                    // convert it into a relative branch target. Note that we check
-                    // target_address > start_address instead of >= so that recursive
-                    // tail calls are not considered branch targets.
-                    let target_address =
-                        resolved.symbol.address.checked_add_signed(resolved.relocation.addend);
-                    if resolved.symbol.section == Some(section_index)
-                        && target_address.is_some_and(|addr| {
-                            addr > function_range.start && addr < function_range.end
-                        })
-                    {
-                        // TODO move this logic up a level
-                        let target_address = target_address.unwrap();
-                        arg_cb(InstructionPart::branch_dest(target_address))?;
-                    } else {
-                        push_reloc(resolved.relocation, &mut arg_cb)?;
-                    }
+                    push_reloc(resolved.relocation, &mut arg_cb)?;
                 } else if let Some(branch_dest) = instruction
                     .get_branch_offset_generic()
                     .map(|o| (instruction.vram() + o).inner() as u64)