[ELF] Generalize ELF symbol type handling

[davide]

Summary

SymbolBody constructor and friends take isFunc and isTLS boolean arguments. ELF symbols have already a type so than be easily passed down. This makes code a little bit more readable, I think.
The next patch in this series will remove isTLS.

[davide]

Note (to future self and others): this minor yak was motivated by a bug I found.
Some symbols are not emitted in the symbol table with the appropriate type, e.g. STT_OBJECT.
The current code logic would require passing another bool isObject around. Up to two additional argument, this stretching exercise was a little bit goofy but still acceptable, but with more types to support, is just too much, IMHO.

[ruiu]

This looks good to me, but do you want to do isTLS in the same patch? This patch itself does not reduce the number of parameters, so I think this patch more sense if you do both at once.

[davide]

In D17871#367706, @ruiu wrote:

This looks good to me, but do you want to do isTLS in the same patch? This patch itself does not reduce the number of parameters, so I think this patch more sense if you do both at once.

Sure, why not.

[davide]

Addressed Rui's comments. Removed isTls sooner rather than later.

[ruiu]

LGTM

[grimar]

Hi Davide,

Change itself looks good for me, but I have few thoughts about possible implementation and prepared a little patch here: http://reviews.llvm.org/D17879.

lld/trunk/ELF/InputSection.cpp

for (size_t I = 0; I < Num; ++I) {
     } else if (Target->needsGot(Type, *Body)) {
       if (Config->EMachine == EM_MIPS && !CBP)
         // Under some conditions relocations against non-local symbols require
         // entries in the local part of MIPS GOT. In that case we need an entry
         // initialized by full address of the symbol.
         SymVA = Out<ELFT>::Got->getMipsLocalFullAddr(*Body);
       else
         SymVA = Body->getGotVA<ELFT>();
-      if (Body->IsTls)
+      if (Body->isTls())
         Type = Target->getTlsGotRel(Type);
     } else if (!Target->needsCopyRel<ELFT>(Type, *Body) &&
                isa<SharedSymbol<ELFT>>(*Body)) {
       continue;
     } else if (Target->isTlsDynRel(Type, *Body)) {
       continue;
     } else if (Target->isSizeRel(Type) && CBP) {
       // A SIZE relocation is supposed to set a symbol size, but if a symbol

lld/trunk/ELF/SymbolTable.cpp

if (auto *Undef = dyn_cast<Undefined>(New)) {
       return;
     }
     // Found a definition for something also in an archive.
     // Ignore the archive definition.
     Sym->Body = New;
     return;
   }
 
