[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:1249
+  }
+  return false;
+}
----------------
ruiu wrote:
> 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.
Yes it is intentional, I've updated the comment. The intention is that if the target of the relocation is an in range thunk we are done and can continue. If false then we are not pointing to a thunk and may need to generate a new one. 

I'm on the fence about whether it is a good idea to use a function here as it is quite difficult to find a name for it. What I originally had in createThunks() was something like:  
```
if (target is Thunk) {
  if (Thunk is in Range)
    continue;
  else
    revert to original target
}
```
I used a function to try and avoid createThunks() getting too large. It may be better to remove the function.


================
Comment at: ELF/Relocations.h:156
   // the Thunk from a relocation to the Thunks symbol definition.
   llvm::DenseMap<SymbolBody *, Thunk *> Thunks;
 
----------------
ruiu wrote:
> 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.
I'll submit this in a separate review. It would increase the memory usage for Targets that don't use Thunks so I think it would be best to keep it separate.


https://reviews.llvm.org/D34692