From e8fb0ce9dece0633db5164c58b33f053ef702880 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Sat, 22 May 2021 22:59:49 +0200 Subject: [PATCH] WinAdapter: Make `IUnknown` and `IMalloc` pure-virtual classes `IUnknown` in Windows' `unknwn.h` and `IMalloc` in `ObjIdl.h` are marked as pure virtual, and are best marked as such in `WinAdapter` for non-Windows platforms too [1]. Only the shim for `IMalloc` was relying on the default refcounting implementation, all other subclasses either contain pure-virtual methods themselves or provide an implementation for `AddRef`/`Release` as required. Likewise the default implementation for `IMalloc` was only instantiated once by `CoGetMalloc`, and has been moved into a local class implementing the `IMalloc` interface instead. [1]: https://github.com/microsoft/DirectXShaderCompiler/pull/3793#issuecomment-846459741 --- include/dxc/Support/WinAdapter.h | 16 +++++--------- include/dxc/Test/CompilationResult.h | 2 +- lib/DxcSupport/FileIOHelper.cpp | 16 ++++++-------- lib/DxcSupport/WinAdapter.cpp | 23 -------------------- lib/DxcSupport/WinFunctions.cpp | 20 ++++++++++++++++- tools/clang/tools/dxclib/dxc.cpp | 7 +++--- tools/clang/tools/libclang/dxcisenseimpl.cpp | 2 +- tools/clang/unittests/HLSL/ExtensionTest.cpp | 4 ++-- 8 files changed, 40 insertions(+), 50 deletions(-) diff --git a/include/dxc/Support/WinAdapter.h b/include/dxc/Support/WinAdapter.h index 0a925c6ba3..714839002c 100644 --- a/include/dxc/Support/WinAdapter.h +++ b/include/dxc/Support/WinAdapter.h @@ -615,16 +615,13 @@ template inline void **IID_PPV_ARGS_Helper(T **pp) { CROSS_PLATFORM_UUIDOF(IUnknown, "00000000-0000-0000-C000-000000000046") struct IUnknown { - IUnknown() : m_count(0) {}; + IUnknown() {}; virtual HRESULT QueryInterface(REFIID riid, void **ppvObject) = 0; - virtual ULONG AddRef(); - virtual ULONG Release(); + virtual ULONG AddRef() = 0; + virtual ULONG Release() = 0; template HRESULT QueryInterface(Q **pp) { return QueryInterface(__uuidof(Q), (void **)pp); } - -private: - std::atomic m_count; }; CROSS_PLATFORM_UUIDOF(INoMarshal, "ECC8691B-C1DB-4DC0-855E-65F6C551AF49") @@ -632,10 +629,9 @@ struct INoMarshal : public IUnknown {}; CROSS_PLATFORM_UUIDOF(IMalloc, "00000002-0000-0000-C000-000000000046") struct IMalloc : public IUnknown { - virtual void *Alloc(size_t size); - virtual void *Realloc(void *ptr, size_t size); - virtual void Free(void *ptr); - virtual HRESULT QueryInterface(REFIID riid, void **ppvObject); + virtual void *Alloc(size_t size) = 0; + virtual void *Realloc(void *ptr, size_t size) = 0; + virtual void Free(void *ptr) = 0; }; CROSS_PLATFORM_UUIDOF(ISequentialStream, "0C733A30-2A1C-11CE-ADE5-00AA0044773D") diff --git a/include/dxc/Test/CompilationResult.h b/include/dxc/Test/CompilationResult.h index 50f5e7c6b3..630e46d603 100644 --- a/include/dxc/Test/CompilationResult.h +++ b/include/dxc/Test/CompilationResult.h @@ -49,7 +49,7 @@ inline HRESULT GetFirstChildFromCursor(IDxcCursor *cursor, return hr; } -class TrivialDxcUnsavedFile : IDxcUnsavedFile +class TrivialDxcUnsavedFile final : IDxcUnsavedFile { private: volatile std::atomic m_dwRef; diff --git a/lib/DxcSupport/FileIOHelper.cpp b/lib/DxcSupport/FileIOHelper.cpp index 6120a0016a..6dda054367 100644 --- a/lib/DxcSupport/FileIOHelper.cpp +++ b/lib/DxcSupport/FileIOHelper.cpp @@ -37,20 +37,20 @@ // Alias for CP_UTF16LE, which is the only one we actually handle. #define CP_UTF16 CP_UTF16LE -struct HeapMalloc : public IMalloc { +struct HeapMalloc final : public IMalloc { public: ULONG STDMETHODCALLTYPE AddRef() override { return 1; } ULONG STDMETHODCALLTYPE Release() override { return 1; } STDMETHODIMP QueryInterface(REFIID iid, void** ppvObject) override { return DoBasicQueryInterface(this, iid, ppvObject); } - virtual void *STDMETHODCALLTYPE Alloc ( + void *STDMETHODCALLTYPE Alloc ( /* [annotation][in] */ _In_ SIZE_T cb) override { return HeapAlloc(GetProcessHeap(), 0, cb); } - virtual void *STDMETHODCALLTYPE Realloc ( + void *STDMETHODCALLTYPE Realloc ( /* [annotation][in] */ _In_opt_ void *pv, /* [annotation][in] */ @@ -59,30 +59,28 @@ struct HeapMalloc : public IMalloc { return HeapReAlloc(GetProcessHeap(), 0, pv, cb); } - virtual void STDMETHODCALLTYPE Free ( + void STDMETHODCALLTYPE Free ( /* [annotation][in] */ _In_opt_ void *pv) override { HeapFree(GetProcessHeap(), 0, pv); } - - virtual SIZE_T STDMETHODCALLTYPE GetSize( + SIZE_T STDMETHODCALLTYPE GetSize( /* [annotation][in] */ _In_opt_ _Post_writable_byte_size_(return) void *pv) { return HeapSize(GetProcessHeap(), 0, pv); } - virtual int STDMETHODCALLTYPE DidAlloc( + int STDMETHODCALLTYPE DidAlloc( /* [annotation][in] */ _In_opt_ void *pv) { return -1; // don't know } - - virtual void STDMETHODCALLTYPE HeapMinimize(void) {} + void STDMETHODCALLTYPE HeapMinimize(void) override {} }; static HeapMalloc g_HeapMalloc; diff --git a/lib/DxcSupport/WinAdapter.cpp b/lib/DxcSupport/WinAdapter.cpp index cd65346e07..81a27b5979 100644 --- a/lib/DxcSupport/WinAdapter.cpp +++ b/lib/DxcSupport/WinAdapter.cpp @@ -12,29 +12,6 @@ #include "dxc/Support/WinAdapter.h" #include "dxc/Support/WinFunctions.h" -//===--------------------------- IUnknown ---------------------------------===// - -ULONG IUnknown::AddRef() { - ++m_count; - return m_count; -} -ULONG IUnknown::Release() { - ULONG result = --m_count; - if (m_count == 0) { - delete this; - } - return result; -} -//===--------------------------- IMalloc ----------------------------------===// - -void *IMalloc::Alloc(size_t size) { return malloc(size); } -void *IMalloc::Realloc(void *ptr, size_t size) { return realloc(ptr, size); } -void IMalloc::Free(void *ptr) { free(ptr); } -HRESULT IMalloc::QueryInterface(REFIID riid, void **ppvObject) { - assert(false && "QueryInterface not implemented for IMalloc."); - return E_NOINTERFACE; -} - //===--------------------------- CAllocator -------------------------------===// void *CAllocator::Reallocate(void *p, size_t nBytes) throw() { diff --git a/lib/DxcSupport/WinFunctions.cpp b/lib/DxcSupport/WinFunctions.cpp index bf2b25e4f0..56dc08e06a 100644 --- a/lib/DxcSupport/WinFunctions.cpp +++ b/lib/DxcSupport/WinFunctions.cpp @@ -20,6 +20,7 @@ #include #include "dxc/Support/WinFunctions.h" +#include "dxc/Support/microcom.h" HRESULT StringCchCopyEx(LPSTR pszDest, size_t cchDest, LPCSTR pszSrc, LPSTR *ppszDestEnd, size_t *pcchRemaining, DWORD dwFlags) { @@ -154,8 +155,25 @@ unsigned char _BitScanForward(unsigned long * Index, unsigned long Mask) { return 1; } +struct CoMalloc final : public IMalloc { + CoMalloc() : m_dwRef(0) {}; + + DXC_MICROCOM_ADDREF_RELEASE_IMPL(m_dwRef) + STDMETHODIMP QueryInterface(REFIID riid, void **ppvObject) override { + assert(false && "QueryInterface not implemented for CoMalloc."); + return E_NOINTERFACE; + } + + void *STDMETHODCALLTYPE Alloc(size_t size) override { return malloc(size); } + void *STDMETHODCALLTYPE Realloc(void *ptr, size_t size) override { return realloc(ptr, size); } + void STDMETHODCALLTYPE Free(void *ptr) override { free(ptr); } + +private: + DXC_MICROCOM_REF_FIELD(m_dwRef) +}; + HRESULT CoGetMalloc(DWORD dwMemContext, IMalloc **ppMalloc) { - *ppMalloc = new IMalloc; + *ppMalloc = new CoMalloc; (*ppMalloc)->AddRef(); return S_OK; } diff --git a/tools/clang/tools/dxclib/dxc.cpp b/tools/clang/tools/dxclib/dxc.cpp index 64a0833c47..efe8ebb121 100644 --- a/tools/clang/tools/dxclib/dxc.cpp +++ b/tools/clang/tools/dxclib/dxc.cpp @@ -604,7 +604,7 @@ int DxcContext::VerifyRootSignature() { } } -class DxcIncludeHandlerForInjectedSources : public IDxcIncludeHandler { +class DxcIncludeHandlerForInjectedSources final : public IDxcIncludeHandler { private: DXC_MICROCOM_REF_FIELD(m_dwRef) @@ -707,7 +707,8 @@ void DxcContext::Recompile(IDxcBlob *pSource, IDxcLibrary *pLibrary, IFT(pPdbUtils->GetEntryPoint(&pEntryPoint)); CComPtr pCompileSource; - CComPtr pIncludeHandler = new DxcIncludeHandlerForInjectedSources(); + DxcIncludeHandlerForInjectedSources *pIncludeHandlerForInjectedSources = new DxcIncludeHandlerForInjectedSources(); + CComPtr pIncludeHandler = pIncludeHandlerForInjectedSources; UINT32 uSourceCount = 0; IFT(pPdbUtils->GetSourceCount(&uSourceCount)); for (UINT32 i = 0; i < uSourceCount; i++) { @@ -715,7 +716,7 @@ void DxcContext::Recompile(IDxcBlob *pSource, IDxcLibrary *pLibrary, CComBSTR pFileName; IFT(pPdbUtils->GetSource(i, &pSourceFile)); IFT(pPdbUtils->GetSourceName(i, &pFileName)); - IFT(pIncludeHandler->insertIncludeFile(pFileName, pSourceFile, 0)); + IFT(pIncludeHandlerForInjectedSources->insertIncludeFile(pFileName, pSourceFile, 0)); if (pMainFileName == pFileName) { pCompileSource.Attach(pSourceFile); } diff --git a/tools/clang/tools/libclang/dxcisenseimpl.cpp b/tools/clang/tools/libclang/dxcisenseimpl.cpp index d2f1bcdbb9..1b0bd000bb 100644 --- a/tools/clang/tools/libclang/dxcisenseimpl.cpp +++ b/tools/clang/tools/libclang/dxcisenseimpl.cpp @@ -51,7 +51,7 @@ HRESULT CreateDxcIntelliSense(_In_ REFIID riid, _Out_ LPVOID* ppv) throw() // This is exposed as a helper class, but the implementation works on // interfaces; we expect callers should be able to use their own. -class DxcBasicUnsavedFile : public IDxcUnsavedFile +class DxcBasicUnsavedFile final : public IDxcUnsavedFile { private: DXC_MICROCOM_TM_REF_FIELDS() diff --git a/tools/clang/unittests/HLSL/ExtensionTest.cpp b/tools/clang/unittests/HLSL/ExtensionTest.cpp index e9a28fab70..d93dca96d7 100644 --- a/tools/clang/unittests/HLSL/ExtensionTest.cpp +++ b/tools/clang/unittests/HLSL/ExtensionTest.cpp @@ -291,7 +291,7 @@ class IntrinsicTable { } }; -class TestIntrinsicTable : public IDxcIntrinsicTable { +class TestIntrinsicTable final : public IDxcIntrinsicTable { private: DXC_MICROCOM_REF_FIELD(m_dwRef) std::vector m_tables; @@ -395,7 +395,7 @@ class TestIntrinsicTable : public IDxcIntrinsicTable { // and defines that should cause warnings. A more realistic validator // would look at the values and make sure (for example) they are // the correct type (integer, string, etc). -class TestSemanticDefineValidator : public IDxcSemanticDefineValidator { +class TestSemanticDefineValidator final : public IDxcSemanticDefineValidator { private: DXC_MICROCOM_REF_FIELD(m_dwRef) std::vector m_errorDefines;