[PATCH] D34692: [LLD][ELF] Add support for multiple passes to createThunks()

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Thank you for writing these large summary comments! They are extremely helpful to understand the code.



================
Comment at: ELF/Relocations.cpp:1034
+// of work. An important principle is that it is not an error if a Thunk cannot
+// be placed in a pre-created Section, when this happens we create a new
+// ThunkSection placed next to the caller. This allows us to handle the vast
----------------
Section -> ThunkSection


================
Comment at: ELF/Relocations.cpp:1085
+          return true;
+        else if (TA && !TB && !TA->getTargetInputSection())
+          // In general place Thunk Sections without specific targets before
----------------
nit: remove `else` after `return`.


================
Comment at: ELF/Relocations.cpp:1249
+  }
+  return false;
+}
----------------
This function returns false if a given relocation does not point to a thunk. Is this correct? I thought that this function is expected to return false only when it undo previous thunk creations.


================
Comment at: ELF/Relocations.cpp:1281
+  if (Pass > 0) {
+    NewThunkSections.clear();
+    // With Thunk Size much smaller than branch range we expect to
----------------
Can you clear this vector at end of this function instead? I think "discarding when it is no longer needed" is easier to understand.


================
Comment at: ELF/Relocations.cpp:1285
+    if (Pass == 10)
+      fatal("Thunk creation not converged");
+  } else if (Target->ThunkSectionSpacing)
----------------
Error messages should start with lowercase letter.


================
Comment at: ELF/Relocations.cpp:1286
+      fatal("Thunk creation not converged");
+  } else if (Target->ThunkSectionSpacing)
     createInitialThunkSections(OutputSections);
----------------
nit: add {}


================
Comment at: ELF/Relocations.h:156
   // the Thunk from a relocation to the Thunks symbol definition.
   llvm::DenseMap<SymbolBody *, Thunk *> Thunks;
 
----------------
This is not directly related to this patch, but I believe we should move this to SymbolBody to eliminate the cost of hash table lookup.


https://reviews.llvm.org/D34692