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)

defect
Not set
normal

Tracking

(firefox54 affected, firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox54 --- affected
firefox57 --- fixed

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
Assignee: nobody → sledru
Depends on: 1336994, 1335324
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 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-
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!
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 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-
Depends on: 1351108
Depends on: 1351109
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'
Blocks: linker-lld
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=)
Depends on: 1384449
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?
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
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 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+
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
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Product: Core → Firefox Build System
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
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.

Attachment

General

Created:
Updated:
Size: