[PATCH] D34689: [LLD][ELF] Pre-create ThunkSections at Target specific intervals

[Peter Smith via Phabricator via llvm-commits]

peter.smith marked 3 inline comments as done.
peter.smith added a comment.

Thanks for the comments, I'll update the diff.

What I'm aiming for is:

  Section 0.0
  ...
  Precreated RangeThunk
  ; (dot) - Section 0.0->OutSecOff < ThunkSectionSpacing)
  Section 1.0
  ...
  Precreated RangeThunk
  ; (dot) - Section 1.0->OutSecOff < ThunkSectionSpacing)

This is a simple way of making sure that as many branches within the gaps between ThunkSections can reach their targets without needing additional ThunkSections creating on later passes.



================
Comment at: ELF/Arch/ARM.cpp:64
   NeedsThunks = true;
+  // Thumb unconditional branch range on system with Thumb2 branch encoding
+  ThunkSectionSpacing = 0x1000000;
----------------
ruiu wrote:
> Can you extend the comment so that it is more friendly to those who don't know much about ARM? e.g. Thumb unconditional branch instructions have xx bit displacement, so it can jump to +-yy bytes. We insert thunks for at least yy bytes to support longer jumps.
I've added to the comment.


================
Comment at: ELF/Arch/ARM.cpp:66
+  ThunkSectionSpacing = 0x1000000;
+  // Allow for 16384 12 byte Thunks per ThunkSectionSpacing
+  ThunkSectionSize = 0x30000;
----------------
ruiu wrote:
> Why do you want to limit the size of thunk size?
What I'm trying to achieve is something like:

```
Range of sections:
start:
Section 0 at OutSecOff 0
...
RangeThunk at offset OFF from start. Where (RangeThunk.OutSecOff + RangeThunk.Size) - start < ThunkSectionSpacing
```

The ThunkSectionSize is a pessimistic guess at the final RangeThunk.Size so that we can guarantee that a branch at offset 0 can reach anywhere within RangeThunk.


================
Comment at: ELF/Relocations.cpp:1012
 
-ThunkSection *ThunkCreator::getOSThunkSec(OutputSection *OS,
-                                          std::vector<InputSection *> *ISR) {
-  if (CurTS == nullptr) {
-    uint32_t Off = 0;
-    for (auto *IS : OS->Sections) {
-      Off = IS->OutSecOff + IS->getSize();
-      if ((IS->Flags & SHF_EXECINSTR) == 0)
-        break;
-    }
-    CurTS = addThunkSection(OS, ISR, Off);
-  }
-  return CurTS;
+ThunkSection *ThunkCreator::getISRThunkSec(OutputSection *OS,
+                                           std::vector<InputSection *> *ISR) {
----------------
ruiu wrote:
> What is ISR? Adding a function comment would help.
I've added a comment. ISR is meant to stand for Input Section Range, which is supposed to represent a range of InputSections (specifically those that come from a single InputSectionDescription), I used range rather than description as at one point we had a distinction between the script and non-script cases.

I'm open to suggestions for better names, or I can expand them to make them a bit more self-descriptive?
  


https://reviews.llvm.org/D34689