[PATCH] D64288: [LLD] [COFF] Share the tail in delayimport symbol thunks

Mon Jul 8 23:05:11 PDT 2019

mstorsjo marked an inline comment as done.
mstorsjo added a comment.

Any other concerns with the patch, or is it ok as is?

================
Comment at: COFF/DLL.cpp:195
+    0x48, 0x8D, 0x05, 0, 0, 0, 0,       // lea     rax, [__imp_<FUNCNAME>]
+    0xE9, 0, 0, 0, 0,                   // jmp     __tailMerge_<lib>
+};
----------------
ruiu wrote:
> rnk wrote:
> > mstorsjo wrote:
> > > ruiu wrote:
> > > > Intel recommends jump targets being aligned to 16 bytes. Does this jump target aligned to the boundaries? Last time I tried, I observed a non-negligible difference between unaligned and aligned jump targets.
> > > I guess that can be done by just marking the tail chunk as 16 byte aligned? Aligning the thunks themselves feels wasteful though.
> > As far as performance is concerned, each delay load thunk runs once, so I would leave them unaligned to optimize for size.
> > 
> > It might end up mattering for CFI, actually. There's some assumption there that indirect branch targets (which these thunks are) are aligned. I forget the details, though.
> > 
> > I'm also wondering if we shouldn't mark the delay load thunks as hotpatchable so they are affected by `/functionpadmin:`. It seems separable though.
> Ah, good point. If they are executed only once, there's no point to align them to trade speed with binary size.
Re CFI, we don't generate any for the thunk right now either. And making them respect `/functionpadmin:` also seems orthogonal to this patch.

Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64288/new/

https://reviews.llvm.org/D64288