Repository navigation
fix(hrr): record the code object HIP loads, not the whole fat binary - #13698
andraghetti wants to merge 20 commits into
Conversation
Capture stored every registered fat binary as a blob whose size came from the bundle header: the furthest offset + size of its entries, or the compressed totalSize. HIP is handed only a pointer, so nothing bounds that value, and an entry declaring offset 0, size 1 TiB made capture read far past the image, much further than HIP itself, which reads the header and the code object it picks for this GPU. Stop storing bundles. The register event is still written, without a blob, and capture records the code object HIP extracted for the current device, with the size COMGR gave it, through the same writer the launch path uses. Fat binaries registered before capture started are swept after the __device__ globals, since resolving a global is what makes HIP load its code object. Replay pre-loads every code object, so kernels and globals resolve as before; older archives with blobs still replay. The header checks in compute_bundle_size go with it. The fat binary integration test now checks that no register event carries a blob, that each loaded bundle's code object is in the archive as a bare ELF, and that a bundle with a terabyte host entry is captured. The roundtrip helper now asks for a blob only when it asks for a D2H check: on a GPU without image support the texture workload copies nothing back, and its fat binary was the only blob it had. Jira: AIRUNTIME-2858
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
There was a problem hiding this comment.
🟡 Changes recommended
Untrusted selected-entry lengths can still cause out-of-bounds reads, and device-specific preloading can break heterogeneous-GPU replay.
2 open issues
What changed in this PR
Replaces whole-fat-binary capture with HIP-selected code objects, avoiding reads of unused bundle entries while preserving registration events.
Changes:
- Records selected code objects and sweeps globals before fat binaries.
- Expands malformed-bundle tests and permits blob-free roundtrips when appropriate.
- Updates capture documentation.
| File | Description |
|---|---|
projects/hrr/tests/integration/hrr_fatbin_header_test.cc |
Checks standalone ELF capture and oversized unused entries. |
projects/hrr/tests/integration/CMakeLists.txt |
Updates test-description comments. |
projects/hrr/tests/include/hrr_test_common.hh |
Makes blob requirements conditional. |
projects/hrr/DESIGN.md |
Documents code-object capture. |
projects/clr/hipamd/src/hrr/hip_capture.cpp |
Replaces bundle recording and reorders initialization sweeps. |
projects/clr/hipamd/src/hip_code_object.hpp |
Updates the fat-binary iterator interface. |
projects/clr/hipamd/src/hip_code_object.cpp |
Passes fat-binary information to callbacks. |
Review effort: Balanced
…ers reach HIP keeps the size the bundle entry declares for the code object it picks, and nothing bounds it, so a GPU entry declaring a terabyte registered fine and capture then hashed past the image. Capture now takes the extent from the ELF's own section and program headers, checked against the declared size, and records nothing for an image that is not an ELF. The fat binary test registers such a bundle in front of an unreadable page.
…stered on Replay pre-loads every code object on the current device before it replays any event, so a code object a fat binary registration loaded on device 1 went to device 0, and on a host with GPUs of more than one kind replay aborted. The register event now names its code object (blob hash set, blob size 0), and pre-load puts it on the device current on the registering thread at that event, from the hipSetDevice events before it. A test registers a fat binary with device 1 current and checks where replay loads its code object.
Pre-load put a code object a fat binary registration named only on the registering device, and replay kept one module per code object, so a kernel from it launched on another device got the module of the first. On GPUs of one kind a library registered with device 1 current and launched on device 0 uses the same code object on both. Replay now keeps a module per code object and device, and pre-loads each code object on every device a registration, a module load or a kernel launch used it on. The fat binary load-device test now launches the kernel on device 0 after registering with device 1 current, and replays it.
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
danielbolsson
left a comment
There was a problem hiding this comment.
Review — needs work
Full review of the whole change (839470366c…5b08ef8d43, four commits), read as a single context rather than a fan-out.
Description names AIRUNTIME-2858 (not retrievable from here); the description was read after the findings were settled.
Blocking
-
Legacy-blob archives lose their name-scan fallback for kernels —
projects/hrr/playback/hip_playback.cpp:653-657,:1600
load_modulenow keysco_modulesas<hash>@<dev>, and the name-only fallback inresolve_kernel_functionskips every key that doesn't end in the current device suffix.playback___hipRegisterFatBinary(still the handler for pre-change archives, which carry a bundle blob) inserts with the barehexkey (:1600), so those modules are never searched. A pre-change archive with a launch that has no recordedco_hash(the case that fallback exists for) now ends in "Kernel not found in any loaded module". The PR description and DESIGN.md both say old archives "replay through the existing handler"; nothing tests it. (module_map-based lookup and the symbol scan at:1623still see them, so this is specific to the kernel scan.)
Confidence 85 · logic · Fix: key the blob module with the same device suffix it was loaded on (or accept unsuffixed keys in the scan), and add a replay test over an archive with a bundle blob. -
The description misstates the change — PR description,
Technical Details
It says "Replay does not change." Replay changes substantially:code_object_devicesand per-device pre-load inhrr_playback.cpp:863-957, device-suffixed module/function keys inhip_playback.cpp, a new two-GPU test (Unit_HRR_FatBinLoadDevice), andhash_program_imagenow truncates every ELF it hashes to its header-bounded size (hip_capture.cpp:955-969), which applies to module-load and launch hashing too, not only fat binaries. Test Plan/Result are also pinned to head07b0f274ee, the first of four commits; the later two commits (per-device pre-load) are not covered by the stated results. Remedy: rewrite the description to the current four-commit scope and re-run the gate on the current head.
Confidence 85 · mechanical · Fix: update the description; the code is not what's wrong here. -
Failing checks on the head —
therock-pr-bot,Multi-Arch CI Summary,Windows::release / Test gfx110X-all / Test Sanity Check
All three report FAILURE. I did not establish whether they are related (the Windows job looks like a sanity/infra failure), and no HRR test job appears in the rollup at all, so the HRR behaviour is only evidenced by the author's local run on the old head. The author should confirm they are unrelated or green them.
Confidence 70 · architectural
Non-blocking
-
Stale comment on the register handler —
projects/hrr/playback/hip_playback.cpp:1528-1533(and:1558)
It says the handler loads the blob and stores it "keyed by the full 32-char hex hash … consistent withload_module()".load_moduleis no longer keyed that way, and for new-format events (blob_size == 0) the handler returns immediately and the pre-load does the work. Say so where the early return is.
Confidence 85 · mechanical -
A failed device switch is reported as a load on that device —
projects/hrr/playback/hrr_playback.cpp:949-956
IfhipSetDevice(dev)fails,movedis false butload_modulestill runs onstart_dev, the log line has already said "on device N", and the result is cached under@start_dev— so the module the later launch ondevneeds is never loaded and nothing says so.
Confidence 70 · logic · Fix: skip/report when the switch fails.
Decisions for the author
- Pre-load failure on a secondary device is fatal — tradeoff
A code object that fails to load on one of its recorded devices aborts the replay (without--continue-on-error), while the comment and DESIGN.md say that on mixed-GPU hosts it "may load on no other". Either that case is expected and should be non-fatal, or it is a real failure and the wording should not suggest otherwise. - Two-GPU test is skipped on single-GPU runners — tradeoff
Unit_HRR_FatBinLoadDevicereturnsHRR_SKIPbelow two GPUs, so the per-device pre-load is exercised only on multi-GPU hardware. Confirm that is where this gets run.
Positive signals
- Using the size HIP/COMGR already holds, bounded by the ELF's own headers, instead of trusting the bundle header is the right fix for the 1 TiB entry, and the guard-page test makes it provable.
- The negative-control run (new tests against old capture code segfaulting) is exactly the evidence a test change like this needs.
Deployment notes
- Capture output format changes:
__hipRegisterFatBinaryevents now carry a code-object hash withblob_size == 0and no bundle blob; older replay builds will not resolve kernels from such archives via that event.
What this covered
Every file in the change read completely, plus the callers and cases around the changed symbols in hip_playback.cpp/hrr_playback.cpp, elf.cpp::getElfSize and hip_platform.cpp::__hipRegisterFatBinary. Nothing was built or run; findings are from reading, and CI status was taken from the check rollup.
Did not run: fan-out (single context — degraded, coverage is lower than a split review), history, prior-changes, code-comments and agent-instruction passes; the ticket AIRUNTIME-2858 was not retrieved. Not reconciled against any discussion here.
…ules to their device An ELF's own section table can declare as much as the bundle entry does, so a section of 1 TiB passed getElfSize() against the entry's 1 TiB and capture hashed past the image. Capture now finds how much of the span can be read, page by page with process_vm_readv, and refuses an ELF whose headers reach past that. Replay keeps a module per code object and device, but resolved a __device__ global in a module of any device, and the handler for archives that still carry a bundle blob keyed its module without a device, so the kernel name scan skipped it. Both now use the current device. The fat binary header test registers a code object whose unloaded section declares 1 TiB, launches a kernel, and replays the archive rewritten as an old one: the code object as a bundle blob and a launch without a code object hash. The two-GPU test writes a __device__ global on device 0 and reads it on both devices, and both of its halves skip on GPUs of two kinds.
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
… the two-GPU cases A code object is pre-loaded only on the devices capture used it on, and a failure there is fatal like any pre-load failure; the comments and DESIGN.md read as if it were expected. Say why the device switch cannot fail. List Unit_HRR_FatBinLoadDevice and its workload in integration.yaml, which must name every case the binary registers.
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
danielbolsson
left a comment
There was a problem hiding this comment.
Re-checked at 4485b41: the old-archive handler now keys its module per device and the rewritten-archive replay covers it, the readable-extent probe bounds the overstated-section case, global lookup is per device, and the description now matches the replay changes.
Nits:
projects/clr/hipamd/src/hrr/hip_capture.cpp:959- a non-EFAULTprocess_vm_readvfailure (e.g. EPERM under a strict seccomp profile) returns 0, so every ELF is dropped with a "does not fit the 0 bytes" warning; fall back to the declared-size bound or log the probe failure distinctly.projects/hrr/tests/integration/hrr_fatbin_header_test.cc:592- the device-0 pre-load from the launch event is not observable: without itresolve_kernel_functionlazily loads byco_hashon device 0 and replay still passes; assert the pre-load log line or soften the comment.
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
An archive whose register events name a code object replays with the pre-load run_pass() already does, on the device replay starts on. The per-device module keys, the map of the devices each code object was used on and the extra launch ids the reader decodes go to their own change, with Unit_HRR_FatBinLoadDevice and Unit_HRR_Format_EveryLaunchIdDecodes. record_fat_binary_code_object() reads the span through program_binary_span(), which takes the device, and record_program_image() hashes, stores and remembers a span for module loads, kernel launches and fat binaries alike. The fat binary tests build every one-target bundle with target_bundle(), whose entry sizes are parameters. Unit_HRR_FatBinHeaderChecks replays the archive as captured; the bundles built to trip compute_bundle_size()'s header checks go with those checks. Comments and DESIGN.md say what the code does, and test_privacy_doc.py is as on develop.
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
danielbolsson
left a comment
There was a problem hiding this comment.
The code at 559d867 is sound for its new, smaller scope, but the PR description still describes the previous scope (per-device <hash>@<device> modules, Unit_HRR_FatBinLoadDevice, Unit_HRR_Format_EveryLaunchIdDecodes) and its test results are pinned to c0cee0c; rewrite it and re-run the gate on this head.
Non-blocking: replay pre-loads each code object on the starting device only, so name the follow-up (AIRUNTIME-3028 covers only context push/pop and graph nodes); and the legacy handler lost its test when Unit_HRR_FatBinHeaderChecks went.
Archives that hold a fat binary as a blob, as capture stored them before this change, replay through the __hipRegisterFatBinary handler, which loads the blob as a module. Unit_HRR_FatBinHeaderChecks rewrites its live kernel's registration that way, with the launch naming no code object so only that module holds the kernel, and replays the archive again.
Unit_HRR_FatBinGlobalCopy no longer looks for replay's missing-symbol warning, and its workload no longer reads the global back to check HIP's own symbol copy, or registers a kernel nothing launches. Unit_HRR_FatBinHeader_Direct no longer checks that hipRTC emits an ELF, and Unit_HRR_CodeObjectScrub no longer repeats Unit_HRR_FatBinHeaderChecks' blob size check. StatCO::ForEachFatBinary passes only the FatBinaryInfo: capture only null-checked the data pointer. inspect_archive.py reads registration records as on develop: a record that names a code object names no blob, so its blobs/ path does not exist and is skipped. AIRUNTIME-2858: tests, capture sweep and inspect_archive trimmed, behaviour unchanged.
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
…hrr-fatbin-code-object-only # Conflicts: # projects/hrr/skills/enable-recording/SKILL.md
danielbolsson
left a comment
There was a problem hiding this comment.
Both points of my last review are addressed: the description now matches the code-object-only scope (names AIRUNTIME-2988 for multi-device, covers the bundle-blob replay) and its gate ran on 47f2db6. The delta since 559d867 (4fd030d2 bundle-blob replay test, 8a5d0b94 trims, develop merge) is test and plumbing only; inspect_archive.py now looks up the hash of a code-object registration, finds no file under blobs/ and skips it. Nothing blocking.
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
…lpers When readable_extent() cannot create its pipe, hash_program_image() warns that it could not probe the ELF rather than calling it malformed; readable_extent() and readable_elf_size() return std::optional for that. readable_extent() takes the page size from amd::Os::pageSize(), and the NOMINMAX guard top.hpp already provides and the branch for platforms CLR does not build go. StatCO::ForEachFatBinary is no longer const, since it hands out mutable FatBinaryInfo pointers. The fat binary test says why it const_casts names for __hipRegisterFunction and __hipRegisterVar. AIRUNTIME-2858
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
# Conflicts: # projects/hrr/skills/enable-recording/SKILL.md
…at binary AIRUNTIME-2858
danielbolsson
left a comment
There was a problem hiding this comment.
Checked the delta since 47f2db6: readable_extent()/readable_elf_size() now return nullopt when the pipe probe cannot run, and hash_program_image() warns and records nothing in that case; pageSize() and the non-const ForEachFatBinary (single caller) are consistent, and the Windows stages built, with only the MSI packaging job failing.
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
# Conflicts: # projects/clr/hipamd/src/hrr/hip_capture.cpp # projects/hrr/tests/integration/hrr_fatbin_header_test.cc
danielbolsson
left a comment
There was a problem hiding this comment.
Head is a merge of develop onto the previously approved head; no HRR change of the PR's own, and the new develop HRR commits (#13895 etc.) sit outside this PR's diff.
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
# Conflicts: # projects/clr/hipamd/src/hip_code_object.hpp
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |


