Open source dxil.dll #6770

python3kgae · 2024-07-11T17:07:36Z

This pull request introduces the open-source implementation of dxil.dll, including hashing and validation functionalities for DXIL containers.

DxilHash.cpp: Implements DXBC/DXIL container hashing functions.
dxcvalidatorp.cpp: Implements the DirectX Validator object, including signing support.
dxildll.cpp: Implements the DxcCreateInstance and DllMain functions for the dxil.dll module.
DxcSigningContainerBuilder.cpp: Implements the Dxil Container Builder.

This is first part for #6769

github-actions · 2024-07-11T17:08:33Z

✅ With the latest revision this PR passed the C/C++ code formatter.

tools/clang/tools/dxildll/DxcSigningContainerBuilder.cpp

tools/clang/tools/dxildll/dxcvalidatorp.cpp

llvm-beanz · 2024-07-13T17:31:21Z

@python3kgae, I realize that you didn't write all this code and you're really just copying it from an internal fork into the public repo. That said, there are a lot of small things about this code that don't meet the LLVM coding standards. It might be worth spending a little bit of time cleaning up some of the low hanging fruit so that the code goes into the public repo in a cleaner state.

I pointed out a few places where the terminology "signing" is used to describe the hashing process. I think we need to correct all of those (there are still more). I know that historically this was called "signing", but it is not a signature and calling it a signature places an unmet expectation on what the code does.

python3kgae · 2024-07-13T19:58:38Z

@python3kgae, I realize that you didn't write all this code and you're really just copying it from an internal fork into the public repo. That said, there are a lot of small things about this code that don't meet the LLVM coding standards. It might be worth spending a little bit of time cleaning up some of the low hanging fruit so that the code goes into the public repo in a cleaner state.

I pointed out a few places where the terminology "signing" is used to describe the hashing process. I think we need to correct all of those (there are still more). I know that historically this was called "signing", but it is not a signature and calling it a signature places an unmet expectation on what the code does.

All the 'signning's are replaced with 'hashing' already.
I can update pSigned to pHashed next.
Do you have anything else in mind?

llvm-beanz · 2024-07-16T14:53:21Z

All the 'signning's are replaced with 'hashing' already. I can update pSigned to pHashed next. Do you have anything else in mind?

We spent months last year reviewing and cleaning up the SM 6.8 code to get closer to meeting LLVM's coding standards. I think spending a little time on this PR makes sense.

Some things to look for:

pSigned should be Hashed or something similar. LLVM does not use Hungarian notation for naming (see: https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly).
There are several places where deep nesting could be simplified with early returns or just different control flow structures (see: https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code).
Windows-isms like UINT and BYTE could be replaced with C++-standard types.
Lots of simple condition blocks have braces (see: https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements)
Look at the usage of DXC's macros (e.g. IFR, IFRBOOL etc) to see if they actually make sense.

I'm not asking that you rewrite this code, just spend a few minutes making sure that the code we introduce to DXC is a little nicer than what we had internally.

python3kgae · 2024-07-16T20:49:22Z

All the 'signning's are replaced with 'hashing' already. I can update pSigned to pHashed next. Do you have anything else in mind?

We spent months last year reviewing and cleaning up the SM 6.8 code to get closer to meeting LLVM's coding standards. I think spending a little time on this PR makes sense.

Some things to look for:

pSigned should be Hashed or something similar. LLVM does not use Hungarian notation for naming (see: https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly).

There are several places where deep nesting could be simplified with early returns or just different control flow structures (see: https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code).

Windows-isms like UINT and BYTE could be replaced with C++-standard types.

Lots of simple condition blocks have braces (see: https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements)

Look at the usage of DXC's macros (e.g. IFR, IFRBOOL etc) to see if they actually make sense.

I'm not asking that you rewrite this code, just spend a few minutes making sure that the code we introduce to DXC is a little nicer than what we had internally.

Updated Signed to Hashed.

For other issues, https://github.com/microsoft/DirectXShaderCompiler/blob/main/tools/clang/tools/dxcompiler/dxcvalidator.cpp need to be updated as well.

Will do the job when work on Build the DXIL validator library both as a dynamic and static library where these 2 validators could be merged into one.

damyanp · 2024-07-18T16:59:00Z

