[clang] recognize hexagon-*-ld.lld variants #117338

androm3da · 2024-11-22T15:20:30Z

If we create a cross toolchain with a ${triple}-ld.lld symlink, clang finds that symlink and when it uses it, it's not recognized as "lld". Let's resolve that symlink and consider it when determining lld-ness.

For example, clang provides hexagon-link specific link arguments such as -mcpu=hexagonv65 and -march=hexagon when hexagon-unknown-linux-musl-ld.lld is found. lld rejects this with the following error:

hexagon-unknown-linux-musl-ld.lld: error: unknown emulation: cpu=hexagonv65

llvmbot · 2024-11-22T15:21:09Z

@llvm/pr-subscribers-libunwind
@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-backend-hexagon

Author: Brian Cain (androm3da)

Changes

If we create a cross toolchain with a ${triple}-ld.lld symlink, clang finds that symlink and when it uses it, it's not recognized as "lld". Let's resolve that symlink and consider it when determining lld-ness.

For example, clang provides hexagon-link specific link arguments such as -mcpu=hexagonv65 and -march=hexagon when hexagon-unknown-linux-musl-ld.lld is found. lld rejects this with the following error:

hexagon-unknown-linux-musl-ld.lld: error: unknown emulation: cpu=hexagonv65

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

2 Files Affected:

(modified) clang/lib/Driver/ToolChain.cpp (+9-3)
(modified) clang/lib/Driver/ToolChains/Hexagon.cpp (+5-3)

