Repository navigation
fix(rocprofiler-sdk): avoid UB and redundant work in the PC sampling completion hook (#8895) - #11871
Merged
Conversation
… at the shim Review asked whether the ROCPROFILER_SDK_HSA_PC_SAMPLING guard could move into is_pc_sample_service_configured instead of living in a separate wrapper. It cannot: queue_hooks.cpp is added to the object library before the early return in CMakeLists.txt so it is always built, while service.cpp defines that function after the early return and is not built when PC sampling is compiled out. Moving the guard would leave the write interceptor with an undefined symbol in that configuration. Record the constraint at the shim so the next reader does not retry the change, and bump the copyright year on the new files to 2026. (cherry picked from commit fa38a01)
…completion hook Three review findings on the completion path: The hook took the queue by reference, so the unit test covering the null-session early return had to synthesize one by dereferencing a null pointer. That is undefined behaviour at argument construction even though the hook returns before touching the queue. Take the queue by pointer instead, which lets the test pass nullptr honestly and documents that the callee must not assume a queue exists. kernel_completion_cb took a mutable reference to a packet it never modifies, forcing the hook to copy the kernel packet on every completion. Make the parameter const and drop the copy. The configured-agent check lived inside kernel_completion_cb, after that copy, so a process with any interceptor active paid the cost on every dispatch even when PC sampling was never configured. The registry-based path cost nothing until a callback was registered. Check is_pc_sample_service_configured in the hook before doing any work, restoring that property; kernel_completion_cb has one caller, so the check is not lost. Also drops a stale kernel_completion_cb declaration in pc_sampling_internals.hpp that still described the removed registry signature, and corrects two comments that referred to the old callback mechanism. (cherry picked from commit c93f8dd)
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
PC-sampling tests and hardware-dependent validation were not run.
Pull request overview
Updates PC-sampling completion hooks to remove undefined behavior, redundant packet copies, and unnecessary dispatch-path work.
Changes:
- Makes queue handling nullable and updates the test.
- Adds early configuration gating and const-correctness.
- Removes stale declarations and documents compile-gate placement.
File summaries
| File | Description |
|---|---|
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/tests/queue_hooks_test.cpp |
Tests null queue handling safely. |
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/tests/pc_sampling_internals.hpp |
Removes a stale callback declaration. |
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/queue_hooks.hpp |
Updates the completion-hook signature. |
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/queue_hooks.cpp |
Adds early gating and avoids packet copying. |
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/hsa_adapter.hpp |
Applies const-correctness to packet handling. |
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/pc_sampling/hsa_adapter.cpp |
Updates callback implementation and comments. |
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hsa/queue.cpp |
Passes the queue pointer to the completion hook. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
In case you're interested in the opinion of a detached observer: if I had designed that interface, I'd want to hear a stronger argument before allowing that one test case to dictate the interface. If in production, no caller is expected to pass a null queue pointer, then passing a reference is a good choice, and the test would provide a mock/fake/ad-hoc queue. |
…ity jobs Both workflows failed before compiling any rocprofiler-sdk source. Code coverage died installing ROCR-Runtime with 'CMake 3.25 or higher is required. You are running version 3.22.1', and release compatibility died resolving find_dependency(NUMA) from hsakmt-config.cmake on the 'latest' ROCm container. This branch predates the fixes for both on develop, so take develop's versions of the two workflows and of the rocprofiler-sdk-util action they depend on. The action change is additive -- python-venv-path defaults to empty and is guarded -- so the continuous integration workflow, which does not pass it, is unaffected. The pinned CMake is held at 3.25 rather than develop's 4.x. The external submodules on this branch (json, pybind11, sqlite, yaml-cpp) still declare cmake_minimum_required below 3.5, which CMake 4 rejects outright, so 4.x would trade one configure failure for another. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
github-actions
Bot
added
the
github actions
…ility Develop dropped -DROCPROFILER_DISABLE_ATT_QUICK_SCAN=ON because it builds rocprof-trace-decoder, but this branch does not, so rocprofiler-sdk-tool's find_package(rocprof-trace-decoder REQUIRED) fails to configure against every ROCm release container. Restore the flag. Co-authored-by: Vivek Kale <vivek.kale@amd.com>
vlaindic
approved these changes
Sep 21, 2026
amd-vkale
merged commit Sep 21, 2026
301d089
into
users/vkale/remove-callbacks-pc-sampling
62 of 70 checks passed
amd-vkale
deleted the
users/vlkale/remove-callbacks-pc-sampling-stacked-withcursorfixes
branch
September 21, 2026 17:02
1 task done
amd-vkale
added a commit
that referenced
this pull request
Oct 2, 2026
…back registry (#11969) Stop routing PC sampling kernel completions through the HSA queue controller's per-queue callback registry. Instead, call `pc_sampling::kernel_dispatch_phase_exit_hook` explicitly from `AsyncSignalHandler` in `hsa/queue.cpp`, which forwards to `kernel_completion_cb` after verifying the service is configured on the dispatch's agent. Add `pc_sampling::is_configured_on_agent` to the `no_real_consumers` gate in `WriteInterceptor`, replacing the notifier count that the old `add_callback` registration used to bump. This keeps marker-packet injection alive for PC-sampling-only runs on configured agents. Both hooks live in a new `queue_hooks.cpp`, added before the HSA-runtime-version early return in `pc_sampling/CMakeLists.txt` and guarded internally by `ROCPROFILER_SDK_HSA_PC_SAMPLING`, so they link as no-ops when HSA PC sampling support is unavailable. This is completion-only: marker injection stays inline in `WriteInterceptor`, with no batching or enqueue-hook change. Counters, thread trace, and SPM keep using the registry until their sibling PRs land. Hook names follow the series' `kernel_dispatch_phase_*_hook` convention. Other changes: - Move `kernel_completion_cb` out of the anonymous namespace and declare it in `hsa_adapter.hpp`; make its `kernel_pkt` param const. - Correct its comment: it runs on ROCr's async signal handler thread, not in the write interceptor. - Remove `intercept_cb_id` from `PCSAgentSession` (no longer needed). - Declare `is_hsa_initialized()` in `service.hpp` (was missing). - Remove stale `kernel_completion_cb` extern from test internals header. - Add unit tests for unconfigured gate, null-session no-op, and seeded-session configured path. Part of the per-service callback removal series. * Sibling PRs: #11967 (thread trace), #11968 (SPM), #11970 (counters) * Combined view: #12135 * Related: #8895, #8586, #8730 * Includes the completion-hook fix reviewed in #11871. JIRA ID: AIPROFSDK-1019 Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Carries the outstanding review fixes for #8895: one instance of undefined behaviour, two unnecessary costs on the dispatch path, and two pieces of cleanup. Stacked on
users/vkale/remove-callbacks-pc-samplingso they land without rewriting that branch's history.Technical Details
Undefined behaviour in the null-session test
signal_completion_hooktookconst Queue&, so the unit test covering the "no active session" path had to produce a reference by dereferencing a null pointer. The hook returns before ever touching the queue in that case, so the test passed — but forming the argument is undefined behaviour regardless of whether the callee reads it.The parameter is now
const Queue*. That makes the nullable contract explicit at the signature and lets the test passnullptrhonestly.Redundant packet copy on every dispatch
kernel_completion_cbtook a mutablerocprofiler_packet&that it never modifies, which forced the hook to copy the packet on every completion. It is nowconst rocprofiler_packet&and the copy is gone.Configured-agent check in the wrong place
is_pc_sample_service_configured()was checked insidekernel_completion_cb, after the copy above. Every dispatch in the process therefore paid PC sampling's cost whenever any interceptor was active — a regression against the registry path, which cost nothing until a callback was actually registered.The check moves to the top of the hook, before any work happens. It is dropped from
kernel_completion_cb, which has exactly one caller.Compile gate placement
A reviewer asked why the
#ifgate lives in the shim rather than insideis_pc_sample_service_configured. It cannot move:service.cppis not compiled when PC sampling is disabled, whereasqueue_hooks.cppis added before the CMake early-return and therefore always compiles. That constraint is now recorded in a comment instead of being rediscovered by the next reader.Stale declaration
pc_sampling_internals.hppstill declaredkernel_completion_cb(counter_callback_info, ...), a leftover of the registry callback that no longer exists.Issue Tracking
JIRA ID: AIPROFSDK-1019
Stacked on #8895. Part of the callback-removal series with #8790, #8891 and #8887.
Test Plan
pc_sampling/tests/queue_hooks_test.cpp— the null-session case now passesnullptrdirectly instead of a null reference, exercising the intended path without relying on undefined behaviour.clang-formatunder the pinned>=11.0.0,<12.0.0.Test Result
Static checks pass. The PC sampling tests were not run: the authoring environment has no ROCm install. They need a CI or reviewer run.
Separately, PC sampling's end-to-end behaviour is hardware-dependent and this series has not been run on MI300A. That validation remains outstanding for #8895 as a whole and is not something this PR can supply.
Submission Checklist