[ELF] Support -z max-page-size option

[phosek]

Summary

This option is already supported by both BFD ld and gold and allows overriding the max page size whose default value is defined by the target.

[davide]

Out of curiosity, are you using this option anywhere?
I think this is semantically fine if you have an use case, I left some comments around.

ELF/Driver.cpp

660 ↗

(On Diff #72383)

You might want to elaborate this a bit. Maybe, mentioning that there's a target default MAXPAGESIZE anyway and this option overrides the value.

test/ELF/zmax-page-size.s

1 ↗

(On Diff #72383)

Linker script tests go in ELF/linkerscript

[phosek]

We do, see https://fuchsia.googlesource.com/magenta/+/master/kernel/arch/x86/rules.mk#150 for example. This option is supported by both BFD ld and gold and it would be nice if LLD supported the option, since it's used by existing software.

[davide]

LGTM

[phosek]

Thanks, there is also a similar patch for -z common-page-size if you want to take a look: https://reviews.llvm.org/D24891.

[davide]

In D24891#551739, @phosek wrote:

Thanks, there is also a similar patch for -z common-page-size if you want to take a look: https://reviews.llvm.org/D24891.

Sure, commented there. In the future, feel free to cc: me or list me as reviewer so that the patch can get my attention more easily.

[phosek]

The updated version combines the logic for handling both -z max-page-size and -z common-page-size which also makes https://reviews.llvm.org/D24898 unnecessary.

[davide]

What happens if you pass a negative value to -z common-page-size ?

ELF/Driver.cpp

660–667 ↗

(On Diff #72416)

There are other -z options supported, but not all of them take an integer option, e.g. -z now. Can this function be generalized to cover all them? That seems a little bit too much to ask, so you can probably just rename this one.

662–663 ↗

(On Diff #72416)

can you add a test for this case?

[phosek]

You will get the invalid value error (I've added a test for this). This is handled by the Value->getAsInteger method since the value is defined as uint64_t.

[grimar]

test/ELF/linkerscript/zcommon-page-size.s

7 ↗

(On Diff #72421)

But should this work ?
We had a patch for this option earlier: D19600 and its implementation checked that:

if (Config->ZMaxPageSize < Target->PageSize ||
  (Config->ZMaxPageSize % Target->PageSize != 0))
    error("wrong page size specified: " + Twine(Config->ZMaxPageSize));

This is checks that GNU linkers do (I think both of them, but not really remember now) and this
checks looks reasonable to have for me.

[ruiu]

ELF/Driver.cpp

267–268 ↗

(On Diff #72421)

I think you could change this function to take one more parameter and return an integer.

uint64_t getZOptionValue(opt::InputArgList &Args, StringRef Key, uint64_t Default)

495–497 ↗

(On Diff #72421)

then you could do

Config->ZStackSize = getZOptionValue(Args, "stack-size", -1);

671–672 ↗

(On Diff #72421)

and

Config->CommonPageSize = getZOption(Args, "common-page-size", Target->PageSize);
Config->MaxPageSize = getZOption(Args, "max-page-size", Target->PageSize);

[phosek]

I was checking the behavior of both BFD ld and gold. BFD ld checks whether the argument is power of 2 while gold doesn't, but I don't see any reason why page size wouldn't be a power 2 so I've added that check as well. The handling of common-page-size is strange, there doesn't seem to be a clear consensus on how exactly that value should be interpreted.

[ruiu]

ELF/Config.h

141 ↗

(On Diff #72573)

Please remove = -1 because the variable is always assigned now.

ELF/Driver.cpp

277 ↗

(On Diff #72573)

Can you return Result here and return Default at end?

666 ↗

(On Diff #72573)

We have isPowerOf2_64 in MathExtras.h.

672 ↗

(On Diff #72573)

If there's no clear consensus how common-page-size should be interpreted, I'd vote for the simplest rule: just -z common-page-size's value to CommonPageSize.

[ruiu]

LGTM

[davide]

I have one single last comment, then LGTM.
I think you can merge the two *.s files into one as they share the same ASM.

Thanks!

[phosek]

Done.

[phosek]

This is getting more complicated, I was looking at using -z max-page-size and -z common-page-size values in the Writer and found that lld's use of page size is somewhat inconsistent so I filed a bug 30541 and proposed possible solutions. It'd be great to decide on the solution for the page size handling before landing this patch as it might require further changes (e.g. if we decide to only use the maximum page size, we probably shouldn't accept the -z common-page-size flag).

[davide]

In D24891#553952, @phosek wrote:

This is getting more complicated, I was looking at using -z max-page-size and -z common-page-size values in the Writer and found that lld's use of page size is somewhat inconsistent so I filed a bug 30541 and proposed possible solutions. It'd be great to decide on the solution for the page size handling before landing this patch as it might require further changes (e.g. if we decide to only use the maximum page size, we probably shouldn't accept the -z common-page-size flag).

Slightly related. There were problems in the past because lld was aliasing MAXPAGESIZE and PAGE_SIZE on x86-64 and this caused the FreeBSD kernel to crash when linked with lld so I want to be sure that configuration is tested when experimenting with page sizes. I assume that if we mimic what bfd does we should be OK in that case, as the kernel is currently linked with a very old (2.17) version of it.

[phosek]

Now that https://reviews.llvm.org/D24987 has landed, I have revised the patch to make sure that the MaxPageSize value is properly propagated to the writer and also added a test that checks that the wiring is working. I've also removed the support for -z common-page-size, I don't think we should be supporting that option until (if ever) we decide to implement the page saving logic because that's the only place where that option comes into play. Could you please take a look again?

[• rafael]

ELF/Driver.cpp

671 ↗

(On Diff #72752)

Why do you need to set bits in Target? It would be more natural to change users to use the Config value and default that to the Target one.

lld/trunk/ELF/Config.h

struct Configuration {
   StripPolicy Strip = StripPolicy::None;
   UnresolvedPolicy UnresolvedSymbols;
   BuildIdKind BuildId = BuildIdKind::None;
   ELFKind EKind = ELFNoneKind;
   uint16_t DefaultSymbolVersion = llvm::ELF::VER_NDX_GLOBAL;
   uint16_t EMachine = llvm::ELF::EM_NONE;
   uint64_t EntryAddr = 0;
   uint64_t ImageBase;
-  uint64_t ZStackSize = -1;
+  uint64_t MaxPageSize;
+  uint64_t ZStackSize;
   unsigned LtoJobs;
   unsigned LtoO;
   unsigned Optimize;
 };
 
 // The only instance of Configuration struct.
 extern Configuration *Config;
 
 } // namespace elf
 } // namespace lld
 
 #endif

lld/trunk/ELF/Driver.cpp

 
 static bool hasZOption(opt::InputArgList &Args, StringRef Key) {
   for (auto *Arg : Args.filtered(OPT_z))
     if (Key == Arg->getValue())
       return true;
   return false;
 }
 
-static Optional<StringRef>
-getZOptionValue(opt::InputArgList &Args, StringRef Key) {
+static uint64_t
+getZOptionValue(opt::InputArgList &Args, StringRef Key, uint64_t Default) {
   for (auto *Arg : Args.filtered(OPT_z)) {
     StringRef Value = Arg->getValue();
     size_t Pos = Value.find("=");
-    if (Pos != StringRef::npos && Key == Value.substr(0, Pos))
-      return Value.substr(Pos + 1);
+    if (Pos != StringRef::npos && Key == Value.substr(0, Pos)) {
+      Value = Value.substr(Pos + 1);
+      uint64_t Result;
+      if (Value.getAsInteger(0, Result))
+        error("invalid " + Key + ": " + Value);
+      return Result;
     }
-  return None;
+  }
+  return Default;
 }
 
 void LinkerDriver::main(ArrayRef<const char *> ArgsArr) {
   ELFOptTable Parser;
   opt::InputArgList Args = Parser.parse(ArgsArr.slice(1));
   if (Args.hasArg(OPT_help)) {
     printHelp(ArgsArr[0]);
     return;
void LinkerDriver::readConfigs(opt::InputArgList &Args) {
   Config->ZNodelete = hasZOption(Args, "nodelete");
   Config->ZNow = hasZOption(Args, "now");
   Config->ZOrigin = hasZOption(Args, "origin");
   Config->ZRelro = !hasZOption(Args, "norelro");
 
   if (!Config->Relocatable)
     Config->Strip = getStripOption(Args);
 
-  if (Optional<StringRef> Value = getZOptionValue(Args, "stack-size"))
-    if (Value->getAsInteger(0, Config->ZStackSize))
-      error("invalid stack size: " + *Value);
+  Config->ZStackSize = getZOptionValue(Args, "stack-size", -1);
 
   // Config->Pic is true if we are generating position-independent code.
   Config->Pic = Config->Pie || Config->Shared;
 
   if (auto *Arg = Args.getLastArg(OPT_hash_style)) {
     StringRef S = Arg->getValue();
     if (S == "gnu") {
       Config->GnuHash = true;
if (auto *Arg = Args.getLastArg(OPT_image_base)) {
     if (S.getAsInteger(0, Config->ImageBase))
       error(Arg->getSpelling() + ": number expected, but got " + S);
     else if ((Config->ImageBase % Target->MaxPageSize) != 0)
       warning(Arg->getSpelling() + ": address isn't multiple of page size");
   } else {
     Config->ImageBase = Config->Pic ? 0 : Target->DefaultImageBase;
   }
 
+  // Initialize Config->MaxPageSize. The default value is defined by
+  // the target, but it can be overriden using the option.
+  Config->MaxPageSize =
+      getZOptionValue(Args, "max-page-size", Target->MaxPageSize);
+  if (!isPowerOf2_64(Config->MaxPageSize))
+    error("max-page-size: value isn't a power of 2");
+
   // Add all files to the symbol table. After this, the symbol table
   // contains all known names except a few linker-synthesized symbols.
   for (InputFile *F : Files)
     Symtab.addFile(F);
 
   // Add the start symbol.
   // It initializes either Config->Entry or Config->EntryAddr.
   // Note that AMDGPU binaries have no entries.

lld/trunk/ELF/LinkerScript.cpp

Expr ScriptParser::readExpr1(Expr Lhs, int MinPrec) {
   }
   return Lhs;
 }
 
 uint64_t static getConstant(StringRef S) {
   if (S == "COMMONPAGESIZE")
     return Target->PageSize;
   if (S == "MAXPAGESIZE")
-    return Target->MaxPageSize;
+    return Config->MaxPageSize;
   error("unknown constant: " + S);
   return 0;
 }
 
 // Parses Tok as an integer. Returns true if successful.
 // It recognizes hexadecimal (prefixed with "0x" or suffixed with "H")
 // and decimal numbers. Decimal numbers may have "K" (kilo) or
 // "M" (mega) prefixes.

lld/trunk/ELF/Writer.cpp

 
 // Assign VAs (addresses at run-time) to output sections.
 template <class ELFT> void Writer<ELFT>::assignAddresses() {
   uintX_t VA = Config->ImageBase + getHeaderSize<ELFT>();
   uintX_t ThreadBssOffset = 0;
   for (OutputSectionBase<ELFT> *Sec : OutputSections) {
     uintX_t Alignment = Sec->getAlignment();
     if (Sec->PageAlign)
-      Alignment = std::max<uintX_t>(Alignment, Target->MaxPageSize);
+      Alignment = std::max<uintX_t>(Alignment, Config->MaxPageSize);
 
     auto I = Config->SectionStartMap.find(Sec->getName());
     if (I != Config->SectionStartMap.end())
       VA = I->second;
 
     // We only assign VAs to allocated sections.
     if (needsPtLoad(Sec)) {
       VA = alignTo(VA, Alignment);

 // Adjusts the file alignment for a given output section and returns
 // its new file offset. The file offset must be the same with its
 // virtual address (modulo the page size) so that the loader can load
 // executables without any address adjustment.
 template <class ELFT, class uintX_t>
 static uintX_t getFileAlignment(uintX_t Off, OutputSectionBase<ELFT> *Sec) {
   uintX_t Alignment = Sec->getAlignment();
   if (Sec->PageAlign)
-    Alignment = std::max<uintX_t>(Alignment, Target->MaxPageSize);
+    Alignment = std::max<uintX_t>(Alignment, Config->MaxPageSize);
   Off = alignTo(Off, Alignment);
 
   // Relocatable output does not have program headers
   // and does not need any other offset adjusting.
   if (Config->Relocatable || !(Sec->getFlags() & SHF_ALLOC))
     return Off;
-  return alignTo(Off, Target->MaxPageSize, Sec->getVA());
+  return alignTo(Off, Config->MaxPageSize, Sec->getVA());
 }
 
 template <class ELFT, class uintX_t>
 void setOffset(OutputSectionBase<ELFT> *Sec, uintX_t &Off) {
   if (Sec->getType() == SHT_NOBITS) {
     Sec->setFileOffset(Off);
     return;
   }
if (First) {
       H.p_filesz = Last->getFileOff() - First->getFileOff();
       if (Last->getType() != SHT_NOBITS)
         H.p_filesz += Last->getSize();
       H.p_memsz = Last->getVA() + Last->getSize() - First->getVA();
       H.p_offset = First->getFileOff();
       H.p_vaddr = First->getVA();
     }
     if (H.p_type == PT_LOAD)
-      H.p_align = Target->MaxPageSize;
+      H.p_align = Config->MaxPageSize;
     else if (H.p_type == PT_GNU_RELRO)
       H.p_align = 1;
 
     if (!P.HasLMA) {
     // The p_paddr field can be set using linker script AT command.
     // By default, it is the same value as p_vaddr.
       H.p_paddr = H.p_vaddr;
       if (H.p_type == PT_LOAD && First)

lld/trunk/test/ELF/linkerscript/page-size.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
+
+# RUN: ld.lld -z max-page-size=0x4000 %t -o %t2
+# RUN: llvm-readobj -program-headers %t2 | FileCheck %s
+
+# CHECK:      ProgramHeaders [
+# CHECK:        ProgramHeader {
+# CHECK:          Type: PT_LOAD
+# CHECK-NEXT:     Offset: 0x0
+# CHECK-NEXT:     VirtualAddress: 0x10000
+# CHECK-NEXT:     PhysicalAddress: 0x10000
+# CHECK-NEXT:     FileSize: 344
+# CHECK-NEXT:     MemSize: 344
+# CHECK-NEXT:     Flags [
+# CHECK-NEXT:       PF_R
+# CHECK-NEXT:     ]
+# CHECK-NEXT:     Alignment: 16384
+# CHECK-NEXT:   }
+# CHECK-NEXT:   ProgramHeader {
+# CHECK-NEXT:     Type: PT_LOAD
+# CHECK-NEXT:     Offset: 0x4000
+# CHECK-NEXT:     VirtualAddress: 0x14000
+# CHECK-NEXT:     PhysicalAddress: 0x14000
+# CHECK-NEXT:     FileSize: 1
+# CHECK-NEXT:     MemSize: 1
+# CHECK-NEXT:     Flags [
+# CHECK-NEXT:       PF_R
+# CHECK-NEXT:       PF_X
+# CHECK-NEXT:     ]
+# CHECK-NEXT:     Alignment: 16384
+# CHECK-NEXT:   }
+# CHECK-NEXT:   ProgramHeader {
+# CHECK-NEXT:     Type: PT_LOAD
+# CHECK-NEXT:     Offset: 0x8000
+# CHECK-NEXT:     VirtualAddress: 0x18000
+# CHECK-NEXT:     PhysicalAddress: 0x18000
+# CHECK-NEXT:     FileSize: 8
+# CHECK-NEXT:     MemSize: 8
+# CHECK-NEXT:     Flags [
+# CHECK-NEXT:       PF_R
+# CHECK-NEXT:       PF_W
+# CHECK-NEXT:     ]
+# CHECK-NEXT:     Alignment: 16384
+# CHECK-NEXT:   }
+
+# RUN: echo "SECTIONS { \
+# RUN:         symbol = CONSTANT(MAXPAGESIZE); \
+# RUN:       }" > %t.script
+# RUN: ld.lld -z max-page-size=0x4000 -o %t1 --script %t.script %t
+# RUN: llvm-objdump -t %t1 | FileCheck -check-prefix CHECK-SCRIPT %s
+
+# CHECK-SCRIPT: 0000000000004000 *ABS* 00000000 symbol
+
+# RUN: not ld.lld -z max-page-size=0x1001 -o %t1 --script %t.script %t 2>&1 \
+# RUN:  | FileCheck -check-prefix=ERR1 %s
+# ERR1: max-page-size: value isn't a power of 2
+
+# RUN: not ld.lld -z max-page-size=-0x1000 -o %t1 --script %t.script %t 2>&1 \
+# RUN:  | FileCheck -check-prefix=ERR2 %s
+# ERR2: invalid max-page-size: -0x1000
+
+.global _start
+_start:
+  nop
+
+.section .a, "aw"
+.quad 0