Closed Bug 1336978 Opened 3 years ago Closed 2 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

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!
See Also: → 1341525
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
https://hg.mozilla.org/mozilla-central/rev/342812d23eb9
Status: NEW → RESOLVED
Closed: 2 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.