[Clang] Ignore CLANG_DEFAULT_LINKER for custom-linker toolchains

[simoll]

Summary

Before, the CLANG_DEFAULT_LINKER cmake option was a global override for
the linker that shall be used on all toolchains. The linker binary
specified that way may not be available on toolchains with custom
linkers. Eg, the only linker for VE is named 'nld' - any other linker
invalidates the toolchain.

This patch removes the hard override and instead lets the generic
toolchain implementation default to CLANG_DEFAULT_LINKER. Toolchains
can now deviate with a custom linker name or deliberatly default to
CLANG_DEFAULT_LINKER.

[phosek]

LGTM

[MaskRay]

LGTM.

clang/lib/Driver/ToolChain.cpp

545

CLANG_DEFAUALT_LINKER[0] == '\0'

[simoll]

Simplified empty-string check as suggested

[ronlieb]

Hi Simon,
i am seeing a failure in our amdgpu buildbot after this patch . https://lab.llvm.org/staging/#/builders/200/builds/1407
we do depend on the cmake flag you removed.
we specify this
-DCLANG_DEFAULT_LINKER=lld

FAILED: openmp/libomptarget/libomptarget.rtl.x86_64.so
: && /home/rlieberm/mono-repo/llvm-project/build/./bin/clang++ --target=x86_64-unknown-linux-gnu -fPIC -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wstring-conversion -Wmisleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -Wall -Wcast-qual -Wformat-pedantic -Wimplicit-fallthrough -Wsign-compare -Wno-extra -Wno-pedantic -std=c++14 -O3 -DNDEBUG -Wl,-z,defs -Wl,-z,nodelete -Wl,--color-diagnostics -shared -Wl,-soname,libomptarget.rtl.x86_64.so -o openmp/libomptarget/libomptarget.rtl.x86_64.so openmp/libomptarget/plugins/common/elf_common/CMakeFiles/elf_common.dir/elf_common.cpp.o openmp/libomptarget/plugins/x86_64/CMakeFiles/omptarget.rtl.x86_64.dir/__/generic-elf-64bit/src/rtl.cpp.o /usr/lib/x86_64-linux-gnu/libffi.so /usr/lib/x86_64-linux-gnu/libelf.so -ldl -lpthread -Wl,--version-script=/home/rlieberm/mono-repo/llvm-project/openmp/libomptarget/plugins/x86_64/../exports /home/rlieberm/mono-repo/llvm-project/build/lib/libLLVMObject.a /home/rlieberm/mono-repo/llvm-project/build/lib/libLLVMBitReader.a /home/rlieberm/mono-repo/llvm-project/build/lib/libLLVMCore.a /home/rlieberm/mono-repo/llvm-project/build/lib/libLLVMRemarks.a /home/rlieberm/mono-repo/llvm-project/build/lib/libLLVMBitstreamReader.a /home/rlieberm/mono-repo/llvm-project/build/lib/libLLVMMCParser.a /home/rlieberm/mono-repo/llvm-project/build/lib/libLLVMMC.a /home/rlieberm/mono-repo/llvm-project/build/lib/libLLVMDebugInfoCodeView.a /home/rlieberm/mono-repo/llvm-project/build/lib/libLLVMTextAPI.a /home/rlieberm/mono-repo/llvm-project/build/lib/libLLVMBinaryFormat.a /home/rlieberm/mono-repo/llvm-project/build/lib/libLLVMSupport.a -lrt -lm /usr/lib/x86_64-linux-gnu/libz.so /usr/lib/x86_64-linux-gnu/libtinfo.so /home/rlieberm/mono-repo/llvm-project/build/lib/libLLVMDemangle.a -ldl -lpthread && :
lld is a generic driver.
Invoke ld.lld (Unix), ld64.lld (macOS), lld-link (Windows), wasm-ld (WebAssembly) instead
clang-14: error: linker command failed with exit code 1 (use -v to see invocation)

[simoll]

I've reverted the patch for now, this may show up for other toolchains, too.

