[PATCH] D56205: Add -z common-page-size option

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added inline comments.


================
Comment at: lld/ELF/Driver.cpp:359
+         S.startswith("max-page-size=") || S.startswith("stack-size=") ||
+         S.startswith("common-page-size=");
 }
----------------
Alpha sort (so put this before the `.startswith("max-page-size=")`


================
Comment at: lld/ELF/Driver.cpp:1182
 
+// Parse -z common-page-size=<value>. The default value 4096
+static uint64_t getCommonPageSize(opt::InputArgList &Args) {
----------------
The default value *is* 4096, and put a period at the end. Also, wouldn't it be more accurate to say that the default value is the page size of the target? That might not necessarily be 4096 ... for example, Arch/SPARCV9.cpp sets it to 8192.

Super nitpicky, but I might place this function above `getMaxPageSize`, to maintain the rough alpha sorting.


================
Comment at: lld/ELF/ScriptParser.cpp:1050
   return [=]() -> uint64_t {
+    if (Target && Config)
+      return Config->CommonPageSize;
----------------
I'm not super familiar with the script parser ... in what situation would Config be null here?


================
Comment at: lld/test/ELF/linkerscript/common-page-size.s:4
+
+# RUN: ld.lld -z common-page-size=0x1000 %t -o %t2
+# RUN: llvm-readobj -program-headers %t2 | FileCheck %s
----------------
Wouldn't it be more interesting to have a test with some value other than `0x1000`, so that you get non-default behavior?


================
Comment at: lld/test/ELF/linkerscript/common-page-size.s:48
+# RUN: echo "SECTIONS { symbol = CONSTANT(MAXPAGESIZE); }" > %t.script
+# RUN: ld.lld -z common-page-size=0x1000 -o %t1 --script %t.script %t
+# RUN: llvm-objdump -t %t1 | FileCheck -check-prefix CHECK-SCRIPT %s
----------------
Same here


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D56205