Will do the job when work on Build the DXIL validator library both as a dynamic and static library where these 2 validators could be merged into one.

Can you help me understand the benefits of doing this later rather than now in this initial PR? I assume that this change comes in with enough testing that we can make NFC refactors to the code without worrying about breaking it, and given that a bar for code style/quality has been set with SM6.8 work it isn't obvious to me why we'd change that for this change.

python3kgae · 2024-07-18T17:26:37Z

Will do the job when work on Build the DXIL validator library both as a dynamic and static library where these 2 validators could be merged into one.

Can you help me understand the benefits of doing this later rather than now in this initial PR? I assume that this change comes in with enough testing that we can make NFC refactors to the code without worrying about breaking it, and given that a bar for code style/quality has been set with SM6.8 work it isn't obvious to me why we'd change that for this change.

The code in
https://github.com/microsoft/DirectXShaderCompiler/blob/main/tools/clang/tools/dxcompiler/dxcvalidator.cpp and the validator code in this change are very similar and have similar issues.
Build the DXIL validator library both as a dynamic and static library will need to merge these 2 validators, where we only need to do the style change in one place.
If we do the style change only in the validator for current PR, when we merge the 2 validators, the code will get more difference just caused by style change.

damyanp · 2024-07-18T22:09:32Z

The code in https://github.com/microsoft/DirectXShaderCompiler/blob/main/tools/clang/tools/dxcompiler/dxcvalidator.cpp and the validator code in this change are very similar and have similar issues.

Oh, I didn't realize that this change was essentially adding a whole load of duplicate code. I think we should figure out how to deduplicate this before completing the PR, otherwise we're just adding more technical debt to the project.

Enable dxildll.rc.

tex3d · 2024-08-01T00:54:29Z

tools/clang/tools/dxcvalidator/dxcvalidator.cpp

