[PATCH] D72344: [LLD][ELF][ARM][AArch64] Only round up ThunkSection Size when large OS.

[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 post an update with a simplification as I don't think I can justify the heuristic without being too hand wavy.



================
Comment at: lld/ELF/Relocations.cpp:1772
+    //     that code.
+    // 2.) The InputSectionDescription is smaller than 4 KiB. This will prevent
+    //     any assertion failures that an InputSectionDescription is < 4 KiB
----------------
MaskRay wrote:
> larger? (` isdLimit - isdBase > 4096`)
Yes, larger. Thanks for spotting that one.


================
Comment at: lld/ELF/Relocations.cpp:1775
+    //     in size.
+    if (isdBase + target->getThunkSectionSpacing() < os->size &&
+        isdLimit - isdBase > 4096) {
----------------
MaskRay wrote:
> MaskRay wrote:
> > Can we simplify `isdBase + target->getThunkSectionSpacing() < os->size` to `off + target->getThunkSectionSpacing() < os->size`
> We can't. 
> 
> `off + target->getThunkSectionSpacing() < os->size` can cause https://bugs.llvm.org//show_bug.cgi?id=44071#c5 to fail with `ld.lld: error: thunk creation not converged`.
> 
> I suggested `off` because I thought `off` corresponds to `prevIsecLimit` in the code below:
> 
> ```lang=cpp
>           if (isecLimit > thunkUpperBound) {
>             addThunkSection(os, isd, prevIsecLimit);
>             thunkUpperBound = prevIsecLimit + thunkSectionSpacing;
>           }
> ```
> 
> Still puzzled why we use `isdBase + target->getThunkSectionSpacing() < os->size` here.
Going over this again, and refreshing where the thunks are actually inserted and not using my memory and looking at the failing chromium example the isdBase, TS1 off, TS2 off, isdLimit are as follows 
| 0 | TS1 0x7fcffb8 | TS2 0x8051488 |  0x10024008 |
The Thunk insertion code tries to insert the first thunk section as far away as thunkUpperBound, but stops at lastThunkLowerBound as we can comfortably reach from a branch from the end of the isd.

Both the ThunkSections + thunkSectionSpacing are > os->size, but isdBase + thunkSectionSpacing is not, so any ThunkSection created in the isd will have its size rounded up.

The isdBase + thunkSectionSpacing < os->size is essentially a heuristic. In effect saying that we are less likely to need thunks and patches when in the last thunkSectionSpacing() of the os. I can only really justify as a balance in avoiding running out of passes versus an arbitrary assertion such as in the linux kernel. If we are less concerned about the latter then I think we can simplify to: 
```
if  (os->size > target->getThunkSectionSpacing() && (isdLimit - isdBase > 4096))
```
This will get both of the problems we know about, but may be too specific to the linux kernel assert.


================
Comment at: lld/ELF/Relocations.cpp:1781
+      // rounding the size up.
+      ts->alignment = 4;
+      ts->roundUpSizeForErrata = true;
----------------
MaskRay wrote:
> I wonder whether we should use 4 unconditionally https://reviews.llvm.org/D72819 .
> 
> Most thunks are 16 bytes. I don't know why we selected 8, instead of 4 or 16 in D29129.
> (If 8 were for locality/cache line considerations, 16 would be a better choice.)
Agreeed. It was probably my fault for introducing it in the first place. I didn't have a good reason for it for Arm/AArch64 but didn't know about other targets so I used config->wordsize.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72344/new/

https://reviews.llvm.org/D72344