Motivation
Capture stored every registered fat binary as a blob. It took the blob's size from the bundle header: the furthest
offset + sizeof the entries, or the compressedtotalSize. HIP receives only a pointer, so nothing bounds that value. #12260 added header checks, but an entry declaringoffset = 0, size = 1 TiBpassed all of them. Capture then read a terabyte from a pointer whose real extent is unknown. HIP itself reads only the header and the code object it picks for this GPU. mangupta raised this in the review of #12260.Replay already pre-loads the code objects the archive holds and resolves kernels and globals in them. So this change stops storing bundles instead of bounding their size.
Technical Details
compute_bundle_size(), its header checks andwrite_fat_binary_blob()are removed.__hipRegisterFatBinaryevent is still written, with blob size 0. Its blob hash names the code object capture recorded for it, or is zero. The event layout does not change, soHRR_VERSIONdoes not either.FatBinaryInfo::GetProgram()and the samehash_program_image()path that kernel launches and module loads use. A fat binary HIP has not digested yet has no code object; a launch from it records the code object then.hash_program_image()therefore hashes an ELF only as far as its sections and segments reach, and only within memory that can be read (a one-bytewrite()into a pipe per page on Linux,VirtualQueryon Windows). An ELF whose headers reach further is not recorded. If the probe cannot run (the pipe cannot be created), the ELF is not recorded either, with its own warning.__device__globals: resolving a global makes HIP load the code object that holds it, which the sweep then records.StatCO::ForEachFatBinaryBlobbecomesForEachFatBinary, and its callback receives theFatBinaryInfo*instead of the data pointer. The method is not const: it hands out mutableFatBinaryInfopointers.blobs/.Not in this change: replay pre-loads each code object on the device it starts on only. Multi-device replay is AIRUNTIME-2988.
The Windows build was not run.
Tests
Unit_HRR_FatBinHeaderChecksis rewritten. No register event carries a blob, each bundle HIP loaded has its code object in the archive as a bare ELF, and no archive file holds a bundle or bytes from a target HIP never loaded. Bundles whose entries declare 1 TiB, registered before and after HIP initialises, must capture without a fault, and the archive rewritten into the old blob form must still replay.Unit_HRR_FatBinGlobalCopyregisters a fat binary with a__device__global before HIP initialises, copies to the global, and launches nothing. The archive must hold the global's code object and must replay.Issue Tracking
JIRA ID: AIRUNTIME-2858
Test Result
Current head b2d0ce92a7 merges develop b09c3ab0ef (with #12557 and #13895); gate
p13698-b2d0ce9againstgdevb09c3a: GREEN, every non-passing case also failing on that develop, and this PR's cases pass. The rest of this section is from the earlier head.Head b9d30a4, which changes one line of
DESIGN.mdon top of 1dd46d5. MI350X (gfx950) gatep13698c-1dd46d5on 1dd46d5 against develop 4a67610 (gdev4a67610): build, unit, handler, generator-rules and integration ctest, then every Catch2 case one at a time, hidden cases included. GREEN: every case has the same status as on develop.Unit_HRR_FatBinHeaderChecks,Unit_HRR_FatBinGlobalCopy,Unit_HRR_FatBinHeader_DirectandUnit_HRR_CodeObjectScrubpass;Unit_HRR_FatBinGlobalCopy_Directskips when run without its driver. With replay's blob handler disabled (run on 8a5d0b9),Unit_HRR_FatBinHeaderChecksfails on the replay of the rewritten archive.Submission Checklist