[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

663

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

663–670

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.

665–666

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

8

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

266–268

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)

499–500

then you could do

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

674–675

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

140–141

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

ELF/Driver.cpp

276

Can you return Result here and return Default at end?

667

We have isPowerOf2_64 in MathExtras.h.

673

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

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.

ELF/Config.h

struct Configuration {
   bool ZOrigin;
   bool ZRelro;
   DiscardPolicy Discard;
   SortSectionPolicy SortSection;
   StripPolicy Strip = StripPolicy::None;
   UnresolvedPolicy UnresolvedSymbols;
   BuildIdKind BuildId = BuildIdKind::None;
   ELFKind EKind = ELFNoneKind;
+  uint64_t CommonPageSize;
   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

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->ZExecStack = hasZOption(Args, "execstack");
   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->PageSize) != 0)
       warning(Arg->getSpelling() + ": address isn't multiple of page size");
   } else {
     Config->ImageBase = Config->Pic ? 0 : Target->DefaultImageBase;
   }
 
+  // Initialize Config->MaxPageSize and Config->CommonPageSize. 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");
+  Config->CommonPageSize =
+      getZOptionValue(Args, "common-page-size", Target->PageSize);
+  if (!isPowerOf2_64(Config->CommonPageSize))
+    error("common-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.
   bool HasEntryAddr = false;
   if (!Config->Entry.empty()) {

ELF/LinkerScript.cpp

while (!atEOF() && !Error) {
 
     Lhs = combine(Op1, Lhs, Rhs);
   }
   return Lhs;
 }
 
 uint64_t static getConstant(StringRef S) {
   if (S == "COMMONPAGESIZE")
-    return Target->PageSize;
+    return Config->CommonPageSize;
   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.

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

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
+
+# RUN: echo "SECTIONS { \
+# RUN:         symbol = CONSTANT(COMMONPAGESIZE); \
+# RUN:       }" > %t.script
+# RUN: ld.lld -z common-page-size=0x1000 -o %t1 --script %t.script %t
+# RUN: llvm-objdump -t %t1 | FileCheck %s
+
+# RUN: not ld.lld -z common-page-size=0x1001 -o %t1 --script %t.script %t 2>&1 \
+# RUN:  | FileCheck -check-prefix=ERR1 %s
+# ERR1: common-page-size: value isn't a power of 2
+
+# RUN: not ld.lld -z common-page-size=-0x1000 -o %t1 --script %t.script %t 2>&1 \
+# RUN:  | FileCheck -check-prefix=ERR2 %s
+# ERR2: invalid common-page-size: -0x1000
+
+# CHECK: 0000000000001000 *ABS* 00000000 symbol
+
+.global _start
+_start:
+  nop

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

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
+
+# RUN: echo "SECTIONS { \
+# RUN:         symbol = CONSTANT(MAXPAGESIZE); \
+# RUN:       }" > %t.script
+# RUN: ld.lld -z max-page-size=0x1000 -o %t1 --script %t.script %t
+# RUN: llvm-objdump -t %t1 | FileCheck %s
+
+# 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
+
+# CHECK: 0000000000001000 *ABS* 00000000 symbol
+
+.global _start
+_start:
+ nop