[PATCH] D61688: [LLD][ELF] Full support for -n (--nmagic) and -N (--omagic) via -zcommon-page-size

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

Few minor nits from me.



================
Comment at: ELF/Driver.cpp:1125
+    case OPT_nmagic:
+      Config->Static = true;
+      break;
----------------
MaskRay wrote:
> IIRC @grimar said this should be moved to `case OPT_Bstatic:`.
I think that is better, yes (to group "case options" by effect).


================
Comment at: ELF/Driver.cpp:1221
+  if (!isPowerOf2_64(Val))
+    error("common-page-size: value isn't a power of 2");
+  if (Config->Nmagic || Config->Omagic) {
----------------
I think we usually use dashes when mention a option name in error message.


================
Comment at: ELF/Driver.cpp:1227
+  }
+  // CommonPageSize can't be larger than MaxPageSize.
+  if (Val > Config->MaxPageSize)
----------------
Should this case have a warning too?


================
Comment at: test/ELF/common-page.s:5
+
+# exits with return code 42 on linux
+.globl _start
----------------
This part is copy paste from our very old (if not the oldest :) test case I think.
I believe you do not need this and can use `nop`, don't you?


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

https://reviews.llvm.org/D61688