-  if (New->IsTls != Existing->IsTls) {
+  if (New->isTls() != Existing->isTls()) {
     error("TLS attribute mismatch for symbol: " + conflictMsg(Existing, New));
     return;
   }
 
   // compare() returns -1, 0, or 1 if the lhs symbol is less preferable,
   // equivalent (conflicting), or more preferable, respectively.
   int Comp = Existing->compare<ELFT>(New);
   if (Comp == 0) {
void SymbolTable<ELFT>::addMemberFile(Undefined *Undef, Lazy *L) {
   // get to the end of the link and must be treated as the weak undefined one.
   // We set UsedInRegularObj in a similar way to what is done with shared
   // symbols and copy information to reduce how many special cases are needed.
   if (Undef->isWeak()) {
     L->setUsedInRegularObj();
     L->setWeak();
 
     // FIXME: Do we need to copy more?
-    L->IsTls = Undef->IsTls;
+    if (Undef->isTls())
+      L->setTls();
     return;
   }
 
   // Fetch a member file that has the definition for L.
   // getMember returns nullptr if the member was already read from the library.
   if (std::unique_ptr<InputFile> File = L->getMember())
     addFile(std::move(File));
 }

lld/trunk/ELF/Symbols.h

public:
   bool isUndefined() const {
     return SymbolKind == UndefinedKind || SymbolKind == UndefinedElfKind;
   }
   bool isDefined() const { return SymbolKind <= DefinedLast; }
   bool isCommon() const { return SymbolKind == DefinedCommonKind; }
   bool isLazy() const { return SymbolKind == LazyKind; }
   bool isShared() const { return SymbolKind == SharedKind; }
   bool isUsedInRegularObj() const { return IsUsedInRegularObj; }
-  bool isFunc() const { return IsFunc; }
+  bool isFunc() const { return Type == llvm::ELF::STT_FUNC; }
+  bool isTls() const { return Type == llvm::ELF::STT_TLS; }
 
   // Returns the symbol name.
   StringRef getName() const { return Name; }
 
   uint8_t getVisibility() const { return Visibility; }
 
   unsigned DynsymIndex = 0;
   uint32_t GlobalDynIndex = -1;
public:
 
   // Decides which symbol should "win" in the symbol table, this or
   // the Other. Returns 1 if this wins, -1 if the Other wins, or 0 if
   // they are duplicate (conflicting) symbols.
   template <class ELFT> int compare(SymbolBody *Other);
 
 protected:
   SymbolBody(Kind K, StringRef Name, bool IsWeak, uint8_t Visibility,
-             bool IsTls, bool IsFunc)
+             uint8_t Type)
       : SymbolKind(K), IsWeak(IsWeak), Visibility(Visibility),
-        MustBeInDynSym(false), NeedsCopyOrPltAddr(false), IsTls(IsTls),
-        IsFunc(IsFunc), Name(Name) {
+        MustBeInDynSym(false), NeedsCopyOrPltAddr(false),
+        Type(Type), Name(Name) {
     IsUsedInRegularObj = K != SharedKind && K != LazyKind;
   }
 
   const unsigned SymbolKind : 8;
   unsigned IsWeak : 1;
   unsigned Visibility : 2;
 
   // True if the symbol was used for linking and thus need to be
   // added to the output file's symbol table. It is usually true,
   // but if it is a shared symbol that were not referenced by anyone,
   // it can be false.
   unsigned IsUsedInRegularObj : 1;
 
 public:
   // If true, the symbol is added to .dynsym symbol table.
   unsigned MustBeInDynSym : 1;
 
   // True if the linker has to generate a copy relocation for this shared
   // symbol or if the symbol should point to its plt entry.
   unsigned NeedsCopyOrPltAddr : 1;
 
-  unsigned IsTls : 1;
-
 protected:
-  unsigned IsFunc : 1;
+  uint8_t Type;
 
   StringRef Name;
   Symbol *Backref = nullptr;
 };
 
 // The base class for any defined symbols.
 class Defined : public SymbolBody {
 public:
-  Defined(Kind K, StringRef Name, bool IsWeak, uint8_t Visibility, bool IsTls,
-          bool IsFunction);
+  Defined(Kind K, StringRef Name, bool IsWeak, uint8_t Visibility,
+          uint8_t Type);
   static bool classof(const SymbolBody *S) { return S->isDefined(); }
 };
 
 // Any defined symbol from an ELF file.
 template <class ELFT> class DefinedElf : public Defined {
 protected:
   typedef typename llvm::object::ELFFile<ELFT>::Elf_Sym Elf_Sym;
 
 public:
   DefinedElf(Kind K, StringRef N, const Elf_Sym &Sym)
       : Defined(K, N, Sym.getBinding() == llvm::ELF::STB_WEAK,
-                Sym.getVisibility(), Sym.getType() == llvm::ELF::STT_TLS,
-                Sym.getType() == llvm::ELF::STT_FUNC),
+                Sym.getVisibility(), Sym.getType()),
         Sym(Sym) {}
 
   const Elf_Sym &Sym;
   static bool classof(const SymbolBody *S) {
     return S->kind() <= DefinedElfLast;
   }
 };
 

 };
 
 // Undefined symbol.
 class Undefined : public SymbolBody {
   typedef SymbolBody::Kind Kind;
   bool CanKeepUndefined;
 
 protected:
-  Undefined(Kind K, StringRef N, bool IsWeak, uint8_t Visibility, bool IsTls);
+  Undefined(Kind K, StringRef N, bool IsWeak, uint8_t Visibility, uint8_t Type);
 
 public:
   Undefined(StringRef N, bool IsWeak, uint8_t Visibility,
             bool CanKeepUndefined);
 
   static bool classof(const SymbolBody *S) { return S->isUndefined(); }
 
   bool canKeepUndefined() const { return CanKeepUndefined; }
public:
   SharedSymbol(SharedFile<ELFT> *F, StringRef Name, const Elf_Sym &Sym)
       : DefinedElf<ELFT>(SymbolBody::SharedKind, Name, Sym), File(F) {}
 
   SharedFile<ELFT> *File;
 
   // OffsetInBss is significant only when needsCopy() is true.
   uintX_t OffsetInBss = 0;
 
-  bool needsCopy() const { return this->NeedsCopyOrPltAddr && !this->IsFunc; }
+  bool needsCopy() const { return this->NeedsCopyOrPltAddr && !this->isFunc(); }
 };
 
 // This class represents a symbol defined in an archive file. It is
 // created from an archive file header, and it knows how to load an
 // object file from an archive to replace itself with a defined
 // symbol. If the resolver finds both Undefined and Lazy for
 // the same name, it will ask the Lazy to load a file.
 class Lazy : public SymbolBody {
 public:
   Lazy(ArchiveFile *F, const llvm::object::Archive::Symbol S)
       : SymbolBody(LazyKind, S.getName(), false, llvm::ELF::STV_DEFAULT,
-                   /*IsTls*/ false, /*IsFunction*/ false),
+                   /* Type */ 0),
         File(F), Sym(S) {}
 
   static bool classof(const SymbolBody *S) { return S->kind() == LazyKind; }
 
   // Returns an object file for this symbol, or a nullptr if the file
   // was already returned.
   std::unique_ptr<InputFile> getMember();
 
