[ClangLinkerWrapper] Refactor target ID sanitization for Windows file…

clang/include/clang/Basic/TargetID.h

@@ -56,6 +56,11 @@ getConflictTargetIDCombination(const std::set<llvm::StringRef> &TargetIDs);
 /// Check whether the provided target ID is compatible with the requested
 /// target ID.
 bool isCompatibleTargetID(llvm::StringRef Provided, llvm::StringRef Requested);
+
+/// Sanitize a target ID string for use in a file name.
+/// Replaces invalid characters (like ':') with safe characters (like '@').
+/// Currently only replaces ':' with '@' on Windows.
+std::string sanitizeTargetIDInFileName(llvm::StringRef TargetID);
 } // namespace clang
 
 #endif

clang/lib/Basic/TargetID.cpp

@@ -9,10 +9,13 @@
 #include "clang/Basic/TargetID.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallSet.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/Support/Path.h"
 #include "llvm/TargetParser/TargetParser.h"
 #include "llvm/TargetParser/Triple.h"
 #include <map>
 #include <optional>
+#include <string>
 
 namespace clang {
 
@@ -183,4 +186,11 @@ bool isCompatibleTargetID(llvm::StringRef Provided, llvm::StringRef Requested) {
   return true;
 }
 
+std::string sanitizeTargetIDInFileName(llvm::StringRef TargetID) {
+  std::string FileName = TargetID.str();
+  if (llvm::sys::path::is_style_windows(llvm::sys::path::Style::native))
+    llvm::replace(FileName, ':', '@');
+  return FileName;
+}
+
 } // namespace clang

clang/lib/Driver/Driver.cpp

