[PATCH] D118724: [lld-macho] Add -pagezero_size

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

Nice, almost there!

Could you add a description of what `-pagezero_size` does to the commit message? I know you linked to your original diff, but it would be nice to avoid having people click through to find it :)



================
Comment at: lld/MachO/Driver.cpp:1126
+
+    // So we are not coping this dumb behavior and doing the it in a logical
+    // way, by always rounding down to page size.
----------------
some ld64 engineers may read this, let's not say 'dumb' :)... 'weird' is fine


================
Comment at: lld/MachO/Driver.cpp:1129-1130
+    if (!isAligned(Align(target->getPageSize()), pagezeroSize)) {
+      warn("__PAGEZERO size is not page aligned, rounding down to realign");
+      pagezeroSize -= pagezeroSize % target->getPageSize();
+    }
----------------
how about printing out the new value? like instead of `rounding down to realign`, we could have `rounding down to 0x10000000` or something

(we can print out the size int easily using `Twine::utohexstr`)


================
Comment at: lld/test/MachO/pagezero.s:10
+# RUN: %lld-watchos -lSystem -arch arm64_32 -o %t/arm64_32 %t/arm64_32.o -pagezero_size 100000
+# RUN: llvm-objdump --macho --section-headers %t/arm64_32 | FileCheck %s --check-prefix=CHECK_ARM64_32_NOMRAL
+
----------------
IMO it would make the test clearer if we used `llvm-readobj --macho-segment` instead. Then we could test the segment size directly as follows:
```
# CHECK:        Name: __PAGEZERO
# CHECK-NEXT:   Size:
# CHECK-NEXT:   vmaddr: 0x0
```
also, we can reuse the same check lines if we use FileCheck's numeric substitutions (https://llvm.org/docs/CommandGuide/FileCheck.html#filecheck-numeric-substitution-blocks):
```
# RUN: llvm-readobj --macho-segment %t/arm64_32 | FileCheck %s -D#SIZE=0x0
# CHECK:        Name: __PAGEZERO
# CHECK-NEXT:   Size:
# CHECK-NEXT:   vmaddr: 0x[[#%x,SIZE]]
```


================
Comment at: lld/test/MachO/pagezero.s:12
+
+# CHECK_X86_64_NOMRAL: Sections:
+# CHECK_X86_64_NOMRAL: Idx Name          Size     VMA              Type
----------------
nit: use `-` instead of `_` for `CHECK-` lines


================
Comment at: lld/test/MachO/pagezero.s:28
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %s -o %t/x86_64_misalign.o
+# RUN: %no_fatal_warnings_lld -lSystem -arch x86_64 -o %t/x86_64_misalign %t/x86_64_misalign.o -pagezero_size 1001 2>&1 | FileCheck %s --check-prefix=LINK
----------------
nit: most other tests use this naming scheme


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118724/new/

https://reviews.llvm.org/D118724