MIPS relocation pairing, '$' prefix option & fixes

Implements MIPS relocation pairing logic.
New option for "Register '$' prefix", off by default.
Fixes various regressions introduced by refactoring.

Resolves #122
Resolves #156

Author: encounter

objdiff-core/config-schema.json

@@ -187,6 +187,13 @@
         }
       ]
     },
+    {
+      "id": "mips.registerPrefix",
+      "type": "boolean",
+      "default": false,
+      "name": "Register '$' prefix",
+      "description": "Display MIPS register names with a '$' prefix."
+    },
     {
       "id": "x86.formatter",
       "type": "choice",
@@ -242,7 +249,8 @@
       "name": "MIPS",
       "properties": [
         "mips.abi",
-        "mips.instrCategory"
+        "mips.instrCategory",
+        "mips.registerPrefix"
       ]
     },
     {

objdiff-core/src/arch/mips.rs

@@ -1,4 +1,4 @@
-use alloc::{string::ToString, vec::Vec};
+use alloc::{collections::BTreeMap, string::ToString, vec::Vec};
 use core::ops::Range;
 
 use anyhow::{bail, Result};
@@ -25,6 +25,7 @@ pub struct ArchMips {
     pub abi: Abi,
     pub isa_extension: Option<IsaExtension>,
     pub ri_gp_value: i32,
+    pub paired_relocations: Vec<BTreeMap<u64, i64>>,
 }
 
 const EF_MIPS_ABI: u32 = 0x0000F000;
@@ -64,16 +65,60 @@ impl ArchMips {
 
         // Parse the ri_gp_value stored in .reginfo to be able to correctly
         // calculate R_MIPS_GPREL16 relocations later. The value is stored
-        // 0x14 bytes into .reginfo (on 32 bit platforms)
+        // 0x14 bytes into .reginfo (on 32-bit platforms)
+        let endianness = object.endianness();
         let ri_gp_value = object
             .section_by_name(".reginfo")
             .and_then(|section| section.data().ok())
             .and_then(|data| data.get(0x14..0x18))
             .and_then(|s| s.try_into().ok())
-            .map(|bytes| object.endianness().read_i32_bytes(bytes))
+            .map(|bytes| endianness.read_i32_bytes(bytes))
             .unwrap_or(0);
 
-        Ok(Self { endianness: object.endianness(), abi, isa_extension, ri_gp_value })
+        // Parse all relocations to pair R_MIPS_HI16 and R_MIPS_LO16. Since the instructions only
+        // have 16-bit immediate fields, the 32-bit addend is split across the two relocations.
+        // R_MIPS_LO16 relocations without an immediately preceding R_MIPS_HI16 use the last seen
+        // R_MIPS_HI16 addend.
+        // See https://refspecs.linuxfoundation.org/elf/mipsabi.pdf pages 4-17 and 4-18
+        let mut paired_relocations = Vec::with_capacity(object.sections().count() + 1);
+        for obj_section in object.sections() {
+            let data = obj_section.data()?;
+            let mut last_hi = None;
+            let mut last_hi_addend = 0;
+            let mut addends = BTreeMap::new();
+            for (addr, reloc) in obj_section.relocations() {
+                if !reloc.has_implicit_addend() {
+                    continue;
+                }
+                match reloc.flags() {
+                    object::RelocationFlags::Elf { r_type: elf::R_MIPS_HI16 } => {
+                        let code = data[addr as usize..addr as usize + 4].try_into()?;
+                        let addend = ((endianness.read_u32_bytes(code) & 0x0000FFFF) << 16) as i32;
+                        last_hi = Some(addr);
+                        last_hi_addend = addend;
+                    }
+                    object::RelocationFlags::Elf { r_type: elf::R_MIPS_LO16 } => {
+                        let code = data[addr as usize..addr as usize + 4].try_into()?;
+                        let addend = (endianness.read_u32_bytes(code) & 0x0000FFFF) as i16 as i32;
+                        let full_addend = (last_hi_addend + addend) as i64;
+                        if let Some(hi_addr) = last_hi.take() {
+                            addends.insert(hi_addr, full_addend);
+                        }
+                        addends.insert(addr, full_addend);
+                    }
+                    _ => {
+                        last_hi = None;
+                    }
+                }
+            }
+            let section_index = obj_section.index().0;
+            if section_index >= paired_relocations.len() {
+                paired_relocations.resize_with(section_index + 1, BTreeMap::new);
+            }
+            paired_relocations[section_index] = addends;
+        }
+
+        Ok(Self { endianness, abi, isa_extension, ri_gp_value, paired_relocations })
     }
 
     fn instruction_flags(&self, diff_config: &DiffObjConfig) -> rabbitizer::InstructionFlags {
@@ -127,18 +172,16 @@ impl Arch for ArchMips {
         diff_config: &DiffObjConfig,
     ) -> Result<Vec<ScannedInstruction>> {
         let instruction_flags = self.instruction_flags(diff_config);
-        let start_address = address;
         let mut ops = Vec::<ScannedInstruction>::with_capacity(code.len() / 4);
-        let mut cur_addr = start_address as u32;
+        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 vram = Vram::new(cur_addr);
-            let instruction = rabbitizer::Instruction::new(code, vram, instruction_flags);
+            let instruction =
+                rabbitizer::Instruction::new(code, Vram::new(cur_addr), instruction_flags);
             let opcode = instruction.opcode() as u16;
-            let branch_dest =
-                instruction.get_branch_offset_generic().map(|o| (vram + o).inner() as u64);
+            let branch_dest = instruction.get_branch_vram_generic().map(|v| v.inner() as u64);
             ops.push(ScannedInstruction {
-                ins_ref: InstructionRef { address, size: 4, opcode },
+                ins_ref: InstructionRef { address: cur_addr as u64, size: 4, opcode },
                 branch_dest,
             });
             cur_addr += 4;
@@ -164,6 +207,7 @@ impl Arch for ArchMips {
             function_range,
             resolved.section_index,
             &display_flags,
+            diff_config,
             cb,
         )?;
         Ok(())
@@ -177,6 +221,17 @@ impl Arch for ArchMips {
         reloc: &object::Relocation,
         flags: RelocationFlags,
     ) -> Result<i64> {
+        // Check for paired R_MIPS_HI16 and R_MIPS_LO16 relocations.
+        if let RelocationFlags::Elf(elf::R_MIPS_HI16 | elf::R_MIPS_LO16) = flags {
+            if let Some(addend) = self
+                .paired_relocations
+                .get(section.index().0)
+                .and_then(|m| m.get(&address).copied())
+            {
+                return Ok(addend);
+            }
+        }
+
         let data = section.data()?;
         let code = data[address as usize..address as usize + 4].try_into()?;
         let addend = self.endianness.read_u32_bytes(code);
@@ -246,6 +301,7 @@ fn push_args(
     function_range: Range<u64>,
     section_index: usize,
     display_flags: &rabbitizer::InstructionDisplayFlags,
+    diff_config: &DiffObjConfig,
     mut arg_cb: impl FnMut(InstructionPart) -> Result<()>,
 ) -> Result<()> {
     let operands = instruction.valued_operands_iter();
@@ -305,9 +361,14 @@ fn push_args(
                     })))?;
                 }
                 arg_cb(InstructionPart::basic("("))?;
-                arg_cb(InstructionPart::opaque(
-                    base.either_name(instruction.flags().abi(), display_flags.named_gpr()),
-                ))?;
+                let mut value =
+                    base.either_name(instruction.flags().abi(), display_flags.named_gpr());
+                if !diff_config.mips_register_prefix {
+                    if let Some(trimmed) = value.strip_prefix('$') {
+                        value = trimmed;
+                    }
+                }
+                arg_cb(InstructionPart::opaque(value))?;
                 arg_cb(InstructionPart::basic(")"))?;
             }
             // ValuedOperand::r5900_immediate15(..) => match relocation {