This patch has pushed down the responsibility for handling -DCLANG_DEFAULT_LINKER to the toolchains.
However, I did not modify all toolchains to account for that. Pending an update..

In D115045#3173520, @ronlieb wrote:

Hi Simon,
i am seeing a failure in our amdgpu buildbot after this patch . https://lab.llvm.org/staging/#/builders/200/builds/1407
we do depend on the cmake flag you removed.
we specify this
-DCLANG_DEFAULT_LINKER=lld

FAILED: openmp/libomptarget/libomptarget.rtl.x86_64.so
: && /home/rlieberm/mono-repo/llvm-project/build/./bin/clang++ --target=x86_64-unknown-linux-gnu -fPIC -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wstring-conversion -Wmisleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -Wall -Wcast-qual -Wformat-pedantic -Wimplicit-fallthrough -Wsign-compare -Wno-extra -Wno-pedantic -std=c++14 -O3 -DNDEBUG -Wl,-z,defs -Wl,-z,nodelete -Wl,--color-diagnostics -shared -Wl,-soname,libomptarget.rtl.x86_64.so -o openmp/libomptarget/libomptarget.rtl.x86_64.so openmp/libomptarget/plugins/common/elf_common/CMakeFiles/elf_common.dir/elf_common.cpp.o openmp/libomptarget/plugins/x86_64/CMakeFiles/omptarget.rtl.x86_64.dir/__/generic-elf-64bit/src/rtl.cpp.o /usr/lib/x86_64-linux-gnu/libffi.so /usr/lib/x86_64-linux-gnu/libelf.so -ldl -lpthread -Wl,--version-script=/home/rlieberm/mono-repo/llvm-project/openmp/libomptarget/plugins/x86_64/../exports /home/rlieberm/mono-repo/llvm-project/build/lib/libLLVMObject.a /home/rlieberm/mono-repo/llvm-project/build/lib/libLLVMBitReader.a /home/rlieberm/mono-repo/llvm-project/build/lib/libLLVMCore.a /home/rlieberm/mono-repo/llvm-project/build/lib/libLLVMRemarks.a /home/rlieberm/mono-repo/llvm-project/build/lib/libLLVMBitstreamReader.a /home/rlieberm/mono-repo/llvm-project/build/lib/libLLVMMCParser.a /home/rlieberm/mono-repo/llvm-project/build/lib/libLLVMMC.a /home/rlieberm/mono-repo/llvm-project/build/lib/libLLVMDebugInfoCodeView.a /home/rlieberm/mono-repo/llvm-project/build/lib/libLLVMTextAPI.a /home/rlieberm/mono-repo/llvm-project/build/lib/libLLVMBinaryFormat.a /home/rlieberm/mono-repo/llvm-project/build/lib/libLLVMSupport.a -lrt -lm /usr/lib/x86_64-linux-gnu/libz.so /usr/lib/x86_64-linux-gnu/libtinfo.so /home/rlieberm/mono-repo/llvm-project/build/lib/libLLVMDemangle.a -ldl -lpthread && :
lld is a generic driver.
Invoke ld.lld (Unix), ld64.lld (macOS), lld-link (Windows), wasm-ld (WebAssembly) instead
clang-14: error: linker command failed with exit code 1 (use -v to see invocation)

[ronlieb]

fyi: if i remove the now 'dead' use of the CMAKE variable from my cmake , then i am able to build. so i see you reverted, thx. i guess some coordination amongst buildbot maintainers who might use this option

checking zorg: the last two are me: not sure who owns the 1st one
"clang-ppc64le-rhel"
"openmp-offload-amdgpu-runtime"
"openmp-offload-amdgpu-runtime-experimental"

[simoll]

Use the -DCLANG_DEFAULT_LINKER linker in all toolchains but VE. Since toolchains are now responsible for this flag, there is a new helper function that emulates the old linker selection behavior.
This should unbreak the AMDGPU staging bot.

[simoll]

Formatting / Cleanup

[phosek]

