Add a test dxil lib for testing loadability #5952

pow2clk · 2023-10-30T17:20:57Z

This creates a dxil library that doesn't sign anything, but does perform the validation very much as the internal validator does. This is used to test that the compiler and validator objects can properly locate, load, and make use of the external dxil library.

The DxilLib test verifies both that loading the compiler with the external dxil library available will load that library by default, resulting in compiles without the usual warning when it is not found and also that explicitly loading the library can be done and the correct API calls can be used.

Fixes #5394

This creates a dxil library that doesn't sign anything, but does perform the validation very much as the internal validator does. This is used to test that the compiler and validator objects can properly locate, load, and make use of the external dxil library. The DxilLib test verifies both that loading the compiler with the external dxil library available will load that library by default, resulting in compiles without the usual warning when it is not found and also that explicitly loading the library can be done and the correct API calls can be used. Fixes microsoft#5394

tools/testdxildll/DxilValidator.cpp

Co-authored-by: Xiang Li <python3kgae@outlook.com>

github-actions · 2023-10-31T18:35:35Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff ceff9b8043df3ac8d7d3ce71b409d83afcd7925b 63d46cd46bfa8ae170e9bc53d93dfe4fb6f55141 -- include/dxc/HLSL/DxilValidator.h lib/HLSL/DxilValidator.cpp tools/no-sig-dxillib/DxilContainerBuilder.cpp tools/no-sig-dxillib/DxilLib.cpp tools/no-sig-dxillib/DxilLibValidator.cpp unittests/DXILLib/DxilLibTest.cpp tools/clang/tools/dxclib/dxc.cpp tools/clang/tools/dxcompiler/dxcapi.cpp tools/clang/tools/dxcompiler/dxcvalidator.cpp

View the diff from clang-format here.

diff --git a/lib/HLSL/DxilValidator.cpp b/lib/HLSL/DxilValidator.cpp
index 5230ec11a..58c5d1442 100644
--- a/lib/HLSL/DxilValidator.cpp
+++ b/lib/HLSL/DxilValidator.cpp
@@ -21,8 +21,8 @@
 #include "dxc/DxilRootSignature/DxilRootSignature.h"
 #include "dxc/Support/FileIOHelper.h"
 #include "dxc/Support/Global.h"
-#include "dxc/dxcapi.h"
 #include "dxc/Support/dxcapi.impl.h"
+#include "dxc/dxcapi.h"
 #include "llvm/Support/MemoryBuffer.h"
 
 #ifdef _WIN32
