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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: COFF/Chunks.cpp:51-52
+void SectionChunk::finalizeContents() {
+  if (!Symbols.empty())
+    return;
+  for (const coff_relocation &Rel : Relocs)
----------------
mstorsjo wrote:
> ruiu wrote:
> > Can this happen?
> I don't think so - perhaps I should make it an assert. I had to insert a call to finalizeContents() in relocateDebugChunk() in PDB.cpp though, so I wanted to make sure.
Actually, yes, it does happen. `finalizeContents` gets called by `assignAddresses`, which gets called repeatedly when relayouting after adding thunks. (After realizing this, I had to make sure MergeChunk::finalizeContents works properly for this case as well.)

The alternative would be to add another callback to Chunk, which we'd call just once, when all symbols are available.


================
Comment at: COFF/Writer.cpp:360
+  // the existing thunks?
+  for (Defined *Sym : TargetThunks) {
+    if (isInRange(Type, Sym->getRVA(), P))
----------------
mstorsjo wrote:
> ruiu wrote:
> > I don't know if your above comment is true, but if the one this loop is looking for is likely at the end of the vector, I'd search in the reverse order. I.e.
> > 
> >   for (Defined *Sym : llvm::reverse(TargetThunks))
> That's a nice idea. I'm changing code to keep the mapping of existing thunks stable across passes, so then the comment doesn't apply quite as much as before. I tested this and it didn't really save any measurable runtime, but it might be worthwhile anyway, as long as we iterate through the whole vector.
In practice with the case I'm testing, TargetThunks will only have 0 or 1 members, so it doesn't really matter much, but in general I guess it could be useful. (My testcase produces a 46 MB clang.exe, so it's only roughly twice as large as the branch range which is 16 MB.)


================
Comment at: COFF/Writer.cpp:406
+        if (WasNew && Thunk) {
+          // TODO: .insert() in a std::vector will move all later elements
+          // forward - is this a concern?
----------------
ruiu wrote:
> Nesting is too deep. Please consider splitting to multiple functions.
It's a bit hard to split this part to a separate method since it touches almost every single local variable from this method, but I can easily change the `if (!isInRange())` into an `if (isInRange()) continue;` to reduce the nesting a little.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D51089