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

[Petr Hosek via Phabricator via cfe-commits]

phosek 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`.
>
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115045