[PATCH] D45642: [PPC64] V2 abi: Add glink section for lazy symbol resolution.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/Arch/PPC64.cpp:198
+  int64_t GotPltOffset = InX::GotPlt->getVA() - (InX::Plt->getVA() + 8);
+  write64(Buf + 52, GotPltOffset);
+}
----------------
Looks like you set 60 to `PltHeaderSize` but you are using only the first 56 bytes. Is this intentional?


================
Comment at: ELF/SyntheticSections.cpp:882-883
 GotPltSection::GotPltSection()
-    : SyntheticSection(SHF_ALLOC | SHF_WRITE, SHT_PROGBITS,
-                       Target->GotPltEntrySize, ".got.plt") {}
+    : SyntheticSection(SHF_ALLOC | SHF_WRITE,
+                       Config->EMachine == EM_PPC64 ? SHT_NOBITS : SHT_PROGBITS,
+                       Target->GotPltEntrySize,
----------------
BSS-ish .got.plt looks pretty odd. Is this really what the spec says? It at least needs some explanation in comment for a reader.


================
Comment at: ELF/SyntheticSections.cpp:885
+                       Target->GotPltEntrySize,
+                       Config->EMachine == EM_PPC64  ? ".plt" : ".got.plt") {}
 
----------------
This also needs an explanation.


================
Comment at: ELF/SyntheticSections.cpp:1029-1033
 template <class ELFT>
+void DynamicSection<ELFT>::addInSecWithOffset(int32_t Tag, InputSection *Sec,
+                                              uint32_t Offset) {
+  Entries.push_back({Tag, [=] { return Sec->getVA(0) + Offset; }});
+}
----------------
Since you are using this only once, I'd inline it and remove this function.


================
Comment at: ELF/Target.h:160-162
+// The DT_PPC64_GLINK tag needs to point this many bytes
+// before the first lazy resolver stub.
+enum { GlinkTagOffset = 32 };
----------------
We often inline this kind of magic numbers with some comments if they are used only once, especially when they are a constant specified by the spec.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D45642