Fix ARM trailing data directive size

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<u64> {
-        // 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(&section_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<Self> {
         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(&section, 0x1004));
+
+        section.relocations.pop();
+        assert!(!arch.has_relocation_overlapping_trailing_halfword(&section, 0x1004));
+    }
+}

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]) {
     }

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<Item = &Relocation> {
+        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>(

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, &section_indices, split_meta.as_ref(), config)?;
     map_relocations(arch.as_ref(), &obj_file, &mut sections, &section_indices, &symbol_indices)?;
+    arch.pre_init(&sections, &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, &sections)?;
     parse_line_info(&obj_file, &mut sections, &section_indices, data)?;

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