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

[MaskRay]

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=.

[MaskRay]

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

[whitequark]

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.

[keith]

Awesome!

[MaskRay]

Improve tests

[MaskRay]

Add a -working-directory test

[jyknight]

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]

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]

clang/lib/Driver/ToolChain.cpp

556–557

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

[MaskRay]

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

[MaskRay]

Respect -B and COMPILER_PATH

[MaskRay]

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=...)

[ro]

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.

[thakis]

LinkerIsLLD isn't getting set when --ld-path is used. (It also isn't getting set when using -fuse-ld= with an absolute path.)

How do you imagine using an absolute path and telling clang "the linker is lld, you can assume it's built at the same rev as you" in this new world should look like? Pass both -fuse-ld=lld and --ld-path=path/to/lld?

[MaskRay]

In D83015#3792397, @thakis wrote:

LinkerIsLLD isn't getting set when --ld-path is used. (It also isn't getting set when using -fuse-ld= with an absolute path.)

How do you imagine using an absolute path and telling clang "the linker is lld, you can assume it's built at the same rev as you" in this new world should look like? Pass both -fuse-ld=lld and --ld-path=path/to/lld?

Yes, specify both options. -fuse-ld= specifies the flavor while --ld-path= specifies a path the Clang Driver does not want to detect.

[thakis]

Yes, specify both options. -fuse-ld= specifies the flavor while --ld-path= specifies a path the Clang Driver does not want to detect.

Thanks, made D134063 for that.

clang/include/clang/Basic/DiagnosticDriverKinds.td

 def err_drv_unsupported_linker : Error<"unsupported value '%0' for -linker option">;
 def err_drv_defsym_invalid_format : Error<"defsym must be of the form: sym=value: %0">;
 def err_drv_defsym_invalid_symval : Error<"Value is not an integer: %0">;
 def warn_drv_msvc_not_found : Warning<
   "unable to find a Visual Studio installation; "
   "try running Clang from a developer command prompt">,
   InGroup<DiagGroup<"msvc-not-found">>;
 
+def warn_drv_use_ld_non_word : Warning<
+  "'-fuse-ld=' taking a path is deprecated. Use '--ld-path=' instead">;
+
 def warn_drv_fine_grained_bitfield_accesses_ignored : Warning<
   "option '-ffine-grained-bitfield-accesses' cannot be enabled together with a sanitizer; flag ignored">,
   InGroup<OptionIgnored>;
 
 def note_drv_verify_prefix_spelling : Note<
   "-verify prefixes must start with a letter and contain only alphanumeric"
   " characters, hyphens, and underscores">;
 

clang/include/clang/Driver/Options.td

multiclass BooleanFFlag<string name> {
   def fno_#NAME : Flag<["-"], "fno-"#name>;
 }
 
 defm : BooleanFFlag<"keep-inline-functions">, Group<clang_ignored_gcc_optimization_f_Group>;
 
 def fprofile_dir : Joined<["-"], "fprofile-dir=">, Group<f_Group>;
 
 def fuse_ld_EQ : Joined<["-"], "fuse-ld=">, Group<f_Group>, Flags<[CoreOption]>;
+def ld_path_EQ : Joined<["--"], "ld-path=">;
 
 defm align_labels : BooleanFFlag<"align-labels">, Group<clang_ignored_gcc_optimization_f_Group>;
 def falign_labels_EQ : Joined<["-"], "falign-labels=">, Group<clang_ignored_gcc_optimization_f_Group>;
 defm align_loops : BooleanFFlag<"align-loops">, Group<clang_ignored_gcc_optimization_f_Group>;
 def falign_loops_EQ : Joined<["-"], "falign-loops=">, Group<clang_ignored_gcc_optimization_f_Group>;
 defm align_jumps : BooleanFFlag<"align-jumps">, Group<clang_ignored_gcc_optimization_f_Group>;
 def falign_jumps_EQ : Joined<["-"], "falign-jumps=">, Group<clang_ignored_gcc_optimization_f_Group>;
 

clang/lib/Driver/ToolChain.cpp

