[ToolChain] Use default linker if the toolchain uses a custom one

[phosek]

Summary

When the toolchain overrides default linker, use it rather than the
default Clang one which is unlikely to be useable for that toolchain
anyway. This avoids the need to explicitly specify -fuse-ld in tests
for all targets that use custom linkers.

[phosek]

This should avoid workarounds such as D49899 or D53249.

[Hahnfeld]

If I read that patch correctly, this will render -fuse-ld with non-absolute paths useless if a toolchain has DefaultLinker != "ld". I don't think that's what we want to do if the user explicitly sets a different linker.

In D53250#1264626, @phosek wrote:

This should avoid workarounds such as D49899 or D53249.

I agree that these changes are ugly, but we do the same with -stdlib=platform and -rtlib=platform for tests that check Clang's choices for a particular platform. For this case it's what you did in D53249: Adding -fuse-ld=ld to tests that check the default linker for a platform. (because there is a case for UseLinker == "ld")

[phosek]

In D53250#1264708, @Hahnfeld wrote:

If I read that patch correctly, this will render -fuse-ld with non-absolute paths useless if a toolchain has DefaultLinker != "ld". I don't think that's what we want to do if the user explicitly sets a different linker.

You're right, I missed that case, I'll try to address it.

I agree that these changes are ugly, but we do the same with -stdlib=platform and -rtlib=platform for tests that check Clang's choices for a particular platform. For this case it's what you did in D53249: Adding -fuse-ld=ld to tests that check the default linker for a platform. (because there is a case for UseLinker == "ld")

I agree about the test case but I think the current logic is broken even for regular use. If the platform specifies a custom linker (e.g. hegaxon-link), CLANG_DEFAULT_LINKER should not override it because it's unlikely that it'll be usable on that platform. Today we treat CLANG_DEFAULT_LINKER as a default value for -fuse-ld= but I think we should instead treat it as a default value for ld (i.e. default system linker) which will be used if -fuse-ld= is unspecified or the platform doesn't specify its own linker. Would you agree with that?

The reason why this is not a problem of -stdlib= and -rtlib= is because many toolchains override ToolChain::GetRuntimeLibType and ToolChain::GetCXXStdlibType (e.g. Fuchsia or WebAssembly) completely bypassing the default logic. The reason why this is not being used for linker selection is because -fuse-ld=/path/to/linker is generally useful even for these targets that use custom linkers (e.g. D53038 explicitly mentions that) so I think we either need to make ToolChain::GetLinkerPath smarter about handling platform-specific linkers or we need to extract the logic for handling linker specified via absolute path into a helper method so toolchains can override ToolChain::GetLinkerPath without re-implementing chunk of its logic.

[Hahnfeld]

Going through my old revisions, is this still something that we want?

[phosek]

Sorry about the late response, I was mostly out last week. This recently came up again so I'd like to look into it but I'll have to rethink the change and come up with a better approach.

clang/include/clang/Driver/ToolChain.h

public:
   /// Choose a tool to use to handle the action \p JA.
   ///
   /// This can be overridden when a particular ToolChain needs to use
   /// a compiler other than Clang.
   virtual Tool *SelectTool(const JobAction &JA) const;
 
   // Helper methods
 
-  std::string GetFilePath(const char *Name) const;
-  std::string GetProgramPath(const char *Name) const;
+  std::string GetFilePath(StringRef Name) const;
+  std::string GetProgramPath(StringRef Name) const;
 
   /// Returns the linker path, respecting the -fuse-ld= argument to determine
   /// the linker suffix or name.
   std::string GetLinkerPath() const;
 
   /// Dispatch to the specific toolchain for verbose printing.
   ///
   /// This is used when handling the verbose option to print detailed,

clang/lib/Driver/ToolChain.cpp

 Tool *ToolChain::SelectTool(const JobAction &JA) const {
   if (getDriver().ShouldUseClangCompiler(JA)) return getClang();
   Action::ActionClass AC = JA.getKind();
   if (AC == Action::AssembleJobClass && useIntegratedAs())
     return getClangAs();
   return getTool(AC);
 }
 
-std::string ToolChain::GetFilePath(const char *Name) const {
+std::string ToolChain::GetFilePath(StringRef Name) const {
   return D.GetFilePath(Name, *this);
 }
 
-std::string ToolChain::GetProgramPath(const char *Name) const {
+std::string ToolChain::GetProgramPath(StringRef Name) const {
   return D.GetProgramPath(Name, *this);
 }
 
 std::string ToolChain::GetLinkerPath() const {
   const Arg* A = Args.getLastArg(options::OPT_fuse_ld_EQ);
   StringRef UseLinker = A ? A->getValue() : CLANG_DEFAULT_LINKER;
+  StringRef DefaultLinker = getDefaultLinker();
 
   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 UseLinker;
-  } else if (UseLinker.empty() || UseLinker == "ld") {
+  } else if (UseLinker.empty() || UseLinker == "ld" || DefaultLinker != "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());
+    // or if the toolchain overrides default linker, then use whatever the
+    // default linker is.
+    return GetProgramPath(DefaultLinker);
   } else {
     llvm::SmallString<8> LinkerName;
     if (Triple.isOSDarwin())
       LinkerName.append("ld64.");
     else
       LinkerName.append("ld.");
     LinkerName.append(UseLinker);
 
-    std::string LinkerPath(GetProgramPath(LinkerName.c_str()));
+    std::string LinkerPath(GetProgramPath(LinkerName));
     if (llvm::sys::fs::can_execute(LinkerPath))
       return LinkerPath;
   }
 
   if (A)
     getDriver().Diag(diag::err_drv_invalid_linker_name) << A->getAsString(Args);
 
-  return GetProgramPath(getDefaultLinker());
+  return GetProgramPath(DefaultLinker);
 }
 
 types::ID ToolChain::LookupTypeForExtension(StringRef Ext) const {
   return types::lookupTypeForExtension(Ext);
 }
 
 bool ToolChain::HasNativeLLVMSupport() const {
   return false;