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

[Peter Smith via Phabricator via llvm-commits]

peter.smith marked 7 inline comments as done.
peter.smith added a comment.

Thanks for the reviews, I'll fix the nits, and will add a comment to describe the difference between max and common page size.



================
Comment at: ELF/Driver.cpp:1125
+    case OPT_nmagic:
+      Config->Static = true;
+      break;
----------------
grimar wrote:
> 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).
Sure, I must have missed the earlier comment, sorry.


================
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) {
----------------
grimar wrote:
> grimar wrote:
> > I think we usually use dashes when mention a option name in error message.
> i.e.
> 
> `error("--common-page-size......`
Sure, in this case it will be -z common-page-size, I'll also do so for -z max-page-size above


================
Comment at: ELF/Driver.cpp:1227
+  }
+  // CommonPageSize can't be larger than MaxPageSize.
+  if (Val > Config->MaxPageSize)
----------------
grimar wrote:
> Should this case have a warning too?
Possibly, I decided not to as we'd need to distinguish between -z common-page-size being increased too high, and -z max-page-size being lowered to below the Target default common page size to avoid a confusing warning. I can add one in a follow up patch if you'd like?


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

https://reviews.llvm.org/D61688