+  void setTls() { this->Type = llvm::ELF::STT_TLS; }
   void setWeak() { IsWeak = true; }
   void setUsedInRegularObj() { IsUsedInRegularObj = true; }
 
 private:
   ArchiveFile *File;
   const llvm::object::Archive::Symbol Sym;
 };
 

lld/trunk/ELF/Symbols.cpp

case DefinedRegularKind: {
     return SC->OutSec->getVA() + SC->getOffset(D->Sym);
   }
   case DefinedCommonKind:
     return Out<ELFT>::Bss->getVA() + cast<DefinedCommon>(this)->OffsetInBss;
   case SharedKind: {
     auto *SS = cast<SharedSymbol<ELFT>>(this);
     if (!SS->NeedsCopyOrPltAddr)
       return 0;
-    if (SS->IsFunc)
+    if (SS->isFunc())
       return getPltVA<ELFT>();
     else
       return Out<ELFT>::Bss->getVA() + SS->OffsetInBss;
   }
   case UndefinedElfKind:
   case UndefinedKind:
     return 0;
   case LazyKind:
if (isCommon()) {
     return -1;
   }
   if (Other->isCommon())
     return 1;
   return 0;
 }
 
 Defined::Defined(Kind K, StringRef Name, bool IsWeak, uint8_t Visibility,
-                 bool IsTls, bool IsFunction)
-    : SymbolBody(K, Name, IsWeak, Visibility, IsTls, IsFunction) {}
+                 uint8_t Type)
+    : SymbolBody(K, Name, IsWeak, Visibility, Type) {}
 
 DefinedBitcode::DefinedBitcode(StringRef Name, bool IsWeak)
