Page MenuHomePhabricator

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

Authored by peter.smith on May 8 2019, 8:51 AM.

Details

Summary

This is an alternative implementation of -n and -N to D61201 as this was suggested as a review comment. Instead of skipping page alignment when -n (--nmagic) and -N (--omagic) are used but leaving the values of the Config->MaxPageSize and Target->PageSize unchanged, we instead:

  • Use Config->CommonPageSize to store the appropriate Target->PageSize value
  • When -n or -N is used we set the Config->MaxPageSize and Config->PageSize to 1 so all page-alignment does nothing.
  • We still have to account for -n and -N meaning -Bstatic.
  • We still have to not allocate headers unless explicitly stated in the linker script.

As we now have a modifiable Config->CommonPageSize it becomes trivial to support the -zcommon-page-size command line option so I've done that and added a test case for it. This could be split into a different patch if it makes sense. I've also added a test to show the effect of increasing -zcommon-page-size. Other test cases have been taken from D61201

This approach has the advantage that none of the page alignment statements have to be changed, and any future ones we add won't need guarding. It has the disadvantage that the CONSTANT(MAXPAGESIZE) and CONSTANT(COMMONPAGESIZE) linker script commands will be affected when -n or -N is used. There is at least one linkerscript in Tianocore edk2 that (ab)uses -n -z common-page-size=0x20 to use CONSTANT(COMMONPAGESIZE) as if it were a preprocessor macro set with -D in the compiler. The usual approach to this is to pre-process the linkerscript.

Diff Detail

Repository
rL LLVM

Event Timeline

peter.smith created this revision.May 8 2019, 8:51 AM

So D61688 implements -z common-page-size as a by-product :)
I compared it with D61201. It has a bit more set-up code in Driver.cpp but that part is easy to reason about. In return, it simplifies a few places where D61201 additionally checks Config->Paged, so I am in favor of D61688:

// Writer<ELFT>::finalizeSections()
-  if (!Script->HasSectionsCommand && !Config->Relocatable && Config->Paged)
+  if (!Script->HasSectionsCommand && !Config->Relocatable)
     fixSectionAlignments();

// computeFileOffset
-    uint64_t PageSize = Config->Paged ? Config->MaxPageSize : 1;
-    uint64_t Alignment = std::max<uint64_t>(OS->Alignment, PageSize);
+    uint64_t Alignment = std::max<uint64_t>(OS->Alignment, Config->MaxPageSize);

// Writer<ELFT>::assignFileOffsets()
-    if (LastRX && LastRX->LastSec == Sec && Config->Paged)
-      Off = alignTo(Off, Target->PageSize);
+    if (LastRX && LastRX->LastSec == Sec)
+      Off = alignTo(Off, Config->CommonPageSize);

// Writer<ELFT>::writeTrapInstr()
-  if (Script->HasSectionsCommand || !Config->Paged)
+  if (Script->HasSectionsCommand)
ruiu added a comment.May 9 2019, 4:08 AM

Sorry for my ignorance, but I don't think I fully understand the difference of "page size" and "common page size". In what situation you want to set different values to these variables? Is there any constraint between them, such as one value is always greater than the other, etc.?

