1455767 - --enable-gold doesn't work with GCC ≥7

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I noticed my builds on Debian unstable were using BFD ld, and tried to configure with --enable-gold, but:

0:01.20 DEBUG: Executing: `/usr/lib/ccache/gcc -std=gnu99 -print-prog-name=ld.gold`
0:01.20 DEBUG: Executing: `/usr/lib/ccache/gcc -std=gnu99 -B /home/jld/src/obj.gecko-dev/obj-x86_64-pc-linux-gnu/build/unix/gold -Wl,--version`
0:01.20 ERROR: Could not find gold

The first print-prog-name command just prints "ld.gold" (if given "ld" it prints an absolute path).  The second command, I set up the .../build/unix/gold/ld symlink and ran it manually under strace; GCC doesn't look for ld in that directory, but it does look for collect2 (and doesn't find it); GCC runs collect2 which runs ld.

This *does* work with GCC 6.4.0 or with Clang, but not GCC 7.3.0 or a GCC 8 build dated 2018-04-14.  Both GCC 6 and 7 pass the -B path to collect2 as a -L option (and nowhere else among its arguments).

However, skipping the -B stuff and running gcc with "-fuse-ld=gold" seems to work on all versions.

Comment 1

[Mike Hommey [:glandium]]

Sylvestre, do you remember what versions of gcc/clang support -fuse-ld? Since we bumped to gcc 6, maybe we can use -fuse-ld=gold everywhere now?

Comment 2

[Sylvestre Ledru [:Sylvestre]]

for clang, it is supported since 3.8.
for gcc, at least 4.7
let me propose a patch

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1455767 - As we have gcc 6 as requirement, use -fuse-ld everywhere instead of the -B trick

Review commit: https://reviewboard.mozilla.org/r/239982/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/239982/

Comment 4

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8971181 [details]
Bug 1455767 - As we have gcc 6 as requirement, use -fuse-ld everywhere instead of the -B trick

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239982/diff/1-2/

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8971181 [details]
Bug 1455767 - As we have gcc 6 as requirement, use -fuse-ld everywhere instead of the -B trick

https://reviewboard.mozilla.org/r/239982/#review246612

::: build/moz.configure/toolchain.configure:1527
(Diff revision 2)
> -        os.symlink(goldFullPath, os.path.join(targetDir, 'ld'))
>  
> -        linker = ['-B', targetDir]
> +    if (enable_gold_option or developer_options) and linker_name != 'bfd':
> -        cmd = cmd_base + linker + version_check
>          if 'GNU gold' in check_cmd_output(*cmd).decode('utf-8'):
>              # We have detected gold, will build with the -B workaround

This comment needs an update.

