Page MenuHomePhabricator

[Driver] Add --ld-path= and deprecate -fuse-ld=/abs/path and -fuse-ld=rel/path
ClosedPublic

Authored by MaskRay on Jul 1 2020, 5:48 PM.

Details

Summary

Supersedes D80225. Add --ld-path= to avoid strange target specific
prefixes and make -fuse-ld= focus on its intended job: "linker flavor".

The way --ld-path= works is similar to "Command Search and Execution" in POSIX.1-2017 2.9.1 Simple Commands.

If --ld-path= specifies

  • an absolute path, the value specifies the linker.
  • a relative path without a path component separator (/), the value is searched using the -B, COMPILER_PATH, then PATH.
  • a relative path with a path component separator, the linker is found relative to the current working directory.

-fuse-ld= and --ld-path= can be composed, e.g. -fuse-ld=lld --ld-path=/usr/bin/ld.lld

The driver can base its linker option decision on the flavor -fuse-ld=, but it should not do fragile
flavor checking with --ld-path=.

Diff Detail

Unit TestsFailed

TimeTest
260 mslinux > Clang.Misc::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; diagtool list-warnings > /mnt/disks/ssd0/agent/llvm-project/build/tools/clang/test/Misc/Output/warning-flags.c.tmp 2>&1
9,590 mslinux > libomp.env::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/env/kmp_set_dispatch_buf.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/env/Output/kmp_set_dispatch_buf.c.tmp -lm -latomic && env KMP_DISP_NUM_BUFFERS=0 /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/env/Output/kmp_set_dispatch_buf.c.tmp
1,920 mslinux > libomp.worksharing/for::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/worksharing/for/kmp_set_dispatch_buf.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/worksharing/for/Output/kmp_set_dispatch_buf.c.tmp -lm -latomic && /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/worksharing/for/Output/kmp_set_dispatch_buf.c.tmp 7
550 mswindows > Clang.Misc::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; diagtool list-warnings > C:\ws\w6\llvm-project\premerge-checks\build\tools\clang\test\Misc\Output\warning-flags.c.tmp 2>&1

Event Timeline

MaskRay created this revision.Jul 1 2020, 5:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2020, 5:48 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I got pinged for my GCC -fuse-ld=path patch today. On GCC side, Martin Liška is fine with clang's choice if we can reach a reasonable consensus: https://gcc.gnu.org/pipermail/gcc-patches/2020-July/549236.html

Thank you for making this change. IIRC I introduced -fuse-ld= out of frustration with the inability to change the linker path in any way other than with a shell script on PATH. It was not a good design.

whitequark accepted this revision.Jul 1 2020, 5:56 PM
This revision is now accepted and ready to land.Jul 1 2020, 5:56 PM
keith added a subscriber: keith.Jul 1 2020, 6:04 PM
keith accepted this revision.Jul 1 2020, 6:18 PM

Awesome!

MaskRay updated this revision to Diff 275507.Jul 4 2020, 9:58 AM

Improve tests

MaskRay updated this revision to Diff 275517.Jul 4 2020, 12:05 PM

Add a -working-directory test

MaskRay edited the summary of this revision. (Show Details)

BTW, I just noticed recently that we have a -mlinker-version= flag, too, which is only used on darwin at the moment. That's another instance of "we need to condition behavior based on what linker we're invoking", but in this case, between multiple versions of apple's linker, rather than which brand of linker. That doesn't impact this directly, but just thought I'd mention it as it's in the same area of concern.

clang/lib/Driver/ToolChain.cpp
556–557

Shouldn't this use -B paths? Why do we want to ignore them?

MaskRay marked an inline comment as done.Jul 6 2020, 1:51 PM
MaskRay added inline comments.
clang/lib/Driver/ToolChain.cpp
556–557

If the value is relative, respect -Bprefix and COMPILER_PATH? (COMPILER is currently not well supported in clang IIUC)

MaskRay marked an inline comment as done.Jul 6 2020, 2:02 PM
MaskRay added inline comments.
clang/lib/Driver/ToolChain.cpp
556–557

D80660's need is an option relative to the current working directory...

MaskRay updated this revision to Diff 275830.Jul 6 2020, 2:30 PM
MaskRay retitled this revision from [Driver] Add -fld-path= and deprecate -fuse-ld=/abs/path and -fuse-ld=rel/path to [Driver] Add --ld-path= and deprecate -fuse-ld=/abs/path and -fuse-ld=rel/path.
MaskRay edited the summary of this revision. (Show Details)
MaskRay added a reviewer: phosek.

Rename -fld-path= to --ld-path=

gkm added a subscriber: gkm.Jul 9 2020, 9:15 AM
MaskRay updated this revision to Diff 278555.Jul 16 2020, 11:29 AM
MaskRay edited the summary of this revision. (Show Details)
MaskRay removed a subscriber: gkm.

Respect -B and COMPILER_PATH

MaskRay added a subscriber: gkm.Jul 16 2020, 11:32 AM
MaskRay added a comment.EditedJul 16 2020, 11:52 AM

I tweaked the following item in the last revision

a relative path without a path component separator (/), the value is searched using the -B, COMPILER_PATH, then PATH.

My communication with GCC folks is that they will be happy to adopt something like --ld-path. GCC -fuse-ld= searches for all of -B, COMPILER_PATH, then PATH.
This clang patch aligns with my proposed GCC patch (https://sourceware.org/pipermail/gcc-patches/2020-July/549386.html I need to tweak the option name but Martin said he would be happy with a clang resolution)

@jyknight @phosek Can I get a bit more approval power? :)

Regarding the name --ld-path:
https://sourceware.org/pipermail/gcc/2020-July/233089.html

-fFOO -> affect generated code (non-target-specific) or language feature

--ld-path does not fit well with the description, so --ld-path is probably fine. (And -fuse-ld actually starts to make sense... because we have code generation decision made by -fuse-ld=...)

This revision was automatically updated to reflect the committed changes.
ro added a subscriber: ro.Jul 22 2020, 5:03 AM

This patch introduced a couple of failures on the Solaris buildbots (Solaris/sparcv9 and Solaris/x86).

E.g.

FAIL: Clang :: Driver/env.c (12768 of 69452)
[...]
/vol/llvm/src/llvm-project/dist/clang/test/Driver/env.c:21:21: error: CHECK-LD-32-NOT: excluded string found in input
// CHECK-LD-32-NOT: warning:
                    ^
<stdin>:5:8: note: found here
clang: warning: '-fuse-ld=' taking a path is deprecated. Use '--ld-path=' instead
       ^~~~~~~~

For reasons explained in D84029, one needs to specify the Solaris linker, either via cmake -DCLANG_DEFAULT_LINKER (so far) or implicitly
as in that patch.

I strongly expect all other uses of CLANG_DEFAULT_LINKER to be broken similarly.