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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added inline comments.


================
Comment at: ELF/Relocations.cpp:1299-1303
+            // If we are a relocation to an existing Thunk, check if it is
+            // still in range. If not then Rel will be altered to point to its
+            // original target so another Thunk can be generated.
+            if (Pass > 0 && normalizeExistingThunk(Rel, Src))
+              continue;
----------------
ruiu wrote:
> I think it feels more natural if you move this piece of code after `needsThunk` check.
> 
Unfortunately it can't as normalizeExistingThunk() will reset the relocation Rel to the original target if the original Thunk has gone out of range, this prompts needsThunk to create a new Thunk to the original target. If it is moved the needsThunk will be testing against a different Target Symbol.


================
Comment at: ELF/Relocations.h:179
+  // All the ThunkSections that we have created, organised by InputSectionRange
+  std::map<std::vector<InputSection *> *, std::vector<ThunkSection *>>
+      NewThunkSections;
----------------
ruiu wrote:
> Using a `std::vector<T> *` as a key seems a bit odd to me. IIUC, this vector is a member of InputSection, so we can attach a vector of ThunkSections to InputSections, no?
The NewThunkSections is just the subset of ThunkSections that we've created this pass. We need to check all the existing ThunkSections to see if we can reuse them for a new Thunk, however we only want to insert the ThunkSections we've created this pass (the others have already been inserted).

Both NewThunkSections and ThunkSections could be made members of InputSectionDescription, but this would make a local mapping table globally visible to all Targets.

I think that these could be potentially made into flat vectors of ThunkSections, at the cost of some logic of separating all the ThunkSections to go into a particular InputSectionDescription later. 


https://reviews.llvm.org/D34692