ELF/Options.td
237 ↗(On Diff #198669)

Please add (default).

240 ↗(On Diff #198669)

Ditto

Sorry for my ignorance, but I don't think I fully understand the difference of "page size" and "common page size". In what situation you want to set different values to these variables? Is there any constraint between them, such as one value is always greater than the other, etc.?

"page size" as in Target->PageSize and "common page size" as in Config->CommonPageSize that I renamed it to from this patch are synonymous. From memory I think ld.bfd uses page_size for common-page and abi_page_size for "max page size", I preferred to make the CommonPageSize more explicit.

There is a difference between "max page size" and "common page size". The best explanation I've seen is in the review comment https://reviews.llvm.org/D33630#767301 in the patch that introduced it's only use in LLD outside of the linker script CONSTANT(COMMONPAGESIZE). A quick summary is:

  • max-page-size or abi-page-size is the largest page size that might be encountered at run-time. For example some AArch64 platforms such as Fedora use 64k page size, others such as Ubuntu use 4k page size. If an executable is to be portable it sets max-page-size to 64k. All significant layout decisions have to use the max-page-size.
  • common-page-size or sometimes just page-size is the "most common" page-size that might be encountered at run-time. There are some optimisations that can be done that use this. The best explanation that I've seen is the description DATA_SEGMENT_ALIGN at https://sourceware.org/binutils/docs-2.30/ld/Builtin-Functions.html#Builtin-Functions. As I understand it, LLD only uses common-page-size for D33630 "Fill the empty space in executable segments with instruction padding" and in the linker script CONSTANT(COMMONPAGESIZE).

There is a constraint (enforced by Gold and this patch) that "common page size" can't be greater than "max page size". This constraint is the only requirement for -n and -N, if we lower -zmax-page-size to 1 the Target->PageSize remains at 4k and this produces an incorrect output. It just so happens that implementing that requires implementing all of -zcommon-page-size apart for the command-line option -zcommon-page-size.

Why would anyone want to change the common-page-size with -zcommon-page-size. Good question; the strongest reason I can come up with is backwards compatibility with the small number programs that use it (for example D56205). I don't think that there is a strong enough reason to implement it on its own, but it is just adding a command line option and a test.

Update diff to add (default) to the --no-omagic and --no-nmagic options.

LGTM

ELF/Driver.cpp
1125 ↗(On Diff #198829)

IIRC @grimar said this should be moved to case OPT_Bstatic:.

Few minor nits from me.

ELF/Driver.cpp
1125 ↗(On Diff #198829)

I think that is better, yes (to group "case options" by effect).

1221 ↗(On Diff #198829)

I think we usually use dashes when mention a option name in error message.

1227 ↗(On Diff #198829)

Should this case have a warning too?

test/ELF/common-page.s
5 ↗(On Diff #198829)

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?

grimar added inline comments.May 13 2019, 5:55 AM
ELF/Driver.cpp
1221 ↗(On Diff #198829)

i.e.

error("--common-page-size......

ruiu accepted this revision.May 13 2019, 6:04 AM

LGTM

  • max-page-size or abi-page-size is the largest page size that might be encountered at run-time. For example some AArch64 platforms such as Fedora use 64k page size, others such as Ubuntu use 4k page size. If an executable is to be portable it sets max-page-size to 64k. All significant layout decisions have to use the max-page-size.
  • common-page-size or sometimes just page-size is the "most common" page-size that might be encountered at run-time. There are some optimisations that can be done that use this. The best explanation that I've seen is the description DATA_SEGMENT_ALIGN at https://sourceware.org/binutils/docs-2.30/ld/Builtin-Functions.html#Builtin-Functions. As I understand it, LLD only uses common-page-size for D33630 "Fill the empty space in executable segments with instruction padding" and in the linker script CONSTANT(COMMONPAGESIZE).

This is an excellent summary of the differences of the two. Could you add this to this patch as a comment?

This revision is now accepted and ready to land.May 13 2019, 6:04 AM
peter.smith marked 7 inline comments as done.May 13 2019, 6:17 AM

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

ELF/Driver.cpp
1125 ↗(On Diff #198829)

Sure, I must have missed the earlier comment, sorry.

1221 ↗(On Diff #198829)

Sure, in this case it will be -z common-page-size, I'll also do so for -z max-page-size above

1227 ↗(On Diff #198829)

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?

grimar added inline comments.May 13 2019, 6:24 AM
ELF/Driver.cpp
1227 ↗(On Diff #198829)

Up to you.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2019, 9:01 AM
Herald added a subscriber: jrtc27. · View Herald Transcript