[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/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,
----------------
ruiu wrote:
> sfertile wrote:
> > ruiu wrote:
> > > 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.
> > yeah this is odd, I don't know why its made to look like a .bss section but it is specified by the abi (both V1 and V2 64 bit abis as well as the 32 bit abi I believe)
> Can you add this comment as a code comment? I believe honest comment like that will be useful in the future.
I confirmed https://members.openpowerfoundation.org/document/dl/576 p.93 says that .plt is SHT_NOBITS.


================
Comment at: ELF/SyntheticSections.cpp:884
+// section. I don't know why we have a BSS style type for the section but it is
+// consitent accross both 64-bit PowerPC abis as well as the 32-bit PowerPC abi.
 GotPltSection::GotPltSection()
----------------
nit: accross -> across, abis -> ABIs, abi -> ABI


================
Comment at: ELF/SyntheticSections.cpp:919
 // On ARM the IgotPltSection is part of the GotSection, on other Targets it is
-// part of the .got.plt
+// part of the .got.plt. On PowerPC the .got.plt section is renamed to .plt.
 IgotPltSection::IgotPltSection()
----------------
I think what you are doing is obvious, so please explain why.


================
Comment at: ELF/SyntheticSections.cpp:1203
+                      [=] {
+                        auto Offset = Target->PltHeaderSize - 32;
+                        return InX::Plt->getVA(0) + Offset;
----------------
Please avoid `auto`.


================
Comment at: ELF/SyntheticSections.cpp:1205
+                        return InX::Plt->getVA(0) + Offset;
+                        }});
+  }
----------------
I think this format does not follow the LLVM coding style. Maybe you want to use clang-format.


================
Comment at: ELF/SyntheticSections.cpp:1927
+    : SyntheticSection(SHF_ALLOC | SHF_EXECINSTR, SHT_PROGBITS, 16,
+                      Config->EMachine == EM_PPC64 ? ".glink" : ".plt"),
       HeaderSize(IsIplt ? 0 : Target->PltHeaderSize), IsIplt(IsIplt) {
----------------
Indentation


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D45642