From 5de3b002eb3ebafad7a6a9da51c082e365be038a Mon Sep 17 00:00:00 2001 From: Anusha GodavarthySurya Date: Fri, 10 Jul 2026 12:29:47 +0000 Subject: [PATCH 1/8] fix(clr): remove kernelNamesOwner from AccumulateCommand to fix graph UAF MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AccumulateCommand retained GraphExecBase* via kernelNamesOwner_ to keep borrowed kernel-name strings alive for ReportActivity(). This caused ~AccumulateCommand on the ROCr async-events thread to release GraphExecBase, triggering cascading graph destruction (including ChildGraphNode deletion) while updateCommandsState was still walking the command batch, resulting in a heap-use-after-free. Repro: Unit_hipGraphAddChildGraphNode_CmplxGrph_SchedLogic with ASAN. Regression introduced by df88b76dad8 (DFS stream assignment, #6799). - Remove kernelNamesOwner_, kernelNamesRef_, and the borrowing API (setKernelNamesRef, addKernelNames) from AccumulateCommand - Replace with kernelNames_ (vector): stable pointers into Kernel::getDemangledName(), resolved at dispatch time from KernelMap() - kernelNames_ and timestamps_ are parallel vectors (one entry each per dispatch slot), walked by the same index in ReportActivity — no null-skip - ~AccumulateCommand no longer touches GraphExecBase; the UAF race is structurally eliminated - dispatchAqlPacketBatchFlat: remove kernelNames param, extract kernel_object from flat AQL packets and look up name from KernelMap at dispatch time - Debug logging path uses KernelMap directly instead of kernelNames param ROCM-27628 - Unit_hipGraphAddChildGraphNode_CmplxGrph_SchedLogic (UAF fixed) - Unit_hipGraphAddChildGraphNode_SingleChildNode (stream leak fixed) - Unit_hipExecutionCtxStreamDetached_ActiveCapture_Invalidated - Unit_hipExecutionCtxStreamDetached_ParallelCapture_Invalidated - Unit_hipGraphMem_Alloc_Free_NodeGetParams_Functional (×4, no regression) Co-Authored-By: Claude --- .../clr/hipamd/src/hip_graph_internal.cpp | 16 +--- projects/clr/rocclr/device/device.hpp | 1 - .../clr/rocclr/device/rocm/rocvirtual.cpp | 30 +++++-- .../clr/rocclr/device/rocm/rocvirtual.hpp | 1 - projects/clr/rocclr/platform/activity.cpp | 25 ++---- projects/clr/rocclr/platform/command.cpp | 8 -- projects/clr/rocclr/platform/command.hpp | 82 ++++--------------- 7 files changed, 52 insertions(+), 111 deletions(-) diff --git a/projects/clr/hipamd/src/hip_graph_internal.cpp b/projects/clr/hipamd/src/hip_graph_internal.cpp index 13218a4918c0..62004d784c7b 100644 --- a/projects/clr/hipamd/src/hip_graph_internal.cpp +++ b/projects/clr/hipamd/src/hip_graph_internal.cpp @@ -2440,13 +2440,9 @@ amd::Command* GraphExecSegmented::EnqueueSegmentedGraph(hip::Stream* launch_stre // Single AccumulateCommand on launch_stream manages all HW event lifetimes // and serves as the dispatch anchor for all segments across all streams. - // Pass `this` as the kernel-names owner: the command borrows kernel-name - // strings owned by this graph's nodes (via setKernelNamesRef during dispatch) - // and reads them in ReportActivity() at completion, after OnLaunchComplete() - // drops the launch's reference. Tying the GraphExecBase's lifetime to the command - // keeps those strings valid through the report (no copies). We already hold a - // launch reference here, so the retain in the constructor needs no trim lock. - auto* graph_accumulate = new amd::AccumulateCommand(*launch_stream, {}, nullptr, this); + // Kernel names are copied into the command at dispatch time (via addKernelName + // in dispatchAqlPacketBatchFlat) — no string borrowing, no GraphExecBase pin. + auto* graph_accumulate = new amd::AccumulateCommand(*launch_stream, {}, nullptr); // Register HW events with graph_accumulate so profiling can read them. for (auto& hw_event : segment_hw_events) { @@ -2564,13 +2560,11 @@ hipError_t GraphExecSegmented::EnqueueSegment(const Segment& segment, hip::Strea return hipSuccess; } - const std::vector* kernelNamesToDispatch; const std::vector* flatData; const std::vector* flatHdrs; const std::vector* metaData = nullptr; if (packetBatch.disabledNodeCount == 0) { - kernelNamesToDispatch = &packetBatch.dispatchKernelNames; flatData = &packetBatch.flatPacketData; flatHdrs = &packetBatch.validPacketFullHeaders; if (!packetBatch.flatMetadataData.empty()) { @@ -2580,7 +2574,6 @@ hipError_t GraphExecSegmented::EnqueueSegment(const Segment& segment, hip::Strea // Guard against stale filtered buffers: rebuildFlatBuffer (called from // UpdateAQLPacket) invalidates the cache. This is a no-op when valid. packetBatch.rebuildFilteredLists(sync_plan_.patch_list); - kernelNamesToDispatch = &packetBatch.enabledKernelNames; flatData = &packetBatch.filteredFlatPacketData; flatHdrs = &packetBatch.filteredValidPacketFullHeaders; if (!packetBatch.filteredFlatMetadataData.empty()) { @@ -2590,8 +2583,7 @@ hipError_t GraphExecSegmented::EnqueueSegment(const Segment& segment, hip::Strea if (!flatData->empty()) { bool batchStatus = stream->vdev()->dispatchAqlPacketBatchFlat( - *flatData, *flatHdrs, accumulate, attach_signal, kernelNamesToDispatch, true, - false, metaData); + *flatData, *flatHdrs, accumulate, attach_signal, true, false, metaData); if (!batchStatus) { return hipErrorUnknown; } diff --git a/projects/clr/rocclr/device/device.hpp b/projects/clr/rocclr/device/device.hpp index c7959981ce89..5aee5b504f43 100644 --- a/projects/clr/rocclr/device/device.hpp +++ b/projects/clr/rocclr/device/device.hpp @@ -1379,7 +1379,6 @@ class VirtualDevice : public amd::ReferenceCountedObject { const std::vector& validFullHeaders, amd::AccumulateCommand* vcmd = nullptr, bool attach_signal = false, - const std::vector* kernelNames = nullptr, bool pre_patched = false, bool blocking = false, const std::vector* flatMetadataData = nullptr) { diff --git a/projects/clr/rocclr/device/rocm/rocvirtual.cpp b/projects/clr/rocclr/device/rocm/rocvirtual.cpp index ac09e68093f6..ab67d9b94a5a 100644 --- a/projects/clr/rocclr/device/rocm/rocvirtual.cpp +++ b/projects/clr/rocclr/device/rocm/rocvirtual.cpp @@ -1560,7 +1560,6 @@ bool VirtualGPU::dispatchAqlPacket(hsa_barrier_and_packet_t* packet, uint16_t he bool VirtualGPU::dispatchAqlPacketBatchFlat(const std::vector& flatPacketData, const std::vector& validFullHeaders, amd::AccumulateCommand* vcmd, bool attach_signal, - const std::vector* kernelNames, bool pre_patched, bool blocking, const std::vector* flatMetadataData) { if (vcmd == nullptr || flatPacketData.empty() || validFullHeaders.empty()) { @@ -1576,8 +1575,27 @@ bool VirtualGPU::dispatchAqlPacketBatchFlat(const std::vector& flatPack profilingBegin(*vcmd); dispatchBlockingWait(nullptr); - if (kernelNames != nullptr && vcmd->profilingInfo().enabled_) { - vcmd->addKernelNames(*kernelNames); + // Resolve kernel names at dispatch time from KernelMap — one per dispatch slot + // (same cardinality as timestamps_). Stable const char* into Kernel objects + // that live for the device lifetime. No copies, no GraphExec coupling. + { + static constexpr size_t kPacketSize = 64; + static constexpr size_t kKernelObjectOffset = + offsetof(hsa_kernel_dispatch_packet_t, kernel_object); + const auto& kernel_map = dev().KernelMap(); + for (size_t i = 0; i < numPackets; ++i) { + uint16_t header = static_cast(validFullHeaders[i]); + uint16_t pkt_type = extractAqlBits(header, HSA_PACKET_HEADER_TYPE, + HSA_PACKET_HEADER_WIDTH_TYPE); + if (pkt_type == HSA_PACKET_TYPE_KERNEL_DISPATCH) { + uint64_t kernel_object = 0; + memcpy(&kernel_object, + flatPacketData.data() + i * kPacketSize + kKernelObjectOffset, + sizeof(kernel_object)); + auto it = kernel_map.find(kernel_object); + vcmd->addKernelName(it != kernel_map.end() ? it->second.getDemangledName().c_str() : nullptr); + } + } } const uint32_t queueSize = gpu_queue_->size; @@ -1737,10 +1755,10 @@ bool VirtualGPU::dispatchAqlPacketBatchFlat(const std::vector& flatPack } } if ((IsLogEnabled(amd::LOG_DETAIL_DEBUG, amd::LOG_KERN2) || - IsLogEnabled(amd::LOG_DETAIL_DEBUG, amd::LOG_AQL)) && - kernelNames != nullptr && i < kernelNames->size() && isKernelDispatch) { + IsLogEnabled(amd::LOG_DETAIL_DEBUG, amd::LOG_AQL)) && isKernelDispatch) { + auto kit = dev().KernelMap().find(slot->kernel_object); ClPrint(amd::LOG_DETAIL_DEBUG, amd::LOG_KERN2, "Graph ShaderName : %s, device id : %u", - (*kernelNames)[i] != nullptr ? (*kernelNames)[i]->c_str() : "", + kit != dev().KernelMap().end() ? kit->second.getDemangledName().c_str() : "", dev().index()); if (isBaseKernelDispatch) { logAqlDispatchPacket(roc_device_, gpu_queue_, hdr, slot, slotIdx, priority_); diff --git a/projects/clr/rocclr/device/rocm/rocvirtual.hpp b/projects/clr/rocclr/device/rocm/rocvirtual.hpp index cfd8ef110c0b..f5a65c6e83d3 100644 --- a/projects/clr/rocclr/device/rocm/rocvirtual.hpp +++ b/projects/clr/rocclr/device/rocm/rocvirtual.hpp @@ -683,7 +683,6 @@ class VirtualGPU : public device::VirtualDevice { const std::vector& validFullHeaders, amd::AccumulateCommand* vcmd = nullptr, bool attach_signal = false, - const std::vector* kernelNames = nullptr, bool pre_patched = false, bool blocking = false, const std::vector* flatMetadataData = nullptr) override; diff --git a/projects/clr/rocclr/platform/activity.cpp b/projects/clr/rocclr/platform/activity.cpp index 9d8db5759cb4..89147ca57d67 100644 --- a/projects/clr/rocclr/platform/activity.cpp +++ b/projects/clr/rocclr/platform/activity.cpp @@ -138,24 +138,13 @@ void ReportActivity(const amd::Command& command) { auto timestamps = static_cast(command).getTimestamps(); const auto& kernel_names = static_cast(command).getKernelNames(); - // timestamps has one entry per HSA_PACKET_TYPE_KERNEL_DISPATCH packet only. - // kernel_names has one entry per AQL packet slot (nullptr for barriers and SDMA/copy nodes - // that don't generate timestamps). Walk kernel_names; for each non-null entry consume - // the next timestamp. - // Queue id to fall back on when a timestamp did not record its stream. - const uint64_t fallback_queue_id = queue->vdev()->index(); - uint32_t ti = 0; - for (uint32_t ki = 0; ki < kernel_names.size() && ti < timestamps.size(); ki++) { - if (kernel_names[ki] == nullptr) continue; - const auto& it = timestamps[ti++]; - record.begin_ns = it.start; - record.end_ns = it.end; - // Graph kernels can run on different streams under one command; attribute - // each to the queue it actually ran on when known. - record.queue_id = (it.queue_index != amd::AccumulateCommand::kInvalidQueueIndex) - ? static_cast(it.queue_index) - : fallback_queue_id; - record.kernel_name = kernel_names[ki]->c_str(); + // kernel_names and timestamps are parallel — one entry each per dispatch slot. + // Names are stable const char* resolved at dispatch time, no lookup needed. + for (uint32_t i = 0; i < kernel_names.size() && i < timestamps.size(); i++) { + auto it = timestamps[i]; + record.begin_ns = it.first; + record.end_ns = it.second; + record.kernel_name = kernel_names[i]; function(ACTIVITY_DOMAIN_HIP_OPS, operation_id, &record); } } else { diff --git a/projects/clr/rocclr/platform/command.cpp b/projects/clr/rocclr/platform/command.cpp index cc1d1485639d..99e1e578ee78 100644 --- a/projects/clr/rocclr/platform/command.cpp +++ b/projects/clr/rocclr/platform/command.cpp @@ -63,14 +63,6 @@ Event::~Event() { // ================================================================================================ AccumulateCommand::~AccumulateCommand() { - // Drop the kernel-names owner pin taken in the constructor (e.g. the - // GraphExec). Done first so it always runs, regardless of the owns_hw_events_ - // early return. Releasing here -- after ReportActivity() has read the names -- - // is what makes the borrowed kernel-name strings safe without copying them. - if (kernelNamesOwner_ != nullptr) { - kernelNamesOwner_->release(); - kernelNamesOwner_ = nullptr; - } // When an external owner (e.g. the graph signal pool) reclaims the signals, // do not destroy them here. if (!owns_hw_events_) { diff --git a/projects/clr/rocclr/platform/command.hpp b/projects/clr/rocclr/platform/command.hpp index 3c349dd78eef..f4e95fed74c3 100644 --- a/projects/clr/rocclr/platform/command.hpp +++ b/projects/clr/rocclr/platform/command.hpp @@ -1673,32 +1673,12 @@ class Marker : public Command { class AccumulateCommand : public Command { private: - //! Kernel names and timestamps list for activity profiling - std::vector kernelNames_; - const std::vector* kernelNamesRef_ = nullptr; - //! Optional owner of the borrowed kernel-name strings (e.g. the GraphExec - //! whose nodes own them). Retained while this command lives so the strings - //! outlive ReportActivity(), which runs at the end of setStatus(CL_COMPLETE) - //! -- after OnLaunchComplete() may have dropped the launch's reference. This - //! ties the strings' lifetime to the consumer (this command) rather than to - //! the graph launch, with no string copies. Set via the constructor. - ReferenceCountedObject* kernelNamesOwner_ = nullptr; - - public: - //! Sentinel queue index meaning "unknown; fall back to the command's queue". - static constexpr uint32_t kInvalidQueueIndex = 0xFFFFFFFFu; - - //! One kernel dispatch's GPU timing plus the queue (vGPU index) it ran on. - //! Graphs span multiple streams under one command, so the queue is tracked - //! per timestamp rather than taken from the command's queue. - struct KernelTimestamp { - uint64_t start; - uint64_t end; - uint32_t queue_index; - }; - - private: - std::vector tsList_; + //! Stable kernel name pointers per dispatch slot (nullptr = non-dispatch). + //! Resolved from KernelMap once at dispatch time — device-lifetime pointers, + //! no copies, no borrowing, no GraphExec coupling. + std::vector kernelNames_; + //! GPU timestamps per dispatch slot — populated at signal completion time. + std::vector> timestamps_; //! HW events that need to be released when this command is destroyed std::unordered_map> hw_events_; //! When false, the destructor does not destroy hw_events_ (an external owner, @@ -1706,21 +1686,9 @@ class AccumulateCommand : public Command { bool owns_hw_events_ = true; public: - //! Create a new accumulate command. kernelNamesOwner, when given, is the - //! object that owns the borrowed kernel-name strings (e.g. the GraphExec); - //! it is retained for the command's whole lifetime and released in the - //! destructor, so the borrowed strings stay valid through ReportActivity() - //! even after OnLaunchComplete() drops the launch's reference -- with no - //! string copies. AccumulateCommand(HostQueue& queue, const EventWaitList& eventWaitList = nullWaitList, - const Event* waitingEvent = nullptr, - ReferenceCountedObject* kernelNamesOwner = nullptr) - : Command(queue, CL_COMMAND_TASK, eventWaitList, 0, waitingEvent), - kernelNamesOwner_(kernelNamesOwner) { - if (kernelNamesOwner_ != nullptr) { - kernelNamesOwner_->retain(); - } - } + const Event* waitingEvent = nullptr) + : Command(queue, CL_COMMAND_TASK, eventWaitList, 0, waitingEvent) {} //! Destructor - release all retained HW events virtual ~AccumulateCommand(); @@ -1748,35 +1716,19 @@ class AccumulateCommand : public Command { //! them across launches instead. void setOwnsHwEvents(bool owns) { owns_hw_events_ = owns; } - //! Add kernel name to the list if available - void addKernelName(const std::string* kernelName) { kernelNames_.push_back(kernelName); } - - //! Add multiple kernel names in bulk - void addKernelNames(const std::vector& kernelNames) { - kernelNames_.insert(kernelNames_.end(), kernelNames.begin(), kernelNames.end()); - } - - //! Set kernel names by reference (cheap; borrows the caller's vector and - //! strings). Safe only while that storage outlives this command. Used on the - //! hot path when no profiler is active, where the names are never read. - void setKernelNamesRef(const std::vector* kernelNames) { - kernelNamesRef_ = kernelNames; - } + //! Record a stable kernel name pointer for one dispatch slot. + void addKernelName(const char* name) { kernelNames_.push_back(name); } - //! Add kernel timestamp to the list if available. queue_index is the vGPU - //! index of the stream the kernel ran on (kInvalidQueueIndex when unknown). - void addTimestamps(uint64_t startTs, uint64_t endTs, - uint32_t queue_index = kInvalidQueueIndex) { - tsList_.push_back(KernelTimestamp{startTs, endTs, queue_index}); + //! Add GPU timestamps for one dispatch slot (called at signal completion). + void addTimestamps(uint64_t startTs, uint64_t endTs) { + timestamps_.push_back({startTs, endTs}); } - //! Return the kernel names (pointers to stable strings, no copies) - const std::vector& getKernelNames() const { - return kernelNamesRef_ != nullptr ? *kernelNamesRef_ : kernelNames_; - } + //! Return kernel name pointers (one per dispatch slot) + const std::vector& getKernelNames() const { return kernelNames_; } - //! Return the kernel timestamps - const std::vector& getTimestamps() const { return tsList_; } + //! Return GPU timestamps (one per dispatch slot, populated at signal completion) + const std::vector>& getTimestamps() const { return timestamps_; } //! The command implementation virtual void submit(device::VirtualDevice& device) { device.submitAccumulate(*this); } From 507d21621dbefb564e845ec2e589806dfc5da10c Mon Sep 17 00:00:00 2001 From: Anusha GodavarthySurya Date: Fri, 10 Jul 2026 13:02:06 +0000 Subject: [PATCH 2/8] fix(clr): address review comments on kernelNames redesign - Handle ext kernel dispatch (HSA_AMD_PACKET_TYPE_EXT_KERNEL_DISPATCH) in kernelNames_ collection so it stays parallel to timestamps_ - Replace nullptr fallback with "" to avoid null kernel_name in downstream activity consumers - Clarify comments: kernelNames_ only covers dispatch slots (base + ext), not all AQL slots; addKernelName() must not be passed nullptr Co-Authored-By: Claude --- .../clr/rocclr/device/rocm/rocvirtual.cpp | 32 +++++++++++++------ projects/clr/rocclr/platform/command.hpp | 14 +++++--- 2 files changed, 32 insertions(+), 14 deletions(-) diff --git a/projects/clr/rocclr/device/rocm/rocvirtual.cpp b/projects/clr/rocclr/device/rocm/rocvirtual.cpp index ab67d9b94a5a..8049f203dd7b 100644 --- a/projects/clr/rocclr/device/rocm/rocvirtual.cpp +++ b/projects/clr/rocclr/device/rocm/rocvirtual.cpp @@ -1575,25 +1575,39 @@ bool VirtualGPU::dispatchAqlPacketBatchFlat(const std::vector& flatPack profilingBegin(*vcmd); dispatchBlockingWait(nullptr); - // Resolve kernel names at dispatch time from KernelMap — one per dispatch slot - // (same cardinality as timestamps_). Stable const char* into Kernel objects - // that live for the device lifetime. No copies, no GraphExec coupling. + // Resolve kernel names at dispatch time from KernelMap — one entry per kernel + // dispatch slot (base HSA_PACKET_TYPE_KERNEL_DISPATCH and AMD ext variants), + // parallel to timestamps_ populated at signal completion. Stable const char* + // into Kernel objects that live for the device lifetime. No copies, no + // GraphExec coupling. Non-dispatch slots (barriers, memcpy) are skipped so + // kernelNames_ and timestamps_ always have the same cardinality. { static constexpr size_t kPacketSize = 64; - static constexpr size_t kKernelObjectOffset = + static constexpr size_t kBaseKernelObjectOffset = offsetof(hsa_kernel_dispatch_packet_t, kernel_object); + static constexpr size_t kExtKernelObjectOffset = + offsetof(hsa_amd_ext_kernel_dispatch_packet_t, kernel_object); const auto& kernel_map = dev().KernelMap(); for (size_t i = 0; i < numPackets; ++i) { uint16_t header = static_cast(validFullHeaders[i]); - uint16_t pkt_type = extractAqlBits(header, HSA_PACKET_HEADER_TYPE, - HSA_PACKET_HEADER_WIDTH_TYPE); - if (pkt_type == HSA_PACKET_TYPE_KERNEL_DISPATCH) { + uint8_t pkt_type = extractAqlBits(header, HSA_PACKET_HEADER_TYPE, + HSA_PACKET_HEADER_WIDTH_TYPE); + uint8_t amd_format = static_cast((validFullHeaders[i] >> 16) & 0xFF); + const bool is_base_dispatch = (pkt_type == HSA_PACKET_TYPE_KERNEL_DISPATCH); + const bool is_ext_dispatch = (pkt_type == HSA_PACKET_TYPE_VENDOR_SPECIFIC && + amd_format == HSA_AMD_PACKET_TYPE_EXT_KERNEL_DISPATCH); + if (is_base_dispatch || is_ext_dispatch) { + size_t offset = is_base_dispatch ? kBaseKernelObjectOffset : kExtKernelObjectOffset; uint64_t kernel_object = 0; memcpy(&kernel_object, - flatPacketData.data() + i * kPacketSize + kKernelObjectOffset, + flatPacketData.data() + i * kPacketSize + offset, sizeof(kernel_object)); auto it = kernel_map.find(kernel_object); - vcmd->addKernelName(it != kernel_map.end() ? it->second.getDemangledName().c_str() : nullptr); + // Use a stable fallback string rather than nullptr to avoid crashes in + // downstream activity consumers that assume a non-null kernel name. + vcmd->addKernelName(it != kernel_map.end() + ? it->second.getDemangledName().c_str() + : ""); } } } diff --git a/projects/clr/rocclr/platform/command.hpp b/projects/clr/rocclr/platform/command.hpp index f4e95fed74c3..0ca6c1ff0d85 100644 --- a/projects/clr/rocclr/platform/command.hpp +++ b/projects/clr/rocclr/platform/command.hpp @@ -1673,11 +1673,14 @@ class Marker : public Command { class AccumulateCommand : public Command { private: - //! Stable kernel name pointers per dispatch slot (nullptr = non-dispatch). - //! Resolved from KernelMap once at dispatch time — device-lifetime pointers, - //! no copies, no borrowing, no GraphExec coupling. + //! Stable kernel name pointers — one entry per kernel dispatch slot (base and + //! ext variants). Resolved from KernelMap at dispatch time; point into Kernel + //! objects that live for the device lifetime. Non-dispatch slots (barriers, + //! SDMA) are skipped so kernelNames_ and timestamps_ are always parallel. + //! "" is used when the kernel_object is not found in KernelMap. std::vector kernelNames_; - //! GPU timestamps per dispatch slot — populated at signal completion time. + //! GPU timestamps — one entry per kernel dispatch slot, parallel to + //! kernelNames_, populated at signal completion time. std::vector> timestamps_; //! HW events that need to be released when this command is destroyed std::unordered_map> hw_events_; @@ -1716,7 +1719,8 @@ class AccumulateCommand : public Command { //! them across launches instead. void setOwnsHwEvents(bool owns) { owns_hw_events_ = owns; } - //! Record a stable kernel name pointer for one dispatch slot. + //! Record a stable kernel name pointer for one kernel dispatch slot. + //! Must not be nullptr — use "" when the name cannot be resolved. void addKernelName(const char* name) { kernelNames_.push_back(name); } //! Add GPU timestamps for one dispatch slot (called at signal completion). From 0803899b737f44971e4c7ed8d7326cb0170c22f2 Mon Sep 17 00:00:00 2001 From: Anusha GodavarthySurya Date: Fri, 10 Jul 2026 15:45:48 +0000 Subject: [PATCH 3/8] fix(clr): gate kernel name collection on profiling active (perf fix) Kernel name resolution (memcpy + KernelMap lookup per dispatch packet) was running unconditionally on every graph launch. Wrap in vcmd->profilingInfo().enabled_ check to preserve the fast path when no profiler is active, matching the original setKernelNamesRef behavior. Co-Authored-By: Claude --- projects/clr/rocclr/device/rocm/rocvirtual.cpp | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/projects/clr/rocclr/device/rocm/rocvirtual.cpp b/projects/clr/rocclr/device/rocm/rocvirtual.cpp index 8049f203dd7b..238740849a1d 100644 --- a/projects/clr/rocclr/device/rocm/rocvirtual.cpp +++ b/projects/clr/rocclr/device/rocm/rocvirtual.cpp @@ -1575,13 +1575,11 @@ bool VirtualGPU::dispatchAqlPacketBatchFlat(const std::vector& flatPack profilingBegin(*vcmd); dispatchBlockingWait(nullptr); - // Resolve kernel names at dispatch time from KernelMap — one entry per kernel - // dispatch slot (base HSA_PACKET_TYPE_KERNEL_DISPATCH and AMD ext variants), - // parallel to timestamps_ populated at signal completion. Stable const char* - // into Kernel objects that live for the device lifetime. No copies, no - // GraphExec coupling. Non-dispatch slots (barriers, memcpy) are skipped so - // kernelNames_ and timestamps_ always have the same cardinality. - { + // Resolve kernel names only when profiling is active. Gated to preserve the + // fast path for non-profiled launches (no KernelMap lookup, no memcpy). + // One entry per kernel dispatch slot (base and AMD ext variants), parallel to + // timestamps_ populated at signal completion. Non-dispatch slots are skipped. + if (vcmd->profilingInfo().enabled_) { static constexpr size_t kPacketSize = 64; static constexpr size_t kBaseKernelObjectOffset = offsetof(hsa_kernel_dispatch_packet_t, kernel_object); From cbd7d962494bc737b7d8ec3c80c16f19a05cd1a0 Mon Sep 17 00:00:00 2001 From: Anu Godavar Date: Wed, 15 Jul 2026 05:13:25 +0000 Subject: [PATCH 4/8] =?UTF-8?q?fix(clr):=20address=20review=20comments=20?= =?UTF-8?q?=E2=80=94=20simplify=20kernel-object=20extraction?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace kBaseKernelObjectOffset/kExtKernelObjectOffset with a single kKernelObjectOffset: both hsa_kernel_dispatch_packet_t and hsa_amd_ext_kernel_dispatch_packet_t place kernel_object at offset 32. Added static_assert to enforce this invariant going forward. - Extract kernelObjectFromPacket lambda to avoid duplicating the memcpy. - Fix profiling gate from vcmd->profilingInfo().enabled_ (always 0 at dispatch time) to IsEnabled(OP_ID_DISPATCH): EnableProfiling() runs inside enqueue(), after all segments are dispatched, so the old gate never fired. Also gate on IsLogEnabled(LOG_DETAIL_DEBUG, LOG_KERN2) to match the log level used in the main dispatch loop. Co-Authored-By: Claude --- .../clr/rocclr/device/rocm/rocvirtual.cpp | 48 ++++++++++++------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/projects/clr/rocclr/device/rocm/rocvirtual.cpp b/projects/clr/rocclr/device/rocm/rocvirtual.cpp index 238740849a1d..ab54dcc800e8 100644 --- a/projects/clr/rocclr/device/rocm/rocvirtual.cpp +++ b/projects/clr/rocclr/device/rocm/rocvirtual.cpp @@ -1570,22 +1570,40 @@ bool VirtualGPU::dispatchAqlPacketBatchFlat(const std::vector& flatPack if (flatPacketData.size() != numPackets * 64) { return false; } - std::scoped_lock lock(execution()); profilingBegin(*vcmd); dispatchBlockingWait(nullptr); - // Resolve kernel names only when profiling is active. Gated to preserve the - // fast path for non-profiled launches (no KernelMap lookup, no memcpy). + // Resolve kernel names only when a ReportActivity consumer is registered + // (roctracer-style callback) or when KERN2 debug logging is on. + // IsEnabled(OP_ID_DISPATCH) is the correct gate: it reflects whether + // report_activity is live right now, which is the same condition under which + // ReportActivity() will later call getKernelNames(). Unlike + // profilingInfo().enabled_, it is valid at dispatch time — EnableProfiling() + // runs inside enqueue(), which happens after all segments are dispatched in + // EnqueueSegmentedGraph, so profilingInfo().enabled_ is always 0 here. // One entry per kernel dispatch slot (base and AMD ext variants), parallel to // timestamps_ populated at signal completion. Non-dispatch slots are skipped. - if (vcmd->profilingInfo().enabled_) { + if (amd::activity_prof::IsEnabled(OP_ID_DISPATCH) || + IsLogEnabled(amd::LOG_DETAIL_DEBUG, amd::LOG_KERN2)) { static constexpr size_t kPacketSize = 64; - static constexpr size_t kBaseKernelObjectOffset = + // Both hsa_kernel_dispatch_packet_t and hsa_amd_ext_kernel_dispatch_packet_t + // place kernel_object at the same byte offset — use a single constant. + static constexpr size_t kKernelObjectOffset = offsetof(hsa_kernel_dispatch_packet_t, kernel_object); - static constexpr size_t kExtKernelObjectOffset = - offsetof(hsa_amd_ext_kernel_dispatch_packet_t, kernel_object); + static_assert(kKernelObjectOffset == + offsetof(hsa_amd_ext_kernel_dispatch_packet_t, kernel_object), + "kernel_object offset mismatch between base and ext dispatch packets"); const auto& kernel_map = dev().KernelMap(); + + auto kernelObjectFromPacket = [&](size_t i) { + uint64_t kernel_object = 0; + memcpy(&kernel_object, + flatPacketData.data() + i * kPacketSize + kKernelObjectOffset, + sizeof(kernel_object)); + return kernel_object; + }; + for (size_t i = 0; i < numPackets; ++i) { uint16_t header = static_cast(validFullHeaders[i]); uint8_t pkt_type = extractAqlBits(header, HSA_PACKET_HEADER_TYPE, @@ -1595,17 +1613,15 @@ bool VirtualGPU::dispatchAqlPacketBatchFlat(const std::vector& flatPack const bool is_ext_dispatch = (pkt_type == HSA_PACKET_TYPE_VENDOR_SPECIFIC && amd_format == HSA_AMD_PACKET_TYPE_EXT_KERNEL_DISPATCH); if (is_base_dispatch || is_ext_dispatch) { - size_t offset = is_base_dispatch ? kBaseKernelObjectOffset : kExtKernelObjectOffset; - uint64_t kernel_object = 0; - memcpy(&kernel_object, - flatPacketData.data() + i * kPacketSize + offset, - sizeof(kernel_object)); + uint64_t kernel_object = kernelObjectFromPacket(i); auto it = kernel_map.find(kernel_object); - // Use a stable fallback string rather than nullptr to avoid crashes in - // downstream activity consumers that assume a non-null kernel name. - vcmd->addKernelName(it != kernel_map.end() + const char* kname = it != kernel_map.end() ? it->second.getDemangledName().c_str() - : ""); + : ""; + vcmd->addKernelName(kname); + ClPrint(amd::LOG_DETAIL_DEBUG, amd::LOG_KERN2, + "Graph profiling: pkt[%zu] kernel_object=0x%llx name=%s", + i, static_cast(kernel_object), kname); } } } From a57500ac469edd1566e70afd54e171e88347809c Mon Sep 17 00:00:00 2001 From: Anu Godavar Date: Wed, 15 Jul 2026 06:23:24 +0000 Subject: [PATCH 5/8] fix(clr): move kernel-name collection into per-packet dispatch loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove the separate upfront name-resolution pass (separate loop over flatPacketData with a memcpy per packet). Instead collect kernel names inside the existing per-packet fixup loop where slot->kernel_object is already accessible directly from queue memory, isKernelDispatch is already computed, and the KernelMap lookup is already done for debug logging. - Add needKernelNames = IsEnabled(OP_ID_DISPATCH) to the outer fixup- loop gate so the loop runs when profiling is active even when timestamp_ is null and debug logging is off. - Hoist the KernelMap::find() so the single result feeds both addKernelName() and the ClPrint — one lookup instead of two. - Move static_assert for equal kernel_object offsets to function entry so it is always checked regardless of which code paths are active. Co-Authored-By: Claude --- .../clr/rocclr/device/rocm/rocvirtual.cpp | 77 +++++-------------- 1 file changed, 19 insertions(+), 58 deletions(-) diff --git a/projects/clr/rocclr/device/rocm/rocvirtual.cpp b/projects/clr/rocclr/device/rocm/rocvirtual.cpp index ab54dcc800e8..6f23d3918d63 100644 --- a/projects/clr/rocclr/device/rocm/rocvirtual.cpp +++ b/projects/clr/rocclr/device/rocm/rocvirtual.cpp @@ -1562,6 +1562,10 @@ bool VirtualGPU::dispatchAqlPacketBatchFlat(const std::vector& flatPack amd::AccumulateCommand* vcmd, bool attach_signal, bool pre_patched, bool blocking, const std::vector* flatMetadataData) { + // Both base and ext kernel dispatch packets place kernel_object at the same offset. + static_assert(offsetof(hsa_kernel_dispatch_packet_t, kernel_object) == + offsetof(hsa_amd_ext_kernel_dispatch_packet_t, kernel_object), + "kernel_object offset mismatch between base and ext dispatch packets"); if (vcmd == nullptr || flatPacketData.empty() || validFullHeaders.empty()) { return false; } @@ -1574,58 +1578,6 @@ bool VirtualGPU::dispatchAqlPacketBatchFlat(const std::vector& flatPack profilingBegin(*vcmd); dispatchBlockingWait(nullptr); - // Resolve kernel names only when a ReportActivity consumer is registered - // (roctracer-style callback) or when KERN2 debug logging is on. - // IsEnabled(OP_ID_DISPATCH) is the correct gate: it reflects whether - // report_activity is live right now, which is the same condition under which - // ReportActivity() will later call getKernelNames(). Unlike - // profilingInfo().enabled_, it is valid at dispatch time — EnableProfiling() - // runs inside enqueue(), which happens after all segments are dispatched in - // EnqueueSegmentedGraph, so profilingInfo().enabled_ is always 0 here. - // One entry per kernel dispatch slot (base and AMD ext variants), parallel to - // timestamps_ populated at signal completion. Non-dispatch slots are skipped. - if (amd::activity_prof::IsEnabled(OP_ID_DISPATCH) || - IsLogEnabled(amd::LOG_DETAIL_DEBUG, amd::LOG_KERN2)) { - static constexpr size_t kPacketSize = 64; - // Both hsa_kernel_dispatch_packet_t and hsa_amd_ext_kernel_dispatch_packet_t - // place kernel_object at the same byte offset — use a single constant. - static constexpr size_t kKernelObjectOffset = - offsetof(hsa_kernel_dispatch_packet_t, kernel_object); - static_assert(kKernelObjectOffset == - offsetof(hsa_amd_ext_kernel_dispatch_packet_t, kernel_object), - "kernel_object offset mismatch between base and ext dispatch packets"); - const auto& kernel_map = dev().KernelMap(); - - auto kernelObjectFromPacket = [&](size_t i) { - uint64_t kernel_object = 0; - memcpy(&kernel_object, - flatPacketData.data() + i * kPacketSize + kKernelObjectOffset, - sizeof(kernel_object)); - return kernel_object; - }; - - for (size_t i = 0; i < numPackets; ++i) { - uint16_t header = static_cast(validFullHeaders[i]); - uint8_t pkt_type = extractAqlBits(header, HSA_PACKET_HEADER_TYPE, - HSA_PACKET_HEADER_WIDTH_TYPE); - uint8_t amd_format = static_cast((validFullHeaders[i] >> 16) & 0xFF); - const bool is_base_dispatch = (pkt_type == HSA_PACKET_TYPE_KERNEL_DISPATCH); - const bool is_ext_dispatch = (pkt_type == HSA_PACKET_TYPE_VENDOR_SPECIFIC && - amd_format == HSA_AMD_PACKET_TYPE_EXT_KERNEL_DISPATCH); - if (is_base_dispatch || is_ext_dispatch) { - uint64_t kernel_object = kernelObjectFromPacket(i); - auto it = kernel_map.find(kernel_object); - const char* kname = it != kernel_map.end() - ? it->second.getDemangledName().c_str() - : ""; - vcmd->addKernelName(kname); - ClPrint(amd::LOG_DETAIL_DEBUG, amd::LOG_KERN2, - "Graph profiling: pkt[%zu] kernel_object=0x%llx name=%s", - i, static_cast(kernel_object), kname); - } - } - } - const uint32_t queueSize = gpu_queue_->size; const uint32_t queueMask = queueSize - 1; const uint32_t sw_queue_size = queueMask; @@ -1742,8 +1694,11 @@ bool VirtualGPU::dispatchAqlPacketBatchFlat(const std::vector& flatPack lastSlotPtr->completion_signal = Barriers().ActiveSignal(); } - // Per-packet fixups: profiling signals, kernel-name printing, and inline barrier logging. - if (timestamp_ != nullptr || IsLogEnabled(amd::LOG_DETAIL_DEBUG, amd::LOG_KERN2) || + // Per-packet fixups: profiling signals, kernel-name collection, kernel-name + // printing, and inline barrier logging. + const bool needKernelNames = amd::activity_prof::IsEnabled(OP_ID_DISPATCH); + if (timestamp_ != nullptr || needKernelNames || + IsLogEnabled(amd::LOG_DETAIL_DEBUG, amd::LOG_KERN2) || IsLogEnabled(amd::LOG_DETAIL_DEBUG, amd::LOG_AQL)) { for (size_t i = chunkStart; i < chunkEnd; ++i) { const uint64_t slotIdx = (startIndex + i) & queueMask; @@ -1782,12 +1737,18 @@ bool VirtualGPU::dispatchAqlPacketBatchFlat(const std::vector& flatPack slot->reserved2 = timestamp_->command().profilingInfo().correlation_id_; } } - if ((IsLogEnabled(amd::LOG_DETAIL_DEBUG, amd::LOG_KERN2) || - IsLogEnabled(amd::LOG_DETAIL_DEBUG, amd::LOG_AQL)) && isKernelDispatch) { + if (isKernelDispatch && (needKernelNames || + IsLogEnabled(amd::LOG_DETAIL_DEBUG, amd::LOG_KERN2) || + IsLogEnabled(amd::LOG_DETAIL_DEBUG, amd::LOG_AQL))) { auto kit = dev().KernelMap().find(slot->kernel_object); + const char* kname = kit != dev().KernelMap().end() + ? kit->second.getDemangledName().c_str() + : ""; + if (needKernelNames) { + vcmd->addKernelName(kname); + } ClPrint(amd::LOG_DETAIL_DEBUG, amd::LOG_KERN2, "Graph ShaderName : %s, device id : %u", - kit != dev().KernelMap().end() ? kit->second.getDemangledName().c_str() : "", - dev().index()); + kname, dev().index()); if (isBaseKernelDispatch) { logAqlDispatchPacket(roc_device_, gpu_queue_, hdr, slot, slotIdx, priority_); } else { From 1e9ba10081a076139a3c628eae3322af6aca53c4 Mon Sep 17 00:00:00 2001 From: Anu Godavar Date: Wed, 15 Jul 2026 07:02:56 +0000 Subject: [PATCH 6/8] fix(clr): update stale comment in ReportActivity The previous comment claimed kernel_names and timestamps are strictly parallel ("one entry each per dispatch slot"), which is only true when profiling is active at both dispatch time and signal completion. The min-size loop bound is the actual safety mechanism. Co-Authored-By: Claude --- projects/clr/rocclr/platform/activity.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/projects/clr/rocclr/platform/activity.cpp b/projects/clr/rocclr/platform/activity.cpp index 89147ca57d67..7b11350814b9 100644 --- a/projects/clr/rocclr/platform/activity.cpp +++ b/projects/clr/rocclr/platform/activity.cpp @@ -138,8 +138,8 @@ void ReportActivity(const amd::Command& command) { auto timestamps = static_cast(command).getTimestamps(); const auto& kernel_names = static_cast(command).getKernelNames(); - // kernel_names and timestamps are parallel — one entry each per dispatch slot. - // Names are stable const char* resolved at dispatch time, no lookup needed. + // kernel_names and timestamps are both populated only when profiling is active + // at dispatch time. Walk the shorter of the two as a safety bound. for (uint32_t i = 0; i < kernel_names.size() && i < timestamps.size(); i++) { auto it = timestamps[i]; record.begin_ns = it.first; From 2d056a27f096d6f44f86c5fdd89ee357af94056b Mon Sep 17 00:00:00 2001 From: Anu Godavar Date: Wed, 15 Jul 2026 07:21:19 +0000 Subject: [PATCH 7/8] fix(clr): simplify lastEnqueueCommand_ update in appendToCommandBatch The previous code pre-updated lastEnqueueCommand_ before releasing the old command to guard against re-entrant queue access: releasing the old AccumulateCommand could trigger kernelNamesOwner_->release() -> ~GraphExecBase -> finish() back into the queue while lastEnqueueCommand_ still pointed to the old command. That re-entrant path no longer exists since this PR removes kernelNamesOwner_ from AccumulateCommand entirely, so the workaround is removed and the order restored to the straightforward release-then-retain. Co-Authored-By: Claude --- projects/clr/rocclr/platform/commandqueue.hpp | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/projects/clr/rocclr/platform/commandqueue.hpp b/projects/clr/rocclr/platform/commandqueue.hpp index dc26bc1418e9..2c3543fd61e2 100644 --- a/projects/clr/rocclr/platform/commandqueue.hpp +++ b/projects/clr/rocclr/platform/commandqueue.hpp @@ -270,22 +270,18 @@ class HostQueue : public CommandQueue { // an invalid access command->retain(); - // Extra retain for the last command - command->retain(); - - // Update lastEnqueueCommand_ before releasing the old one to prevent - // use-after-free if the release triggers re-entrant access to this queue - // (e.g. ~GraphExec calling finish() on graph-internal streams during - // cascading destruction from the old command's release). - Command* prevLastEnqueue = lastEnqueueCommand_; - lastEnqueueCommand_ = command; - - if (prevLastEnqueue != nullptr) { - prevLastEnqueue->release(); + // Release the last command in the batch + if (lastEnqueueCommand_ != nullptr) { + lastEnqueueCommand_->release(); } else { // The queue becomes active. Add it to the set of activeQueues. device_.addToActiveQueues(this); } + + // Extra retain for the last command + command->retain(); + + lastEnqueueCommand_ = command; } //! Flushes submitted commands if the batch size significantly grew From 0fe4830f7cb5252569ba568050532ab94906e2b4 Mon Sep 17 00:00:00 2001 From: Anusha GodavarthySurya Date: Mon, 20 Jul 2026 01:29:23 +0000 Subject: [PATCH 8/8] fix(clr): drop queue_index_ from addTimestamps call addTimestamps() was changed to take 2 args (start, end) when KernelTimestamp was removed, but this call site was missed, causing a build failure. Co-Authored-By: Claude --- projects/clr/rocclr/device/rocm/rocvirtual.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/projects/clr/rocclr/device/rocm/rocvirtual.cpp b/projects/clr/rocclr/device/rocm/rocvirtual.cpp index 6f23d3918d63..9609ce70a5b5 100644 --- a/projects/clr/rocclr/device/rocm/rocvirtual.cpp +++ b/projects/clr/rocclr/device/rocm/rocvirtual.cpp @@ -491,8 +491,7 @@ void Timestamp::ExtractSignalTiming(ProfilingSignal* signal, if ((command().type() == CL_COMMAND_TASK) && (signal->flags_.isPacketDispatch_ == true)) { static_cast(command()).addTimestamps( static_cast(sig_start * ticksToTime_), - static_cast(sig_end * ticksToTime_), - signal->queue_index_); + static_cast(sig_end * ticksToTime_)); } signal->flags_.done_ = true;