[PATCH] D17934: [ELF] Implement infrastructure for thunk code creation

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/OutputSections.cpp:806
@@ +805,3 @@
+                                   SymbolBody &S) {
+  ThunkChunk<ELFT> *&TC = ThunkChunks[&C];
+  if (!TC)
----------------
Using a hash table feels a bit tricky. Can you add a pointer to SymbolBody to point to a thunk?

================
Comment at: ELF/Target.cpp:1687
@@ -1676,2 +1686,3 @@
 
 template <class ELFT>
+void MipsTargetInfo<ELFT>::writeThunk(uint8_t *Buf, uint64_t S) const {
----------------
Needs a comment.

================
Comment at: ELF/Target.cpp:1721
@@ +1720,3 @@
+bool MipsTargetInfo<ELFT>::needsThunk(uint32_t Type, const InputFile &File,
+                            const SymbolBody &S) const {
+  // Any MIPS PIC code function is invoked with its address in register $t9.
----------------
Please run clang-format-diff.

================
Comment at: ELF/Target.cpp:1733
@@ +1732,3 @@
+  // If current file has PIC code, LA25 stub is not required.
+  if (F->getObj().getHeader()->e_flags & llvm::ELF::EF_MIPS_PIC)
+    return false;
----------------
We have `using namespace llvm::ELF`.

================
Comment at: ELF/Target.cpp:1735
@@ +1734,3 @@
+    return false;
+  auto *ElfSym = dyn_cast<DefinedRegular<ELFT>>(&S);
+  if (!ElfSym || !ElfSym->Section)
----------------
We usually name an object of type Elf_Sym ElfSym. In this case, the return type of RHS is still our own symbol type, so I'd name differently, say just Sym, D, Defined or something like that.

================
Comment at: ELF/Target.cpp:1739-1745
@@ +1738,9 @@
+  // LA25 is required if target file has PIC code or ...
+  if (ElfSym->Section->getFile()->getObj().getHeader()->e_flags &
+      llvm::ELF::EF_MIPS_PIC)
+    return true;
+  // ... target symbol is a PIC symbol.
+  if ((ElfSym->Sym.st_other & llvm::ELF::STO_MIPS_MIPS16) == STO_MIPS_PIC)
+    return true;
+  return false;
+}
----------------
This can be

  return (ElfSym->Section->getFile()->getObj().getHeader()->e_flags & EF_MIPS_PIC) ||
         (ElfSym->Sym.st_other & STO_MIPS_MIPS16) == STO_MIPS_PIC;


================
Comment at: ELF/Writer.cpp:346-352
@@ -345,1 +345,9 @@
 
+    // Some targets might require creation of thunk for relocations.
+    // For example MIPS needs LA25 thunk to local call PIC code from non-PIC.
+    if (Body && !Body->hasThunk() && Target->needsThunk(Type, File, *Body)) {
+      const InputSectionBase<ELFT> *Section =
+          cast<DefinedRegular<ELFT>>(Body)->Section;
+      Section->OutSec->addThunk(*Section, *Body);
+    }
+
----------------
I don't like to add new code to this function because this function is overloaded. Particularly, when this code will be executed is not very obvious because if a condition above is true and it reaches a `continue`, this code will be skipped. Will this code be skipped for all correct cases?

Can you create a new loop over relocations and move this code there?

================
Comment at: ELF/Writer.cpp:1050-1051
@@ -1041,1 +1049,4 @@
 
+  for (OutputSectionBase<ELFT> *Sec : getSections())
+    Sec->reassignOffsets();
+
----------------
This seems a bit odd.

If we always reassign offsets, then we don't want to reassign offsets at all. Instead, we want to assign offsets right here only once.

================
Comment at: ELF/Writer.cpp:1068-1072
@@ -1056,2 +1067,7 @@
         CopyRelSymbols.push_back(SC);
+    if (auto *SS = dyn_cast<DefinedSynthetic<ELFT>>(Body))
+      // Setup values of synthetic symbols that need
+      // to point to end of a section.
+      if (SS->Value == DefinedSynthetic<ELFT>::End)
+        SS->Value = SS->Section.getSize();
 
----------------
I don't understand what this is supposed to do?

================
Comment at: ELF/Writer.cpp:1183
@@ -1166,3 +1182,3 @@
       Symtab.addSynthetic(Start, *OS, 0, STV_DEFAULT);
-      Symtab.addSynthetic(End, *OS, OS->getSize(), STV_DEFAULT);
+      Symtab.addSynthetic(End, *OS, DefinedSynthetic<ELFT>::End, STV_DEFAULT);
     } else {
----------------
What is this change for?


Repository:
  rL LLVM

http://reviews.llvm.org/D17934