[ELF][MIPS] Delete GotSection::addMipsLocalEntry method

[atanasyan]

Summary

Now local symbols have SymbolBody so we can handle all kind of symbols in the GotSection::addEntry method. The patch moves the code from addMipsLocalEntry to addEntry.

[ruiu]

I'm not very sure whether this is positive for readability or not. This seems to be neutral to me. If you could just remove the code that would be fine, but this patch add a new if (Body.isPreemptible()), which is a bit difficult to understand. Is there any way to simplify it further?

Note that I'm not against this patch.

[atanasyan]

In D18302#378884, @ruiu wrote:

I'm not very sure whether this is positive for readability or not. This seems to be neutral to me. If you could just remove the code that would be fine, but this patch add a new if (Body.isPreemptible()), which is a bit difficult to understand. Is there any way to simplify it further?

Unfortunately the if (Body.isPreemptible()) is mandatory because we do not need to add to dynamic symbol table symbols which have entries in the "local" part of MIPS GOT. I can add a comment to describe this if statement. Even after that the code is shorter.

[• rafael]

Probably OK. Can you just fix these comments and upload a patch for another look?

ELF/OutputSections.cpp

93 ↗

(On Diff #51127)

A local symbol is not preemptable, so you can simplify the if.

ELF/Writer.cpp

409 ↗

(On Diff #51127)

Can this now be moved to addEntry? It is getting a "if mips" in this patch.

[atanasyan]

• Simplified condition in the GotSection::addEntry method.
• Move the Sym.MustBeInDynSym = true to the GotSection::addEntry method.

[ruiu]

I will try to find a way to handle this MIPS ABI oddity in some more readable way, but this is OK for now (and I'm not sure if such better way exists). LGTM.

By the way, I'm on vacation this week, so my response would be a bit slow this week.

ELF/OutputSections.cpp

93 ↗

(On Diff #51195)

I think we need a brief explanation what we are doing here for MIPS even if it is a duplicate with a comment in other place.

[• rafael]

LGTM too.

[atanasyan]

Thanks for review.

lld/trunk/ELF/OutputSections.h

template <class ELFT> class GotSection final : public OutputSectionBase<ELFT> {
   typedef OutputSectionBase<ELFT> Base;
   typedef typename ELFT::uint uintX_t;
 
 public:
   GotSection();
   void finalize() override;
   void writeTo(uint8_t *Buf) override;
   void addEntry(SymbolBody &Sym);
-  void addMipsLocalEntry();
   bool addDynTlsEntry(SymbolBody &Sym);
   bool addTlsIndex();
   bool empty() const { return MipsLocalEntries == 0 && Entries.empty(); }
   uintX_t getMipsLocalFullAddr(const SymbolBody &B);
   uintX_t getMipsLocalPageAddr(uintX_t Addr);
   uintX_t getGlobalDynAddr(const SymbolBody &B) const;
   uintX_t getNumEntries() const { return Entries.size(); }
 

lld/trunk/ELF/OutputSections.cpp

 GotSection<ELFT>::GotSection()
     : OutputSectionBase<ELFT>(".got", SHT_PROGBITS, SHF_ALLOC | SHF_WRITE) {
   if (Config->EMachine == EM_MIPS)
     this->Header.sh_flags |= SHF_MIPS_GPREL;
   this->Header.sh_addralign = sizeof(uintX_t);
 }
 
 template <class ELFT> void GotSection<ELFT>::addEntry(SymbolBody &Sym) {
+  if (Config->EMachine == EM_MIPS) {
+    if (Sym.isPreemptible())
+      // All symbols with MIPS GOT entries should be represented
+      // in the dynamic symbols table. See "Global Offset Table" in Chapter 5:
+      // ftp://www.linux-mips.org/pub/linux/mips/doc/ABI/mipsabi.pdf
+      Sym.MustBeInDynSym = true;
+    else {
+      // FIXME (simon): Do not add so many redundant entries.
+      ++MipsLocalEntries;
+      return;
+    }
+  }
   Sym.GotIndex = Entries.size();
   Entries.push_back(&Sym);
 }
 
-template <class ELFT> void GotSection<ELFT>::addMipsLocalEntry() {
-  ++MipsLocalEntries;
-}
-
 template <class ELFT> bool GotSection<ELFT>::addDynTlsEntry(SymbolBody &Sym) {
   if (Sym.hasGlobalDynIndex())
     return false;
   Sym.GlobalDynIndex = Target->GotHeaderEntriesNum + Entries.size();
   // Global Dynamic TLS entries take two GOT slots.
   Entries.push_back(&Sym);
   Entries.push_back(nullptr);
   return true;

lld/trunk/ELF/Writer.cpp

for (const RelTy &RI : Rels) {
     bool Preemptible = Body.isPreemptible();
     if (handleTlsRelocation<ELFT>(Type, Body, C, RI))
       continue;
 
     if (Target->needsDynRelative(Type))
       Out<ELFT>::RelaDyn->addReloc({Target->RelativeRel, &C, RI.r_offset, true,
                                     &Body, getAddend<ELFT>(RI)});
 
-    // MIPS has a special rule to create GOTs for local symbols.
-    if (Config->EMachine == EM_MIPS && !Preemptible &&
-        Target->needsGot(Type, Body)) {
-      // FIXME (simon): Do not add so many redundant entries.
-      Out<ELFT>::Got->addMipsLocalEntry();
-      continue;
-    }
-
     // If a symbol in a DSO is referenced directly instead of through GOT,
     // we need to create a copy relocation for the symbol.
     if (auto *B = dyn_cast<SharedSymbol<ELFT>>(&Body)) {
       if (B->needsCopy())
         continue;
       if (Target->needsCopyRel<ELFT>(Type, *B)) {
         B->NeedsCopyOrPltAddr = true;
         Out<ELFT>::RelaDyn->addReloc(
for (const RelTy &RI : Rels) {
     }
 
     // If a relocation needs GOT, we create a GOT slot for the symbol.
     if (Target->needsGot(Type, Body)) {
       if (Body.isInGot())
         continue;
       Out<ELFT>::Got->addEntry(Body);
 
-      if (Config->EMachine == EM_MIPS) {
+      if (Config->EMachine == EM_MIPS)
         // MIPS ABI has special rules to process GOT entries
         // and doesn't require relocation entries for them.
         // See "Global Offset Table" in Chapter 5 in the following document
         // for detailed description:
         // ftp://www.linux-mips.org/pub/linux/mips/doc/ABI/mipsabi.pdf
-        Body.MustBeInDynSym = true;
         continue;
-      }
 
       bool Dynrel = Config->Pic && !Target->isRelRelative(Type) &&
                     !Target->isSizeRel(Type);
       if (Preemptible || Dynrel) {
         uint32_t DynType;
         if (Body.IsTls)
           DynType = Target->TlsGotRel;
         else if (Preemptible)