[PATCH] D52156: [LLD] [COFF] Alternative ARM range thunk algorithm

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added a comment.

In https://reviews.llvm.org/D52156#1237627, @peter.smith wrote:

> My best suggestion for a test that will need exceed the margin is to use input sections with high alignment requirements. If these are on a natural boundary prior to adding thunks then adding a thunk will result in a large amount of bytes inserted to maintain section alignment. As long as these padding bytes are in between the source and destination of a branch and exceed the margin then a test case can be constructed without needing to insert hundreds of Thunks.


The biggest section alignment that COFF supports is 8 KB. Using that probably can reduce the testcase to requiring a few dozen of thunks, and that's probably manageable to write a testcase for. Thanks for the idea! I looked into alignments before, for creating the whole gap to cause the need for thunks, but it wasn't enough as you can't do a 16 MB alignment.



================
Comment at: COFF/Writer.cpp:369-371
+  int64_t Diff = S - P - 4;
+  Diff = abs(Diff);
+  Diff += Margin;
----------------
ruiu wrote:
> I'd write these three lines one line.
Will do


================
Comment at: COFF/Writer.cpp:385
+static std::pair<Defined *, bool>
+getThunk(DenseMap<uint64_t, Defined *> &LastThunks, Defined *Target, uint64_t P,
+         uint16_t Type, int Margin) {
----------------
ruiu wrote:
> The relationship between section and its thunk is 1:N. I believe that's why you are using a map to manage thunk -> symbol mapping.
> 
> However, I think you visit sections in the increasing address order, so if the last thunk is not reachable, no other thunks are reachable. Can you use the fact?
> The relationship between section and its thunk is 1:N. I believe that's why you are using a map to manage thunk -> symbol mapping.

For every target Symbol, there can be N thunks that point to it, yes.

> However, I think you visit sections in the increasing address order, so if the last thunk is not reachable, no other thunks are reachable. Can you use the fact?

Yes - I'm using exactly this fact. This map allows me to look up the last thunk pointing to each specific RVA. (In the previous version, I had something similar to `DenseMap<uint64_t, vector<Defined *>>`, to allow finding all thunks for a specific target. But as you say, as we progress linearly we only need to keep track of the last thunk for each target.)


================
Comment at: COFF/Writer.cpp:485
+    return;
+  size_t OrigNumChunks = 0, NumChunks = 0;
+  for (OutputSection *Sec : OutputSections) {
----------------
peter.smith wrote:
> Given that we expect most programs not to need any thunks at all then would moving the verify part ahead of createThunks make sense? Something like
> 
> ```
> while (true) {
>   if (verifyRanges)
>     // were done exit
>   else {
>     if pass 0 initialise margin else double it.
>     create thunks()
>     assignAddresses()
>     ++ThunkPasses;
>   }
> }
> ```
> That would mean one more call to verifyRanges if thunks are needed, but that is expected to be the exception.
> 
> 
>     
That sounds like a good idea. That also avoids the case of needlessly adding thunks for the case when the margin would cause them to be added, even if the total image size still is slightly below the maximum branch range.


================
Comment at: COFF/Writer.cpp:491
+  int ThunkPass = 0;
+  bool AddressesChanged;
+  int Margin = 1024 * 100;
----------------
peter.smith wrote:
> Can you move this to line 500?
Oh, indeed - I don't need it after the loop any longer.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D52156