[PATCH] D27406: [LLD][ELF] Ifunc implementation using synthetic sections

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

I think I like this patch. Some minor comments.



================
Comment at: ELF/Symbols.h:120
 
+  // True if this symbol is in the Iplt sub-section of the Plt
+  unsigned IsInIplt : 1;
----------------
nit: add `.`


================
Comment at: ELF/Symbols.h:121
+  // True if this symbol is in the Iplt sub-section of the Plt
+  unsigned IsInIplt : 1;
+
----------------
I wonder if they need to be IPlt and IGot instead of Iplt or Igot.


================
Comment at: ELF/SyntheticSections.cpp:726
+template <class ELFT> size_t IgotPltSection<ELFT>::getSize() const {
+  return (Entries.size()) * Target->GotPltEntrySize;
+}
----------------
nit: remove extraneous ().


================
Comment at: ELF/SyntheticSections.cpp:838
   this->Link = In<ELFT>::DynStrTab->OutSec->SectionIndex;
-
-  if (!In<ELFT>::RelaDyn->empty()) {
+  if (In<ELFT>::RelaDyn->OutSec->Size > 0) {
     bool IsRela = Config->Rela;
----------------
 


================
Comment at: ELF/Writer.cpp:352
   Symtab<ELFT>::X->Sections.push_back(In<ELFT>::RelaPlt);
+  // The RelaIplt immediately follows .rel.plt (.rel.dyn for ARM) to ensure
+  // that the IRelative relocations are processed last by the dynamic loader
----------------
Please add a blank line before the comment.


================
Comment at: ELF/Writer.cpp:355-356
+  In<ELFT>::RelaIplt = make<RelocationSection<ELFT>>(
+      Config->Rela ? ".rela.plt" : (Config->EMachine == EM_ARM) ? ".rel.dyn"
+                                                                : ".rel.plt",
+      false /*Sort*/);
----------------
This is a bit tricky. How about this?

  (Config->EMachine == EM_ARM) ? ".rel.dyn" : In<ELFT>::RelaPlt->Name


https://reviews.llvm.org/D27406