Repository: gimli-rs/object
Pull request #676 (2024-05-01 04:41:26)

write/macho: ensure Mach-O subsections are not zero size
For Mach-O, `add_symbol_data` now ensures that the symbol size is at least 1 when subsections via symbols are enabled. This change was made to support linking with ld-prime. It is also unclear how this previously managed to work with ld64.

`write::Object::add_subsection` no longer enables subsections via symbols for Mach-O. Use `set_subsections_via_symbols` instead. This change was made because Mach-O subsections are all or nothing, so this decision must be made before any symbols are added.

`write::Object::add_subsection` no longer adds data to the subsection. This change was made because it was done with `append_section_data`, but this is often not the correct way to add data to the subsection. Usually `add_symbol_data` is a better choice.

philipc: This is to fix https://github.com/rust-lang/rustc_codegen_cranelift/issues/1456, along with the cranelift change in https://github.com/philipc/wasmtime/commit/d851125fab33b10ec51038bdbc323373b1ab07d6
philipc: Note that rustc_codegen_cranelift isn't currently setting `MH_SUBSECTIONS_VIA_SYMBOLS`, so there still needs to be a change somewhere to enable that (maybe cranelift-object can always set it). Also note that ld64 still works even with that flag set and without the fix in this PR, so I'm not completely sure that this bug is related to `MH_SUBSECTIONS_VIA_SYMBOLS`, although it does make sense to me that zero size symbols can't work with it set.
philipc: This code in llvm confirms that basing this on `MH_SUBSECTIONS_VIA_SYMBOLS` makes sense:
https://github.com/llvm/llvm-project/blob/176d6fbed3988032dbdabe7abfc5f54e1f0e2bbb/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp#L3812-L3816

llvm always sets `MH_SUBSECTIONS_VIA_SYMBOLS`, independent of -ffunction-sections:
https://github.com/llvm/llvm-project/blob/176d6fbed3988032dbdabe7abfc5f54e1f0e2bbb/llvm/lib/Target/X86/X86AsmPrinter.cpp#L990-L995

philipc: For zero length functions, llvm emits a noop instead:
https://github.com/llvm/llvm-project/blob/176d6fbed3988032dbdabe7abfc5f54e1f0e2bbb/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp#L1950-L1960

I'm not keen on handling this here. I think that's better done in cranelift if needed.
philipc: @bjorn3 Are you happy with this change?

Patch:
diff --git a/crates/examples/src/bin/simple_write.rs b/crates/examples/src/bin/simple_write.rs
index 76c1c18..3a35046 100644
--- a/crates/examples/src/bin/simple_write.rs
+++ b/crates/examples/src/bin/simple_write.rs
@@ -66,8 +66,8 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
     main_data.extend_from_slice(&[0xc3]);
 
     // Add the main function in its own subsection (equivalent to -ffunction-sections).