I don't think the updated logic is correct. For example, in the case of Fuchsia we don't want to take CLANG_DEFAULT_LINKER into account, that's why we override getDefaultLinker. I assume it's the same for other toolchains that override getDefaultLinker.

The issue that affected AMDGPU bots is that StringRef UseLinker = A ? A->getValue() : "" is now going to evaluate to an empty string unless -fuse-ld= is set, and we'll take the UseLinker.empty() || UseLinker == "ld" branch, where const char *DefaultLinker = getDefaultLinker() is going to return "lld" because AMDGPU bot sets -DCLANG_DEFAULT_LINKER=lld in their build. That's not a valid name, the valid name is ld.lld. Prior to your patch, we would take the !llvm::sys::path::is_absolute(UseLinker) branch and construct the correct linker name by appending CLANG_DEFAULT_LINKER to '"ld."`.

I'd argue that your originally patch is actually the behavior we want. Rather, we shouldn't consider -DCLANG_DEFAULT_LINKER=lld as a valid value. Instead AMDGPU bot should use -DCLANG_DEFAULT_LINKER=ld.lld. Even better would be to introduce a second CMake variable so we can control the default value for -fuse-ld= and for --ld-path options separately. Right now it's not clear which of the two is controlled by -DCLANG_DEFAULT_LINKER= (that's because -DCLANG_DEFAULT_LINKER= actually predates the --ld-path option).

[phosek]

In D115045#3175648, @phosek wrote:

I don't think the updated logic is correct. For example, in the case of Fuchsia we don't want to take CLANG_DEFAULT_LINKER into account, that's why we override getDefaultLinker. I assume it's the same for other toolchains that override getDefaultLinker.

The issue that affected AMDGPU bots is that StringRef UseLinker = A ? A->getValue() : "" is now going to evaluate to an empty string unless -fuse-ld= is set, and we'll take the UseLinker.empty() || UseLinker == "ld" branch, where const char *DefaultLinker = getDefaultLinker() is going to return "lld" because AMDGPU bot sets -DCLANG_DEFAULT_LINKER=lld in their build. That's not a valid name, the valid name is ld.lld. Prior to your patch, we would take the !llvm::sys::path::is_absolute(UseLinker) branch and construct the correct linker name by appending CLANG_DEFAULT_LINKER to '"ld."`.

I'd argue that your originally patch is actually the behavior we want. Rather, we shouldn't consider -DCLANG_DEFAULT_LINKER=lld as a valid value. Instead AMDGPU bot should use -DCLANG_DEFAULT_LINKER=ld.lld. Even better would be to introduce a second CMake variable so we can control the default value for -fuse-ld= and for --ld-path options separately. Right now it's not clear which of the two is controlled by -DCLANG_DEFAULT_LINKER= (that's because -DCLANG_DEFAULT_LINKER= actually predates the --ld-path option).

I forgot to mention, I don't think your updated logic is actually going to address the failure we've seen on the AMDGPU bot because the test failure they're seeing is not in the AMDGPU target, it's in the x86_64-unknown-linux-gnu target and that's still going to follow the control flow I described above.

[mstorsjo]

In D115045#3175648, @phosek wrote:

Rather, we shouldn't consider -DCLANG_DEFAULT_LINKER=lld as a valid value. Instead AMDGPU bot should use -DCLANG_DEFAULT_LINKER=ld.lld.

I think this would be a kinda disruptive change; I (and others) specify it as -DCLANG_DEFAULT_LINKER=lld so far: https://github.com/mstorsjo/llvm-mingw/blob/master/build-llvm.sh#L162 And other projects too: https://github.com/msys2/MINGW-packages/blob/master/mingw-w64-clang/PKGBUILD#L204

[phosek]

In D115045#3175760, @mstorsjo wrote:

In D115045#3175648, @phosek wrote:

Rather, we shouldn't consider -DCLANG_DEFAULT_LINKER=lld as a valid value. Instead AMDGPU bot should use -DCLANG_DEFAULT_LINKER=ld.lld.

