[PATCH] D13856: [ELF2] - Lazy relocation support for x86_64.

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/OutputSections.cpp:312
@@ -253,1 +311,3 @@
   }
+  // RelaPlt can be missing if target does not supports lazy relocations.
+  if (Out<ELFT>::RelaPlt && Out<ELFT>::RelaPlt->hasRelocs()) {
----------------
ruiu wrote:
> Remove comment
Done.

================
Comment at: ELF/OutputSections.cpp:386
@@ -320,1 +385,3 @@
   }
+  // RelaPlt can be missing if target does not supports lazy relocations.
+  if (Out<ELFT>::RelaPlt && Out<ELFT>::RelaPlt->hasRelocs()) {
----------------
ruiu wrote:
> Ditto
Done.

================
Comment at: ELF/OutputSections.h:113-114
@@ +112,4 @@
+class GotPltSection final : public OutputSectionBase<ELFT> {
+  typedef OutputSectionBase<ELFT> Base;
+  typedef typename Base::uintX_t uintX_t;
+
----------------
ruiu wrote:
> Use ELFFile<ELFT>::uintX_t instead of Base.
Done.

================
Comment at: ELF/Target.cpp:219-226
@@ -197,2 +218,10 @@
   RelativeReloc = R_X86_64_RELATIVE;
+  LazyRelocations = true;
+  if (LazyRelocations) {
+    PltEntrySize = 16;
+    PltZeroEntrySize = 16;
+  } else {
+    PltEntrySize = 8;
+    PltZeroEntrySize = 0;
+  }
 }
----------------
ruiu wrote:
> Setting true and immediately use that value in `if` looks very odd.
> 
>   LazyRelocations = true;
>   PltEntrySize = 16;
>   PltZeroEntrySize = 16;
> 
Done.

================
Comment at: ELF/Target.cpp:230-231
@@ +229,4 @@
+void X86_64TargetInfo::writeGotPltEntry(uint8_t *Buf, uint64_t Plt) const {
+  if (!LazyRelocations)
+    return;
+  // Skip 6 bytes of "jmpq *got(%rip)"
----------------
ruiu wrote:
> Always assume it's true.
Done.

================
Comment at: ELF/Target.cpp:238-240
@@ +237,5 @@
+                                         uint64_t PltEntryAddr) const {
+  if (!LazyRelocations)
+    return;
+
+  const uint8_t PltData[] = {
----------------
ruiu wrote:
> Same
Done.

================
Comment at: ELF/Target.cpp:254
@@ +253,3 @@
+                                     int32_t Index) const {
+  if (LazyRelocations) {
+    const uint8_t Inst[] = {
----------------
ruiu wrote:
> Ditto.
Done.

================
Comment at: ELF/Target.cpp:620
@@ -550,2 +619,3 @@
   // GotReloc = FIXME
+  // PltReloc = FIXME
 }
----------------
ruiu wrote:
> Please do not add this kind of TODO. We should rather remove all useless "= FIXME", but that should be done in other patch.
Removed.

================
Comment at: ELF/Target.cpp:704
@@ -629,2 +703,3 @@
   // GotReloc = FIXME
+  // PltReloc = FIXME
   PageSize = 65536;
----------------
ruiu wrote:
> Ditto.
Removed.

================
Comment at: ELF/Writer.cpp:105
@@ -104,1 +104,3 @@
+  GotPltSection<ELFT> GotPlt;
+  Out<ELFT>::GotPlt = Target->supportsLazyRelocations() ? &GotPlt : nullptr;
   PltSection<ELFT> Plt;
----------------
ruiu wrote:
> Global variables are initialized to nullptr, so
> 
>   if (Target->supportsLazyRelocations())
>     Out<ELFT>::GotPlt = &GotPlt;
Done.

================
Comment at: ELF/Writer.cpp:118
@@ -114,1 +117,3 @@
+  RelocationSection<ELFT> RelaPlt(IsRela ? ".rela.plt" : ".rel.plt", IsRela);
+  Out<ELFT>::RelaPlt = Target->supportsLazyRelocations() ? &RelaPlt : nullptr;
   DynamicSection<ELFT> Dynamic(*Symtab);
----------------
ruiu wrote:
> Ditto.
Done.

================
Comment at: ELF/Writer.cpp:195
@@ -188,3 +194,3 @@
     if (Body) {
-      if (Target->relocNeedsPlt(Type, *Body)) {
+      if ((NeedsPlt = Target->relocNeedsPlt(Type, *Body))) {
         if (Body->isInPlt())
----------------
ruiu wrote:
> Move the assignment out of this `if`.
Done.

================
Comment at: ELF/Writer.cpp:200
@@ -193,6 +199,3 @@
       }
-      NeedsGot = Target->relocNeedsGot(Type, *Body);
-      if (NeedsGot) {
-        if (Body->isInGot())
-          continue;
-        Out<ELFT>::Got->addEntry(Body);
+      if ((NeedsGot = Target->relocNeedsGot(Type, *Body))) {
+        if (NeedsPlt && Target->supportsLazyRelocations()) {
----------------
ruiu wrote:
> Ditto.
Done.


http://reviews.llvm.org/D13856