[PATCH] D29129: [LLD][ELF] Use Synthetic Sections for Thunks

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/Relocations.cpp:809
+// This may invalidate any output section offsets stored outside of InputSection
+template <class ELFT>
+static void mergeThunks(OutputSection<ELFT> *OS,
----------------
Capitalize all local variables in this function (i.e. thunkCmp, a, b, tmp and mergeCmp).


================
Comment at: ELF/Relocations.cpp:823-826
+    // All thunks go before any non-executable InputSections
+    if ((IS->Flags & SHF_EXECINSTR) == 0)
+      return true;
+    // Equal Offsets to mean place Thunk before InputSection
----------------
Could you describe not only what it is doing but why we want that behavior?


================
Comment at: ELF/Relocations.cpp:831
+             Thunks.end(), std::back_inserter(tmp), mergeCmp);
+  OS->Sections.swap(tmp);
+  OS->Size = 0;
----------------
Probably move assignment is more C++11-ish than `swap`.


================
Comment at: ELF/Relocations.cpp:863
   for (OutputSectionBase *Base : OutputSections) {
-    auto *OS = dyn_cast<OutputSection<ELFT>>(Base);
-    if (OS == nullptr)
-      continue;
-    for (InputSection<ELFT> *IS : OS->Sections) {
-      for (const Relocation &Rel : IS->Relocations) {
-        if (Rel.Sym == nullptr)
-          continue;
-        RelExpr Expr = Rel.Expr;
-        // Some targets might require creation of thunks for relocations.
-        // Now we support only MIPS which requires LA25 thunk to call PIC
-        // code from non-PIC one, and ARM which requires interworking.
-        if (Expr == R_THUNK_ABS || Expr == R_THUNK_PC ||
-            Expr == R_THUNK_PLT_PC)
-          addThunk<ELFT>(Rel.Type, *Rel.Sym, *IS);
+    if (auto *OS = dyn_cast<OutputSection<ELFT>>(Base)) {
+      ThunkSection<ELFT> *OSTS = nullptr;
----------------
Flip the condition to use `continue`.


================
Comment at: ELF/Relocations.cpp:872-874
+          if (Target->needsThunk(Expr, Type, IS->getFile(), Body)) {
+            Thunk<ELFT> *T = ThunkedSymbols.lookup(&Body);
+            if (T == nullptr) {
----------------
Can you flip the conditions of these `if`s to reduce indentation?


================
Comment at: ELF/Symbols.h:419-420
       DefinedCommon, DefinedRegular<llvm::object::ELF64LE>, DefinedSynthetic,
-      Undefined<llvm::object::ELF64LE>, SharedSymbol<llvm::object::ELF64LE>,
+      Undefined, SharedSymbol<llvm::object::ELF64LE>,
       LazyArchive, LazyObject>
       Body;
----------------
clang-format-diff?


================
Comment at: ELF/SyntheticSections.h:708
+  ThunkSection(OutputSectionBase *OS, uint64_t Off);
+  // Add a newly created Thunk to this container:
+  // Thunk is given offset from start of this InputSection
----------------
nit: add a blank line.


================
Comment at: ELF/Thunks.cpp:264-266
   else if (Config->EMachine == EM_MIPS)
-    addThunkMips<ELFT>(RelocType, S, IS);
+    return addThunkMips<ELFT>(S);
   else
----------------
Remove `else` after `return`


https://reviews.llvm.org/D29129