Repository: gimli-rs/object
Pull request #702 (2024-06-27 13:54:01)

Reverse the order of emitting relocations on MachO
This prevents a crash of Apple's new linker.

philipc: This probably should be treated as a breaking change. What do you think about trying to avoid that by detecting if it needs to be reversed? Too magical?
```rust
                let need_reverse = |relocs: &[Relocation]| {
                    let Some(first) = relocs.first() else { return false; };
                    let Some(last) = relocs.last() else { return false; };
                    first.offset < last.offset
                }; 
                if need_reverse(&section.relocations) {
                    for reloc in section.relocations.iter().rev() {
                        write_reloc(reloc)?;
                    }
                } else {
                    for reloc in &section.relocations {
                        write_reloc(reloc)?;
                    }
                }
```

Also, out of interest, I checked that just swapping the `ARM64_RELOC_GOT_LOAD_PAGE21`/ `ARM64_RELOC_GOT_LOAD_PAGEOFF12` pairs is enough for the cg_clif tests to pass.
bjorn3: > Too magical?

Maybe? I applied your suggestion anyway.

Patch:
diff --git a/src/write/macho.rs b/src/write/macho.rs
index abce7cb..3594769 100644
--- a/src/write/macho.rs
+++ b/src/write/macho.rs
@@ -729,7 +729,8 @@ impl<'a> Object<'a> {
             if !section.relocations.is_empty() {
                 write_align(buffer, pointer_align);
                 debug_assert_eq!(section_offsets[index].reloc_offset, buffer.len());
-                for reloc in &section.relocations {
+
+                let mut write_reloc = |reloc: &Relocation| {
                     let (r_type, r_pcrel, r_length) = if let RelocationFlags::MachO {
                         r_type,
                         r_pcrel,
@@ -783,6 +784,29 @@ impl<'a> Object<'a> {
                         r_type,
                     };
                     buffer.write(&reloc_info.relocation(endian));
+                    Ok(())
+                };
+
+                // Relocations are emitted in descending order as otherwise Apple's
+                // new linker crashes. This matches LLVM's behavior too:
+                // https://github.com/llvm/llvm-project/blob/e9b8cd0c8/llvm/lib/MC/MachObjectWriter.cpp#L1001-L1002
+                let need_reverse = |relocs: &[Relocation]| {
+                    let Some(first) = relocs.first() else {
+                        return false;
+                    };
+                    let Some(last) = relocs.last() else {
+                        return false;
+                    };
+                    first.offset < last.offset
+                };
+                if need_reverse(&section.relocations) {
+                    for reloc in section.relocations.iter().rev() {
+                        write_reloc(reloc)?;
+                    }
+                } else {
+                    for reloc in &section.relocations {
+                        write_reloc(reloc)?;
+                    }
                 }
             }
         }


Test patch:
diff --git a/tests/round_trip/mod.rs b/tests/round_trip/mod.rs
index 3a592eb..be5c96d 100644
--- a/tests/round_trip/mod.rs
+++ b/tests/round_trip/mod.rs
@@ -454,27 +454,27 @@ fn macho_x86_64() {
 
     let (offset, relocation) = relocations.next().unwrap();
     println!("{:?}", relocation);
-    assert_eq!(offset, 8);
-    assert_eq!(relocation.kind(), RelocationKind::Absolute);
-    assert_eq!(relocation.encoding(), RelocationEncoding::Generic);
-    assert_eq!(relocation.size(), 64);
+    assert_eq!(offset, 16);
+    assert_eq!(relocation.kind(), RelocationKind::Relative);
+    assert_eq!(relocation.encoding(), RelocationEncoding::X86RipRelative);
+    assert_eq!(relocation.size(), 32);
     assert_eq!(
         relocation.target(),
         read::RelocationTarget::Symbol(func1_symbol)
     );
-    assert_eq!(relocation.addend(), 0);
+    assert_eq!(relocation.addend(), -4);
 
     let (offset, relocation) = relocations.next().unwrap();
     println!("{:?}", relocation);
-    assert_eq!(offset, 16);
-    assert_eq!(relocation.kind(), RelocationKind::Relative);
-    assert_eq!(relocation.encoding(), RelocationEncoding::X86RipRelative);
-    assert_eq!(relocation.size(), 32);
+    assert_eq!(offset, 8);
+    assert_eq!(relocation.kind(), RelocationKind::Absolute);
+    assert_eq!(relocation.encoding(), RelocationEncoding::Generic);
+    assert_eq!(relocation.size(), 64);
     assert_eq!(
         relocation.target(),
         read::RelocationTarget::Symbol(func1_symbol)
     );
-    assert_eq!(relocation.addend(), -4);
+    assert_eq!(relocation.addend(), 0);
 
     let map = object.symbol_map();
     let symbol = map.get(func1_offset + 1).unwrap();
@@ -564,14 +564,6 @@ fn macho_any() {
 
         let mut relocations = data.relocations();
 
-        let (offset, relocation) = relocations.next().unwrap();
-        println!("{:?}", relocation);
-        assert_eq!(offset, 8);
-        assert_eq!(relocation.kind(), RelocationKind::Absolute);
-        assert_eq!(relocation.encoding(), RelocationEncoding::Generic);
-        assert_eq!(relocation.size(), 32);
-        assert_eq!(relocation.addend(), 0);
-
         if arch.address_size().unwrap().bytes() >= 8 {
             let (offset, relocation) = relocations.next().unwrap();
             println!("{:?}", relocation);
@@ -581,6 +573,14 @@ fn macho_any() {
             assert_eq!(relocation.size(), 64);
             assert_eq!(relocation.addend(), 0);
         }
+
+        let (offset, relocation) = relocations.next().unwrap();
+        println!("{:?}", relocation);
+        assert_eq!(offset, 8);
+        assert_eq!(relocation.kind(), RelocationKind::Absolute);
+        assert_eq!(relocation.encoding(), RelocationEncoding::Generic);
+        assert_eq!(relocation.size(), 32);
+        assert_eq!(relocation.addend(), 0);
     }
 }
 
diff --git a/tests/round_trip/tls.rs b/tests/round_trip/tls.rs
index c8ee334..fa41096 100644
--- a/tests/round_trip/tls.rs
+++ b/tests/round_trip/tls.rs
@@ -260,49 +260,49 @@ fn macho_x86_64_tls() {
 
     let (offset, relocation) = relocations.next().unwrap();
     println!("{:?}", relocation);
-    assert_eq!(offset, 0);
+    assert_eq!(offset, 40);
     assert_eq!(relocation.kind(), RelocationKind::Absolute);
     assert_eq!(relocation.encoding(), RelocationEncoding::Generic);
     assert_eq!(relocation.size(), 64);
     assert_eq!(
         relocation.target(),
-        read::RelocationTarget::Symbol(tlv_bootstrap_symbol)
+        read::RelocationTarget::Symbol(tls2_init_symbol)
     );
     assert_eq!(relocation.addend(), 0);
 
     let (offset, relocation) = relocations.next().unwrap();
     println!("{:?}", relocation);
-    assert_eq!(offset, 16);
+    assert_eq!(offset, 24);
     assert_eq!(relocation.kind(), RelocationKind::Absolute);
     assert_eq!(relocation.encoding(), RelocationEncoding::Generic);
     assert_eq!(relocation.size(), 64);
     assert_eq!(
         relocation.target(),
-        read::RelocationTarget::Symbol(tls1_init_symbol)
+        read::RelocationTarget::Symbol(tlv_bootstrap_symbol)
     );
     assert_eq!(relocation.addend(), 0);
 
     let (offset, relocation) = relocations.next().unwrap();
     println!("{:?}", relocation);
-    assert_eq!(offset, 24);
+    assert_eq!(offset, 16);
     assert_eq!(relocation.kind(), RelocationKind::Absolute);
     assert_eq!(relocation.encoding(), RelocationEncoding::Generic);
     assert_eq!(relocation.size(), 64);
     assert_eq!(
         relocation.target(),
-        read::RelocationTarget::Symbol(tlv_bootstrap_symbol)
+        read::RelocationTarget::Symbol(tls1_init_symbol)
     );
     assert_eq!(relocation.addend(), 0);
 
     let (offset, relocation) = relocations.next().unwrap();
     println!("{:?}", relocation);
-    assert_eq!(offset, 40);
+    assert_eq!(offset, 0);
     assert_eq!(relocation.kind(), RelocationKind::Absolute);
     assert_eq!(relocation.encoding(), RelocationEncoding::Generic);
     assert_eq!(relocation.size(), 64);
     assert_eq!(
         relocation.target(),
-        read::RelocationTarget::Symbol(tls2_init_symbol)
+        read::RelocationTarget::Symbol(tlv_bootstrap_symbol)
     );
     assert_eq!(relocation.addend(), 0);
 }