std::string ToolChain::GetFilePath(const char *Name) const {
   return D.GetFilePath(Name, *this);
 }
 
 std::string ToolChain::GetProgramPath(const char *Name) const {
   return D.GetProgramPath(Name, *this);
 }
 
 std::string ToolChain::GetLinkerPath() const {
+  // Get -fuse-ld= first to prevent -Wunused-command-line-argument. -fuse-ld= is
+  // considered as the linker flavor, e.g. "bfd", "gold", or "lld".
   const Arg* A = Args.getLastArg(options::OPT_fuse_ld_EQ);
   StringRef UseLinker = A ? A->getValue() : CLANG_DEFAULT_LINKER;
 
+  // --ld-path= takes precedence over -fuse-ld= and specifies the executable
+  // name. -B, COMPILER_PATH and PATH and consulted if the value does not
+  // contain a path component separator.
+  if (const Arg *A = Args.getLastArg(options::OPT_ld_path_EQ)) {
+    std::string Path(A->getValue());
+    if (!Path.empty()) {
+      if (llvm::sys::path::parent_path(Path).empty())
+        Path = GetProgramPath(A->getValue());
+      if (llvm::sys::fs::can_execute(Path))
+        return std::string(Path);
+    }
+    getDriver().Diag(diag::err_drv_invalid_linker_name) << A->getAsString(Args);
+    return GetProgramPath(getDefaultLinker());
+  }
+  // If we're passed -fuse-ld= with no argument, or with the argument ld,
+  // then use whatever the default system linker is.
+  if (UseLinker.empty() || UseLinker == "ld")
+    return GetProgramPath(getDefaultLinker());
+
+  // Extending -fuse-ld= to an absolute or relative path is unexpected. Checking
+  // for the linker flavor is brittle. In addition, prepending "ld." or "ld64."
+  // to a relative path is surprising. This is more complex due to priorities
+  // among -B, COMPILER_PATH and PATH. --ld-path= should be used instead.
+  if (UseLinker.find('/') != StringRef::npos)
+    getDriver().Diag(diag::warn_drv_use_ld_non_word);
+
   if (llvm::sys::path::is_absolute(UseLinker)) {
     // If we're passed what looks like an absolute path, don't attempt to
     // second-guess that.
     if (llvm::sys::fs::can_execute(UseLinker))
       return std::string(UseLinker);
-  } else if (UseLinker.empty() || UseLinker == "ld") {
-    // If we're passed -fuse-ld= with no argument, or with the argument ld,
-    // then use whatever the default system linker is.
-    return GetProgramPath(getDefaultLinker());
   } else {
     llvm::SmallString<8> LinkerName;
     if (Triple.isOSDarwin())
       LinkerName.append("ld64.");
     else
       LinkerName.append("ld.");
     LinkerName.append(UseLinker);
 

clang/test/Driver/Inputs/basic_freebsd64_tree/usr/bin/ld.bfd

+#!/bin/true

clang/test/Driver/fuse-ld.c

 // RUN: %clang %s -### \
 // RUN:     -fuse-ld=/usr/local/bin/or1k-linux-ld 2>&1 \
 // RUN:     -target x86_64-unknown-linux \
 // RUN:   | FileCheck %s --check-prefix=CHECK-ABSOLUTE-LD
+// CHECK-ABSOLUTE-LD: warning: '-fuse-ld=' taking a path is deprecated. Use '--ld-path=' instead
 // CHECK-ABSOLUTE-LD: /usr/local/bin/or1k-linux-ld
 
 
 // RUN: %clang %s -### \
 // RUN:     -target x86_64-unknown-freebsd 2>&1 \
 // RUN:   | FileCheck %s --check-prefix=CHECK-FREEBSD-LD
 // CHECK-FREEBSD-LD: ld
 

clang/test/Driver/ld-path.c

+/// This tests uses the PATH environment variable.
+// UNSUPPORTED: system-windows
+
+// RUN: cd %S
+
+/// If --ld-path= specifies a word (without /), -B and COMPILER_PATH are
+/// consulted to locate the linker.
+// RUN: %clang %s -### -B %S/Inputs/basic_freebsd_tree/usr/bin --ld-path=ld.bfd \
+// RUN:   --target=x86_64-unknown-freebsd --sysroot=%S/Inputs/basic_freebsd_tree 2>&1 | \
+// RUN:   FileCheck %s --check-prefix=BFD
+// RUN: env COMPILER_PATH=%S/Inputs/basic_freebsd_tree/usr/bin %clang %s -### --ld-path=ld.bfd \
+// RUN:   --target=x86_64-unknown-freebsd --sysroot=%S/Inputs/basic_freebsd_tree 2>&1 | \
+// RUN:   FileCheck %s --check-prefix=BFD
+/// Then PATH is consulted.
+// RUN: env PATH=%S/Inputs/basic_freebsd_tree/usr/bin %clang %s -### --ld-path=ld.bfd \
+// RUN:   --target=x86_64-unknown-freebsd --sysroot=%S/Inputs/basic_freebsd_tree 2>&1 | \
+// RUN:   FileCheck %s --check-prefix=BFD
+
+// BFD: Inputs/basic_freebsd_tree/usr/bin/ld.bfd"
+
+// RUN: env PATH=%S/Inputs/basic_freebsd_tree/usr/bin %clang %s -### --ld-path=ld.gold \
+// RUN:   --target=x86_64-unknown-freebsd --sysroot=%S/Inputs/basic_freebsd_tree 2>&1 | \
+// RUN:   FileCheck %s --check-prefix=GOLD
+
+// GOLD: Inputs/basic_freebsd_tree/usr/bin/ld.gold"
+
+// RUN: env COMPILER_PATH= PATH=%S/Inputs/basic_freebsd_tree/usr/bin %clang %s -### --ld-path=not_exist \
+// RUN:   --target=x86_64-unknown-freebsd --sysroot=%S/Inputs/basic_freebsd_tree 2>&1 | \
+// RUN:   FileCheck %s --check-prefix=NOT_EXIST
+
+// NOT_EXIST: error: invalid linker name in argument '--ld-path=not_exist'
+
+// RUN: %clang %s -### --ld-path= \
+// RUN:   --target=x86_64-unknown-freebsd --sysroot=%S/Inputs/basic_freebsd_tree 2>&1 | \
+// RUN:   FileCheck %s --check-prefix=EMPTY
+
+// EMPTY: error: invalid linker name in argument '--ld-path='
+
+/// If --ld-path= contains a slash, PATH is not consulted.
+// RUN: env COMPILER_PATH=%S/Inputs/basic_freebsd_tree/usr/bin %clang %s -### --ld-path=./ld.bfd \
+// RUN:   --target=x86_64-unknown-freebsd --sysroot=%S/Inputs/basic_freebsd_tree 2>&1 | \
+// RUN:   FileCheck %s --check-prefix=NO_BFD
+
+// NO_BFD: error: invalid linker name in argument '--ld-path=./ld.bfd'
+
+/// --ld-path can specify an absolute path.
+// RUN: %clang %s -### --ld-path=%S/Inputs/basic_freebsd_tree/usr/bin/ld.bfd \
+// RUN:   --target=x86_64-unknown-freebsd --sysroot=%S/Inputs/basic_freebsd_tree 2>&1 | \
+// RUN:   FileCheck %s --check-prefix=BFD
+
+// RUN: %clang %s -### --ld-path=Inputs/basic_freebsd_tree/usr/bin/ld.bfd \
+// RUN:   --target=x86_64-unknown-freebsd --sysroot=%S/Inputs/basic_freebsd_tree 2>&1 | \
+// RUN:   FileCheck %s --check-prefix=BFD
+
+/// --ld-path= and -fuse-ld= can be used together. --ld-path= takes precedence.
+/// -fuse-ld= can be used to specify the linker flavor.
+// RUN: %clang %s -### -Werror --ld-path=%S/Inputs/basic_freebsd_tree/usr/bin/ld.bfd -fuse-ld=gold \
+// RUN:   --target=x86_64-unknown-freebsd --sysroot=%S/Inputs/basic_freebsd_tree 2>&1 | \
+// RUN:   FileCheck %s --check-prefix=BFD --implicit-check-not=error:
+
+/// --ld-path= respects -working-directory.
+// RUN: %clang %s -### --ld-path=usr/bin/ld.bfd -working-directory=%S/Inputs/basic_freebsd_tree \
+// RUN:   --target=x86_64-unknown-freebsd --sysroot=%S/Inputs/basic_freebsd_tree 2>&1 | \
+// RUN:   FileCheck %s --check-prefix=USR_BIN_BFD
+
+// USR_BIN_BFD: "usr/bin/ld.bfd"

clang/test/Misc/warning-flags.c

(2) It prevents us adding new warnings to Clang that have no -W flag.  All
     new warnings should have -W flags.
 
     If you add a new warning without a flag, this test will fail.  To fix
     this test, simply add a warning group to that warning.
 
 
 The list of warnings below should NEVER grow.  It should gradually shrink to 0.
 
-CHECK: Warnings without flags (68):
+CHECK: Warnings without flags (69):
 
 CHECK-NEXT:   ext_expected_semi_decl_list
 CHECK-NEXT:   ext_explicit_specialization_storage_class
 CHECK-NEXT:   ext_missing_declspec
 CHECK-NEXT:   ext_missing_whitespace_after_macro_name
 CHECK-NEXT:   ext_new_paren_array_nonconst
 CHECK-NEXT:   ext_plain_complex
 CHECK-NEXT:   ext_template_arg_extra_parens

 CHECK-NEXT:   warn_char_constant_too_large
 CHECK-NEXT:   warn_collection_expr_type
 CHECK-NEXT:   warn_conflicting_variadic
 CHECK-NEXT:   warn_delete_array_type
 CHECK-NEXT:   warn_double_const_requires_fp64
 CHECK-NEXT:   warn_drv_assuming_mfloat_abi_is
 CHECK-NEXT:   warn_drv_clang_unsupported
 CHECK-NEXT:   warn_drv_pch_not_first_include
+CHECK-NEXT:   warn_drv_use_ld_non_word
 CHECK-NEXT:   warn_dup_category_def
 CHECK-NEXT:   warn_enum_value_overflow
 CHECK-NEXT:   warn_expected_qualified_after_typename
 CHECK-NEXT:   warn_extraneous_char_constant
 CHECK-NEXT:   warn_fe_backend_unsupported
 CHECK-NEXT:   warn_fe_cc_log_diagnostics_failure
 CHECK-NEXT:   warn_fe_cc_print_header_failure
 CHECK-NEXT:   warn_fe_macro_contains_embedded_newline