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

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added inline comments.


================
Comment at: ELF/Arch/ARM.cpp:239
       return true;
-    break;
+    // Fall through
+  case R_ARM_CALL: {
----------------
You should use `LLVM_FALLTHROUGH`.


================
Comment at: ELF/Arch/ARM.cpp:250
       return true;
-    break;
+    // Fall through
+  case R_ARM_THM_CALL: {
----------------
Same here.


================
Comment at: ELF/Relocations.cpp:1031
+  // many Thunks. Create a new ThunkSection.
+  return addThunkSection(OS, ISR, IS->OutSecOff);
 }
----------------
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?


================
Comment at: ELF/Relocations.cpp:1043
+  // InputSection (IS) that we need to precede is in.
+  OutputSection *TOS = IS->getParent();
   std::vector<InputSection *> *Range = nullptr;
----------------
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).


================
Comment at: ELF/Relocations.cpp:1147
 //
-// FIXME: All Thunks are assumed to be in range of the relocation. Range
-// extension Thunks are not yet supported.
-bool ThunkCreator::createThunks(ArrayRef<OutputSection *> OutputSections) {
+// FIXME: Initial support for RangeThunks, only one pass supported
+bool ThunkCreator::createThunks(
----------------
Nits: The comma should be a semicolon, and there should be a period at the end.


================
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)
----------------
Did `clang-format` do this? The combined line is under 80 chars, so I'm not sure why it would be split.


================
Comment at: ELF/Thunks.cpp:249
   case R_ARM_JUMP24:
+  case R_ARM_CALL:
     if (Config->Pic)
----------------
Are the changes in this function related to this diff?


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


https://reviews.llvm.org/D34691