+
+static void HashAndUpdateOrCopy(uint32_t Flags, IDxcBlob *Shader,
+                                IDxcBlob **Hashed) {
+  if (Flags & DxcValidatorFlags_InPlaceEdit) {


If we were to add a flag to skip hashing and container modification called DxcValidatorFlags_SkipHash, we could implement it like so:

Suggested change

if (Flags & DxcValidatorFlags_InPlaceEdit) {

if (Flags & DxcValidatorFlags_SkipHash) {

*Hashed = Shader;

Shader->AddRef();

} else if (Flags & DxcValidatorFlags_InPlaceEdit) {

tex3d · 2024-08-01T00:57:34Z

tools/clang/tools/dxcvalidator/dxcvalidator.cpp

+    // Possible gotcha: the blob allocated here is tied to this .dll, so the
+    // DLL shouldn't be unloaded before the blob is released.


That's generally how returned blobs work in the DXC APIs. Do we need to point this out as a comment here? It probably belongs in the API documentation, and potentially as a comment on the API definition.

tex3d · 2024-08-01T02:13:02Z

tools/clang/tools/dxrfallbackcompiler/dxcvalidator.cpp

@@ -64,7 +64,7 @@ HRESULT STDMETHODCALLTYPE DxcValidator::Validate(
    IDxcOperationResult *
        *ppResult // Validation output status, buffer, and errors
 ) {
-  return hlsl::validate(pShader, Flags, ppResult);
+  return hlsl::validate(pShader, Flags, true, ppResult);


This should not be true, and I don't think we should have this implementation of IDxcValidator in dxrfallbackcompiler in the first place.

tex3d · 2024-08-01T02:15:15Z

tools/clang/tools/dxcvalidator/dxcvalidator.cpp

@@ -155,24 +222,16 @@ runRootSignatureValidation(IDxcBlob *Shader,
 uint32_t hlsl::validate(
    IDxcBlob *Shader,            // Shader to validate.
    uint32_t Flags,              // Validation flags.
+    bool IsInternalValidator,    // Run internal validator.


I don't think we should need this flag, ultimately. But that will require a cleanup of how we do internal validation.

In fact, it appears that the only way this flag is used is to ignore DxcValidatorFlags_ModuleOnly if it's not set. I don't think this is right. See my comment on the use of this flag in hlsl::validateWithDebug.

tex3d · 2024-08-01T02:16:32Z

tools/clang/tools/dxcvalidator/dxcvalidator.cpp

 static uint32_t runValidation(
    IDxcBlob *Shader,
    uint32_t Flags,            // Validation flags.
    llvm::Module *Module,      // Module to validate, if available.
    llvm::Module *DebugModule, // Debug module to validate, if available
-    AbstractMemoryStream *DiagMemStream) {
+    AbstractMemoryStream *DiagMemStream, IDxcBlob **Hashed) {


This Hashed output pointer is not written to in this function, nor does it compute the hash.

This function is for the internal case, which I think needs an overhaul, especially now that we are integrating the hash here.

It's likely best if we did the overhaul as a separate change before integrating the hash changes. It should greatly simplify the scenarios.

We should get rid of the special internal validator case where it accepts the main module as a llvm::Module and always validate the serialized bitcode in the container. We will want to keep the debug module argument, so it may be passed along internally, avoiding extra deserialization, since it's only used for diagnostic locations, and not the thing being validated.

Ok, so this is really meant to be the inner function used by both code paths that have debug bitcode and the internal path that has a debug module. We should probably use this in the other runValidation overload, and in the case of DxcValidatorFlags_ModuleOnly use a different function entirely to call this one which deserialized the debug module and does not have a Hashed output at all.

tex3d · 2024-08-01T02:28:47Z

tools/clang/tools/dxcvalidator/dxcvalidator.cpp

+    if (Flags & DxcValidatorFlags_RootSignatureOnly)
+      ValidationStatus =
+          runRootSignatureValidation(Shader, DiagMemStream, Flags, &HashedBlob);
+    else if ((Flags & DxcValidatorFlags_ModuleOnly) && IsInternalValidator)


DxcValidatorFlags_ModuleOnly should allow validation of raw bitcode, and disable hashing. IsInternalValidator should not be required, and I don't think this code handles this case properly.

Current dxil.dll doesn't support DxcValidatorFlags_ModuleOnly, just return DXC_E_CONTAINER_INVALID.
Shall we change that in this PR?

tex3d · 2024-08-01T02:40:34Z

tools/clang/tools/dxcvalidator/dxcvalidator.cpp

-                             &DiagContext, true);
-    std::unique_ptr<llvm::Module> DebugModule;
-    if (OptDebugBitcode) {
-      hr = ValidateLoadModule((const char *)OptDebugBitcode->Ptr,


Why has this been removed? We should be passing the loaded debug module through to runValidation, right?

Nevermind, it's just missing from the DxcValidatorFlags_ModuleOnly case, I'll make a separate comment.

Ok, I think this original structure should be restored so that a debug module may be passed down to a validateWithOptModules function that will apply the hash when applicable.

Recovered debug module.

tex3d · 2024-08-01T02:43:48Z

tools/clang/tools/dxcvalidator/dxcvalidator.cpp

+      ValidationStatus =
+          runRootSignatureValidation(Shader, DiagMemStream, Flags, &HashedBlob);
+    else if ((Flags & DxcValidatorFlags_ModuleOnly) && IsInternalValidator)
+      ValidationStatus = runValidation(Shader, Flags, nullptr, nullptr,


This should be passing the OptDebugBitcode along, though that will require deserialization in this overloaded runValidation path.

tools/clang/tools/dxildll/dxcvalidatorp.cpp

tex3d · 2024-08-01T03:57:30Z

tools/clang/tools/dxcvalidator/dxcvalidator.cpp

-static uint32_t
-runRootSignatureValidation(IDxcBlob *Shader,
-                           AbstractMemoryStream *DiagMemStream) {
+static uint32_t runValidation(IDxcBlob *Shader,


I think it would be better if this was combined with the other overload, and the combined function handled whether DxcValidatorFlags_ModuleOnly was being performed, or if we have a full container to sign. Right now it's confusing, and an internal path with a debug module can't go through the signing path.

This can be accomplished by moving debug module serialization out, but it will also require a change to ValidateDxilContainer to take the llvm::Module* instead of the serialized bitcode.

Hmm... perhaps this isn't sufficient, since it might need to load the debug module from the container instead. So maybe we just need to add a debug llvm::Module argument to pass along if we already have one.

…alidator.

python3kgae requested a review from a team as a code owner July 11, 2024 17:07

damyanp assigned python3kgae Jul 11, 2024

llvm-beanz reviewed Jul 12, 2024

View reviewed changes

tools/clang/tools/dxildll/DxcSigningContainerBuilder.cpp Outdated Show resolved Hide resolved

tools/clang/tools/dxildll/dxcvalidatorp.cpp Outdated Show resolved Hide resolved

tools/clang/tools/dxildll/dxcvalidatorp.cpp Outdated Show resolved Hide resolved

damyanp mentioned this pull request Jul 16, 2024

Binary release artifacts for macOS #3686

Open

python3kgae and others added 12 commits July 26, 2024 14:47

Move dxil.dll to projects/dxildll.

4041bd3

Move to tools/clang/tools/dxildll

26f9bec

Fix file header.

2ac8a70

Enable dxildll.rc.

Add unit test for DxilHash.

b810ae7

Fix the header clang-format.

3b9ce6e

chore: autopublish 2024-07-11T17:22:09Z

9ff5161

Remove unused header.

5f4fc67

Remove SAL annotations.

c9dcca3

Rename signing to hashing.

f5552bd

Rename Signed to Hashed.

da3a5bf

Fix clang format.

72cfe5b

Fix styles.

31c754c

python3kgae force-pushed the validator_hash branch from 20378d5 to 31c754c Compare July 28, 2024 14:29

python3kgae added 2 commits July 28, 2024 18:21

Recover internal validator bahavior.

8361702

Fix clang format.

acd4226

tex3d reviewed Aug 1, 2024

View reviewed changes

Remove comment in wrong place.

e2ca038

tex3d reviewed Aug 1, 2024

View reviewed changes

tools/clang/tools/dxildll/dxcvalidatorp.cpp Outdated Show resolved Hide resolved

python3kgae added 3 commits July 31, 2024 23:35

Recover DebugModule.

a2801cd

Fix clang format.

d2c42ed

Rename file.

f4366ce

tex3d reviewed Aug 1, 2024

View reviewed changes

Support DxcValidatorFlags_ModuleOnly for both internal and external v…

7844b64

…alidator.

python3kgae closed this Oct 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Open source dxil.dll #6770

Open source dxil.dll #6770

python3kgae commented Jul 11, 2024

github-actions bot commented Jul 11, 2024 •

edited

Loading

llvm-beanz commented Jul 13, 2024

python3kgae commented Jul 13, 2024

llvm-beanz commented Jul 16, 2024

python3kgae commented Jul 16, 2024

damyanp commented Jul 18, 2024

python3kgae commented Jul 18, 2024

damyanp commented Jul 18, 2024

tex3d Aug 1, 2024

tex3d Aug 1, 2024

python3kgae Aug 1, 2024

tex3d Aug 1, 2024

tex3d Aug 1, 2024

tex3d Aug 1, 2024

tex3d Aug 1, 2024

tex3d Aug 1, 2024

python3kgae Aug 1, 2024

tex3d Aug 1, 2024

python3kgae Aug 1, 2024

tex3d Aug 1, 2024

tex3d Aug 1, 2024

tex3d Aug 1, 2024

python3kgae Aug 1, 2024

tex3d Aug 1, 2024

python3kgae Aug 1, 2024

tex3d Aug 1, 2024

tex3d Aug 1, 2024

tex3d Aug 1, 2024

-  if (Flags & DxcValidatorFlags_InPlaceEdit) {
+  if (Flags & DxcValidatorFlags_SkipHash) {
+    *Hashed = Shader;
+    Shader->AddRef();
+  } else if (Flags & DxcValidatorFlags_InPlaceEdit) {

		// Possible gotcha: the blob allocated here is tied to this .dll, so the
		// DLL shouldn't be unloaded before the blob is released.

Open source dxil.dll #6770

Open source dxil.dll #6770

Conversation

python3kgae commented Jul 11, 2024

github-actions bot commented Jul 11, 2024 • edited Loading

llvm-beanz commented Jul 13, 2024

python3kgae commented Jul 13, 2024

llvm-beanz commented Jul 16, 2024

python3kgae commented Jul 16, 2024

damyanp commented Jul 18, 2024

python3kgae commented Jul 18, 2024

damyanp commented Jul 18, 2024

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 11, 2024 •

edited

Loading