Create new class to handle external validation, and rename existing dll loading class.

[bob80905]

This PR creates a class that is responsible for loading and unloading dxil.dll, allowing for external validation.
Tests are added to test this class's functionality. Additionally, an interface is defined that joins the existing singular dll loading helper class, and the new class that tracks two dlls. The classes were renamed to be more abstract, as well as all existing uses of the original singular dll loader class.
Fixes #7422

[github-actions]

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

[tex3d]

I had a number of comments, but many of these issues would have been noticed when trying to create a unit test that used this new DxcDllExtValidationSupport class. I think we need a unit test.

[damyanp]

Are all the changes to SPIRV-Tools here intended?

[bob80905]

Are all the changes to SPIRV-Tools here intended?

An accidental git add . put these changes in :(

[damyanp]

Please mark this as deleted and remove the HlslIntellisenseSupport move constructor.

[bob80905]

Do we want to delete this solely because it's an interface class? Or is there some other reason?

[damyanp]

There's no way for someone who has a pointer to a DllLoader instance to safely copy or move it, so we shouldn't have copy/move constructors for it. Plus, since there's no actual need to ever copy these we shouldn't be spending time trying to make it work. As it is now, the default copy constructor for SpecificDllLoader will end up leaking the dll HANDLE, so we don't want people trying to copy that.

[damyanp]

Thanks for bearing with me. I think the interface now is in much better shape. I look forward to seeing this hooked up to some real functionality so we can understand how it all fits together!

[bogner]

The comment doesn't match the function here - is the argument a DllLoader or a SpecificDllLoader? Also you've forward declared both of these above, but only one is used.

[bogner]

Why does InitializeInternal take a fnName argument if it's only ever called with a fixed string? Do we need InitializeInternal at all, or should we just update the implementation to be an implementation of Initialize()?

[bob80905]

Good point, looks like we dont need it at all. I'll remove InitializeInternal and move its implementation to Initialize().

[bogner]

These using statements are no longer necessary

[bogner]

Do we need both of these?

[bob80905]

Yes, without them the uses in 364 / 369 fail to compile.

[bogner]

Maybe I'm missing something, but I only see uses of SpecificDllLoader on those lines. Is DllLoader also used?

[bogner]

This came from a suggestion of mine, but now that the API has been simplified a bit and this only in SpecificDllLoader I find many of the updated users of this to be confusing. We often create the SpecificDllLoader, don't call Initialize() at all, and then call OverrideDll - this isn't really overriding, it's just initializing. I think I steered you wrong and the original InitializeForDll name was better.

[bogner]

What I'm saying is is "do we even want an Initialize() function with no parameters on SpecificDllLoader? What if SpecificDllLoader only had InitializeForDll() and to use it you had to specify which dll you wanted to load?

For convenience, we could subclass this with DxcompilerDllLoader that specifically loads dxcompiler.dll, much like we do with HlslIntellisenseSupport. That object could reasonably have a Initialize() that knows what Dll to load. This would match the Initialize() on the external validator class that knows which two Dlls it's going to load.

[bob80905]

To your last comment, I do think there is an argument to be made for having an Initialize() with no arguments.
This way, we can at a singular location define the default, which are these parameters: kDxCompilerLib, "DxcCreateInstance
If we remove this no-arg function, and remove the useless Initialize() function as a result, then at each location where Initialize() is used, I'll need to replace it with InitializeForDll(kDxCompilerLib, "DxcCreateInstance");, which means I'll need to define kDxCompilerLib in multiple locations throughout the project, and add its definition to many more CMakeLists.txt, which seems kinda messy to me.
The way its done now, we have a clearly defined default, and it's implemented in a singular location, which is very nice.
Right now we've got a pure abstract interface, an abstract class that can load / handle any specific dll, with a default of handling dxcompiler.dll, and a specific class that handles 2 specific dlls. I don't think there's any missing functionality here.

[damyanp]

Looks like some changes to SPIRV-Tools have snuck in again.

[bogner]

To your last comment, I do think there is an argument to be made for having an Initialize() with no arguments. This way, we can at a singular location define the default, which are these parameters: kDxCompilerLib, "DxcCreateInstance If we remove this no-arg function, and remove the useless Initialize() function as a result, then at each location where Initialize() is used, I'll need to replace it with InitializeForDll(kDxCompilerLib, "DxcCreateInstance");, which means I'll need to define kDxCompilerLib in multiple locations throughout the project, and add its definition to many more CMakeLists.txt, which seems kinda messy to me. The way its done now, we have a clearly defined default, and it's implemented in a singular location, which is very nice. Right now we've got a pure abstract interface, an abstract class that can load / handle any specific dll, with a default of handling dxcompiler.dll, and a specific class that handles 2 specific dlls. I don't think there's any missing functionality here.

This comment seems to have missed the second half of my suggestion:

For convenience, we could subclass this with DxcompilerDllLoader that specifically loads dxcompiler.dll. That object could reasonably have a Initialize() that knows what Dll to load. This would match the Initialize() on the external validator class that knows which two Dlls it's going to load.

So what I mean is that we could have something like:

class DxCompilerDllLoader : public SpecificDllLoader {
public:
  HRESULT Initialize() {
    return InitializeForDll(kDxCompilerLib, "DxcCreateInstance");
  }
};

For usages where we want to load kDxCompilerLib, you would use this object, like so:

DxcompilerDllLoader DXCSupport;
DXCSupport.Initialize();
// Call APIs that accept the DllLoader interface

For a usage that wants some other specific Dll, you would use InitializeForDll:

SpecificDllLoader DXRFallbackSupport;
DXRFallbackSupport.InitializeForDll("DxrFallbackCompiler.dll",
                                    "DxcCreateDxrFallbackCompiler");
// Call APIs that accept the DllLoader interface

The nice thing here is that for DXRFallbackSupport above, since it's a SpecificDllLoader and doesn't have Initialize() at all, you couldn't have some user of SpecificDllLoader decide to call Initialize() on it and accidentally replace the dll with dxcompiler.dll innocuously - any user of SpecificDllLoader that wanted to change what Dll it loads would do so explicitly.

The point I'm trying to make here is that SpecificDllLoader loading dxcompiler.dll by default is confusing - it's a thing that can load a dll, why would a user of a class with that name expect dxcompiler.dll in particular? It isn't DxcompilerOrMaybeSomeOtherSpecificDllLoader (which is kind of what the original DxcSupport class was used like, but that's besides the point), it's just a loader for a Dll, so I'm not convinced a default Dll makes a lot of sense.

