[PATCH] D13651: [ELF2] - Initial lazy loading support (x86_x64)

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/OutputSections.cpp:39-41
@@ +38,5 @@
+  // .got.plt has 3 reserved entry
+  Entries.push_back(nullptr);
+  Entries.push_back(nullptr);
+  Entries.push_back(nullptr);
+}
----------------
  Entries.resize(3);

================
Comment at: ELF/OutputSections.cpp:65-70
@@ +64,8 @@
+  for (const SymbolBody *B : Entries) {
+    uint8_t *Entry = Buf;
+    Buf += sizeof(uintX_t);
+    if (B) {
+      uint64_t Plt = Out<ELFT>::Plt->getEntryAddr(*B);
+      Target->writeGotPltEntry(Entry, Plt);
+    }
+  }
----------------
  if (B)
    Target->writeGotPltEntry(Buf, Out<ELFT>::Plt->getEntryAddr(*B);
  Buf += sizeof(uintX_t);


================
Comment at: ELF/OutputSections.cpp:115
@@ +114,3 @@
+
+  // first write PLT[0] entry which is special
+  Target->writePltBaseEntry(Buf, Out<ELFT>::GotPlt->getVA(), this->getVA());
----------------
Start with an uppercase character and end with a period.

================
Comment at: ELF/OutputSections.cpp:116
@@ +115,3 @@
+  // first write PLT[0] entry which is special
+  Target->writePltBaseEntry(Buf, Out<ELFT>::GotPlt->getVA(), this->getVA());
+  Buf += Target->getPltBaseSize();
----------------
Is PLT base is a common terminology? I prefer writePltZeroEntry as the name  of the function.

================
Comment at: ELF/OutputSections.cpp:174-208
@@ -125,31 +173,37 @@
 
     bool CanBePreempted = canBePreempted(Body);
     uintX_t Addend = 0;
     if (!CanBePreempted) {
       if (IsRela) {
         if (Body)
           Addend += getSymVA<ELFT>(cast<ELFSymbolBody<ELFT>>(*Body));
         else
           Addend += getLocalSymVA(
               Obj.getRelocationSymbol(&RI, File.getSymbolTable()), File);
       }
       P->setSymbolAndType(0, Target->getRelativeReloc(), IsMips64EL);
     }
 
     if (Body && Target->relocNeedsGot(Type, *Body)) {
-      P->r_offset = Out<ELFT>::Got->getEntryAddr(*Body);
+      bool NeedsPlt = Target->relocNeedsPlt(Type, *Body);
+      if (NeedsPlt)
+        P->r_offset = Out<ELFT>::GotPlt->getEntryAddr(*Body);
+      else
+        P->r_offset = Out<ELFT>::Got->getEntryAddr(*Body);
       if (CanBePreempted)
         P->setSymbolAndType(Body->getDynamicSymbolTableIndex(),
-                            Target->getGotReloc(), IsMips64EL);
+                            NeedsPlt ? Target->getPltReloc()
+                                     : Target->getGotReloc(),
+                            IsMips64EL);
     } else {
       if (IsRela)
         Addend += static_cast<const Elf_Rela &>(RI).r_addend;
       P->r_offset = RI.r_offset + C.getOutputSectionOff() + OutSec->getVA();
       if (CanBePreempted)
         P->setSymbolAndType(Body->getDynamicSymbolTableIndex(), Type,
                             IsMips64EL);
     }
 
     if (IsRela)
       static_cast<Elf_Rela *>(P)->r_addend = Addend;
   }
----------------
This code needs explaining. I'm not actually sure that this code is correct overall.

================
Comment at: ELF/OutputSections.cpp:762-766
@@ -696,2 +761,7 @@
 
+template class GotPltSection<ELF32LE>;
+template class GotPltSection<ELF32BE>;
+template class GotPltSection<ELF64LE>;
+template class GotPltSection<ELF64BE>;
+
 template class PltSection<ELF32LE>;
----------------
Move this before template class GotSection<ELF32LE>;

================
Comment at: ELF/OutputSections.h:305
@@ -287,2 +304,3 @@
   static GotSection<ELFT> *Got;
+  static GotPltSection<ELFT> *GotPlt;
   static HashTableSection<ELFT> *HashTab;
----------------
Sort.

================
Comment at: ELF/OutputSections.h:320
@@ -300,2 +319,3 @@
 template <class ELFT> GotSection<ELFT> *Out<ELFT>::Got;
+template <class ELFT> GotPltSection<ELFT> *Out<ELFT>::GotPlt;
 template <class ELFT> HashTableSection<ELFT> *Out<ELFT>::HashTab;
----------------
Sort.

================
Comment at: ELF/Target.cpp:127-136
@@ +126,12 @@
+
+  uint64_t NextPC = PltEntryAddr + 6;
+  int64_t Delta = GotEntryAddr - NextPC + 8; //GOT + 8
+  assert(isInt<32>(Delta));
+  write32le(Buf + 2, Delta);
+  Buf += 6;
+
+  NextPC += 6;
+  Delta = GotEntryAddr - NextPC + 16; //GOT + 16
+  write32le(Buf + 2, Delta);
+  Buf += 6;
+}
----------------
  write32le(Buf + 2, GotEntryAddr - PltEntryAddr + 2);  // GOT+8
  write32le(Buf + 8, GotEntryAddr - PltEntryAddr + 4);  // GOT+16

No assert() needed.

================
Comment at: ELF/Target.cpp:147-161
@@ -109,7 +146,17 @@
   memcpy(Buf, Inst, sizeof(Inst));
 
   uint64_t NextPC = PltEntryAddr + 6;
   int64_t Delta = GotEntryAddr - NextPC;
   assert(isInt<32>(Delta));
   write32le(Buf + 2, Delta);
+  Buf += 6;
+
+  NextPC += 5;
+  write32le(Buf + 1, Index);
+  Buf += 5;
+
+  NextPC += 5;
+  Delta = PltEntryAddr - Index * PltEntrySize - PltBaseSize - NextPC;
+  assert(isInt<32>(Delta));
+  write32le(Buf + 1, Delta);
 }
----------------
  write32le(Buf + 2, GotEntryAdrr - PltEntryAddr - 6);
  write32le(Buf + 7, Index);
  write32le(Buf + 11, ...);



http://reviews.llvm.org/D13651