https://github.com/yxsamliu created https://github.com/llvm/llvm-project/pull/168744
… names Fix non-RDC mode HIP compilation for the new driver on Windows due to invalid temporary file names when offload arch is a target ID containing ':', which is invalid in file names on Windows. Refactor the existing handling of ':' in file names on Windows from clang driver into a shared function sanitizeTargetIDInFileName in clang/Basic/TargetID.h. This function replaces ':' with '@' on Windows only, preserving the original behavior. Update both clang/lib/Driver/Driver.cpp and clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp to use this shared function, ensuring consistent handling across both tools. >From c5fa73fbc62d76a3ed9b1fe5b37c72b98180ef8d Mon Sep 17 00:00:00 2001 From: "Yaxun (Sam) Liu" <[email protected]> Date: Wed, 19 Nov 2025 13:00:13 -0500 Subject: [PATCH] [ClangLinkerWrapper] Refactor target ID sanitization for Windows file names Fix non-RDC mode HIP compilation for the new driver on Windows due to invalid temporary file names when offload arch is a target ID containing ':', which is invalid in file names on Windows. Refactor the existing handling of ':' in file names on Windows from clang driver into a shared function sanitizeTargetIDInFileName in clang/Basic/TargetID.h. This function replaces ':' with '@' on Windows only, preserving the original behavior. Update both clang/lib/Driver/Driver.cpp and clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp to use this shared function, ensuring consistent handling across both tools. --- clang/include/clang/Basic/TargetID.h | 5 +++ clang/lib/Basic/TargetID.cpp | 10 ++++++ clang/lib/Driver/Driver.cpp | 7 +--- clang/test/Driver/linker-wrapper-hip-no-rdc.c | 32 ++++++++++++++----- .../ClangLinkerWrapper.cpp | 7 ++-- 5 files changed, 44 insertions(+), 17 deletions(-) diff --git a/clang/include/clang/Basic/TargetID.h b/clang/include/clang/Basic/TargetID.h index cef9cb5f0fb21..902151d76556d 100644 --- a/clang/include/clang/Basic/TargetID.h +++ b/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 diff --git a/clang/lib/Basic/TargetID.cpp b/clang/lib/Basic/TargetID.cpp index 96e45ab5d9020..0aca490e17903 100644 --- a/clang/lib/Basic/TargetID.cpp +++ b/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 diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp index 426fc796ffc20..de8d4601210ae 100644 --- a/clang/lib/Driver/Driver.cpp +++ b/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? diff --git a/clang/test/Driver/linker-wrapper-hip-no-rdc.c b/clang/test/Driver/linker-wrapper-hip-no-rdc.c index 7545205f22ea0..addbec5fae613 100644 --- a/clang/test/Driver/linker-wrapper-hip-no-rdc.c +++ b/clang/test/Driver/linker-wrapper-hip-no-rdc.c @@ -1,4 +1,3 @@ -// UNSUPPORTED: system-windows // REQUIRES: amdgpu-registered-target // REQUIRES: lld @@ -11,14 +10,30 @@ __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 @@ -26,16 +41,17 @@ __attribute__((visibility("protected"), used)) int x; // 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 diff --git a/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp b/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp index 4a4a43db6ef25..fcb6c591ec5ca 100644 --- a/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp +++ b/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(); _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
