Page MenuHomePhabricator

[LLD] [COFF] Share the tail in delayimport symbol thunks
ClosedPublic

Authored by mstorsjo on Jul 6 2019, 12:47 PM.

Details

Summary

E.g. for x86_64, previously each symbol's thunk was 87 bytes. Now there's a 12 byte thunk per symbol, plus a shared 83 byte tail function.

This is similar to what both MS link.exe and GNU tools do for delay imports.

Diff Detail

Repository
rL LLVM

Event Timeline

mstorsjo created this revision.Jul 6 2019, 12:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2019, 12:48 PM
Herald added a subscriber: javed.absar. · View Herald Transcript
ruiu added a comment.Jul 8 2019, 12:33 AM

Nice!

COFF/DLL.cpp
195 ↗(On Diff #208277)

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.

mstorsjo marked an inline comment as done.Jul 8 2019, 8:25 AM
mstorsjo added inline comments.
COFF/DLL.cpp
195 ↗(On Diff #208277)

I guess that can be done by just marking the tail chunk as 16 byte aligned? Aligning the thunks themselves feels wasteful though.

rnk added inline comments.Jul 8 2019, 1:47 PM
COFF/DLL.cpp
195 ↗(On Diff #208277)

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.

ruiu added inline comments.Jul 8 2019, 10:11 PM
COFF/DLL.cpp
195 ↗(On Diff #208277)

Ah, good point. If they are executed only once, there's no point to align them to trade speed with binary size.

mstorsjo marked an inline comment as done.Jul 8 2019, 11:04 PM

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

COFF/DLL.cpp
195 ↗(On Diff #208277)

Re CFI, we don't generate any for the thunk right now either. And making them respect /functionpadmin: also seems orthogonal to this patch.

rnk accepted this revision.Jul 10 2019, 1:13 PM

lgtm, and I think Rui approves as well.

This revision is now accepted and ready to land.Jul 10 2019, 1:13 PM
ruiu added a comment.Jul 10 2019, 6:36 PM

Yes, as long as Reid is fine, I'm fine with this patch.

This revision was automatically updated to reflect the committed changes.