@@ -219,7 +219,7 @@ HRESULT DxilValidator::RunValidation(
 
 HRESULT
 DxilValidator::RunRootSignatureValidation(IDxcBlob *pShader,
-                                         AbstractMemoryStream *pDiagStream) {
+                                          AbstractMemoryStream *pDiagStream) {
 
   const DxilContainerHeader *pDxilContainer = IsDxilContainerLike(
       pShader->GetBufferPointer(), pShader->GetBufferSize());
@@ -275,4 +275,3 @@ HRESULT RunInternalValidator(IDxcValidator *pValidator, llvm::Module *pModule,
   return pInternalValidator->ValidateWithOptModules(pShader, Flags, pModule,
                                                     pDebugModule, ppResult);
 }
-
diff --git a/tools/clang/tools/dxcompiler/dxcvalidator.cpp b/tools/clang/tools/dxcompiler/dxcvalidator.cpp
index ba68aa70c..5a11a96b4 100644
--- a/tools/clang/tools/dxcompiler/dxcvalidator.cpp
+++ b/tools/clang/tools/dxcompiler/dxcvalidator.cpp
@@ -50,15 +50,15 @@ private:
 public:
   DXC_MICROCOM_TM_ADDREF_RELEASE_IMPL()
   DXC_MICROCOM_TM_ALLOC(DxcValidator)
-  DxcValidator(IMalloc *pMalloc) : DxilValidator(pMalloc), m_dwRef(0), m_pMalloc(pMalloc) {}
+  DxcValidator(IMalloc *pMalloc)
+      : DxilValidator(pMalloc), m_dwRef(0), m_pMalloc(pMalloc) {}
 
   HRESULT STDMETHODCALLTYPE QueryInterface(REFIID iid,
                                            void **ppvObject) override {
-    return DoBasicQueryInterface<IDxcValidator, IDxcValidator2, IDxcVersionInfo>(this, iid,
-                                                                ppvObject);
+    return DoBasicQueryInterface<IDxcValidator, IDxcValidator2,
+                                 IDxcVersionInfo>(this, iid, ppvObject);
   }
 
-
   // IDxcVersionInfo
   HRESULT STDMETHODCALLTYPE GetVersion(UINT32 *pMajor, UINT32 *pMinor) override;
   HRESULT STDMETHODCALLTYPE GetFlags(UINT32 *pFlags) override;
diff --git a/tools/no-sig-dxillib/DxilLibValidator.cpp b/tools/no-sig-dxillib/DxilLibValidator.cpp
index b2636ff36..1028f0ea6 100644
--- a/tools/no-sig-dxillib/DxilLibValidator.cpp
+++ b/tools/no-sig-dxillib/DxilLibValidator.cpp
@@ -10,34 +10,34 @@
 //                                                                           //
 ///////////////////////////////////////////////////////////////////////////////
 
-#include "dxc/HLSL/DxilValidator.h"
 #include "dxc/HLSL/DxilValidation.h"
+#include "dxc/HLSL/DxilValidator.h"
 #include "dxc/Support/FileIOHelper.h"
 #include "dxc/Support/dxcapi.impl.h"
 
-class DxilLibValidator : public DxilValidator, public IDxcVersionInfo
-{
+class DxilLibValidator : public DxilValidator, public IDxcVersionInfo {
 private:
   DXC_MICROCOM_TM_REF_FIELDS()
 
 public:
   DXC_MICROCOM_TM_ADDREF_RELEASE_IMPL()
-  DxilLibValidator(IMalloc *pMalloc) : DxilValidator(pMalloc), m_dwRef(0), m_pMalloc(pMalloc) {}
+  DxilLibValidator(IMalloc *pMalloc)
+      : DxilValidator(pMalloc), m_dwRef(0), m_pMalloc(pMalloc) {}
   DXC_MICROCOM_TM_ALLOC(DxilLibValidator)
 
   HRESULT STDMETHODCALLTYPE QueryInterface(REFIID iid,
                                            void **ppvObject) override {
-    return DoBasicQueryInterface<IDxcValidator, IDxcValidator2, IDxcVersionInfo>(this, iid, ppvObject);
+    return DoBasicQueryInterface<IDxcValidator, IDxcValidator2,
+                                 IDxcVersionInfo>(this, iid, ppvObject);
   }
 
   // IDxcVersionInfo
   HRESULT STDMETHODCALLTYPE GetVersion(UINT32 *pMajor, UINT32 *pMinor) override;
   HRESULT STDMETHODCALLTYPE GetFlags(UINT32 *pFlags) override;
-
 };
 
 HRESULT STDMETHODCALLTYPE DxilLibValidator::GetVersion(UINT32 *pMajor,
-                                                   UINT32 *pMinor) {
+                                                       UINT32 *pMinor) {
   if (pMajor == nullptr || pMinor == nullptr)
     return E_INVALIDARG;
   hlsl::GetValidationVersion(pMajor, pMinor);
diff --git a/unittests/DXILLib/DxilLibTest.cpp b/unittests/DXILLib/DxilLibTest.cpp
index c369c085c..1011899a7 100644
--- a/unittests/DXILLib/DxilLibTest.cpp
+++ b/unittests/DXILLib/DxilLibTest.cpp
@@ -101,8 +101,8 @@ TEST(DxilLibTest, LoadFromCompiler) {
 
   if (errBuf)
     EXPECT_EQ(errBuf->GetBufferSize(), 0U)
-      << "Unexpected diagnostics found in compiler error buffer: "
-      << (const char *)errBuf->GetBufferPointer();
+        << "Unexpected diagnostics found in compiler error buffer: "
+        << (const char *)errBuf->GetBufferPointer();
 }
 
 // Check that dxil lib can be loaded and trivially used by a validator object
@@ -140,7 +140,9 @@ TEST(DxilLibTest, LoadFromValidator) {
   EXPECT_TRUE(SUCCEEDED(status1));
   ASSERT_TRUE(SUCCEEDED(validationResult1->GetErrorBuffer(&errBuf1)));
   if (errBuf1)
-    EXPECT_EQ(errBuf1->GetBufferSize(), 0U) << "Unexpected diagnostics found in validator1 error buffer: " << (const char *)errBuf1->GetBufferPointer();
+    EXPECT_EQ(errBuf1->GetBufferSize(), 0U)
+        << "Unexpected diagnostics found in validator1 error buffer: "
+        << (const char *)errBuf1->GetBufferPointer();
 
   CComPtr<IDxcOperationResult> validationResult2;
   HRESULT status2 = S_OK;
@@ -151,5 +153,7 @@ TEST(DxilLibTest, LoadFromValidator) {
   EXPECT_TRUE(SUCCEEDED(status2));
   ASSERT_TRUE(SUCCEEDED(validationResult2->GetErrorBuffer(&errBuf2)));
   if (errBuf2)
-    EXPECT_EQ(errBuf2->GetBufferSize(), 0U) << "Unexpected diagnostics found in validator2 error buffer: " << (const char *)errBuf2->GetBufferPointer();
+    EXPECT_EQ(errBuf2->GetBufferSize(), 0U)
+        << "Unexpected diagnostics found in validator2 error buffer: "
+        << (const char *)errBuf2->GetBufferPointer();
 }

Check this box to apply formatting changes to this branch.

unittests/DXILLib/DxilLibTest.cpp

python3kgae

Do we need a test case for validation fail?

pow2clk · 2023-11-02T15:33:30Z

The intent of this is not to test validation really, but just that the library is loadable and usable. We have a lot of tests for validation failures. Originally, my intent was to use the complete suite of validation tests on the loadable dxil lib, but was convinced that was overkill

tex3d · 2023-11-03T20:51:42Z

The intent of this is not to test validation really, but just that the library is loadable and usable. We have a lot of tests for validation failures. Originally, my intent was to use the complete suite of validation tests on the loadable dxil lib, but was convinced that was overkill

Using this library to validate more content is actually useful because it takes a different validation path than the internal validator. This validation path can and has been broken multiple times before while internal validation path did not catch the problem. This is because the internal validation path uses the main and debug llvm modules from in-memory form without going through serialization and deserialization. Something that doesn't get preserved exactly the same way through serialization/deserialization can cause a failure with the external validation path that isn't caught by the internal path.

In short, I think we should run all content against this external validation path when possible. We don't even need to run content twice to do this, we can simply use this as the available external validator when running tests in GitHub pipelines.

hekota · 2023-11-03T21:14:06Z

tools/testdxildll/DxilContainerBuilder.cpp

+// License. See LICENSE.TXT for details.                                     //
+//                                                                           //
+// Implements the Dxil Container Builder                                     //
+//                                                                           //


The comment is incorrect, it does not really implement a container builder. Any reason why this is not part of the DxilLib.cpp?

Good catch. The reason it's not part of DxilLib.cpp is because of some inclusions this requires that confuse the code in DxilLib.cpp. I spent a little while trying to resolve them, but decided it wasn't worth it as this is the structure of similar code elsewhere likely for the same reason.

tex3d

Mainly: I think we might be able to move dxcvalidator.cpp into shared lib instead of reimplementing most of it here.

tex3d · 2023-11-03T21:39:15Z

tools/testdxildll/DxilContainerBuilder.cpp

+using namespace hlsl;
+
+HRESULT CreateDxcContainerBuilder(_In_ REFIID riid, _Out_ LPVOID *ppv) {
+  // Call dxil.dll's containerbuilder


Is this comment accurate? Isn't this creating the local container builder instead?

tex3d · 2023-11-03T21:47:27Z

tools/testdxildll/DxilValidator.cpp

+using namespace llvm;
+using namespace hlsl;
+
+class DxcValidator : public IDxcValidator2, public IDxcVersionInfo {


Wait, why can't we move DxcValidator.cpp out of dxcompiler project into shared lib, then you can simply use the same one instead reimplementing so much here?

tex3d · 2023-11-03T21:48:24Z

tools/testdxildll/DxilContainerBuilder.cpp

+
+using namespace hlsl;
+
+HRESULT CreateDxcContainerBuilder(_In_ REFIID riid, _Out_ LPVOID *ppv) {


Couldn't this be implemented in DxilLib.cpp, since we really aren't supplying another implementation here? One might almost mix it up with DxcContainerBuilder.cpp where we implement the internal builder that this code uses.

There were some compilation errors when I tried to do this before that had to do with conflicting defines. I'll take another run at it.

…iler into dxiltest

hekota · 2023-11-09T18:48:12Z

tools/testdxildll/DxilLib.cpp

@@ -0,0 +1,150 @@
+///////////////////////////////////////////////////////////////////////////////


Would you mind renaming DxilLib.* files to TestDxilLib.*? I am often searching through the whole code base and since this code is similar to the actual DxilLib.cpp it would be nice to have it under a different name so it can be clearly seen in the search results that this is the test dll.

dashes match how directories in the same place are named. no-sig is a clearer prefix because it describes function instead of intent and makes it less likely to mistake it for a test utility. dxillib is platform neutral unline dxildll

No changes, just to make reviewing the actual changes easier since the DxilValidator parent class is mostly derived from dxcvalidator with some removals

This consolidates the validation part of the validator while separating the version queries since dxcompiler gets a different version of versioninfo than dxil lib does. DxilValidator.cpp was previously a direct copy of dxcvalidator.cpp. This strips the versioninfo from DxilValidator and strips the Validation from dxcvalidator as it now gets that functionality from the parent class

Update comment in dxc.cpp that misrepresented what legacy FXC flags are supported and which ones are planned for the future. The warning passed into DxcContainerBuilder is unassigned in the default case. It is assumed to be null if unneeded, but that's not guaranteed unless it is initialized properly.

This enables us to use it as the default unsigning validator

Remove line numbers from a number of tests that, lacking the internal validator, will not have access to line numbers anymore Take more care with default values and possibly null pointers in DxilLibTest Correctly set the out directory for the test lib copying

tex3d · 2023-11-27T19:39:29Z

include/dxc/HLSL/DxilValidator.h

+class AbstractMemoryStream;
+}
+
+class DxilValidator : public IDxcValidator2 {


Could we rename DxilValidator to DxcValidatorBase?

It's already a little confusing to have DxilValidation and DxcValidator, but it's a pattern that is used elsewhere, with core implementation in Dxil* and Dxc* being the implementation of the IDxc* interface.

Adding DxilValidator confuses things more, while I think DxcValidatorBase makes the purpose of being a base class for the IDxcValidator implementation clearer.

Added a NoSig prefix to all files and classes to clarify that these are not the main dxil lib

tex3d · 2023-11-28T20:56:54Z

tools/no-sig-dxillib/DxilLibValidator.cpp

+HRESULT STDMETHODCALLTYPE DxilLibValidator::GetFlags(UINT32 *pFlags) {
+  if (pFlags == nullptr)
+    return E_INVALIDARG;
+  *pFlags = DxcVersionInfoFlags_None;


One issue I recently thought of is that the lack of DxcVersionInfoFlags_Internal would normally be used to detect that you have a signing validator. This makes the result from GetFlags on this dll potentially confusing, right? Maybe we could have a DxcVersionInfoFlags_Test flag for the test dll, so at least we can tell this is the non-production external test validator. What do you think?

DxcVersionInfoFlags_Internal can also be used to determine whether we should expect the line-only debug info supplied to the validator when the validator is called automatically from Compile().

tex3d · 2023-11-28T23:32:35Z

tools/clang/test/CodeGenHLSL/mesh-val/asOversizePayload.hlsl

@@ -2,7 +2,7 @@
 // RUN: %dxilver 1.6 | %dxc -E main -T as_6_5 %s | FileCheck %s -check-prefix=CHK_NODB

 // CHK_DB: 23:5: error: For amplification shader with entry 'main', payload size 16400 is greater than maximum size of 16384 bytes.
-// CHK_NODB: 23:5: error: For amplification shader with entry 'main', payload size 16400 is greater than maximum size of 16384 bytes.
+// CHK_NODB: error: For amplification shader with entry 'main', payload size 16400 is greater than maximum size of 16384 bytes.


This change shouldn't have been necessary. IDxcValidator2::ValidateWithDebug allows you to provide the serialized debug module, which is normally provided to the external validator when present. This must work with the normal DXIL.dll because these don't normally fail when testing against that. So perhaps there's something wrong with the new test validator?

Renamed DxilValidator to DxcValidatorBase to prevent confusion between DxilValdation and DxilValidator. Renamed dxil lib back to testdxil because testing dxil.dll broadly is going to require more care to be done right.

correctly identify dependency of LLVMHLSL on DxilRootSignature. The dependency was pre-existing, but never caused any problems because the order of linking just happened to come out right for the Linux linker Secondly, a mistaken direct usage of declspec was left in from testing instead of using the define

pow2clk · 2023-11-30T20:54:02Z

Ready for review again! I recommend looking at the changes to the DxcValidatorBase using this sequence of changes that starts just after dxcvalidator.cpp was copied into that place so you can see the changes that were meaningful without the confusing copy. https://github.com/microsoft/DirectXShaderCompiler/pull/5952/files/d00cb4dfff7497a0aa096121bbdceafa0a8ea3ce..d67697a3bc2d458e948a79e6b89fd9b32794f7c3

Since we're not using the fake dxil.dll everywhere, these will still be available

damyanp · 2025-01-08T18:40:50Z

Now that we've open sourced the validator/hashing we'll likely address this a different way.

pow2clk force-pushed the dxiltest branch 3 times, most recently from 555772b to 737636f Compare October 30, 2023 18:54

pow2clk force-pushed the dxiltest branch from 6182592 to 3ae2a52 Compare October 30, 2023 23:38

pow2clk marked this pull request as ready for review October 30, 2023 23:39

pow2clk changed the title ~~Add a test dxil lib for testing~~ Add a test dxil lib for testing loadability Oct 30, 2023

python3kgae reviewed Oct 31, 2023

View reviewed changes

tools/testdxildll/DxilValidator.cpp Outdated Show resolved Hide resolved

python3kgae reviewed Oct 31, 2023

View reviewed changes

tools/testdxildll/DxilValidator.cpp Outdated Show resolved Hide resolved

pow2clk and others added 2 commits October 31, 2023 11:34

Remove unneeded headers from testdxildll

496b071

Co-authored-by: Xiang Li <python3kgae@outlook.com>

Remove more unneeded headers from testdxildll

df3fdf7

Co-authored-by: Xiang Li <python3kgae@outlook.com>

Fixup clang-format aftermath of suggested changes

ea7d6fc

python3kgae reviewed Nov 1, 2023

View reviewed changes

unittests/DXILLib/DxilLibTest.cpp Show resolved Hide resolved

python3kgae reviewed Nov 1, 2023

View reviewed changes

unittests/DXILLib/DxilLibTest.cpp Outdated Show resolved Hide resolved

python3kgae approved these changes Nov 2, 2023

View reviewed changes

hekota reviewed Nov 3, 2023

View reviewed changes

tex3d reviewed Nov 3, 2023

View reviewed changes

Merge branch 'main' of https://github.com/microsoft/DirectXShaderComp…

740fb27

…iler into dxiltest

hekota reviewed Nov 9, 2023

View reviewed changes

pow2clk added 6 commits November 21, 2023 16:22

Rename testdxildll to no-sig-dxillib

cab6842

dashes match how directories in the same place are named. no-sig is a clearer prefix because it describes function instead of intent and makes it less likely to mistake it for a test utility. dxillib is platform neutral unline dxildll

Copy dxcvalidator to DxilValidator

d00cb4d

No changes, just to make reviewing the actual changes easier since the DxilValidator parent class is mostly derived from dxcvalidator with some removals

rename testdxil lib product to just dxil

b080b8e

This enables us to use it as the default unsigning validator

Fix test errors

63d46cd

Remove line numbers from a number of tests that, lacking the internal validator, will not have access to line numbers anymore Take more care with default values and possibly null pointers in DxilLibTest Correctly set the out directory for the test lib copying

tex3d reviewed Nov 27, 2023

View reviewed changes

Rename no signature dxil lib files

bd3e013

Added a NoSig prefix to all files and classes to clarify that these are not the main dxil lib

pow2clk added 2 commits November 28, 2023 10:13

Move CreateDxcContainerBuilder

f0e0cb2

clang-format and SIL purge

b93e5aa

tex3d reviewed Nov 28, 2023

View reviewed changes

pow2clk added 2 commits November 29, 2023 13:57

Rename DxilValidator and dxil lib

eac1e7d

Renamed DxilValidator to DxcValidatorBase to prevent confusion between DxilValdation and DxilValidator. Renamed dxil lib back to testdxil because testing dxil.dll broadly is going to require more care to be done right.

pow2clk added 2 commits November 30, 2023 13:01

Revert tests that had line numbers removed

ba0d7c4

Since we're not using the fake dxil.dll everywhere, these will still be available

re-remove assert

1c95008

pow2clk mentioned this pull request Mar 26, 2024

Fix auto-dxilver logic in FileCheckerTest #6368

Merged

pow2clk mentioned this pull request Aug 28, 2024

Move dxil.dll to DXC repo. #6866

Merged

damyanp closed this Jan 8, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a test dxil lib for testing loadability #5952

Add a test dxil lib for testing loadability #5952

pow2clk commented Oct 30, 2023

github-actions bot commented Oct 31, 2023 •

edited

Loading

python3kgae left a comment

pow2clk commented Nov 2, 2023

tex3d commented Nov 3, 2023

hekota Nov 3, 2023 •

edited

Loading

pow2clk Nov 6, 2023

tex3d left a comment

tex3d Nov 3, 2023

tex3d Nov 3, 2023

pow2clk Nov 27, 2023

tex3d Nov 3, 2023

pow2clk Nov 27, 2023

hekota Nov 9, 2023 •

edited

Loading

pow2clk Nov 27, 2023

tex3d Nov 27, 2023

tex3d Nov 28, 2023

tex3d Nov 28, 2023

tex3d Nov 28, 2023 •

edited

Loading

pow2clk commented Nov 30, 2023

damyanp commented Jan 8, 2025


		using namespace hlsl;

		HRESULT CreateDxcContainerBuilder(_In_ REFIID riid, _Out_ LPVOID *ppv) {

		@@ -0,0 +1,150 @@
		///////////////////////////////////////////////////////////////////////////////

Add a test dxil lib for testing loadability #5952

Add a test dxil lib for testing loadability #5952

Conversation

pow2clk commented Oct 30, 2023

github-actions bot commented Oct 31, 2023 • edited Loading

python3kgae left a comment

Choose a reason for hiding this comment

pow2clk commented Nov 2, 2023

tex3d commented Nov 3, 2023

hekota Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tex3d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hekota Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tex3d Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

pow2clk commented Nov 30, 2023

damyanp commented Jan 8, 2025

github-actions bot commented Oct 31, 2023 •

edited

Loading

hekota Nov 3, 2023 •

edited

Loading

hekota Nov 9, 2023 •

edited

Loading

tex3d Nov 28, 2023 •

edited

Loading