diff --git a/clang/lib/Driver/ToolChain.cpp b/clang/lib/Driver/ToolChain.cpp
index 0d426a467e9a3b..4bf87a9f16d9d3 100644
--- a/clang/lib/Driver/ToolChain.cpp
+++ b/clang/lib/Driver/ToolChain.cpp
@@ -964,7 +964,7 @@ std::string ToolChain::GetLinkerPath(bool *LinkerIsLLD) const {
   StringRef UseLinker = A ? A->getValue() : CLANG_DEFAULT_LINKER;
 
   // --ld-path= takes precedence over -fuse-ld= and specifies the executable
-  // name. -B, COMPILER_PATH and PATH and consulted if the value does not
+  // name. -B, COMPILER_PATH and PATH are consulted if the value does not
   // contain a path component separator.
   // -fuse-ld=lld can be used with --ld-path= to inform clang that the binary
   // that --ld-path= points to is lld.
@@ -974,8 +974,11 @@ std::string ToolChain::GetLinkerPath(bool *LinkerIsLLD) const {
       if (llvm::sys::path::parent_path(Path).empty())
         Path = GetProgramPath(A->getValue());
       if (llvm::sys::fs::can_execute(Path)) {
+        SmallString<256> RealPath;
+        if (llvm::sys::fs::real_path(Path, RealPath))
+          RealPath = llvm::sys::path::stem(RealPath);
         if (LinkerIsLLD)
-          *LinkerIsLLD = UseLinker == "lld";
+          *LinkerIsLLD = UseLinker == "lld" || RealPath == "lld";
         return std::string(Path);
       }
     }
@@ -1014,8 +1017,11 @@ std::string ToolChain::GetLinkerPath(bool *LinkerIsLLD) const {
 
     std::string LinkerPath(GetProgramPath(LinkerName.c_str()));
     if (llvm::sys::fs::can_execute(LinkerPath)) {
+      SmallString<256> RealPath;
+      if (llvm::sys::fs::real_path(LinkerPath, RealPath))
+        RealPath = llvm::sys::path::stem(RealPath);
       if (LinkerIsLLD)
-        *LinkerIsLLD = UseLinker == "lld";
+        *LinkerIsLLD = UseLinker == "lld" || RealPath == "lld";
       return LinkerPath;
     }
   }
diff --git a/clang/lib/Driver/ToolChains/Hexagon.cpp b/clang/lib/Driver/ToolChains/Hexagon.cpp
index 29781399cbab44..625979d56730bd 100644
--- a/clang/lib/Driver/ToolChains/Hexagon.cpp
+++ b/clang/lib/Driver/ToolChains/Hexagon.cpp
@@ -294,9 +294,11 @@ constructHexagonLinkArgs(Compilation &C, const JobAction &JA,
   bool IncStartFiles = !Args.hasArg(options::OPT_nostartfiles);
   bool IncDefLibs = !Args.hasArg(options::OPT_nodefaultlibs);
   bool UseG0 = false;
-  const char *Exec = Args.MakeArgString(HTC.GetLinkerPath());
-  bool UseLLD = (llvm::sys::path::filename(Exec).equals_insensitive("ld.lld") ||
-                 llvm::sys::path::stem(Exec).equals_insensitive("ld.lld"));
+  bool UseLLD = false;
+  const char *Exec = Args.MakeArgString(HTC.GetLinkerPath(&UseLLD));
+  UseLLD = UseLLD ||
+           llvm::sys::path::filename(Exec).equals_insensitive("ld.lld") ||
+           llvm::sys::path::stem(Exec).equals_insensitive("ld.lld");
   bool UseShared = IsShared && !IsStatic;
   StringRef CpuVer = toolchains::HexagonToolChain::GetTargetCPUVersion(Args);

androm3da · 2024-11-22T15:21:29Z

Can I create a symlink in a lit test (and presumably remove it after the test is done?).

I couldn't think of an effective way to test this, so if reviewers have suggestions, it's much appreciated.

MaskRay · 2024-11-22T22:52:38Z

clang/lib/Driver/ToolChain.cpp

@@ -974,8 +974,11 @@ std::string ToolChain::GetLinkerPath(bool *LinkerIsLLD) const {
      if (llvm::sys::path::parent_path(Path).empty())
        Path = GetProgramPath(A->getValue());
      if (llvm::sys::fs::can_execute(Path)) {
+        SmallString<1024> RealPath;
+        if (llvm::sys::fs::real_path(Path, RealPath))


We should not make an extra syscall when UseLinker == "lld"

clang/lib/Driver/ToolChains/Hexagon.cpp

MaskRay · 2024-11-25T17:42:32Z

cmake/Modules/HandleOutOfTreeLLVM.cmake

@@ -0,0 +1,80 @@
+if (NOT DEFINED LLVM_PATH)


I am not sure that these cmake changes should be included in this patch. Many folks concerned with cmake changes will likely notice that this PR has these changes.

ugh! sorry, this was unintentional. I'll fix it.

That was a careless mistake, failed to keep different development branches separate.

Fixed.

MaskRay

I am still not sure about this change.

I think this common utility is only to make -fuse-ld=lld recognize LinkerIsLLD. Toolchains/Hexagon.cpp detects ld.lld and you can make the change there?

androm3da · 2024-11-27T16:16:43Z

I am still not sure about this change.

I think this common utility is only to make -fuse-ld=lld recognize LinkerIsLLD. Toolchains/Hexagon.cpp detects ld.lld and you can make the change there?

Okay - the scope is now reduced to Toolchains/Hexagon.cpp only.

If we create a cross toolchain with a ${triple}-ld.lld symlink, clang finds that symlink and when it uses it, it's not recognized as "lld". Let's consider it as lld as long as it ends in "ld.lld". For example, clang provides hexagon-link specific link arguments such as `-mcpu=hexagonv65` and `-march=hexagon` when hexagon-unknown-linux-musl-ld.lld is found. lld rejects this with the following error: hexagon-unknown-linux-musl-ld.lld: error: unknown emulation: cpu=hexagonv65

If we create a cross toolchain with a ${triple}-ld.lld symlink, clang finds that symlink and when it uses it, it's not recognized as "lld". Let's resolve that symlink and consider it when determining lld-ness. For example, clang provides hexagon-link specific link arguments such as `-mcpu=hexagonv65` and `-march=hexagon` when hexagon-unknown-linux-musl-ld.lld is found. lld rejects this with the following error: hexagon-unknown-linux-musl-ld.lld: error: unknown emulation: cpu=hexagonv65 (cherry picked from commit 2dc0de7)

If we create a cross toolchain with a ${triple}-ld.lld symlink, clang finds that symlink and when it uses it, it's not recognized as "lld". Let's resolve that symlink and consider it when determining lld-ness. For example, clang provides hexagon-link specific link arguments such as `-mcpu=hexagonv65` and `-march=hexagon` when hexagon-unknown-linux-musl-ld.lld is found. lld rejects this with the following error: hexagon-unknown-linux-musl-ld.lld: error: unknown emulation: cpu=hexagonv65

androm3da requested review from nico and MaskRay November 22, 2024 15:20

androm3da self-assigned this Nov 22, 2024

llvmbot added clang Clang issues not falling into any other category backend:Hexagon clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Nov 22, 2024

androm3da requested a review from SidManning November 22, 2024 15:29

androm3da force-pushed the bcain/lld_symlink branch from 15a0dea to 40e018a Compare November 22, 2024 21:41

SidManning approved these changes Nov 22, 2024

View reviewed changes

MaskRay reviewed Nov 22, 2024

View reviewed changes

clang/lib/Driver/ToolChains/Hexagon.cpp Show resolved Hide resolved

androm3da requested review from a team as code owners November 22, 2024 23:25

llvmbot added cmake Build system in general and CMake in particular compiler-rt libunwind labels Nov 22, 2024

MaskRay reviewed Nov 25, 2024

View reviewed changes

androm3da force-pushed the bcain/lld_symlink branch from c1464c9 to 6f5c037 Compare November 25, 2024 17:57

ldionne removed the request for review from a team November 26, 2024 20:34

androm3da mentioned this pull request Nov 27, 2024

*-ld.lld linker not recognized as "lld" #117834

Closed

MaskRay requested changes Nov 27, 2024

View reviewed changes

androm3da removed cmake Build system in general and CMake in particular compiler-rt libunwind labels Nov 27, 2024

androm3da force-pushed the bcain/lld_symlink branch from 6f5c037 to 6077468 Compare November 27, 2024 16:15

androm3da changed the title ~~[clang] recognize any *-ld.lld variant~~ [clang] recognize hexagon-*-ld.lld variants Nov 27, 2024

androm3da force-pushed the bcain/lld_symlink branch from 6077468 to af213f3 Compare November 27, 2024 16:37

androm3da force-pushed the bcain/lld_symlink branch from af213f3 to fa2c015 Compare November 27, 2024 17:24

androm3da requested a review from MaskRay November 27, 2024 23:45

MaskRay approved these changes Nov 28, 2024

View reviewed changes

androm3da merged commit 2dc0de7 into llvm:main Nov 29, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang] recognize hexagon-*-ld.lld variants #117338

[clang] recognize hexagon-*-ld.lld variants #117338

androm3da commented Nov 22, 2024

llvmbot commented Nov 22, 2024 •

edited

Loading

androm3da commented Nov 22, 2024

MaskRay Nov 22, 2024

androm3da Nov 22, 2024

MaskRay Nov 25, 2024

androm3da Nov 25, 2024

androm3da Nov 25, 2024

MaskRay left a comment

androm3da commented Nov 27, 2024

[clang] recognize hexagon-*-ld.lld variants #117338

[clang] recognize hexagon-*-ld.lld variants #117338

Conversation

androm3da commented Nov 22, 2024

llvmbot commented Nov 22, 2024 • edited Loading

androm3da commented Nov 22, 2024

MaskRay Nov 22, 2024

Choose a reason for hiding this comment

androm3da Nov 22, 2024

Choose a reason for hiding this comment

MaskRay Nov 25, 2024

Choose a reason for hiding this comment

androm3da Nov 25, 2024

Choose a reason for hiding this comment

androm3da Nov 25, 2024

Choose a reason for hiding this comment

MaskRay left a comment

Choose a reason for hiding this comment

androm3da commented Nov 27, 2024

llvmbot commented Nov 22, 2024 •

edited

Loading