[andraghetti]

Motivation

HRR capture puts its shims into the live HIP dispatch table during hip::init() and takes them out again at exit. On develop (89103b1c577) both steps copy a whole HipDispatchTable over the live one with memcpy while other threads may be calling through it. A caller can read a slot in the middle of the copy, and a bulk copy does not guarantee that each function pointer is written in one store. Uninstall also copies back the entire snapshot taken at install, which discards any slot another component changed after hip::init(). Tools that register through rocprofiler-register normally change the table at its first access, before hip::init(), so they are not affected; a tool attached to a running process through rocprofiler-register's attach path receives the table later. The in-tree CLR profiler also writes the tables after HRR has installed its shims: it copies whole tables over the live ones during hip::init() when GPU_CLR_PROFILE_OUTPUT is set, and later from hipProfilerEnableExt() and hipProfilerDisableExt() on any thread. With capture on, the table it puts back on disable already holds HRR's shims. That code is left for a follow-up, AIRUNTIME-2845.

Technical Details

• HipDispatchTable and HipCompilerDispatchTable are a size_t followed only by function pointers, which HIP_ENFORCE_ABI in hip_api_trace.cpp enforces outside Windows. Install and uninstall now walk the pointer slots instead of copying the struct. A static_assert in hip_capture.cpp states the layout against size_t, because those ABI checks are not compiled on Windows.
• Install writes each slot whose shim differs from the snapshot with one release store. Uninstall uses one compare-and-swap per slot, from the shim back to the saved function, so a slot that no longer holds the shim is left alone. The size field and the slots without a shim are never written.
• amdhip64 is built as C++17 on Linux and C++20 on Windows, so std::atomic_ref is available only on Windows. One #if picks the slot type and both atomics together. Under IS_WINDOWS the slots are a plain view written through std::atomic_ref, as in packet_store_release() in rocclr/device/rocm/rocvirtual.cpp; MSVC does no type-based alias analysis and clang-cl disables it by default. Elsewhere they are a may_alias view written with the __atomic builtins of GCC and Clang, and any other compiler stops at an #error.
• A slot that another component wrapped after install keeps calling the capture shim after uninstall. write_event_raw(), write_blob() and write_code_object() in hip_capture_writer.cpp therefore drop anything that reaches them once the clean-shutdown trailer is written: the reader would replay such an event as part of a complete archive, and such a blob would be missing from the manifest's blob_count. A blob or code object that is still being written when flush() runs is counted when it claims its file under g_blob_mu, before the write, and flush() sets the flag under g_blob_mu as well, so the manifest counts every file claimed before the trailer and a call that only reaches the claim after it is dropped. open() and the fork child already reset the flag, which is now a std::atomic<bool> because emergency_finalize() uses it without either lock. The comment on flush() says it ends the capture and that checkpoint() is the flush to use while capture runs. note_unreplayable() only stages its note, and write_event_raw() lists it in manifest.unreplayable_apis under the same lock that accepts the event, so a dropped event lists nothing. atomic_write_file() counts a failing fclose() as a failed write, because a buffered write can fail only there.
• The merge of develop after #12255 keeps one flag for both changes: g_events_finalized, which flush() sets to cut off events and blob claims. The writer locks are now taken in the order g_reopen_mu, g_file_mu, g_blob_mu, g_unreplayable_mu, and atfork_prepare() follows it. flush() waits for claimed writes under g_blob_mu while it holds g_file_mu, so develop's order, g_blob_mu before g_file_mu, could deadlock it against fork(). A forked child lists only its own unreplayable APIs: open() clears the list for a new archive. Unit_HRR_ForkWhileNotingUnreplayable therefore expects the API in the parent's manifest and not in the child's.
• Two Copilot findings on merged PRs, fixed here in 8a0f890c8e. From #12256: two threads capturing the same new bytes both reserved space for the blob, so with room for one copy above the reserve the second reservation stopped the capture. A caller now claims the key (g_blob_claims) before reserving; another caller with the same bytes waits for the claim to end and gets the hash only once the file is on disk. From #12252: the secure-execution test's uncaptured run unsets HIP_HRR_CAPTURE_OUTPUT, so a test launched under capture does not pass it on.
• The compiler table install is emitted by projects/hrr/tools/gen_hrr_api_args.py. It now emits a call to the new hip_capture_install_compiler_table() instead of the memcpy, and hip_capture_generated.cpp is regenerated; that call is the only change across the three generated files. The compiler table loses its shims, the same way, only when the archive cannot be opened (#12253), never at shutdown.
• GetHipDispatchTable() is marked NO_VECTORIZE so that GCC does not vectorise the table's one-time initialisation into code that runs ahead of the initialisation check on every call. It is a performance workaround, it does not change how slots are read or written, and Clang ignores the attribute. It matters here only as a reminder that a plain slot-by-slot copy loop gets vectorised back into stores that span two slots; atomic stores are never merged or widened.
• The entry points in hip_table_interface.cpp still load slots with ordinary reads (AIRUNTIME-2837). With every write now a single aligned pointer-sized store, a caller gets either the old function or the new one.
• projects/hrr/DESIGN.md describes the new install, uninstall and shutdown, including that the compiler table keeps its shims at shutdown.

Issue Tracking

JIRA ID: AIRUNTIME-2834, AIRUNTIME-2832

Test Plan

• Regenerate with gen_hrr_api_args.py and compare with the committed generated files.
• Build amdhip64 with GCC 11.4, with and without HIP_HRR_CAPTURE_ENABLED, and compile the three HRR objects again with AMD Clang 23 and -Wall -Wextra.
• HRR unit and integration suites on MI350X (gfx950) with capture compiled into CLR.
• Trailer check: hip_capture_writer.cpp compiled on its own against small stubs for logging and the clock, with GCC 13 and Clang 23. One driver opens an archive, writes three events, a blob and a code object, calls flush(), writes one more of each as a wrapped slot would, calls close(), and counts the records after the trailer and the blob and code object files the manifest does not count. A second driver keeps writing blobs and events from another thread while shutdown runs flush() and close(). Both run against the writer on develop and after this change.
• In-flight check: the same standalone writer with fopen() and snprintf() wrapped, so that the driver can hold one thread just before it claims a blob or code object, or inside the write, while the main thread runs flush(). A third case has a second call for the same content return while the first is held and record its event before the trailer. Built with GCC 13.3, Clang 18.1, GCC 11.4 and AMD Clang 20, and the threaded cases also run under ThreadSanitizer with GCC 13.
• Slot check: the install and uninstall block of hip_capture.cpp compiled on its own with GCC 13 and Clang 23, the non-Windows branch as C++17 and the IS_WINDOWS branch as C++20 with libstdc++'s std::atomic_ref, with a driver that installs and uninstalls shims on a small table.

projects/hrr/tests/integration/hrr_dispatch_swap_test.cc reaches the live table through rocprofiler-register, with hrr_dispatch_tool.cc as the tool library. The tool stands in front of fopen() and open(), since the manifest is opened with open(). It covers the wrapped slot across uninstall, the trailer cut-off, a blob write that fails in fwrite() or only in fclose(), an unreplayable call made after the trailer, and fork() after a failed write.

Test Result

Check	Result
Generated files vs. generator output	identical
amdhip64, capture compiled out (GCC 11.4)	0 errors, 0 warnings in src/hrr
amdhip64, HIP_HRR_CAPTURE_ENABLED (GCC 11.4)	0 errors, 0 warnings in src/hrr
HRR objects, AMD Clang 23 with -Wall -Wextra, both configurations	0 warnings in the HRR files
HRR unit tests (host only, they don't load libamdhip64)	pass, 23 test cases, 98 assertions
HRR integration tests, MI350X, capture compiled in	pass, 54 test cases, 584 assertions
Slot check, both branches, GCC 13 and Clang 23	compiles, pass
Unit_HRR_DiskSpace_DuplicateChargedOnce, MI350X	pass; fails with the writer before 8a0f890c8e: capture stops once, the archive is incomplete and the copies are not recorded
Trailer check, writer on develop	1 event after the trailer, 1 blob and 1 code object not in the manifest's count
Trailer check, writer after the change	0 events after the trailer, no uncounted file
Shutdown race, 3 runs, writer on develop	3 to 5 events after the trailer, 3 to 5 blobs not in the manifest's count
Shutdown race, 3 runs, writer after the change	0 events after the trailer, 0 such blobs
In-flight check, writer at 05aed750c1	1 file not in the manifest's count in each of the 6 cases
In-flight check, writer after ff982e7b3e	no uncounted file; a call held before its claim is dropped; the file the second call refers to is present and counted
Shutdown race with 4 threads under ThreadSanitizer (GCC 13), 10 runs	1 or 2 uncounted files in 5 runs at 05aed750c1, none after ff982e7b3e; no ThreadSanitizer report on either
hip_capture_writer.cpp after ff982e7b3e, with the CLR build's compile command and AMD Clang 20, both configurations	0 warnings

Windows was not built locally. The Windows CI build of an earlier head, 37967472f0, compiled hip-clr with MSVC 19.44 and passed (job); for MSVC, 0331f1c607 only reorders the statements it compiled, and clang-cl now takes the same plain view.

Submission Checklist

• Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.

[therock-pr-bot]

✅ All Policy Checks Passed

Check	Status	Details
📝 PR Description	✅ Pass	—
⛔ Forbidden Files	✅ Pass	—
🧪 Unit Test	✅ Pass	—
🚫 Draft PR	🔜 To Be Enabled	—
🚩 Feature Flag	🔜 To Be Enabled	—
📊 Code Coverage	🔜 To Be Enabled	—

🎉 All policy checks passed!

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

[Copilot]

Copilot review overview

🟡 Changes recommended

Critical issues remain with non-atomic readers and function-pointer slot reinterpretation.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2

Open (2)

• Function-pointer fields are incorrectly accessed as void pointer slots · New
• HIP entry points perform non-atomic reads of atomically updated slots · New

What changed in this PR

Updates HRR dispatch-table installation to use per-slot atomic operations, preserving external slot changes during uninstall.

Changes:

• Replaces bulk table copies with slot-wise installation and conditional restoration.
• Updates compiler-table generation and generated output.
• Documents the revised dispatch behavior.

File	Summary
projects/​hrr/​tools/​gen_hrr_api_args.py	Generates the compiler-table installation call.
projects/​hrr/​DESIGN.md	Documents slot-wise dispatch behavior.
projects/​clr/​hipamd/​src/​hrr/​hip_capture.h	Declares the compiler-table installer.
projects/​clr/​hipamd/​src/​hrr/​hip_capture.cpp	Implements atomic slot installation and restoration.
projects/​clr/​hipamd/​src/​hrr/​hip_capture_generated.cpp	Uses the new compiler-table installer.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[danielbolsson]

Thanks for this. Replacing the whole-table memcpy with per-slot release stores and CAS is a clear improvement. The size field is no longer overwritten, and an install or uninstall that runs before hip_capture_build_table() now writes nothing instead of copying a zeroed table over the live one. I checked the layout assumptions against hip_api_trace.hpp/.cpp: all 545 runtime and 9 compiler members after size are function pointers, the live size equals sizeof(Table), and the slots are 8-byte aligned. Regenerating with gen_hrr_api_args.py reproduces the committed hip_capture_generated.cpp byte for byte.

I found nothing blocking. There is one non-blocking teardown point and a few nits, all inline. The reader-side atomic loads and the function-pointer vs void* aliasing that Copilot raised are already covered (AIRUNTIME-2837), so I haven't repeated them.

One more nit I couldn't anchor inline: projects/hrr/DESIGN.md:664 still says "Shutdown uninstalls shims". With this change, slots that another component re-wrapped are deliberately left in place, so a short qualifier there would keep it consistent with the new paragraph at line 281.

[andraghetti]

Also qualified the shutdown line in DESIGN.md: it now says wrapped slots keep their wrapper and that events reaching a shim after the trailer are dropped.

[danielbolsson]

Approving. Swapping the capture shims in slot by slot is correct and fixes the torn-table race. Ordering, layout (static_asserts, 8-byte-aligned slots) and chaining with rocprofiler-sdk all check out. The generator output is byte-identical to the checked-in files (gen_hrr_api_args.py --check-hrr-coverage passes with a clean tree). No blocking issues. Follow-ups, most important first:

1. The CLR profiler still copies the whole live table (outside this diff): profiler/hip_clr_dispatch_wrappers.cpp:6101-6126.
   • Install and Remove each memcpy the full dispatch tables, from hipProfilerEnableExt/DisableExt on any thread after init. That is the same race this PR fixes.
   • The profiler's "next" snapshot holds HRR shim pointers. A disable after HRR's atexit uninstall would put the shims back, which only the new post-trailer check makes harmless.
   • Worth a follow-up (ideally one shared slot-wise helper). The PR description should also say that the in-tree profiler changes the table after init.
2. Blobs and code objects are still written after the trailer. write_blob and write_code_object only check g_events_fd < 0. A late shim can write an orphan blob and bump g_blob_count after the manifest is finalised. Suggest adding || g_trailer_written (both already hold g_file_mu).
3. DESIGN.md wording:
   • Around line 291 and lines 676-678, the text implies the compiler table is uninstalled too. It never is; its shims stay live through static destruction.
   • Line 49 still describes installing a "parallel capture table".
   • Line 287 calls hip_table_interface.cpp "shared with rocprofiler".
4. Platform branches in hip_capture.cpp:
   • DispatchSlot tests GCC/Clang first, but store_slot/replace_slot test IS_WINDOWS first. On clang-cl the may_alias protection is bypassed.
   • The helpers have no #else #error.
5. Nits:
   • g_trailer_written is a plain bool, written without the lock in emergency_finalize. That path is unreachable today, but std::atomic<bool> would be safer.
   • Dropped post-trailer events are silent, and the header still says flush() is "safe to call multiple times" although it is now terminal.
   • #include <cstring> in the generated file is unused now.
   • A one-line comment on why install_shims is last-writer-wins would help.

Not verified: I didn't build or run anything, including Windows and clang-cl builds. A reader-side test that records after the trailer are not replayed would be cheap using the existing hrr_recovery_test.cc fixtures.

[mangupta]

🤖 AI-assisted pre-review (for @mangupta)

[IMPORTANT] write_event_raw gates on g_trailer_written, so any non-terminal trailer write silently stops capture for the rest of the process

The new early return in write_event_raw keys the "stop recording" decision on g_trailer_written — the flag that records that an EOF trailer has been emitted — rather than on a shutdown/closing latch. g_trailer_written is only cleared by writer::open() and in the fork child (per the author's reply on r4120637536), so if the trailer is ever written on a path that is not process teardown, every subsequent event is dropped for the remaining lifetime of the process while g_events_fd is still valid and the archive stays open.

Suggested fix: Latch a dedicated closing flag (e.g. g_closing) set under the same lock that writes the trailer on the shutdown path only, and test that flag here instead of g_trailer_written — this is what the reviewer originally proposed. If writer::flush() is genuinely shutdown-only, clear g_trailer_written at the end of a non-terminal flush or assert the shutdown-only precondition at the trailer write site so a future checkpoint caller cannot silently disable capture. Also record the drop (a counter or one-shot log) so a truncated archive is diagnosable rather than silent.

[andraghetti]

I'm not sure a separate g_closing adds much while flush() only runs at exit, so in 0331f1c607 I've changed its comment instead, to say it ends the capture and to point to checkpoint(). The drop stays silent like the one after close(); would a one-shot log line or an assert at the trailer write be worth it to you?

[andraghetti]

Follow-ups 2 to 5 are in 295fd7ecc7 and 0331f1c607 apart from a log for the dropped events, and the profiler's whole-table copy has its own ticket, AIRUNTIME-2845. It builds with and without capture here (GCC 11.4, and the HRR objects with AMD Clang 23), the HRR suites pass on MI350X, and Windows CI built hip-clr with MSVC 19.44 on the previous head, but I haven't tried clang-cl; the numbers are in the description's Test Result. The reader-side test would fail until the reader stops at the trailer. Should we create a separate small ticket for it?

[danielbolsson]

Checked 295fd7e and 0331f1c: follow-ups 2 to 5 look done.

• Dropping after the trailer: the updated comment is enough. A one-shot log line would be nice but is optional; no assert needed.
• Reader-side test: yes, please open a separate small ticket.

The branch now has merge conflicts with develop.

[andraghetti]

Merged develop in 05aed750c1, which brings <cstring> back because develop's new shims use std::memcpy and std::strlen. The reader-side test is AIRUNTIME-2848.

[Copilot]

Copilot review overview

🟡 Changes recommended

Blob and code-object writes can still race finalization and produce artifacts after the trailer or manifest.

Review effort: Balanced
Findings: 1

Open (1)

• Coordinate blob writes with archive finalization · New

Resolved since last review (2)

• HIP entry points perform non-atomic reads of atomically updated slots
• Function-pointer fields are incorrectly accessed as void pointer slots

[andraghetti]

Decision record for the finding "Clear inherited unreplayable API metadata when resetting child archives" (r5414764727, projects/clr/hipamd/src/hrr/hip_capture_writer.cpp:515): fixed in ff9bb9b. The fork child clears g_unreplayable_apis in atfork_child, and a fresh archive in open() clears it too, so each manifest lists only the APIs its own archive recorded. The generated capture shims now note every truncation; only the warning stays once per process, since a child or a fresh archive starts with an empty list. atfork_prepare takes g_unreplayable_mu so the child does not inherit it locked. Unit_HRR_Fork_ChildListsOnlyItsOwnUnreplayableApis checks that the parent lists hipLaunchHostFunc and hipGraphAddEmptyNode, and the child lists only hipGraphAddEmptyNode. It fails with either the writer change or the generated-file change reverted.

[Copilot]

Copilot review overview

🔵 Needs a closer look

The concurrency-sensitive shutdown and dispatch changes require human validation, and warning deduplication remains incorrect across fresh archives.

Review effort: Balanced
Findings: 1

Open (1)

• Preserve warning deduplication across archives · New

[Copilot]

Copilot review overview

🟡 Changes recommended

The new integration tests fail on supported Linux HIP configurations without rocprofiler-register support.

Review effort: Balanced
Findings: 1

Open (1)

• Gate HIP driver tests when rocprofiler-register is unavailable · New

Resolved since last review (1)

• Preserve warning deduplication across archives

[mangupta]

🤖 AI-assisted pre-review (for @mangupta)

Re-reviewed the updated head (ff9bb9b4cc5b) — no new findings at or above the posting threshold.

[Copilot]

Copilot review overview

🔵 Needs a closer look

The platform-specific atomic dispatch mutation and concurrent shutdown/fork behavior warrant final human review.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Gate HIP driver tests when rocprofiler-register is unavailable

[mangupta]

🤖 AI-assisted pre-review (for @mangupta)

Re-reviewed the updated head (4b9c5cd9fa44) — no new findings at or above the posting threshold.

[mangupta]

🤖 AI-assisted pre-review (for @mangupta)

Re-reviewed the updated head (e19ace10230c) — no new findings at or above the posting threshold.

[mangupta]

🤖 AI-assisted pre-review (for @mangupta)

Re-reviewed the updated head (3c2064e1105c) — no new findings at or above the posting threshold.

[mangupta]

🤖 AI-assisted pre-review (for @mangupta)

Re-reviewed the updated head (587804566853) — no new findings at or above the posting threshold.

[mangupta]

🤖 AI-assisted pre-review (for @mangupta)

Re-reviewed the updated head (8a0f890c8e95) — no new findings at or above the posting threshold.

[systems-assistant]

TheRock Submodule Bump Activity

Newest first

• 2026-10-13 — Included in TheRock via ROCm/TheRock#8940.