@@ -321,9 +382,14 @@ fn push_args(
             //     }
             // },
             _ => {
-                arg_cb(InstructionPart::opaque(
-                    op.display(instruction, display_flags, None::<&str>).to_string(),
-                ))?;
+                let value = op.display(instruction, display_flags, None::<&str>).to_string();
+                if !diff_config.mips_register_prefix {
+                    if let Some(value) = value.strip_prefix('$') {
+                        arg_cb(InstructionPart::opaque(value))?;
+                        continue;
+                    }
+                }
+                arg_cb(InstructionPart::opaque(value))?;
             }
         }
     }

objdiff-core/src/obj/read.rs

@@ -263,7 +263,7 @@ const LOW_PRIORITY_SYMBOLS: &[&str] =
 fn best_symbol<'r, 'data, 'file>(
     symbols: &'r [object::Symbol<'data, 'file>],
     address: u64,
-) -> Option<object::SymbolIndex> {
+) -> Option<(object::SymbolIndex, u64)> {
     let mut closest_symbol_index = match symbols.binary_search_by_key(&address, |s| s.address()) {
         Ok(index) => Some(index),
         Err(index) => index.checked_sub(1),
@@ -297,18 +297,31 @@ fn best_symbol<'r, 'data, 'file>(
             best_symbol = Some(symbol);
         }
     }