I think this would be a kinda disruptive change; I (and others) specify it as -DCLANG_DEFAULT_LINKER=lld so far: https://github.com/mstorsjo/llvm-mingw/blob/master/build-llvm.sh#L162 And other projects too: https://github.com/msys2/MINGW-packages/blob/master/mingw-w64-clang/PKGBUILD#L204

In that case, perhaps we should consider -DCLANG_DEFAULT_LINKER= to set the default value for -fuse-ld only and have another variable for --ld-path? Open question is if getDefaultLinker() should return the name of the linker or the filename. I'd argue for the former which would be consistent with -DCLANG_DEFAULT_LINKER=.

[simoll]

In D115045#3175648, @phosek wrote:

I don't think the updated logic is correct. For example, in the case of Fuchsia we don't want to take CLANG_DEFAULT_LINKER into account, that's why we override getDefaultLinker. I assume it's the same for other toolchains that override getDefaultLinker.

You do. With the logic as before this patch, if CLANG_DEFAULT_LINKER is an absolute path, your build overrides the default linker on Fuchsia (and elsewhere).

The issue that affected AMDGPU bots is that StringRef UseLinker = A ? A->getValue() : "" is now going to evaluate to an empty string unless -fuse-ld= is set, and we'll take the UseLinker.empty() || UseLinker == "ld" branch, where const char *DefaultLinker = getDefaultLinker() is going to return "lld" because AMDGPU bot sets -DCLANG_DEFAULT_LINKER=lld in their build. That's not a valid name, the valid name is ld.lld. Prior to your patch, we would take the !llvm::sys::path::is_absolute(UseLinker) branch and construct the correct linker name by appending CLANG_DEFAULT_LINKER to '"ld."`.

Right, the updated patch does not emulate the old behavior here. What makes a linker name valid?

I'd argue that your originally patch is actually the behavior we want. Rather, we shouldn't consider -DCLANG_DEFAULT_LINKER=lld as a valid value. Instead AMDGPU bot should use -DCLANG_DEFAULT_LINKER=ld.lld. Even better would be to introduce a second CMake variable so we can control the default value for -fuse-ld= and for --ld-path options separately. Right now it's not clear which of the two is controlled by -DCLANG_DEFAULT_LINKER= (that's because -DCLANG_DEFAULT_LINKER= actually predates the --ld-path option).

The interplay of CLANG_DEFAULT_LINKER , -fuse-ld and --ld-path is really a mess. How do we proceed here?
To be honest, it is hard to change anything about these flags as any semantic change may result in breakage on some (maybe downstream) toolchain and build configuration.

clang/include/clang/Driver/ToolChain.h

public:
 
   /// Get the default trivial automatic variable initialization.
   virtual LangOptions::TrivialAutoVarInitKind
   GetDefaultTrivialAutoVarInit() const {
     return LangOptions::TrivialAutoVarInitKind::Uninitialized;
   }
 
   /// GetDefaultLinker - Get the default linker to use.
-  virtual const char *getDefaultLinker() const { return "ld"; }
+  virtual const char *getDefaultLinker() const;
 
   /// GetDefaultRuntimeLibType - Get the default runtime library variant to use.
   virtual RuntimeLibType GetDefaultRuntimeLibType() const {
     return ToolChain::RLT_Libgcc;
   }
 
   virtual CXXStdlibType GetDefaultCXXStdlibType() const {
     return ToolChain::CST_Libstdcxx;

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);
 }
 
