Closed
Bug 1336978
Opened 8 years ago
Closed 7 years ago
Add support of lld by adding a configure option --enable-linker='bfd', 'gold', 'lld', 'other'
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox54 affected, firefox57 fixed)
RESOLVED
FIXED
mozilla57
People
(Reporter: Sylvestre, Assigned: Sylvestre)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
lld provides much better performances than ld.
As it will ship with clang 4.0 as first stable version, I think we should add an option.
Note that this requires clang >= 3.8 (older versions doesn't have the option -fuse-ld=lld).
Snapshot packages of lld are available for Debian and Ubuntu on http://apt.llvm.org/
On an old workstation (8 cpu, 32g of RAM and SSD)
linker libxul.so with LD:
real 0m22,170s
user 0m19,660s
sys 0m1,976s
with LLD:
real 0m3,147s
user 0m4,904s
sys 0m1,276s
I tried also with a more recent system and it is very similar: 0m17,772s / 0m2,745s
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → sledru
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 2•8 years ago
|
||
Of course, Firefox starts. about:buildconfig
----
Configure options
--enable-debug CC=clang-4.0 CXX=clang++-4.0 MAKE=/usr/bin/make --disable-gold --enable-lld
----
Comment 3•8 years ago
|
||
mozreview-review |
Comment on attachment 8833975 [details]
Bug 1336978 - Add support of lld by adding a configure option --enable-linker='bfd', 'gold', 'lld', 'other'
https://reviewboard.mozilla.org/r/110078/#review111432
I'd rather not do this that way. I'd rather we have one option for both gold and lld. Like --enable-linker={lld,gold}. For compatibility, we may want --enable-gold to mean --enable-linker=gold. And we should move all this to python configure, because doing multiple options in autoconf is messy. BTW, I think all the compilers we support now have the -fuse-ld option, so we can also drop the awful way we set gold up to just use that.
::: build/autoconf/compiler-opts.m4:155
(Diff revision 1)
> + _SAVE_LDFLAGS=$LDFLAGS
> + LDFLAGS="$LDFLAGS -fuse-ld=lld"
> + AC_TRY_LINK(,,AC_MSG_RESULT([yes])
> + [MOZ_PROGRAM_LDFLAGS="$MOZ_PROGRAM_LDFLAGS -fuse-ld=lld"],
> + AC_MSG_RESULT([no])
> + AC_MSG_ERROR([--enable-lld requires clang (>= 3.8) and the option -fuse-ld=lld.]))
Doesn't lld work with gcc? Why clang >= 3.8? I thought older versions supported -fuse-ld, isn't that the case?
::: build/autoconf/compiler-opts.m4:157
(Diff revision 1)
> + # Force the deactivation of gold when lld is going to be used
> + MOZ_FORCE_GOLD=
We avoid silently changing behavior that was explicitly requested. Python configure make that easier :)
Attachment #8833975 -
Flags: review?(mh+mozilla) → review-
Assignee | ||
Comment 4•8 years ago
|
||
mozreview-review |
Comment on attachment 8833975 [details]
Bug 1336978 - Add support of lld by adding a configure option --enable-linker='bfd', 'gold', 'lld', 'other'
https://reviewboard.mozilla.org/r/110078/#review115560
::: build/autoconf/compiler-opts.m4:155
(Diff revision 1)
> + _SAVE_LDFLAGS=$LDFLAGS
> + LDFLAGS="$LDFLAGS -fuse-ld=lld"
> + AC_TRY_LINK(,,AC_MSG_RESULT([yes])
> + [MOZ_PROGRAM_LDFLAGS="$MOZ_PROGRAM_LDFLAGS -fuse-ld=lld"],
> + AC_MSG_RESULT([no])
> + AC_MSG_ERROR([--enable-lld requires clang (>= 3.8) and the option -fuse-ld=lld.]))
-fuse-ld=lld is not accepted by gcc yet:
g++: error: unrecognized command line option ‘-fuse-ld=lld’; did you mean ‘-fuse-ld=bfd’?
::: build/autoconf/compiler-opts.m4:157
(Diff revision 1)
> + # Force the deactivation of gold when lld is going to be used
> + MOZ_FORCE_GOLD=
ok, thanks!
Assignee | ||
Comment 5•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8833975 [details]
Bug 1336978 - Add support of lld by adding a configure option --enable-linker='bfd', 'gold', 'lld', 'other'
https://reviewboard.mozilla.org/r/110078/#review111432
> Doesn't lld work with gcc? Why clang >= 3.8? I thought older versions supported -fuse-ld, isn't that the case?
fuse-ld=lld is not supported by gcc yet.
clang accepts lld as a value only from 3.8
Comment hidden (mozreview-request) |
Comment 7•8 years ago
|
||
mozreview-review |
Comment on attachment 8833975 [details]
Bug 1336978 - Add support of lld by adding a configure option --enable-linker='bfd', 'gold', 'lld', 'other'
https://reviewboard.mozilla.org/r/110078/#review117640
::: build/autoconf/compiler-opts.m4
(Diff revision 2)
> ## to typedef it to a C type with the same layout when the headers are included
> ## from C.
> _WARNINGS_CXXFLAGS="${_WARNINGS_CXXFLAGS} -Wno-unknown-warning-option -Wno-return-type-c-linkage"
> fi
>
> -if test -n "$DEVELOPER_OPTIONS"; then
You're leaving this out. It should be preserved. Which means DEVELOPER_OPTIONS needs to move to python configure first.
It would also be better if you moved the --enable-gold stuff to python configure in a separate patch, and /then/ add --enable-linker support.
::: build/moz.configure/toolchain.configure:982
(Diff revision 2)
> +
> +# Linker
> +# ==============================================================
> +# Choice between LLD / LD
> +js_option('--enable-linker', nargs=1,
> + choices=('bft', 'ld', 'gold', 'lld'),
s/bft/bfd/
We probably want some when= to make the option only available when the compiler is clang or gcc.
::: build/moz.configure/toolchain.configure:987
(Diff revision 2)
> + choices=('bft', 'ld', 'gold', 'lld'),
> + help='Select the linker')
> +
> +@deprecated_option('--enable-gold')
> +def select_linker(value):
> + return True
This breaks calling configure with an explicit --disable-gold. Also, the result is not really great if one does --enable-gold --enable-linker=lld for some reason.
It would be better to use an imply_option() such that --enable-gold implies --enable-linker=gold, which will then error out automatically if there's a conflicting --enable-linker=foo given.
::: build/moz.configure/toolchain.configure:998
(Diff revision 2)
> + linker = value[0]
> + else:
> + # Option --enable-gold has been used. Force gold
> + linker = 'gold'
> +
> + if linker == 'ld' or linker == "bfd":
if linker in ('ld', 'bfd')
... although, why have 2 values for bfd ld?
::: build/moz.configure/toolchain.configure:1000
(Diff revision 2)
> + # Option --enable-gold has been used. Force gold
> + linker = 'gold'
> +
> + if linker == 'ld' or linker == "bfd":
> + linker = "bfd"
> + set_define('LD_IS_BFD', 1)
you can't use set_define here. It needs to be at the top-level, like set_config, so you need a separate function for its value. Or to make this function return a namespace().
::: build/moz.configure/toolchain.configure:1003
(Diff revision 2)
> + if linker == 'ld' or linker == "bfd":
> + linker = "bfd"
> + set_define('LD_IS_BFD', 1)
> + elif linker == 'lld':
> + if c_compiler.type != 'clang':
> + die("lld requires clang (version >= 3.8)")
Is that actually true if using the -B trick, which is still needed to keep the current setup working? (which btw, means you should preserve it)
Attachment #8833975 -
Flags: review?(mh+mozilla) → review-
Assignee | ||
Updated•7 years ago
|
Summary: Add --enable-lld to link with lld instead of ld → Add support of lld by adding a configure option --enable-linker='bfd', 'gold', 'lld', 'other'
Assignee | ||
Updated•7 years ago
|
Blocks: linker-lld
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
Tested with:
ac_add_options --enable-linker=bfd
ac_add_options --enable-linker=other
ac_add_options --enable-linker=gold
ac_add_options --enable-linker=lld
ac_add_options --enable-gold
Not that the patch doesn't work yet (fails on the when=)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8833975 [details]
Bug 1336978 - Add support of lld by adding a configure option --enable-linker='bfd', 'gold', 'lld', 'other'
https://reviewboard.mozilla.org/r/110078/#review117640
> if linker in ('ld', 'bfd')
>
> ... although, why have 2 values for bfd ld?
Because I saw the two names for the same thing. Do you want me to remove one of them?
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•7 years ago
|
||
Assignee | ||
Comment 16•7 years ago
|
||
OK, I found the error.
find_program("ld.gold") doesn't return anything
now, why my patch is breaking that when it was working, this is a different story
Assignee | ||
Comment 17•7 years ago
|
||
Normal build are showing "checking for ld... bfd"
OK, get it:
enable_gnu_linker(enable_gold_option, ...)
enable_gold_option = True when it was False by default
Comment hidden (mozreview-request) |
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8833975 [details]
Bug 1336978 - Add support of lld by adding a configure option --enable-linker='bfd', 'gold', 'lld', 'other'
https://reviewboard.mozilla.org/r/110078/#review171596
I'm sure this can be improved and more generic. Like, supporting -B for lld when -fuse-ld=lld doesn't work, and support -fuse-ld=gold when supported. But let's keep that for a followup.
::: build/moz.configure/toolchain.configure
(Diff revision 8)
>
>
> option('--enable-gold',
> env='MOZ_FORCE_GOLD',
> - help='Enable GNU Gold Linker when it is not already the default',
> + help='Enable GNU Gold Linker when it is not already the default')
> - when=build_not_win_mac)
This when should be preserved. You just need to add the same when in the depends_if below.
Attachment #8833975 -
Flags: review?(mh+mozilla) → review+
Comment hidden (mozreview-request) |
Comment 21•7 years ago
|
||
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/342812d23eb9
Add support of lld by adding a configure option --enable-linker='bfd', 'gold', 'lld', 'other' r=glandium
Comment 22•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
Product: Core → Firefox Build System
Comment 23•7 years ago
|
||
Is there a way to accept things like "ac_add_options --enable-linker=lld-6.0" ?
Debian stable doesn't have lld (it shipped with llvm 3.8) and using the third-party repo from comment 0, the binary I get is called /usr/bin/lld-6.0.
My .mozconfig:
export CC=clang-6.0
export CXX=clang++-6.0
#ac_add_options --enable-linker=lld-6.0
Assignee | ||
Comment 24•7 years ago
|
||
Francois, please create a new bug to request the feature (I will have a look)
httsp://apt.llvm.org provides a meta package for lld.
if you install lld, it will install /usr/bin/lld which will be used by the compiler.
You need to log in
before you can comment on or make changes to this bug.
Description
•