[ELF] Set Dot initially to --image-base value when using linker scripts

[jhenderson]

Summary

This is an alternative to D38238. When parsing linker scripts, LLD starts with a '.' value of 0, regardless of the internal default image base for the target, and regardless of switches such as --image-base. It seems reasonable to use a different image base value when using linker scripts and --image-base is specified, since otherwise the switch has no effect. However, the default image base should not be used when processing linker scripts, because this will change the behaviour for existing linker script users, and potentially result in invalid output being produced, as a subsequent assignment to dot could move the location counter backwards (see the comments in D38238 for examples).

[grimar]

ELF/Config.h

177 ↗

(On Diff #116989)

I think proper place for this would be ScriptConfiguration,
though please wait for Rui's opinion.

[ruiu]

ELF/Driver.cpp

911–912 ↗

(On Diff #116989)

This and other functions in this file are intended to be "pure" functions, that don't have any side-effects. They are "getters" and are not expected to do anything other than returning values. So please do not set a value to a Config member from a "get" function.

[jhenderson]

Address review comment.

@grimar - I don't think the new variable should be in ScriptConfiguration. From what I can tell, ScriptConfiguration is intended for settings controlled by the linker script, which this is not. This is controlled by the command-line.

[ruiu]

ELF/Config.h

176–177 ↗

(On Diff #117682)

So, now you have two variables for the image base. Their valid combinations are:

• ImageBase has value N, and InitialDot also has N. This happens only when you explicitly passed an -image-base.
• ImageBase has value N, and InitialDot has 0. This happens only when you didn't pass an -image-base.

Other combinations are all invalid. That means you are effectively using InitiaDot as a boolean variable. It doesn't have to be an integer, and to convey the intention how it is used, it should be a boolean.

I'd make these changes:

• Change the type of ImageBase from uint64_t to Optional<uint64_t>, and set a value only when an -image-base is given.
• Define a function, getImageBase, which returns either Config->ImageBase or Target->DefaultImageBase.

[jhenderson]

Addressed review comments.

I'm not sure where to put the new getImageBase function, because it is needed in SyntheticSections.cpp as well as LinkerScript.cpp. To me it feels like it really belongs as a member of the Configuration class, but that is currently made up entirely of variables, and no methods, so I've put it in the LinkerScript class. I'm open to other suggestions.

[grimar]

ELF/Driver.cpp

924 ↗

(On Diff #117810)

It is common to

return None;

[jhenderson]

Thanks @grimar. I've changed to use None.

[ruiu]

ELF/LinkerScript.cpp

349 ↗

(On Diff #117814)

I wonder if you really have to call this function twice. Can you initialize Dot only once in LinkerScript's ctor?

355 ↗

(On Diff #117814)

This function does not use any LinkerScript's private/protected members. It is a good practice to not make such function a member function of some class. They can just be a non-member function. By doing that, it is obvious that the function doesn't access any private members.

That said, you can move this function to Target, and by doing that, you can make Target::DefaultImageBase a protected member. Can you do that?

[jhenderson]

Address review comments.

[jhenderson]

ELF/LinkerScript.cpp

349 ↗

(On Diff #117814)

No, because the value is not final until after command-line processing, whereas the LinkerScript instance is constructed during program initialisation. We also need to reset Dot every time we call assignAddresses, because it might be called multiple times. However, having looked closer it does not need setting in processCommands (it's never updated, and should never be referenced, although it currently could be in SUBALIGN expressions - this is a bug, these expressions should only accept constants, and the current behaviour results in an assertion failure if it is used). This allows me to move the function inline into assignAddresses.

[ruiu]

LGTM

ELF/LinkerScript.cpp

349 ↗

(On Diff #117814)

Thank you for removing the assignment to Dot from this function!

ELF/Target.cpp

173 ↗

(On Diff #118193)

this is a TargetInfo, so remove Target->.

lld/trunk/ELF/Config.h

struct Configuration {
   StripPolicy Strip;
   UnresolvedPolicy UnresolvedSymbols;
   Target2Policy Target2;
   BuildIdKind BuildId = BuildIdKind::None;
   ELFKind EKind = ELFNoneKind;
   uint16_t DefaultSymbolVersion = llvm::ELF::VER_NDX_GLOBAL;
   uint16_t EMachine = llvm::ELF::EM_NONE;
   uint64_t ErrorLimit = 20;
-  uint64_t ImageBase;
+  llvm::Optional<uint64_t> ImageBase;
   uint64_t MaxPageSize;
   uint64_t ZStackSize;
   unsigned LTOPartitions;
   unsigned LTOO;
   unsigned Optimize;
   unsigned ThinLTOJobs;
 
   // The following config options do not directly correspond to any

lld/trunk/ELF/Driver.cpp

static uint64_t getMaxPageSize(opt::InputArgList &Args) {
   uint64_t Val =
       getZOptionValue(Args, "max-page-size", Target->DefaultMaxPageSize);
   if (!isPowerOf2_64(Val))
     error("max-page-size: value isn't a power of 2");
   return Val;
 }
 
 // Parses -image-base option.
-static uint64_t getImageBase(opt::InputArgList &Args) {
-  // Use default if no -image-base option is given.
-  // Because we are using "Target" here, this function
-  // has to be called after the variable is initialized.
+static Optional<uint64_t> getImageBase(opt::InputArgList &Args) {
+  // Because we are using "Config->MaxPageSize" here, this function has to be
+  // called after the variable is initialized.
   auto *Arg = Args.getLastArg(OPT_image_base);
   if (!Arg)
-    return Config->Pic ? 0 : Target->DefaultImageBase;
+    return None;
 
   StringRef S = Arg->getValue();
   uint64_t V;
   if (!to_integer(S, V)) {
     error("-image-base: number expected, but got " + S);
     return 0;
   }
   if ((V % Config->MaxPageSize) != 0)

lld/trunk/ELF/LinkerScript.cpp

void LinkerScript::processCommands(OutputSectionFactory &Factory) {
   Aether->SectionIndex = 1;
   auto State = make_unique<AddressState>(Opt);
   // CurAddressState captures the local AddressState and makes it accessible
   // deliberately. This is needed as there are some cases where we cannot just
   // thread the current state through to a lambda function created by the
   // script parser.
   CurAddressState = State.get();
   CurAddressState->OutSec = Aether;
-  Dot = 0;
 
   for (size_t I = 0; I < Opt.Commands.size(); ++I) {
     // Handle symbol assignments outside of any output section.
     if (auto *Cmd = dyn_cast<SymbolAssignment>(Opt.Commands[I])) {
       addSymbol(Cmd);
       continue;
     }
 
void LinkerScript::fabricateDefaultCommands() {
 
   // The Sections with -T<section> have been sorted in order of ascending
   // address. We must lower StartAddr if the lowest -T<section address> as
   // calls to setDot() must be monotonically increasing.
   for (auto &KV : Config->SectionStartMap)
     StartAddr = std::min(StartAddr, KV.second);
 
   auto Expr = [=] {
-    return std::min(StartAddr, Config->ImageBase + elf::getHeaderSize());
+    return std::min(StartAddr, Target->getImageBase() + elf::getHeaderSize());
   };
   Opt.Commands.insert(Opt.Commands.begin(),
                       make<SymbolAssignment>(".", Expr, ""));
 }
 
 static OutputSection *findByName(ArrayRef<BaseCommand *> Vec,
                                  StringRef Name) {
   for (BaseCommand *Base : Vec)

 
 LinkerScript::AddressState::AddressState(const ScriptConfiguration &Opt) {
   for (auto &MRI : Opt.MemoryRegions) {
     const MemoryRegion *MR = MRI.second;
     MemRegionOffset[MR] = MR->Origin;
   }
 }
 
-void LinkerScript::assignAddresses() {
-  // Assign addresses as instructed by linker script SECTIONS sub-commands.
-  Dot = 0;
+void LinkerScript::assignAddresses() {
+  // By default linker scripts use an initial value of 0 for '.', but prefer
+  // -image-base if set.
+  Dot = Config->ImageBase ? *Config->ImageBase : 0;
   auto State = make_unique<AddressState>(Opt);
   // CurAddressState captures the local AddressState and makes it accessible
   // deliberately. This is needed as there are some cases where we cannot just
   // thread the current state through to a lambda function created by the
   // script parser.
   CurAddressState = State.get();
   ErrorOnMissingSection = true;
   switchTo(Aether);

lld/trunk/ELF/SyntheticSections.cpp

template <class ELFT> void DynamicSection<ELFT>::finalizeContents() {
   if (HasVerNeed) {
     add({DT_VERNEED, In<ELFT>::VerNeed});
     add({DT_VERNEEDNUM, In<ELFT>::VerNeed->getNeedNum()});
   }
 
   if (Config->EMachine == EM_MIPS) {
     add({DT_MIPS_RLD_VERSION, 1});
     add({DT_MIPS_FLAGS, RHF_NOTPOT});
-    add({DT_MIPS_BASE_ADDRESS, Config->ImageBase});
+    add({DT_MIPS_BASE_ADDRESS, Target->getImageBase()});
     add({DT_MIPS_SYMTABNO, InX::DynSymTab->getNumSymbols()});
     add({DT_MIPS_LOCAL_GOTNO, InX::MipsGot->getLocalEntriesNum()});
     if (const SymbolBody *B = InX::MipsGot->getFirstGlobalEntry())
       add({DT_MIPS_GOTSYM, B->DynsymIndex});
     else
       add({DT_MIPS_GOTSYM, InX::DynSymTab->getNumSymbols()});
     add({DT_PLTGOT, InX::MipsGot});
     if (InX::MipsRldMap)

lld/trunk/ELF/Target.h

virtual RelExpr getRelExpr(uint32_t Type, const SymbolBody &S,
                              const uint8_t *Loc) const = 0;
   virtual void relocateOne(uint8_t *Loc, uint32_t Type, uint64_t Val) const = 0;
   virtual ~TargetInfo();
 
   unsigned TlsGdRelaxSkip = 1;
   unsigned PageSize = 4096;
   unsigned DefaultMaxPageSize = 4096;
 
-  // On FreeBSD x86_64 the first page cannot be mmaped.
-  // On Linux that is controled by vm.mmap_min_addr. At least on some x86_64
-  // installs that is 65536, so the first 15 pages cannot be used.
-  // Given that, the smallest value that can be used in here is 0x10000.
-  uint64_t DefaultImageBase = 0x10000;
+  uint64_t getImageBase();
 
   // Offset of _GLOBAL_OFFSET_TABLE_ from base of .got section. Use -1 for
   // end of .got
   uint64_t GotBaseSymOff = 0;
 
   uint32_t CopyRel;
   uint32_t GotRel;
   uint32_t PltRel;
public:
 
   virtual RelExpr adjustRelaxExpr(uint32_t Type, const uint8_t *Data,
                                   RelExpr Expr) const;
   virtual void relaxGot(uint8_t *Loc, uint64_t Val) const;
   virtual void relaxTlsGdToIe(uint8_t *Loc, uint32_t Type, uint64_t Val) const;
   virtual void relaxTlsGdToLe(uint8_t *Loc, uint32_t Type, uint64_t Val) const;
   virtual void relaxTlsIeToLe(uint8_t *Loc, uint32_t Type, uint64_t Val) const;
   virtual void relaxTlsLdToLe(uint8_t *Loc, uint32_t Type, uint64_t Val) const;
+
+protected:
+  // On FreeBSD x86_64 the first page cannot be mmaped.
+  // On Linux that is controled by vm.mmap_min_addr. At least on some x86_64
+  // installs that is 65536, so the first 15 pages cannot be used.
+  // Given that, the smallest value that can be used in here is 0x10000.
+  uint64_t DefaultImageBase = 0x10000;
 };
 
 TargetInfo *getAArch64TargetInfo();
 TargetInfo *getAMDGPUTargetInfo();
 TargetInfo *getARMTargetInfo();
 TargetInfo *getAVRTargetInfo();
 TargetInfo *getPPC64TargetInfo();
 TargetInfo *getPPCTargetInfo();

lld/trunk/ELF/Target.cpp

void TargetInfo::relaxTlsIeToLe(uint8_t *Loc, uint32_t Type,
                                 uint64_t Val) const {
   llvm_unreachable("Should not have claimed to be relaxable");
 }
 
 void TargetInfo::relaxTlsLdToLe(uint8_t *Loc, uint32_t Type,
                                 uint64_t Val) const {
   llvm_unreachable("Should not have claimed to be relaxable");
 }
+
+uint64_t TargetInfo::getImageBase() {
+  // Use -image-base if set. Fall back to the target default if not.
+  if (Config->ImageBase)
+    return *Config->ImageBase;
+  return Config->Pic ? 0 : DefaultImageBase;
+}

lld/trunk/test/ELF/linkerscript/image-base.s

+# REQUIRES: x86
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+# RUN: echo "SECTIONS { mysym = .; }" > %t.script
+
+# RUN: ld.lld %t.o -o %t-default.elf -T %t.script
+# RUN: llvm-readobj --symbols %t-default.elf | FileCheck %s --check-prefix=DEFAULT
+# DEFAULT: Name: mysym
+# DEFAULT-NEXT: Value: 0x0
+
+# RUN: ld.lld %t.o -o %t-switch.elf -T %t.script --image-base=0x100000
+# RUN: llvm-readobj --symbols %t-switch.elf | FileCheck %s --check-prefix=SWITCH
+# SWITCH: Name: mysym
+# SWITCH-NEXT: Value: 0x100000
+
+.global _start
+_start:
+    nop