[PATCH] D61201: [LLD][ELF] Full support for -n (--nmagic) and -N (--omagic)

[Peter Smith via Phabricator via llvm-commits]

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

Thanks all for the comments. I'll upload a new diff with the changes shortly.



================
Comment at: ELF/Driver.cpp:826
       Args.hasFlag(OPT_merge_exidx_entries, OPT_no_merge_exidx_entries, true);
+  Config->Nmagic = Args.hasFlag(OPT_nmagic, OPT_no_nmagic, false);
   Config->NoinhibitExec = Args.hasArg(OPT_noinhibit_exec);
----------------
MaskRay wrote:
> Since `--omagic` implies `--nmagic`, we can probably use:
> 
> `Config->Nmagic = Config->OMagic || Args.hasFlag(OPT_nmagic, OPT_no_nmagic, false);`
> 
> and delete `Config::Paged`.
> 
> or
> 
> `Config->Paged = !(Config->OMagic || Args.hasFlag(OPT_nmagic, OPT_no_nmagic, false));`
I've gone for the second option (Config->Paged) as I think that disabling Page Alignment when Config->Paged == false makes much more sense than referring directly to Omagic and Nmagic as these option names aren't self describing. 


================
Comment at: ELF/Writer.cpp:2083
   if (OS == First) {
-    uint64_t Alignment = std::max<uint64_t>(OS->Alignment, Config->MaxPageSize);
+    uint64_t PageSize = Config->Paged ? Config->MaxPageSize : 1;
+    uint64_t Alignment = std::max<uint64_t>(OS->Alignment, PageSize);
----------------
MaskRay wrote:
> Do other occurrences of `Config->MaxPageSize` need similar changes?
> 
> i.e, what will happen if we do `if (Config->Nmagic) Config->MaxPageSize = 1;`
> Do other occurrences of Config->MaxPageSize need similar changes?
I don't think any other occurrences need changing in the current implementation, there are some raw uses of Config->MaxPageSize and Target->PageSize but these are not called unless Config->Paged. For example writeTrapInstr() will early exit unless Config->Paged. I thought of wrapping all calls to Config->MaxPageSize and Target->PageSize with a function that substituted 1 if !Config->Paged, I thought that might hide the intention behind the code too much. I don't have a strong opinion so I'd be happy to do this another way.

> i.e, what will happen if we do if (Config->Nmagic) Config->MaxPageSize = 1;
I'm not sure I fully understand here, I think what you mean is that if we make Config->NMagic set Config->MaxPageSize = 1 then will everything work? My first implementation did that but I ran across a couple of problems:
- The Target->PageSize is unaffected which causes problems as there seems to be a working assumption that this is smaller than MaxPageSize
- Use of -n and -N in ld.bfd does not affect the linkerscript commands that refer to Config->MaxPageSize and Target->PageSize such as COMMONPAGESIZE, MAXPAGESIZE, DATA_SEGMENT_RELRO_END. I have my suspicion that these were not supposed to be used in a linker script that uses -N and -n but I prefer consistency where possible.
I think there are some alternative implementations that could work:
- Handle Target->PageSize like Config->MaxPageSize and set them both to 1 (would affect linker scripts as above, but most likely minor).
- Have Config->Paged skip any use of Target->PageSize

Hope I've interpreted that question the right way? 


================
Comment at: test/ELF/aarch64-script-nmagic.s:1
+// REQUIRES: aarch64
+// RUN: llvm-mc -filetype=obj -triple=aarch64-linux-gnu %s -o %t.o
----------------
grimar wrote:
> Can it be not AArch64 specific test, but generic, i.e. x86?
Sure, I've changed it to x86 (just need to change the alignment to 4096).


================
Comment at: test/ELF/aarch64-script-nmagic.s:21
+// RUN: ld.lld %t.o -o %t2.so --shared --hash-style=sysv -n --script %t.script
+// RUN: llvm-readobj --program-headers %t2.so
+
----------------
grimar wrote:
> grimar wrote:
> > This like does not have any `FileCheck`.
> like->line
Thanks for the spot, now fixed.


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

https://reviews.llvm.org/D61201