-    : Defined(DefinedBitcodeKind, Name, IsWeak, STV_DEFAULT, false, false) {}
+    : Defined(DefinedBitcodeKind, Name, IsWeak, STV_DEFAULT, 0 /* Type */) {}
 
 bool DefinedBitcode::classof(const SymbolBody *S) {
   return S->kind() == DefinedBitcodeKind;
 }
 
 Undefined::Undefined(SymbolBody::Kind K, StringRef N, bool IsWeak,
-                     uint8_t Visibility, bool IsTls)
-    : SymbolBody(K, N, IsWeak, Visibility, IsTls, /*IsFunction*/ false),
+                     uint8_t Visibility, uint8_t Type)
+    : SymbolBody(K, N, IsWeak, Visibility, Type),
       CanKeepUndefined(false) {}
 
 Undefined::Undefined(StringRef N, bool IsWeak, uint8_t Visibility,
                      bool CanKeepUndefined)
-    : Undefined(SymbolBody::UndefinedKind, N, IsWeak, Visibility,
-                /*IsTls*/ false) {
+    : Undefined(SymbolBody::UndefinedKind, N, IsWeak, Visibility, 0 /* Type */) {
   this->CanKeepUndefined = CanKeepUndefined;
 }
 
 template <typename ELFT>
 UndefinedElf<ELFT>::UndefinedElf(StringRef N, const Elf_Sym &Sym)
     : Undefined(SymbolBody::UndefinedElfKind, N,
                 Sym.getBinding() == llvm::ELF::STB_WEAK, Sym.getVisibility(),
-                Sym.getType() == llvm::ELF::STT_TLS),
+                Sym.getType()),
       Sym(Sym) {}
 
 template <typename ELFT>
 DefinedSynthetic<ELFT>::DefinedSynthetic(StringRef N, uintX_t Value,
                                          OutputSectionBase<ELFT> &Section,
                                          uint8_t Visibility)
     : Defined(SymbolBody::DefinedSyntheticKind, N, false, Visibility,
-              /*IsTls*/ false, /*IsFunction*/ false),
+              0 /* Type */),
       Value(Value), Section(Section) {}
 
 DefinedCommon::DefinedCommon(StringRef N, uint64_t Size, uint64_t Alignment,
                              bool IsWeak, uint8_t Visibility)
     : Defined(SymbolBody::DefinedCommonKind, N, IsWeak, Visibility,
-              /*IsTls*/ false, /*IsFunction*/ false) {
+              0 /* Type */) {
   MaxAlignment = Alignment;
   this->Size = Size;
 }
 
 std::unique_ptr<InputFile> Lazy::getMember() {
   MemoryBufferRef MBRef = File->getMember(&Sym);
 
   // getMember returns an empty buffer if the member was already

lld/trunk/ELF/Target.cpp

void X86TargetInfo::writePlt(uint8_t *Buf, uint64_t GotEntryAddr,
   write32le(Buf + 12, -Index * PltEntrySize - PltZeroSize - 16);
 }
 
 bool X86TargetInfo::needsCopyRelImpl(uint32_t Type) const {
   return Type == R_386_32 || Type == R_386_16 || Type == R_386_8;
 }
 
 bool X86TargetInfo::needsGot(uint32_t Type, SymbolBody &S) const {
-  if (S.IsTls && Type == R_386_TLS_GD)
+  if (S.isTls() && Type == R_386_TLS_GD)
     return Target->canRelaxTls(Type, &S) && canBePreempted(&S);
   if (Type == R_386_TLS_GOTIE || Type == R_386_TLS_IE)
     return !canRelaxTls(Type, &S);
   return Type == R_386_GOT32 || needsPlt<ELF32LE>(Type, S);
 }
 
 bool X86TargetInfo::needsPltImpl(uint32_t Type) const {
   return Type == R_386_PLT32;
case R_386_TLS_LE_32:
     write32le(Loc, Out<ELF32LE>::TlsPhdr->p_memsz - SA);
     break;
   default:
     fatal("unrecognized reloc " + Twine(Type));
   }
 }
 
 bool X86TargetInfo::canRelaxTls(unsigned Type, const SymbolBody *S) const {
-  if (Config->Shared || (S && !S->IsTls))
+  if (Config->Shared || (S && !S->isTls()))
     return false;
   return Type == R_386_TLS_LDO_32 || Type == R_386_TLS_LDM ||
          Type == R_386_TLS_GD || (Type == R_386_TLS_IE && !canBePreempted(S)) ||
          (Type == R_386_TLS_GOTIE && !canBePreempted(S));
 }
 
 bool X86TargetInfo::needsDynRelative(unsigned Type) const {
   return Config->Shared && Type == R_386_TLS_IE;
bool X86_64TargetInfo::isRelRelative(uint32_t Type) const {
   }
 }
 
 bool X86_64TargetInfo::isSizeRel(uint32_t Type) const {
   return Type == R_X86_64_SIZE32 || Type == R_X86_64_SIZE64;
 }
 
 bool X86_64TargetInfo::canRelaxTls(unsigned Type, const SymbolBody *S) const {
-  if (Config->Shared || (S && !S->IsTls))
+  if (Config->Shared || (S && !S->isTls()))
     return false;
   return Type == R_X86_64_TLSGD || Type == R_X86_64_TLSLD ||
          Type == R_X86_64_DTPOFF32 ||
          (Type == R_X86_64_GOTTPOFF && !canBePreempted(S));
 }
 
 // "Ulrich Drepper, ELF Handling For Thread-Local Storage" (5.5
 // x86-x64 linker optimizations, http://www.akkadia.org/drepper/tls.pdf) shows
