[lld-macho] Follow-up to D77893

[int3]

Summary

1. Don't have isHidden() depend on isNeeded(). Whether a section is hidden is orthogonal from whether it is needed: hidden sections will never have a header regardless of whether they have a body. (I know we override this method with return false for synthetic sections, but regardless I think it's confusing to write it this way for non-synthetic sections.)

2. Don't call writeTo() on unneeded sections. D78270 assumes that this is true when implementing the stub helper section.

3. Filter out the unneeded sections early on to avoid having to deal with them in multiple places.

4. Remove assumption in test that the referenced file has no other symbols. (We should create separate input files for future tests to avoid such issues.)

[smeenai]

LGTM, but I'd like @Ktwu to take a look as well.

[Ktwu]

lld/MachO/OutputSegment.cpp

43

I personally find this more complicated to read :(

Knowing that hidden state isn't as simple as checking isHidden() -- isNeeded() is also needed -- seemed confusing to me :/

lld/MachO/Writer.cpp

117

My bad, was this an actual bug? Why not hide the empty() check within a function?

128

Thanks!

[int3]

lld/MachO/OutputSegment.cpp

43

I agree that sprinkling isNeeded() checks around isn't pretty, but the flip side is that there's no longer an implicit requirement that every implementation of OutputSection must maintain the invariant that isNeeded() == false => isHidden() == false.

But see also https://reviews.llvm.org/D77893#2022314 -- I think we could further clean things up by filtering out all the unneeded sections earlier.

lld/MachO/Writer.cpp

117

It wasn't an actual bug, but the check here was to avoid calling firstSection() below on an empty sections vector. I think using isNeeded() which also checks if any contained sections are needed obscures this (plus it confused me for a bit as to why we had to check isNeeded() here when we were already checking it before calling writeTo()).

I'll add a comment here to make its purpose clear

[int3]

lld/MachO/OutputSegment.cpp

43

Actually, I can do the filtering in this diff, inside sortOutputSegmentsAndSections, instead of depending on the larger proposed refactor. How does that sound?

[MaskRay]

Can unneeded sections be removed?

[Ktwu]

lld/MachO/OutputSegment.cpp

43

Yup, do it!

[int3]

remove unneeded sections early

[int3]

update

[int3]

lld/MachO/Writer.cpp

117

Actually, I think it was an actual bug: the whole purpose of this check is for __LINKEDIT, which is the only segment that may have writeTo() called on it even if it contained no sections. But isNeeded() will return true for __LINKEDIT.

[int3]

asan

lld/MachO/OutputSection.h

public:
   uint64_t getSegmentOffset() const;
 
   // How much space the section occupies in the address space.
   virtual size_t getSize() const = 0;
   // How much space the section occupies in the file. Most sections are copied
   // as-is so their file size is the same as their address space size.
   virtual uint64_t getFileSize() const { return getSize(); }
 
-  // Hidden sections omit header content, but body content is still present.
-  virtual bool isHidden() const { return !this->isNeeded(); }
+  // Hidden sections omit header content, but body content may still be present.
+  virtual bool isHidden() const { return false; }
   // Unneeded sections are omitted entirely (header and body).
   virtual bool isNeeded() const { return true; }
 
   // Some sections may allow coalescing other raw input sections.
   virtual void mergeInput(InputSection *input);
 
   // Specifically finalizes addresses and section size, not content.
   virtual void finalize() {

lld/MachO/OutputSegment.h

for (const SectionMapEntry &i : sections) {
         return true;
     }
     return false;
   }
 
   OutputSection *getOrCreateOutputSection(StringRef name);
   void addOutputSection(OutputSection *os);
   void sortOutputSections(OutputSegmentComparator *comparator);
+  void removeUnneededSections();
 
   const SectionMap &getSections() const { return sections; }
   size_t numNonHiddenSections() const;
 
   uint64_t fileOff = 0;
   StringRef name;
   uint32_t maxProt = 0;
   uint32_t initProt = 0;

lld/MachO/OutputSegment.cpp

if (name == segment_names::pageZero)
     return 0;
   return VM_PROT_READ | VM_PROT_WRITE | VM_PROT_EXECUTE;
 }
 
 size_t OutputSegment::numNonHiddenSections() const {
   size_t count = 0;
   for (const OutputSegment::SectionMapEntry &i : sections) {
     OutputSection *os = i.second;
-    count += (os->isHidden() ? 0 : 1);
+    count += (!os->isHidden() ? 1 : 0);
   }
   return count;
 }
 
 void OutputSegment::addOutputSection(OutputSection *os) {
   os->parent = this;
   std::pair<SectionMap::iterator, bool> result =
       sections.insert(SectionMapEntry(os->name, os));
OutputSection *OutputSegment::getOrCreateOutputSection(StringRef name) {
   addOutputSection(os);
   return os;
 }
 
 void OutputSegment::sortOutputSections(OutputSegmentComparator *comparator) {
   llvm::stable_sort(sections, *comparator->sectionComparator(this));
 }
 
+void OutputSegment::removeUnneededSections() {
+  sections.remove_if([](const std::pair<StringRef, OutputSection *> &p) {
+    return !p.second->isNeeded();
+  });
+}
+
 OutputSegmentComparator::OutputSegmentComparator() {
   // This defines the order of segments and the sections within each segment.
   // Segments that are not mentioned here will end up at defaultPosition;
   // sections that are not mentioned will end up at the end of the section
   // list for their given segment.
   std::vector<std::pair<StringRef, std::vector<StringRef>>> ordering{
       {segment_names::pageZero, {}},
       {segment_names::text, {section_names::header}},
void macho::sortOutputSegmentsAndSections() {
 
   // Now that the output sections are sorted, assign the final
   // output section indices.
   uint32_t sectionIndex = 0;
   for (OutputSegment *seg : outputSegments) {
     seg->sortOutputSections(&comparator);
     for (auto &p : seg->getSections()) {
       OutputSection *section = p.second;
-      if (!section->isHidden()) {
+      if (!section->isHidden())
         section->index = ++sectionIndex;
-      }
-    }
-  }
-}

lld/MachO/Writer.cpp

void writeTo(uint8_t *buf) const override {
 
     c->cmd = LC_SEGMENT_64;
     c->cmdsize = getSize();
     memcpy(c->segname, name.data(), name.size());
     c->fileoff = seg->fileOff;
     c->maxprot = seg->maxProt;
     c->initprot = seg->initProt;
 
-    if (!seg->isNeeded())
+    if (seg->getSections().empty())
       return;
 
     c->vmaddr = seg->firstSection()->addr;
     c->vmsize =
         seg->lastSection()->addr + seg->lastSection()->getSize() - c->vmaddr;
     c->nsects = seg->numNonHiddenSections();
 
     for (auto &p : seg->getSections()) {
       StringRef s = p.first;
       OutputSection *section = p.second;
       c->filesize += section->getFileSize();
+
       if (section->isHidden())
         continue;
 
       auto *sectHdr = reinterpret_cast<section_64 *>(buf);
       buf += sizeof(section_64);
 
       memcpy(sectHdr->sectname, s.data(), s.size());
       memcpy(sectHdr->segname, name.data(), name.size());
case MH_DYLIB:
     headerSection->addLoadCommand(make<LCIdDylib>(config->installName));
     break;
   default:
     llvm_unreachable("unhandled output file type");
   }
 
   uint8_t segIndex = 0;
   for (OutputSegment *seg : outputSegments) {
-    if (seg->isNeeded()) {
-      headerSection->addLoadCommand(make<LCSegment>(seg->name, seg));
-      seg->index = segIndex++;
-    }
-  }
 
   uint64_t dylibOrdinal = 1;
   for (InputFile *file : inputFiles) {
     if (auto *dylibFile = dyn_cast<DylibFile>(file)) {
       headerSection->addLoadCommand(make<LCLoadDylib>(dylibFile->dylibName));
       dylibFile->ordinal = dylibOrdinal++;
     }
   }
void Writer::createOutputSections() {
   }
 
   // Then merge input sections into output sections/segments.
   for (InputSection *isec : inputSections) {
     getOrCreateOutputSegment(isec->segname)
         ->getOrCreateOutputSection(isec->name)
         ->mergeInput(isec);
   }
+
+  // Remove unneeded segments and sections.
+  // TODO: Avoid creating unneeded segments in the first place
+  for (auto it = outputSegments.begin(); it != outputSegments.end();) {
+    OutputSegment *seg = *it;
+    seg->removeUnneededSections();
+    if (!seg->isNeeded())
+      it = outputSegments.erase(it);
+    else
+      ++it;
+  }
 }
 
 void Writer::assignAddresses(OutputSegment *seg) {
   addr = alignTo(addr, PageSize);
   fileOff = alignTo(fileOff, PageSize);
   seg->fileOff = fileOff;
 
   for (auto &p : seg->getSections()) {

lld/test/MachO/section-merge.s

 # CHECK-DAG:  {{[0-9a-z]+}} g     F __TEXT,__text _main
 # CHECK-DAG:  {{[0-9a-z]+}} g     F __TEXT,__text _some_function
 
 # RUN: llvm-objdump -d %t/output | FileCheck %s --check-prefix DATA
 # DATA:      Disassembly of section __TEXT,__text:
 # DATA:        {{0*}}[[#%x,BASE:]] <_some_function>:
 # DATA-NEXT:             [[#BASE]]: 48 c7 c0 01 00 00 00          movq    $1, %rax
 # DATA-NEXT:       [[#BASE + 0x7]]: c3                            retq
-# DATA:      {{0*}}[[#BASE + 0x8]] <_main>:
-# DATA-NEXT:       [[#BASE + 0x8]]: 48 c7 c0 00 00 00 00          movq    $0, %rax
-# DATA-NEXT:       [[#BASE + 0xf]]: c3                            retq
+# DATA:        {{0*}}[[#%x,MAIN:]] <_main>:
+# DATA-NEXT:             [[#MAIN]]: 48 c7 c0 00 00 00 00          movq    $0, %rax
+# DATA-NEXT:       [[#MAIN + 0x7]]: c3                            retq
 
 .section __TEXT,__text
 .global _main
 
 _main:
   mov $0, %rax
   ret