From fa0bf9b9c72ea58227edb13e8a6c360433b673da Mon Sep 17 00:00:00 2001 From: Samiii777 <58442200+Samiii777@users.noreply.github.com> Date: Thu, 7 May 2026 14:32:43 -0400 Subject: [PATCH] [offload-arch] Fix HIP DLL discovery and loading on Windows (#194063) On Windows, offload-arch fails to find or loads the wrong amdhip64 DLL when running from a source-built LLVM/Clang installation where the executable and HIP runtime are in different subdirectories. Three fixes: 1. getSearchPaths(): walk parent directories appending /bin to each, so layouts like /lib/llvm/bin/offload-arch can discover /bin/amdhip64_*.dll. Capped at 6 levels with root detection. Case-insensitive dedup for Windows paths. 2. findNewestHIPDLL(): use stable_sort to preserve search-path order on version ties, so a colocated build DLL wins over a system copy. 3. printGPUsByHIP(): prime the DLL load with LoadLibraryExW and LOAD_WITH_ALTERED_SEARCH_PATH so transitive dependencies resolve from the DLL own directory. Uses LLVM convertUTF8ToUTF16String for path conversion. Also fixes two pre-existing bugs: inverted current_path error check (line 138) and missing break in HipApiVersion switch (line 421). --------- Co-authored-by: Samiii777 --- clang/tools/offload-arch/AMDGPUArchByHIP.cpp | 67 +++++++++- clang/unittests/CMakeLists.txt | 1 + clang/unittests/offload-arch/CMakeLists.txt | 10 ++ .../offload-arch/OffloadArchTest.cpp | 114 ++++++++++++++++++ 4 files changed, 188 insertions(+), 4 deletions(-) create mode 100644 clang/unittests/offload-arch/CMakeLists.txt create mode 100644 clang/unittests/offload-arch/OffloadArchTest.cpp diff --git a/clang/tools/offload-arch/AMDGPUArchByHIP.cpp b/clang/tools/offload-arch/AMDGPUArchByHIP.cpp index d7f6d79b135d..9cfccf416f78 100644 --- a/clang/tools/offload-arch/AMDGPUArchByHIP.cpp +++ b/clang/tools/offload-arch/AMDGPUArchByHIP.cpp @@ -22,6 +22,7 @@ #include "llvm/Support/Process.h" #include "llvm/Support/Program.h" #include "llvm/Support/VersionTuple.h" +#include "llvm/Support/WithColor.h" #include "llvm/Support/raw_ostream.h" #include #include @@ -82,13 +83,40 @@ static cl::opt HipApi( cl::init(HipApiVersion::Auto), cl::cat(AMDGPUArchByHIPCategory)); #ifdef _WIN32 +// Return candidate bin/ directories by walking parent dirs of ExeDir. +SmallVector getCandidateBinPaths(StringRef ExeDir) { + SmallVector Paths; + Paths.push_back(sys::path::convert_to_slash(ExeDir)); + // Search parent/bin dirs: /lib/llvm/bin needs depth 2, + // /opt/rocm/lib/llvm/bin needs 3. Cap at 6. + constexpr int MaxParentLevels = 6; + SmallString<256> Parent(sys::path::parent_path(ExeDir)); + for (int Depth = 0; Depth < MaxParentLevels && !Parent.empty(); ++Depth) { + if (sys::path::root_path(Parent) == StringRef(Parent)) + break; + SmallString<256> Candidate(Parent); + sys::path::append(Candidate, "bin"); + std::string CandStr = sys::path::convert_to_slash(Candidate); + auto IsDup = [&](const std::string &P) { + return StringRef(P).equals_insensitive(CandStr); + }; + if (llvm::none_of(Paths, IsDup)) + Paths.push_back(CandStr); + Parent = SmallString<256>(sys::path::parent_path(Parent)); + } + return Paths; +} + static std::vector getSearchPaths() { std::vector Paths; // Get the directory of the current executable if (auto MainExe = sys::fs::getMainExecutable(nullptr, nullptr); - !MainExe.empty()) - Paths.push_back(sys::path::parent_path(MainExe).str()); + !MainExe.empty()) { + StringRef ExeDir = sys::path::parent_path(MainExe); + auto BinPaths = getCandidateBinPaths(ExeDir); + Paths.insert(Paths.end(), BinPaths.begin(), BinPaths.end()); + } // Get the system directory wchar_t SystemDirectory[MAX_PATH]; @@ -129,7 +157,8 @@ static std::vector getSearchPaths() { } // Custom comparison function for dll name -static bool compareVersions(StringRef A, StringRef B) { +// Returns true when A's version is greater than B's (descending order). +bool compareVersions(StringRef A, StringRef B) { auto ParseVersion = [](StringRef S) -> VersionTuple { StringRef Filename = sys::path::filename(S); size_t Pos = Filename.find_last_of('_'); @@ -181,7 +210,9 @@ static std::pair findNewestHIPDLL() { if (DLLNames.empty()) return {"amdhip64.dll", true}; - llvm::sort(DLLNames, compareVersions); + // stable_sort preserves the insertion order from getSearchPaths() on + // version ties, so a colocated build DLL wins over a system copy. + llvm::stable_sort(DLLNames, compareVersions); return {DLLNames[0], false}; #else // On Linux, fallback to default shared object @@ -189,6 +220,28 @@ static std::pair findNewestHIPDLL() { #endif } +#ifdef _WIN32 +// Pre-load DLL with LOAD_WITH_ALTERED_SEARCH_PATH so transitive deps +// resolve from its directory. Pinned so getPermanentLibrary reuses it. +static void primeLibraryLoad(StringRef Path) { + // One DLL primed per process; subsequent calls are no-ops. + // Not thread-safe, but offload-arch is single-threaded. + static HMODULE PinnedModule = nullptr; + if (PinnedModule || !sys::path::is_absolute(Path)) + return; + SmallVector WPath; + if (!convertUTF8ToUTF16String(Path, WPath)) + return; + WPath.push_back(0); + PinnedModule = LoadLibraryExW(reinterpret_cast(WPath.data()), + nullptr, LOAD_WITH_ALTERED_SEARCH_PATH); + DWORD Err = GetLastError(); + if (!PinnedModule && Verbose) + WithColor::note() << "priming LoadLibraryExW failed for " << Path + << " (error " << Err << ")\n"; +} +#endif + int printGPUsByHIP() { auto [DynamicHIPPath, IsFallback] = findNewestHIPDLL(); @@ -200,6 +253,11 @@ int printGPUsByHIP() { } std::string ErrMsg; +#ifdef _WIN32 + // Prime DLL load so transitive deps resolve from its directory. + if (!IsFallback) + primeLibraryLoad(DynamicHIPPath); +#endif auto DynlibHandle = std::make_unique( llvm::sys::DynamicLibrary::getPermanentLibrary(DynamicHIPPath.c_str(), &ErrMsg)); @@ -356,6 +414,7 @@ int printGPUsByHIP() { break; case HipApiVersion::Unversioned: OK = TryUnversioned(I); + break; } if (ArchName.empty()) { diff --git a/clang/unittests/CMakeLists.txt b/clang/unittests/CMakeLists.txt index 2ae0906dd157..71c7a6842fed 100644 --- a/clang/unittests/CMakeLists.txt +++ b/clang/unittests/CMakeLists.txt @@ -110,6 +110,7 @@ add_subdirectory(Support) if (CLANG_ENABLE_CIR) add_subdirectory(CIR) endif() +add_subdirectory(offload-arch) # If we're doing a single merged clang unit test binary, add that target after # all the previous subdirectories have been processed. diff --git a/clang/unittests/offload-arch/CMakeLists.txt b/clang/unittests/offload-arch/CMakeLists.txt new file mode 100644 index 000000000000..db4fa5ceba5e --- /dev/null +++ b/clang/unittests/offload-arch/CMakeLists.txt @@ -0,0 +1,10 @@ +if(CMAKE_SYSTEM_NAME STREQUAL "Windows") + add_distinct_clang_unittest(OffloadArchTests + OffloadArchTest.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/../../tools/offload-arch/AMDGPUArchByHIP.cpp + CLANG_LIBS + clangBasic + LLVM_COMPONENTS + Support + ) +endif() diff --git a/clang/unittests/offload-arch/OffloadArchTest.cpp b/clang/unittests/offload-arch/OffloadArchTest.cpp new file mode 100644 index 000000000000..4b07af39a918 --- /dev/null +++ b/clang/unittests/offload-arch/OffloadArchTest.cpp @@ -0,0 +1,114 @@ +//===-- OffloadArchTest.cpp - Tests for offload-arch helpers ---*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "llvm/ADT/SmallVector.h" +#include "llvm/ADT/StringRef.h" +#include "llvm/Support/CommandLine.h" +#include "gtest/gtest.h" +#include +#include + +// Defined in AMDGPUArchByHIP.cpp (non-static, compiled into this test). +#ifdef _WIN32 +bool compareVersions(llvm::StringRef A, llvm::StringRef B); +llvm::SmallVector getCandidateBinPaths(llvm::StringRef ExeDir); +#endif + +using namespace llvm; + +cl::opt Verbose("offload-arch-test-verbose", cl::Hidden, cl::init(false)); + +#ifdef _WIN32 + +// --- compareVersions --- + +TEST(CompareVersions, HigherVersionWins) { + EXPECT_TRUE( + compareVersions("C:/bin/amdhip64_7.dll", "C:/bin/amdhip64_6.dll")); + EXPECT_FALSE( + compareVersions("C:/bin/amdhip64_6.dll", "C:/bin/amdhip64_7.dll")); +} + +TEST(CompareVersions, EqualVersionsReturnFalse) { + EXPECT_FALSE(compareVersions("C:/a/amdhip64_7.dll", "C:/b/amdhip64_7.dll")); +} + +TEST(CompareVersions, MultiDigitVersions) { + EXPECT_TRUE(compareVersions("amdhip64_12.dll", "amdhip64_6.dll")); +} + +TEST(CompareVersions, StableSortPreservesInsertionOrder) { + std::vector DLLs = {"C:/rocm/bin/amdhip64_7.dll", + "C:/Windows/System32/amdhip64_7.dll"}; + llvm::stable_sort(DLLs, compareVersions); + EXPECT_EQ(DLLs[0], "C:/rocm/bin/amdhip64_7.dll"); +} + +// --- getCandidateBinPaths --- + +TEST(CandidateBinPaths, FindsParentBin) { + auto Paths = getCandidateBinPaths("C:/root/lib/llvm/bin"); + bool Found = false; + for (const auto &P : Paths) + if (StringRef(P).equals_insensitive("C:/root/bin")) + Found = true; + EXPECT_TRUE(Found); +} + +TEST(CandidateBinPaths, NoDuplicatesWhenExeInBin) { + auto Paths = getCandidateBinPaths("C:/root/bin"); + int Count = 0; + for (const auto &P : Paths) + if (StringRef(P).equals_insensitive("C:/root/bin")) + Count++; + EXPECT_EQ(Count, 1); +} + +TEST(CandidateBinPaths, CaseInsensitiveDedup) { + // Paths differing only in case should not both appear. + auto Paths = getCandidateBinPaths("C:/Root/Lib/Bin"); + int Count = 0; + for (const auto &P : Paths) + if (StringRef(P).equals_insensitive("C:/Root/bin")) + Count++; + EXPECT_LE(Count, 1); +} + +TEST(CandidateBinPaths, StopsWithinBound) { + auto Paths = getCandidateBinPaths("C:/a/b/c/d/e/f/g/h"); + // MaxParentLevels=6 + self = 7 max entries. + EXPECT_LE(Paths.size(), 7u); +} + +TEST(CandidateBinPaths, RootInput) { + auto Paths = getCandidateBinPaths("C:/"); + // Should produce at least 1 entry (self) and not crash. + EXPECT_GE(Paths.size(), 1u); +} + +TEST(CandidateBinPaths, NonAsciiPath) { + // Paths with non-ASCII characters should not crash. + auto Paths = getCandidateBinPaths("C:/\xC3\xBCser/\xC3\xA4pp/bin"); + EXPECT_GE(Paths.size(), 1u); +} + +TEST(CandidateBinPaths, UnicodePathDedup) { + auto Paths = + getCandidateBinPaths("C:/\xE6\x97\xA5\xE6\x9C\xAC\xE8\xAA\x9E/lib/bin"); + // Should produce entries without crashing on CJK characters. + EXPECT_GE(Paths.size(), 1u); +} + +TEST(CandidateBinPaths, NoDriveRootBin) { + auto Paths = getCandidateBinPaths("C:\\Program Files\\AMD\\HIP\\bin"); + for (const auto &P : Paths) + EXPECT_FALSE(StringRef(P).equals_insensitive("C:/bin")) + << "Drive-root bin/ must not appear (DLL planting risk)"; +} + +#endif // _WIN32