case R_AARCH64_TLSLE_ADD_TPREL_LO12_NC: {
     break;
   }
   default:
     fatal("unrecognized reloc " + Twine(Type));
   }
 }
 
 bool AArch64TargetInfo::canRelaxTls(unsigned Type, const SymbolBody *S) const {
-  if (Config->Shared || (S && !S->IsTls))
+  if (Config->Shared || (S && !S->isTls()))
     return false;
 
   // Global-Dynamic relocs can be relaxed to Initial-Exec if the target is
   // an executable.  And if the target is local it can also be fully relaxed to
   // Local-Exec.
   if (isTlsGlobalDynamicRel(Type))
     return !canBePreempted(S);
 

lld/trunk/ELF/Writer.cpp

if (Target->canRelaxTls(Type, nullptr))
       return true;
     if (Out<ELFT>::Got->addTlsIndex())
       Out<ELFT>::RelaDyn->addReloc({Target->TlsModuleIndexRel,
                                     DynamicReloc<ELFT>::Off_LTlsIndex,
                                     nullptr});
     return true;
   }
 
-  if (!Body || !Body->IsTls)
+  if (!Body || !Body->isTls())
     return false;
 
   if (Target->isTlsGlobalDynamicRel(Type)) {
     if (!Target->canRelaxTls(Type, Body)) {
       if (Out<ELFT>::Got->addDynTlsEntry(Body)) {
         Out<ELFT>::RelaDyn->addReloc({Target->TlsModuleIndexRel,
                                       DynamicReloc<ELFT>::Off_GTlsIndex, Body});
         Out<ELFT>::RelaDyn->addReloc(
if (Body && Target->needsGot(Type, *Body)) {
         continue;
       }
 
       bool Dynrel = Config->Shared && !Target->isRelRelative(Type) &&
                     !Target->isSizeRel(Type);
       if (CBP || Dynrel) {
         uint32_t DynType;
         if (CBP)
-          DynType = Body->IsTls ? Target->TlsGotRel : Target->GotRel;
+          DynType = Body->isTls() ? Target->TlsGotRel : Target->GotRel;
         else
           DynType = Target->RelativeRel;
         Out<ELFT>::RelaDyn->addReloc(
             {DynType, DynamicReloc<ELFT>::Off_Got, !CBP, Body});
       }
       continue;
     }