From 819f345efb8137180f2f86ff6d025a764215ee81 Mon Sep 17 00:00:00 2001 From: Joshua Batista Date: Fri, 10 Jan 2025 15:21:02 -0800 Subject: [PATCH 1/8] fix error handling --- lib/DxilContainer/DxcContainerBuilder.cpp | 25 +++++++++-------------- tools/clang/tools/dxclib/dxc.cpp | 2 +- 2 files changed, 11 insertions(+), 16 deletions(-) diff --git a/lib/DxilContainer/DxcContainerBuilder.cpp b/lib/DxilContainer/DxcContainerBuilder.cpp index 3c10b0e70a..43768ba857 100644 --- a/lib/DxilContainer/DxcContainerBuilder.cpp +++ b/lib/DxilContainer/DxcContainerBuilder.cpp @@ -105,6 +105,9 @@ HRESULT STDMETHODCALLTYPE DxcContainerBuilder::RemovePart(UINT32 fourCC) { HRESULT STDMETHODCALLTYPE DxcContainerBuilder::SerializeContainer(IDxcOperationResult **ppResult) { DxcThreadMalloc TM(m_pMalloc); + if (ppResult == nullptr || *ppResult == nullptr) + return E_INVALIDARG; + try { // Allocate memory for new dxil container. uint32_t ContainerSize = ComputeContainerSize(); @@ -161,6 +164,13 @@ DxcContainerBuilder::SerializeContainer(IDxcOperationResult **ppResult) { errorHeap.Detach(); } + // Add Hash. + LPVOID PTR = pResult->GetBufferPointer(); + if (!IsDxilContainerLike(PTR, pResult->GetBufferSize())) + return E_FAIL; + + HashAndUpdate((DxilContainerHeader *)PTR); + IFT(DxcResult::Create( valHR, DXC_OUT_OBJECT, {DxcOutputObject::DataOutput(DXC_OUT_OBJECT, pResult, DxcOutNoName), @@ -169,21 +179,6 @@ DxcContainerBuilder::SerializeContainer(IDxcOperationResult **ppResult) { } CATCH_CPP_RETURN_HRESULT(); - if (ppResult == nullptr || *ppResult == nullptr) - return S_OK; - - HRESULT HR; - (*ppResult)->GetStatus(&HR); - if (FAILED(HR)) - return HR; - - CComPtr pObject; - IFR((*ppResult)->GetResult(&pObject)); - - // Add Hash. - LPVOID PTR = pObject->GetBufferPointer(); - if (IsDxilContainerLike(PTR, pObject->GetBufferSize())) - HashAndUpdate((DxilContainerHeader *)PTR); return S_OK; } diff --git a/tools/clang/tools/dxclib/dxc.cpp b/tools/clang/tools/dxclib/dxc.cpp index 1bcf5d8e3f..cdcfe2b3f6 100644 --- a/tools/clang/tools/dxclib/dxc.cpp +++ b/tools/clang/tools/dxclib/dxc.cpp @@ -644,7 +644,7 @@ int DxcContext::VerifyRootSignature() { IFT(pContainerBuilder->AddPart(hlsl::DxilFourCC::DFCC_RootSignature, pRootSignature)); CComPtr pOperationResult; - pContainerBuilder->SerializeContainer(&pOperationResult); + IFT(pContainerBuilder->SerializeContainer(&pOperationResult)); HRESULT status = E_FAIL; CComPtr pResult; IFT(pOperationResult->GetStatus(&status)); From 7dc1d0d905768f1b23c389321ee4d010d2a5abc6 Mon Sep 17 00:00:00 2001 From: Joshua Batista Date: Fri, 10 Jan 2025 16:50:48 -0800 Subject: [PATCH 2/8] remove indirection check --- lib/DxilContainer/DxcContainerBuilder.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/DxilContainer/DxcContainerBuilder.cpp b/lib/DxilContainer/DxcContainerBuilder.cpp index 43768ba857..20de70a3ea 100644 --- a/lib/DxilContainer/DxcContainerBuilder.cpp +++ b/lib/DxilContainer/DxcContainerBuilder.cpp @@ -105,7 +105,7 @@ HRESULT STDMETHODCALLTYPE DxcContainerBuilder::RemovePart(UINT32 fourCC) { HRESULT STDMETHODCALLTYPE DxcContainerBuilder::SerializeContainer(IDxcOperationResult **ppResult) { DxcThreadMalloc TM(m_pMalloc); - if (ppResult == nullptr || *ppResult == nullptr) + if (ppResult == nullptr) return E_INVALIDARG; try { From 3c4b56658cc2613d542bc3f0cc9e4f7e33c74e1c Mon Sep 17 00:00:00 2001 From: Joshua Batista Date: Mon, 13 Jan 2025 11:12:37 -0800 Subject: [PATCH 3/8] address feedback, no tests yet --- lib/DxilContainer/DxcContainerBuilder.cpp | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/lib/DxilContainer/DxcContainerBuilder.cpp b/lib/DxilContainer/DxcContainerBuilder.cpp index 20de70a3ea..770aa910a4 100644 --- a/lib/DxilContainer/DxcContainerBuilder.cpp +++ b/lib/DxilContainer/DxcContainerBuilder.cpp @@ -104,10 +104,11 @@ HRESULT STDMETHODCALLTYPE DxcContainerBuilder::RemovePart(UINT32 fourCC) { HRESULT STDMETHODCALLTYPE DxcContainerBuilder::SerializeContainer(IDxcOperationResult **ppResult) { - DxcThreadMalloc TM(m_pMalloc); if (ppResult == nullptr) return E_INVALIDARG; + DxcThreadMalloc TM(m_pMalloc); + try { // Allocate memory for new dxil container. uint32_t ContainerSize = ComputeContainerSize(); @@ -165,11 +166,9 @@ DxcContainerBuilder::SerializeContainer(IDxcOperationResult **ppResult) { } // Add Hash. - LPVOID PTR = pResult->GetBufferPointer(); - if (!IsDxilContainerLike(PTR, pResult->GetBufferSize())) - return E_FAIL; - - HashAndUpdate((DxilContainerHeader *)PTR); + if (SUCCEEDED(valHR)) + HashAndUpdate(IsDxilContainerLike(pResult->GetBufferPointer(), + pResult->GetBufferSize())); IFT(DxcResult::Create( valHR, DXC_OUT_OBJECT, From 0370c176b30f930b248be606394e04e46911f6b4 Mon Sep 17 00:00:00 2001 From: Joshua Batista Date: Thu, 16 Jan 2025 15:47:34 -0800 Subject: [PATCH 4/8] add test no hash when validation failure --- tools/clang/unittests/HLSL/ValidationTest.cpp | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/tools/clang/unittests/HLSL/ValidationTest.cpp b/tools/clang/unittests/HLSL/ValidationTest.cpp index 08f67f35d0..3e18df7b38 100644 --- a/tools/clang/unittests/HLSL/ValidationTest.cpp +++ b/tools/clang/unittests/HLSL/ValidationTest.cpp @@ -205,6 +205,7 @@ class ValidationTest : public ::testing::Test { TEST_METHOD(SimpleGs1Fail) TEST_METHOD(UavBarrierFail) TEST_METHOD(UndefValueFail) + TEST_METHOD(ValidationFailNoHash) TEST_METHOD(UpdateCounterFail) TEST_METHOD(LocalResCopy) TEST_METHOD(ResCounter) @@ -1178,6 +1179,57 @@ TEST_F(ValidationTest, UavBarrierFail) { TEST_F(ValidationTest, UndefValueFail) { TestCheck(L"..\\CodeGenHLSL\\UndefValue.hlsl"); } +// verify that containers that are not valid DXIL do not +// get assigned a hash. +TEST_F(ValidationTest, ValidationFailNoHash) { + if (m_ver.SkipDxilVersion(1, 8)) + return; + CComPtr pProgram; + LPCSTR pSource = "float main(snorm float b : B) : SV_DEPTH \ + { \ + float a; \ + return b + a; \ + }"; + + CComPtr pSourceBlob; + Utf8ToBlob(m_dllSupport, pSource, &pSourceBlob); + std::vector pArguments = {L"-Vd"}; + LPCSTR pShaderModel = "ps_6_0"; + bool result = CompileSource(pSourceBlob, pShaderModel, pArguments.data(), 1, + nullptr, 0, &pProgram); + + VERIFY_IS_TRUE(result); + + CComPtr pValidator; + CComPtr pResult; + unsigned Flags = 0; + VERIFY_SUCCEEDED( + m_dllSupport.CreateInstance(CLSID_DxcValidator, &pValidator)); + + VERIFY_SUCCEEDED(pValidator->Validate(pProgram, Flags, &pResult)); + HRESULT status; + VERIFY_IS_NOT_NULL(pResult); + CComPtr pValidationOutput; + pResult->GetStatus(&status); + + // expect validation to fail + VERIFY_FAILED(status); + pResult->GetResult(&pValidationOutput); + // Make sure the validation output is not null when hashing. + VERIFY_SUCCEEDED(pValidationOutput != nullptr); + + hlsl::DxilContainerHeader *pHeader = + (hlsl::DxilContainerHeader *)pProgram->GetBufferPointer(); + // Validate the hash. + constexpr uint32_t HashStartOffset = + offsetof(struct DxilContainerHeader, Version); + auto *DataToHash = (const BYTE *)pHeader + HashStartOffset; + UINT AmountToHash = pHeader->ContainerSizeInBytes - HashStartOffset; + BYTE Result[DxilContainerHashSize]; + ComputeHashRetail(DataToHash, AmountToHash, Result); + // Should be unequal, this proves the hash isn't written when validation fails + VERIFY_ARE_NOT_EQUAL(memcmp(Result, pHeader->Hash.Digest, sizeof(Result)), 0); +} TEST_F(ValidationTest, UpdateCounterFail) { if (m_ver.SkipIRSensitiveTest()) return; From 4bc8379ba5885ffa0e00536f8dba0709e3a745e3 Mon Sep 17 00:00:00 2001 From: Joshua Batista Date: Thu, 16 Jan 2025 17:58:52 -0800 Subject: [PATCH 5/8] address Tex --- tools/clang/unittests/HLSL/ValidationTest.cpp | 31 ++++++++++--------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/tools/clang/unittests/HLSL/ValidationTest.cpp b/tools/clang/unittests/HLSL/ValidationTest.cpp index 3e18df7b38..1627d392cb 100644 --- a/tools/clang/unittests/HLSL/ValidationTest.cpp +++ b/tools/clang/unittests/HLSL/ValidationTest.cpp @@ -1185,11 +1185,15 @@ TEST_F(ValidationTest, ValidationFailNoHash) { if (m_ver.SkipDxilVersion(1, 8)) return; CComPtr pProgram; - LPCSTR pSource = "float main(snorm float b : B) : SV_DEPTH \ - { \ - float a; \ - return b + a; \ - }"; + + // we need any shader that will pass compilation but fail validation + LPCSTR pSource = R"( + float main(snorm float b : B) : SV_DEPTH + { + float a; + return b + a; + } +)"; CComPtr pSourceBlob; Utf8ToBlob(m_dllSupport, pSource, &pSourceBlob); @@ -1215,20 +1219,17 @@ TEST_F(ValidationTest, ValidationFailNoHash) { // expect validation to fail VERIFY_FAILED(status); pResult->GetResult(&pValidationOutput); - // Make sure the validation output is not null when hashing. + // Make sure the validation output is not null even when validation fails VERIFY_SUCCEEDED(pValidationOutput != nullptr); hlsl::DxilContainerHeader *pHeader = - (hlsl::DxilContainerHeader *)pProgram->GetBufferPointer(); - // Validate the hash. - constexpr uint32_t HashStartOffset = - offsetof(struct DxilContainerHeader, Version); - auto *DataToHash = (const BYTE *)pHeader + HashStartOffset; - UINT AmountToHash = pHeader->ContainerSizeInBytes - HashStartOffset; - BYTE Result[DxilContainerHashSize]; - ComputeHashRetail(DataToHash, AmountToHash, Result); + IsDxilContainerLike(pProgram->GetBufferPointer(), pProgram->GetBufferSize()); + VERIFY_IS_NOT_NULL(pHeader); + + BYTE Result[DxilContainerHashSize] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}; + // ComputeHashRetail(DataToHash, AmountToHash, Result); // Should be unequal, this proves the hash isn't written when validation fails - VERIFY_ARE_NOT_EQUAL(memcmp(Result, pHeader->Hash.Digest, sizeof(Result)), 0); + VERIFY_ARE_EQUAL(memcmp(Result, pHeader->Hash.Digest, sizeof(Result)), 0); } TEST_F(ValidationTest, UpdateCounterFail) { if (m_ver.SkipIRSensitiveTest()) From efea0a08aef67d02a95806305212485971ca6eff Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Fri, 17 Jan 2025 02:12:25 +0000 Subject: [PATCH 6/8] chore: autopublish 2025-01-17T02:12:25Z --- tools/clang/unittests/HLSL/ValidationTest.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tools/clang/unittests/HLSL/ValidationTest.cpp b/tools/clang/unittests/HLSL/ValidationTest.cpp index 1627d392cb..6cac08c27a 100644 --- a/tools/clang/unittests/HLSL/ValidationTest.cpp +++ b/tools/clang/unittests/HLSL/ValidationTest.cpp @@ -1222,11 +1222,12 @@ TEST_F(ValidationTest, ValidationFailNoHash) { // Make sure the validation output is not null even when validation fails VERIFY_SUCCEEDED(pValidationOutput != nullptr); - hlsl::DxilContainerHeader *pHeader = - IsDxilContainerLike(pProgram->GetBufferPointer(), pProgram->GetBufferSize()); + hlsl::DxilContainerHeader *pHeader = IsDxilContainerLike( + pProgram->GetBufferPointer(), pProgram->GetBufferSize()); VERIFY_IS_NOT_NULL(pHeader); - BYTE Result[DxilContainerHashSize] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}; + BYTE Result[DxilContainerHashSize] = {0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0}; // ComputeHashRetail(DataToHash, AmountToHash, Result); // Should be unequal, this proves the hash isn't written when validation fails VERIFY_ARE_EQUAL(memcmp(Result, pHeader->Hash.Digest, sizeof(Result)), 0); From 9194c5300d03c3163779108d8d27c86dce6afd4b Mon Sep 17 00:00:00 2001 From: Joshua Batista Date: Fri, 17 Jan 2025 17:42:29 -0800 Subject: [PATCH 7/8] address Tex --- tools/clang/unittests/HLSL/ValidationTest.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/tools/clang/unittests/HLSL/ValidationTest.cpp b/tools/clang/unittests/HLSL/ValidationTest.cpp index 6cac08c27a..cb2a5ce282 100644 --- a/tools/clang/unittests/HLSL/ValidationTest.cpp +++ b/tools/clang/unittests/HLSL/ValidationTest.cpp @@ -1186,7 +1186,8 @@ TEST_F(ValidationTest, ValidationFailNoHash) { return; CComPtr pProgram; - // we need any shader that will pass compilation but fail validation + // We need any shader that will pass compilation but fail validation. + // This shader reads from uninitialized 'float a', which works for now. LPCSTR pSource = R"( float main(snorm float b : B) : SV_DEPTH { @@ -1226,11 +1227,11 @@ TEST_F(ValidationTest, ValidationFailNoHash) { pProgram->GetBufferPointer(), pProgram->GetBufferSize()); VERIFY_IS_NOT_NULL(pHeader); - BYTE Result[DxilContainerHashSize] = {0, 0, 0, 0, 0, 0, 0, 0, + BYTE ZeroHash[DxilContainerHashSize] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}; - // ComputeHashRetail(DataToHash, AmountToHash, Result); - // Should be unequal, this proves the hash isn't written when validation fails - VERIFY_ARE_EQUAL(memcmp(Result, pHeader->Hash.Digest, sizeof(Result)), 0); + + // Should be equal, this proves the hash isn't written when validation fails + VERIFY_ARE_EQUAL(memcmp(ZeroHash, pHeader->Hash.Digest, sizeof(ZeroHash)), 0); } TEST_F(ValidationTest, UpdateCounterFail) { if (m_ver.SkipIRSensitiveTest()) From 2461915577daee62ccda4117e940d5e556b3fe56 Mon Sep 17 00:00:00 2001 From: Joshua Batista Date: Tue, 21 Jan 2025 10:22:16 -0800 Subject: [PATCH 8/8] clang format --- tools/clang/unittests/HLSL/ValidationTest.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/clang/unittests/HLSL/ValidationTest.cpp b/tools/clang/unittests/HLSL/ValidationTest.cpp index cb2a5ce282..e7d3bfaebd 100644 --- a/tools/clang/unittests/HLSL/ValidationTest.cpp +++ b/tools/clang/unittests/HLSL/ValidationTest.cpp @@ -1228,7 +1228,7 @@ TEST_F(ValidationTest, ValidationFailNoHash) { VERIFY_IS_NOT_NULL(pHeader); BYTE ZeroHash[DxilContainerHashSize] = {0, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0}; + 0, 0, 0, 0, 0, 0, 0, 0}; // Should be equal, this proves the hash isn't written when validation fails VERIFY_ARE_EQUAL(memcmp(ZeroHash, pHeader->Hash.Digest, sizeof(ZeroHash)), 0);