[lld-macho] Make sure ZeroFill sections are at the end of their segments

[int3]

Summary

ld64 does this, and references an internal rdar:// number as an explanation. No
idea what that rdar issue is, but in practice, it seems that not putting a BSS
section at the end can cause subsequent sections in the same segment to be
overwritten with zeroes.

[smeenai]

This depends on D81887 right?

[int3]

Yup. Not sure why arc didn't automatically link it up...

[smeenai]

In D81888#2094383, @int3 wrote:

Yup. Not sure why arc didn't automatically link it up...

AFAIK arc doesn't infer the link automatically based on your git commit stacking ... you have to set it up manually or put "Depends on Dxxxxx" in your Summary :(

[int3]

On my Mac it was somehow configured to add "Depends on Dxxxxx" automatically

[MaskRay]

Hi, your git commit contains extra Phabricator tags. You can drop Reviewers: Subscribers: Tags: and the text Summary: from the git commit with the following script:

arcfilter () {
        arc amend
        git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -
}

Reviewed By: is considered important by some people. Please keep the tag. (--date=now is my personal preference (author dates are usually not useful. Using committer dates can make log almost monotonic in time))

llvm/utils/git/pre-push.py can validate the message does not include unneeded tags.

[int3]

Yeah sorry I keep forgetting to clean up the commit message before landing. I'll use those scripts in the future. Thanks!

lld/MachO/Writer.cpp

static int sectionOrder(OutputSection *osec) {
   } else if (segname == segment_names::linkEdit) {
     return StringSwitch<int>(osec->name)
         .Case(section_names::binding, -4)
         .Case(section_names::export_, -3)
         .Case(section_names::symbolTable, -2)
         .Case(section_names::stringTable, -1)
         .Default(0);
   }
+  // ZeroFill sections must always be the at the end of their segments,
+  // otherwise subsequent sections may get overwritten with zeroes at runtime.
+  if (isZeroFill(osec->flags))
+    return std::numeric_limits<int>::max();
   return 0;
 }
 
 template <typename T, typename F>
 static std::function<bool(T, T)> compareByOrder(F ord) {
   return [=](T a, T b) { return ord(a) < ord(b); };
 }
 

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 that __bss takes up zero file size, is at file offset zero, and
+## appears at the end of its segment.
 
-# CHECK:        Name: __bss
+# CHECK:        Index: 1
+# CHECK-NEXT:   Name: __data
+# CHECK-NEXT:   Segment: __DATA
+# CHECK-NEXT:   Address:
+# CHECK-NEXT:   Size: 0x8
+# CHECK-NEXT:   Offset: 4096
+# CHECK-NEXT:   Alignment: 0
+# CHECK-NEXT:   RelocationOffset: 0x0
+# CHECK-NEXT:   RelocationCount: 0
+# CHECK-NEXT:   Type: Regular (0x0)
+# CHECK-NEXT:   Attributes [ (0x0)
+# CHECK-NEXT:   ]
+# CHECK-NEXT:   Reserved1: 0x0
+# CHECK-NEXT:   Reserved2: 0x0
+# CHECK-NEXT:   Reserved3: 0x0
+
+# CHECK:        Index: 2
+# CHECK-NEXT:   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: vmsize: 0xC
 # CHECK-NEXT: fileoff:
-# CHECK-NEXT: filesize: 0
+# CHECK-NEXT: filesize: 8
 
 .globl _main
 
 .text
 _main:
   movq $0, %rax
   retq
 
 .bss
 .zero 4
+
+.data
+.quad 0x1234