diff --git a/objdiff-core/src/arch/arm.rs b/objdiff-core/src/arch/arm.rs index 4838ed28..67867f93 100644 --- a/objdiff-core/src/arch/arm.rs +++ b/objdiff-core/src/arch/arm.rs @@ -159,9 +159,26 @@ impl ArchArm { }; Ok(ins) } + + fn has_relocation_overlapping_trailing_halfword( + &self, + section: &Section, + end_address: u64, + ) -> bool { + let word_start = end_address.saturating_sub(4); + let halfword_start = end_address.saturating_sub(2); + section.relocations_at(word_start, 4).any(|relocation| { + relocation.address.saturating_add(self.data_reloc_size(relocation.flags) as u64) + > halfword_start + }) + } } impl Arch for ArchArm { + fn pre_init(&mut self, sections: &[Section], symbols: &[Symbol], _symbol_indices: &[usize]) { + self.disasm_modes = Self::get_mapping_symbols(sections, symbols); + } + fn post_init(&mut self, sections: &[Section], symbols: &[Symbol], _symbol_indices: &[usize]) { self.disasm_modes = Self::get_mapping_symbols(sections, symbols); } @@ -474,20 +491,21 @@ impl Arch for ArchArm { section: &Section, mut next_address: u64, ) -> Result { - // TODO: This should probably check the disasm mode and trim accordingly, - // but self.disasm_modes isn't populated until post_init, so it needs a refactor. - // Trim any trailing 2-byte zeroes from the end (padding) while next_address >= symbol.address + 2 + && !symbol.section.is_some_and(|section_idx| { + self.disasm_modes + .get(§ion_idx) + .and_then(|mappings| { + mappings.iter().rfind(|mapping| mapping.address as u64 <= next_address - 2) + }) + .is_some_and(|mapping| mapping.ends_with_complete_data_words(next_address)) + }) && let Some(data) = section.data_range(next_address - 2, 2) && data == [0u8; 2] + && !self.has_relocation_overlapping_trailing_halfword(section, next_address) { next_address -= 2; - if let Some(relocation) = section.relocation_at(next_address, 2) { - // Avoid cutting trailing relocations in half. - next_address += self.data_reloc_size(relocation.flags) as u64; - break; - } } Ok(next_address.saturating_sub(symbol.address)) } @@ -500,6 +518,16 @@ struct DisasmMode { } impl DisasmMode { + fn ends_with_complete_data_words(self, end_address: u64) -> bool { + let data_size = end_address.saturating_sub(self.address as u64); + let bytes_until_word_alignment = (4 - (self.address as u64 % 4)) % 4; + let aligned_data_size = data_size.saturating_sub(bytes_until_word_alignment); + self.mapping == unarm::ParseMode::Data + && data_size >= bytes_until_word_alignment + && aligned_data_size >= 4 + && aligned_data_size.is_multiple_of(4) + } + fn from_object_symbol<'a>(sym: &object::Symbol<'a, '_, &'a [u8]>) -> Option { sym.name() .ok() @@ -645,3 +673,59 @@ impl unarm::FormatIns for ArgsFormatter<'_> { Ok(()) } } + +#[cfg(test)] +mod tests { + use object::elf; + + use super::{ArchArm, DisasmMode}; + use crate::obj::{Relocation, RelocationFlags, Section}; + + #[test] + fn complete_data_words_exclude_trailing_halfword_padding() { + let mapping = DisasmMode { address: 0x1000, mapping: unarm::ParseMode::Data }; + + assert!(!mapping.ends_with_complete_data_words(0x1002)); + assert!(mapping.ends_with_complete_data_words(0x1004)); + assert!(!mapping.ends_with_complete_data_words(0x1006)); + assert!(mapping.ends_with_complete_data_words(0x1008)); + + let unaligned_mapping = DisasmMode { address: 0x1002, mapping: unarm::ParseMode::Data }; + assert!(!unaligned_mapping.ends_with_complete_data_words(0x1004)); + assert!(!unaligned_mapping.ends_with_complete_data_words(0x1006)); + assert!(unaligned_mapping.ends_with_complete_data_words(0x1008)); + assert!(!unaligned_mapping.ends_with_complete_data_words(0x100a)); + assert!(unaligned_mapping.ends_with_complete_data_words(0x100c)); + } + + #[test] + fn checks_every_relocation_before_trimming_trailing_halfword() { + let arch = ArchArm { + disasm_modes: Default::default(), + detected_version: None, + endianness: object::Endianness::Little, + }; + let mut section = Section { + relocations: vec![ + Relocation { + flags: RelocationFlags::Elf(elf::R_ARM_ABS16), + address: 0x1000, + target_symbol: 0, + addend: 0, + }, + Relocation { + flags: RelocationFlags::Elf(elf::R_ARM_ABS16), + address: 0x1002, + target_symbol: 1, + addend: 0, + }, + ], + ..Default::default() + }; + + assert!(arch.has_relocation_overlapping_trailing_halfword(§ion, 0x1004)); + + section.relocations.pop(); + assert!(!arch.has_relocation_overlapping_trailing_halfword(§ion, 0x1004)); + } +} diff --git a/objdiff-core/src/arch/mod.rs b/objdiff-core/src/arch/mod.rs index eb0029e6..8fb30c4e 100644 --- a/objdiff-core/src/arch/mod.rs +++ b/objdiff-core/src/arch/mod.rs @@ -390,6 +390,9 @@ impl dyn Arch { } pub trait Arch: Any + Debug + Send + Sync { + /// Performs arch-specific initialization needed before inferring zero-sized symbols. + fn pre_init(&mut self, _sections: &[Section], _symbols: &[Symbol], _symbol_indices: &[usize]) {} + /// Finishes arch-specific initialization that must be done after sections have been combined. fn post_init(&mut self, _sections: &[Section], _symbols: &[Symbol], _symbol_indices: &[usize]) { } diff --git a/objdiff-core/src/obj/mod.rs b/objdiff-core/src/obj/mod.rs index 479186e1..d0c3eca6 100644 --- a/objdiff-core/src/obj/mod.rs +++ b/objdiff-core/src/obj/mod.rs @@ -113,21 +113,16 @@ impl Section { self.align.map_or(MIN_ALIGNMENT, |align| align.get().max(MIN_ALIGNMENT)) } + pub fn relocations_at(&self, address: u64, size: u8) -> impl Iterator { + let start = self.relocations.partition_point(|relocation| relocation.address < address); + let end = address.saturating_add(size as u64); + self.relocations[start..] + .iter() + .take_while(move |relocation| relocation.address == address || relocation.address < end) + } + pub fn relocation_at(&self, address: u64, size: u8) -> Option<&Relocation> { - match self.relocations.binary_search_by_key(&address, |r| r.address) { - Ok(mut i) => { - // Find the first relocation at the address - while i - .checked_sub(1) - .and_then(|n| self.relocations.get(n)) - .is_some_and(|r| r.address == address) - { - i -= 1; - } - self.relocations.get(i) - } - Err(i) => self.relocations.get(i).filter(|r| r.address < address + size as u64), - } + self.relocations_at(address, size).next() } pub fn resolve_relocation_at<'obj>( diff --git a/objdiff-core/src/obj/read.rs b/objdiff-core/src/obj/read.rs index 6000159d..eb624615 100644 --- a/objdiff-core/src/obj/read.rs +++ b/objdiff-core/src/obj/read.rs @@ -1076,6 +1076,7 @@ pub fn parse(data: &[u8], config: &DiffObjConfig, diff_side: DiffSide) -> Result let (mut symbols, symbol_indices) = map_symbols(arch.as_ref(), &obj_file, §ion_indices, split_meta.as_ref(), config)?; map_relocations(arch.as_ref(), &obj_file, &mut sections, §ion_indices, &symbol_indices)?; + arch.pre_init(§ions, &symbols, &symbol_indices); // Infer symbol sizes for 0-size symbols (must be done after map_relocations is called) infer_symbol_sizes(arch.as_ref(), &mut symbols, §ions)?; parse_line_info(&obj_file, &mut sections, §ion_indices, data)?; diff --git a/objdiff-core/tests/arch_arm.rs b/objdiff-core/tests/arch_arm.rs index 96bc7e17..ed114369 100644 --- a/objdiff-core/tests/arch_arm.rs +++ b/objdiff-core/tests/arch_arm.rs @@ -115,6 +115,28 @@ fn trim_trailing_hword() { insta::assert_snapshot!(output); } +#[test] +#[cfg(feature = "arm")] +fn preserve_trailing_data_directive() { + let diff_config = diff::DiffObjConfig::default(); + let obj = obj::read::parse( + include_object!("data/arm/issue_382.o"), + &diff_config, + diff::DiffSide::Base, + ) + .unwrap(); + let symbol_idx = obj.symbols.iter().position(|s| s.name == "sub_08014184").unwrap(); + let symbol = &obj.symbols[symbol_idx]; + assert_eq!(symbol.size, 0xac); + + let diff = diff::code::no_diff_code(&obj, symbol_idx, &diff_config).unwrap(); + let output = common::display_diff(&obj, &diff, symbol_idx, &diff_config); + assert!( + output.contains(r#"Opcode(".word", 65534)"#), + "the final $d mapping symbol must preserve the 4-byte data directive:\n{output}" + ); +} + #[test] #[cfg(feature = "arm")] fn do_not_trim_trailing_relocations() { diff --git a/objdiff-core/tests/data/arm/issue_382.o b/objdiff-core/tests/data/arm/issue_382.o new file mode 100644 index 00000000..37244a74 Binary files /dev/null and b/objdiff-core/tests/data/arm/issue_382.o differ