Fix ARM trailing data directive size

Author: floze-the-genius

objdiff-core/src/arch/arm.rs

@@ -162,6 +162,10 @@ impl ArchArm {
 }
 
 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 +478,24 @@ 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]
+            && !section.relocation_at(next_address.saturating_sub(4), 4).is_some_and(|relocation| {
+                relocation.address + self.data_reloc_size(relocation.flags) as u64
+                    > next_address - 2
+            })
         {
             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 +508,11 @@ 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);
+        self.mapping == unarm::ParseMode::Data && data_size >= 4 && data_size.is_multiple_of(4)
+    }
+
     fn from_object_symbol<'a>(sym: &object::Symbol<'a, '_, &'a [u8]>) -> Option<Self> {
         sym.name()
             .ok()
@@ -645,3 +658,18 @@ impl unarm::FormatIns for ArgsFormatter<'_> {
         Ok(())
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::DisasmMode;
+
+    #[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));
+    }
+}

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