Skip to content

Conversation

@thevinster
Copy link
Contributor

It's possible to have an ld-path point to a linker that doesn't have the ld.lld filename (e.g. linker wrapper that may emit telemetry before invoking the linker). This was causing mis-compilations with fatLTO since the check couldn't reliably detect that it was using lld. Instead, rely on the value from -fuse-ld to determine whether lld is enabled.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Feb 22, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 22, 2025

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Vincent Lee (thevinster)

Changes

It's possible to have an ld-path point to a linker that doesn't have the ld.lld filename (e.g. linker wrapper that may emit telemetry before invoking the linker). This was causing mis-compilations with fatLTO since the check couldn't reliably detect that it was using lld. Instead, rely on the value from -fuse-ld to determine whether lld is enabled.


Full diff: https://github.com/llvm/llvm-project/pull/128285.diff

3 Files Affected:

  • (modified) clang/lib/Driver/ToolChains/CommonArgs.cpp (+2-3)
  • (added) clang/test/Driver/Inputs/basic_cross_linux_tree/usr/x86_64-unknown-linux-gnu/bin/lld-wrapper ()
  • (modified) clang/test/Driver/fat-lto-objects.c (+3)
diff --git a/clang/lib/Driver/ToolChains/CommonArgs.cpp b/clang/lib/Driver/ToolChains/CommonArgs.cpp
index 360754bdb3161..4f60287a1142b 100644
--- a/clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ b/clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -862,13 +862,12 @@ void tools::addLTOOptions(const ToolChain &ToolChain, const ArgList &Args,
   const llvm::Triple &Triple = ToolChain.getTriple();
   const bool IsOSAIX = Triple.isOSAIX();
   const bool IsAMDGCN = Triple.isAMDGCN();
-  const char *Linker = Args.MakeArgString(ToolChain.GetLinkerPath());
+  StringRef Linker = Args.getLastArgValue(options::OPT_fuse_ld_EQ);
   const Driver &D = ToolChain.getDriver();
   const bool IsFatLTO = Args.hasFlag(options::OPT_ffat_lto_objects,
                                      options::OPT_fno_fat_lto_objects, false);
   const bool IsUnifiedLTO = Args.hasArg(options::OPT_funified_lto);
-  if (llvm::sys::path::filename(Linker) != "ld.lld" &&
-      llvm::sys::path::stem(Linker) != "ld.lld" && !Triple.isOSOpenBSD()) {
+  if (Linker != "lld" && Linker != "lld-link" && !Triple.isOSOpenBSD()) {
     // Tell the linker to load the plugin. This has to come before
     // AddLinkerInputs as gold requires -plugin and AIX ld requires -bplugin to
     // come before any -plugin-opt/-bplugin_opt that -Wl might forward.
diff --git a/clang/test/Driver/Inputs/basic_cross_linux_tree/usr/x86_64-unknown-linux-gnu/bin/lld-wrapper b/clang/test/Driver/Inputs/basic_cross_linux_tree/usr/x86_64-unknown-linux-gnu/bin/lld-wrapper
new file mode 100755
index 0000000000000..e69de29bb2d1d
diff --git a/clang/test/Driver/fat-lto-objects.c b/clang/test/Driver/fat-lto-objects.c
index fae64ea7fd1a1..7b87e2b468886 100644
--- a/clang/test/Driver/fat-lto-objects.c
+++ b/clang/test/Driver/fat-lto-objects.c
@@ -49,5 +49,8 @@
 // RUN:   -fuse-ld=lld -flto -ffat-lto-objects -### 2>&1 | FileCheck --check-prefix=LTO %s
 // RUN: %clang --target=x86_64-unknown-linux-gnu --sysroot=%S/Inputs/basic_cross_linux_tree %s \
 // RUN:   -fuse-ld=lld -fno-lto -ffat-lto-objects -### 2>&1 | FileCheck --check-prefix=NOLTO %s
+// RUN: %clang --target=x86_64-unknown-linux-gnu --sysroot=%S/Inputs/basic_cross_linux_tree %s \
+// RUN:   -fuse-ld=lld --ld-path=%S/Inputs/basic_cross_linux_tree/usr/x86_64-unknown-linux-gnu/bin/lld-wrapper \
+// RUN:   -flto -ffat-lto-objects -### 2>&1 | FileCheck --check-prefix=LTO %s
 // LTO: "--fat-lto-objects"
 // NOLTO-NOT: "--fat-lto-objects"

void tools::addLTOOptions(const ToolChain &ToolChain, const ArgList &Args,
ArgStringList &CmdArgs, const InputInfo &Output,
const InputInfo &Input, bool IsThinLTO) {
const llvm::Triple &Triple = ToolChain.getTriple();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change would be desired as -fuse-ld= is better than --ld-path=. However, some users customize CLANG_DEFAULT_LINKER or hard code lld. They have no -fuse-ld= but still use lld....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can restore the original behavior on the linker path plus the additional check on -fuse-ld=. This would at least solve our use case if that's what you're suggesting here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think it's best to keep the old check as well as adding the new one.

@thevinster thevinster merged commit 40b0619 into llvm:main Feb 23, 2025
11 checks passed
@thevinster thevinster deleted the fix-linker-path branch February 23, 2025 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants