[PATCH] D34691: [LLD][ELF] Introduce range extension thunks for ARM

[Peter Smith via Phabricator via llvm-commits]

peter.smith added inline comments.


================
Comment at: ELF/Relocations.cpp:1031
+  // many Thunks. Create a new ThunkSection.
+  return addThunkSection(OS, ISR, IS->OutSecOff);
 }
----------------
smeenai wrote:
> This assumes that `IS->OutSecOff` is in range of `Src`, correct?
> 
> If I understand correctly, you can insert a `ThunkSection` at the start or the end of an `InputSection`, but not in the middle. Over here, should we instead check if the start and the end of `IS` are in range of `Src`, use whichever one is in range, and raise an error if neither of them is?
Agreed, I've made the change and I've added a couple of test cases for it.


================
Comment at: ELF/Relocations.cpp:1043
+  // InputSection (IS) that we need to precede is in.
+  OutputSection *TOS = IS->getParent();
   std::vector<InputSection *> *Range = nullptr;
----------------
smeenai wrote:
> The changes in this function are just an NFC variable rename, and don't seem super related to this patch. You could upload them separately (or even just commit them directly).
I've extracted these into D37627 


================
Comment at: ELF/Relocations.cpp:1148-1149
+// FIXME: Initial support for RangeThunks, only one pass supported
+bool ThunkCreator::createThunks(
+    ArrayRef<OutputSection *> OutputSections) {
   if (Pass > 0)
----------------
smeenai wrote:
> Did `clang-format` do this? The combined line is under 80 chars, so I'm not sure why it would be split.
I think I had run clang-format on the previous and next patch but not this one. I've now run it on this one.


================
Comment at: ELF/Thunks.cpp:249
   case R_ARM_JUMP24:
+  case R_ARM_CALL:
     if (Config->Pic)
----------------
smeenai wrote:
> Are the changes in this function related to this diff?
Yes they are. Thunks were previously only supported for branch (B, B.w) instructions as these cannot be transformed into a BLX for interworking. The change here permits BL and BLX instructions to have Thunks.


================
Comment at: ELF/Writer.cpp:1392
   if (Target->NeedsThunks) {
     // FIXME: only ARM Interworking and Mips LA25 Thunks are implemented,
     // these
----------------
smeenai wrote:
> This comment should be updated.
I've removed it.


https://reviews.llvm.org/D34691