[lld-macho] Ensure __bss sections we output have file offset of zero

[int3]

Summary

llvm-mc emits __bss sections with an offset of zero, but we weren't expecting
that in our input, so we were copying non-zero data from the start of the file and
putting it in __bss, with obviously undesirable runtime results. (It appears that
the kernel will copy those nonzero bytes as long as the offset is nonzero, regardless
of whether S_ZERO_FILL is set.)

I debated on whether to make a special ZeroFillSection -- separate from a
regular InputSection -- but it seemed like too much work for now. But I'm happy
to refactor if anyone feels strongly about having it as a separate class.

Depends on D80857.

[MaskRay]

I'm not sure it's strictly necessary, but it's what ld64 does.

If no tool enforces this, then maybe this is unnecessary? I believe the ELF counterpart is SHT_NOBITS. sh_offset of a SHT_NOBITS section is ignored, but for aesthetic reasons and practical reasons (monotonic property), linkers reuses the previous section's offset.

BTW, llvm-readobj -S for ELF is so verbose that I stopped using it and switched to llvm-readelf -S for most new tests. If you find Mach-O output verbose, maybe it is time to add a compact style.

[int3]

If no tool enforces this, then maybe this is unnecessary?

If we don't have an offset of zero, then it appears that whatever bytes at that non-zero offset get copied into __bss. Unlike __PAGEZERO, which is a segment, there's no way to say that a section has a non-zero vmsize but a zero filesize: there's just a single size field. So I guess I should've said it's necessary if we don't want to actually allocate disk space for the zero bytes in our binary.

[smeenai]

I'm okay with your isZeroFill solution. Making a separate class seems like overkill for now, considering the check is needed in a grand total of three places (four if you count the resulting conditional in InputSection::writeTo, but still). A separate class might be cleaner from a Software Engineering™ perspective, but I think you'd still end up with two checks in that case (one to make a ZeroFillSection instead of an InputSection, and one for the writer fileOff logic), so it doesn't seem worth the extra complexity.

[int3]

fix ubsan

[int3]

update SAN fix

[int3]

update

lld/MachO/InputFiles.cpp

