Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 54 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
54 commits
Select commit Hold shift + click to select a range
d7d1936
implement stage 2
bob80905 Jun 6, 2025
c527502
add crucial headers
bob80905 Jun 6, 2025
7f7dc30
chore: autopublish 2025-06-06T00:27:21Z
github-actions[bot] Jun 6, 2025
e7ec854
address Tex non-header changes
bob80905 Jun 6, 2025
39f913c
handle includes
bob80905 Jun 6, 2025
3700bfd
swap include order
bob80905 Jun 6, 2025
33c88f9
address Tex, and make a flawed attempt at writing a unit test
bob80905 Jun 18, 2025
544a9c7
chore: autopublish 2025-06-18T04:25:45Z
github-actions[bot] Jun 18, 2025
eeb1516
validated that local test passes
bob80905 Jun 18, 2025
9800c91
address tex, update unit test
bob80905 Jun 25, 2025
a4c9629
ifdef for windows vs unix env var addition
bob80905 Jun 25, 2025
a9282fd
add wstring conversion stuff and env var resets
bob80905 Jun 25, 2025
e781627
address tex
bob80905 Jun 25, 2025
2915a8d
special casing for unix systems
bob80905 Jun 25, 2025
57dd853
address tex, make wstrings more universal too
bob80905 Jun 30, 2025
98b25af
add includes for _wgetenv
bob80905 Jun 30, 2025
c274e5b
remove wstring for simplicity
bob80905 Jun 30, 2025
b46def4
fix undeclared variables in non windows case, and pointer lifetime bug
bob80905 Jun 30, 2025
b76fab2
adjust for build errors
bob80905 Jul 1, 2025
89dcc4a
address final forgotten comments
bob80905 Jul 1, 2025
acbf68a
try removing unicode from cmakelists
bob80905 Jul 2, 2025
f2890f5
update comment Justin pointed out was outdated
bob80905 Jul 3, 2025
eb1e96f
remove Linux definitions from windows only testing, address ambiguity…
bob80905 Jul 7, 2025
36c33a2
do not inherit from DxcDllSupport
bob80905 Jul 7, 2025
6577046
add win32 preprocessor gate to allow non windows envs to build
bob80905 Jul 7, 2025
80b50a6
remove test from non-windows since it depends on wide strings
bob80905 Jul 7, 2025
bfc508f
remove friend, remove protected:
bob80905 Jul 8, 2025
a144ca1
address Damyan final concerns
bob80905 Jul 11, 2025
bd01fa5
try another way to emit an hresult
bob80905 Jul 12, 2025
647c946
try E_INVALIDARG
bob80905 Jul 14, 2025
a29f6ee
use an interface
bob80905 Jul 18, 2025
fffb407
fix nix error
bob80905 Jul 18, 2025
5411894
remove comment on header that is likely to become stale
bob80905 Jul 24, 2025
45f74fa
perform rename
bob80905 Jul 28, 2025
64f61a8
resolve merge conflicts
bob80905 Jul 28, 2025
ed23ada
update old class name
bob80905 Jul 28, 2025
2383541
rename to IDllLoader, rename to OverrideDll, remove defaulted functio…
bob80905 Aug 6, 2025
209582f
address all of Justin + Damyan 2c
bob80905 Aug 7, 2025
daf6f0a
add override
bob80905 Aug 7, 2025
c8d867e
add more overrides
bob80905 Aug 7, 2025
0aa3c8d
add virtual to destructor for HlslIntellisense
bob80905 Aug 7, 2025
15fceac
address Damyan: specify constructors in interface, remove cleanup fro…
bob80905 Aug 7, 2025
09be489
address Damyan
bob80905 Aug 8, 2025
be99192
Merge branch 'main' into create_DxcDllExtValidationSupport
bob80905 Aug 8, 2025
cba829b
we keep trying, this should be it
bob80905 Aug 8, 2025
17f26c6
clang format
bob80905 Aug 8, 2025
c0ee094
remove move ctor and copy ctor
bob80905 Aug 8, 2025
cdf321e
address all but justins last comment
bob80905 Aug 11, 2025
c8b704a
do a function rename
bob80905 Aug 11, 2025
965f78c
try undoing spirv change
bob80905 Aug 12, 2025
48fb6fa
remove unneeded forward decl
bob80905 Aug 12, 2025
b100acd
add DxCompilerDllLoader class, and use it
bob80905 Aug 13, 2025
f66408c
address Justin
bob80905 Aug 13, 2025
c8bebee
fix build err
bob80905 Aug 13, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions include/dxc/Support/dxcapi.extval.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
#include "dxc/Support/dxcapi.use.h"
#include <string>

