[ELF] - Linkerscript: simplify access to templated methods from parser.

[grimar]

Summary

Previous way of accessing templated methods was a bit bulky,
I suggest to solve this using small interface class.

[• rafael]

ELF/LinkerScript.h

140 ↗

(On Diff #69243)

This is the same as Script<ELFT>::X, just with a non templated type, no?

Can you rename ScriptMethods to LinkerScriptBase and replace Script<ELFT>::X with a "LinkerScriptBase *Script"?

[ruiu]

As I wrote in the comment to the other patch, I think I still prefer the current way than the way you are suggesting in this patch. The current approach seems silly but simple.

[grimar]

• Rebased.
• Addressed comments partially.

[grimar]

In D23872#529252, @ruiu wrote:

As I wrote in the comment to the other patch, I think I still prefer the current way than the way you are suggesting in this patch. The current approach seems silly but simple.

I see. But there are 4 methods already and probably we`ll need more. Each one use of non-template stuff is an excessive call redirection and always a new method with bunch of code.
Virtual calls for linkerscript tasks costs nothing at the same time.

I updated this patch just in case to show how much code is removed from head revision.

ELF/LinkerScript.h

140 ↗

(On Diff #69243)

Not sure I understand the idea correctly. If I replace usings of Script<ELFT>::X with a "LinkerScriptBase *Script",
I`ll need to add more virtual methods to the LinkerScriptBase. And that is not possible for all of them, because few
are templated:

std::vector<PhdrEntry<ELFT>> createPhdrs();
bool shouldKeep(InputSectionBase<ELFT> *S);

I tried to do:

extern LinkerScriptBase* ScriptBase;
template <class ELFT> LinkerScript<ELFT> *getScript() {
  return static_cast<LinkerScript<ELFT> *>(ScriptBase);
}

Instead of:

template <class ELFT> struct Script { static LinkerScript<ELFT> *X; };
template <class ELFT> LinkerScript<ELFT> *Script<ELFT>::X;

But use of getScript<ELFT() everywhere in the code introduces lots of minor changes,
not sure it looks nicer either.

I ended up with adding single global variable.

[• rafael]

Something like getScript is what I had in mind, but the current solution is OK too.

I like this change, but wait to see if Rui is OK with it too.

ELF/LinkerScript.cpp

629 ↗

(On Diff #69822)

Is this being used?

[ruiu]

I'm trying to not do too much stuff in the "object-oriented" style, but this seems to be undeniably an improvement, so we probably should go with this.

ELF/LinkerScript.cpp

44 ↗

(On Diff #69822)

Format.

ELF/LinkerScript.h

192–193 ↗

(On Diff #69822)

This comment is for struct Script and Script<ELFT>::X, so you shouldn't add the new line below this.

194 ↗

(On Diff #69822)

clang-format.

[grimar]

• Addressed review comments.

[ruiu]

LGTM with this fix.

ELF/LinkerScript.h

205 ↗

(On Diff #70511)

Please run clang-format on this patch. You want to put a space before * instead of after it.

[grimar]

ELF/LinkerScript.h

205 ↗

(On Diff #70511)

Sorry for this.

lld/trunk/ELF/Driver.cpp

 // all linker scripts have already been parsed.
 template <class ELFT> void LinkerDriver::link(opt::InputArgList &Args) {
   SymbolTable<ELFT> Symtab;
   elf::Symtab<ELFT>::X = &Symtab;
 
   std::unique_ptr<TargetInfo> TI(createTarget());
   Target = TI.get();
   LinkerScript<ELFT> LS;
-  Script<ELFT>::X = &LS;
+  ScriptBase = Script<ELFT>::X = &LS;
 
   Config->Rela = ELFT::Is64Bits || Config->EMachine == EM_X86_64;
   Config->Mips64EL =
       (Config->EMachine == EM_MIPS && Config->EKind == ELF64LEKind);
 
   // Add entry symbol. Note that AMDGPU binaries have no entry points.
   if (Config->Entry.empty() && !Config->Shared && !Config->Relocatable &&
       Config->EMachine != EM_AMDGPU)

lld/trunk/ELF/LinkerScript.h

 struct PhdrsCommand {
   StringRef Name;
   unsigned Type;
   bool HasFilehdr;
   bool HasPhdrs;
   unsigned Flags;
 };
 
+class LinkerScriptBase {
+public:
+  virtual uint64_t getOutputSectionAddress(StringRef Name) = 0;
+  virtual uint64_t getOutputSectionSize(StringRef Name) = 0;
+  virtual uint64_t getHeaderSize() = 0;
+  virtual uint64_t getSymbolValue(StringRef S) = 0;
+};
+
 // ScriptConfiguration holds linker script parse results.
 struct ScriptConfiguration {
   // Used to create symbol assignments outside SECTIONS command.
   std::vector<std::unique_ptr<SymbolAssignment>> Assignments;
   // Used to assign addresses to sections.
   std::vector<std::unique_ptr<BaseCommand>> Commands;
 
   // Used to assign sections to headers.
   std::vector<PhdrsCommand> PhdrsCommands;
 
   bool HasContents = false;
 
   llvm::BumpPtrAllocator Alloc;
 
   // List of section patterns specified with KEEP commands. They will
   // be kept even if they are unused and --gc-sections is specified.
   std::vector<llvm::Regex *> KeptSections;
 };
 
 extern ScriptConfiguration *ScriptConfig;
 
 // This is a runner of the linker script.
-template <class ELFT> class LinkerScript {
+template <class ELFT> class LinkerScript final : public LinkerScriptBase {
   typedef typename ELFT::uint uintX_t;
 
 public:
   LinkerScript();
   ~LinkerScript();
   void createAssignments();
   void createSections(OutputSectionFactory<ELFT> &Factory);
 
   std::vector<PhdrEntry<ELFT>> createPhdrs();
   bool ignoreInterpSection();
 
   ArrayRef<uint8_t> getFiller(StringRef Name);
   Expr getLma(StringRef Name);
   bool shouldKeep(InputSectionBase<ELFT> *S);
   void assignAddresses();
   int compareSections(StringRef A, StringRef B);
   bool hasPhdrsCommands();
-  uintX_t getOutputSectionAddress(StringRef Name);
-  uintX_t getOutputSectionSize(StringRef Name);
-  uintX_t getHeaderSize();
+  uint64_t getOutputSectionAddress(StringRef Name) override;
+  uint64_t getOutputSectionSize(StringRef Name) override;
+  uint64_t getHeaderSize() override;
+  uint64_t getSymbolValue(StringRef S) override;
 
   std::vector<OutputSectionBase<ELFT> *> *OutputSections;
 
 private:
   std::vector<InputSectionBase<ELFT> *>
   getInputSections(const InputSectionDescription *);
 
   void discard(OutputSectionCommand &Cmd);
private:
   uintX_t Dot;
 };
 
 // Variable template is a C++14 feature, so we can't template
 // a global variable. Use a struct to workaround.
 template <class ELFT> struct Script { static LinkerScript<ELFT> *X; };
 template <class ELFT> LinkerScript<ELFT> *Script<ELFT>::X;
 
+extern LinkerScriptBase *ScriptBase;
+
 } // namespace elf
 } // namespace lld
 
 #endif

lld/trunk/ELF/LinkerScript.cpp

 #include "llvm/Support/StringSaver.h"
 
 using namespace llvm;
 using namespace llvm::ELF;
 using namespace llvm::object;
 using namespace lld;
 using namespace lld::elf;
 
+LinkerScriptBase *elf::ScriptBase;
 ScriptConfiguration *elf::ScriptConfig;
 
 template <class ELFT>
 static void addRegular(SymbolAssignment *Cmd) {
   Symbol *Sym = Symtab<ELFT>::X->addRegular(Cmd->Name, STB_GLOBAL, STV_DEFAULT);
   Sym->Visibility = Cmd->Hidden ? STV_HIDDEN : STV_DEFAULT;
   Cmd->Sym = Sym->body();
 }
int LinkerScript<ELFT>::compareSections(StringRef A, StringRef B) {
   return I < J ? -1 : 1;
 }
 
 template <class ELFT> bool LinkerScript<ELFT>::hasPhdrsCommands() {
   return !Opt.PhdrsCommands.empty();
 }
 
 template <class ELFT>
-typename ELFT::uint
-LinkerScript<ELFT>::getOutputSectionAddress(StringRef Name) {
+uint64_t LinkerScript<ELFT>::getOutputSectionAddress(StringRef Name) {
   for (OutputSectionBase<ELFT> *Sec : *OutputSections)
     if (Sec->getName() == Name)
       return Sec->getVA();
   error("undefined section " + Name);
   return 0;
 }
 
 template <class ELFT>
-typename ELFT::uint LinkerScript<ELFT>::getOutputSectionSize(StringRef Name) {
+uint64_t LinkerScript<ELFT>::getOutputSectionSize(StringRef Name) {
   for (OutputSectionBase<ELFT> *Sec : *OutputSections)
     if (Sec->getName() == Name)
       return Sec->getSize();
   error("undefined section " + Name);
   return 0;
 }
 
-template <class ELFT>
-typename ELFT::uint LinkerScript<ELFT>::getHeaderSize() {
+template <class ELFT> uint64_t LinkerScript<ELFT>::getHeaderSize() {
   return Out<ELFT>::ElfHeader->getSize() + Out<ELFT>::ProgramHeaders->getSize();
 }
 
+template <class ELFT> uint64_t LinkerScript<ELFT>::getSymbolValue(StringRef S) {
+  if (SymbolBody *B = Symtab<ELFT>::X->find(S))
+    return B->getVA<ELFT>();
+  error("symbol not found: " + S);
+  return 0;
+}
+
 // Returns indices of ELF headers containing specific section, identified
 // by Name. Each index is a zero based number of ELF header listed within
 // PHDRS {} script block.
 template <class ELFT>
 std::vector<size_t> LinkerScript<ELFT>::getPhdrIndices(StringRef SectionName) {
   for (const std::unique_ptr<BaseCommand> &Base : Opt.Commands) {
     auto *Cmd = dyn_cast<OutputSectionCommand>(Base.get());
     if (!Cmd || Cmd->Name != SectionName)
SymbolAssignment *ScriptParser::readProvideOrAssignment(StringRef Tok,
   if (Cmd && MakeAbsolute)
     Cmd->IsAbsolute = true;
   return Cmd;
 }
 
 static uint64_t getSymbolValue(StringRef S, uint64_t Dot) {
   if (S == ".")
     return Dot;
-
-  switch (Config->EKind) {
-  case ELF32LEKind:
-    if (SymbolBody *B = Symtab<ELF32LE>::X->find(S))
-      return B->getVA<ELF32LE>();
-    break;
-  case ELF32BEKind:
-    if (SymbolBody *B = Symtab<ELF32BE>::X->find(S))
-      return B->getVA<ELF32BE>();
-    break;
-  case ELF64LEKind:
-    if (SymbolBody *B = Symtab<ELF64LE>::X->find(S))
-      return B->getVA<ELF64LE>();
-    break;
-  case ELF64BEKind:
-    if (SymbolBody *B = Symtab<ELF64BE>::X->find(S))
-      return B->getVA<ELF64BE>();
-    break;
-  default:
-    llvm_unreachable("unsupported target");
-  }
-  error("symbol not found: " + S);
-  return 0;
-}
-
-static uint64_t getSectionSize(StringRef Name) {
-  switch (Config->EKind) {
-  case ELF32LEKind:
-    return Script<ELF32LE>::X->getOutputSectionSize(Name);
-  case ELF32BEKind:
-    return Script<ELF32BE>::X->getOutputSectionSize(Name);
-  case ELF64LEKind:
-    return Script<ELF64LE>::X->getOutputSectionSize(Name);
-  case ELF64BEKind:
-    return Script<ELF64BE>::X->getOutputSectionSize(Name);
-  default:
-    llvm_unreachable("unsupported target");
-  }
-}
-
-static uint64_t getSectionAddress(StringRef Name) {
-  switch (Config->EKind) {
-  case ELF32LEKind:
-    return Script<ELF32LE>::X->getOutputSectionAddress(Name);
-  case ELF32BEKind:
-    return Script<ELF32BE>::X->getOutputSectionAddress(Name);
-  case ELF64LEKind:
-    return Script<ELF64LE>::X->getOutputSectionAddress(Name);
-  case ELF64BEKind:
-    return Script<ELF64BE>::X->getOutputSectionAddress(Name);
-  default:
-    llvm_unreachable("unsupported target");
-  }
-}
-
-static uint64_t getHeaderSize() {
-  switch (Config->EKind) {
-  case ELF32LEKind:
-    return Script<ELF32LE>::X->getHeaderSize();
-  case ELF32BEKind:
-    return Script<ELF32BE>::X->getHeaderSize();
-  case ELF64LEKind:
-    return Script<ELF64LE>::X->getHeaderSize();
-  case ELF64BEKind:
-    return Script<ELF64BE>::X->getHeaderSize();
-  default:
-    llvm_unreachable("unsupported target");
-  }
+  return ScriptBase->getSymbolValue(S);
 }
 
 SymbolAssignment *ScriptParser::readAssignment(StringRef Name) {
   StringRef Op = next();
   bool IsAbsolute = false;
   Expr E;
   assert(Op == "=" || Op == "+=");
   if (skip("ABSOLUTE")) {
Expr ScriptParser::readPrimary() {
   }
 
   // Built-in functions are parsed here.
   // https://sourceware.org/binutils/docs/ld/Builtin-Functions.html.
   if (Tok == "ADDR") {
     expect("(");
     StringRef Name = next();
     expect(")");
-    return [=](uint64_t Dot) { return getSectionAddress(Name); };
+    return
+        [=](uint64_t Dot) { return ScriptBase->getOutputSectionAddress(Name); };
   }
   if (Tok == "ASSERT")
     return readAssert();
   if (Tok == "ALIGN") {
     Expr E = readParenExpr();
     return [=](uint64_t Dot) { return alignTo(Dot, E(Dot)); };
   }
   if (Tok == "CONSTANT") {
if (Tok == "DATA_SEGMENT_RELRO_END") {
     readExpr();
     expect(")");
     return [](uint64_t Dot) { return alignTo(Dot, Target->PageSize); };
   }
   if (Tok == "SIZEOF") {
     expect("(");
     StringRef Name = next();
     expect(")");
-    return [=](uint64_t Dot) { return getSectionSize(Name); };
+    return [=](uint64_t Dot) { return ScriptBase->getOutputSectionSize(Name); };
   }
   if (Tok == "SIZEOF_HEADERS")
-    return [=](uint64_t Dot) { return getHeaderSize(); };
+    return [=](uint64_t Dot) { return ScriptBase->getHeaderSize(); };
 
   // Tok is a literal number.
   uint64_t V;
   if (readInteger(Tok, V))
     return [=](uint64_t Dot) { return V; };
 
   // Tok is a symbol name.
   if (Tok != "." && !isValidCIdentifier(Tok))