[PATCH] D51089: [LLD] [COFF] [RFC] Add support for creating range extension thunks for ARM

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added a comment.

In https://reviews.llvm.org/D51089#1209038, @peter.smith wrote:

> I've left a couple of comments where I think that there may be some unintentional inefficiencies, but as far as I can tell it looks like it will be correct. I suggest adding a lot more test cases as you go for things like thunk reuse.


Thanks for taking a look!

Yup, some more tests definitely would be good. Do you have any suggestions on how to test things like this efficiently without creating >16 MB binaries? The existing tests (that used to check for errors) just use absolute symbols as targets to make it fail. I see that the ELF tests either do padding with `.space` or huge alignment with `.balign` - I guess something like that would work here as well. And I see that the existing ELF tests also produce huge binaries (although they hopefully are stored sparsely).



================
Comment at: COFF/Writer.cpp:356
+         uint32_t EstimatedThunkRVA) {
+  std::vector<Defined *> &TargetThunks = ExistingThunks[Target->getRVA()];
+  // TODO: Since we create thunks linearly in the same order as we iterate
----------------
peter.smith wrote:
> Assuming getRVA() is the virtual address of the symbol, is Target->getRVA() stable between passes? Presumably if thunks are inserted then assignAddresses() may cause some symbols to change address? I'm not too familiar with the COFF code base so I could be missing something here.
> 
> If I'm right the reuse between passes may not work as well as it could do.
> 
> 
No, the RVAs aren't stable between passes, but we don't keep the map between passes either; it's a local variable in createThunks below.

I guess it would be useful to allow this to find a different thunk from the previous pass (that would also reduce the amount of changes in later passes, reducing the number of passes required before it converges), but that's not implemented (yet).


================
Comment at: COFF/Writer.cpp:408
+          // forward - is this a concern?
+          Chunks.insert(Chunks.begin() + ThunkInsertionSpot, Thunk->getChunk());
+          ThunkInsertionSpot++;
----------------
peter.smith wrote:
> In theory if you are iterating a fixed number (Chunks.size()) of the Chunks vector then inserting thunks into the Chunks vector in the same loop will mean that Chunks near the end may not be scanned for Thunks. Given that the algorithm will only terminate when 0 thunks are inserted you'll eventually scan all of them but it may cost you more passes than you would need if you inserted all Thunks in one go. I think you'll be unlikely to hit 10 passes without a contrived test case though.
Hmm, I'm not quite sure I understand what you mean here. You mean that since I'm adding more elements to the Chunks vector, I'd miss the last few ones that were pushed forward? The limit on the outer loop, on line 379, explicitly checks for Chunks.size(), so it will loop until the very end of the vector, even if Chunks grows meanwhile.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D51089