From 658ccdc8a5d6c2df4c05b2ae602b972f49225925 Mon Sep 17 00:00:00 2001 From: Ioannis Assiouras Date: Wed, 16 Sep 2026 23:07:01 +0000 Subject: [PATCH 1/9] hip-tests: write contract test artifacts to the temp dir, not cwd MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The graph_debug and library_file contract tests built bare relative output paths, so the files landed in the current working directory. That only works when cwd happens to be writable. Running an installed test tree as an unprivileged user it is not: /opt/rocm/share/hip/catch_tests is root:root drwxr-xr-x, so hipGraphDebugDotPrint() and the code-object write fail on permissions before the API under test is meaningfully exercised. The failure is recorded as an ordinary assertion failure with no indication that the cause was the filesystem, which makes it easy to misread as a runtime regression on whatever target happens to be under test. Both paths now go through std::filesystem::temp_directory_path(), which is writable by definition and honours TMPDIR. Changes: - graph_debug: UniqueDotPath() returns a path under the temp directory. - library_file: WriteCodeObjectFile() writes the code object under the temp directory, and its stale "current working directory" comment is corrected. - library_file: the filename gains a per-process token. The old fixed name was adequate in a per-run cwd but not in a shared temp dir, where a leftover file owned by another user is not writable. This mirrors what UniqueDotPath() already does in the graph_debug contract. - Both files include ; library_file also includes for the intptr_t cast it now performs. The tests already removed these files by the returned path, so cleanup needed no change. catch/CMakeLists.txt sets CMAKE_CXX_STANDARD 17, and contract executables are built through hip_add_exe_to_target(), which already links stdc++fs on UNIX (catch/cmake/hip-tests.cmake), so no CMake or linkage change is required. Note: catch/unit/memory/mempool_common.hh has the same defect class - it binds an IPC socket at a bare relative sun_path and gets EACCES from a read-only cwd. It is not touched here. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../graph_debug/test_hip_graph_debug_contract.cc | 10 ++++++---- .../test_hip_library_file_contract.cc | 15 +++++++++++---- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/projects/hip-tests/catch/contract/graph_debug/test_hip_graph_debug_contract.cc b/projects/hip-tests/catch/contract/graph_debug/test_hip_graph_debug_contract.cc index b0552f36ccac..74ec823d1950 100644 --- a/projects/hip-tests/catch/contract/graph_debug/test_hip_graph_debug_contract.cc +++ b/projects/hip-tests/catch/contract/graph_debug/test_hip_graph_debug_contract.cc @@ -8,6 +8,7 @@ #include #include #include +#include #include #include @@ -17,13 +18,14 @@ namespace { constexpr size_t kByteCount = 64; -// Builds a unique temporary file path for the exported dot graph so concurrent -// test binaries do not collide on a shared filename. +// Use the temp directory because installed test directories may be read-only. std::string UniqueDotPath() { int device = 0; HIP_CHECK(hipGetDevice(&device)); - return std::string("hip_contract_graph_debug_") + std::to_string(device) + "_" + - std::to_string(static_cast(reinterpret_cast(&device))) + ".dot"; + return (std::filesystem::temp_directory_path() / + (std::string("hip_contract_graph_debug_") + std::to_string(device) + "_" + + std::to_string(static_cast(reinterpret_cast(&device))) + ".dot")) + .string(); } hipMemsetParams MakeByteMemsetParams(void* device_ptr, unsigned int value) { diff --git a/projects/hip-tests/catch/contract/library_file/test_hip_library_file_contract.cc b/projects/hip-tests/catch/contract/library_file/test_hip_library_file_contract.cc index bf627ffd0471..3a2dc3e93b2d 100644 --- a/projects/hip-tests/catch/contract/library_file/test_hip_library_file_contract.cc +++ b/projects/hip-tests/catch/contract/library_file/test_hip_library_file_contract.cc @@ -8,8 +8,10 @@ #include #include +#include #include #include +#include #include #include @@ -68,15 +70,20 @@ bool CompileLibrarySource(std::vector& code) { return true; } -// Compiles the library source and writes it to a unique per-test file in the -// current working directory, returning the path. Skips when HIPRTC is -// unavailable. The caller must remove the file when done. +// Compile to a temp file: installed test directories may be read-only. +// Returns the path; the caller must remove the file. std::string WriteCodeObjectFile(const char* suffix) { std::vector code; if (!CompileLibrarySource(code)) { HIP_SKIP_TEST("HIPRTC compilation is not supported by this device/runtime path."); } - const std::string path = std::string("hip-contract-library-file-") + suffix + ".code"; + // Add an address token to reduce collisions in the shared temp directory. + const std::string token = + std::to_string(static_cast(reinterpret_cast(&code))); + const std::string path = + (std::filesystem::temp_directory_path() / + (std::string("hip-contract-library-file-") + suffix + "-" + token + ".code")) + .string(); std::ofstream out(path, std::ios::binary); REQUIRE(out.is_open()); out.write(code.data(), static_cast(code.size())); From f958679c95412abc4ae3119a655b31fb905a8155 Mon Sep 17 00:00:00 2001 From: Ioannis Assiouras Date: Thu, 17 Sep 2026 20:35:59 +0000 Subject: [PATCH 2/9] hip-tests: move IPC socket paths to the temp dir and simplify artifact naming MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extends the contract-test fix to the two remaining places that build bare relative paths, and simplifies the naming introduced earlier. mempool_common.hh and hip_vmm_common.hh bind an AF_UNIX socket at a bare relative sun_path, so they get EACCES from a read-only cwd for the same reason the contract tests did. Both now build the path under std::filesystem::temp_directory_path() via a shared ipcSocketPath(pid) helper, guarded per platform (!HT_WIN and __linux__ respectively) since AF_UNIX paths are POSIX-only here. sun_path is a fixed-size buffer, and the temp directory prefix makes the path longer, so both call sites now reject an over-long path explicitly instead of letting strcpy truncate it into a different, silently wrong socket name. Artifact naming in the contract tests is simplified to the PID, replacing the address-of-a-local token used previously. The PID is stable within a process and distinct between concurrent test binaries, which is what these names need, and it reads better than a hashed stack address. Changes: - mempool_common.hh, hip_vmm_common.hh: ipcSocketPath(pid) helper placing the socket under the temp directory; platform guards; explicit length check against sizeof(sun_path) - 1 at both the bind and the connect site. - graph_debug, library_file: name artifacts by PID; drop the temp-directory helper in favour of the direct temp_directory_path() call. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../test_hip_graph_debug_contract.cc | 19 ++++++++---- .../test_hip_library_file_contract.cc | 16 ++++++---- .../catch/unit/memory/mempool_common.hh | 27 +++++++++++++---- .../virtualMemoryManagement/hip_vmm_common.hh | 29 ++++++++++++++----- 4 files changed, 67 insertions(+), 24 deletions(-) diff --git a/projects/hip-tests/catch/contract/graph_debug/test_hip_graph_debug_contract.cc b/projects/hip-tests/catch/contract/graph_debug/test_hip_graph_debug_contract.cc index 74ec823d1950..a2d6d7cc807d 100644 --- a/projects/hip-tests/catch/contract/graph_debug/test_hip_graph_debug_contract.cc +++ b/projects/hip-tests/catch/contract/graph_debug/test_hip_graph_debug_contract.cc @@ -7,10 +7,17 @@ #include #include #include -#include #include +#include #include +#ifdef _WIN32 +#include +#define getpid _getpid +#else +#include +#endif + #include #include #include @@ -19,12 +26,12 @@ namespace { constexpr size_t kByteCount = 64; // Use the temp directory because installed test directories may be read-only. -std::string UniqueDotPath() { +std::string DotPath() { int device = 0; HIP_CHECK(hipGetDevice(&device)); return (std::filesystem::temp_directory_path() / - (std::string("hip_contract_graph_debug_") + std::to_string(device) + "_" + - std::to_string(static_cast(reinterpret_cast(&device))) + ".dot")) + ("hip_contract_graph_debug_" + std::to_string(device) + "_" + + std::to_string(getpid()) + ".dot")) .string(); } @@ -63,7 +70,7 @@ HIP_TEST_CASE(Contract_GraphDebug_HipGraphDebugDotPrint_Default_WritesNonEmptyFi hipMemsetParams memset_params = MakeByteMemsetParams(device_ptr, 0x5A); HIP_CHECK(hipGraphAddMemsetNode(&node, graph, nullptr, 0, &memset_params)); - const std::string path = UniqueDotPath(); + const std::string path = DotPath(); std::remove(path.c_str()); // Exporting a non-empty graph to a dot file must succeed (or report the @@ -89,7 +96,7 @@ HIP_TEST_CASE(Contract_GraphDebug_HipGraphDebugDotPrint_Default_VerboseFlagIsAcc cleanup.Add([graph] { (void)hipGraphDestroy(graph); }); HIP_CHECK(hipGraphAddEmptyNode(&node, graph, nullptr, 0)); - const std::string path = UniqueDotPath(); + const std::string path = DotPath(); std::remove(path.c_str()); // The verbose flag augments the output but must not change the success diff --git a/projects/hip-tests/catch/contract/library_file/test_hip_library_file_contract.cc b/projects/hip-tests/catch/contract/library_file/test_hip_library_file_contract.cc index 3a2dc3e93b2d..3699194783fc 100644 --- a/projects/hip-tests/catch/contract/library_file/test_hip_library_file_contract.cc +++ b/projects/hip-tests/catch/contract/library_file/test_hip_library_file_contract.cc @@ -8,13 +8,19 @@ #include #include -#include #include -#include #include +#include #include #include +#ifdef _WIN32 +#include +#define getpid _getpid +#else +#include +#endif + // The library-from-file and managed-symbol APIs (hipLibraryLoadFromFile, // hipLibraryGetManaged) are exercised on both backends: on NVIDIA they map to // the CUDA driver cuLibrary* entry points. @@ -77,12 +83,10 @@ std::string WriteCodeObjectFile(const char* suffix) { if (!CompileLibrarySource(code)) { HIP_SKIP_TEST("HIPRTC compilation is not supported by this device/runtime path."); } - // Add an address token to reduce collisions in the shared temp directory. - const std::string token = - std::to_string(static_cast(reinterpret_cast(&code))); const std::string path = (std::filesystem::temp_directory_path() / - (std::string("hip-contract-library-file-") + suffix + "-" + token + ".code")) + ("hip-contract-library-file-" + std::string(suffix) + "-" + + std::to_string(getpid()) + ".code")) .string(); std::ofstream out(path, std::ios::binary); REQUIRE(out.is_open()); diff --git a/projects/hip-tests/catch/unit/memory/mempool_common.hh b/projects/hip-tests/catch/unit/memory/mempool_common.hh index 4d559d26961f..56d0c4d3abe5 100644 --- a/projects/hip-tests/catch/unit/memory/mempool_common.hh +++ b/projects/hip-tests/catch/unit/memory/mempool_common.hh @@ -13,6 +13,7 @@ #include #include +#include #include namespace { @@ -628,6 +629,13 @@ struct ipcHdl { char *name; }; +#if !HT_WIN +// IPC socket path in the temp directory so tests work from read-only cwd. +inline std::string ipcSocketPath(pid_t pid) { + return (std::filesystem::temp_directory_path() / std::to_string(pid)).string(); +} +#endif + class ipcSocketCom { ipcHdl *handle; @@ -645,8 +653,8 @@ class ipcSocketCom { int server_fd; struct sockaddr_un servaddr; - char name[16]; - sprintf(name, "%u", getpid()); + std::string nameStr = ipcSocketPath(getpid()); + const char *name = nameStr.c_str(); handle = new ipcHdl; if (nullptr == handle) { @@ -729,10 +737,14 @@ class ipcSocketCom { bzero(&cliaddr, sizeof(cliaddr)); cliaddr.sun_family = AF_UNIX; - char name[16]; - sprintf(name, "%u", getpid()); + std::string nameStr = ipcSocketPath(getpid()); + const char *name = nameStr.c_str(); + if (strlen(name) > (sizeof(cliaddr.sun_path) - 1)) { + perror("Socket failure: Cannot bind provided name to socket. Name too large"); + return -1; + } strcpy(cliaddr.sun_path, name); if (bind(sock, (struct sockaddr *)&cliaddr, sizeof(cliaddr)) < 0) { perror("Socket failure: Binding socket failed"); @@ -892,7 +904,12 @@ public: bzero(&cliaddr, sizeof(cliaddr)); cliaddr.sun_family = AF_UNIX; - strcpy(cliaddr.sun_path, std::to_string(process).c_str()); + std::string destPath = ipcSocketPath(process); + if (destPath.size() > (sizeof(cliaddr.sun_path) - 1)) { + perror("Socket failure: Cannot address socket. Name too large"); + return -1; + } + strcpy(cliaddr.sun_path, destPath.c_str()); int sendfd = (int)shareableHdl; diff --git a/projects/hip-tests/catch/unit/virtualMemoryManagement/hip_vmm_common.hh b/projects/hip-tests/catch/unit/virtualMemoryManagement/hip_vmm_common.hh index 21c1286ade6b..e477f75a3263 100644 --- a/projects/hip-tests/catch/unit/virtualMemoryManagement/hip_vmm_common.hh +++ b/projects/hip-tests/catch/unit/virtualMemoryManagement/hip_vmm_common.hh @@ -17,6 +17,7 @@ #include #include #endif +#include #include "hip_test_context.hh" #define checkVMMSupported(device) \ @@ -71,6 +72,13 @@ struct ipcHdl { char *name; }; +#ifdef __linux__ +// IPC socket path in the temp directory so tests work from read-only cwd. +inline std::string ipcSocketPath(pid_t pid) { + return (std::filesystem::temp_directory_path() / std::to_string(pid)).string(); +} +#endif + class ipcSocketCom { ipcHdl *handle; // method to create socket from server @@ -78,9 +86,8 @@ class ipcSocketCom { int server_fd; struct sockaddr_un servaddr; - char name[16]; - // Create a unique socket name based on current pid - sprintf(name, "%u", getpid()); + std::string nameStr = ipcSocketPath(getpid()); + const char *name = nameStr.c_str(); // Create the socket handle handle = new ipcHdl; @@ -140,11 +147,14 @@ class ipcSocketCom { bzero(&cliaddr, sizeof(cliaddr)); cliaddr.sun_family = AF_UNIX; - char name[16]; - // Create a unique socket name based on current process id. - sprintf(name, "%u", getpid()); + std::string nameStr = ipcSocketPath(getpid()); + const char *name = nameStr.c_str(); + if (strlen(name) > (sizeof(cliaddr.sun_path) - 1)) { + perror("Socket failure: Cannot bind provided name to socket. Name too large"); + return -1; + } strcpy(cliaddr.sun_path, name); if (bind(sock, (struct sockaddr *)&cliaddr, sizeof(cliaddr)) < 0) { perror("Socket failure: Binding socket failed"); @@ -242,7 +252,12 @@ public: // Construct client address to send this SHareable handle to bzero(&cliaddr, sizeof(cliaddr)); cliaddr.sun_family = AF_UNIX; - strcpy(cliaddr.sun_path, std::to_string(process).c_str()); + std::string destPath = ipcSocketPath(process); + if (destPath.size() > (sizeof(cliaddr.sun_path) - 1)) { + perror("Socket failure: Cannot address socket. Name too large"); + return -1; + } + strcpy(cliaddr.sun_path, destPath.c_str()); // Send corresponding shareable handle to the client int sendfd = (int)shareableHdl; From e8ba70c0f86b1f0abc60aabe372d89f3b6f0b250 Mon Sep 17 00:00:00 2001 From: Ioannis Assiouras Date: Thu, 17 Sep 2026 20:45:45 +0000 Subject: [PATCH 3/9] hip-tests: report over-long IPC socket paths without perror MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review feedback on the sun_path length checks. These checks are local validation, not syscall failures, so errno is not set when they trip. perror() appends strerror(errno) regardless, so the message came out with a stale suffix - typically "... Name too large: Success" - which reads as a contradiction and hides what actually went wrong. Print with fprintf(stderr, ...) instead, which says exactly what happened and nothing more. Applies to all three length checks in each of mempool_common.hh and hip_vmm_common.hh: the two added by this PR (the bind-site and connect-site guards on the client address) and the pre-existing guard on the server address, which has the same defect and is converted here for consistency rather than leaving two spellings of the same check side by side. The perror() calls that follow a real socket()/bind()/send()/recv() failure are unchanged, since errno is meaningful there. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- projects/hip-tests/catch/unit/memory/mempool_common.hh | 6 +++--- .../catch/unit/virtualMemoryManagement/hip_vmm_common.hh | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/projects/hip-tests/catch/unit/memory/mempool_common.hh b/projects/hip-tests/catch/unit/memory/mempool_common.hh index 56d0c4d3abe5..985a4a7b0643 100644 --- a/projects/hip-tests/catch/unit/memory/mempool_common.hh +++ b/projects/hip-tests/catch/unit/memory/mempool_common.hh @@ -677,7 +677,7 @@ class ipcSocketCom { size_t len = strlen(name); if (len > (sizeof(servaddr.sun_path) - 1)) { - perror("Socket failure: Cannot bind provided name to socket. Name too large"); + fprintf(stderr, "Socket failure: Cannot bind provided name to socket. Name too large\n"); return -1; } @@ -742,7 +742,7 @@ class ipcSocketCom { const char *name = nameStr.c_str(); if (strlen(name) > (sizeof(cliaddr.sun_path) - 1)) { - perror("Socket failure: Cannot bind provided name to socket. Name too large"); + fprintf(stderr, "Socket failure: Cannot bind provided name to socket. Name too large\n"); return -1; } strcpy(cliaddr.sun_path, name); @@ -906,7 +906,7 @@ public: cliaddr.sun_family = AF_UNIX; std::string destPath = ipcSocketPath(process); if (destPath.size() > (sizeof(cliaddr.sun_path) - 1)) { - perror("Socket failure: Cannot address socket. Name too large"); + fprintf(stderr, "Socket failure: Cannot address socket. Name too large\n"); return -1; } strcpy(cliaddr.sun_path, destPath.c_str()); diff --git a/projects/hip-tests/catch/unit/virtualMemoryManagement/hip_vmm_common.hh b/projects/hip-tests/catch/unit/virtualMemoryManagement/hip_vmm_common.hh index e477f75a3263..044be33fa23f 100644 --- a/projects/hip-tests/catch/unit/virtualMemoryManagement/hip_vmm_common.hh +++ b/projects/hip-tests/catch/unit/virtualMemoryManagement/hip_vmm_common.hh @@ -112,7 +112,7 @@ class ipcSocketCom { size_t len = strlen(name); if (len > (sizeof(servaddr.sun_path) - 1)) { - perror("Socket failure: Cannot bind provided name to socket. Name too large"); + fprintf(stderr, "Socket failure: Cannot bind provided name to socket. Name too large\n"); return -1; } @@ -152,7 +152,7 @@ class ipcSocketCom { const char *name = nameStr.c_str(); if (strlen(name) > (sizeof(cliaddr.sun_path) - 1)) { - perror("Socket failure: Cannot bind provided name to socket. Name too large"); + fprintf(stderr, "Socket failure: Cannot bind provided name to socket. Name too large\n"); return -1; } strcpy(cliaddr.sun_path, name); @@ -254,7 +254,7 @@ public: cliaddr.sun_family = AF_UNIX; std::string destPath = ipcSocketPath(process); if (destPath.size() > (sizeof(cliaddr.sun_path) - 1)) { - perror("Socket failure: Cannot address socket. Name too large"); + fprintf(stderr, "Socket failure: Cannot address socket. Name too large\n"); return -1; } strcpy(cliaddr.sun_path, destPath.c_str()); From dc43f30f2062f788c8605ffd1ba3bc463c9b1f12 Mon Sep 17 00:00:00 2001 From: Ioannis Assiouras Date: Fri, 18 Sep 2026 08:56:10 +0000 Subject: [PATCH 4/9] hip-tests: release socket and handle on IPC setup failure paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moving the IPC socket path into the temp directory means it can now exceed sizeof(sun_path) - 1, so the over-length check in these helpers is reachable for the first time. That early return left the descriptor open and the ipcHdl allocation live. The neighbouring socket() and bind() failure returns have the same omission and are fixed alongside it, so every failure path in these four functions now cleans up consistently. Changes: - mempool_common.hh: close the fd and delete/reset handle on the socket(), over-length and bind() failure returns in createSocket() and openSocket() - hip_vmm_common.hh: same four-site cleanup in createSocket() and openSocket() Verified by extracting the patched functions verbatim into a standalone harness and counting /proc/self/fd across 200 iterations of each helper: path length | before | after 40 (normal) 400 200 (remainder held by live handles) 120 (over-long) 400 0 The 120-byte arm is the newly reachable branch; it leaked one fd and one ipcHdl per call and now leaks neither. Addresses the resource-leak findings raised in review. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../catch/unit/memory/mempool_common.hh | 16 ++++++++++++++++ .../virtualMemoryManagement/hip_vmm_common.hh | 16 ++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/projects/hip-tests/catch/unit/memory/mempool_common.hh b/projects/hip-tests/catch/unit/memory/mempool_common.hh index 985a4a7b0643..58cfae8b0539 100644 --- a/projects/hip-tests/catch/unit/memory/mempool_common.hh +++ b/projects/hip-tests/catch/unit/memory/mempool_common.hh @@ -668,6 +668,8 @@ class ipcSocketCom { if ((server_fd = socket(AF_UNIX, SOCK_DGRAM, 0)) < 0) { perror("Socket failure: Socket creation failed"); + delete handle; + handle = nullptr; return -1; } @@ -678,6 +680,9 @@ class ipcSocketCom { size_t len = strlen(name); if (len > (sizeof(servaddr.sun_path) - 1)) { fprintf(stderr, "Socket failure: Cannot bind provided name to socket. Name too large\n"); + close(server_fd); + delete handle; + handle = nullptr; return -1; } @@ -685,6 +690,9 @@ class ipcSocketCom { if (bind(server_fd, (struct sockaddr *)&servaddr, SUN_LEN(&servaddr)) < 0) { perror("Socket failure: Binding socket failed"); + close(server_fd); + delete handle; + handle = nullptr; return -1; } @@ -732,6 +740,8 @@ class ipcSocketCom { if ((sock = socket(AF_UNIX, SOCK_DGRAM, 0)) < 0) { perror("IPC failure:Socket creation error"); + delete handle; + handle = nullptr; return -1; } @@ -743,11 +753,17 @@ class ipcSocketCom { if (strlen(name) > (sizeof(cliaddr.sun_path) - 1)) { fprintf(stderr, "Socket failure: Cannot bind provided name to socket. Name too large\n"); + close(sock); + delete handle; + handle = nullptr; return -1; } strcpy(cliaddr.sun_path, name); if (bind(sock, (struct sockaddr *)&cliaddr, sizeof(cliaddr)) < 0) { perror("Socket failure: Binding socket failed"); + close(sock); + delete handle; + handle = nullptr; return -1; } diff --git a/projects/hip-tests/catch/unit/virtualMemoryManagement/hip_vmm_common.hh b/projects/hip-tests/catch/unit/virtualMemoryManagement/hip_vmm_common.hh index 044be33fa23f..042829f9b8a9 100644 --- a/projects/hip-tests/catch/unit/virtualMemoryManagement/hip_vmm_common.hh +++ b/projects/hip-tests/catch/unit/virtualMemoryManagement/hip_vmm_common.hh @@ -103,6 +103,8 @@ class ipcSocketCom { // Creating socket if ((server_fd = socket(AF_UNIX, SOCK_DGRAM, 0)) == 0) { perror("Socket failure: Socket creation failed"); + delete handle; + handle = nullptr; return -1; } @@ -113,6 +115,9 @@ class ipcSocketCom { size_t len = strlen(name); if (len > (sizeof(servaddr.sun_path) - 1)) { fprintf(stderr, "Socket failure: Cannot bind provided name to socket. Name too large\n"); + close(server_fd); + delete handle; + handle = nullptr; return -1; } @@ -120,6 +125,9 @@ class ipcSocketCom { if (bind(server_fd, (struct sockaddr *)&servaddr, SUN_LEN(&servaddr)) < 0) { perror("Socket failure: Binding socket failed"); + close(server_fd); + delete handle; + handle = nullptr; return -1; } @@ -142,6 +150,8 @@ class ipcSocketCom { if ((sock = socket(AF_UNIX, SOCK_DGRAM, 0)) < 0) { perror("IPC failure:Socket creation error"); + delete handle; + handle = nullptr; return -1; } @@ -153,11 +163,17 @@ class ipcSocketCom { if (strlen(name) > (sizeof(cliaddr.sun_path) - 1)) { fprintf(stderr, "Socket failure: Cannot bind provided name to socket. Name too large\n"); + close(sock); + delete handle; + handle = nullptr; return -1; } strcpy(cliaddr.sun_path, name); if (bind(sock, (struct sockaddr *)&cliaddr, sizeof(cliaddr)) < 0) { perror("Socket failure: Binding socket failed"); + close(sock); + delete handle; + handle = nullptr; return -1; } From c1eb0ac7da431e49aaa022a7239ffe75c6d219e0 Mon Sep 17 00:00:00 2001 From: Ioannis Assiouras Date: Thu, 24 Sep 2026 09:45:49 +0000 Subject: [PATCH 5/9] hip-tests: share the IPC socket naming between mempool and VMM MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both IPC test headers carried their own copy of the socket path helper, naming the AF_UNIX rendezvous by bare pid. Move the naming into a single header and give it a tag identifying the mount namespace, so peers that do not share a temp directory cannot collide on identical pids. Both ends derive the tag independently rather than exchanging it, the same way the shared-memory segment derives the parent identity via getParentProcessId(). That is what makes it survive SpawnProc, which fork()s and then execvp()s: the child re-runs the code in a fresh process image and computes the same value. A generated per-run token does not have that property. Changes: - Add include/hip_test_ipc_common.hh with NamespaceTag() and SocketPath() - Drop the duplicated ipcSocketPath() from mempool_common.hh and hip_vmm_common.hh and route all six call sites through the shared helper - Use the in-tree fs:: alias from hip_test_filesystem.hh instead of std::filesystem directly, matching the rest of the suite and keeping the experimental-filesystem fallback intact Verified: parent, fork() child and execvp() child compute the same tag; three concurrently running containers compute three different tags. Path is 33 of 107 sun_path bytes. Namespace inode numbers are recycled after a namespace is destroyed, so the tag is unique among concurrent peers rather than over all time, which is the property needed here; this is noted in the header. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../catch/include/hip_test_ipc_common.hh | 72 +++++++++++++++++++ .../catch/unit/memory/mempool_common.hh | 15 ++-- .../virtualMemoryManagement/hip_vmm_common.hh | 15 ++-- 3 files changed, 80 insertions(+), 22 deletions(-) create mode 100644 projects/hip-tests/catch/include/hip_test_ipc_common.hh diff --git a/projects/hip-tests/catch/include/hip_test_ipc_common.hh b/projects/hip-tests/catch/include/hip_test_ipc_common.hh new file mode 100644 index 000000000000..08feaff1eca8 --- /dev/null +++ b/projects/hip-tests/catch/include/hip_test_ipc_common.hh @@ -0,0 +1,72 @@ +/* + * Copyright (c) Advanced Micro Devices, Inc., or its affiliates. + * + * SPDX-License-Identifier: MIT + */ + +#pragma once + +#include "hip_test_context.hh" +#include "hip_test_filesystem.hh" + +#include + +#if !HT_WIN +#include +#include +#include + +// Naming for the AF_UNIX rendezvous used by the mempool and VMM IPC tests. +// Shared by both so the two implementations cannot drift apart. +// +// Both ends of the rendezvous derive the name independently instead of +// exchanging it, the same way the shared-memory segment in +// hipMemPoolExportToShareableHandle.cc derives the parent identity with +// getParentProcessId(). That property is what makes this survive +// SpawnProc, which fork()s and then execvp()s: the child re-runs this code +// in a fresh process image and computes the same answer. A randomly +// generated per-run token would not - the child would compute a different +// one and bind a name the parent never addresses. +namespace hip_ipc { + +// Identifies the mount namespace, i.e. "which temp directory is this". +// Peers that share a temp directory compute the same tag; peers that do not +// (separate containers) compute different ones, so identical pids in +// separate pid namespaces cannot collide on a shared temp mount. +// +// The kernel reuses namespace inode numbers once a namespace is destroyed, so +// this is unique among *concurrent* peers rather than over all time. That is +// the property needed here: a socket belonging to an exited peer is already +// gone, and createSocket() unlinks any leftover before binding. +inline std::string NamespaceTag() { + unsigned long long id = 0; + struct stat st; + if (stat("/proc/self/ns/mnt", &st) == 0) { + id = static_cast(st.st_ino); + } else { + // No procfs (for example WSL1, where these tests also run). Fall back to + // the UTS name, which containers default to their own id. + char host[65] = {}; + if (gethostname(host, sizeof(host) - 1) == 0) { + for (const char* p = host; *p != '\0'; ++p) { + id = id * 131 + static_cast(*p); + } + } + } + char buf[9]; + snprintf(buf, sizeof(buf), "%08llx", id & 0xffffffffULL); + return std::string(buf); +} + +// Socket path for the peer owning `pid`. Lives in the temp directory because +// installed test directories may be read-only. The sender names its peer from +// the peer's pid, which it already knows from fork()/spawn(), so no handshake +// is needed. +inline std::string SocketPath(pid_t pid) { + static const std::string tag = NamespaceTag(); + return (fs::temp_directory_path() / ("hip_test_ipc_" + tag + "_" + std::to_string(pid))) + .string(); +} + +} // namespace hip_ipc +#endif // !HT_WIN diff --git a/projects/hip-tests/catch/unit/memory/mempool_common.hh b/projects/hip-tests/catch/unit/memory/mempool_common.hh index 58cfae8b0539..3d990658ac4c 100644 --- a/projects/hip-tests/catch/unit/memory/mempool_common.hh +++ b/projects/hip-tests/catch/unit/memory/mempool_common.hh @@ -7,13 +7,13 @@ #pragma once #include +#include #include #include #include #include #include -#include #include namespace { @@ -629,13 +629,6 @@ struct ipcHdl { char *name; }; -#if !HT_WIN -// IPC socket path in the temp directory so tests work from read-only cwd. -inline std::string ipcSocketPath(pid_t pid) { - return (std::filesystem::temp_directory_path() / std::to_string(pid)).string(); -} -#endif - class ipcSocketCom { ipcHdl *handle; @@ -653,7 +646,7 @@ class ipcSocketCom { int server_fd; struct sockaddr_un servaddr; - std::string nameStr = ipcSocketPath(getpid()); + std::string nameStr = hip_ipc::SocketPath(getpid()); const char *name = nameStr.c_str(); handle = new ipcHdl; @@ -748,7 +741,7 @@ class ipcSocketCom { bzero(&cliaddr, sizeof(cliaddr)); cliaddr.sun_family = AF_UNIX; - std::string nameStr = ipcSocketPath(getpid()); + std::string nameStr = hip_ipc::SocketPath(getpid()); const char *name = nameStr.c_str(); if (strlen(name) > (sizeof(cliaddr.sun_path) - 1)) { @@ -920,7 +913,7 @@ public: bzero(&cliaddr, sizeof(cliaddr)); cliaddr.sun_family = AF_UNIX; - std::string destPath = ipcSocketPath(process); + std::string destPath = hip_ipc::SocketPath(process); if (destPath.size() > (sizeof(cliaddr.sun_path) - 1)) { fprintf(stderr, "Socket failure: Cannot address socket. Name too large\n"); return -1; diff --git a/projects/hip-tests/catch/unit/virtualMemoryManagement/hip_vmm_common.hh b/projects/hip-tests/catch/unit/virtualMemoryManagement/hip_vmm_common.hh index 042829f9b8a9..a94df472cd69 100644 --- a/projects/hip-tests/catch/unit/virtualMemoryManagement/hip_vmm_common.hh +++ b/projects/hip-tests/catch/unit/virtualMemoryManagement/hip_vmm_common.hh @@ -17,8 +17,8 @@ #include #include #endif -#include #include "hip_test_context.hh" +#include "hip_test_ipc_common.hh" #define checkVMMSupported(device) \ { \ @@ -72,13 +72,6 @@ struct ipcHdl { char *name; }; -#ifdef __linux__ -// IPC socket path in the temp directory so tests work from read-only cwd. -inline std::string ipcSocketPath(pid_t pid) { - return (std::filesystem::temp_directory_path() / std::to_string(pid)).string(); -} -#endif - class ipcSocketCom { ipcHdl *handle; // method to create socket from server @@ -86,7 +79,7 @@ class ipcSocketCom { int server_fd; struct sockaddr_un servaddr; - std::string nameStr = ipcSocketPath(getpid()); + std::string nameStr = hip_ipc::SocketPath(getpid()); const char *name = nameStr.c_str(); // Create the socket handle @@ -158,7 +151,7 @@ class ipcSocketCom { bzero(&cliaddr, sizeof(cliaddr)); cliaddr.sun_family = AF_UNIX; - std::string nameStr = ipcSocketPath(getpid()); + std::string nameStr = hip_ipc::SocketPath(getpid()); const char *name = nameStr.c_str(); if (strlen(name) > (sizeof(cliaddr.sun_path) - 1)) { @@ -268,7 +261,7 @@ public: // Construct client address to send this SHareable handle to bzero(&cliaddr, sizeof(cliaddr)); cliaddr.sun_family = AF_UNIX; - std::string destPath = ipcSocketPath(process); + std::string destPath = hip_ipc::SocketPath(process); if (destPath.size() > (sizeof(cliaddr.sun_path) - 1)) { fprintf(stderr, "Socket failure: Cannot address socket. Name too large\n"); return -1; From d489de8c6afe49426f107b4a87fec788c3b2533f Mon Sep 17 00:00:00 2001 From: Ioannis Assiouras Date: Thu, 24 Sep 2026 10:19:54 +0000 Subject: [PATCH 6/9] hip-tests: narrow this PR to the contract test artifacts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Split the IPC socket path work out of this PR. It addresses the same read-only-cwd problem but in a different set of tests, and it carries its own design discussion; it will follow separately so the contract test fix can land on its own. Changes: - Restore mempool_common.hh and hip_vmm_common.hh to their pre-PR state - Remove include/hip_test_ipc_common.hh - Use the in-tree fs:: alias from hip_test_filesystem.hh rather than std::filesystem directly. The suite provides that header to fall back to , so going straight to std::filesystem would not build on toolchains that only have the experimental one. The PR is now the two contract test files only: they write their generated dot graph and code object under the temp directory instead of the current working directory, which may be read-only in an installed test tree. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../test_hip_graph_debug_contract.cc | 4 +- .../test_hip_library_file_contract.cc | 4 +- .../catch/include/hip_test_ipc_common.hh | 72 ------------------- .../catch/unit/memory/mempool_common.hh | 38 ++-------- .../virtualMemoryManagement/hip_vmm_common.hh | 40 +++-------- 5 files changed, 18 insertions(+), 140 deletions(-) delete mode 100644 projects/hip-tests/catch/include/hip_test_ipc_common.hh diff --git a/projects/hip-tests/catch/contract/graph_debug/test_hip_graph_debug_contract.cc b/projects/hip-tests/catch/contract/graph_debug/test_hip_graph_debug_contract.cc index a2d6d7cc807d..aa5b5cbb2379 100644 --- a/projects/hip-tests/catch/contract/graph_debug/test_hip_graph_debug_contract.cc +++ b/projects/hip-tests/catch/contract/graph_debug/test_hip_graph_debug_contract.cc @@ -7,7 +7,6 @@ #include #include #include -#include #include #include @@ -20,6 +19,7 @@ #include #include +#include #include namespace { @@ -29,7 +29,7 @@ constexpr size_t kByteCount = 64; std::string DotPath() { int device = 0; HIP_CHECK(hipGetDevice(&device)); - return (std::filesystem::temp_directory_path() / + return (fs::temp_directory_path() / ("hip_contract_graph_debug_" + std::to_string(device) + "_" + std::to_string(getpid()) + ".dot")) .string(); diff --git a/projects/hip-tests/catch/contract/library_file/test_hip_library_file_contract.cc b/projects/hip-tests/catch/contract/library_file/test_hip_library_file_contract.cc index 3699194783fc..5f33437b2245 100644 --- a/projects/hip-tests/catch/contract/library_file/test_hip_library_file_contract.cc +++ b/projects/hip-tests/catch/contract/library_file/test_hip_library_file_contract.cc @@ -7,9 +7,9 @@ #include #include #include +#include #include -#include #include #include #include @@ -84,7 +84,7 @@ std::string WriteCodeObjectFile(const char* suffix) { HIP_SKIP_TEST("HIPRTC compilation is not supported by this device/runtime path."); } const std::string path = - (std::filesystem::temp_directory_path() / + (fs::temp_directory_path() / ("hip-contract-library-file-" + std::string(suffix) + "-" + std::to_string(getpid()) + ".code")) .string(); diff --git a/projects/hip-tests/catch/include/hip_test_ipc_common.hh b/projects/hip-tests/catch/include/hip_test_ipc_common.hh deleted file mode 100644 index 08feaff1eca8..000000000000 --- a/projects/hip-tests/catch/include/hip_test_ipc_common.hh +++ /dev/null @@ -1,72 +0,0 @@ -/* - * Copyright (c) Advanced Micro Devices, Inc., or its affiliates. - * - * SPDX-License-Identifier: MIT - */ - -#pragma once - -#include "hip_test_context.hh" -#include "hip_test_filesystem.hh" - -#include - -#if !HT_WIN -#include -#include -#include - -// Naming for the AF_UNIX rendezvous used by the mempool and VMM IPC tests. -// Shared by both so the two implementations cannot drift apart. -// -// Both ends of the rendezvous derive the name independently instead of -// exchanging it, the same way the shared-memory segment in -// hipMemPoolExportToShareableHandle.cc derives the parent identity with -// getParentProcessId(). That property is what makes this survive -// SpawnProc, which fork()s and then execvp()s: the child re-runs this code -// in a fresh process image and computes the same answer. A randomly -// generated per-run token would not - the child would compute a different -// one and bind a name the parent never addresses. -namespace hip_ipc { - -// Identifies the mount namespace, i.e. "which temp directory is this". -// Peers that share a temp directory compute the same tag; peers that do not -// (separate containers) compute different ones, so identical pids in -// separate pid namespaces cannot collide on a shared temp mount. -// -// The kernel reuses namespace inode numbers once a namespace is destroyed, so -// this is unique among *concurrent* peers rather than over all time. That is -// the property needed here: a socket belonging to an exited peer is already -// gone, and createSocket() unlinks any leftover before binding. -inline std::string NamespaceTag() { - unsigned long long id = 0; - struct stat st; - if (stat("/proc/self/ns/mnt", &st) == 0) { - id = static_cast(st.st_ino); - } else { - // No procfs (for example WSL1, where these tests also run). Fall back to - // the UTS name, which containers default to their own id. - char host[65] = {}; - if (gethostname(host, sizeof(host) - 1) == 0) { - for (const char* p = host; *p != '\0'; ++p) { - id = id * 131 + static_cast(*p); - } - } - } - char buf[9]; - snprintf(buf, sizeof(buf), "%08llx", id & 0xffffffffULL); - return std::string(buf); -} - -// Socket path for the peer owning `pid`. Lives in the temp directory because -// installed test directories may be read-only. The sender names its peer from -// the peer's pid, which it already knows from fork()/spawn(), so no handshake -// is needed. -inline std::string SocketPath(pid_t pid) { - static const std::string tag = NamespaceTag(); - return (fs::temp_directory_path() / ("hip_test_ipc_" + tag + "_" + std::to_string(pid))) - .string(); -} - -} // namespace hip_ipc -#endif // !HT_WIN diff --git a/projects/hip-tests/catch/unit/memory/mempool_common.hh b/projects/hip-tests/catch/unit/memory/mempool_common.hh index 3d990658ac4c..4d559d26961f 100644 --- a/projects/hip-tests/catch/unit/memory/mempool_common.hh +++ b/projects/hip-tests/catch/unit/memory/mempool_common.hh @@ -7,7 +7,6 @@ #pragma once #include -#include #include #include #include @@ -646,8 +645,8 @@ class ipcSocketCom { int server_fd; struct sockaddr_un servaddr; - std::string nameStr = hip_ipc::SocketPath(getpid()); - const char *name = nameStr.c_str(); + char name[16]; + sprintf(name, "%u", getpid()); handle = new ipcHdl; if (nullptr == handle) { @@ -661,8 +660,6 @@ class ipcSocketCom { if ((server_fd = socket(AF_UNIX, SOCK_DGRAM, 0)) < 0) { perror("Socket failure: Socket creation failed"); - delete handle; - handle = nullptr; return -1; } @@ -672,10 +669,7 @@ class ipcSocketCom { size_t len = strlen(name); if (len > (sizeof(servaddr.sun_path) - 1)) { - fprintf(stderr, "Socket failure: Cannot bind provided name to socket. Name too large\n"); - close(server_fd); - delete handle; - handle = nullptr; + perror("Socket failure: Cannot bind provided name to socket. Name too large"); return -1; } @@ -683,9 +677,6 @@ class ipcSocketCom { if (bind(server_fd, (struct sockaddr *)&servaddr, SUN_LEN(&servaddr)) < 0) { perror("Socket failure: Binding socket failed"); - close(server_fd); - delete handle; - handle = nullptr; return -1; } @@ -733,30 +724,18 @@ class ipcSocketCom { if ((sock = socket(AF_UNIX, SOCK_DGRAM, 0)) < 0) { perror("IPC failure:Socket creation error"); - delete handle; - handle = nullptr; return -1; } bzero(&cliaddr, sizeof(cliaddr)); cliaddr.sun_family = AF_UNIX; + char name[16]; - std::string nameStr = hip_ipc::SocketPath(getpid()); - const char *name = nameStr.c_str(); + sprintf(name, "%u", getpid()); - if (strlen(name) > (sizeof(cliaddr.sun_path) - 1)) { - fprintf(stderr, "Socket failure: Cannot bind provided name to socket. Name too large\n"); - close(sock); - delete handle; - handle = nullptr; - return -1; - } strcpy(cliaddr.sun_path, name); if (bind(sock, (struct sockaddr *)&cliaddr, sizeof(cliaddr)) < 0) { perror("Socket failure: Binding socket failed"); - close(sock); - delete handle; - handle = nullptr; return -1; } @@ -913,12 +892,7 @@ public: bzero(&cliaddr, sizeof(cliaddr)); cliaddr.sun_family = AF_UNIX; - std::string destPath = hip_ipc::SocketPath(process); - if (destPath.size() > (sizeof(cliaddr.sun_path) - 1)) { - fprintf(stderr, "Socket failure: Cannot address socket. Name too large\n"); - return -1; - } - strcpy(cliaddr.sun_path, destPath.c_str()); + strcpy(cliaddr.sun_path, std::to_string(process).c_str()); int sendfd = (int)shareableHdl; diff --git a/projects/hip-tests/catch/unit/virtualMemoryManagement/hip_vmm_common.hh b/projects/hip-tests/catch/unit/virtualMemoryManagement/hip_vmm_common.hh index a94df472cd69..21c1286ade6b 100644 --- a/projects/hip-tests/catch/unit/virtualMemoryManagement/hip_vmm_common.hh +++ b/projects/hip-tests/catch/unit/virtualMemoryManagement/hip_vmm_common.hh @@ -18,7 +18,6 @@ #include #endif #include "hip_test_context.hh" -#include "hip_test_ipc_common.hh" #define checkVMMSupported(device) \ { \ @@ -79,8 +78,9 @@ class ipcSocketCom { int server_fd; struct sockaddr_un servaddr; - std::string nameStr = hip_ipc::SocketPath(getpid()); - const char *name = nameStr.c_str(); + char name[16]; + // Create a unique socket name based on current pid + sprintf(name, "%u", getpid()); // Create the socket handle handle = new ipcHdl; @@ -96,8 +96,6 @@ class ipcSocketCom { // Creating socket if ((server_fd = socket(AF_UNIX, SOCK_DGRAM, 0)) == 0) { perror("Socket failure: Socket creation failed"); - delete handle; - handle = nullptr; return -1; } @@ -107,10 +105,7 @@ class ipcSocketCom { size_t len = strlen(name); if (len > (sizeof(servaddr.sun_path) - 1)) { - fprintf(stderr, "Socket failure: Cannot bind provided name to socket. Name too large\n"); - close(server_fd); - delete handle; - handle = nullptr; + perror("Socket failure: Cannot bind provided name to socket. Name too large"); return -1; } @@ -118,9 +113,6 @@ class ipcSocketCom { if (bind(server_fd, (struct sockaddr *)&servaddr, SUN_LEN(&servaddr)) < 0) { perror("Socket failure: Binding socket failed"); - close(server_fd); - delete handle; - handle = nullptr; return -1; } @@ -143,30 +135,19 @@ class ipcSocketCom { if ((sock = socket(AF_UNIX, SOCK_DGRAM, 0)) < 0) { perror("IPC failure:Socket creation error"); - delete handle; - handle = nullptr; return -1; } bzero(&cliaddr, sizeof(cliaddr)); cliaddr.sun_family = AF_UNIX; + char name[16]; - std::string nameStr = hip_ipc::SocketPath(getpid()); - const char *name = nameStr.c_str(); + // Create a unique socket name based on current process id. + sprintf(name, "%u", getpid()); - if (strlen(name) > (sizeof(cliaddr.sun_path) - 1)) { - fprintf(stderr, "Socket failure: Cannot bind provided name to socket. Name too large\n"); - close(sock); - delete handle; - handle = nullptr; - return -1; - } strcpy(cliaddr.sun_path, name); if (bind(sock, (struct sockaddr *)&cliaddr, sizeof(cliaddr)) < 0) { perror("Socket failure: Binding socket failed"); - close(sock); - delete handle; - handle = nullptr; return -1; } @@ -261,12 +242,7 @@ public: // Construct client address to send this SHareable handle to bzero(&cliaddr, sizeof(cliaddr)); cliaddr.sun_family = AF_UNIX; - std::string destPath = hip_ipc::SocketPath(process); - if (destPath.size() > (sizeof(cliaddr.sun_path) - 1)) { - fprintf(stderr, "Socket failure: Cannot address socket. Name too large\n"); - return -1; - } - strcpy(cliaddr.sun_path, destPath.c_str()); + strcpy(cliaddr.sun_path, std::to_string(process).c_str()); // Send corresponding shareable handle to the client int sendfd = (int)shareableHdl; From 152f56250ccdda64d8687a1251cf2c28a5a340ae Mon Sep 17 00:00:00 2001 From: Ioannis Assiouras Date: Thu, 24 Sep 2026 11:05:25 +0000 Subject: [PATCH 7/9] hip-tests: name contract artifacts with a random suffix, not the pid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A pid-derived name in the temp directory is predictable, so on a shared temp directory it can be pre-seeded with a symlink that the export or the ofstream then follows. Use a random suffix instead, matching how hip_test_process.hh names its own temporary capture files. Changes: - Add a RandomTag() helper to both contract tests: 64 bits from std::random_device, mixed with the clock so the name stays unique even where random_device is a weak source, formatted as 16 hex chars - Drop the std::remove() before hipGraphDebugDotPrint. With a random name there is nothing to clear, and it was the gap between unlink and open that made the symlink race reachable. The remove after each check stays; that is the cleanup. - Remove the _WIN32/getpid shims, no longer needed now that neither file calls getpid This is unpredictability rather than atomic reservation: mkstemp is POSIX only and hip-tests also runs on Windows, and hipGraphDebugDotPrint takes a path rather than a descriptor, so a reserved fd could not be handed to it. The existing convention in hip_test_process.hh has the same property. Verified: compiles clean under -Wall -Wextra; 100000 draws produced no duplicate. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../test_hip_graph_debug_contract.cc | 29 ++++++++++------ .../test_hip_library_file_contract.cc | 34 ++++++++++++------- 2 files changed, 40 insertions(+), 23 deletions(-) diff --git a/projects/hip-tests/catch/contract/graph_debug/test_hip_graph_debug_contract.cc b/projects/hip-tests/catch/contract/graph_debug/test_hip_graph_debug_contract.cc index aa5b5cbb2379..1d7a4b33aebe 100644 --- a/projects/hip-tests/catch/contract/graph_debug/test_hip_graph_debug_contract.cc +++ b/projects/hip-tests/catch/contract/graph_debug/test_hip_graph_debug_contract.cc @@ -4,19 +4,14 @@ * SPDX-License-Identifier: MIT */ +#include #include #include #include #include +#include #include -#ifdef _WIN32 -#include -#define getpid _getpid -#else -#include -#endif - #include #include #include @@ -25,13 +20,27 @@ namespace { constexpr size_t kByteCount = 64; +// Random suffix for temp artifacts, the same way hip_test_process.hh names its +// capture files. Random rather than derived from the pid: a guessable path in a +// shared temp directory can be pre-seeded with a symlink, which the export then +// follows. The clock is mixed in so the name stays unique even where +// std::random_device is a weak source. +std::string RandomTag() { + std::random_device dev; + uint64_t value = (static_cast(dev()) << 32) ^ dev(); + value ^= static_cast( + std::chrono::high_resolution_clock::now().time_since_epoch().count()); + char buf[17]; + snprintf(buf, sizeof(buf), "%016llx", static_cast(value)); + return std::string(buf); +} + // Use the temp directory because installed test directories may be read-only. std::string DotPath() { int device = 0; HIP_CHECK(hipGetDevice(&device)); return (fs::temp_directory_path() / - ("hip_contract_graph_debug_" + std::to_string(device) + "_" + - std::to_string(getpid()) + ".dot")) + ("hip_contract_graph_debug_" + std::to_string(device) + "_" + RandomTag() + ".dot")) .string(); } @@ -71,7 +80,6 @@ HIP_TEST_CASE(Contract_GraphDebug_HipGraphDebugDotPrint_Default_WritesNonEmptyFi HIP_CHECK(hipGraphAddMemsetNode(&node, graph, nullptr, 0, &memset_params)); const std::string path = DotPath(); - std::remove(path.c_str()); // Exporting a non-empty graph to a dot file must succeed (or report the // feature unsupported) and, on success, produce a non-empty file on disk. @@ -97,7 +105,6 @@ HIP_TEST_CASE(Contract_GraphDebug_HipGraphDebugDotPrint_Default_VerboseFlagIsAcc HIP_CHECK(hipGraphAddEmptyNode(&node, graph, nullptr, 0)); const std::string path = DotPath(); - std::remove(path.c_str()); // The verbose flag augments the output but must not change the success // contract: a valid graph still exports to a non-empty file. diff --git a/projects/hip-tests/catch/contract/library_file/test_hip_library_file_contract.cc b/projects/hip-tests/catch/contract/library_file/test_hip_library_file_contract.cc index 5f33437b2245..8220dc79e52c 100644 --- a/projects/hip-tests/catch/contract/library_file/test_hip_library_file_contract.cc +++ b/projects/hip-tests/catch/contract/library_file/test_hip_library_file_contract.cc @@ -9,22 +9,33 @@ #include #include +#include +#include #include #include +#include #include #include -#ifdef _WIN32 -#include -#define getpid _getpid -#else -#include -#endif - // The library-from-file and managed-symbol APIs (hipLibraryLoadFromFile, // hipLibraryGetManaged) are exercised on both backends: on NVIDIA they map to // the CUDA driver cuLibrary* entry points. namespace { +// Random suffix for temp artifacts, the same way hip_test_process.hh names its +// capture files. Random rather than derived from the pid: a guessable path in a +// shared temp directory can be pre-seeded with a symlink, which the subsequent +// open then follows. The clock is mixed in so the name stays unique even where +// std::random_device is a weak source. +std::string RandomTag() { + std::random_device dev; + uint64_t value = (static_cast(dev()) << 32) ^ dev(); + value ^= static_cast( + std::chrono::high_resolution_clock::now().time_since_epoch().count()); + char buf[17]; + snprintf(buf, sizeof(buf), "%016llx", static_cast(value)); + return std::string(buf); +} + constexpr char const kWriteKernelName[] = "write_value"; constexpr char const kLibrarySource[] = @@ -83,11 +94,10 @@ std::string WriteCodeObjectFile(const char* suffix) { if (!CompileLibrarySource(code)) { HIP_SKIP_TEST("HIPRTC compilation is not supported by this device/runtime path."); } - const std::string path = - (fs::temp_directory_path() / - ("hip-contract-library-file-" + std::string(suffix) + "-" + - std::to_string(getpid()) + ".code")) - .string(); + const std::string path = (fs::temp_directory_path() / + ("hip-contract-library-file-" + std::string(suffix) + "-" + + RandomTag() + ".code")) + .string(); std::ofstream out(path, std::ios::binary); REQUIRE(out.is_open()); out.write(code.data(), static_cast(code.size())); From d6a8994be6ba3360b24be0fea67cfcc37e9ae1f1 Mon Sep 17 00:00:00 2001 From: Ioannis Assiouras Date: Thu, 24 Sep 2026 11:30:31 +0000 Subject: [PATCH 8/9] hip-tests: share the temp artifact naming and seed it with the pid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both contract tests had their own copy of the random-suffix helper. Move it to include/hip_test_temp_path.hh so the two cannot drift, and so the one platform shim for getpid is written once rather than per test. Seed with the pid rather than the clock. The mix-in only matters when std::random_device degenerates to a fixed sequence, which the standard permits and at least one toolchain does. In that case live pids within a namespace are distinct by definition, so concurrent processes still get distinct names; two processes can sample the same clock value. Changes: - Add include/hip_test_temp_path.hh with RandomTag() and TempPath() - Route both contract tests through it, dropping their local copies Built ContractGraphDebugTest and ContractLibraryFileTest against a gfx1260 toolchain, clean under -Wall -Wextra, and ran both on a device-free FFM model: 2/2 and 3/3 assertions pass. Confirmed the artifacts really do go through temp_directory_path() by pointing TMPDIR at a read-only directory: ContractGraphDebugTest then fails 2/2 and ContractLibraryFileTest 2/3, the one remaining pass being MissingFile_IsRejected, which is the only case that never writes a file. Not covered: the read-only-cwd case this PR exists for cannot be exercised under the model, because the model itself opens a log file in the working directory and aborts when it cannot. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../test_hip_graph_debug_contract.cc | 23 +------- .../test_hip_library_file_contract.cc | 26 ++-------- .../catch/include/hip_test_temp_path.hh | 52 +++++++++++++++++++ 3 files changed, 57 insertions(+), 44 deletions(-) create mode 100644 projects/hip-tests/catch/include/hip_test_temp_path.hh diff --git a/projects/hip-tests/catch/contract/graph_debug/test_hip_graph_debug_contract.cc b/projects/hip-tests/catch/contract/graph_debug/test_hip_graph_debug_contract.cc index 1d7a4b33aebe..f19529610560 100644 --- a/projects/hip-tests/catch/contract/graph_debug/test_hip_graph_debug_contract.cc +++ b/projects/hip-tests/catch/contract/graph_debug/test_hip_graph_debug_contract.cc @@ -4,44 +4,25 @@ * SPDX-License-Identifier: MIT */ -#include #include #include #include #include -#include #include #include #include -#include +#include #include namespace { constexpr size_t kByteCount = 64; -// Random suffix for temp artifacts, the same way hip_test_process.hh names its -// capture files. Random rather than derived from the pid: a guessable path in a -// shared temp directory can be pre-seeded with a symlink, which the export then -// follows. The clock is mixed in so the name stays unique even where -// std::random_device is a weak source. -std::string RandomTag() { - std::random_device dev; - uint64_t value = (static_cast(dev()) << 32) ^ dev(); - value ^= static_cast( - std::chrono::high_resolution_clock::now().time_since_epoch().count()); - char buf[17]; - snprintf(buf, sizeof(buf), "%016llx", static_cast(value)); - return std::string(buf); -} - // Use the temp directory because installed test directories may be read-only. std::string DotPath() { int device = 0; HIP_CHECK(hipGetDevice(&device)); - return (fs::temp_directory_path() / - ("hip_contract_graph_debug_" + std::to_string(device) + "_" + RandomTag() + ".dot")) - .string(); + return hip_test::TempPath("hip_contract_graph_debug_" + std::to_string(device), ".dot"); } hipMemsetParams MakeByteMemsetParams(void* device_ptr, unsigned int value) { diff --git a/projects/hip-tests/catch/contract/library_file/test_hip_library_file_contract.cc b/projects/hip-tests/catch/contract/library_file/test_hip_library_file_contract.cc index 8220dc79e52c..d16931ad9f39 100644 --- a/projects/hip-tests/catch/contract/library_file/test_hip_library_file_contract.cc +++ b/projects/hip-tests/catch/contract/library_file/test_hip_library_file_contract.cc @@ -7,13 +7,10 @@ #include #include #include -#include +#include -#include -#include #include #include -#include #include #include @@ -21,21 +18,6 @@ // hipLibraryGetManaged) are exercised on both backends: on NVIDIA they map to // the CUDA driver cuLibrary* entry points. namespace { -// Random suffix for temp artifacts, the same way hip_test_process.hh names its -// capture files. Random rather than derived from the pid: a guessable path in a -// shared temp directory can be pre-seeded with a symlink, which the subsequent -// open then follows. The clock is mixed in so the name stays unique even where -// std::random_device is a weak source. -std::string RandomTag() { - std::random_device dev; - uint64_t value = (static_cast(dev()) << 32) ^ dev(); - value ^= static_cast( - std::chrono::high_resolution_clock::now().time_since_epoch().count()); - char buf[17]; - snprintf(buf, sizeof(buf), "%016llx", static_cast(value)); - return std::string(buf); -} - constexpr char const kWriteKernelName[] = "write_value"; constexpr char const kLibrarySource[] = @@ -94,10 +76,8 @@ std::string WriteCodeObjectFile(const char* suffix) { if (!CompileLibrarySource(code)) { HIP_SKIP_TEST("HIPRTC compilation is not supported by this device/runtime path."); } - const std::string path = (fs::temp_directory_path() / - ("hip-contract-library-file-" + std::string(suffix) + "-" + - RandomTag() + ".code")) - .string(); + const std::string path = + hip_test::TempPath("hip-contract-library-file-" + std::string(suffix), ".code"); std::ofstream out(path, std::ios::binary); REQUIRE(out.is_open()); out.write(code.data(), static_cast(code.size())); diff --git a/projects/hip-tests/catch/include/hip_test_temp_path.hh b/projects/hip-tests/catch/include/hip_test_temp_path.hh new file mode 100644 index 000000000000..252dbf107fd2 --- /dev/null +++ b/projects/hip-tests/catch/include/hip_test_temp_path.hh @@ -0,0 +1,52 @@ +/* + * Copyright (c) Advanced Micro Devices, Inc., or its affiliates. + * + * SPDX-License-Identifier: MIT + */ + +#pragma once + +#include "hip_test_filesystem.hh" + +#include +#include +#include +#include + +#ifdef _WIN32 +#include +#define HIP_TEST_GETPID _getpid +#else +#include +#define HIP_TEST_GETPID getpid +#endif + +// Naming for temporary artifacts written by tests. Tests write under the temp +// directory because an installed test tree may be read-only, and that directory +// is shared, so the name has to be both unique and unguessable: a predictable +// path can be pre-seeded with a symlink that the subsequent open follows. +namespace hip_test { + +// 16 hex characters: 64 bits from std::random_device, mixed with the pid. +// +// The pid is what keeps the name unique if std::random_device degenerates. +// The standard allows it to return a fixed sequence when the implementation +// cannot do better, and at least one toolchain does exactly that. Live pids +// within a namespace are distinct by definition, so concurrent processes still +// get distinct names in that case, which a clock reading would not guarantee. +inline std::string RandomTag() { + std::random_device dev; + uint64_t value = (static_cast(dev()) << 32) ^ dev(); + value ^= static_cast(HIP_TEST_GETPID()); + char buf[17]; + snprintf(buf, sizeof(buf), "%016llx", static_cast(value)); + return std::string(buf); +} + +// Path under the temp directory as -. The caller supplies +// the stem so each test keeps its own recognisable prefix. +inline std::string TempPath(const std::string& stem, const std::string& extension) { + return (fs::temp_directory_path() / (stem + "-" + RandomTag() + extension)).string(); +} + +} // namespace hip_test From aa99e144f9acf2f5c37ca6e2350666feecf7a0cc Mon Sep 17 00:00:00 2001 From: Ioannis Assiouras Date: Thu, 24 Sep 2026 11:34:23 +0000 Subject: [PATCH 9/9] hip-tests: trim the temp path comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rebuilt and re-ran both contract tests: same results as before. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../test_hip_graph_debug_contract.cc | 2 +- .../test_hip_library_file_contract.cc | 4 ++-- .../catch/include/hip_test_temp_path.hh | 19 ++++++------------- 3 files changed, 9 insertions(+), 16 deletions(-) diff --git a/projects/hip-tests/catch/contract/graph_debug/test_hip_graph_debug_contract.cc b/projects/hip-tests/catch/contract/graph_debug/test_hip_graph_debug_contract.cc index f19529610560..ac3d44c5f641 100644 --- a/projects/hip-tests/catch/contract/graph_debug/test_hip_graph_debug_contract.cc +++ b/projects/hip-tests/catch/contract/graph_debug/test_hip_graph_debug_contract.cc @@ -18,7 +18,7 @@ namespace { constexpr size_t kByteCount = 64; -// Use the temp directory because installed test directories may be read-only. +// Temp dir: an installed test tree may be read-only. std::string DotPath() { int device = 0; HIP_CHECK(hipGetDevice(&device)); diff --git a/projects/hip-tests/catch/contract/library_file/test_hip_library_file_contract.cc b/projects/hip-tests/catch/contract/library_file/test_hip_library_file_contract.cc index d16931ad9f39..2858f37d72a8 100644 --- a/projects/hip-tests/catch/contract/library_file/test_hip_library_file_contract.cc +++ b/projects/hip-tests/catch/contract/library_file/test_hip_library_file_contract.cc @@ -69,8 +69,8 @@ bool CompileLibrarySource(std::vector& code) { return true; } -// Compile to a temp file: installed test directories may be read-only. -// Returns the path; the caller must remove the file. +// Compile to a temp file: an installed test tree may be read-only. +// Caller removes the returned path. std::string WriteCodeObjectFile(const char* suffix) { std::vector code; if (!CompileLibrarySource(code)) { diff --git a/projects/hip-tests/catch/include/hip_test_temp_path.hh b/projects/hip-tests/catch/include/hip_test_temp_path.hh index 252dbf107fd2..87385c64c98a 100644 --- a/projects/hip-tests/catch/include/hip_test_temp_path.hh +++ b/projects/hip-tests/catch/include/hip_test_temp_path.hh @@ -21,19 +21,13 @@ #define HIP_TEST_GETPID getpid #endif -// Naming for temporary artifacts written by tests. Tests write under the temp -// directory because an installed test tree may be read-only, and that directory -// is shared, so the name has to be both unique and unguessable: a predictable -// path can be pre-seeded with a symlink that the subsequent open follows. +// Naming for temporary test artifacts. The temp directory is shared, so names +// must be unguessable: a predictable path can be pre-seeded with a symlink. namespace hip_test { -// 16 hex characters: 64 bits from std::random_device, mixed with the pid. -// -// The pid is what keeps the name unique if std::random_device degenerates. -// The standard allows it to return a fixed sequence when the implementation -// cannot do better, and at least one toolchain does exactly that. Live pids -// within a namespace are distinct by definition, so concurrent processes still -// get distinct names in that case, which a clock reading would not guarantee. +// 64 bits from std::random_device as 16 hex chars. The pid keeps the name +// unique if random_device degenerates to a fixed sequence, which the standard +// permits; live pids in a namespace are distinct, clock readings are not. inline std::string RandomTag() { std::random_device dev; uint64_t value = (static_cast(dev()) << 32) ^ dev(); @@ -43,8 +37,7 @@ inline std::string RandomTag() { return std::string(buf); } -// Path under the temp directory as -. The caller supplies -// the stem so each test keeps its own recognisable prefix. +// /-. inline std::string TempPath(const std::string& stem, const std::string& extension) { return (fs::temp_directory_path() / (stem + "-" + RandomTag() + extension)).string(); }