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

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/InputSection.cpp:42-45
@@ +41,6 @@
+template <class ELFT> size_t InputSectionBase<ELFT>::getSize() const {
+  auto *D = dyn_cast<InputSection<ELFT>>(this);
+  if (!D || D->getThunksSize() == 0)
+    return Header->sh_size;
+  return D->getThunkOff() + D->getThunksSize();
+}
----------------
I feel like this is slightly more readable.

  if (auto *D = dyn_cast<InputSection<ELFT>>(this))
    if (D->getThunksSize() > 0)
      return D->getThunkOff() + D->getThunksSize();
  return Header->sh_size;

================
Comment at: ELF/InputSection.cpp:121
@@ +120,3 @@
+template <class ELFT> uint64_t InputSection<ELFT>::getThunkOff() const {
+  return alignTo(this->Header->sh_size, this->Align);
+}
----------------
Is the thunk need to be aligned? If so, does it have the same alignment as the original section?

================
Comment at: ELF/InputSection.cpp:317
@@ -296,1 +316,3 @@
+    } else if (Target->needsThunk(Type, *this->getFile(), Body)) {
+      SymVA = Body.getThunkVA<ELFT>();
     } else if (Config->EMachine == EM_MIPS) {
----------------
This needs a comment.

================
Comment at: ELF/InputSection.cpp:359
@@ +358,3 @@
+
+  if (!Thunks.empty()) {
+    Buf += OutSecOff + getThunkOff();
----------------
This needs a comment, saying that a section may have attached data that needs to be written after the section, and that data is usually a thunk to jump to a location that is too far to fit in a short jump instruction.

================
Comment at: ELF/InputSection.h:172
@@ +171,3 @@
+  // to a mmap'ed file, target is requested to write an actual thunk code.
+  void addThunk(SymbolBody &Body);
+
----------------
I'd also mention that you can ignore this if you are interested only in x86/x86-64.

================
Comment at: ELF/InputSection.h:190
@@ -179,1 +189,3 @@
+
+  std::vector<SymbolBody *> Thunks;
 };
----------------
TinyPtrVector is probably better as I expect this is empty in most cases.

================
Comment at: ELF/Target.cpp:1749
@@ +1748,3 @@
+  // to save the target function address.
+  // See page 3-38 ftp://www.linux-mips.org/pub/linux/mips/doc/ABI/mipsabi.pdf
+  if (Type != R_MIPS_26)
----------------
It is probably better to just say that "see MipsTargetInfo::writeThunk for details" instead of copying the comment.

================
Comment at: ELF/Writer.cpp:483-495
@@ -482,1 +482,15 @@
+
+  // Some targets might require creation of thunk for relocations.
+  // For example, MIPS needs LA25 thunk to local call PIC code from non-PIC.
+  for (const RelTy &RI : Rels) {
+    uint32_t Type = RI.getType(Config->Mips64EL);
+    uint32_t SymIndex = RI.getSymbol(Config->Mips64EL);
+    SymbolBody &Body = File.getSymbolBody(SymIndex).repl();
+
+    if (!Body.hasThunk() && Target->needsThunk(Type, File, Body)) {
+      auto *D = cast<DefinedRegular<ELFT>>(&Body);
+      auto *S = cast<InputSection<ELFT>>(D->Section);
+      S->addThunk(Body);
+    }
+  }
 }
----------------
Please make this a separate function and call only for platforms that need thunks.


Repository:
  rL LLVM

http://reviews.llvm.org/D17934