[bogner]

Maybe I'm missing something, but I only see uses of SpecificDllLoader on those lines. Is DllLoader also used?

[bogner]

Why change this from Initialize()?

[bob80905]

Without taking into account the overarching comment you just made in this latest review, I had thought being extra explicit was nice, rather than using the default initializer.

[bogner]

Now that we have DxCompilerDllLoader there's an argument that using that type in the API would be even more explicit.

[bogner]

This is looking pretty good. Just a few more notes and questions inline.

[bogner]

This will presumably need to become a DllLoader in the future in order to support the ext validation case, right? This is using a SpecificDllLoader for now since you're planning to make those changes separately?

[bogner]

Should this one be a DxCompilerDllLoader? That seems to be what its used for.

[bob80905]

No, because on line 323, it loads an arbitrary dll from the command line options. This would break the specificity of that class, so it seems to me SpecificDllLoader is as specific (the least abstract) as we can get.

[bogner]

Would it make more sense for EnsureEnabled to take a DxCompilerDllLoader? I think that everywhere that uses it pretty clearly intends to be loading dxcompiler.dll.

Do note though that this API will probably need to change once we start plugging the external validator into the various tools and tests, as it won't be able to handle that type as is.

[bob80905]

I don't think so. For example, dxc.cpp:1464 has a use of ensureenabled.
The dll that is loaded depends on dxcOpts, and the code dynamically loads a specified dll via the setup function on line 1455. Because that dll could potentially not be the standard dxcompiler.dll, I shouldn't change the type that ensure enabled takes. Writing the function body inline also seems ugly for a case by case basis.
I'll make sure the right changes are performed when combing over the dxc.cpp test suite specifically.

[bogner]

Would it be clearer if m_dxcSupport were a DxCompilerDllLoader here? m_dxrFallbackSupport being a different SpecificDllLoader makes sense though.

[bogner]

Now that we have DxCompilerDllLoader there's an argument that using that type in the API would be even more explicit.