Sort MachO sections in -r mode

[pete]

Summary

Currently, final linked images are sorted according to things like text before data.

-r files however, are only sorted according to the atom sorter, which is correct within a section, but not really across sections.

After discussing this with Nick, he said there was no reason for the difference, and so this patch removes all the special logic which avoided sorting -r files, and just uses the same sort for everything.

[lhames]

Assuming those comments are just clarifying existing requirements, this looks good to me. Thanks!

lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp

409

Is this a new requirement with this patch, or are you just documenting an existing requirement?

482

And here?

[pete]

Thanks for the review. Committed as r256786.

lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp

409

Turns out it was an existing requirement. This is mostly here so that if we change something again in future we know where to look.

482

Likewise here.

lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp

 }
 
 void Util::organizeSections() {
-  if (_ctx.outputMachOType() == llvm::MachO::MH_OBJECT) {
-    // Leave sections ordered as normalized file specified.
-    uint32_t sectionIndex = 1;
-    for (SectionInfo *si : _sectionInfos) {
-      si->finalSectionIndex = sectionIndex++;
-    }
-  } else {
-    switch (_ctx.outputMachOType()) {
+  // NOTE!: Keep this in sync with assignAddressesToSections.
+  switch (_ctx.outputMachOType()) {
     case llvm::MachO::MH_EXECUTE:
       // Main executables, need a zero-page segment
       segmentForName("__PAGEZERO");

       break;
     default:
       break;
+  }
+  // Group sections into segments.
+  for (SectionInfo *si : _sectionInfos) {
+    SegmentInfo *seg = segmentForName(si->segmentName);
+    seg->sections.push_back(si);
+  }
+  // Sort segments.
+  std::sort(_segmentInfos.begin(), _segmentInfos.end(), SegmentSorter());
+
+  // Sort sections within segments.
+  for (SegmentInfo *seg : _segmentInfos) {
+    if (seg->name.equals("__TEXT")) {
+      std::sort(seg->sections.begin(), seg->sections.end(),
+                TextSectionSorter());
     }
-    // Group sections into segments.
-    for (SectionInfo *si : _sectionInfos) {
-      SegmentInfo *seg = segmentForName(si->segmentName);
-      seg->sections.push_back(si);
-    }
-    // Sort segments.
-    std::sort(_segmentInfos.begin(), _segmentInfos.end(), SegmentSorter());
-
-    // Sort sections within segments.
-    for (SegmentInfo *seg : _segmentInfos) {
-      if (seg->name.equals("__TEXT")) {
-        std::sort(seg->sections.begin(), seg->sections.end(),
-                                                          TextSectionSorter());
-      }
-    }
+  }
 
-    // Record final section indexes.
-    uint32_t segmentIndex = 0;
-    uint32_t sectionIndex = 1;
-    for (SegmentInfo *seg : _segmentInfos) {
-      seg->normalizedSegmentIndex = segmentIndex++;
-      for (SectionInfo *sect : seg->sections) {
-        sect->finalSectionIndex = sectionIndex++;
-      }
-    }
+    for (SectionInfo *sect : seg->sections)
   }
 }
 

 }
 
 void Util::assignAddressesToSections(const NormalizedFile &file) {
+  // NOTE!: Keep this in sync with organizeSections.
   size_t hlcSize = headerAndLoadCommandsSize(file);
   uint64_t address = 0;
-  if (_ctx.outputMachOType() != llvm::MachO::MH_OBJECT) {
-    for (SegmentInfo *seg : _segmentInfos) {
-      if (seg->name.equals("__PAGEZERO")) {
-        seg->size = _ctx.pageZeroSize();
-        address += seg->size;
-      }
-      else if (seg->name.equals("__TEXT")) {
-        // _ctx.baseAddress()  == 0 implies it was either unspecified or
-        // pageZeroSize is also 0. In either case resetting address is safe.
-        address = _ctx.baseAddress() ? _ctx.baseAddress() : address;
-        layoutSectionsInTextSegment(hlcSize, seg, address);
-      } else
-        layoutSectionsInSegment(seg, address);
-
-      address = llvm::RoundUpToAlignment(address, _ctx.pageSize());
+  for (SegmentInfo *seg : _segmentInfos) {
+    if (seg->name.equals("__PAGEZERO")) {
+      seg->size = _ctx.pageZeroSize();
+      address += seg->size;
     }
-    DEBUG_WITH_TYPE("WriterMachO-norm",
-      llvm::dbgs() << "assignAddressesToSections()\n";
-      for (SegmentInfo *sgi : _segmentInfos) {
-        llvm::dbgs()  << "   address=" << llvm::format("0x%08llX", sgi->address)
-                      << ", size="  << llvm::format("0x%08llX", sgi->size)
-                      << ", segment-name='" << sgi->name
-                      << "'\n";
-        for (SectionInfo *si : sgi->sections) {
-          llvm::dbgs()<< "      addr="  << llvm::format("0x%08llX", si->address)
-                      << ", size="  << llvm::format("0x%08llX", si->size)
-                      << ", section-name='" << si->sectionName
-                      << "\n";
-        }
+    else if (seg->name.equals("__TEXT")) {
+      // _ctx.baseAddress()  == 0 implies it was either unspecified or
+      // pageZeroSize is also 0. In either case resetting address is safe.
+      address = _ctx.baseAddress() ? _ctx.baseAddress() : address;
+      layoutSectionsInTextSegment(hlcSize, seg, address);
+    } else
+      layoutSectionsInSegment(seg, address);
+
+    address = llvm::RoundUpToAlignment(address, _ctx.pageSize());
+  }
+  DEBUG_WITH_TYPE("WriterMachO-norm",
+    llvm::dbgs() << "assignAddressesToSections()\n";
+    for (SegmentInfo *sgi : _segmentInfos) {
+      llvm::dbgs()  << "   address=" << llvm::format("0x%08llX", sgi->address)
+                    << ", size="  << llvm::format("0x%08llX", sgi->size)
+                    << ", segment-name='" << sgi->name
+                    << "'\n";
+      for (SectionInfo *si : sgi->sections) {
+        llvm::dbgs()<< "      addr="  << llvm::format("0x%08llX", si->address)
+                    << ", size="  << llvm::format("0x%08llX", si->size)
+                    << ", section-name='" << si->sectionName
+                    << "\n";
       }
-    );
-  } else {
-    for (SectionInfo *sect : _sectionInfos) {
-      sect->address = llvm::RoundUpToAlignment(address, sect->alignment);
-      address = sect->address + sect->size;
     }
-    DEBUG_WITH_TYPE("WriterMachO-norm",
-      llvm::dbgs() << "assignAddressesToSections()\n";
-      for (SectionInfo *si : _sectionInfos) {
-        llvm::dbgs()  << "      section=" << si->sectionName
-                      << " address= "  << llvm::format("0x%08X", si->address)
-                      << " size= "  << llvm::format("0x%08X", si->size)
-                      << "\n";
-      }
-    );
-  }
+  );
 }
 
 void Util::copySegmentInfo(NormalizedFile &file) {

 
 void Util::copySectionInfo(NormalizedFile &file) {
   file.sections.reserve(_sectionInfos.size());
-  // For final linked images, write sections grouped by segment.
-  if (_ctx.outputMachOType() != llvm::MachO::MH_OBJECT) {
-    for (SegmentInfo *sgi : _segmentInfos) {
-      for (SectionInfo *si : sgi->sections) {
-        appendSection(si, file);
-      }
-    }
-  } else {
-    // Object files write sections in default order.
-    for (SectionInfo *si : _sectionInfos) {
+  // Write sections grouped by segment.
+  for (SegmentInfo *sgi : _segmentInfos) {
+    for (SectionInfo *si : sgi->sections) {
       appendSection(si, file);
     }
   }

 
 void Util::updateSectionInfo(NormalizedFile &file) {
   file.sections.reserve(_sectionInfos.size());
-  if (_ctx.outputMachOType() != llvm::MachO::MH_OBJECT) {
-    // For final linked images, sections grouped by segment.
-    for (SegmentInfo *sgi : _segmentInfos) {
-      Segment *normSeg = &file.segments[sgi->normalizedSegmentIndex];
-      normSeg->address = sgi->address;
-      normSeg->size = sgi->size;
-      for (SectionInfo *si : sgi->sections) {
-        Section *normSect = &file.sections[si->normalizedSectionIndex];
-        normSect->address = si->address;
-      }
-    }
-  } else {
-    // Object files write sections in default order.
-    for (SectionInfo *si : _sectionInfos) {
+  // sections grouped by segment.
+  for (SegmentInfo *sgi : _segmentInfos) {
+    Segment *normSeg = &file.segments[sgi->normalizedSegmentIndex];
+    normSeg->address = sgi->address;
+    normSeg->size = sgi->size;
+    for (SectionInfo *si : sgi->sections) {
       Section *normSect = &file.sections[si->normalizedSectionIndex];
       normSect->address = si->address;
     }

 
 uint32_t Util::sectionIndexForAtom(const Atom *atom) {
   uint64_t address = _atomToAddress[atom];
-  uint32_t index = 1;
   for (const SectionInfo *si : _sectionInfos) {
     if ((si->address <= address) && (address < si->address+si->size))
-      return index;
-    ++index;
+      return si->finalSectionIndex;
   }
   llvm_unreachable("atom not in any section");
 }

test/mach-o/arm64-reloc-negDelta32-fixup.yaml

 # The reference from FDE->CIE is implicitly created as a negDelta32.
 # We don't emit these in to the binary as relocations, so we need to
 # make sure that the offset in the FDE to the CIE is the correct value.
-# CHECK: 0010 10000000 00000000 017a5200 01781e01
-# CHECK: 0020 100c1f00 20000000 18000000 e4ffffff
+# CHECK: {{[0-9abcdef]*}} 10000000 00000000 017a5200 01781e01
+# CHECK: {{[0-9abcdef]*}} 100c1f00 20000000 18000000 e4ffffff
 # Note, this one that matters   ^~~~~~~~
 # It needs to be 0x18 as that is the offset back to 0 where the CIE is.
-# CHECK: 0030 ffffffff 20000000 00000000 00480e10
-# CHECK: 0040 9e019d02 00000000
+# CHECK: {{[0-9abcdef]*}} ffffffff 20000000 00000000 00480e10
+# CHECK: {{[0-9abcdef]*}} 9e019d02 00000000
 
 ---  !mach-o
 arch:            arm64

test/mach-o/arm64-section-order.yaml

+# RUN: lld -flavor darwin -arch arm64 -r -print_atoms %s -o %t
+# RUN: lld -flavor darwin -arch arm64 -r -print_atoms %t -o %t2
+# RUN: llvm-objdump -section-headers %t | FileCheck %s
+# RUN: llvm-objdump -section-headers %t2 | FileCheck %s
+
+# Make sure that the sections are sorted.  Currently we want this order:
+# __text, __unwind_info
+
+# CHECK: Sections:
+# CHECK: 0 __text        {{.*}} TEXT
+# CHECK: 1 __compact_unwind {{.*}}
+
+
+--- !mach-o
+arch:            arm64
+file-type:       MH_OBJECT
+flags:           [ MH_SUBSECTIONS_VIA_SYMBOLS ]
+compat-version:  0.0
+current-version: 0.0
+has-UUID:        false
+OS:              unknown
+sections:
+  - segment:         __TEXT
+    section:         __text
+    type:            S_REGULAR
+    attributes:      [ S_ATTR_PURE_INSTRUCTIONS, S_ATTR_SOME_INSTRUCTIONS ]
+    alignment:       8
+    address:         0x0000000000000000
+    content:         [ 0xC0, 0x03, 0x5F, 0xD6, 0xC0, 0x03, 0x5F, 0xD6 ]
+  - segment:         __LD
+    section:         __compact_unwind
+    type:            S_REGULAR
+    attributes:      [  ]
+    alignment:       8
+    address:         0x0000000000000008
+    content:         [ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+                       0x04, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x03,
+                       0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+                       0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+                       0x04, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+                       0x04, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x03,
+                       0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+                       0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 ]
+    relocations:
+      - offset:          0x00000020
+        type:            ARM64_RELOC_UNSIGNED
+        length:          3
+        pc-rel:          false
+        extern:          false
+        symbol:          1
+      - offset:          0x00000000
+        type:            ARM64_RELOC_UNSIGNED
+        length:          3
+        pc-rel:          false
+        extern:          false
+        symbol:          1
+global-symbols:
+  - name:            __Z3fooi
+    type:            N_SECT
+    scope:           [ N_EXT ]
+    sect:            1
+    value:           0x0000000000000000
+  - name:            __Z4foo2i
+    type:            N_SECT
+    scope:           [ N_EXT ]
+    sect:            1
+    value:           0x0000000000000004

test/mach-o/parse-data-relocs-x86_64.yaml

 # CHECK:    type:            data
 # CHECK:    content:         [ 00, 00, 00, 00, 00, 00, 00, 00, 04, 00, 00, 00, 
 # CHECK:                       00, 00, 00, 00, F0, FF, FF, FF, FF, FF, FF, FF, 
-# CHECK:                       {{..}}, 00, 00, 00, 00, 00, 00, 00, {{..}}, 00, 00, 00, 
+# CHECK:                       {{..}}, {{..}}, 00, 00, 00, 00, 00, 00, {{..}}, {{..}}, 00, 00,
 # CHECK:                       00, 00, 00, 00, D8, FF, FF, FF, FF, FF, FF, FF, 
 # CHECK:                       D4, FF, FF, FF, FF, FF, FF, FF, {{..}}, {{..}}, {{..}}, {{..}},
 # CHECK:                       {{..}}, {{..}}, {{..}}, {{..}}, C0, FF, FF, FF, C0, FF, FF, FF,