[llvm-commits] [PATCH] [lld][ELF] Add support for IFUNC.

[Shankar Kalpathi Easwaran]

================
Comment at: lib/ReaderWriter/ELF/DefaultELFLayout.h:329-332
@@ -310,2 +328,6 @@
     return ".bss";
+  if (contentType == DefinedAtom::typeGOT)
+    return ".got";
+  if (contentType == DefinedAtom::typeStub)
+    return ".plt";
   if (name.startswith(".text"))
----------------
Is there a need to distinguish the .got/.plt for IRELATIVE relocations from actual .got/.plt ?

================
Comment at: lib/ReaderWriter/ELF/DefaultELFLayout.h:255-263
@@ -253,1 +254,11 @@
 
+  ELFRelocationTable<ELFT> *getRelocationTable() {
+    // Only create the relocation table if it is needed.
+    if (!_relocationTable) {
+      _relocationTable =
+          new (_allocator) ELFRelocationTable<ELFT>(".rela.plt", ORDER_REL);
+      addSection(_relocationTable);
+    }
+    return _relocationTable;
+  }
+
----------------
This would be in the targetHandler after WriterELF adds code to call the targetHandler right ?

================
Comment at: lib/ReaderWriter/ELF/DefaultELFLayout.h:352
@@ -329,2 +351,3 @@
   case ORDER_DYNAMIC_STRINGS:
+  case ORDER_REL:
   case ORDER_INIT:
----------------
Does binutils, create a seperate segment for ORDER_REL ?

================
Comment at: lib/ReaderWriter/ELF/ELFSectionChunks.h:304-305
@@ -296,1 +303,4 @@
 Section<ELFT>::type() {
+  if (_sectionKind == K_SymbolTable)
+    return llvm::ELF::SHT_SYMTAB;
+
----------------
Couldnt follow on why is this needed ?

================
Comment at: lib/ReaderWriter/ELF/ELFSectionChunks.h:725-726
@@ -702,1 +724,4 @@
 
+template <class ELFT> class ELFRelocationTable : public Section<ELFT> {
+public:
+  typedef llvm::object::Elf_Rel_Impl<ELFT, true> Elf_Rela;
----------------
Should this class be renamed differently, since its main purpose is to support IRELATIVE relocations ?

================
Comment at: lib/ReaderWriter/ELF/FileELF.h:55-56
@@ -54,1 +54,4 @@
       : File(MB->getBufferIdentifier()), _elfTargetInfo(ti) {
+    static uint32_t lastOrdinal = 0;
+    _ordinal = lastOrdinal++;
+
----------------
Wasnt ordinals supposed to be set by OrderPass ?

================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:159-160
@@ -158,2 +158,4 @@
   _runtimeFile.addAbsoluteAtom("__init_array_end");
+  _runtimeFile.addAbsoluteAtom("__rela_iplt_start");
+  _runtimeFile.addAbsoluteAtom("__rela_iplt_end");
 }
----------------
I think even this will go into each Target once Writer hooks are being called into the target right ?

================
Comment at: lib/ReaderWriter/ELF/WriterELF.cpp:179-181
@@ -176,12 +178,5 @@
  auto initArrayStartIter = _layout->findAbsoluteAtom("__init_array_start");
  auto initArrayEndIter = _layout->findAbsoluteAtom("__init_array_end");
-
- auto section = _layout->findOutputSection(".init_array");
- if (section) {
-   initArrayStartIter->setValue(section->virtualAddr());
-   initArrayEndIter->setValue(section->virtualAddr() +
-                              section->memSize());
- } else {
-   initArrayStartIter->setValue(0);
-   initArrayEndIter->setValue(0);
- }
+  auto realIpltStartIter = _layout->findAbsoluteAtom("__rela_iplt_start");
+  auto realIpltEndIter = _layout->findAbsoluteAtom("__rela_iplt_end");
+
----------------
Is there an indentation issue ?

================
Comment at: lib/ReaderWriter/ELF/DefaultELFLayout.h:329-332
@@ -310,2 +328,6 @@
     return ".bss";
+  if (contentType == DefinedAtom::typeGOT)
+    return ".got";
+  if (contentType == DefinedAtom::typeStub)
+    return ".plt";
   if (name.startswith(".text"))
----------------
Shankar Kalpathi Easwaran wrote:
> Is there a need to distinguish the .got/.plt for IRELATIVE relocations from actual .got/.plt ?
If the GOTAtom/PLT atom have defined a function to return SectionName, this wouldnt be necessary. I think that would be cleaner.

================
Comment at: lib/ReaderWriter/ELF/X86_64/X86_64TargetInfo.cpp:89-91
@@ +88,5 @@
+
+class PLTPass : public Pass {
+public:
+  PLTPass(const ELFTargetInfo &ti) : _file(ti) {}
+
----------------
should the class be changed to IPLT to refer to IFUNC ?

================
Comment at: lib/ReaderWriter/ELF/X86_64/X86_64TargetInfo.cpp:131
@@ -16,2 +130,3 @@
+
 #define LLD_CASE(name) .Case(#name, llvm::ELF::name)
 
----------------
Should this go into a generic header ? I think every other code for relcations are going to use the same.

================
Comment at: lib/ReaderWriter/ELF/X86_64Reference.cpp:115-120
@@ -114,2 +114,8 @@
                                    uint64_t targetAddress) {
+  if (reloc == llvm::ELF::R_X86_64_IRELATIVE ||
+      reloc == llvm::ELF::R_X86_64_GOTPCREL ||
+      reloc == llvm::ELF::R_X86_64_PLT32 ||
+      reloc == llvm::ELF::R_X86_64_GOTTPOFF ||
+      reloc == llvm::ELF::R_X86_64_TPOFF32)
+    return;
   if (_fixupHandler[reloc])
----------------
should you have an assert here, than return 0 ?


http://llvm-reviews.chandlerc.com/D324