-    let (main_section, main_offset) =
-        obj.add_subsection(StandardSection::Text, b"main", &main_data, 1);
+    let main_section = obj.add_subsection(StandardSection::Text, b"main");
+    let main_offset = obj.append_section_data(main_section, &main_data, 1);
     // Add a globally visible symbol for the main function.
     obj.add_symbol(Symbol {
         name: b"main".into(),
diff --git a/src/write/macho.rs b/src/write/macho.rs
index d5260ef..33ff5d8 100644
--- a/src/write/macho.rs
+++ b/src/write/macho.rs
@@ -66,16 +66,6 @@ impl<'a> Object<'a> {
 
 // Private methods.
 impl<'a> Object<'a> {
-    pub(crate) fn macho_set_subsections_via_symbols(&mut self) {
-        let flags = match self.flags {
-            FileFlags::MachO { flags } => flags,
-            _ => 0,
-        };
-        self.flags = FileFlags::MachO {
-            flags: flags | macho::MH_SUBSECTIONS_VIA_SYMBOLS,
-        };
-    }
-
     pub(crate) fn macho_segment_name(&self, segment: StandardSegment) -> &'static [u8] {
         match segment {
             StandardSegment::Text => &b"__TEXT"[..],
@@ -570,10 +560,13 @@ impl<'a> Object<'a> {
             cpusubtype = cpu_subtype;
         }
 
-        let flags = match self.flags {
+        let mut flags = match self.flags {
             FileFlags::MachO { flags } => flags,
             _ => 0,
         };
+        if self.macho_subsections_via_symbols {
+            flags |= macho::MH_SUBSECTIONS_VIA_SYMBOLS;
+        }
         macho.write_mach_header(
             buffer,
             MachHeader {
diff --git a/src/write/mod.rs b/src/write/mod.rs
index 198ef5e..c16ed2e 100644
--- a/src/write/mod.rs
+++ b/src/write/mod.rs
@@ -90,6 +90,9 @@ pub struct Object<'a> {
     macho_cpu_subtype: Option<u32>,
     #[cfg(feature = "macho")]
     macho_build_version: Option<MachOBuildVersion>,
+    /// Mach-O MH_SUBSECTIONS_VIA_SYMBOLS flag. Only ever set if format is Mach-O.
+    #[cfg(feature = "macho")]
+    macho_subsections_via_symbols: bool,
 }
 
 impl<'a> Object<'a> {
@@ -115,6 +118,8 @@ impl<'a> Object<'a> {
             macho_cpu_subtype: None,
             #[cfg(feature = "macho")]
             macho_build_version: None,
+            #[cfg(feature = "macho")]
+            macho_subsections_via_symbols: false,
         }
     }
 
@@ -273,41 +278,34 @@ impl<'a> Object<'a> {
 
     /// Add a subsection. Returns the `SectionId` and section offset of the data.
     ///
-    /// Must not be called for sections that contain uninitialized data.
-    /// `align` must be a power of two.
-    pub fn add_subsection(
-        &mut self,
-        section: StandardSection,
-        name: &[u8],
-        data: &[u8],
-        align: u64,
-    ) -> (SectionId, u64) {
-        let section_id = if self.has_subsections_via_symbols() {
-            self.set_subsections_via_symbols();
+    /// For Mach-O, this does not create a subsection, and instead uses the
+    /// section from [`Self::section_id`]. Use [`Self::set_subsections_via_symbols`]
+    /// to enable subsections via symbols.
+    pub fn add_subsection(&mut self, section: StandardSection, name: &[u8]) -> SectionId {
+        if self.has_subsections_via_symbols() {
             self.section_id(section)
         } else {
             let (segment, name, kind, flags) = self.subsection_info(section, name);
             let id = self.add_section(segment.to_vec(), name, kind);
             self.section_mut(id).flags = flags;
             id
-        };
-        let offset = self.append_section_data(section_id, data, align);
-        (section_id, offset)
+        }
     }
 
     fn has_subsections_via_symbols(&self) -> bool {
-        match self.format {
-            BinaryFormat::Coff | BinaryFormat::Elf | BinaryFormat::Xcoff => false,
-            BinaryFormat::MachO => true,
-            _ => unimplemented!(),
-        }
+        self.format == BinaryFormat::MachO
     }
 
-    fn set_subsections_via_symbols(&mut self) {
-        match self.format {
-            #[cfg(feature = "macho")]
-            BinaryFormat::MachO => self.macho_set_subsections_via_symbols(),
-            _ => unimplemented!(),
+    /// Enable subsections via symbols if supported.
+    ///
+    /// This should be called before adding any subsections or symbols.
+    ///
+    /// For Mach-O, this sets the `MH_SUBSECTIONS_VIA_SYMBOLS` flag.
+    /// For other formats, this does nothing.
+    pub fn set_subsections_via_symbols(&mut self) {
+        #[cfg(feature = "macho")]
+        if self.format == BinaryFormat::MachO {
+            self.macho_subsections_via_symbols = true;
         }
     }
 
@@ -480,6 +478,9 @@ impl<'a> Object<'a> {
     /// For Mach-O, this also creates a `__thread_vars` entry for TLS symbols, and the
     /// symbol will indirectly point to the added data via the `__thread_vars` entry.
     ///
+    /// For Mach-O, if [`Self::set_subsections_via_symbols`] is enabled, this will
+    /// automatically ensure the data size is at least 1.
+    ///
     /// Returns the section offset of the data.
     ///
     /// Must not be called for sections that contain uninitialized data.
@@ -488,9 +489,13 @@ impl<'a> Object<'a> {
         &mut self,
         symbol_id: SymbolId,
         section: SectionId,
-        data: &[u8],
+        mut data: &[u8],
         align: u64,
     ) -> u64 {
+        #[cfg(feature = "macho")]
+        if data.is_empty() && self.macho_subsections_via_symbols {
+            data = &[0];
+        }
         let offset = self.append_section_data(section, data, align);
         self.set_symbol_data(symbol_id, section, offset, data.len() as u64);
         offset
@@ -501,6 +506,9 @@ impl<'a> Object<'a> {
     /// For Mach-O, this also creates a `__thread_vars` entry for TLS symbols, and the
     /// symbol will indirectly point to the added data via the `__thread_vars` entry.
     ///
+    /// For Mach-O, if [`Self::set_subsections_via_symbols`] is enabled, this will
+    /// automatically ensure the data size is at least 1.
+    ///
     /// Returns the section offset of the data.
     ///
     /// Must not be called for sections that contain initialized data.
@@ -509,9 +517,13 @@ impl<'a> Object<'a> {
         &mut self,
         symbol_id: SymbolId,
         section: SectionId,
-        size: u64,
+        mut size: u64,
         align: u64,
     ) -> u64 {
+        #[cfg(feature = "macho")]
+        if size == 0 && self.macho_subsections_via_symbols {
+            size = 1;
+        }
         let offset = self.append_section_bss(section, size, align);
         self.set_symbol_data(symbol_id, section, offset, size);
         offset


Test patch:
diff --git a/tests/round_trip/comdat.rs b/tests/round_trip/comdat.rs
index d700555..baeca82 100644
--- a/tests/round_trip/comdat.rs
+++ b/tests/round_trip/comdat.rs
@@ -13,11 +13,11 @@ fn coff_x86_64_comdat() {
     let mut object =
         write::Object::new(BinaryFormat::Coff, Architecture::X86_64, Endianness::Little);
 
-    let (section1, offset) =
-        object.add_subsection(write::StandardSection::Text, b"s1", &[0, 1, 2, 3], 4);
+    let section1 = object.add_subsection(write::StandardSection::Text, b"s1");
+    let offset = object.append_section_data(section1, &[0, 1, 2, 3], 4);
     object.section_symbol(section1);
-    let (section2, _) =
-        object.add_subsection(write::StandardSection::Data, b"s1", &[0, 1, 2, 3], 4);
+    let section2 = object.add_subsection(write::StandardSection::Data, b"s1");
+    object.append_section_data(section2, &[0, 1, 2, 3], 4);
     object.section_symbol(section2);
 
     let symbol = object.add_symbol(write::Symbol {
@@ -132,10 +132,10 @@ fn elf_x86_64_comdat() {
     let mut object =
         write::Object::new(BinaryFormat::Elf, Architecture::X86_64, Endianness::Little);
 
-    let (section1, offset) =
-        object.add_subsection(write::StandardSection::Text, b"s1", &[0, 1, 2, 3], 4);
-    let (section2, _) =
-        object.add_subsection(write::StandardSection::Data, b"s1", &[0, 1, 2, 3], 4);
+    let section1 = object.add_subsection(write::StandardSection::Text, b"s1");
+    let offset = object.append_section_data(section1, &[0, 1, 2, 3], 4);
+    let section2 = object.add_subsection(write::StandardSection::Data, b"s1");
+    object.append_section_data(section2, &[0, 1, 2, 3], 4);
 
     let symbol = object.add_symbol(write::Symbol {
         name: b"s1".to_vec(),
diff --git a/tests/round_trip/elf.rs b/tests/round_trip/elf.rs
index 60cac7c..157015b 100644
--- a/tests/round_trip/elf.rs
+++ b/tests/round_trip/elf.rs
@@ -13,8 +13,8 @@ fn symtab_shndx() {
 
     for i in 0..0x10000 {
         let name = format!("func{}", i).into_bytes();
-        let (section, offset) =
-            object.add_subsection(write::StandardSection::Text, &name, &[0xcc], 1);
+        let section = object.add_subsection(write::StandardSection::Text, &name);
+        let offset = object.append_section_data(section, &[0xcc], 1);
         object.add_symbol(write::Symbol {
             name,
             value: offset,