class DxcDllExtValidationSupport : public dxc::DxcDllSupport {
// DxcDllExtValidationSupport manages the
// lifetime of dxcompiler.dll, while the member, m_DxilSupport,
// manages the lifetime of dxil.dll
protected:
dxc::DxcDllSupport DxilSupport;

std::string DxilDllPath;

// override DxcDllSupport's implementation of InitializeInternal,
// adding the environment variable value check for a path to a dxil.dll
HRESULT InitializeInternal(LPCSTR dllName, LPCSTR fnName) override;

public:
std::string GetDxilDllPath() { return DxilDllPath; }
bool DxilDllFailedToLoad() {
return !DxilDllPath.empty() && !DxilSupport.IsEnabled();
}

void Cleanup() override {
DxilSupport.Cleanup();
DxcDllSupport::Cleanup();
}

HMODULE Detach() override {
// Can't Detach and return a handle for DxilSupport. Cleanup() instead.
DxilSupport.Cleanup();
return DxcDllSupport::Detach();
}

HRESULT CreateInstance(REFCLSID clsid, REFIID riid,
IUnknown **pResult) override;
HRESULT CreateInstance2(IMalloc *pMalloc, REFCLSID clsid, REFIID riid,
IUnknown **pResult) override;
};
24 changes: 17 additions & 7 deletions include/dxc/Support/dxcapi.use.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class DxcDllSupport {
DxcCreateInstanceProc m_createFn;
DxcCreateInstance2Proc m_createFn2;

HRESULT InitializeInternal(LPCSTR dllName, LPCSTR fnName) {
virtual HRESULT InitializeInternal(LPCSTR dllName, LPCSTR fnName) {
if (m_dll != nullptr)
return S_OK;

Expand Down Expand Up @@ -85,7 +85,7 @@ class DxcDllSupport {
other.m_createFn2 = nullptr;
}

~DxcDllSupport() { Cleanup(); }
virtual ~DxcDllSupport() { Cleanup(); }

HRESULT Initialize() {
return InitializeInternal(kDxCompilerLib, "DxcCreateInstance");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having this particular implementation of Initialize() in SpecificDllLoader is kind of irksome. This means that SpecificDllLoader really does want to load dxcompiler.dll in particular, even in cases where we're never using it for that dll. It would be nice for DxcompilerLoader to be a subclass akin to HlslIntellisenseSupport that specifically bakes in the defaults.

I don't think this is really possible without either removing Initialize() from the interface or explicitly implementing SpecificDllLoader::Initialize() to throw a runtime error though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it would raise an eyebrow on first glance, but the initialize function needs a dll to work with, because it has no args. We need some dll to default to, and dxcompilerlib is a fine one to default to. If users wanted something else, they could initialize with OverrideDll and specify the dll.
I've removed Initialize from the Interface abstract class, but I still think this function is fine as is, as a default constructor of sorts.

Copy link
Collaborator

@bogner bogner Aug 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand All @@ -100,7 +100,8 @@ class DxcDllSupport {
return CreateInstance(clsid, __uuidof(TInterface), (IUnknown **)pResult);
}

HRESULT CreateInstance(REFCLSID clsid, REFIID riid, IUnknown **pResult) {
virtual HRESULT CreateInstance(REFCLSID clsid, REFIID riid,
IUnknown **pResult) {
if (pResult == nullptr)
return E_POINTER;
if (m_dll == nullptr)
Expand All @@ -116,8 +117,8 @@ class DxcDllSupport {
(IUnknown **)pResult);
}

HRESULT CreateInstance2(IMalloc *pMalloc, REFCLSID clsid, REFIID riid,
IUnknown **pResult) {
virtual HRESULT CreateInstance2(IMalloc *pMalloc, REFCLSID clsid, REFIID riid,
IUnknown **pResult) {
if (pResult == nullptr)
return E_POINTER;
if (m_dll == nullptr)
Expand All @@ -132,7 +133,16 @@ class DxcDllSupport {

bool IsEnabled() const { return m_dll != nullptr; }

void Cleanup() {
bool GetCreateInstanceProcs(DxcCreateInstanceProc *pCreateFn,
DxcCreateInstance2Proc *pCreateFn2) const {
if (pCreateFn == nullptr || pCreateFn2 == nullptr || m_createFn == nullptr)
return false;
*pCreateFn = m_createFn;
*pCreateFn2 = m_createFn2;
return true;
}

virtual void Cleanup() {
if (m_dll != nullptr) {
m_createFn = nullptr;
m_createFn2 = nullptr;
Expand All @@ -145,7 +155,7 @@ class DxcDllSupport {
}
}

HMODULE Detach() {
virtual HMODULE Detach() {
HMODULE hModule = m_dll;
m_dll = nullptr;
return hModule;
Expand Down
1 change: 1 addition & 0 deletions lib/DxcSupport/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ add_llvm_library(LLVMDxcSupport
WinAdapter.cpp
WinIncludes.cpp
WinFunctions.cpp
dxcapi.extval.cpp
)

#generate header with platform-specific library name
Expand Down
54 changes: 54 additions & 0 deletions lib/DxcSupport/dxcapi.extval.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
#include "dxc/Support/WinIncludes.h"
#include <dxc/Support/Global.h> // for hresult handling with DXC_FAILED
#include <filesystem> // C++17 and later
// WinIncludes must come before dxcapi.extval.h
#include "dxc/Support/dxcapi.extval.h"

HRESULT DxcDllExtValidationSupport::InitializeInternal(LPCSTR dllName,
LPCSTR fnName) {
// Load dxcompiler.dll
HRESULT Result = DxcDllSupport::InitializeInternal(dllName, fnName);
// if dxcompiler.dll fails to load, return the failed HRESULT
if (DXC_FAILED(Result)) {
return Result;
}

// now handle external dxil.dll
const char *EnvVarVal = std::getenv("DXC_DXIL_DLL_PATH");
if (!EnvVarVal || std::string(EnvVarVal).empty()) {
return S_OK;
}

std::string DllPathStr(EnvVarVal);
DxilDllPath = DllPathStr;
std::filesystem::path DllPath(DllPathStr);

// Check if path is absolute and exists
if (!DllPath.is_absolute() || !std::filesystem::exists(DllPath)) {
return S_OK;
}
// code that calls this function is responsible for checking
// to see if dxil.dll is successfully loaded.
// the CheckDxilDLLLoaded function can determine whether there were any
// problems loading dxil.dll or not
DxilSupport.InitializeForDll(DllPathStr.data(), fnName);

return S_OK;
}

HRESULT DxcDllExtValidationSupport::CreateInstance(REFCLSID clsid, REFIID riid,
IUnknown **pResult) {
if (DxilSupport.IsEnabled() && clsid == CLSID_DxcValidator)
return DxilSupport.CreateInstance(clsid, riid, pResult);

return DxcDllSupport::CreateInstance(clsid, riid, pResult);
}

HRESULT DxcDllExtValidationSupport::CreateInstance2(IMalloc *pMalloc,
REFCLSID clsid, REFIID riid,
IUnknown **pResult) {
if (DxilSupport.IsEnabled() && clsid == CLSID_DxcValidator)
return DxilSupport.CreateInstance2(pMalloc, clsid, riid, pResult);

return DxcDllSupport::CreateInstance2(pMalloc, clsid, riid, pResult);
}
109 changes: 109 additions & 0 deletions tools/clang/unittests/HLSL/ValidationTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,13 @@
#include "dxc/DxilContainer/DxilContainerAssembler.h"
#include "dxc/DxilContainer/DxilPipelineStateValidation.h"
#include "dxc/DxilHash/DxilHash.h"
#include "dxc/Support/Unicode.h" // for wstring conversions like WideToUtf8String
#include "dxc/Support/WinIncludes.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Regex.h"
#include <cstdlib> // for setenv(), getenv(), unsetenv()
#include <wchar.h> // for _wgetenv

#ifdef _WIN32
#include <atlbase.h>
Expand All @@ -32,6 +35,7 @@
#include "dxc/Support/Global.h"

#include "dxc/DXIL/DxilShaderModel.h"
#include "dxc/Support/dxcapi.extval.h"
#include "dxc/Test/DxcTestUtils.h"
#include "dxc/Test/HlslTestUtils.h"

Expand Down Expand Up @@ -323,6 +327,7 @@ class ValidationTest : public ::testing::Test {
TEST_METHOD(PSVContentValidationCS)
TEST_METHOD(PSVContentValidationMS)
TEST_METHOD(PSVContentValidationAS)
TEST_METHOD(UnitTestExtValidationSupport)
TEST_METHOD(WrongPSVSize)
TEST_METHOD(WrongPSVSizeOnZeros)
TEST_METHOD(WrongPSVVersion)
Expand Down Expand Up @@ -4207,6 +4212,110 @@ TEST_F(ValidationTest, ValidateWithHash) {
VERIFY_ARE_EQUAL(memcmp(Result, pHeader->Hash.Digest, sizeof(Result)), 0);
}

std::wstring GetEnvVarW(const std::wstring &VarName) {
#ifdef _WIN32
if (const wchar_t *Result = _wgetenv(VarName.c_str()))
return std::wstring(Result);
#else
std::string VarNameUtf8;
Unicode::WideToUTF8String(VarName.c_str(), &VarNameUtf8);
if (const char *Result = std::getenv(VarNameUtf8.c_str())) {
std::wstring ResultWide;
Unicode::UTF8ToWideString(Result, &ResultWide);
return std::wstring(ResultWide);
}
#endif
return std::wstring();
}

void SetEnvVarW(const std::wstring &VarName, const std::wstring &VarValue) {
#ifdef _WIN32
_wputenv_s(VarName.c_str(), VarValue.c_str());
#else
std::string VarNameUtf8;
std::string VarValueUtf8;
Unicode::WideToUTF8String(VarName.c_str(), &VarNameUtf8);
Unicode::WideToUTF8String(VarValue.c_str(), &VarValueUtf8);
setenv(VarNameUtf8.c_str(), VarValueUtf8.c_str(), 1);
#endif
}

// For now, 3 things are tested:
// 1. The environment variable is not set. GetDxilDllPath() is empty and
// DxilDllFailedToLoad() returns false
// 2. Given a bogus path in the environment variable, GetDxilDllPath()
// retrieves the path but fails to load it as a dll, and returns true
// for DxilDllFailedToLoad()
// 3. CLSID_DxcCompiler, CLSID_DxcLinker, CLSID_DxcValidator
// may be created through DxcDllExtValidationSupport.
// This is all to simply test that the new class, DxcDllExtValidationSupport,
// works as intended.

TEST_F(ValidationTest, UnitTestExtValidationSupport) {
DxcDllExtValidationSupport ExtSupportEmpty;
DxcDllExtValidationSupport ExtSupportBogus;

// capture any existing value in the environment variable,
// so that it can be restored after the test
std::wstring OldEnvVal = GetEnvVarW(L"DXC_DXIL_DLL_PATH");

// 1. with no env var set, test GetDxilDllPath() and DxilDllFailedToLoad()

// make sure the variable is cleared, in case other tests may have set it
SetEnvVarW(L"DXC_DXIL_DLL_PATH", L"");

// empty initialization should succeed
VERIFY_SUCCEEDED(ExtSupportEmpty.Initialize());

VERIFY_IS_FALSE(ExtSupportEmpty.DxilDllFailedToLoad());
std::string EmptyPath = ExtSupportBogus.GetDxilDllPath();
VERIFY_ARE_EQUAL_STR(EmptyPath.c_str(), "");

// 2. Test with a bogus path in the environment variable
SetEnvVarW(L"DXC_DXIL_DLL_PATH", L"bogus");

if (!ExtSupportBogus.IsEnabled()) {
VERIFY_SUCCEEDED(ExtSupportBogus.Initialize());
}

// validate that m_dllExtSupport2 was able to capture the environment
// variable's value, and that loading the bogus path was unsuccessful
std::string BogusPath = ExtSupportBogus.GetDxilDllPath();
VERIFY_ARE_EQUAL_STR(BogusPath.c_str(), "bogus");
VERIFY_IS_TRUE(ExtSupportBogus.DxilDllFailedToLoad());

// 3. Test production of class IDs CLSID_DxcCompiler, CLSID_DxcLinker,
// and CLSID_DxcValidator through DxcDllExtValidationSupport.
CComPtr<IDxcCompiler> Compiler;
CComPtr<IDxcLinker> Linker;
CComPtr<IDxcValidator> Validator;

VERIFY_SUCCEEDED(ExtSupportBogus.CreateInstance(
CLSID_DxcCompiler, __uuidof(IDxcCompiler), (IUnknown **)&Compiler));
VERIFY_SUCCEEDED(ExtSupportBogus.CreateInstance(
CLSID_DxcLinker, __uuidof(IDxcLinker), (IUnknown **)&Linker));
VERIFY_SUCCEEDED(ExtSupportBogus.CreateInstance(
CLSID_DxcValidator, __uuidof(IDxcValidator), (IUnknown **)&Validator));

CComPtr<IMalloc> Malloc;
CComPtr<IDxcCompiler2> Compiler2;
Linker.Release();
Validator.Release();
VERIFY_SUCCEEDED(DxcCoGetMalloc(1, &Malloc));
VERIFY_SUCCEEDED(ExtSupportBogus.CreateInstance2(Malloc, CLSID_DxcCompiler,
__uuidof(IDxcCompiler),
(IUnknown **)&Compiler2));
VERIFY_SUCCEEDED(ExtSupportBogus.CreateInstance2(
Malloc, CLSID_DxcLinker, __uuidof(IDxcLinker), (IUnknown **)&Linker));
VERIFY_SUCCEEDED(ExtSupportBogus.CreateInstance2(Malloc, CLSID_DxcValidator,
__uuidof(IDxcValidator),
(IUnknown **)&Validator));

// reset the environment variable to its previous value,
// or the empty string if there was no previous value
SetEnvVarW(L"DXC_DXIL_DLL_PATH", OldEnvVal);
}

TEST_F(ValidationTest, ValidatePreviewBypassHash) {
if (m_ver.SkipDxilVersion(1, ShaderModel::kHighestMinor))
return;
Expand Down