[PATCH] D44962: ELF: Allow thunks to change size. NFCI.

[Peter Smith via Phabricator via llvm-commits]

peter.smith added inline comments.


================
Comment at: lld/ELF/Relocations.cpp:1361
             if (IsNew) {
               AddressesChanged = true;
               // Find or create a ThunkSection for the new Thunk
----------------
With assignOffsets() this line may no longer be required. From memory it was used when a thunk was added in a later pass to a pre-existing ThunkSection and if there were no other thunk sections added we reported no addresses changed. It looks like assignOffsets() would catch this.


================
Comment at: lld/ELF/Thunks.h:54
 
+  Defined *getThunkSym() const { return Syms[0]; }
+
----------------
Perhaps getThunkTarget() or getThunkTargetSym() as this would reinforce the 1 target of the Thunk. 


================
Comment at: lld/ELF/Thunks.h:59
   Symbol &Destination;
-  Symbol *ThunkSym;
+  llvm::SmallVector<Defined *, 3> Syms;
   uint64_t Offset = 0;
----------------
ruiu wrote:
> Can you add a comment somewhere why you could have more than one symbol for one thunk?
I think that the extra symbols are to account for the presence of Arm/Thumb mapping symbols. These are aids for disassembly and aren't part of the interface. There will only ever be one "Thunk Symbol". Perhaps the comment on addSymbols() should say something like "All thunks must define a symbol, known as the thunk symbol, so that we can redirect relocations to it. The thunk may define additional symbols, but these are never targets for relocations.


https://reviews.llvm.org/D44962