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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Sorry for the belated response. I was thinking of this patch for a while.

Every time I saw the code of thunk range extension, I wonder if we really need this multi-pass algorithm which add thunks iterative on each pass. I believe in almost all cases, the algorithm finishes on the first iteration, if we allow a very small margin when determining "reachability". As long as a margin is small, size increase by allowing a margin should be negligible.

For pathetic executables for which we need to generate tons of thunks (which enlarges distance between callers and callees and thus need multiple passes with the current algorithm), we can simply discard everything that we made in the previous iteration instead of keeping them, double the margin, and then try again from scratch. In practice, I believe that fallback doesn't happen too frequently.

What do you think of the algorithm? If it works, I prefer that algorithm because discarding everything and redo with a larger margin is simpler than keeping thunks created in previous passes.



================
Comment at: COFF/Chunks.cpp:658
+// A Thumb2, PIC, non-interworking range extension thunk.
+const uint8_t RangeExtensionThunkARMData[] = {
+    0x40, 0xf2, 0x00, 0x0c, // P:  movw ip,:lower16:S - (P + (L1-P) + 4)
----------------
This variable name seems a bit too long to my taste; I'd name `ArmThunk` or something like that, and that should be fine as long as this is a file-scope variable.


================
Comment at: COFF/Chunks.cpp:824-831
   for (SectionChunk *C : Sections) {
     if (!C->isLive())
       continue;
     size_t Off = Builder.getOffset(toStringRef(C->getContents()));
     C->setOutputSection(Out);
     C->setRVA(RVA + Off);
     C->OutputSectionOff = OutputSectionOff + Off;
----------------
This part is not guarded by `Finalized` -- is that intended?


================
Comment at: COFF/PDB.cpp:775
          "debug sections should not be in output sections");
+  DebugChunk->readRelocTargets();
   DebugChunk->writeTo(Buffer);
----------------
If you change this line to

  cast<SectionChunk>(DebugChunk)->readRelocTargets();

then can you make `readRelocTargets` a non-virtual member function that belongs to `SectionChunk`?


================
Comment at: COFF/Writer.cpp:365
+  uint64_t TargetChunkRVA = TargetChunk ? TargetChunk->getRVA() : 0;
+  // A unique representation of the target address of a Defined symbol,
+  // stable across relayouts. This is represented as the base Chunk* with
----------------
Please insert a blank line before a multi-line comment.


https://reviews.llvm.org/D51089