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

[Petr Hosek via Phabricator via cfe-commits]

phosek added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115045