1570597 - mingw clang builds don't handle delayload correctly

Status: RESOLVED FIXED

(Firefox Build System :: General: Unsupported Platforms, defect)

Description

[Jacek Caban]

I noticed that clang builds don't handle delayload correctly. They pass delayload flag to the linker like: -DELAYLOAD:crypt32.dll. This is not the right way. It happens to not cause an error because it's treated just like any other -D argument (so it's like #define). lld-link supports delay loading, so changing that to use -Wl,-Xlink=-delayload:crypt32.dll should be enough to get it right.

Comment 1

[Tom Ritter [:tjr] (OOTO until mid-August)]

Interesting. I can fix this up easily I expect but now I'm curious what the purpose of this is...

Comment 2

[Jacek Caban]

Usual reasons for delay loading is faster load time and saving resources in cases when libraries are not really used in runtime. I've seen cases where the code may be sensitive to such subtle differences (although it wasn't Mozilla code base), so since clang-cl uses delay load, it's safer to use it in clang builds as well.

Anyway, it's not really important, but nice to have.

Comment 3

[Jacek Caban]

LLVM now supports nicer command line support:
https://reviews.llvm.org/D65728

Comment 4

[Tom Ritter [:tjr] (OOTO until mid-August)]

Attachment: Bug 1570597 - Pass the -delayload flag to lld correctly for MinGW builds r?#build

Comment 5

[Tom Ritter [:tjr] (OOTO until mid-August)]

To use the new command line syntax I'd have to backport a patch to clang and this is much simpler. This look good to you Jacek?

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=022fa2fe098615dcd98b6e415d39e5c1c3d26bb9 (ignore the Fission failures)

Comment 6

[Jacek Caban]

Yes, it looks good to me.

Comment 7

[Tom Ritter [:tjr] (OOTO until mid-August)]

[Tracking Requested - why for this release]: Only affects the mingw build; would be nice to put in ESR for Tor.

Comment 8

[Tom Ritter [:tjr] (OOTO until mid-August)]

Comment on attachment 9088147 [details]
Bug 1570597 - Pass the -delayload flag to lld correctly for MinGW builds r?#build

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a small performance improvement on browser start time that would be good to give to Tor.
• User impact if declined: Tor would either need to carry the patch or be a little slower.
• Fix Landed on Version: 70
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Only affects the MinGW build.
• String or UUID changes made by this patch:

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/59422c064fd1
Pass the -delayload flag to lld correctly for MinGW builds r=firefox-build-system-reviewers,mshal

Comment 10

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/59422c064fd1

Comment 11

[Julien Cristau [:jcristau]]

Comment on attachment 9088147 [details]
Bug 1570597 - Pass the -delayload flag to lld correctly for MinGW builds r?#build

build improvement for mingw, approved for 68.2.

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-esr68/rev/4fab973331ec