From be788b63d4bd7590f4d8440c52174056bf6f8bd7 Mon Sep 17 00:00:00 2001 From: Jack Elliott Date: Sat, 8 Aug 2026 09:39:26 +1200 Subject: [PATCH] [HLSL] Add a CPU oracle check for bytes outside a stored matrix 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 --- .../clang/unittests/HLSLExec/LinAlgTests.cpp | 257 ++++++++++++++++++ 1 file changed, 257 insertions(+) diff --git a/tools/clang/unittests/HLSLExec/LinAlgTests.cpp b/tools/clang/unittests/HLSLExec/LinAlgTests.cpp index f8ecf9e201..58cef231d7 100644 --- a/tools/clang/unittests/HLSLExec/LinAlgTests.cpp +++ b/tools/clang/unittests/HLSLExec/LinAlgTests.cpp @@ -1186,6 +1186,118 @@ static bool verifyMatrixBuffer(const void *ActualBuffer, return false; } +// The bytes a matrix does not occupy -- the prologue before the offset, and +// the padding between rows when the stride exceeds a packed row -- must +// survive a store untouched. +// +// Comparing elements alone would not catch a store that damages the bytes +// around them. A store that ignored the stride entirely writes its elements to +// the wrong addresses and fails the element comparison anyway, but a store +// that places every element correctly and also widens its writes over the +// padding produces a correct matrix while silently corrupting whatever else +// shared the buffer. +// Seeding the destination with a poison pattern and checking that the +// non-element bytes still hold it separates those two cases. +// +// The pattern varies with the byte offset rather than repeating a single +// value. A constant would be indistinguishable from a store that happened to +// write that same value, and also from memory nobody wrote at all -- 0xcd, the +// obvious choice, is what the MSVC debug allocator fills fresh heap with. +// Multiplying the offset by an odd number keeps consecutive bytes distinct, so +// a store writing any constant over two or more adjacent bytes is always +// caught, and a single overwritten byte survives only if it happens to match +// the pattern at exactly that offset. +// +// Counting is kept separate from reporting so the check can be unit tested in +// both directions. verifyUntouchedBytes reports through Log::Error, which +// marks the calling test failed, so a test that deliberately supplies a +// corrupted buffer cannot call it. +static BYTE poisonByteAt(BYTE Seed, size_t Offset) { + return static_cast(Seed ^ static_cast(Offset * 31u)); +} + +static void fillPoison(void *Buffer, size_t BufferSize, BYTE Seed) { + BYTE *Bytes = static_cast(Buffer); + for (size_t I = 0; I < BufferSize; ++I) + Bytes[I] = poisonByteAt(Seed, I); +} + +// Returns the number of offending bytes, or nullopt if the buffer cannot hold +// the described matrix at all. FirstOffsets, when supplied, collects the +// leading offenders for diagnostics. +static std::optional countTouchedBytesOutsideElements( + ComponentType CompType, MatrixDim M, MatrixDim N, + const MatrixBufferLayout &Layout, const void *Buffer, size_t BufferSize, + BYTE PoisonSeed, std::vector *FirstOffsets = nullptr) { + static constexpr size_t MaxReportedOffsets = 8; + + std::optional RequiredBytes = + getMatrixBufferSize(CompType, M, N, Layout); + if (!RequiredBytes || BufferSize < *RequiredBytes) + return std::nullopt; + + const size_t ElementBytes = elementSize(CompType); + std::vector Owned(BufferSize, false); + for (MatrixDim Row = 0; Row < M; ++Row) { + for (MatrixDim Column = 0; Column < N; ++Column) { + std::optional ByteOffset = + getElementByteOffset(CompType, M, N, Row, Column, Layout); + if (!ByteOffset || *ByteOffset + ElementBytes > BufferSize) + return std::nullopt; + for (size_t I = 0; I < ElementBytes; ++I) + Owned[*ByteOffset + I] = true; + } + } + + const BYTE *Bytes = static_cast(Buffer); + size_t Corrupted = 0; + for (size_t I = 0; I < BufferSize; ++I) { + if (Owned[I] || Bytes[I] == poisonByteAt(PoisonSeed, I)) + continue; + if (FirstOffsets && FirstOffsets->size() < MaxReportedOffsets) + FirstOffsets->push_back(I); + ++Corrupted; + } + return Corrupted; +} + +// Reporting wrapper around countTouchedBytesOutsideElements for the execution +// tests. +static bool verifyUntouchedBytes(ComponentType CompType, MatrixDim M, + MatrixDim N, const MatrixBufferLayout &Layout, + const void *Buffer, size_t BufferSize, + BYTE PoisonSeed, bool Verbose) { + std::vector FirstOffsets; + std::optional Corrupted = countTouchedBytesOutsideElements( + CompType, M, N, Layout, Buffer, BufferSize, PoisonSeed, &FirstOffsets); + + if (!Corrupted) { + hlsl_test::LogErrorFmt( + L"Buffer of %zu bytes cannot hold the requested matrix layout", + BufferSize); + return false; + } + + if (*Corrupted == 0) { + if (Verbose) + hlsl_test::LogCommentFmt(L"Every byte outside the stored elements still " + L"holds the poison pattern for seed 0x%02x", + PoisonSeed); + return true; + } + + for (size_t Offset : FirstOffsets) + hlsl_test::LogErrorFmt( + L"Byte %zu is outside every element but was overwritten: " + L"actual=0x%02x, expected poison=0x%02x", + Offset, static_cast(Buffer)[Offset], + poisonByteAt(PoisonSeed, Offset)); + hlsl_test::LogErrorFmt(L"%zu bytes outside the stored elements were " + L"overwritten", + *Corrupted); + return false; +} + } // namespace cpu_oracle static std::string buildCompilerArgs(const MatrixParams &Params, @@ -1416,6 +1528,7 @@ class LinAlgCPUOracleTests { END_TEST_CLASS() TEST_METHOD(TypedMatrixBufferRoundTrip); + TEST_METHOD(UntouchedByteVerification); }; void LinAlgCPUOracleTests::TypedMatrixBufferRoundTrip() { @@ -1542,6 +1655,150 @@ void LinAlgCPUOracleTests::TypedMatrixBufferRoundTrip() { std::string::npos); } +// The padding check is verified here rather than only through the execution +// tests because a GPU round trip cannot easily produce a store that places +// every element correctly and still damages the bytes around them, which is +// the single case this check exists to catch. +void LinAlgCPUOracleTests::UntouchedByteVerification() { + using namespace cpu_oracle; + + // A 2x3 uint32 matrix at a 4 byte offset with a 16 byte stride occupies + // bytes 4..15 and 20..31, leaving a 4 byte prologue at 0..3 and 4 bytes of + // padding at 16..19. + std::optional Matrix = + makeTypedMatrix(2, 3, {1, 2, 3, 4, 5, 6}); + VERIFY_IS_TRUE(Matrix.has_value()); + + const MatrixBufferLayout Layout = { + MatrixLayout::RowMajor, + /*OffsetBytes=*/4, + /*StrideBytes=*/16, + }; + std::optional Size = getMatrixBufferSize(*Matrix, Layout); + VERIFY_IS_TRUE(Size.has_value()); + VERIFY_ARE_EQUAL(size_t(32), *Size); + + constexpr BYTE PoisonSeed = 0xa5; + std::vector Buffer(*Size); + fillPoison(Buffer.data(), Buffer.size(), PoisonSeed); + VERIFY_IS_TRUE(writeMatrixBuffer(*Matrix, Layout, Buffer)); + + auto CountTouched = [&Layout, PoisonSeed](const std::vector &Bytes) { + return countTouchedBytesOutsideElements(ComponentType::U32, 2, 3, Layout, + Bytes.data(), Bytes.size(), + PoisonSeed); + }; + + // A correctly encoded buffer leaves every non-element byte poisoned. + std::optional Clean = CountTouched(Buffer); + VERIFY_IS_TRUE(Clean.has_value()); + VERIFY_ARE_EQUAL(size_t(0), *Clean); + VERIFY_IS_TRUE(verifyUntouchedBytes(ComponentType::U32, 2, 3, Layout, + Buffer.data(), Buffer.size(), PoisonSeed, + /*Verbose=*/false)); + + // Damaging an element is the element comparison's job, not this check's, so + // the count must stay at zero. + std::vector ElementTouched = Buffer; + ElementTouched[4] ^= 0xff; + std::optional AfterElement = CountTouched(ElementTouched); + VERIFY_IS_TRUE(AfterElement.has_value()); + VERIFY_ARE_EQUAL(size_t(0), *AfterElement); + + // Damaging the prologue or the inter-row padding is what this check exists + // to catch, so each one must be counted. + for (size_t Offset : {size_t(0), size_t(16)}) { + std::vector PaddingTouched = Buffer; + PaddingTouched[Offset] ^= 0xff; + std::optional AfterPadding = CountTouched(PaddingTouched); + VERIFY_IS_TRUE(AfterPadding.has_value()); + VERIFY_ARE_EQUAL(size_t(1), *AfterPadding); + } + + // Every non-element byte damaged at once is still counted exactly. + std::vector AllTouched(*Size); + fillPoison(AllTouched.data(), AllTouched.size(), PoisonSeed); + for (BYTE &Byte : AllTouched) + Byte = static_cast(~Byte); + VERIFY_IS_TRUE(writeMatrixBuffer(*Matrix, Layout, AllTouched)); + std::optional AfterAll = CountTouched(AllTouched); + VERIFY_IS_TRUE(AfterAll.has_value()); + VERIFY_ARE_EQUAL(size_t(8), *AfterAll); + + // A buffer filled with one repeated value is fully detected, which is the + // reason the pattern varies with the offset. A constant poison would score + // zero here whenever the store happened to pick that same value, and 0xcd in + // particular is what the MSVC debug allocator leaves in memory nobody wrote. + std::vector ConstantFill(*Size, BYTE(0xcd)); + std::optional AfterConstant = CountTouched(ConstantFill); + VERIFY_IS_TRUE(AfterConstant.has_value()); + VERIFY_ARE_EQUAL(size_t(8), *AfterConstant); + + // The case a constant poison cannot survive: a store that writes a value the + // poison pattern itself uses. Because the pattern varies, that value matches + // at exactly one offset, so seven of the eight non-element bytes are still + // caught. A constant poison would match everywhere and report nothing. + std::vector PoisonValuedFill(*Size, poisonByteAt(PoisonSeed, 0)); + std::optional AfterPoisonValued = CountTouched(PoisonValuedFill); + VERIFY_IS_TRUE(AfterPoisonValued.has_value()); + VERIFY_ARE_EQUAL(size_t(7), *AfterPoisonValued); + + // No two adjacent bytes share a poison value, so a constant written over any + // two neighbours cannot hide in both. + for (size_t Offset = 1; Offset < *Size; ++Offset) + VERIFY_ARE_NOT_EQUAL(poisonByteAt(PoisonSeed, Offset - 1), + poisonByteAt(PoisonSeed, Offset)); + + // The diagnostic list is capped but the count is not, so the two have to be + // checked against a buffer with more offenders than the cap. A 2x3 uint32 + // matrix at a 16 byte offset with a 16 byte stride occupies bytes 16..27 and + // 32..43, leaving twenty bytes outside the elements. + const MatrixBufferLayout PaddedLayout = { + MatrixLayout::RowMajor, + /*OffsetBytes=*/16, + /*StrideBytes=*/16, + }; + std::optional PaddedSize = + getMatrixBufferSize(ComponentType::U32, 2, 3, PaddedLayout); + VERIFY_IS_TRUE(PaddedSize.has_value()); + VERIFY_ARE_EQUAL(size_t(44), *PaddedSize); + + std::vector AllPaddingTouched(*PaddedSize); + fillPoison(AllPaddingTouched.data(), AllPaddingTouched.size(), PoisonSeed); + for (BYTE &Byte : AllPaddingTouched) + Byte = static_cast(~Byte); + std::vector ReportedOffsets; + std::optional AfterPadded = countTouchedBytesOutsideElements( + ComponentType::U32, 2, 3, PaddedLayout, AllPaddingTouched.data(), + AllPaddingTouched.size(), PoisonSeed, &ReportedOffsets); + VERIFY_IS_TRUE(AfterPadded.has_value()); + VERIFY_ARE_EQUAL(size_t(20), *AfterPadded); + VERIFY_ARE_EQUAL(size_t(8), ReportedOffsets.size()); + for (size_t I = 0; I < ReportedOffsets.size(); ++I) + VERIFY_ARE_EQUAL(I, ReportedOffsets[I]); + + // Below the cap every offender is reported, and by its offset in the buffer + // rather than its position among the offenders. + std::vector TwoPaddingBytes(*PaddedSize); + fillPoison(TwoPaddingBytes.data(), TwoPaddingBytes.size(), PoisonSeed); + TwoPaddingBytes[28] ^= 0xff; + TwoPaddingBytes[29] ^= 0xff; + std::vector TwoOffsets; + std::optional AfterTwo = countTouchedBytesOutsideElements( + ComponentType::U32, 2, 3, PaddedLayout, TwoPaddingBytes.data(), + TwoPaddingBytes.size(), PoisonSeed, &TwoOffsets); + VERIFY_IS_TRUE(AfterTwo.has_value()); + VERIFY_ARE_EQUAL(size_t(2), *AfterTwo); + VERIFY_ARE_EQUAL(size_t(2), TwoOffsets.size()); + VERIFY_ARE_EQUAL(size_t(28), TwoOffsets[0]); + VERIFY_ARE_EQUAL(size_t(29), TwoOffsets[1]); + + // A buffer too small for the layout cannot be checked at all. + std::vector TooSmall(*Size - 1); + fillPoison(TooSmall.data(), TooSmall.size(), PoisonSeed); + VERIFY_IS_FALSE(CountTouched(TooSmall).has_value()); +} + class LinAlgCapabilityTests { public: BEGIN_TEST_CLASS(LinAlgCapabilityTests)