::: build/moz.configure/toolchain.configure:1571
(Diff revision 2)
>           extra_toolchain_flags, when=is_linker_option_enabled)
>  @checking('for linker', lambda x: x.KIND)
>  def select_linker(linker, c_compiler, developer_options, build_env, toolchain_flags):
>      linker = linker[0] if linker else 'other'
>      if linker in ('gold', 'bfd', 'other'):
>          return enable_gnu_linker(linker == 'gold', c_compiler, developer_options,

With all the code you're removing from enable_gnu_linker, it seems it would become simpler if you merged everything in this function.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8971181 [details]
Bug 1455767 - As we have gcc 6 as requirement, use -fuse-ld everywhere instead of the -B trick

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239982/diff/2-3/

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8971181 [details]
Bug 1455767 - As we have gcc 6 as requirement, use -fuse-ld everywhere instead of the -B trick

https://reviewboard.mozilla.org/r/239982/#review248092

::: build/moz.configure/toolchain.configure:1528
(Diff revision 3)
> +    if linker in ('gold', 'bfd', 'other'):
> +                # Used to check the kind of linker
> +        linker, cmd = get_linker_fuse_syntax(c_compiler, linker)

You're doing:

if linker in ('gold', 'bfd', 'other'):
    linker, cmd = get_linker_fuse_syntax(c_compiler, linker)
(...)
if linker == 'lld':
    lld, cmd = get_linker_fuse_syntax(c_compiler, linker)

And that covers all the possible values of linker, so, you could move the get_linker_fuse_syntax call before the tests. And even better, inline it.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8971181 [details]
Bug 1455767 - As we have gcc 6 as requirement, use -fuse-ld everywhere instead of the -B trick

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239982/diff/3-4/

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 8971181 [details]
Bug 1455767 - As we have gcc 6 as requirement, use -fuse-ld everywhere instead of the -B trick

https://reviewboard.mozilla.org/r/239982/#review248142

::: build/moz.configure/toolchain.configure:1516
(Diff revision 4)
> -    # Used to check the kind of linker
> +    def get_linker_fuse_syntax(c_compiler, linker):
> +        # Generate two commands:
> +        # - The call to the version of the linker to know which one we are actually getting
> +        # - The call to the compiler with the right syntax to call the linker
> -    version_check = ['-Wl,--version']
> +        version_check = ['-Wl,--version']
> -    cmd_base = c_compiler.wrapper + [c_compiler.compiler] + c_compiler.flags
> +        cmd_base = c_compiler.wrapper + [c_compiler.compiler] + c_compiler.flags
> -    if toolchain_flags:
> -        cmd_base += toolchain_flags
> +        compiler_arg = ["-fuse-ld=" + linker] if linker != "other" else []
> +        cmd = cmd_base + compiler_arg + version_check
> +        return compiler_arg, cmd

It seems rather pointless to keep this as a separate function.

::: build/moz.configure/toolchain.configure:1533
(Diff revision 4)
> -        if os.path.exists(targetDir):
> -            shutil.rmtree(targetDir)
> -        os.makedirs(targetDir)
> -        os.symlink(goldFullPath, os.path.join(targetDir, 'ld'))
> +    # Used to check the kind of linker
> +    linker, cmd = get_linker_fuse_syntax(c_compiler, linker)
> +    if toolchain_flags:
> +        cmd += toolchain_flags
>  
> -        linker = ['-B', targetDir]
> +    if (linker == 'gold' or developer_options) and linker != 'bfd':

when you reach here, you've overwritten linker with ["-fuse-ld=..."], so this can't work.

::: build/moz.configure/toolchain.configure:1566
(Diff revision 4)
> -        else:
> +    else:
> +        if linker == 'lld':

elif

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8971181 [details]
Bug 1455767 - As we have gcc 6 as requirement, use -fuse-ld everywhere instead of the -B trick

https://reviewboard.mozilla.org/r/239982/#review248092

> You're doing:
> 
> if linker in ('gold', 'bfd', 'other'):
>     linker, cmd = get_linker_fuse_syntax(c_compiler, linker)
> (...)
> if linker == 'lld':
>     lld, cmd = get_linker_fuse_syntax(c_compiler, linker)
> 
> And that covers all the possible values of linker, so, you could move the get_linker_fuse_syntax call before the tests. And even better, inline it.

Bien vu!
merci beaucoup!

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8971181 [details]
Bug 1455767 - As we have gcc 6 as requirement, use -fuse-ld everywhere instead of the -B trick

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239982/diff/4-5/

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8971181 [details]
Bug 1455767 - As we have gcc 6 as requirement, use -fuse-ld everywhere instead of the -B trick

https://reviewboard.mozilla.org/r/239982/#review248478

I think at some point we should write a test case for all this.

::: build/moz.configure/toolchain.configure:1540
(Diff revision 5)
> -        elif enable_gold_option:
> +        if linker == 'gold':
>              die('Could not find gold')
>          # Else fallthrough.
>  
> -    cmd = cmd_base + version_check
>      cmd_output = check_cmd_output(*cmd).decode('utf-8')

Food for followup: this could move before all the checks, to avoid it running twice when gold is not found in the default case.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8971181 [details]
Bug 1455767 - As we have gcc 6 as requirement, use -fuse-ld everywhere instead of the -B trick

https://reviewboard.mozilla.org/r/239982/#review248478

Do you have examples on how we do that currently?

> Food for followup: this could move before all the checks, to avoid it running twice when gold is not found in the default case.

Sure, I will open a follow up bug about that.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7c7118c12864
As we have gcc 6 as requirement, use -fuse-ld everywhere instead of the -B trick r=glandium

Comment 15

[Stefan Hindli [:stefan_hindli]]

Backed out changeset 7c7118c12864 (bug 1455767) for failures in /python/mozbuild/mozbuild/test/configure/lint.py on a CLOSED TREE

Problematic push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7c7118c128642433047df82ec98ca480e0b33176&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
Failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=177621933
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=177621933&repo=autoland&lineNumber=35569
Backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7858e8a22a6835142fc7aab232d1455c5eb8f5ea&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified

[task 2018-05-09T09:02:12.560Z] 09:02:12     INFO - ============================= test session starts ==============================
[task 2018-05-09T09:02:12.560Z] 09:02:12     INFO - platform linux2 -- Python 2.7.9, pytest-3.1.3, py-1.4.34, pluggy-0.4.0 -- /builds/worker/workspace/build/src/obj-firefox/_virtualenvs/init/bin/python
[task 2018-05-09T09:02:12.560Z] 09:02:12     INFO - rootdir: /builds/worker/workspace/build/src/python/mozbuild, inifile:
[task 2018-05-09T09:02:12.560Z] 09:02:12     INFO - collecting ... collected 6 items
[task 2018-05-09T09:02:12.560Z] 09:02:12  WARNING - ../python/mozbuild/mozbuild/test/configure/lint.py::Lint::test_browser TEST-UNEXPECTED-FAIL
[task 2018-05-09T09:02:12.561Z] 09:02:12  WARNING - ../python/mozbuild/mozbuild/test/configure/lint.py::Lint::test_embedding_ios TEST-UNEXPECTED-FAIL
[task 2018-05-09T09:02:12.562Z] 09:02:12  WARNING - ../python/mozbuild/mozbuild/test/configure/lint.py::Lint::test_extensions TEST-UNEXPECTED-FAIL
[task 2018-05-09T09:02:12.563Z] 09:02:12  WARNING - ../python/mozbuild/mozbuild/test/configure/lint.py::Lint::test_js TEST-UNEXPECTED-FAIL
[task 2018-05-09T09:02:12.563Z] 09:02:12  WARNING - ../python/mozbuild/mozbuild/test/configure/lint.py::Lint::test_memory TEST-UNEXPECTED-FAIL
[task 2018-05-09T09:02:12.564Z] 09:02:12  WARNING - ../python/mozbuild/mozbuild/test/configure/lint.py::Lint::test_mobile_android TEST-UNEXPECTED-FAIL
[task 2018-05-09T09:02:12.565Z] 09:02:12     INFO - =================================== FAILURES ===================================

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8971181 [details]
Bug 1455767 - As we have gcc 6 as requirement, use -fuse-ld everywhere instead of the -B trick

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239982/diff/5-6/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a42df29b0d3d
As we have gcc 6 as requirement, use -fuse-ld everywhere instead of the -B trick r=glandium

Comment 18

[Sylvestre Ledru [:Sylvestre]]

the error was caused by a parameter being useless (which was true)
the interdiff is pretty clear:
https://reviewboard.mozilla.org/r/239982/diff/5-6/

Comment 19

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/a42df29b0d3d