-    best_symbol.map(|s| s.index())
+    best_symbol.map(|s| (s.index(), s.address()))
 }
 
-fn map_relocations(
+fn map_section_relocations(
     arch: &dyn Arch,
     obj_file: &object::File,
     obj_section: &object::Section,
     symbol_indices: &[usize],
+    ordered_symbols: &[Vec<object::Symbol>],
 ) -> Result<Vec<Relocation>> {
     let mut relocations = Vec::<Relocation>::with_capacity(obj_section.relocations().count());
-    let mut ordered_symbols = None;
     for (address, reloc) in obj_section.relocations() {
+        let flags = match reloc.flags() {
+            object::RelocationFlags::Elf { r_type } => RelocationFlags::Elf(r_type),
+            object::RelocationFlags::Coff { typ } => RelocationFlags::Coff(typ),
+            flags => {
+                bail!("Unhandled relocation flags: {:?}", flags);
+            }
+        };
+        // TODO validate reloc here?
+        let mut addend = if reloc.has_implicit_addend() {
+            arch.implcit_addend(obj_file, obj_section, address, &reloc, flags)?
+        } else {
+            reloc.addend()
+        };
         let target_symbol = match reloc.target() {
             object::RelocationTarget::Symbol(idx) => {
                 if idx.0 == u32::MAX as usize {
@@ -323,24 +336,16 @@ fn map_relocations(
                 {
                     let section_index =
                         section_symbol.section_index().context("Section symbol without section")?;
-                    let ordered_symbols = ordered_symbols.get_or_insert_with(|| {
-                        let mut vec = obj_file
-                            .symbols()
-                            .filter(|s| {
-                                s.section_index() == Some(section_index)
-                                    && s.kind() != object::SymbolKind::Section
-                            })
-                            .collect::<Vec<_>>();
-                        vec.sort_by(|a, b| {
-                            a.address().cmp(&b.address()).then(a.size().cmp(&b.size()))
-                        });
-                        vec
-                    });
-                    best_symbol(
-                        ordered_symbols,
-                        section_symbol.address().wrapping_add_signed(reloc.addend()),
-                    )
-                    .unwrap_or(idx)
+                    let target_address = section_symbol.address().wrapping_add_signed(addend);
+                    if let Some((new_idx, addr)) = ordered_symbols
+                        .get(section_index.0)
+                        .and_then(|symbols| best_symbol(symbols, target_address))
+                    {
+                        addend = target_address.wrapping_sub(addr) as i64;
+                        new_idx
+                    } else {
+                        idx
+                    }
                 } else {
                     idx
                 };
@@ -359,24 +364,55 @@ fn map_relocations(
             }
             _ => bail!("Unhandled relocation target: {:?}", reloc.target()),
         };
-        let flags = match reloc.flags() {
-            object::RelocationFlags::Elf { r_type } => RelocationFlags::Elf(r_type),
-            object::RelocationFlags::Coff { typ } => RelocationFlags::Coff(typ),
-            flags => {
-                bail!("Unhandled relocation flags: {:?}", flags);
-            }
-        };
-        // TODO validate reloc here?
-        let addend = if reloc.has_implicit_addend() {
-            arch.implcit_addend(obj_file, obj_section, address, &reloc, flags)?
-        } else {
-            reloc.addend()
-        };
         relocations.push(Relocation { address, flags, target_symbol, addend });
     }
+    relocations.sort_by_key(|r| r.address);
     Ok(relocations)
 }
 
+fn map_relocations(
+    arch: &dyn Arch,
+    obj_file: &object::File,
+    sections: &mut [Section],
+    section_indices: &[usize],
+    symbol_indices: &[usize],
+) -> Result<()> {
+    // Generate a list of symbols for each section
+    let mut ordered_symbols =
+        Vec::<Vec<object::Symbol>>::with_capacity(obj_file.sections().count() + 1);
+    for symbol in obj_file.symbols() {
+        let Some(section_index) = symbol.section_index() else {
+            continue;
+        };
+        if symbol.kind() == object::SymbolKind::Section {
+            continue;
+        }
+        if section_index.0 >= ordered_symbols.len() {
+            ordered_symbols.resize_with(section_index.0 + 1, Vec::new);
+        }
+        ordered_symbols[section_index.0].push(symbol);
+    }
+    // Sort symbols by address and size
+    for vec in &mut ordered_symbols {
+        vec.sort_by(|a, b| a.address().cmp(&b.address()).then(a.size().cmp(&b.size())));
+    }
+    // Map relocations for each section. Section-relative relocations use the ordered symbols list
+    // to find a better target symbol, if available.
+    for obj_section in obj_file.sections() {
+        let section = &mut sections[section_indices[obj_section.index().0]];
+        if section.kind != SectionKind::Unknown {
+            section.relocations = map_section_relocations(
+                arch,
+                obj_file,
+                &obj_section,
+                symbol_indices,
+                &ordered_symbols,
+            )?;
+        }
+    }
+    Ok(())
+}
+
 fn parse_line_info(
     obj_file: &object::File,
     sections: &mut [Section],
@@ -767,13 +803,7 @@ pub fn parse(data: &[u8], config: &DiffObjConfig) -> Result<Object> {
         map_sections(arch.as_ref(), &obj_file, split_meta.as_ref())?;
     let (mut symbols, symbol_indices) =
         map_symbols(arch.as_ref(), &obj_file, &sections, &section_indices, split_meta.as_ref())?;
-    for obj_section in obj_file.sections() {
-        let section = &mut sections[section_indices[obj_section.index().0]];
-        if section.kind != SectionKind::Unknown {
-            section.relocations =
-                map_relocations(arch.as_ref(), &obj_file, &obj_section, &symbol_indices)?;
-        }
-    }
+    map_relocations(arch.as_ref(), &obj_file, &mut sections, &section_indices, &symbol_indices)?;
     parse_line_info(&obj_file, &mut sections, &section_indices, data)?;
     if config.combine_data_sections || config.combine_text_sections {
         combine_sections(&mut sections, &mut symbols, config)?;

objdiff-core/tests/arch_mips.rs

@@ -0,0 +1,19 @@
+use objdiff_core::{diff, obj};
+
+mod common;
+
+#[test]
+#[cfg(feature = "mips")]
+fn read_mips() {
+    let diff_config = diff::DiffObjConfig {
+        mips_register_prefix: true,
+        ..Default::default()
+    };
+    let obj = obj::read::parse(include_object!("data/mips/main.c.o"), &diff_config).unwrap();
+    insta::assert_debug_snapshot!(obj);
+    let symbol_idx = obj.symbols.iter().position(|s| s.name == "ControlEntry").unwrap();
+    let diff = diff::code::no_diff_code(&obj, symbol_idx, &diff_config).unwrap();
+    insta::assert_debug_snapshot!(diff.instruction_rows);
+    let output = common::display_diff(&obj, &diff, symbol_idx, &diff_config);
+    insta::assert_snapshot!(output);
+}