[ELF] - Simplify a SymbolBody interface a bit.

[grimar]

Summary

I think we can get rid of few accessors in that class, and replace
them with direct field access.

Positive features from that are (just an IMHO of course):

• Use of direct fields instead of accessor is more readable as you can never know the exact logic of accessor when looking on it (will it just return the pre-ready result or has some condition).
• Code generally is a bit shorter (please refer to the patch code).
• Less methods in interfaces.
• Currently 2 bitfields *probably* consume less memory than single int8 (that depends on structures aligment I think and I did not look close on difference for this patch). Also looks like we will not need any other flags, but even if we will, I think 1-2 new bitfields is probably more preferable than 1-2 new methods because of profits mentioned above.

[• rafael]

maybe just wait and see the bug Davide was referring to in the other
patch? (some symbols not getting a STT_OBJECT).

Cheers,
Rafael

[grimar]

In D17879#367958, @rafael wrote:

maybe just wait and see the bug Davide was referring to in the other
patch? (some symbols not getting a STT_OBJECT).

Cheers,
Rafael

Do you mean this http://reviews.llvm.org/D17876 ? Then its aleady abandoned.

[grimar]

grimar added a comment.

In http://reviews.llvm.org/D17879#367958, @rafael wrote:

maybe just wait and see the bug Davide was referring to in the other
patch? (some symbols not getting a STT_OBJECT).

Cheers,
Rafael

Do you mean this http://reviews.llvm.org/D17876 ? Then its aleady abandoned.

Looks like it.

Davide, was that the one you were referring to?

Cheers,
Rafael

Actually my patch is closer to this: http://reviews.llvm.org/D17871.
It modifies it and introduces another way to do the same.

George.

[davide]

I'm not entirely convinced this is a big win honestly. ELF has already a type field we can just leave untouched. isTLS() member function is pretty self-explicative. Maybe if you feel is not it can be renamed to isTlsType() but I don't think that's really required. Still, you have to look at how it's set somewhere in the code to understand.
Also, accessing the field directly rather than hiding the information behind a member function, I'm not a huge fan of that either.
That said, if others like this approach better, I have no strong objections against it.

[davide]

Also, about the bit field consumes less memory, I'm note entirely sure of that given the current layout. I sort of feel it like a premature'ish optimization. If Symbolbody turns out to be too big then we can reconsider the whole layout to make it smaller. Also, Type is per-se less than 8 bits, so you can apply your proposed optimization to my field as well.

[ruiu]

LGTM

I don't have a strong preference here, and this patch doesn't reduce the size of SymbolBody (it remains 48 bytes on 64-bit), but if I had written it I would have do that way, so go for it.

[• rafael]

I also have a small preference for it.

The reason being that if we don't need the generality of an arbitrary Type, not storing makes it clear that it is not used.

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/OutputSections.cpp

if (Body->isUndefined()) {
 
     // Otherwise, just resolve to 0.
     return false;
   }
   if (!Config->Shared)
     return false;
   if (Body->getVisibility() != STV_DEFAULT)
     return false;
-  if (Config->Bsymbolic || (Config->BsymbolicFunctions && Body->isFunc()))
+  if (Config->Bsymbolic || (Config->BsymbolicFunctions && Body->IsFunc))
     return false;
   return true;
 }
 
 static void fill(uint8_t *Buf, size_t Size, ArrayRef<uint8_t> A) {
   size_t I = 0;
   for (; I + A.size() < Size; I += A.size())
     memcpy(Buf + I, A.data(), A.size());

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?
-    if (Undef->isTls())
-      L->setTls();
+    L->IsTls |= Undef->IsTls;
     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 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:
   // 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,
              uint8_t Type)
       : SymbolKind(K), IsWeak(IsWeak), Visibility(Visibility),
-        MustBeInDynSym(false), NeedsCopyOrPltAddr(false),
-        Type(Type), Name(Name) {
+        MustBeInDynSym(false), NeedsCopyOrPltAddr(false), Name(Name) {
+    IsFunc = Type == llvm::ELF::STT_FUNC;
+    IsTls = Type == llvm::ELF::STT_TLS;
     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;
 
-protected:
-  uint8_t Type;
+  unsigned IsTls : 1;
+  unsigned IsFunc : 1;
 
+protected:
   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,
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,
                    /* 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:

lld/trunk/ELF/Target.cpp

case EM_X86_64:
     return new X86_64TargetInfo();
   }
   fatal("Unknown target machine");
 }
 
 TargetInfo::~TargetInfo() {}
 
 bool TargetInfo::canRelaxTls(uint32_t Type, const SymbolBody *S) const {
-  if (Config->Shared || (S && !S->isTls()))
+  if (Config->Shared || (S && !S->IsTls))
     return false;
 
   // We know we are producing an executable.
 
   // Global-Dynamic relocs can be relaxed to Initial-Exec or Local-Exec
   // depending on the symbol being locally defined or not.
   if (isTlsGlobalDynamicRel(Type))
     return true;
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;

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;
     }