[PATCH] D115045: [Clang] Ignore CLANG_DEFAULT_LINKER for custom-linker toolchains

[Simon Moll via Phabricator via cfe-commits]

simoll added a comment.

In D115045#3175648 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115045/new/

https://reviews.llvm.org/D115045