Skip to content

[PIX] Fix DXR invocation log bounds - #8845

Open
Damyan Pepper (damyanp) wants to merge 1 commit into
users/damyanp/pix-fixes-03from
users/damyanp/pix-fixes-04
Open

[PIX] Fix DXR invocation log bounds#8845
Damyan Pepper (damyanp) wants to merge 1 commit into
users/damyanp/pix-fixes-03from
users/damyanp/pix-fixes-04

Conversation

@damyanp

@damyanp Damyan Pepper (damyanp) commented Aug 27, 2026

Copy link
Copy Markdown
Member

Part 4 of 14 in the PIX instrumentation stack. It targets users/damyanp/pix-fixes-03. Its content depends on PR 2 for the shared helpers and PR 1 for the validation helpers.

Each DXR invocation takes one slot in the log with an atomic increment on the counter UAV. If the slot number is too large, the pass limits it to the last slot. Every invocation that overflows therefore writes over the last correct record. PIX sees a full log with a plausible final entry and cannot detect the truncation.

A capacity of zero makes that clamp wrap to a very large value, so a log with no slots accepts writes.

An out-of-range slot must produce no write. The correct records then stay unchanged, and the overflow stays visible, because the claimed count is larger than the capacity.

An invocation that overflows contributes no record. A tool that treats a full log as a complete log must compare the claimed count with the capacity.

Assisted-by: Copilot

This changes only the PIX instrumentation, so it needs no release note.


Stack created with GitHub Stacks CLIGive Feedback 💬

Each DXR invocation takes one slot in the log with an atomic increment on the counter UAV. If the slot number is too large, the pass limits it to the last slot. Every invocation that overflows therefore writes over the last correct record. PIX sees a full log with a plausible final entry and cannot detect the truncation.

A capacity of zero makes that clamp wrap to a very large value, so a log with no slots accepts writes.

An out-of-range slot must produce no write. The correct records then stay unchanged, and the overflow stays visible, because the claimed count is larger than the capacity.

An invocation that overflows contributes no record. A tool that treats a full log as a complete log must compare the claimed count with the capacity.

Assisted-by: Copilot

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 40dc9de3-617e-4caf-ab0d-fba0a033ed93

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes PIX DXR invocation logging so out-of-capacity entries do not overwrite valid records.

Changes:

  • Replaces index clamping with a guarded write path.
  • Disables instrumentation for zero-capacity logs.
  • Adds overflow, boundary, and validation tests.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
lib/DxilPIXPasses/DxilPIXDXRInvocationsLog.cpp Adds bounds-checked record writes.
tools/clang/unittests/HLSL/PixTest.cpp Adds capacity and validation tests.
tools/clang/test/HLSLFileCheck/pix/InvocationsLog_OverflowIsNotClamped.hlsl Verifies overflow is guarded rather than clamped.
tools/clang/test/HLSLFileCheck/pix/InvocationsLog_ClosestHit.hlsl Updates checks to reflect removal of clamping.
Suppressed comments (3)

tools/clang/unittests/HLSL/PixTest.cpp:3671

  • These declarations infer straightforward CComPtr<IDxcBlob> and std::vector<std::string> types. Please spell them explicitly per the repository's almost-never-auto convention (PixTest.cpp:1814-1815 and PixTest.cpp:3724 show the established forms).
  auto compiledLib =
      Compile(m_dllSupport, kSingleMissInvocationLogShader, L"lib_6_6", {});
  auto output = RunDxilPIXDXRInvocationsLog(compiledLib, 1);
  auto lines = Tokenize(Disassemble(output), "\n");

tools/clang/unittests/HLSL/PixTest.cpp:3680

  • These declarations infer straightforward CComPtr<IDxcBlob> and std::vector<std::string> types. Please spell them explicitly per the repository's almost-never-auto convention (PixTest.cpp:1814-1815 and PixTest.cpp:3724 show the established forms).
  auto compiledLib =
      Compile(m_dllSupport, kSingleMissInvocationLogShader, L"lib_6_6", {});
  auto output = RunDxilPIXDXRInvocationsLog(compiledLib, 24);
  auto lines = Tokenize(Disassemble(output), "\n");

tools/clang/unittests/HLSL/PixTest.cpp:3688

  • These declarations infer the short CComPtr<IDxcBlob> type. Please spell it explicitly per the repository's almost-never-auto convention; the same test file uses this explicit form at PixTest.cpp:1814-1815.
  auto compiledLib =
      Compile(m_dllSupport, kSingleMissInvocationLogShader, L"lib_6_6", {});
  auto output = RunDxilPIXDXRInvocationsLog(compiledLib, 1);

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

Comment on lines +207 to +208
auto *EntryIndexIsInRange = Builder.CreateICmpULT(
EntryIndex, MaxEntryCountAsConstant, "EntryIndexIsInRange");
Comment on lines +3655 to +3663
auto compiledLib =
Compile(m_dllSupport, kSingleMissInvocationLogShader, L"lib_6_6", {});

auto oneEntryOutput = RunDxilPIXDXRInvocationsLog(compiledLib, 1);
auto oneEntryLines = Tokenize(Disassemble(oneEntryOutput), "\n");
VERIFY_ARE_EQUAL(2, CountToolsUAVRecords(oneEntryLines));

auto zeroEntryOutput = RunDxilPIXDXRInvocationsLog(compiledLib, 0);
auto zeroEntryLines = Tokenize(Disassemble(zeroEntryOutput), "\n");
Comment on lines +687 to +693
const std::string expectedSuffix = ", " + std::to_string(expectedEntryCount);
for (auto const &line : lines) {
if (line.find("icmp ult i32 %EntryIndexResult") != std::string::npos &&
line.find(expectedSuffix) != std::string::npos) {
return true;
}
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: New

Development

Successfully merging this pull request may close these issues.

3 participants