Use lld-link for official WIndows builds

RESOLVED FIXED in Firefox 63

Status

enhancement
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: dmajor, Assigned: dmajor)

Tracking

(Blocks 1 bug)

Trunk
mozilla63
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment)

With the clang-cl transition going smoothly so far, the next step is to use lld-link as our linker.
Posted patch lld-linkSplinter Review
Assignee: nobody → dmajor
Attachment #8991749 - Flags: review?(gps)
Comment on attachment 8991749 [details] [diff] [review]
lld-link

Review of attachment 8991749 [details] [diff] [review]:
-----------------------------------------------------------------

I'll be honest: I'm not sure of the technical implications of this change. glandium would be a better reviewer on that front.

But, you've been doing great work on the clang front and I trust that if you are asking for review everything is expected to mostly work and not cause major problems. If I'm wrong and this isn't a rubber stamp review, then please ask glandium for review. Regardless, \o/
Attachment #8991749 - Flags: review?(gps) → review+
The riskiest thing about lld-link is going to be debug information, but with bug 1458109 finally sorted out I'm feeling much more confident in its quality.
Just to be safe I don't want the first Nightlies to go unmonitored over the weekend. I'll land this on Sunday for the Monday morning builds.
Not sure how important this is, but Microsoft folks alerted us to an LLD flaw in CFI implementation that caused a crash for lld-linked Chrome in the MS x86 emulator, which is used to run win32 programs on arm64 devices:

https://bugs.chromium.org/p/chromium/issues/detail?id=857012

Comment 6

9 months ago
I think that's very likely unrelated. This here isn't on an arm device, and that bug is about /cfguard not about stack probes too.

Comment 7

9 months ago
(Also, that bug is fixed if your lld is built at at least llvm r335864)

Comment 8

9 months ago
Sorry, ignore comment 6, I thought you had commented on bug 1475431. Comment 7 is relevant though: That bug has been fixed in lld since r335864
> That bug has been fixed in lld since r335864

Which we already have, hooray! https://searchfox.org/mozilla-central/source/build/build-clang/clang-win64.json

Comment 10

9 months ago
Pushed by dmajor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5650748284b
Use lld-link for official Windows builds. r=gps

Comment 11

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b5650748284b
Status: NEW → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Are there any performance improvements caused by this?
Flags: needinfo?(dmajor)
(In reply to Jens Hausdorf from comment #12)
> Are there any performance improvements caused by this?

Build times have reduced:

== Change summary for alert #14358 (as of Sun, 15 Jul 2018 22:02:01 GMT) ==

Improvements:

  7%  build times windows2012-32-noopt debug taskcluster-c4.4xlarge     1,554.22 -> 1,451.30
  5%  build times windows2012-64-noopt debug taskcluster-c4.4xlarge     1,592.99 -> 1,511.48

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=14358
Flags: needinfo?(dmajor)
Depends on: 1484406
Depends on: 1486759
You need to log in before you can comment on or make changes to this bug.