@@ -6285,12 +6285,7 @@ const char *Driver::GetNamedOutputPath(Compilation &C, const JobAction &JA,
                                        StringRef OrigBoundArch, bool AtTopLevel,
                                        bool MultipleArchs,
                                        StringRef OffloadingPrefix) const {
-  std::string BoundArch = OrigBoundArch.str();
-  if (is_style_windows(llvm::sys::path::Style::native)) {
-    // BoundArch may contains ':', which is invalid in file names on Windows,
-    // therefore replace it with '%'.
-    llvm::replace(BoundArch, ':', '@');
-  }
+  std::string BoundArch = sanitizeTargetIDInFileName(OrigBoundArch);
 
   llvm::PrettyStackTraceString CrashInfo("Computing output path");
   // Output to a user requested destination?

clang/test/Driver/linker-wrapper-hip-no-rdc.c

@@ -1,4 +1,3 @@
-// UNSUPPORTED: system-windows
 // REQUIRES: amdgpu-registered-target
 // REQUIRES: lld
 
@@ -11,31 +10,48 @@ __attribute__((visibility("protected"), used)) int x;
 // Create device binaries and package them
 // RUN: %clang -cc1 %s -triple amdgcn-amd-amdhsa -emit-llvm-bc -o %t.amdgpu.bc
 // RUN: llvm-offload-binary -o %t.out \
-// RUN:   --image=file=%t.amdgpu.bc,kind=hip,triple=amdgcn-amd-amdhsa,arch=gfx1100 \
+// RUN:   --image=file=%t.amdgpu.bc,kind=hip,triple=amdgcn-amd-amdhsa,arch=gfx9-4-generic:xnack+ \
 // RUN:   --image=file=%t.amdgpu.bc,kind=hip,triple=amdgcn-amd-amdhsa,arch=gfx1200
 
 // Test that linker wrapper outputs .hipfb file without -r option for HIP non-RDC
 // The linker wrapper is called directly with the packaged device binary (not embedded in host object)
 // Note: When called directly (not through the driver), the linker wrapper processes architectures
 // from the packaged binary. The test verifies it can process at least one architecture correctly.
-// RUN: clang-linker-wrapper --emit-fatbin-only --linker-path=/usr/bin/ld %t.out -o %t.hipfb 2>&1
+// RUN: %if system-windows %{ \
+// RUN:   clang-linker-wrapper --wrapper-verbose --device-linker=amdgcn-amd-amdhsa=-v --device-compiler=-v --emit-fatbin-only --linker-path=/usr/bin/ld %t.out -o %t.hipfb 2>&1 | FileCheck %s --check-prefix=CMD-WIN \
+// RUN: %} %else %{ \
+// RUN:   clang-linker-wrapper --wrapper-verbose --device-linker=amdgcn-amd-amdhsa=-v --device-compiler=-v --emit-fatbin-only --linker-path=/usr/bin/ld %t.out -o %t.hipfb 2>&1 | FileCheck %s --check-prefix=CMD-LINUX \
+// RUN: %}
+
+// On Linux, ':' is preserved in file names
+// CMD-LINUX-DAG: clang{{.*}} -o {{.*}}hipfb.amdgcn.gfx9-4-generic:xnack+{{.*}}.img
+// CMD-LINUX-DAG: clang{{.*}} -o {{.*}}hipfb.amdgcn.gfx1200{{.*}}.img
+// CMD-LINUX-DAG: ld.lld{{.*}} -o {{.*}}hipfb.amdgcn.gfx9-4-generic:xnack+{{.*}}.img
+// CMD-LINUX-DAG: ld.lld{{.*}} -o {{.*}}hipfb.amdgcn.gfx1200{{.*}}.img
+
+// On Windows, ':' is replaced with '@' in file names
+// CMD-WIN-DAG: clang{{.*}} -o {{.*}}hipfb.amdgcn.gfx9-4-generic@xnack+{{.*}}.img
+// CMD-WIN-DAG: clang{{.*}} -o {{.*}}hipfb.amdgcn.gfx1200{{.*}}.img
+// CMD-WIN-DAG: ld.lld{{.*}} -o {{.*}}hipfb.amdgcn.gfx9-4-generic@xnack+{{.*}}.img
+// CMD-WIN-DAG: ld.lld{{.*}} -o {{.*}}hipfb.amdgcn.gfx1200{{.*}}.img
 
 // Verify the fat binary was created
 // RUN: test -f %t.hipfb
 
 // List code objects in the fat binary
 // RUN: clang-offload-bundler -type=o -input=%t.hipfb -list | FileCheck %s --check-prefix=HIP-FATBIN-LIST
 
-// HIP-FATBIN-LIST-DAG: hip-amdgcn-amd-amdhsa--gfx1100
+// HIP-FATBIN-LIST-DAG: hip-amdgcn-amd-amdhsa--gfx9-4-generic:xnack+
 // HIP-FATBIN-LIST-DAG: hip-amdgcn-amd-amdhsa--gfx1200
 // HIP-FATBIN-LIST-DAG: host-x86_64-unknown-linux-gnu-
 
 // Extract code objects for both architectures from the fat binary
-// RUN: clang-offload-bundler -type=o -targets=hip-amdgcn-amd-amdhsa--gfx1100,hip-amdgcn-amd-amdhsa--gfx1200 \
-// RUN:   -output=%t.gfx1100.co -output=%t.gfx1200.co -input=%t.hipfb -unbundle
+// Use '-' instead of ':' in file names to avoid issues on Windows
+// RUN: clang-offload-bundler -type=o -targets=hip-amdgcn-amd-amdhsa--gfx9-4-generic:xnack+,hip-amdgcn-amd-amdhsa--gfx1200 \
+// RUN:   -output=%t.gfx9-4-generic-xnack+.co -output=%t.gfx1200.co -input=%t.hipfb -unbundle
 
 // Verify extracted code objects exist and are not empty
-// RUN: test -f %t.gfx1100.co
-// RUN: test -s %t.gfx1100.co
+// RUN: test -f %t.gfx9-4-generic-xnack+.co
+// RUN: test -s %t.gfx9-4-generic-xnack+.co
 // RUN: test -f %t.gfx1200.co
 // RUN: test -s %t.gfx1200.co

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp

@@ -228,11 +228,13 @@ std::string getMainExecutable(const char *Name) {
 Expected<StringRef> createOutputFile(const Twine &Prefix, StringRef Extension) {
   std::scoped_lock<decltype(TempFilesMutex)> Lock(TempFilesMutex);
   SmallString<128> OutputFile;
+  std::string PrefixStr = clang::sanitizeTargetIDInFileName(Prefix.str());
+
   if (SaveTemps) {
-    (Prefix + "." + Extension).toNullTerminatedStringRef(OutputFile);
+    (PrefixStr + "." + Extension).toNullTerminatedStringRef(OutputFile);
   } else {
     if (std::error_code EC =
-            sys::fs::createTemporaryFile(Prefix, Extension, OutputFile))
+            sys::fs::createTemporaryFile(PrefixStr, Extension, OutputFile))
       return createFileError(OutputFile, EC);
   }
 
@@ -625,7 +627,6 @@ Expected<StringRef> writeOffloadFile(const OffloadFile &File) {
   SmallString<128> Filename;
   (Prefix + "-" + Binary.getTriple() + "-" + Binary.getArch())
       .toVector(Filename);
-  llvm::replace(Filename, ':', '-');
   auto TempFileOrErr = createOutputFile(Filename, "o");
   if (!TempFileOrErr)
     return TempFileOrErr.takeError();