From 5238ee0c4a079c3530587a1540760d470393bcf2 Mon Sep 17 00:00:00 2001 From: Damyan Pepper Date: Mon, 24 Aug 2026 15:05:14 -0700 Subject: [PATCH 1/8] [PIX] Add validation support to PIX pass tests The PIX passes change DXIL after the compiler completes. A pass can add a resource, change a root signature, insert an operation, and remove a declaration. The result can be a module that the validator refuses. The tests only examine the disassembly, so they cannot find this type of defect. The new helpers run the DXIL validator on the output of a pass. They also separate the diagnostics that PIX instrumentation is permitted to cause from the diagnostics that are defects. The set of permitted diagnostics decides what every later layer can ignore, so it is the part to examine with care. There is no change to the compiler. Assisted-by: Copilot Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 40dc9de3-617e-4caf-ab0d-fba0a033ed93 --- tools/clang/unittests/HLSL/PixTest.cpp | 247 +++++++++++++++++++++++++ 1 file changed, 247 insertions(+) diff --git a/tools/clang/unittests/HLSL/PixTest.cpp b/tools/clang/unittests/HLSL/PixTest.cpp index 18c8a1c58b..e21546c9ac 100644 --- a/tools/clang/unittests/HLSL/PixTest.cpp +++ b/tools/clang/unittests/HLSL/PixTest.cpp @@ -160,6 +160,12 @@ class PixTest : public ::testing::Test { TEST_METHOD(NonUniformResourceIndex_DescriptorHeap) TEST_METHOD(NonUniformResourceIndex_Raytracing) + // Control tests for the PIX pass validation harness below + // (ValidateInstrumentedModule / VerifyInstrumentedModuleIsValid). + TEST_METHOD(Validation_ControlValidModulePasses) + TEST_METHOD(Validation_ControlInvalidModuleFails) + TEST_METHOD(Validation_ControlBoilerplateOnlyFailureIsRejected) + dxc::DxCompilerDllLoader m_dllSupport; VersionSupportInfo m_ver; @@ -263,6 +269,142 @@ class PixTest : public ::testing::Test { std::move(pOptimizedModule), {}, Tokenize(outputText.c_str(), "\n")}; } + // Runs one named PIX or DXIL pass and returns the resulting module and + // its disassembly lines. + struct SinglePassOutput { + CComPtr Module; + std::vector Lines; + }; + + SinglePassOutput RunSinglePass(IDxcBlob *dxil, LPCWSTR passOption) { + CComPtr pOptimizer; + VERIFY_SUCCEEDED( + m_dllSupport.CreateInstance(CLSID_DxcOptimizer, &pOptimizer)); + std::vector Options; + Options.push_back(L"-opt-mod-passes"); + Options.push_back(passOption); + + CComPtr pOptimizedModule; + CComPtr pText; + VERIFY_SUCCEEDED(pOptimizer->RunOptimizer( + dxil, Options.data(), Options.size(), &pOptimizedModule, &pText)); + + SinglePassOutput ret; + ret.Module = pOptimizedModule; + ret.Lines = Tokenize(BlobToUtf8(pText).c_str(), "\n"); + return ret; + } + + // PIX does not validate the shaders its passes instrument, so a pass + // that produces invalid DXIL goes undetected elsewhere. Validate here + // instead. + struct ValidationResult { + bool Valid; + std::string Errors; + }; + + ValidationResult ValidateInstrumentedModule(IDxcBlob *pModule) { + CComPtr pContainer; + + // Some pass runners return a bare bitcode module; others already + // return a container. The validator accepts only a container. + if (hlsl::IsDxilContainerLike(pModule->GetBufferPointer(), + pModule->GetBufferSize()) != nullptr) { + pContainer = pModule; + } else { + pContainer = pix_test::WrapInNewContainer(m_dllSupport, pModule); + } + + CComPtr pValidator; + VERIFY_SUCCEEDED( + m_dllSupport.CreateInstance(CLSID_DxcValidator, &pValidator)); + + CComPtr pValidationResult; + VERIFY_SUCCEEDED(pValidator->Validate(pContainer, DxcValidatorFlags_Default, + &pValidationResult)); + + HRESULT validationStatus; + VERIFY_SUCCEEDED(pValidationResult->GetStatus(&validationStatus)); + if (SUCCEEDED(validationStatus)) { + return {true, {}}; + } + + CComPtr pValidationErrors; + VERIFY_SUCCEEDED(pValidationResult->GetErrorBuffer(&pValidationErrors)); + return {false, BlobToUtf8(pValidationErrors)}; + } + + // Significant holds validator diagnostics other than boilerplate and the + // permitted metadata exception. PermittedExceptionCount counts the + // exception separately. + struct FilteredValidationDiagnostics { + std::vector Significant; + int PermittedExceptionCount = 0; + }; + + // Filters out boilerplate ("Validation failed.") and the permitted + // metadata exception: virtual-register annotation passes add metadata + // that DXIL does not consume, so the validator reports it as unused. Do + // not widen this filter. + FilteredValidationDiagnostics + GetSignificantValidationDiagnostics(const std::string &errors) { + FilteredValidationDiagnostics result; + std::stringstream errorStream(errors); + std::string line; + while (std::getline(errorStream, line)) { + if (!line.empty() && line.back() == '\r') { + line.pop_back(); + } + if (line.empty() || line == "Validation failed.") { + continue; + } + if (line.find("All metadata must be used by dxil") != std::string::npos) { + result.PermittedExceptionCount++; + continue; + } + result.Significant.push_back(line); + } + return result; + } + + // True only if the diagnostics contain no significant errors and at + // least one instance of the permitted metadata exception. + bool IsPermittedValidationException( + const FilteredValidationDiagnostics &diagnostics) { + return diagnostics.Significant.empty() && + diagnostics.PermittedExceptionCount > 0; + } + + // Asserts an instrumented module validates. Accepts a module whose only + // diagnostic is the permitted metadata exception; logs and fails on any + // other validator error. + void VerifyInstrumentedModuleIsValid(IDxcBlob *pModule, + const char *description) { + ValidationResult validation = ValidateInstrumentedModule(pModule); + if (validation.Valid) { + return; + } + + FilteredValidationDiagnostics diagnostics = + GetSignificantValidationDiagnostics(validation.Errors); + if (IsPermittedValidationException(diagnostics)) { + return; + } + + std::string joined; + if (diagnostics.Significant.empty()) { + joined = "(validator reported failure with no significant diagnostic " + "text, and no permitted metadata exception was found)"; + } else { + for (auto const &significantError : diagnostics.Significant) { + joined += significantError + "\n"; + } + } + WEX::Logging::Log::Error(WEX::Common::String().Format( + L"Validation failed after %S:\n%S", description, joined.c_str())); + VERIFY_FAIL(); + } + CComPtr FindModule(hlsl::DxilFourCC fourCC, IDxcBlob *pSource) { const UINT32 BC_C0DE = ((INT32)(INT8)'B' | (INT32)(INT8)'C' << 8 | (INT32)0xDEC0 << 16); // BC0xc0de in big endian @@ -3467,3 +3609,108 @@ void main(uint3 tid : SV_DispatchThreadID) { } VERIFY_ARE_EQUAL(debugBreakBitSetCount, 2); } + +/////////////////////////////////////////////////////////////////////////////// +// Control tests for the PIX pass validation harness +// (ValidateInstrumentedModule / VerifyInstrumentedModuleIsValid). +// +// Both tests instrument the same trivial pixel shader with the +// virtual-register annotation pass, so the valid and invalid cases are +// directly comparable. + +TEST_F(PixTest, Validation_ControlValidModulePasses) { + const char *source = R"x( +float main() : SV_Target +{ + return 0; +})x"; + + // Virtual-register annotation adds metadata that DXIL does not consume, + // so this module only validates via the permitted metadata exception. + auto compiled = Compile(m_dllSupport, source, L"ps_6_0", {L"-Od"}); + auto output = RunSinglePass(compiled, L"-dxil-annotate-with-virtual-regs"); + VerifyInstrumentedModuleIsValid( + output.Module, + "virtual-register annotation of a trivial pixel shader (validation " + "harness control)"); +} + +TEST_F(PixTest, Validation_ControlInvalidModuleFails) { + const char *source = R"x( +float main() : SV_Target +{ + return 0; +})x"; + + // Same shader and pass as Validation_ControlValidModulePasses; only the + // corruption below differs. + auto compiled = Compile(m_dllSupport, source, L"ps_6_0", {L"-Od"}); + auto output = RunSinglePass(compiled, L"-dxil-annotate-with-virtual-regs"); + + // Confirm the baseline validates before corrupting it, so the failure + // below is caused by the corruption and nothing else. + VerifyInstrumentedModuleIsValid( + output.Module, + "virtual-register annotation of a trivial pixel shader, uncorrupted " + "baseline (validation harness control)"); + + // Mislabel the shader stage. The validator must reject this regardless + // of the permitted metadata exception. + std::string disassembly = Disassemble(output.Module); + const std::string shaderKindTag = "!\"ps\","; + auto tagPosition = disassembly.find(shaderKindTag); + VERIFY_IS_TRUE(tagPosition != std::string::npos); + disassembly.replace(tagPosition, shaderKindTag.size(), "!\"vs\","); + + CComPtr pDisassemblyBlob; + CreateBlobFromText(m_dllSupport, disassembly.c_str(), &pDisassemblyBlob); + + CComPtr pAssembler; + VERIFY_SUCCEEDED( + m_dllSupport.CreateInstance(CLSID_DxcAssembler, &pAssembler)); + CComPtr pAssembleResult; + VERIFY_SUCCEEDED( + pAssembler->AssembleToContainer(pDisassemblyBlob, &pAssembleResult)); + HRESULT assembleStatus; + VERIFY_SUCCEEDED(pAssembleResult->GetStatus(&assembleStatus)); + VERIFY_SUCCEEDED(assembleStatus); + CComPtr pCorruptedContainer; + VERIFY_SUCCEEDED(pAssembleResult->GetResult(&pCorruptedContainer)); + + ValidationResult validation = ValidateInstrumentedModule(pCorruptedContainer); + VERIFY_IS_FALSE(validation.Valid); + + // Confirm the corruption produces a real diagnostic, not just the + // permitted metadata exception. + FilteredValidationDiagnostics diagnostics = + GetSignificantValidationDiagnostics(validation.Errors); + VERIFY_IS_FALSE(diagnostics.Significant.empty()); +} + +// Tests that a validator failure is rejected unless its only diagnostic is +// the permitted metadata exception. A failure with only the "Validation +// failed." boilerplate and no exception must not pass. +TEST_F(PixTest, Validation_ControlBoilerplateOnlyFailureIsRejected) { + // Boilerplate only, no permitted exception: must be rejected. + FilteredValidationDiagnostics boilerplateOnly = + GetSignificantValidationDiagnostics("Validation failed.\n"); + VERIFY_IS_TRUE(boilerplateOnly.Significant.empty()); + VERIFY_ARE_EQUAL(boilerplateOnly.PermittedExceptionCount, 0); + VERIFY_IS_FALSE(IsPermittedValidationException(boilerplateOnly)); + + // Permitted exception present: must be accepted. + FilteredValidationDiagnostics exceptionOnly = + GetSignificantValidationDiagnostics( + "Validation failed.\n" + "All metadata must be used by dxil's users.\n"); + VERIFY_IS_TRUE(exceptionOnly.Significant.empty()); + VERIFY_IS_TRUE(exceptionOnly.PermittedExceptionCount > 0); + VERIFY_IS_TRUE(IsPermittedValidationException(exceptionOnly)); + + // Real diagnostic present: must be rejected, even with the exception. + FilteredValidationDiagnostics realDiagnostic = + GetSignificantValidationDiagnostics("Validation failed.\n" + "Some real validator diagnostic.\n"); + VERIFY_IS_FALSE(realDiagnostic.Significant.empty()); + VERIFY_IS_FALSE(IsPermittedValidationException(realDiagnostic)); +} From fac85e19962e832e4c9c9849df21f17b430fbd21 Mon Sep 17 00:00:00 2001 From: Damyan Pepper Date: Wed, 2 Sep 2026 10:17:05 -0700 Subject: [PATCH 2/8] [PIX] Allow only known PIX metadata before validation Virtual-register annotation attaches metadata that DXIL does not consume, so validation of an instrumented module always reported a generic "unused metadata" diagnostic even when the module was otherwise correct. The prior handling matched that diagnostic by substring, which would also swallow a genuinely unrelated unused-metadata defect. Replace it with a structural check: on direct validation failure, clone the module, strip only the four known PIX virtual-register metadata kinds, and revalidate. Accept only if the stripped clone validates, proving PIX metadata was the sole cause. Rework Validation_ControlInvalidModuleFails so the corrupted container itself proves both facts independently: direct validation's diagnostic confirms the permitted PIX metadata is present and unused, and the harness's rejection confirms a real, non-boilerplate defect remains. Replace Validation_ControlBoilerplateOnlyFailureIsRejected, which exercised the removed string classifier, with Validation_ControlNonPixUnusedMetadataIsRejected: a foreign metadata kind alongside the module's own PIX metadata must still be rejected after the four-kind strip. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- tools/clang/unittests/HLSL/PixTest.cpp | 234 ++++++++++++++----------- 1 file changed, 135 insertions(+), 99 deletions(-) diff --git a/tools/clang/unittests/HLSL/PixTest.cpp b/tools/clang/unittests/HLSL/PixTest.cpp index e21546c9ac..e6c803a55c 100644 --- a/tools/clang/unittests/HLSL/PixTest.cpp +++ b/tools/clang/unittests/HLSL/PixTest.cpp @@ -164,7 +164,7 @@ class PixTest : public ::testing::Test { // (ValidateInstrumentedModule / VerifyInstrumentedModuleIsValid). TEST_METHOD(Validation_ControlValidModulePasses) TEST_METHOD(Validation_ControlInvalidModuleFails) - TEST_METHOD(Validation_ControlBoilerplateOnlyFailureIsRejected) + TEST_METHOD(Validation_ControlNonPixUnusedMetadataIsRejected) dxc::DxCompilerDllLoader m_dllSupport; VersionSupportInfo m_ver; @@ -303,18 +303,18 @@ class PixTest : public ::testing::Test { std::string Errors; }; - ValidationResult ValidateInstrumentedModule(IDxcBlob *pModule) { - CComPtr pContainer; - - // Some pass runners return a bare bitcode module; others already - // return a container. The validator accepts only a container. + // The validator (and the assembler, when reconstructing a container + // from bare bitcode) both require a container; some pass runners + // return bare bitcode instead. + CComPtr NormalizeToContainer(IDxcBlob *pModule) { if (hlsl::IsDxilContainerLike(pModule->GetBufferPointer(), pModule->GetBufferSize()) != nullptr) { - pContainer = pModule; - } else { - pContainer = pix_test::WrapInNewContainer(m_dllSupport, pModule); + return pModule; } + return pix_test::WrapInNewContainer(m_dllSupport, pModule); + } + ValidationResult RunValidator(IDxcBlob *pContainer) { CComPtr pValidator; VERIFY_SUCCEEDED( m_dllSupport.CreateInstance(CLSID_DxcValidator, &pValidator)); @@ -334,21 +334,82 @@ class PixTest : public ::testing::Test { return {false, BlobToUtf8(pValidationErrors)}; } - // Significant holds validator diagnostics other than boilerplate and the - // permitted metadata exception. PermittedExceptionCount counts the - // exception separately. - struct FilteredValidationDiagnostics { - std::vector Significant; - int PermittedExceptionCount = 0; - }; + // The four metadata kinds PIX's virtual-register annotation pass + // intentionally leaves unused for downstream tools to consume. See + // DxilPIXVirtualRegisters.h. + static constexpr const char *KnownPixVirtualRegisterMetadataKinds[] = { + pix_dxil::PixDxilInstNum::MDName, pix_dxil::PixDxilReg::MDName, + pix_dxil::PixAllocaReg::MDName, pix_dxil::PixAllocaRegWrite::MDName}; + + // Removes the four known PIX metadata kinds from every function and + // instruction. + static void StripKnownPixVirtualRegisterMetadata(llvm::Module &M) { + llvm::LLVMContext &Ctx = M.getContext(); + for (const char *kind : KnownPixVirtualRegisterMetadataKinds) { + unsigned kindID = Ctx.getMDKindID(kind); + for (llvm::Function &F : M) { + F.setMetadata(kindID, nullptr); + for (llvm::BasicBlock &BB : F) { + for (llvm::Instruction &I : BB) { + I.setMetadata(kindID, nullptr); + } + } + } + } + } + + // Parses pContainer into an isolated LLVM module, applies Mutate to it, + // and re-serializes into a fresh validator-ready container. + template + CComPtr CloneModuleAndMutate(IDxcBlob *pContainer, + MutatorFn Mutate) { + ModuleAndHangersOn moduleEtc(pContainer); + llvm::Module *M = moduleEtc.GetDxilModule().GetModule(); + Mutate(*M); + + llvm::SmallVector bitcode; + { + llvm::raw_svector_ostream OS(bitcode); + llvm::WriteBitcodeToFile(M, OS); + } + + CComPtr pLibrary; + VERIFY_SUCCEEDED(m_dllSupport.CreateInstance(CLSID_DxcLibrary, &pLibrary)); + CComPtr pBitcodeBlob; + VERIFY_SUCCEEDED(pLibrary->CreateBlobWithEncodingFromPinned( + bitcode.data(), static_cast(bitcode.size()), CP_ACP, + &pBitcodeBlob)); + + return pix_test::WrapInNewContainer(m_dllSupport, pBitcodeBlob); + } + + ValidationResult ValidateInstrumentedModule(IDxcBlob *pModule) { + CComPtr pContainer = NormalizeToContainer(pModule); + + ValidationResult direct = RunValidator(pContainer); + if (direct.Valid) { + return direct; + } + + // The validator's "unused metadata" diagnostic names the metadata + // node, not the kind, so text can't separate PIX's own annotations + // from any other unsupported metadata. Strip only the four known PIX + // kinds and revalidate; if that alone fixes it, PIX metadata was the + // sole cause. + CComPtr strippedContainer = + CloneModuleAndMutate(pContainer, StripKnownPixVirtualRegisterMetadata); + if (RunValidator(strippedContainer).Valid) { + return {true, {}}; + } + + return direct; + } - // Filters out boilerplate ("Validation failed.") and the permitted - // metadata exception: virtual-register annotation passes add metadata - // that DXIL does not consume, so the validator reports it as unused. Do - // not widen this filter. - FilteredValidationDiagnostics + // Joins diagnostic lines, skipping blanks and "Validation failed." + // boilerplate. + static std::string GetSignificantValidationDiagnostics(const std::string &errors) { - FilteredValidationDiagnostics result; + std::string result; std::stringstream errorStream(errors); std::string line; while (std::getline(errorStream, line)) { @@ -358,26 +419,14 @@ class PixTest : public ::testing::Test { if (line.empty() || line == "Validation failed.") { continue; } - if (line.find("All metadata must be used by dxil") != std::string::npos) { - result.PermittedExceptionCount++; - continue; - } - result.Significant.push_back(line); + result += line + "\n"; } return result; } - // True only if the diagnostics contain no significant errors and at - // least one instance of the permitted metadata exception. - bool IsPermittedValidationException( - const FilteredValidationDiagnostics &diagnostics) { - return diagnostics.Significant.empty() && - diagnostics.PermittedExceptionCount > 0; - } - - // Asserts an instrumented module validates. Accepts a module whose only - // diagnostic is the permitted metadata exception; logs and fails on any - // other validator error. + // Asserts an instrumented module validates, allowing for the four known + // PIX metadata kinds being unused; logs and fails on any other + // validator error. void VerifyInstrumentedModuleIsValid(IDxcBlob *pModule, const char *description) { ValidationResult validation = ValidateInstrumentedModule(pModule); @@ -385,23 +434,9 @@ class PixTest : public ::testing::Test { return; } - FilteredValidationDiagnostics diagnostics = - GetSignificantValidationDiagnostics(validation.Errors); - if (IsPermittedValidationException(diagnostics)) { - return; - } - - std::string joined; - if (diagnostics.Significant.empty()) { - joined = "(validator reported failure with no significant diagnostic " - "text, and no permitted metadata exception was found)"; - } else { - for (auto const &significantError : diagnostics.Significant) { - joined += significantError + "\n"; - } - } WEX::Logging::Log::Error(WEX::Common::String().Format( - L"Validation failed after %S:\n%S", description, joined.c_str())); + L"Validation failed after %S:\n%S", description, + GetSignificantValidationDiagnostics(validation.Errors).c_str())); VERIFY_FAIL(); } @@ -3626,7 +3661,8 @@ float main() : SV_Target })x"; // Virtual-register annotation adds metadata that DXIL does not consume, - // so this module only validates via the permitted metadata exception. + // so this module only validates because that metadata is one of the + // four known PIX kinds. auto compiled = Compile(m_dllSupport, source, L"ps_6_0", {L"-Od"}); auto output = RunSinglePass(compiled, L"-dxil-annotate-with-virtual-regs"); VerifyInstrumentedModuleIsValid( @@ -3642,20 +3678,11 @@ float main() : SV_Target return 0; })x"; - // Same shader and pass as Validation_ControlValidModulePasses; only the - // corruption below differs. auto compiled = Compile(m_dllSupport, source, L"ps_6_0", {L"-Od"}); auto output = RunSinglePass(compiled, L"-dxil-annotate-with-virtual-regs"); - // Confirm the baseline validates before corrupting it, so the failure - // below is caused by the corruption and nothing else. - VerifyInstrumentedModuleIsValid( - output.Module, - "virtual-register annotation of a trivial pixel shader, uncorrupted " - "baseline (validation harness control)"); - - // Mislabel the shader stage. The validator must reject this regardless - // of the permitted metadata exception. + // Mislabel the shader stage, so the container carries both the + // harness's permitted PIX metadata and a real defect. std::string disassembly = Disassemble(output.Module); const std::string shaderKindTag = "!\"ps\","; auto tagPosition = disassembly.find(shaderKindTag); @@ -3677,40 +3704,49 @@ float main() : SV_Target CComPtr pCorruptedContainer; VERIFY_SUCCEEDED(pAssembleResult->GetResult(&pCorruptedContainer)); + // Direct validation's own diagnostic proves the PIX metadata is present + // and otherwise unused, alongside rejecting for the mislabeled stage. + ValidationResult direct = RunValidator(pCorruptedContainer); + VERIFY_IS_FALSE(direct.Valid); + VERIFY_IS_TRUE(direct.Errors.find("All metadata must be used by dxil") != + std::string::npos); + + // The harness must still reject it, for a reason other than the + // permitted metadata. ValidationResult validation = ValidateInstrumentedModule(pCorruptedContainer); VERIFY_IS_FALSE(validation.Valid); + VERIFY_IS_FALSE( + GetSignificantValidationDiagnostics(validation.Errors).empty()); +} - // Confirm the corruption produces a real diagnostic, not just the - // permitted metadata exception. - FilteredValidationDiagnostics diagnostics = - GetSignificantValidationDiagnostics(validation.Errors); - VERIFY_IS_FALSE(diagnostics.Significant.empty()); -} - -// Tests that a validator failure is rejected unless its only diagnostic is -// the permitted metadata exception. A failure with only the "Validation -// failed." boilerplate and no exception must not pass. -TEST_F(PixTest, Validation_ControlBoilerplateOnlyFailureIsRejected) { - // Boilerplate only, no permitted exception: must be rejected. - FilteredValidationDiagnostics boilerplateOnly = - GetSignificantValidationDiagnostics("Validation failed.\n"); - VERIFY_IS_TRUE(boilerplateOnly.Significant.empty()); - VERIFY_ARE_EQUAL(boilerplateOnly.PermittedExceptionCount, 0); - VERIFY_IS_FALSE(IsPermittedValidationException(boilerplateOnly)); - - // Permitted exception present: must be accepted. - FilteredValidationDiagnostics exceptionOnly = - GetSignificantValidationDiagnostics( - "Validation failed.\n" - "All metadata must be used by dxil's users.\n"); - VERIFY_IS_TRUE(exceptionOnly.Significant.empty()); - VERIFY_IS_TRUE(exceptionOnly.PermittedExceptionCount > 0); - VERIFY_IS_TRUE(IsPermittedValidationException(exceptionOnly)); - - // Real diagnostic present: must be rejected, even with the exception. - FilteredValidationDiagnostics realDiagnostic = - GetSignificantValidationDiagnostics("Validation failed.\n" - "Some real validator diagnostic.\n"); - VERIFY_IS_FALSE(realDiagnostic.Significant.empty()); - VERIFY_IS_FALSE(IsPermittedValidationException(realDiagnostic)); +// A foreign, unused instruction metadata kind alongside the module's own +// permitted PIX metadata must still be rejected: stripping only the four +// known PIX kinds leaves it behind. +TEST_F(PixTest, Validation_ControlNonPixUnusedMetadataIsRejected) { + const char *source = R"x( +float main() : SV_Target +{ + return 0; +})x"; + + CComPtr compiled = Compile(m_dllSupport, source, L"ps_6_0", {L"-Od"}); + SinglePassOutput output = + RunSinglePass(compiled, L"-dxil-annotate-with-virtual-regs"); + CComPtr pContainer = NormalizeToContainer(output.Module); + + CComPtr withForeignMetadata = + CloneModuleAndMutate(pContainer, [](llvm::Module &M) { + for (llvm::Function &F : M) { + if (F.isDeclaration()) { + continue; + } + llvm::Instruction &I = *F.begin()->begin(); + I.setMetadata("not-a-pix-kind", + llvm::MDNode::get(M.getContext(), {})); + break; + } + }); + + ValidationResult validation = ValidateInstrumentedModule(withForeignMetadata); + VERIFY_IS_FALSE(validation.Valid); } From 6613667d74b5eadbefd08713e594dca5bc6cf02d Mon Sep 17 00:00:00 2001 From: Damyan Pepper Date: Wed, 2 Sep 2026 17:18:09 -0700 Subject: [PATCH 3/8] [PIX] Spell explicit types in L1 validation tests Apply the repository's almost-never-auto convention to every remaining auto introduced by the L1 original and direct-feedback commits. Use the declared Compile and RunSinglePass result types and std::string::size_type for the find result. No behavior changes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- tools/clang/unittests/HLSL/PixTest.cpp | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/tools/clang/unittests/HLSL/PixTest.cpp b/tools/clang/unittests/HLSL/PixTest.cpp index e6c803a55c..316aa91a09 100644 --- a/tools/clang/unittests/HLSL/PixTest.cpp +++ b/tools/clang/unittests/HLSL/PixTest.cpp @@ -3663,8 +3663,10 @@ float main() : SV_Target // Virtual-register annotation adds metadata that DXIL does not consume, // so this module only validates because that metadata is one of the // four known PIX kinds. - auto compiled = Compile(m_dllSupport, source, L"ps_6_0", {L"-Od"}); - auto output = RunSinglePass(compiled, L"-dxil-annotate-with-virtual-regs"); + CComPtr compiled = + Compile(m_dllSupport, source, L"ps_6_0", {L"-Od"}); + SinglePassOutput output = + RunSinglePass(compiled, L"-dxil-annotate-with-virtual-regs"); VerifyInstrumentedModuleIsValid( output.Module, "virtual-register annotation of a trivial pixel shader (validation " @@ -3678,14 +3680,16 @@ float main() : SV_Target return 0; })x"; - auto compiled = Compile(m_dllSupport, source, L"ps_6_0", {L"-Od"}); - auto output = RunSinglePass(compiled, L"-dxil-annotate-with-virtual-regs"); + CComPtr compiled = + Compile(m_dllSupport, source, L"ps_6_0", {L"-Od"}); + SinglePassOutput output = + RunSinglePass(compiled, L"-dxil-annotate-with-virtual-regs"); // Mislabel the shader stage, so the container carries both the // harness's permitted PIX metadata and a real defect. std::string disassembly = Disassemble(output.Module); const std::string shaderKindTag = "!\"ps\","; - auto tagPosition = disassembly.find(shaderKindTag); + std::string::size_type tagPosition = disassembly.find(shaderKindTag); VERIFY_IS_TRUE(tagPosition != std::string::npos); disassembly.replace(tagPosition, shaderKindTag.size(), "!\"vs\","); From fda0a597d1f3c1c9a0845de542cb2d8894f842b4 Mon Sep 17 00:00:00 2001 From: Damyan Pepper Date: Wed, 2 Sep 2026 18:19:28 -0700 Subject: [PATCH 4/8] [PIX] Remove metadata counts from comments Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- tools/clang/unittests/HLSL/PixTest.cpp | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/tools/clang/unittests/HLSL/PixTest.cpp b/tools/clang/unittests/HLSL/PixTest.cpp index 316aa91a09..3a046dc16c 100644 --- a/tools/clang/unittests/HLSL/PixTest.cpp +++ b/tools/clang/unittests/HLSL/PixTest.cpp @@ -334,15 +334,14 @@ class PixTest : public ::testing::Test { return {false, BlobToUtf8(pValidationErrors)}; } - // The four metadata kinds PIX's virtual-register annotation pass - // intentionally leaves unused for downstream tools to consume. See + // The metadata kinds that PIX's virtual-register annotation pass + // intentionally leaves unused for downstream tools. See // DxilPIXVirtualRegisters.h. static constexpr const char *KnownPixVirtualRegisterMetadataKinds[] = { pix_dxil::PixDxilInstNum::MDName, pix_dxil::PixDxilReg::MDName, pix_dxil::PixAllocaReg::MDName, pix_dxil::PixAllocaRegWrite::MDName}; - // Removes the four known PIX metadata kinds from every function and - // instruction. + // Removes the known PIX metadata kinds from every function and instruction. static void StripKnownPixVirtualRegisterMetadata(llvm::Module &M) { llvm::LLVMContext &Ctx = M.getContext(); for (const char *kind : KnownPixVirtualRegisterMetadataKinds) { @@ -393,9 +392,9 @@ class PixTest : public ::testing::Test { // The validator's "unused metadata" diagnostic names the metadata // node, not the kind, so text can't separate PIX's own annotations - // from any other unsupported metadata. Strip only the four known PIX - // kinds and revalidate; if that alone fixes it, PIX metadata was the - // sole cause. + // from any other unsupported metadata. Strip only the known PIX kinds + // and revalidate. If this fixes the module, PIX metadata was the only + // cause. CComPtr strippedContainer = CloneModuleAndMutate(pContainer, StripKnownPixVirtualRegisterMetadata); if (RunValidator(strippedContainer).Valid) { @@ -3723,9 +3722,8 @@ float main() : SV_Target GetSignificantValidationDiagnostics(validation.Errors).empty()); } -// A foreign, unused instruction metadata kind alongside the module's own -// permitted PIX metadata must still be rejected: stripping only the four -// known PIX kinds leaves it behind. +// A foreign, unused instruction metadata kind with the module's PIX metadata +// must still be rejected. Stripping the known PIX kinds leaves it behind. TEST_F(PixTest, Validation_ControlNonPixUnusedMetadataIsRejected) { const char *source = R"x( float main() : SV_Target From f80826f89481efaf520cf32c36dc2c1174ab3ba4 Mon Sep 17 00:00:00 2001 From: Damyan Pepper Date: Wed, 2 Sep 2026 18:35:23 -0700 Subject: [PATCH 5/8] [PIX] Format L1 validation changes Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- tools/clang/unittests/HLSL/PixTest.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tools/clang/unittests/HLSL/PixTest.cpp b/tools/clang/unittests/HLSL/PixTest.cpp index 3a046dc16c..39856eae77 100644 --- a/tools/clang/unittests/HLSL/PixTest.cpp +++ b/tools/clang/unittests/HLSL/PixTest.cpp @@ -361,7 +361,7 @@ class PixTest : public ::testing::Test { // and re-serializes into a fresh validator-ready container. template CComPtr CloneModuleAndMutate(IDxcBlob *pContainer, - MutatorFn Mutate) { + MutatorFn Mutate) { ModuleAndHangersOn moduleEtc(pContainer); llvm::Module *M = moduleEtc.GetDxilModule().GetModule(); Mutate(*M); @@ -3712,7 +3712,7 @@ float main() : SV_Target ValidationResult direct = RunValidator(pCorruptedContainer); VERIFY_IS_FALSE(direct.Valid); VERIFY_IS_TRUE(direct.Errors.find("All metadata must be used by dxil") != - std::string::npos); + std::string::npos); // The harness must still reject it, for a reason other than the // permitted metadata. @@ -3731,7 +3731,8 @@ float main() : SV_Target return 0; })x"; - CComPtr compiled = Compile(m_dllSupport, source, L"ps_6_0", {L"-Od"}); + CComPtr compiled = + Compile(m_dllSupport, source, L"ps_6_0", {L"-Od"}); SinglePassOutput output = RunSinglePass(compiled, L"-dxil-annotate-with-virtual-regs"); CComPtr pContainer = NormalizeToContainer(output.Module); From e6e17f3c52823f1a45fe2e08330e874cbfe7bcb5 Mon Sep 17 00:00:00 2001 From: Damyan Pepper Date: Thu, 3 Sep 2026 09:32:05 -0700 Subject: [PATCH 6/8] [PIX] Follow LLVM naming conventions Use Capitalized names for the parameters and local variables added by the PIX validation harness. Remove Hungarian prefixes without changing older code in PixTest.cpp. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 819bcd16-83fc-4619-ac1a-b12ee50a2fe8 --- tools/clang/unittests/HLSL/PixTest.cpp | 249 ++++++++++++------------- 1 file changed, 123 insertions(+), 126 deletions(-) diff --git a/tools/clang/unittests/HLSL/PixTest.cpp b/tools/clang/unittests/HLSL/PixTest.cpp index 39856eae77..bdc50e8671 100644 --- a/tools/clang/unittests/HLSL/PixTest.cpp +++ b/tools/clang/unittests/HLSL/PixTest.cpp @@ -276,23 +276,23 @@ class PixTest : public ::testing::Test { std::vector Lines; }; - SinglePassOutput RunSinglePass(IDxcBlob *dxil, LPCWSTR passOption) { - CComPtr pOptimizer; + SinglePassOutput RunSinglePass(IDxcBlob *Dxil, LPCWSTR PassOption) { + CComPtr Optimizer; VERIFY_SUCCEEDED( - m_dllSupport.CreateInstance(CLSID_DxcOptimizer, &pOptimizer)); + m_dllSupport.CreateInstance(CLSID_DxcOptimizer, &Optimizer)); std::vector Options; Options.push_back(L"-opt-mod-passes"); - Options.push_back(passOption); + Options.push_back(PassOption); - CComPtr pOptimizedModule; - CComPtr pText; - VERIFY_SUCCEEDED(pOptimizer->RunOptimizer( - dxil, Options.data(), Options.size(), &pOptimizedModule, &pText)); + CComPtr OptimizedModule; + CComPtr Text; + VERIFY_SUCCEEDED(Optimizer->RunOptimizer( + Dxil, Options.data(), Options.size(), &OptimizedModule, &Text)); - SinglePassOutput ret; - ret.Module = pOptimizedModule; - ret.Lines = Tokenize(BlobToUtf8(pText).c_str(), "\n"); - return ret; + SinglePassOutput Result; + Result.Module = OptimizedModule; + Result.Lines = Tokenize(BlobToUtf8(Text).c_str(), "\n"); + return Result; } // PIX does not validate the shaders its passes instrument, so a pass @@ -306,32 +306,32 @@ class PixTest : public ::testing::Test { // The validator (and the assembler, when reconstructing a container // from bare bitcode) both require a container; some pass runners // return bare bitcode instead. - CComPtr NormalizeToContainer(IDxcBlob *pModule) { - if (hlsl::IsDxilContainerLike(pModule->GetBufferPointer(), - pModule->GetBufferSize()) != nullptr) { - return pModule; + CComPtr NormalizeToContainer(IDxcBlob *Module) { + if (hlsl::IsDxilContainerLike(Module->GetBufferPointer(), + Module->GetBufferSize()) != nullptr) { + return Module; } - return pix_test::WrapInNewContainer(m_dllSupport, pModule); + return pix_test::WrapInNewContainer(m_dllSupport, Module); } - ValidationResult RunValidator(IDxcBlob *pContainer) { - CComPtr pValidator; + ValidationResult RunValidator(IDxcBlob *Container) { + CComPtr Validator; VERIFY_SUCCEEDED( - m_dllSupport.CreateInstance(CLSID_DxcValidator, &pValidator)); + m_dllSupport.CreateInstance(CLSID_DxcValidator, &Validator)); - CComPtr pValidationResult; - VERIFY_SUCCEEDED(pValidator->Validate(pContainer, DxcValidatorFlags_Default, - &pValidationResult)); + CComPtr OperationResult; + VERIFY_SUCCEEDED(Validator->Validate(Container, DxcValidatorFlags_Default, + &OperationResult)); - HRESULT validationStatus; - VERIFY_SUCCEEDED(pValidationResult->GetStatus(&validationStatus)); - if (SUCCEEDED(validationStatus)) { + HRESULT ValidationStatus; + VERIFY_SUCCEEDED(OperationResult->GetStatus(&ValidationStatus)); + if (SUCCEEDED(ValidationStatus)) { return {true, {}}; } - CComPtr pValidationErrors; - VERIFY_SUCCEEDED(pValidationResult->GetErrorBuffer(&pValidationErrors)); - return {false, BlobToUtf8(pValidationErrors)}; + CComPtr ValidationErrors; + VERIFY_SUCCEEDED(OperationResult->GetErrorBuffer(&ValidationErrors)); + return {false, BlobToUtf8(ValidationErrors)}; } // The metadata kinds that PIX's virtual-register annotation pass @@ -344,50 +344,50 @@ class PixTest : public ::testing::Test { // Removes the known PIX metadata kinds from every function and instruction. static void StripKnownPixVirtualRegisterMetadata(llvm::Module &M) { llvm::LLVMContext &Ctx = M.getContext(); - for (const char *kind : KnownPixVirtualRegisterMetadataKinds) { - unsigned kindID = Ctx.getMDKindID(kind); + for (const char *Kind : KnownPixVirtualRegisterMetadataKinds) { + unsigned KindID = Ctx.getMDKindID(Kind); for (llvm::Function &F : M) { - F.setMetadata(kindID, nullptr); + F.setMetadata(KindID, nullptr); for (llvm::BasicBlock &BB : F) { for (llvm::Instruction &I : BB) { - I.setMetadata(kindID, nullptr); + I.setMetadata(KindID, nullptr); } } } } } - // Parses pContainer into an isolated LLVM module, applies Mutate to it, + // Parses Container into an isolated LLVM module, applies Mutate to it, // and re-serializes into a fresh validator-ready container. template - CComPtr CloneModuleAndMutate(IDxcBlob *pContainer, + CComPtr CloneModuleAndMutate(IDxcBlob *Container, MutatorFn Mutate) { - ModuleAndHangersOn moduleEtc(pContainer); - llvm::Module *M = moduleEtc.GetDxilModule().GetModule(); + ModuleAndHangersOn ModuleEtc(Container); + llvm::Module *M = ModuleEtc.GetDxilModule().GetModule(); Mutate(*M); - llvm::SmallVector bitcode; + llvm::SmallVector Bitcode; { - llvm::raw_svector_ostream OS(bitcode); + llvm::raw_svector_ostream OS(Bitcode); llvm::WriteBitcodeToFile(M, OS); } - CComPtr pLibrary; - VERIFY_SUCCEEDED(m_dllSupport.CreateInstance(CLSID_DxcLibrary, &pLibrary)); - CComPtr pBitcodeBlob; - VERIFY_SUCCEEDED(pLibrary->CreateBlobWithEncodingFromPinned( - bitcode.data(), static_cast(bitcode.size()), CP_ACP, - &pBitcodeBlob)); + CComPtr Library; + VERIFY_SUCCEEDED(m_dllSupport.CreateInstance(CLSID_DxcLibrary, &Library)); + CComPtr BitcodeBlob; + VERIFY_SUCCEEDED(Library->CreateBlobWithEncodingFromPinned( + Bitcode.data(), static_cast(Bitcode.size()), CP_ACP, + &BitcodeBlob)); - return pix_test::WrapInNewContainer(m_dllSupport, pBitcodeBlob); + return pix_test::WrapInNewContainer(m_dllSupport, BitcodeBlob); } - ValidationResult ValidateInstrumentedModule(IDxcBlob *pModule) { - CComPtr pContainer = NormalizeToContainer(pModule); + ValidationResult ValidateInstrumentedModule(IDxcBlob *Module) { + CComPtr Container = NormalizeToContainer(Module); - ValidationResult direct = RunValidator(pContainer); - if (direct.Valid) { - return direct; + ValidationResult DirectResult = RunValidator(Container); + if (DirectResult.Valid) { + return DirectResult; } // The validator's "unused metadata" diagnostic names the metadata @@ -395,47 +395,46 @@ class PixTest : public ::testing::Test { // from any other unsupported metadata. Strip only the known PIX kinds // and revalidate. If this fixes the module, PIX metadata was the only // cause. - CComPtr strippedContainer = - CloneModuleAndMutate(pContainer, StripKnownPixVirtualRegisterMetadata); - if (RunValidator(strippedContainer).Valid) { + CComPtr StrippedContainer = + CloneModuleAndMutate(Container, StripKnownPixVirtualRegisterMetadata); + if (RunValidator(StrippedContainer).Valid) { return {true, {}}; } - return direct; + return DirectResult; } // Joins diagnostic lines, skipping blanks and "Validation failed." // boilerplate. static std::string - GetSignificantValidationDiagnostics(const std::string &errors) { - std::string result; - std::stringstream errorStream(errors); - std::string line; - while (std::getline(errorStream, line)) { - if (!line.empty() && line.back() == '\r') { - line.pop_back(); + GetSignificantValidationDiagnostics(const std::string &Errors) { + std::string Result; + std::stringstream ErrorStream(Errors); + std::string Line; + while (std::getline(ErrorStream, Line)) { + if (!Line.empty() && Line.back() == '\r') { + Line.pop_back(); } - if (line.empty() || line == "Validation failed.") { + if (Line.empty() || Line == "Validation failed.") { continue; } - result += line + "\n"; + Result += Line + "\n"; } - return result; + return Result; } - // Asserts an instrumented module validates, allowing for the four known - // PIX metadata kinds being unused; logs and fails on any other - // validator error. - void VerifyInstrumentedModuleIsValid(IDxcBlob *pModule, - const char *description) { - ValidationResult validation = ValidateInstrumentedModule(pModule); - if (validation.Valid) { + // Asserts that an instrumented module is valid when known PIX metadata is + // unused. Logs and fails on any other validator error. + void VerifyInstrumentedModuleIsValid(IDxcBlob *Module, + const char *Description) { + ValidationResult Result = ValidateInstrumentedModule(Module); + if (Result.Valid) { return; } WEX::Logging::Log::Error(WEX::Common::String().Format( - L"Validation failed after %S:\n%S", description, - GetSignificantValidationDiagnostics(validation.Errors).c_str())); + L"Validation failed after %S:\n%S", Description, + GetSignificantValidationDiagnostics(Result.Errors).c_str())); VERIFY_FAIL(); } @@ -3648,97 +3647,95 @@ void main(uint3 tid : SV_DispatchThreadID) { // Control tests for the PIX pass validation harness // (ValidateInstrumentedModule / VerifyInstrumentedModuleIsValid). // -// Both tests instrument the same trivial pixel shader with the -// virtual-register annotation pass, so the valid and invalid cases are -// directly comparable. +// These tests use the same trivial pixel shader and virtual-register +// annotation pass. This keeps the valid and invalid cases comparable. TEST_F(PixTest, Validation_ControlValidModulePasses) { - const char *source = R"x( + const char *Source = R"x( float main() : SV_Target { return 0; })x"; // Virtual-register annotation adds metadata that DXIL does not consume, - // so this module only validates because that metadata is one of the - // four known PIX kinds. - CComPtr compiled = - Compile(m_dllSupport, source, L"ps_6_0", {L"-Od"}); - SinglePassOutput output = - RunSinglePass(compiled, L"-dxil-annotate-with-virtual-regs"); + // so this module validates only because the metadata kind is known to PIX. + CComPtr Compiled = + Compile(m_dllSupport, Source, L"ps_6_0", {L"-Od"}); + SinglePassOutput Output = + RunSinglePass(Compiled, L"-dxil-annotate-with-virtual-regs"); VerifyInstrumentedModuleIsValid( - output.Module, + Output.Module, "virtual-register annotation of a trivial pixel shader (validation " "harness control)"); } TEST_F(PixTest, Validation_ControlInvalidModuleFails) { - const char *source = R"x( + const char *Source = R"x( float main() : SV_Target { return 0; })x"; - CComPtr compiled = - Compile(m_dllSupport, source, L"ps_6_0", {L"-Od"}); - SinglePassOutput output = - RunSinglePass(compiled, L"-dxil-annotate-with-virtual-regs"); + CComPtr Compiled = + Compile(m_dllSupport, Source, L"ps_6_0", {L"-Od"}); + SinglePassOutput Output = + RunSinglePass(Compiled, L"-dxil-annotate-with-virtual-regs"); // Mislabel the shader stage, so the container carries both the // harness's permitted PIX metadata and a real defect. - std::string disassembly = Disassemble(output.Module); - const std::string shaderKindTag = "!\"ps\","; - std::string::size_type tagPosition = disassembly.find(shaderKindTag); - VERIFY_IS_TRUE(tagPosition != std::string::npos); - disassembly.replace(tagPosition, shaderKindTag.size(), "!\"vs\","); - - CComPtr pDisassemblyBlob; - CreateBlobFromText(m_dllSupport, disassembly.c_str(), &pDisassemblyBlob); - - CComPtr pAssembler; + std::string Disassembly = Disassemble(Output.Module); + const std::string ShaderKindTag = "!\"ps\","; + std::string::size_type TagPosition = Disassembly.find(ShaderKindTag); + VERIFY_IS_TRUE(TagPosition != std::string::npos); + Disassembly.replace(TagPosition, ShaderKindTag.size(), "!\"vs\","); + + CComPtr DisassemblyBlob; + CreateBlobFromText(m_dllSupport, Disassembly.c_str(), &DisassemblyBlob); + + CComPtr Assembler; + VERIFY_SUCCEEDED(m_dllSupport.CreateInstance(CLSID_DxcAssembler, &Assembler)); + CComPtr AssembleResult; VERIFY_SUCCEEDED( - m_dllSupport.CreateInstance(CLSID_DxcAssembler, &pAssembler)); - CComPtr pAssembleResult; - VERIFY_SUCCEEDED( - pAssembler->AssembleToContainer(pDisassemblyBlob, &pAssembleResult)); - HRESULT assembleStatus; - VERIFY_SUCCEEDED(pAssembleResult->GetStatus(&assembleStatus)); - VERIFY_SUCCEEDED(assembleStatus); - CComPtr pCorruptedContainer; - VERIFY_SUCCEEDED(pAssembleResult->GetResult(&pCorruptedContainer)); + Assembler->AssembleToContainer(DisassemblyBlob, &AssembleResult)); + HRESULT AssembleStatus; + VERIFY_SUCCEEDED(AssembleResult->GetStatus(&AssembleStatus)); + VERIFY_SUCCEEDED(AssembleStatus); + CComPtr CorruptedContainer; + VERIFY_SUCCEEDED(AssembleResult->GetResult(&CorruptedContainer)); // Direct validation's own diagnostic proves the PIX metadata is present // and otherwise unused, alongside rejecting for the mislabeled stage. - ValidationResult direct = RunValidator(pCorruptedContainer); - VERIFY_IS_FALSE(direct.Valid); - VERIFY_IS_TRUE(direct.Errors.find("All metadata must be used by dxil") != - std::string::npos); + ValidationResult DirectResult = RunValidator(CorruptedContainer); + VERIFY_IS_FALSE(DirectResult.Valid); + VERIFY_IS_TRUE(DirectResult.Errors.find( + "All metadata must be used by dxil") != std::string::npos); // The harness must still reject it, for a reason other than the // permitted metadata. - ValidationResult validation = ValidateInstrumentedModule(pCorruptedContainer); - VERIFY_IS_FALSE(validation.Valid); + ValidationResult HarnessResult = + ValidateInstrumentedModule(CorruptedContainer); + VERIFY_IS_FALSE(HarnessResult.Valid); VERIFY_IS_FALSE( - GetSignificantValidationDiagnostics(validation.Errors).empty()); + GetSignificantValidationDiagnostics(HarnessResult.Errors).empty()); } // A foreign, unused instruction metadata kind with the module's PIX metadata // must still be rejected. Stripping the known PIX kinds leaves it behind. TEST_F(PixTest, Validation_ControlNonPixUnusedMetadataIsRejected) { - const char *source = R"x( + const char *Source = R"x( float main() : SV_Target { return 0; })x"; - CComPtr compiled = - Compile(m_dllSupport, source, L"ps_6_0", {L"-Od"}); - SinglePassOutput output = - RunSinglePass(compiled, L"-dxil-annotate-with-virtual-regs"); - CComPtr pContainer = NormalizeToContainer(output.Module); + CComPtr Compiled = + Compile(m_dllSupport, Source, L"ps_6_0", {L"-Od"}); + SinglePassOutput Output = + RunSinglePass(Compiled, L"-dxil-annotate-with-virtual-regs"); + CComPtr Container = NormalizeToContainer(Output.Module); - CComPtr withForeignMetadata = - CloneModuleAndMutate(pContainer, [](llvm::Module &M) { + CComPtr WithForeignMetadata = + CloneModuleAndMutate(Container, [](llvm::Module &M) { for (llvm::Function &F : M) { if (F.isDeclaration()) { continue; @@ -3750,6 +3747,6 @@ float main() : SV_Target } }); - ValidationResult validation = ValidateInstrumentedModule(withForeignMetadata); - VERIFY_IS_FALSE(validation.Valid); + ValidationResult Result = ValidateInstrumentedModule(WithForeignMetadata); + VERIFY_IS_FALSE(Result.Valid); } From ca7cc662960c664c784eeb6a9b4e26713ebd57fe Mon Sep 17 00:00:00 2001 From: Damyan Pepper Date: Thu, 3 Sep 2026 10:23:37 -0700 Subject: [PATCH 7/8] [PIX] Use lower camel case for helper functions Rename the PIX validation helpers to verb phrases that start with a lowercase letter, as required by the LLVM coding standards. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 819bcd16-83fc-4619-ac1a-b12ee50a2fe8 --- tools/clang/unittests/HLSL/PixTest.cpp | 52 +++++++++++++------------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/tools/clang/unittests/HLSL/PixTest.cpp b/tools/clang/unittests/HLSL/PixTest.cpp index bdc50e8671..440d961288 100644 --- a/tools/clang/unittests/HLSL/PixTest.cpp +++ b/tools/clang/unittests/HLSL/PixTest.cpp @@ -161,7 +161,7 @@ class PixTest : public ::testing::Test { TEST_METHOD(NonUniformResourceIndex_Raytracing) // Control tests for the PIX pass validation harness below - // (ValidateInstrumentedModule / VerifyInstrumentedModuleIsValid). + // (validateInstrumentedModule / verifyInstrumentedModuleIsValid). TEST_METHOD(Validation_ControlValidModulePasses) TEST_METHOD(Validation_ControlInvalidModuleFails) TEST_METHOD(Validation_ControlNonPixUnusedMetadataIsRejected) @@ -276,7 +276,7 @@ class PixTest : public ::testing::Test { std::vector Lines; }; - SinglePassOutput RunSinglePass(IDxcBlob *Dxil, LPCWSTR PassOption) { + SinglePassOutput runSinglePass(IDxcBlob *Dxil, LPCWSTR PassOption) { CComPtr Optimizer; VERIFY_SUCCEEDED( m_dllSupport.CreateInstance(CLSID_DxcOptimizer, &Optimizer)); @@ -306,7 +306,7 @@ class PixTest : public ::testing::Test { // The validator (and the assembler, when reconstructing a container // from bare bitcode) both require a container; some pass runners // return bare bitcode instead. - CComPtr NormalizeToContainer(IDxcBlob *Module) { + CComPtr normalizeToContainer(IDxcBlob *Module) { if (hlsl::IsDxilContainerLike(Module->GetBufferPointer(), Module->GetBufferSize()) != nullptr) { return Module; @@ -314,7 +314,7 @@ class PixTest : public ::testing::Test { return pix_test::WrapInNewContainer(m_dllSupport, Module); } - ValidationResult RunValidator(IDxcBlob *Container) { + ValidationResult runValidator(IDxcBlob *Container) { CComPtr Validator; VERIFY_SUCCEEDED( m_dllSupport.CreateInstance(CLSID_DxcValidator, &Validator)); @@ -342,7 +342,7 @@ class PixTest : public ::testing::Test { pix_dxil::PixAllocaReg::MDName, pix_dxil::PixAllocaRegWrite::MDName}; // Removes the known PIX metadata kinds from every function and instruction. - static void StripKnownPixVirtualRegisterMetadata(llvm::Module &M) { + static void stripKnownPixVirtualRegisterMetadata(llvm::Module &M) { llvm::LLVMContext &Ctx = M.getContext(); for (const char *Kind : KnownPixVirtualRegisterMetadataKinds) { unsigned KindID = Ctx.getMDKindID(Kind); @@ -360,7 +360,7 @@ class PixTest : public ::testing::Test { // Parses Container into an isolated LLVM module, applies Mutate to it, // and re-serializes into a fresh validator-ready container. template - CComPtr CloneModuleAndMutate(IDxcBlob *Container, + CComPtr cloneModuleAndMutate(IDxcBlob *Container, MutatorFn Mutate) { ModuleAndHangersOn ModuleEtc(Container); llvm::Module *M = ModuleEtc.GetDxilModule().GetModule(); @@ -382,10 +382,10 @@ class PixTest : public ::testing::Test { return pix_test::WrapInNewContainer(m_dllSupport, BitcodeBlob); } - ValidationResult ValidateInstrumentedModule(IDxcBlob *Module) { - CComPtr Container = NormalizeToContainer(Module); + ValidationResult validateInstrumentedModule(IDxcBlob *Module) { + CComPtr Container = normalizeToContainer(Module); - ValidationResult DirectResult = RunValidator(Container); + ValidationResult DirectResult = runValidator(Container); if (DirectResult.Valid) { return DirectResult; } @@ -396,8 +396,8 @@ class PixTest : public ::testing::Test { // and revalidate. If this fixes the module, PIX metadata was the only // cause. CComPtr StrippedContainer = - CloneModuleAndMutate(Container, StripKnownPixVirtualRegisterMetadata); - if (RunValidator(StrippedContainer).Valid) { + cloneModuleAndMutate(Container, stripKnownPixVirtualRegisterMetadata); + if (runValidator(StrippedContainer).Valid) { return {true, {}}; } @@ -407,7 +407,7 @@ class PixTest : public ::testing::Test { // Joins diagnostic lines, skipping blanks and "Validation failed." // boilerplate. static std::string - GetSignificantValidationDiagnostics(const std::string &Errors) { + getSignificantValidationDiagnostics(const std::string &Errors) { std::string Result; std::stringstream ErrorStream(Errors); std::string Line; @@ -425,16 +425,16 @@ class PixTest : public ::testing::Test { // Asserts that an instrumented module is valid when known PIX metadata is // unused. Logs and fails on any other validator error. - void VerifyInstrumentedModuleIsValid(IDxcBlob *Module, + void verifyInstrumentedModuleIsValid(IDxcBlob *Module, const char *Description) { - ValidationResult Result = ValidateInstrumentedModule(Module); + ValidationResult Result = validateInstrumentedModule(Module); if (Result.Valid) { return; } WEX::Logging::Log::Error(WEX::Common::String().Format( L"Validation failed after %S:\n%S", Description, - GetSignificantValidationDiagnostics(Result.Errors).c_str())); + getSignificantValidationDiagnostics(Result.Errors).c_str())); VERIFY_FAIL(); } @@ -3645,7 +3645,7 @@ void main(uint3 tid : SV_DispatchThreadID) { /////////////////////////////////////////////////////////////////////////////// // Control tests for the PIX pass validation harness -// (ValidateInstrumentedModule / VerifyInstrumentedModuleIsValid). +// (validateInstrumentedModule / verifyInstrumentedModuleIsValid). // // These tests use the same trivial pixel shader and virtual-register // annotation pass. This keeps the valid and invalid cases comparable. @@ -3662,8 +3662,8 @@ float main() : SV_Target CComPtr Compiled = Compile(m_dllSupport, Source, L"ps_6_0", {L"-Od"}); SinglePassOutput Output = - RunSinglePass(Compiled, L"-dxil-annotate-with-virtual-regs"); - VerifyInstrumentedModuleIsValid( + runSinglePass(Compiled, L"-dxil-annotate-with-virtual-regs"); + verifyInstrumentedModuleIsValid( Output.Module, "virtual-register annotation of a trivial pixel shader (validation " "harness control)"); @@ -3679,7 +3679,7 @@ float main() : SV_Target CComPtr Compiled = Compile(m_dllSupport, Source, L"ps_6_0", {L"-Od"}); SinglePassOutput Output = - RunSinglePass(Compiled, L"-dxil-annotate-with-virtual-regs"); + runSinglePass(Compiled, L"-dxil-annotate-with-virtual-regs"); // Mislabel the shader stage, so the container carries both the // harness's permitted PIX metadata and a real defect. @@ -3705,7 +3705,7 @@ float main() : SV_Target // Direct validation's own diagnostic proves the PIX metadata is present // and otherwise unused, alongside rejecting for the mislabeled stage. - ValidationResult DirectResult = RunValidator(CorruptedContainer); + ValidationResult DirectResult = runValidator(CorruptedContainer); VERIFY_IS_FALSE(DirectResult.Valid); VERIFY_IS_TRUE(DirectResult.Errors.find( "All metadata must be used by dxil") != std::string::npos); @@ -3713,10 +3713,10 @@ float main() : SV_Target // The harness must still reject it, for a reason other than the // permitted metadata. ValidationResult HarnessResult = - ValidateInstrumentedModule(CorruptedContainer); + validateInstrumentedModule(CorruptedContainer); VERIFY_IS_FALSE(HarnessResult.Valid); VERIFY_IS_FALSE( - GetSignificantValidationDiagnostics(HarnessResult.Errors).empty()); + getSignificantValidationDiagnostics(HarnessResult.Errors).empty()); } // A foreign, unused instruction metadata kind with the module's PIX metadata @@ -3731,11 +3731,11 @@ float main() : SV_Target CComPtr Compiled = Compile(m_dllSupport, Source, L"ps_6_0", {L"-Od"}); SinglePassOutput Output = - RunSinglePass(Compiled, L"-dxil-annotate-with-virtual-regs"); - CComPtr Container = NormalizeToContainer(Output.Module); + runSinglePass(Compiled, L"-dxil-annotate-with-virtual-regs"); + CComPtr Container = normalizeToContainer(Output.Module); CComPtr WithForeignMetadata = - CloneModuleAndMutate(Container, [](llvm::Module &M) { + cloneModuleAndMutate(Container, [](llvm::Module &M) { for (llvm::Function &F : M) { if (F.isDeclaration()) { continue; @@ -3747,6 +3747,6 @@ float main() : SV_Target } }); - ValidationResult Result = ValidateInstrumentedModule(WithForeignMetadata); + ValidationResult Result = validateInstrumentedModule(WithForeignMetadata); VERIFY_IS_FALSE(Result.Valid); } From a1fd01162f92fb18f42841c5ab5862e5607c7108 Mon Sep 17 00:00:00 2001 From: Damyan Pepper Date: Thu, 3 Sep 2026 11:44:42 -0700 Subject: [PATCH 8/8] [PIX] Validate known metadata before stripping Reject known PIX metadata with an invalid attachment location or payload before removing it for DXIL validation. Add controls for function metadata, metadata on the wrong instruction type, and malformed metadata. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 819bcd16-83fc-4619-ac1a-b12ee50a2fe8 --- tools/clang/unittests/HLSL/PixTest.cpp | 172 +++++++++++++++++++++++-- 1 file changed, 162 insertions(+), 10 deletions(-) diff --git a/tools/clang/unittests/HLSL/PixTest.cpp b/tools/clang/unittests/HLSL/PixTest.cpp index 440d961288..9d9131de5d 100644 --- a/tools/clang/unittests/HLSL/PixTest.cpp +++ b/tools/clang/unittests/HLSL/PixTest.cpp @@ -50,6 +50,7 @@ #include "llvm/ADT/SmallString.h" #include "llvm/ADT/StringSwitch.h" #include "llvm/Bitcode/ReaderWriter.h" +#include "llvm/IR/Constants.h" #include "llvm/IR/DebugInfo.h" #include "llvm/IR/Instructions.h" #include "llvm/IR/IntrinsicInst.h" @@ -165,6 +166,7 @@ class PixTest : public ::testing::Test { TEST_METHOD(Validation_ControlValidModulePasses) TEST_METHOD(Validation_ControlInvalidModuleFails) TEST_METHOD(Validation_ControlNonPixUnusedMetadataIsRejected) + TEST_METHOD(Validation_ControlInvalidPixMetadataIsRejected) dxc::DxCompilerDllLoader m_dllSupport; VersionSupportInfo m_ver; @@ -341,16 +343,83 @@ class PixTest : public ::testing::Test { pix_dxil::PixDxilInstNum::MDName, pix_dxil::PixDxilReg::MDName, pix_dxil::PixAllocaReg::MDName, pix_dxil::PixAllocaRegWrite::MDName}; - // Removes the known PIX metadata kinds from every function and instruction. + // Checks that known PIX metadata has the expected location and payload. + static bool hasValidKnownPixVirtualRegisterMetadata(llvm::Module &M) { + llvm::LLVMContext &Ctx = M.getContext(); + unsigned InstNumKindID = Ctx.getMDKindID(pix_dxil::PixDxilInstNum::MDName); + unsigned RegKindID = Ctx.getMDKindID(pix_dxil::PixDxilReg::MDName); + unsigned AllocaRegKindID = Ctx.getMDKindID(pix_dxil::PixAllocaReg::MDName); + unsigned AllocaRegWriteKindID = + Ctx.getMDKindID(pix_dxil::PixAllocaRegWrite::MDName); + + for (llvm::Function &F : M) { + for (const char *Kind : KnownPixVirtualRegisterMetadataKinds) { + if (F.getMetadata(Ctx.getMDKindID(Kind)) != nullptr) { + return false; + } + } + + for (llvm::BasicBlock &BB : F) { + for (llvm::Instruction &I : BB) { + if (I.getMetadata(InstNumKindID) != nullptr) { + std::uint32_t InstNum; + if (!pix_dxil::PixDxilInstNum::FromInst(&I, &InstNum)) { + return false; + } + } + + if (I.getMetadata(RegKindID) != nullptr) { + std::uint32_t RegNum; + if (!pix_dxil::PixDxilReg::FromInst(&I, &RegNum)) { + return false; + } + } + + if (I.getMetadata(AllocaRegKindID) != nullptr) { + llvm::AllocaInst *Alloca = llvm::dyn_cast(&I); + std::uint32_t RegBase; + std::uint32_t RegSize; + if (Alloca == nullptr || + !pix_dxil::PixAllocaReg::FromInst(Alloca, &RegBase, &RegSize)) { + return false; + } + } + + if (I.getMetadata(AllocaRegWriteKindID) != nullptr) { + llvm::StoreInst *Store = llvm::dyn_cast(&I); + std::uint32_t RegBase; + std::uint32_t RegSize; + llvm::Value *Index; + if (Store == nullptr || !pix_dxil::PixAllocaRegWrite::FromInst( + Store, &RegBase, &RegSize, &Index)) { + return false; + } + } + } + } + } + return true; + } + + // Removes valid PIX metadata from its documented instruction types. static void stripKnownPixVirtualRegisterMetadata(llvm::Module &M) { llvm::LLVMContext &Ctx = M.getContext(); - for (const char *Kind : KnownPixVirtualRegisterMetadataKinds) { - unsigned KindID = Ctx.getMDKindID(Kind); - for (llvm::Function &F : M) { - F.setMetadata(KindID, nullptr); - for (llvm::BasicBlock &BB : F) { - for (llvm::Instruction &I : BB) { - I.setMetadata(KindID, nullptr); + unsigned InstNumKindID = Ctx.getMDKindID(pix_dxil::PixDxilInstNum::MDName); + unsigned RegKindID = Ctx.getMDKindID(pix_dxil::PixDxilReg::MDName); + unsigned AllocaRegKindID = Ctx.getMDKindID(pix_dxil::PixAllocaReg::MDName); + unsigned AllocaRegWriteKindID = + Ctx.getMDKindID(pix_dxil::PixAllocaRegWrite::MDName); + + for (llvm::Function &F : M) { + for (llvm::BasicBlock &BB : F) { + for (llvm::Instruction &I : BB) { + I.setMetadata(InstNumKindID, nullptr); + I.setMetadata(RegKindID, nullptr); + if (llvm::isa(&I)) { + I.setMetadata(AllocaRegKindID, nullptr); + } + if (llvm::isa(&I)) { + I.setMetadata(AllocaRegWriteKindID, nullptr); } } } @@ -395,8 +464,17 @@ class PixTest : public ::testing::Test { // from any other unsupported metadata. Strip only the known PIX kinds // and revalidate. If this fixes the module, PIX metadata was the only // cause. - CComPtr StrippedContainer = - cloneModuleAndMutate(Container, stripKnownPixVirtualRegisterMetadata); + bool KnownPixMetadataIsValid = false; + CComPtr StrippedContainer = cloneModuleAndMutate( + Container, [&KnownPixMetadataIsValid](llvm::Module &M) { + KnownPixMetadataIsValid = hasValidKnownPixVirtualRegisterMetadata(M); + if (KnownPixMetadataIsValid) { + stripKnownPixVirtualRegisterMetadata(M); + } + }); + if (!KnownPixMetadataIsValid) { + return DirectResult; + } if (runValidator(StrippedContainer).Valid) { return {true, {}}; } @@ -3750,3 +3828,77 @@ float main() : SV_Target ValidationResult Result = validateInstrumentedModule(WithForeignMetadata); VERIFY_IS_FALSE(Result.Valid); } + +TEST_F(PixTest, Validation_ControlInvalidPixMetadataIsRejected) { + const char *Source = R"x( +float main() : SV_Target +{ + return 0; +})x"; + + CComPtr Compiled = + Compile(m_dllSupport, Source, L"ps_6_0", {L"-Od"}); + SinglePassOutput Output = + runSinglePass(Compiled, L"-dxil-annotate-with-virtual-regs"); + CComPtr Container = normalizeToContainer(Output.Module); + + bool AddedFunctionMetadata = false; + CComPtr WithFunctionMetadata = cloneModuleAndMutate( + Container, [&AddedFunctionMetadata](llvm::Module &M) { + for (llvm::Function &F : M) { + if (F.isDeclaration()) { + continue; + } + F.setMetadata(pix_dxil::PixDxilInstNum::MDName, + llvm::MDNode::get(M.getContext(), {})); + AddedFunctionMetadata = true; + break; + } + }); + VERIFY_IS_TRUE(AddedFunctionMetadata); + VERIFY_IS_FALSE(validateInstrumentedModule(WithFunctionMetadata).Valid); + + bool AddedMisplacedMetadata = false; + CComPtr WithMisplacedMetadata = cloneModuleAndMutate( + Container, [&AddedMisplacedMetadata](llvm::Module &M) { + llvm::LLVMContext &Ctx = M.getContext(); + llvm::Type *Int32Ty = llvm::Type::getInt32Ty(Ctx); + llvm::MDNode *ValidAllocaReg = llvm::MDNode::get( + Ctx, + {llvm::ConstantAsMetadata::get( + llvm::ConstantInt::get(Int32Ty, pix_dxil::PixAllocaReg::ID)), + llvm::ConstantAsMetadata::get(llvm::ConstantInt::get(Int32Ty, 0)), + llvm::ConstantAsMetadata::get( + llvm::ConstantInt::get(Int32Ty, 1))}); + for (llvm::Function &F : M) { + for (llvm::BasicBlock &BB : F) { + for (llvm::Instruction &I : BB) { + if (!llvm::isa(&I)) { + I.setMetadata(pix_dxil::PixAllocaReg::MDName, ValidAllocaReg); + AddedMisplacedMetadata = true; + return; + } + } + } + } + }); + VERIFY_IS_TRUE(AddedMisplacedMetadata); + VERIFY_IS_FALSE(validateInstrumentedModule(WithMisplacedMetadata).Valid); + + bool AddedMalformedMetadata = false; + CComPtr WithMalformedMetadata = cloneModuleAndMutate( + Container, [&AddedMalformedMetadata](llvm::Module &M) { + for (llvm::Function &F : M) { + for (llvm::BasicBlock &BB : F) { + for (llvm::Instruction &I : BB) { + I.setMetadata(pix_dxil::PixDxilInstNum::MDName, + llvm::MDNode::get(M.getContext(), {})); + AddedMalformedMetadata = true; + return; + } + } + } + }); + VERIFY_IS_TRUE(AddedMalformedMetadata); + VERIFY_IS_FALSE(validateInstrumentedModule(WithMalformedMetadata).Valid); +}