[ELF] - Refactoring of end/edata/etext implementation.

[grimar]

Summary

Just minor refactoring of how end/edata/etext symbols are handled.

[• rafael]

LGTM with nits.

ELF/Symbols.h

393 ↗

(On Diff #53696)

SymPair maybe?

ELF/Writer.cpp

1665 ↗

(On Diff #53696)

This can be a static function, no need to make it a lambda.

[grimar]

ELF/Symbols.h

393 ↗

(On Diff #53696)

Done.

ELF/Writer.cpp

1665 ↗

(On Diff #53696)

Done.

[ruiu]

I probably wouldn't do that because it doesn't seem to be worth to create a new type. If you don't like the repetition of if (...) ... = Val;, then how about defining a helper lambda for assignment like this?

auto Set = [&](DefinedRegular<ELFT> *S, uint64_t V) { If (S) S->Value = V; }

[grimar]

In D19109#401395, @ruiu wrote:

I probably wouldn't do that because it doesn't seem to be worth to create a new type. If you don't like the repetition of if (...) ... = Val;, then how about defining a helper lambda for assignment like this?

auto Set = [&](DefinedRegular<ELFT> *S, uint64_t V) { If (S) S->Value = V; }

I would prefer mine version, I dont like not only repetitions of assignments, but also additional variables declarations.
And that's not real type, just typedef for pair, so I dont find this to excessive.

But returning to subject - since that was already committed, do you think this should be reverted ?

lld/trunk/ELF/Symbols.h

 
 private:
   MemoryBufferRef MBRef;
 };
 
 // Some linker-generated symbols need to be created as
 // DefinedRegular symbols.
 template <class ELFT> struct ElfSym {
+  typedef std::pair<DefinedRegular<ELFT> *, DefinedRegular<ELFT> *> SymPair;
+
   // The content for _etext and etext symbols.
-  static DefinedRegular<ELFT> *Etext;
-  static DefinedRegular<ELFT> *Etext2;
+  static SymPair Etext;
 
   // The content for _edata and edata symbols.
-  static DefinedRegular<ELFT> *Edata;
-  static DefinedRegular<ELFT> *Edata2;
+  static SymPair Edata;
 
   // The content for _end and end symbols.
-  static DefinedRegular<ELFT> *End;
-  static DefinedRegular<ELFT> *End2;
+  static SymPair End;
 
   // The content for _gp symbol for MIPS target.
   static SymbolBody *MipsGp;
 
   static SymbolBody *MipsLocalGp;
   static SymbolBody *MipsGpDisp;
 
   // __rel_iplt_start/__rel_iplt_end for signaling
   // where R_[*]_IRELATIVE relocations do live.
   static SymbolBody *RelaIpltStart;
   static SymbolBody *RelaIpltEnd;
 };
 
-template <class ELFT> DefinedRegular<ELFT> *ElfSym<ELFT>::Etext;
-template <class ELFT> DefinedRegular<ELFT> *ElfSym<ELFT>::Etext2;
-template <class ELFT> DefinedRegular<ELFT> *ElfSym<ELFT>::Edata;
-template <class ELFT> DefinedRegular<ELFT> *ElfSym<ELFT>::Edata2;
-template <class ELFT> DefinedRegular<ELFT> *ElfSym<ELFT>::End;
-template <class ELFT> DefinedRegular<ELFT> *ElfSym<ELFT>::End2;
+template <class ELFT> typename ElfSym<ELFT>::SymPair ElfSym<ELFT>::Etext;
+template <class ELFT> typename ElfSym<ELFT>::SymPair ElfSym<ELFT>::Edata;
+template <class ELFT> typename ElfSym<ELFT>::SymPair ElfSym<ELFT>::End;
+
 template <class ELFT> SymbolBody *ElfSym<ELFT>::MipsGp;
 template <class ELFT> SymbolBody *ElfSym<ELFT>::MipsLocalGp;
 template <class ELFT> SymbolBody *ElfSym<ELFT>::MipsGpDisp;
 template <class ELFT> SymbolBody *ElfSym<ELFT>::RelaIpltStart;
 template <class ELFT> SymbolBody *ElfSym<ELFT>::RelaIpltEnd;
 
 } // namespace elf
 } // namespace lld
 
 #endif

lld/trunk/ELF/Writer.cpp

template <class ELFT> void Writer<ELFT>::addReservedSymbols() {
 
   // __tls_get_addr is defined by the dynamic linker for dynamic ELFs. For
   // static linking the linker is required to optimize away any references to
   // __tls_get_addr, so it's not defined anywhere. Create a hidden definition
   // to avoid the undefined symbol error.
   if (!isOutputDynamic())
     Symtab.addIgnored("__tls_get_addr");
 
-  auto Define = [this](StringRef S, DefinedRegular<ELFT> *&Sym,
-                       DefinedRegular<ELFT> *&Sym2) {
-    Sym = Symtab.addIgnored(S, STV_DEFAULT);
+  auto Define = [this](StringRef S, ElfSym<ELFT>::SymPair &Sym) {
+    Sym.first = Symtab.addIgnored(S, STV_DEFAULT);
 
     // The name without the underscore is not a reserved name,
     // so it is defined only when there is a reference against it.
     assert(S.startswith("_"));
     S = S.substr(1);
     if (SymbolBody *B = Symtab.find(S))
       if (B->isUndefined())
-        Sym2 = Symtab.addAbsolute(S, STV_DEFAULT);
+        Sym.second = Symtab.addAbsolute(S, STV_DEFAULT);
   };
 
-  Define("_end", ElfSym<ELFT>::End, ElfSym<ELFT>::End2);
-  Define("_etext", ElfSym<ELFT>::Etext, ElfSym<ELFT>::Etext2);
-  Define("_edata", ElfSym<ELFT>::Edata, ElfSym<ELFT>::Edata2);
+  Define("_end", ElfSym<ELFT>::End);
+  Define("_etext", ElfSym<ELFT>::Etext);
+  Define("_edata", ElfSym<ELFT>::Edata);
 }
 
 // Sort input sections by section name suffixes for
 // __attribute__((init_priority(N))).
 template <class ELFT> static void sortInitFini(OutputSectionBase<ELFT> *S) {
   if (S)
     reinterpret_cast<OutputSection<ELFT> *>(S)->sortInitFini();
 }

 static uint16_t getELFType() {
   if (Config->Pic)
     return ET_DYN;
   if (Config->Relocatable)
     return ET_REL;
   return ET_EXEC;
 }
 
+template <class ELFT, class SymPair, class uintX_t>
+static void assignSymValue(SymPair &Sym, uintX_t Val) {
+  if (Sym.first)
+    Sym.first->Value = Val;
+  if (Sym.second)
+    Sym.second->Value = Val;
+}
+
 // This function is called after we have assigned address and size
 // to each section. This function fixes some predefined absolute
 // symbol values that depend on section address and size.
 template <class ELFT> void Writer<ELFT>::fixAbsoluteSymbols() {
   // _etext is the first location after the last read-only loadable segment.
   // _edata is the first location after the last read-write loadable segment.
   // _end is the first location after the uninitialized data region.
   for (Phdr &P : Phdrs) {
     Elf_Phdr &H = P.H;
     if (H.p_type != PT_LOAD)
       continue;
-    uintX_t Val = H.p_vaddr + H.p_memsz;
-    if (ElfSym<ELFT>::End)
-      ElfSym<ELFT>::End->Value = Val;
-    if (ElfSym<ELFT>::End2)
-      ElfSym<ELFT>::End2->Value = Val;
-
-    Val = H.p_vaddr + H.p_filesz;
-    if (H.p_flags & PF_W) {
-      if (ElfSym<ELFT>::Edata)
-        ElfSym<ELFT>::Edata->Value = Val;
-      if (ElfSym<ELFT>::Edata2)
-        ElfSym<ELFT>::Edata2->Value = Val;
-    } else {
-      if (ElfSym<ELFT>::Etext)
-        ElfSym<ELFT>::Etext->Value = Val;
-      if (ElfSym<ELFT>::Etext2)
-        ElfSym<ELFT>::Etext2->Value = Val;
-    }
+    assignSymValue<ELFT>(ElfSym<ELFT>::End, H.p_vaddr + H.p_memsz);
+
+    uintX_t Val = H.p_vaddr + H.p_filesz;
+    if (H.p_flags & PF_W)
+      assignSymValue<ELFT>(ElfSym<ELFT>::Edata, Val);
+    else
+      assignSymValue<ELFT>(ElfSym<ELFT>::Etext, Val);
   }
 }
 
 template <class ELFT> void Writer<ELFT>::writeHeader() {
   uint8_t *Buf = Buffer->getBufferStart();
   memcpy(Buf, "\177ELF", 4);
 
   auto &FirstObj = cast<ELFFileBase<ELFT>>(*Config->FirstElf);