+const char *ToolChain::getDefaultLinker() const {
+  if (CLANG_DEFAULT_LINKER[0] == '\0')
+    return "ld";
+  return CLANG_DEFAULT_LINKER;
+}
+
 std::string ToolChain::GetLinkerPath(bool *LinkerIsLLD) const {
   if (LinkerIsLLD)
     *LinkerIsLLD = false;
 
   // 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;
+  StringRef UseLinker = A ? A->getValue() : "";
 
   // --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())

clang/test/Driver/ve-toolchain.c

 // RUN: %clang -### -target ve %s 2>&1 | FileCheck -check-prefix=DEFADDRSIG %s
 // DEFADDRSIG: clang{{.*}} "-cc1"
 // DEFADDRSIG-NOT: "-faddrsig"
 
 ///-----------------------------------------------------------------------------
 /// Checking -fintegrated-as
 
 // RUN: %clang -### -target ve \
-// RUN:    -x assembler -fuse-ld=ld %s 2>&1 | \
+// RUN:    -x assembler %s 2>&1 | \
 // RUN:    FileCheck -check-prefix=AS %s
 // RUN: %clang -### -target ve \
-// RUN:    -fno-integrated-as -fuse-ld=ld -x assembler %s 2>&1 | \
+// RUN:    -fno-integrated-as -x assembler %s 2>&1 | \
 // RUN:    FileCheck -check-prefix=NAS %s
 
 // AS: clang{{.*}} "-cc1as"
 // AS: nld{{.*}}
 
 // NAS: nas{{.*}}
 // NAS: nld{{.*}}
 
 ///-----------------------------------------------------------------------------
 /// Checking default behavior:
 ///  - dynamic linker
 ///  - library paths
 ///  - nld VE specific options
 ///  - sjlj exception
 
 // RUN: %clang -### -target ve-unknown-linux-gnu \
 // RUN:     --sysroot %S/Inputs/basic_ve_tree \
 // RUN:     -resource-dir=%S/Inputs/basic_ve_tree/resource_dir \
-// RUN:     -fuse-ld=ld \
 // RUN:     %s 2>&1 | FileCheck -check-prefix=DEF %s
 
 // DEF:      clang{{.*}}" "-cc1"
 // DEF-SAME: "-resource-dir" "[[RESOURCE_DIR:[^"]+]]"
 // DEF-SAME: "-isysroot" "[[SYSROOT:[^"]+]]"
 // DEF-SAME: "-exception-model=sjlj"
 // DEF:      nld"
 // DEF-SAME: "--sysroot=[[SYSROOT]]"

clang/test/Driver/ve-toolchain.cpp

 // RUN: %clangxx -### -target ve %s 2>&1 | FileCheck -check-prefix=DEFADDRSIG %s
 // DEFADDRSIG: clang{{.*}} "-cc1"
 // DEFADDRSIG-NOT: "-faddrsig"
 
 ///-----------------------------------------------------------------------------
 /// Checking -fintegrated-as
 
 // RUN: %clangxx -### -target ve \
-// RUN:    -x assembler -fuse-ld=ld %s 2>&1 | \
+// RUN:    -x assembler %s 2>&1 | \
 // RUN:    FileCheck -check-prefix=AS %s
 // RUN: %clangxx -### -target ve \
-// RUN:    -fno-integrated-as -x assembler -fuse-ld=ld %s 2>&1 | \
+// RUN:    -fno-integrated-as -x assembler %s 2>&1 | \
 // RUN:    FileCheck -check-prefix=NAS %s
 
 // AS: clang{{.*}} "-cc1as"
 // AS: nld{{.*}}
 
 // NAS: nas{{.*}}
 // NAS: nld{{.*}}
 
 ///-----------------------------------------------------------------------------
 /// Checking default behavior:
 ///  - dynamic linker
 ///  - library paths
 ///  - nld VE specific options
 ///  - sjlj exception
 
 // RUN: %clangxx -### -target ve-unknown-linux-gnu \
 // RUN:     --sysroot %S/Inputs/basic_ve_tree \
-// RUN:     -fuse-ld=ld \
 // RUN:     -resource-dir=%S/Inputs/basic_ve_tree/resource_dir \
 // RUN:     --stdlib=c++ %s 2>&1 | FileCheck -check-prefix=DEF %s
 
 // DEF:      clang{{.*}}" "-cc1"
 // DEF-SAME: "-resource-dir" "[[RESOURCE_DIR:[^"]+]]"
 // DEF-SAME: "-isysroot" "[[SYSROOT:[^"]+]]"
 // DEF-SAME: "-exception-model=sjlj"
 // DEF:      nld"