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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
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);
----------------
peter.smith wrote:
> 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? 
> I think what you mean is that if we make Config->NMagic set Config->MaxPageSize = 1 then will everything work?

That was my question. Thank you for the explanation. 

I also asked this in the binutils-gdb bugzilla https://sourceware.org/bugzilla/show_bug.cgi?id=24505
I will forward their responses.


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

https://reviews.llvm.org/D61201