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

Thanks for the feedback! I'll fold this into the next iteration of the patch; I have a bunch of other improvements planned.



================
Comment at: COFF/Chunks.cpp:51-52
+void SectionChunk::finalizeContents() {
+  if (!Symbols.empty())
+    return;
+  for (const coff_relocation &Rel : Relocs)
----------------
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.


================
Comment at: COFF/Chunks.cpp:53-54
+    return;
+  for (const coff_relocation &Rel : Relocs)
+    Symbols.push_back(File->getSymbol(Rel.SymbolTableIndex));
+}
----------------
ruiu wrote:
> Since this vector can be very large, it is perhaps better to call `reserve()`.
Indeed, I'll make it use reserve in the next iteration.


================
Comment at: COFF/Chunks.cpp:424
+  size_t Counter = 0;
   for (const coff_relocation &Rel : Relocs) {
     uint8_t Ty = getBaserelType(Rel);
----------------
ruiu wrote:
> It is more straightforward to write this loop in as a plain old `for` loop instead of a range-based for loop with `Counter`.
Ok, will do.


================
Comment at: COFF/ICF.cpp:150
     }
+    // This doesn't use the potentially remapped Symbol, but at this point,
+    // we shouldn't have done any remapping yet (and using the original
----------------
ruiu wrote:
> I'd think this comment is not easy to understand if you don't know about this comment is in the context of ARM thunk support. Maybe we can just omit it?
> 
> Does ICF run after ARM thunk creation?
Yeah, it's probably best to omit the comment. ICF runs before the thunk creation.

The original reason for the comment was that I wanted to replace all uses of coff_relocation::SymbolTableIndex with the Symbols vector (outside the case when initializing the vector), to make things consistent, but it wasn't practical in all cases. In reality it mostly is necessary in SectionChunk::writeTo() and SectionChunk::getBaserels().


================
Comment at: COFF/Writer.cpp:367
+  Defined *D = dyn_cast_or_null<Defined>(Symtab->addSynthetic(
+      Saver.save("__thunk_" + Target->getName() + "_" + Twine(ThunkCounter++)),
+      C));
----------------
ruiu wrote:
> I don't think you need to pass this `ThunkCounter` around; I'd define this as a static local variable in this function.
I later figured out I didn't need to name the thunk symbol at all; I just create the DefinedSynthetic directly without a name, and don't add it to Symtab - just like we do with object file local symbols.


================
Comment at: COFF/Writer.cpp:370
+  if (!D)
+    fatal("Thunk collision with other symbol?");
+  TargetThunks.push_back(D);
----------------
ruiu wrote:
> Error messages should start with a lowercase letter.
> 
> Shouldn't this be an `assert`? If this can be triggered by a valid user input, this error message should contain more info as to what is the problem.
I managed to remove this error altogether by not adding the thunk symbols to the symbol table.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D51089