Create and use DXC DLL ext validation support class

[bob80905]

This PR creates a class that is responsible for loading and unloading dxil.dll Fixes https://github.com/microsoft/DirectXShaderCompiler/issues/7422

[github-actions[bot]]

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

[bogner]

The interface is a good idea, but there are a couple of things that still need some work here. Notably, Tex's concerns are about the usage of the DxcDllSupport APIs, and as is this doesn't really address those users.

I would expect the base class (which you currently call IDllSupport) to be the thing that's used in most APIs if they just need to generically have a dll that can be loaded. If some APIs require the new fallback-aware version, they might take that concretely instead. If some need to use a specific dll and fallback wouldn't make sense, they might take that one concretely. The current patch continues to use DxcDllSupport in all of the users, so it doesn't really give us a path for how one would use DxcDllExtValidationSupport instead.

A note on naming. IDllSupport probably isn't a great name. We don't really have a convention of prefixing "I" for interfaces, and it's generally better if this name tells us what the object is for. Just DllSupport or something like DllLoader might be better - though there's also a weak argument to reuse the DxcDllSupport name to simplify user's API updates. I would probably change the name of the current DxcDllSupport to something like SpecificDllLoader to indicate that it concretely loads one and only one dll. DxcDllExtValidationSupport is pretty specific to the particular case so that name is probably okay (though if we do change to the "Loader" noun then it should change as appropriate).

As for usage, there are a number of places that pass DxcDllSupport objects around today - how do they need to be updated based on the change you're doing here?

• Some users are trying to load dxcompiler, and could use the external validator version. These presumably want the base object
• Some users use these APIs to load some other specific Dll. Using the ext validation support version wouldn't make any sense here, so they probably want the Specific Dll object.

[bob80905]

Are all the changes to SPIRV-Tools here intended?

An accidental git add . put these changes in :(

[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.