-
Notifications
You must be signed in to change notification settings - Fork 792
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
base: main
Are you sure you want to change the base?
Changes from 45 commits
d7d1936
c527502
7f7dc30
e7ec854
39f913c
3700bfd
33c88f9
544a9c7
eeb1516
9800c91
a4c9629
a9282fd
e781627
2915a8d
57dd853
98b25af
c274e5b
b46def4
b76fab2
89dcc4a
acbf68a
f2890f5
eb1e96f
36c33a2
6577046
80b50a6
bfc508f
a144ca1
bd01fa5
647c946
a29f6ee
fffb407
5411894
45f74fa
64f61a8
ed23ada
2383541
209582f
daf6f0a
c8d867e
0aa3c8d
15fceac
09be489
be99192
cba829b
17f26c6
c0ee094
cdf321e
c8b704a
965f78c
48fb6fa
b100acd
f66408c
c8bebee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
#include "dxc/Support/dxcapi.use.h" | ||
#include <cassert> | ||
#include <string> | ||
|
||
namespace dxc { | ||
class DxcDllExtValidationLoader : public DllLoader { | ||
// DxCompilerSupport manages the | ||
// lifetime of dxcompiler.dll, while DxilExtValSupport | ||
// manages the lifetime of dxil.dll | ||
dxc::SpecificDllLoader DxCompilerSupport; | ||
dxc::SpecificDllLoader DxilExtValSupport; | ||
|
||
std::string DxilDllPath; | ||
tex3d marked this conversation as resolved.
Show resolved
Hide resolved
|
||
HRESULT InitializeInternal(LPCSTR fnName); | ||
|
||
public: | ||
std::string GetDxilDllPath() { return DxilDllPath; } | ||
bool DxilDllFailedToLoad() { | ||
tex3d marked this conversation as resolved.
Show resolved
Hide resolved
damyanp marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return !DxilDllPath.empty() && !DxilExtValSupport.IsEnabled(); | ||
} | ||
|
||
HRESULT CreateInstanceImpl(REFCLSID clsid, REFIID riid, | ||
IUnknown **pResult) override; | ||
HRESULT CreateInstance2Impl(IMalloc *pMalloc, REFCLSID clsid, REFIID riid, | ||
IUnknown **pResult) override; | ||
|
||
HRESULT Initialize() { return InitializeInternal("DxcCreateInstance"); } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, looks like we dont need it at all. I'll remove InitializeInternal and move its implementation to Initialize(). |
||
|
||
|
||
bool IsEnabled() const override { return DxCompilerSupport.IsEnabled(); } | ||
}; | ||
tex3d marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} // namespace dxc |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,9 +19,47 @@ namespace dxc { | |
extern const char *kDxCompilerLib; | ||
extern const char *kDxilLib; | ||
|
||
// Helper class to dynamically load the dxcompiler or a compatible libraries. | ||
class DxcDllSupport { | ||
// Interface for common dll operations | ||
class DllLoader { | ||
damyanp marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
protected: | ||
virtual HRESULT CreateInstanceImpl(REFCLSID clsid, REFIID riid, | ||
IUnknown **pResult) = 0; | ||
virtual HRESULT CreateInstance2Impl(IMalloc *pMalloc, REFCLSID clsid, | ||
REFIID riid, IUnknown **pResult) = 0; | ||
virtual ~DllLoader() {} | ||
|
||
public: | ||
DllLoader() = default; | ||
DllLoader(const DllLoader &) = default; // needed for HlslIntellisenseSupport | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please mark this as deleted and remove the HlslIntellisenseSupport move constructor. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to delete this solely because it's an interface class? Or is there some other reason? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's no way for someone who has a pointer to a |
||
DllLoader(DllLoader &&) = delete; | ||
|
||
template <typename TInterface> | ||
HRESULT CreateInstance(REFCLSID clsid, TInterface **pResult) { | ||
return CreateInstanceImpl(clsid, __uuidof(TInterface), | ||
(IUnknown **)pResult); | ||
} | ||
HRESULT CreateInstance(REFCLSID clsid, REFIID riid, IUnknown **pResult) { | ||
return CreateInstanceImpl(clsid, riid, (IUnknown **)pResult); | ||
} | ||
|
||
template <typename TInterface> | ||
HRESULT CreateInstance2(IMalloc *pMalloc, REFCLSID clsid, | ||
TInterface **pResult) { | ||
return CreateInstance2Impl(pMalloc, clsid, __uuidof(TInterface), | ||
(IUnknown **)pResult); | ||
} | ||
HRESULT CreateInstance2(IMalloc *pMalloc, REFCLSID clsid, REFIID riid, | ||
IUnknown **pResult) { | ||
return CreateInstance2Impl(pMalloc, clsid, riid, (IUnknown **)pResult); | ||
} | ||
|
||
virtual bool IsEnabled() const = 0; | ||
}; | ||
|
||
// Helper class to dynamically load the dxcompiler or a compatible libraries. | ||
class SpecificDllLoader : public DllLoader { | ||
|
||
HMODULE m_dll; | ||
DxcCreateInstanceProc m_createFn; | ||
DxcCreateInstance2Proc m_createFn2; | ||
|
@@ -74,33 +112,24 @@ class DxcDllSupport { | |
} | ||
|
||
public: | ||
DxcDllSupport() : m_dll(nullptr), m_createFn(nullptr), m_createFn2(nullptr) {} | ||
|
||
DxcDllSupport(DxcDllSupport &&other) { | ||
m_dll = other.m_dll; | ||
other.m_dll = nullptr; | ||
m_createFn = other.m_createFn; | ||
other.m_createFn = nullptr; | ||
m_createFn2 = other.m_createFn2; | ||
other.m_createFn2 = nullptr; | ||
} | ||
SpecificDllLoader() | ||
: m_dll(nullptr), m_createFn(nullptr), m_createFn2(nullptr) {} | ||
|
||
~DxcDllSupport() { Cleanup(); } | ||
~SpecificDllLoader() override { Cleanup(); } | ||
|
||
HRESULT Initialize() { | ||
return InitializeInternal(kDxCompilerLib, "DxcCreateInstance"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having this particular implementation of I don't think this is really possible without either removing There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What I'm saying is is "do we even want an For convenience, we could subclass this with |
||
} | ||
|
||
HRESULT InitializeForDll(LPCSTR dll, LPCSTR entryPoint) { | ||
HRESULT OverrideDll(LPCSTR dll, LPCSTR entryPoint) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
return InitializeInternal(dll, entryPoint); | ||
} | ||
|
||
template <typename TInterface> | ||
HRESULT CreateInstance(REFCLSID clsid, TInterface **pResult) { | ||
return CreateInstance(clsid, __uuidof(TInterface), (IUnknown **)pResult); | ||
} | ||
|
||
HRESULT CreateInstance(REFCLSID clsid, REFIID riid, IUnknown **pResult) { | ||
// Also bring visibility into the interface definition of this function | ||
// which takes 2 args | ||
using DllLoader::CreateInstanceImpl; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These |
||
HRESULT CreateInstanceImpl(REFCLSID clsid, REFIID riid, | ||
IUnknown **pResult) override { | ||
if (pResult == nullptr) | ||
return E_POINTER; | ||
if (m_dll == nullptr) | ||
|
@@ -109,15 +138,11 @@ class DxcDllSupport { | |
return hr; | ||
} | ||
|
||
template <typename TInterface> | ||
HRESULT CreateInstance2(IMalloc *pMalloc, REFCLSID clsid, | ||
TInterface **pResult) { | ||
return CreateInstance2(pMalloc, clsid, __uuidof(TInterface), | ||
(IUnknown **)pResult); | ||
} | ||
|
||
HRESULT CreateInstance2(IMalloc *pMalloc, REFCLSID clsid, REFIID riid, | ||
IUnknown **pResult) { | ||
// Also bring visibility into the interface definition of this function | ||
// which takes 3 args | ||
using DllLoader::CreateInstance2Impl; | ||
HRESULT CreateInstance2Impl(IMalloc *pMalloc, REFCLSID clsid, REFIID riid, | ||
IUnknown **pResult) override { | ||
if (pResult == nullptr) | ||
return E_POINTER; | ||
if (m_dll == nullptr) | ||
|
@@ -130,7 +155,16 @@ class DxcDllSupport { | |
|
||
bool HasCreateWithMalloc() const { return m_createFn2 != nullptr; } | ||
|
||
bool IsEnabled() const { return m_dll != nullptr; } | ||
bool IsEnabled() const override { return m_dll != nullptr; } | ||
|
||
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; | ||
} | ||
|
||
void Cleanup() { | ||
if (m_dll != nullptr) { | ||
|
@@ -162,8 +196,8 @@ inline DxcDefine GetDefine(LPCWSTR name, LPCWSTR value) { | |
// Checks an HRESULT and formats an error message with the appended data. | ||
void IFT_Data(HRESULT hr, LPCWSTR data); | ||
|
||
void EnsureEnabled(DxcDllSupport &dxcSupport); | ||
void ReadFileIntoBlob(DxcDllSupport &dxcSupport, LPCWSTR pFileName, | ||
void EnsureEnabled(SpecificDllLoader &dxcSupport); | ||
void ReadFileIntoBlob(DllLoader &dxcSupport, LPCWSTR pFileName, | ||
IDxcBlobEncoding **ppBlobEncoding); | ||
void WriteBlobToConsole(IDxcBlob *pBlob, DWORD streamType = STD_OUTPUT_HANDLE); | ||
void WriteBlobToFile(IDxcBlob *pBlob, LPCWSTR pFileName, UINT32 textCodePage); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment doesn't match the function here - is the argument a
DllLoader
or aSpecificDllLoader
? Also you've forward declared both of these above, but only one is used.