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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

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.



================
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
----------------
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.




================
Comment at: COFF/Writer.cpp:408
+          // forward - is this a concern?
+          Chunks.insert(Chunks.begin() + ThunkInsertionSpot, Thunk->getChunk());
+          ThunkInsertionSpot++;
----------------
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.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D51089