Repository navigation
feat(hrr): make capture metadata-only unless HIP_HRR_CAPTURE_CONTENT is set - #13700
andraghetti wants to merge 12 commits into
Conversation
…is set HIP_HRR_CAPTURE_OUTPUT alone used to store every host buffer, kernel argument and code object, and a measured inference capture held the user's prompt and the model weights. Capture now records the call sequence, handles and sizes by default; contents need a second variable, HIP_HRR_CAPTURE_CONTENT=1, independent of any consent decision. Without it the writer stores no blob and no code object, and does not even hash them (the hash of a four-byte token identifier is as good as the identifier). It returns HRR_HASH_WITHHELD in both halves instead, so each event still says that it had contents. By-value kernel arguments of launches and graph kernel nodes are recorded as zeros of their size; pointer arguments are addresses and stay. The stderr notice names the mode. Replay's blob and code-object loaders refuse a withheld hash: they end the replay and say that content mode would have recorded what the call needed, rather than running on with missing data. The existing capture tests set HIP_HRR_CAPTURE_CONTENT=1, so they keep recording contents. New tests: Unit_HRR_MetadataOnlyCapture (archive holds no blob or code object, launches carry the withheld hash and zeroed scalars, replay refuses naming the mode) and Unit_HRR_Blob_WithheldContentIsRefused (loader refusal, no GPU). Jira: AIRUNTIME-2880
github-actions
Bot
added
documentation
✅ 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
Legacy argument capture still writes application contents without the content opt-in.
1 open issue
What changed in this PR
Makes HIP record-and-replay capture metadata-only by default, requiring explicit opt-in to record application contents.
Changes:
- Adds content gating, argument redaction, and mode-specific notices.
- Marks withheld contents and rejects replay when they are needed.
- Updates tests and documentation for both capture modes.
| File | Description |
|---|---|
| projects/hrr/tools/gen_hrr_api_args.py | Generates withheld-hash constant. |
| projects/hrr/tests/unit/hrr_blob_refusal_test.cc | Tests withheld-content refusal. |
| projects/hrr/tests/integration/hrr_roundtrip_test.cc | Tests metadata-only capture; updates content-mode cases. |
| projects/hrr/tests/integration/hrr_regions_test.cc | Enables content capture. |
| projects/hrr/tests/integration/hrr_kernel_args_test.cc | Enables content capture. |
| projects/hrr/tests/integration/hrr_fatbin_header_test.cc | Enables content capture. |
| projects/hrr/tests/integration/hrr_dispatch_swap_test.cc | Enables content capture. |
| projects/hrr/tests/integration/hrr_disk_space_test.cc | Enables content capture. |
| projects/hrr/tests/integration/hrr_capture_capabilities_test.cc | Enables content capture. |
| projects/hrr/tests/integration/hrr_capture_access_test.cc | Enables content capture. |
| projects/hrr/tests/integration/hrr_api_matrix_test.cc | Enables content capture. |
| projects/hrr/tests/include/hrr_test_common.hh | Updates shared capture helpers. |
| projects/hrr/README.md | Documents capture modes. |
| projects/hrr/playback/hip_playback.h | Declares withheld-content refusal helper. |
| projects/hrr/playback/hip_playback.cpp | Rejects withheld contents during replay. |
| projects/hrr/include/hrr/hrr_api_args.h | Defines withheld-hash constant. |
| projects/hrr/DESIGN.md | Updates capture commands. |
| projects/hip/docs/how-to/hip_record_replay.rst | Explains content opt-in. |
| projects/hip-tests/catch/unit/hrr/hrr_capture_smoke_test.cc | Enables content capture. |
| projects/clr/rocclr/utils/flags.hpp | Adds content opt-in flag. |
| projects/clr/hipamd/src/hrr/hip_capture.cpp | Redacts arguments and reports capture mode. |
| projects/clr/hipamd/src/hrr/hip_capture_writer.h | Declares content-mode control. |
| projects/clr/hipamd/src/hrr/hip_capture_writer.cpp | Skips content hashing and storage by default. |
Review effort: Balanced
…IRUNTIME-2880-add-a-metadata-only-capture-mode # Conflicts: # projects/clr/hipamd/src/hrr/hip_capture_writer.h
The generated hipSetupArgument shim copied up to 256 bytes of the pushed kernel argument into the event whatever the capture mode, so a by-value argument pushed through the pre-chevron ABI reached the archive without HIP_HRR_CAPTURE_CONTENT. A Deref can now be marked as content; its copy runs only in content mode, and hipSetupArgument's argument is marked so. The count and the present flag are still recorded. No other generated shim copies application data inline. Unit_HRR_MetadataOnlySetupArgument pushes a known scalar and searches the archive for it: absent without the opt-in, present with it. The set of integration cases changes: Unit_HRR_MetadataOnlySetupArgument and Unit_HRR_SetupArgument_Direct are added.
…ips the code-object load A metadata-only capture records zeros for by-value kernel arguments. The only thing that stopped replay was the withheld code-object hash, and --replace-kernel resolves the kernel without loading the recorded code object, so the launch would have run on the zeros. Capture now writes the withheld hash for every launch and kernel node in metadata-only mode, also where no code object was hashed, and the shared kernel resolver refuses it before the replacement lookup. Comments and docs now say a by-value struct is withheld whole, embedded device pointers included.
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
danielbolsson
left a comment
danielbolsson
left a comment
There was a problem hiding this comment.
Review of cde7d43 — needs work
Review of the whole change (merge base c34c1b1f56e8, head cde7d43743b1). Read against the code only; not reconciled with the existing review discussion. Nothing was built or run, so test-coverage verdicts below come from reading, not mutation runs.
Blocking
-
The description omits work in the later commits, and its test results cover an older head. The results say "The gate ran on head 76d5dc4", the first of four commits.
f9d9e046andcde7d437add behaviour the description never mentions:- Legacy
hipSetupArgumentbytes are withheld: generatorDeref.contentflag,_emit_copy, the guard athip_capture_generated.cpp:4435-4437, andUnit_HRR_MetadataOnlySetupArgument. resolve_kernel_function(hip_playback.cpp:~607-615) now refuses before--replace-kernelruns, and the launch/node code-object hash is forced to withheld even when no code object was hashed (hip_capture.cpp:381,:587).
The test-plan table, test list and "Not in this change" list do not reflect them. Fix: extend the description to cover both commits and re-run the gate on
cde7d437. - Legacy
-
Metadata-only mode still copies application host memory for pitched copies, and can mark the capture incomplete over data it would not have recorded.
projects/clr/hipamd/src/hrr/hip_capture.cpp:~1820-1846(write_host_rect_blob),hip_capture_writer.cpp:1572. For a non-dense pitched copy it allocatesnbytes and runs the row-by-rowmemcpygather, then callswrite_blob, which returnskWithheldimmediately and discards the buffer. If the allocation fails it callsmark_incomplete("pitched host copy not recorded")over data that would not have been recorded either way. Fix: return the withheld hash at the top ofwrite_host_rect_blob(or query the writer's mode) before gathering or allocating. -
New capture-side branches that no test exercises.
hip_capture.cpp:381and:1384(launch hash override),:584-587and:601-606(graph kernel node hash and by-value argument zeroing),:405-408(struct arguments with embedded device pointers recorded as kind 0).Unit_HRR_MetadataOnlyCapturewould still pass with each removed::381:write_code_objectalready returns the withheld hash whenever a code object is found; the override only matters when no program span exists, and no test reaches that.- Graph node hash/argument zeroing: the only graph test,
Unit_HRR_Blob_WithheldArgumentsRefuseReplacedKernel, builds its record by hand; no test captures a graph kernel node without content. - Struct arguments:
Unit_HRR_EmbeddedPtrStruct_Directruns only in content mode.
Fix: add a graph kernel-node capture and an embedded-pointer-struct capture without content, plus a case where the launch's program span is unavailable.
-
Failing CI checks on the current head:
Multi-Arch CI Summary,therock-pr-bot,Linux::release / Test gfx94X-dcgpu / Test rocprofiler-sdk (shard 1/1),Windows::release / Test gfx110X-all / Test Sanity Check. By name they look unrelated to HRR, but I did not read the logs. No check runs the HRR integration tests (the only HRR workflow ishrr-skills.yml). Fix: get them green or flag them as known flakes with evidence.
Non-blocking
-
Documentation is incomplete, and some statements are now false (
projects/hip/docs/how-to/hip_record_replay.rst,projects/hrr/README.md,projects/hrr/DESIGN.md):- RST
:73-82: after the reflow one paragraph mixes three topics. - RST
:11,:19,:95-102still say the archive holds host buffers and code objects, that capture is enabled with one variable, and that recorded buffers are verbatim. All true only in content mode; the last is the privacy warning. - RST
:90-91marksblobs/andcode_objects/"content mode only", but the writer still creates both unconditionally (hip_capture_writer.cpp:1055-1058). - README env-var table (
:95-96) has noHIP_HRR_CAPTURE_CONTENTrow. - DESIGN
:760-766documents onlyHIP_HRR_CAPTURE_OUTPUTas the enable flag;:1051(theLD_PRELOADcommand) now produces an unreplayable archive;:34-35,:57,:667-672,:705still describe the old unconditional behaviour.
- RST
-
Scalar application values are still recorded in metadata-only mode:
hip_capture.cpp:~1588(hipMemPoolSetAttributevalue),:~2500-2533(batch mem-opnodeParams_bytes), thehipStreamBatchMemOparray pointee, memset fill values. OnlyhipSetupArgumentgotcontent=True, while the RST says the archive holds "the call sequence, handles and sizes". Decide the policy: gate these too, or say in the flag help and docs that scalar API arguments are kept. -
Refusal messages are mislabelled and duplicated (
hip_playback.cpp:1574-1576,:3646-3648,:3761-3763,:3805-3807,:4384-4400,:380-382).refuse_withheldis always called with"host buffer", even for fat binaries,hipLinkAddDataand D2H expected blobs, and each caller then prints its own "not found" line (D2H callers also count a validation failure), so the user sees a refusal followed by a contradictory failure.:3217already checksfatal_errorand stays quiet; the others should too. -
The
enable-recordingskill text andinspect_archive.pyare now wrong about metadata-only archives.SKILL.md:18says capture "replays on another machine";:36and:132describeblobs/as verbatim content;evals.json:56expects buffers recorded "in the clear".inspect_archive.py:247-248and:650-656count zero blobs against a manifestblob_countof zero and return "recorded", a "worth sending" verdict on an archive that cannot reproduce anything. Have it detect the mode from a manifest marker or from withheld hashes inevents.bin. -
More test gaps: the withheld hash on module-load events is not asserted (the "no files under
code_objects/" check passes even ifwrite_code_objectreturned{}); the plain launch handler's refusal under--replace-kernelis never driven (only the graph-node path is);--infoand inspect run only on the SetupArgument archive, which holds no withheld hashes; the final CHECK inUnit_HRR_MetadataOnlyCapturematches the shared message tail, so it cannot tell which refusal fired. -
Per-launch code-object work still runs in metadata-only mode (
hip_capture.cpp:674,:1391,:1006-1031):kernel_code_object_hashtakesg_prog_hash_mu, reads the binary span and caches a value that:381/:587then overwrite. Return the withheld hash at its top.
Decisions for the author
- No
HRR_VERSIONbump, and no marker in the file header or manifest. An olderhrr-playbacktreats a missing blob as non-fatal (Unit_HRR_Blob_AbsentBlobDoesNotEndTheReplay), so it would replay a metadata-only archive and silently skip the H2D copies. #12521 bumped to v7 forHRR_FILE_FLAG_PACKED_HOST_RECTSon the argument that the version is what keeps such a reader out. The docs say playback reads archives from the runtime built from the same commit, so cross-version replay is already unsupported. A bump or header flag would give older readers a clean refusal and give--info/inspect_archivesomething to read. Confirm the same-commit contract is the intended answer. - The
enable-recordingwrapper does not setHIP_HRR_CAPTURE_CONTENT(hrr_capture.sh:694exports onlyHIP_HRR_CAPTURE_OUTPUT), so skill captures are now metadata-only anddecode-and-triagehits the refusal on replay. The description says this is intended; the stale skill text above still stands. manifest.jsonand--infodo not say an archive is metadata-only, and async D2H shims still synchronize the stream for a snapshot that is no longer written. Both are called out in the description as known follow-ups; the first sign of the mode is currently a mid-replay refusal.- A D2H expected-output blob counts as a withheld input and ends replay at the first D2H copy. Refusing keeps "never look faithful"; skipping validation silently would let replay continue. In practice the fat-binary and H2D refusals fire first.
- Legacy
hipSetupArgumentreplay passes zeros to the real runtime until the laterhipLaunchByPtris refused. No hash on thehipSetupArgumentevent marks the bytes withheld, so refusal depends on every legacy launch going throughserialize_kernel_launch. A one-line comment saying so would protect a future launch path. - The flag is read at three sites (
hrr_capture_content(), the writer'sg_record_content, direct read in generated code). They cannot diverge today; a single path would be cheaper to maintain.
Positive signals
resolve_kernel_functionrefuses before--replace-kernelcan run a substituted kernel on the recorded zeros (hip_playback.cpp:~607), closing a bypass the loaders alone would have left.- Not hashing withheld content is deliberate and documented in the writer, with the right reason: a hash of a four-byte token identifier is as good as the identifier.
hrr_api_args.h,hip_capture_generated.cppandhip_playback_generated.cppregenerate byte-identically from the changed generator.- Every existing capture-then-replay test and the shared helpers now set
HIP_HRR_CAPTURE_CONTENT=1, so content mode stays regression-tested; none was missed. - The description's negative control (tests against a revert of the production change) is a check that rarely gets done.
Deployment notes
Anyone who sets HIP_HRR_CAPTURE_OUTPUT alone now gets a metadata-only archive, and replay stops at the first fat-binary registration or host-buffer copy; anything that drives capture needs HIP_HRR_CAPTURE_CONTENT=1. No new dependency or migration. There is no CHANGELOG entry, although projects/clr/CHANGELOG.md records HRR changes.
…g zeros Metadata-only capture zeroed the legacy hipSetupArgument value but kept the present flag at 1, so replay could not tell a withheld value from a genuine zero and pushed zeros. The pushed value is only read by hipLaunchByPtr, and the launch that follows is already refused for its withheld code-object hash, so no kernel ran on them; the setup event itself still replayed as faithful. A content Deref now records HRR_PRESENT_WITHHELD (2) in its present flag when the bytes were not copied, and the generated playback handler refuses it with the metadata-only message and fatal_error before any HIP call. A value captured in content mode keeps flag 1 and replays, zeros included. Unit_HRR_Blob_WithheldSetupArgumentIsRefused calls the generated handler with a withheld record. Unit_HRR_SetupArgument_Direct now also pushes a zero, and Unit_HRR_MetadataOnlySetupArgument replays both archives: the metadata-only one fails at the setup event, since nothing is launched, and the content one passes.
In metadata-only mode kernel_code_object_hash() now returns the withheld hash before reading or caching anything, for every launch and kernel node, so the two overrides after it go. write_host_rect_blob() returns it before gathering a pitched rect, so a failed allocation no longer marks the capture incomplete over bytes it would not have recorded. load_blob() takes the name of what it loads, so a refusal says fat binary, linker input image or device-to-host result to validate against instead of host buffer. Callers that print their own "not found" line, and the D2H ones that count a failed validation, stay quiet once fatal_error is set, as the pinned host snapshot already did. Unit_HRR_MetadataOnlyKernelArguments captures a graph kernel node and a by-value struct with embedded device pointers without the opt-in, and checks the withheld hash and zeroed arguments. Unit_HRR_Blob_WithheldFatBinaryIsNamed checks the fat binary label and the missing duplicate line. The set of integration cases changes: Unit_HRR_MetadataOnlyKernelArguments is added.
The record-and-replay how-to, README and DESIGN still described every capture as holding host buffers and code objects. They now say that only HIP_HRR_CAPTURE_CONTENT records them, that blobs/ and code_objects/ stay empty without it, and how replay refuses a withheld launch, registration or argument. The README environment table gains HIP_HRR_CAPTURE_CONTENT.
…ay refusal A metadata-only replay of Unit_HRR_SetupArgument_Direct is refused at the workload's own fat binary, at event 0, before it reaches a hipSetupArgument event, so the refusal message the test looked for never appears. The test now reads both pushes from each archive: HRR_PRESENT_WITHHELD without the opt-in, present with it and the zero recorded as zero. The content replay still has to pass and the metadata-only one to fail. Unit_HRR_Blob_WithheldSetupArgumentIsRefused drives the handler itself.
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
…mat to v10 Capture without HIP_HRR_CAPTURE_CONTENT sets HRR_FILE_FLAG_METADATA_ONLY in the events.bin header. hrr-playback --info prints the mode as its Contents line, and replay names it before the first event. A v9 reader ignores the flag and skips each H2D copy whose blob it cannot find, so the format goes to v10, as it did for the packed host rects at v7.
…ata-only hrr_capture.sh run --content exports HIP_HRR_CAPTURE_CONTENT=1. Without it the archive is metadata-only, and run says that replay refuses it instead of offering a replay. inspect_archive.py reads HRR_FILE_FLAG_METADATA_ONLY from each events.bin header and reports contents withheld per process. SKILL.md and evals.json say what each mode records.
Memset fill values, stream write and wait values, batch mem-op values and attribute values are arguments of the call, at most 8 bytes each, and stay recorded in both modes. PRIVACY.md now describes the two capture modes as they are, with a row for those values, and projects/clr/CHANGELOG.md records the new default and the v10 format.
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
Motivation
Setting
HIP_HRR_CAPTURE_OUTPUTused to record every host buffer, kernel argument and code object. A measured inference capture held the user's prompt and the model weights. No mode recorded the call sequence without the contents. With this change, capture records only metadata by default. Contents need a second variable,HIP_HRR_CAPTURE_CONTENT=1. That opt-in is independent of the consent decision in AIRUNTIME-2857.Technical Details
HIP_HRR_CAPTURE_CONTENT, sits next toHIP_HRR_CAPTURE_OUTPUT. It defaults to off.write_blob()andwrite_code_object()write no file. They do not hash the contents either. A hash of a four-byte token ID gives the token away.HRR_HASH_WITHHELDconstant (all ones) in both hash halves. Each event that had contents still shows that it had them.kernel_code_object_hash()returns it before reading anything. That hash marks the by-value arguments as withheld, and the shared kernel resolver refuses it before--replace-kernellooks for a replacement.hipSetupArgumentvalue is application data too. The generator marks it as content (Deref.content): without the opt-in its bytes stay zero and its present flag is the newHRR_PRESENT_WITHHELD, which the generated playback handler refuses with the same message andfatal_error. A value captured in content mode, zero included, replays as before.load_blob()andload_code_object()refuse a withheld hash. They print the event's sequence number, what it needed (host buffer, fat binary, linker input image, device-to-host result to validate against, code object), and that content mode would have recorded it. Handlers that print their own "not found" line stay quiet after a refusal.fatal_error, so the replay exits 1, as the existing refusal for blobs over the file-size cap does. Running on would replay wrong data while looking faithful.__hipRegisterFatBinary.HRR_HASH_WITHHELDandHRR_PRESENT_WITHHELDare added togen_hrr_api_args.pyand to the checked-inhrr_api_args.h. Regenerating gives byte-identical files. The event layout is unchanged, so there is no format version bump.HIP_HRR_CAPTURE_CONTENT=1. That covers the HRR integration tests, the shared helpers inhrr_test_common.hhand the hip-tests smoke test.Unit_HRR_CaptureStartNoticenow expects the content-mode notice.Unit_HRR_MetadataOnlyCapture(new) captures the GPU workload without the opt-in. It checks the notice, then that the archive holds no blob or code-object file. H2D copies and launches must carry the withheld hash. The saxpy launch must keep its pointers and zero its float. Replay must fail and name the mode.Unit_HRR_MetadataOnlySetupArgument(new) captures ahipSetupArgumentpush of a known scalar and of a zero, with and without the opt-in. Only the content archive holds the scalar; the metadata-only pushes are markedHRR_PRESENT_WITHHELD. The content replay passes and the metadata-only one fails.Unit_HRR_MetadataOnlyKernelArguments(new) captures a graph kernel node and a by-value struct with embedded device pointers without the opt-in. It checks the withheld hash, the kept pointer, the zeroed by-value bytes, and the struct recorded as kind 0.hrr-handler-tests, without a GPU (all new):Unit_HRR_Blob_WithheldContentIsRefusedchecks the loader refusal,WithheldArgumentsRefuseReplacedKernelthe refusal under--replace-kernel,WithheldSetupArgumentIsRefusedthe generatedhipSetupArgumentrefusal, andWithheldFatBinaryIsNamedthe fat binary label with no duplicate line.manifest.jsonand--infodo not yet say that an archive is metadata-only.HIP_HRR_CAPTURE_CONTENT, so its archives are now metadata-only unless the user exports it.Issue Tracking
JIRA ID: AIRUNTIME-2880
Test Plan
HRR_HASH_WITHHELDstay so that the tests compile.hipSetupArgumentmarker and refusal, the by-value zeroing and the fat binary label reverted.hrr-handler-tests. I built it by hand in each gate's build tree on gbt350, with no GPU. I ran itsUnit_HRR_Blob_*cases there.Test Result
MetadataOnlyCapture,CaptureStartNotice), 4 skipMetadataOnlyCapture,MetadataOnlySetupArgument,MetadataOnlyKernelArguments), 4 skiphrr-handler-tests(by hand)WithheldContentIsRefusedfails 6 assertionsWithheldSetupArgumentIsRefusedandWithheldFatBinaryIsNamedfailThe visible counts include one case that passes with a warning on every run. Apart from the new and changed cases, every case has the same status as on develop. The 28 hidden cases that do not pass also fail or skip on develop; the two hidden cases added since,
SetupArgument_DirectandForkedChildStaysIncomplete_Directfrom the develop merge, pass.Untested: the enable-recording skill with the new default, and Windows. This change touches no Windows-specific code.
Submission Checklist