Skip to content

Conversation

@jhuber6
Copy link
Contributor

@jhuber6 jhuber6 commented Jul 23, 2024

Summary:
We have the -Xoffload-linker=triple=arg syntax that split the argument
meant only for a single toolchain. However this borke if it was an a=b
type argument. Make it only treat it like a triple if it's a valid
triple.

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

llvmbot commented Jul 23, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-driver

Author: Joseph Huber (jhuber6)

Changes

Summary:
We have the -Xoffload-linker=triple=arg syntax that split the argument
meant only for a single toolchain. However this borke if it was an a=b
type argument. Make it only treat it like a triple if it's a valid
triple.


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

2 Files Affected:

  • (modified) clang/test/Driver/linker-wrapper.c (+4-3)
  • (modified) clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp (+7-1)
diff --git a/clang/test/Driver/linker-wrapper.c b/clang/test/Driver/linker-wrapper.c
index 63d43921be9ac..342907c1a3390 100644
--- a/clang/test/Driver/linker-wrapper.c
+++ b/clang/test/Driver/linker-wrapper.c
@@ -128,11 +128,12 @@ __attribute__((visibility("protected"), used)) int x;
 // RUN: %clang -cc1 %s -triple x86_64-unknown-linux-gnu -emit-obj -o %t.o \
 // RUN:   -fembed-offload-object=%t.out
 // RUN: clang-linker-wrapper --dry-run --host-triple=x86_64-unknown-linux-gnu \
-// RUN:   --linker-path=/usr/bin/ld --device-linker=a --device-linker=nvptx64-nvidia-cuda=b \
+// RUN:   --linker-path=/usr/bin/ld --device-linker=foo=bar --device-linker=a \
+// RUN:   --device-linker=nvptx64-nvidia-cuda=b \
 // RUN:   %t.o -o a.out 2>&1 | FileCheck %s --check-prefix=LINKER-ARGS
 
-// LINKER-ARGS: clang{{.*}}--target=amdgcn-amd-amdhsa{{.*}}a
-// LINKER-ARGS: clang{{.*}}--target=nvptx64-nvidia-cuda{{.*}}a b
+// LINKER-ARGS: clang{{.*}}--target=amdgcn-amd-amdhsa{{.*}}foo=bar{{.*}}a
+// LINKER-ARGS: clang{{.*}}--target=nvptx64-nvidia-cuda{{.*}}foo=bar{{.*}}a b
 
 // RUN: not clang-linker-wrapper --dry-run --host-triple=x86_64-unknown-linux-gnu -ldummy \
 // RUN:   --linker-path=/usr/bin/ld --device-linker=a --device-linker=nvptx64-nvidia-cuda=b \
diff --git a/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp b/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
index 9a38e4fb098f9..c93cc800bca27 100644
--- a/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
+++ b/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
@@ -1214,7 +1214,13 @@ DerivedArgList getLinkerArgs(ArrayRef<OffloadFile> Input,
   // Forward '-Xoffload-linker' options to the appropriate backend.
   for (StringRef Arg : Args.getAllArgValues(OPT_device_linker_args_EQ)) {
     auto [Triple, Value] = Arg.split('=');
-    if (Value.empty())
+    llvm::Triple TT(Triple);
+    // If this isn't a recognized triple then it's an `arg=value` option.
+    if (TT.getArch() <= Triple::ArchType::UnknownArch ||
+        TT.getArch() >= Triple::ArchType::LastArchType)
+      DAL.AddJoinedArg(nullptr, Tbl.getOption(OPT_linker_arg_EQ),
+                       Args.MakeArgString(Arg));
+    else if (Value.empty())
       DAL.AddJoinedArg(nullptr, Tbl.getOption(OPT_linker_arg_EQ),
                        Args.MakeArgString(Triple));
     else if (Triple == DAL.getLastArgValue(OPT_triple_EQ))

Summary:
We have the `-Xoffload-linker=triple=arg` syntax that split the argument
meant only for a single toolchain. However this borke if it was an `a=b`
type argument. Make it only treat it like a triple if it's a valid
triple.
Copy link
Contributor

@shiltian shiltian left a comment

Choose a reason for hiding this comment

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

I think LLVM generally doesn't follow the convention that -a x and --abc=x?

@jhuber6
Copy link
Contributor Author

jhuber6 commented Jul 24, 2024

I think LLVM generally doesn't follow the convention that -a x and --abc=x?

cl::opt options are arbitrary, but this isn't about convention really, it's for things like -Xoffload-linker -Wl,-plugin-opt=blah not being passed correctly.

@jhuber6 jhuber6 merged commit af611a0 into llvm:main Jul 24, 2024
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
Summary:
We have the `-Xoffload-linker=triple=arg` syntax that split the argument
meant only for a single toolchain. However this borke if it was an `a=b`
type argument. Make it only treat it like a triple if it's a valid
triple.

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60250759
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.

3 participants