void InputFile::parseSections(ArrayRef<section_64> sections) {
   subsections.reserve(sections.size());
   auto *buf = reinterpret_cast<const uint8_t *>(mb.getBufferStart());
 
   for (const section_64 &sec : sections) {
     InputSection *isec = make<InputSection>();
     isec->file = this;
     isec->name = StringRef(sec.sectname, strnlen(sec.sectname, 16));
     isec->segname = StringRef(sec.segname, strnlen(sec.segname, 16));
-    isec->data = {buf + sec.offset, static_cast<size_t>(sec.size)};
+    isec->data = {isZeroFill(sec.flags) ? nullptr : buf + sec.offset,
+                  static_cast<size_t>(sec.size)};
     if (sec.align >= 32)
       error("alignment " + std::to_string(sec.align) + " of section " +
             isec->name + " is too large");
     else
       isec->align = 1 << sec.align;
     isec->flags = sec.flags;
     subsections.push_back({{0, isec}});
   }

lld/MachO/InputSection.h

struct Reloc {
   // to.
   uint32_t offset;
   // Adding this offset to the address of the target symbol or subsection gives
   // the destination that this relocation refers to.
   uint64_t addend;
   llvm::PointerUnion<Symbol *, InputSection *> target;
 };
 
+inline bool isZeroFill(uint8_t flags) {
+  return (flags & llvm::MachO::SECTION_TYPE) == llvm::MachO::S_ZEROFILL;
+}
+
 class InputSection {
 public:
   virtual ~InputSection() = default;
   virtual uint64_t getSize() const { return data.size(); }
-  virtual uint64_t getFileSize() const { return getSize(); }
+  virtual uint64_t getFileSize() const {
+    return isZeroFill(flags) ? 0 : getSize();
+  }
   uint64_t getFileOffset() const;
   uint64_t getVA() const;
 
   virtual void writeTo(uint8_t *buf);
 
   InputFile *file = nullptr;
   StringRef name;
   StringRef segname;

lld/MachO/InputSection.cpp

 
 uint64_t InputSection::getFileOffset() const {
   return parent->fileOff + outSecFileOff;
 }
 
 uint64_t InputSection::getVA() const { return parent->addr + outSecOff; }
 
 void InputSection::writeTo(uint8_t *buf) {
-  if (!data.empty())
-    memcpy(buf, data.data(), data.size());
+  if (getFileSize() == 0)
+    return;
+
 
   for (Reloc &r : relocs) {
     uint64_t va = 0;
     if (auto *s = r.target.dyn_cast<Symbol *>())
       va = target->getSymbolVA(*s, r.type);
     else if (auto *isec = r.target.dyn_cast<InputSection *>())
       va = isec->getVA();
 
     uint64_t val = va + r.addend;
     if (r.pcrel)
       val -= getVA() + r.offset;
     target->relocateOne(buf + r.offset, r, val);
   }
 }

lld/MachO/SyntheticSections.cpp

 #include "InputFiles.h"
 #include "MachOStructs.h"
 #include "OutputSegment.h"
 #include "SymbolTable.h"
 #include "Symbols.h"
 #include "Writer.h"
 
 #include "lld/Common/ErrorHandler.h"
+#include "lld/Common/Memory.h"
 #include "llvm/Support/EndianStream.h"
 #include "llvm/Support/LEB128.h"
 
 using namespace llvm;
 using namespace llvm::MachO;
 using namespace llvm::support;
 using namespace llvm::support::endian;
 
void StubHelperSection::setup() {
 
   inputSections.push_back(in.imageLoaderCache);
   symtab->addDefined("__dyld_private", in.imageLoaderCache, 0);
 }
 
 ImageLoaderCacheSection::ImageLoaderCacheSection() {
   segname = segment_names::data;
   name = "__data";
+  uint8_t *arr = bAlloc.Allocate<uint8_t>(WordSize);
+  memset(arr, 0, WordSize);
+  data = {arr, WordSize};
 }
 
 LazyPointerSection::LazyPointerSection()
     : SyntheticSection(segment_names::data, "__la_symbol_ptr") {
   align = 8;
   flags = S_LAZY_SYMBOL_POINTERS;
 }
 

lld/MachO/Writer.cpp

void Writer::assignAddresses(OutputSegment *seg) {
   fileOff = alignTo(fileOff, PageSize);
   seg->fileOff = fileOff;
 
   for (auto &p : seg->getSections()) {
     OutputSection *section = p.second;
     addr = alignTo(addr, section->align);
     fileOff = alignTo(fileOff, section->align);
     section->addr = addr;
-    section->fileOff = fileOff;
+    section->fileOff = isZeroFill(section->flags) ? 0 : fileOff;
     section->finalize();
 
     addr += section->getSize();
     fileOff += section->getFileSize();
   }
 }
 
 void Writer::openFile() {

lld/test/MachO/bss.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %s -o %t.o
+# RUN: lld -flavor darwinnew -o %t %t.o
+# RUN: llvm-readobj --section-headers --macho-segment %t | FileCheck %s
+
+## Check that __bss takes up zero file size and is at file offset zero.
+
+# CHECK:        Name: __bss
+# CHECK-NEXT:   Segment: __DATA
+# CHECK-NEXT:   Address:
+# CHECK-NEXT:   Size: 0x4
+# CHECK-NEXT:   Offset: 0
+# CHECK-NEXT:   Alignment: 0
+# CHECK-NEXT:   RelocationOffset: 0x0
+# CHECK-NEXT:   RelocationCount: 0
+# CHECK-NEXT:   Type: ZeroFill (0x1)
+# CHECK-NEXT:   Attributes [ (0x0)
+# CHECK-NEXT:   ]
+# CHECK-NEXT:   Reserved1: 0x0
+# CHECK-NEXT:   Reserved2: 0x0
+# CHECK-NEXT:   Reserved3: 0x0
+
+# CHECK:      Name: __DATA
+# CHECK-NEXT: Size:
+# CHECK-NEXT: vmaddr:
+# CHECK-NEXT: vmsize: 0x4
+# CHECK-NEXT: fileoff:
+# CHECK-NEXT: filesize: 0
+
+.globl _main
+
+.text
+_main:
+  movq $0, %rax
+  retq
+
+.bss
+.zero 4