[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

170

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

[ruiu]

ELF/Driver.cpp

918–919

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

169–170

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

It is common to

return None;

[jhenderson]

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

[ruiu]

ELF/LinkerScript.cpp

349

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

355

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

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

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->.

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
   // particualr command line options.

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)

ELF/LinkerScript.h

 
 public:
   bool ErrorOnMissingSection = false;
   OutputSection *createOutputSection(StringRef Name, StringRef Location);
   OutputSection *getOrCreateOutputSection(StringRef Name);
 
   bool hasPhdrsCommands() { return !Opt.PhdrsCommands.empty(); }
   uint64_t getDot() { return Dot; }
+  uint64_t getImageBase();
   void discard(ArrayRef<InputSectionBase *> V);
 
   ExprValue getSymbolValue(const Twine &Loc, StringRef S);
   bool isDefined(StringRef S);
 
   void fabricateDefaultCommands();
   void addOrphanSections(OutputSectionFactory &Factory);
   void removeEmptyCommands();

ELF/LinkerScript.cpp

for (BaseCommand *Base : OutCmd.Commands) {
 
     Cmd->Sections = computeInputSections(Cmd);
     Ret.insert(Ret.end(), Cmd->Sections.begin(), Cmd->Sections.end());
   }
 
   return Ret;
 }
 
+static uint64_t getInitialDot() {
+  // By default linker scripts use an initial value of 0 for '.', but prefer
+  // -image-base if set.
+  return Config->ImageBase ? *Config->ImageBase : 0;
+}
+
+uint64_t LinkerScript::getImageBase() {
+  // Use -image-base if set. Fall back to the target default if not.
+  if (Config->ImageBase)
+    return *Config->ImageBase;
+  return Config->Pic ? 0 : Target->DefaultImageBase;
+}
+
 void LinkerScript::processCommands(OutputSectionFactory &Factory) {
   // A symbol can be assigned before any section is mentioned in the linker
   // script. In an DSO, the symbol values are addresses, so the only important
   // section values are:
   // * SHN_UNDEF
   // * SHN_ABS
   // * Any value meaning a regular section.
   // To handle that, create a dummy aether section that fills the void before
   // the linker scripts switches to another section. It has an index of one
   // which will map to whatever the first actual section is.
   Aether = make<OutputSection>("", 0, SHF_ALLOC);
   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;
+  Dot = getInitialDot();
 
   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, getImageBase() + elf::getHeaderSize());
   };
   Opt.Commands.insert(Opt.Commands.begin(),
                       make<SymbolAssignment>(".", Expr, ""));
 }
 
 // Add sections that didn't match any sections command.
 void LinkerScript::addOrphanSections(OutputSectionFactory &Factory) {
   unsigned NumCommands = Opt.Commands.size();
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;
+  Dot = getInitialDot();
   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);

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, Script->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)

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