Skip to content

[HLSL] Add a CPU oracle check for bytes outside a stored matrix - #8745

Open
Jack Elliott (JoeCitizen) wants to merge 1 commit into
microsoft:mainfrom
JoeCitizen:linalg-hlk-untouched-bytes-oracle
Open

[HLSL] Add a CPU oracle check for bytes outside a stored matrix#8745
Jack Elliott (JoeCitizen) wants to merge 1 commit into
microsoft:mainfrom
JoeCitizen:linalg-hlk-untouched-bytes-oracle

Conversation

@JoeCitizen

@JoeCitizen Jack Elliott (JoeCitizen) commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

verifyMatrixBuffer only compares the bytes the matrix elements occupy. Once a
matrix sits at a non-zero offset or has a padded stride, everything else in the
buffer is unchecked, so a store that writes the right values in the right places
but also scribbles over the offset prologue or the inter-row padding passes.

This adds countTouchedBytesOutsideElements, a pure function returning how many
non-element bytes no longer hold the expected poison, and verifyUntouchedBytes,
a logging wrapper. The split is deliberate: the logging helper marks a test
failed by construction, so a test exercising its failure path could not itself
pass.

The poison is position-dependent - poisonByteAt(Seed, Offset) is
Seed ^ (BYTE)(Offset * 31u). 31 is odd, so consecutive offsets always differ
and a store writing any single constant across two or more adjacent bytes is
always caught; a constant poison would have been invisible to a store that
happened to write the poison value itself. Element bytes are excluded by
ownership rather than by value, so a correct value can never be mistaken for
poison - verifyMatrixBuffer owns those.

LinAlgCPUOracleTests::UntouchedByteVerification covers a clean buffer, damaged
element bytes (correctly ignored), a damaged prologue, damaged padding, a fill
of the poison seed itself, an adjacency sweep, and a buffer too small for the
layout. A second padded layout leaves twenty bytes outside the elements, more
than the diagnostic cap, so the count is checked to stay exact at twenty while
the reported list caps at eight, with sub-cap offenders reported by buffer
offset.

No GPU test uses it yet; a following change converts the descriptor I/O runner
and adds the padded and cross-layout cases that give it something to find.

Copilot AI balanced review requested due to automatic review settings August 7, 2026 21:53

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

Adds CPU-side validation for matrix stores that corrupt non-element buffer bytes.

Changes:

  • Adds poison-byte corruption counting and diagnostics.
  • Adds host tests for clean, corrupted, and undersized buffers.

Comment thread tools/clang/unittests/HLSLExec/LinAlgTests.cpp Outdated
Comment thread tools/clang/unittests/HLSLExec/LinAlgTests.cpp
Comment thread tools/clang/unittests/HLSLExec/LinAlgTests.cpp Outdated
Copilot AI review requested due to automatic review settings August 7, 2026 22:30
@JoeCitizen
Jack Elliott (JoeCitizen) force-pushed the linalg-hlk-untouched-bytes-oracle branch from 0599f1d to 69604a0 Compare August 7, 2026 22:30

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

const BYTE *Bytes = static_cast<const BYTE *>(Buffer);
size_t Corrupted = 0;
for (size_t I = 0; I < BufferSize; ++I) {
if (Owned[I] || Bytes[I] == Poison)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

what happens if a correct value happens to be the same value as the Poison?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Made the poison value positional so it mitigates this risk

A matrix stored to a buffer does not own every byte of it. The prologue before
the offset, and the padding between rows when the stride exceeds a packed row,
belong to whatever else shares the buffer and must survive a store untouched.

Comparing elements alone does not check this. A store that ignores the stride
writes its elements to the wrong addresses and fails the element comparison
anyway, so the only case a byte-level check adds is a store that places every
element correctly and also widens its writes over the padding, which produces a
correct matrix while silently corrupting its neighbours. Wide vectorised writes
overrunning a row are a plausible way for an implementation to do exactly that.

Counting is separated from reporting because verifyUntouchedBytes reports
through Log::Error, which marks the calling test failed. A unit test that
deliberately supplies a corrupted buffer therefore cannot call it, so the
counting half is exposed on its own and the unit test drives that.

The execution tests do not use either function yet; a following change converts
the descriptor load and store runner to seed its destination and call it.

UntouchedByteVerification covers both directions on the host: a correctly
encoded buffer reports no offending bytes, damaging an element reports none
because that is the element comparison's job, damaging the prologue or the
padding reports exactly one, damaging every non-element byte reports all eight,
and a buffer too small for the layout is rejected outright. Stubbing the count
to always return zero fails the test, so it is not vacuous.

Validated on WARP: 28 total, 23 passed, the same 4 pre-existing failures and 1
skip as before, so no existing outcome changed.

Assisted-by: GitHub Copilot

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 83725f5d-8e98-4c1d-91ee-ad47629e007b
Copilot AI review requested due to automatic review settings August 8, 2026 01:33
@JoeCitizen
Jack Elliott (JoeCitizen) force-pushed the linalg-hlk-untouched-bytes-oracle branch from 69604a0 to be788b6 Compare August 8, 2026 01:33

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

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