From 9beaf218f1be5a4ab4a3a61b2658904513da355f Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Wed, 19 May 2021 10:24:25 +0200 Subject: [PATCH 1/4] WinAdapter: Remove virtual dtors from IUnknown to fix vtable ABI The vtable for `IUnknown` and its subclasses contain two deletion pointers when compiled on non-Windows systems with `IUnknown` from `WinAdapter.h`: vtable for 'DxcLibrary' @ 0x7ffff7cbc5f8 (subobject @ 0x5555556bb9e0): [0]: 0x7ffff6a56d40 [1]: 0x7ffff6a56d20 [2]: 0x7ffff6a56d30 [3]: 0x7ffff6b36bc0 // Complete object destructor [4]: 0x7ffff6a57130 // Deleting destructor [5]: 0x7ffff6a56d50 [6]: 0x7ffff6a56d60 ... More DxcLibrary virtual functions This shifts the the pointers for functions for all subclasses, and is [annoying] to deal with in otherwise cross-platform applications using DirectXShaderCompiler as library. `dxcompiler.dll` compiled on/for Windows without `WinAdapter.h` does not suffer this problem, and only has three function pointers for `IUnknown`. Fortunately it is easily solved by removing the virtual destructor from `IUnknown`. LLVM enables `-Wnon-virtual-dtor` that warns against classes with virtual methods but no virtual destructor, though this warning is best not enabled akin to Windows builds where `IUnknown` from `windows.h` (`unknwn.h`) results in the same warning on MSVC ([1]/[2]). [annoying]: https://github.com/Traverse-Research/hassle-rs/blob/1e624792fc3a252ac7788e3c1c5feda52887272f/src/unknown.rs [1]: https://github.com/microsoft/DirectXShaderCompiler/issues/3783#issuecomment-844189358 [2]: https://godbolt.org/z/hKPT6ThEf --- cmake/modules/HandleLLVMOptions.cmake | 38 ++++++++++++++++++--------- include/dxc/Support/WinAdapter.h | 1 - include/dxc/Support/microcom.h | 2 +- lib/DxcSupport/FileIOHelper.cpp | 4 +-- lib/DxcSupport/WinAdapter.cpp | 2 -- 5 files changed, 28 insertions(+), 19 deletions(-) diff --git a/cmake/modules/HandleLLVMOptions.cmake b/cmake/modules/HandleLLVMOptions.cmake index 6d19b1217e..8651d1b2d2 100644 --- a/cmake/modules/HandleLLVMOptions.cmake +++ b/cmake/modules/HandleLLVMOptions.cmake @@ -400,19 +400,31 @@ elseif( LLVM_COMPILER_IS_GCC_COMPATIBLE ) append_if(USE_NO_UNINITIALIZED "-Wno-uninitialized" CMAKE_CXX_FLAGS) append_if(USE_NO_MAYBE_UNINITIALIZED "-Wno-maybe-uninitialized" CMAKE_CXX_FLAGS) - # Check if -Wnon-virtual-dtor warns even though the class is marked final. - # If it does, don't add it. So it won't be added on clang 3.4 and older. - # This also catches cases when -Wnon-virtual-dtor isn't supported by - # the compiler at all. - set(OLD_CMAKE_REQUIRED_FLAGS ${CMAKE_REQUIRED_FLAGS}) - set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} -std=c++11 -Werror=non-virtual-dtor") - CHECK_CXX_SOURCE_COMPILES("class base {public: virtual void anchor();protected: ~base();}; - class derived final : public base { public: ~derived();}; - int main() { return 0; }" - CXX_WONT_WARN_ON_FINAL_NONVIRTUALDTOR) - set(CMAKE_REQUIRED_FLAGS ${OLD_CMAKE_REQUIRED_FLAGS}) - append_if(CXX_WONT_WARN_ON_FINAL_NONVIRTUALDTOR - "-Wnon-virtual-dtor" CMAKE_CXX_FLAGS) + # HLSL Change Starts + + # Windows' and by extension WinAdapter's non-Windows implementation for IUnknown + # use virtual methods without virtual destructor, as that would add two extra + # function-pointers to the vtable in turn offsetting those for every subclass, + # resulting in ABI mismatches: + # https://github.com/microsoft/DirectXShaderCompiler/issues/3783. + # The -Wnon-virtual-dtor warning is disabled to allow this, conforming + # with MSVC behaviour. + + # # Check if -Wnon-virtual-dtor warns even though the class is marked final. + # # If it does, don't add it. So it won't be added on clang 3.4 and older. + # # This also catches cases when -Wnon-virtual-dtor isn't supported by + # # the compiler at all. + # set(OLD_CMAKE_REQUIRED_FLAGS ${CMAKE_REQUIRED_FLAGS}) + # set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} -std=c++11 -Werror=non-virtual-dtor") + # CHECK_CXX_SOURCE_COMPILES("class base {public: virtual void anchor();protected: ~base();}; + # class derived final : public base { public: ~derived();}; + # int main() { return 0; }" + # CXX_WONT_WARN_ON_FINAL_NONVIRTUALDTOR) + # set(CMAKE_REQUIRED_FLAGS ${OLD_CMAKE_REQUIRED_FLAGS}) + # append_if(CXX_WONT_WARN_ON_FINAL_NONVIRTUALDTOR + # "-Wnon-virtual-dtor" CMAKE_CXX_FLAGS) + + # HLSL Change Ends # Check if -Wcomment is OK with an // comment ending with '\' if the next # line is also a // comment. diff --git a/include/dxc/Support/WinAdapter.h b/include/dxc/Support/WinAdapter.h index 354442fecc..31cfa7cd59 100644 --- a/include/dxc/Support/WinAdapter.h +++ b/include/dxc/Support/WinAdapter.h @@ -618,7 +618,6 @@ struct IUnknown { virtual HRESULT QueryInterface(REFIID riid, void **ppvObject) = 0; virtual ULONG AddRef(); virtual ULONG Release(); - virtual ~IUnknown(); template HRESULT QueryInterface(Q **pp) { return QueryInterface(__uuidof(Q), (void **)pp); } diff --git a/include/dxc/Support/microcom.h b/include/dxc/Support/microcom.h index 4cc3cc9103..a3c6865d80 100644 --- a/include/dxc/Support/microcom.h +++ b/include/dxc/Support/microcom.h @@ -101,7 +101,7 @@ inline T *CreateOnMalloc(IMalloc * pMalloc, Args&&... args) { template void DxcCallDestructor(T *obj) { - obj->~T(); + obj->T::~T(); } // The "TM" version keep an IMalloc field that, if not null, indicate diff --git a/lib/DxcSupport/FileIOHelper.cpp b/lib/DxcSupport/FileIOHelper.cpp index 3644b90ca8..c4153ebf5c 100644 --- a/lib/DxcSupport/FileIOHelper.cpp +++ b/lib/DxcSupport/FileIOHelper.cpp @@ -321,7 +321,7 @@ class InternalDxcBlobEncoding_Impl : public _T { ULONG result = (ULONG)--m_dwRef; if (result == 0) { CComPtr pTmp(m_pMalloc); - this->~InternalDxcBlobEncoding_Impl(); + this->InternalDxcBlobEncoding_Impl::~InternalDxcBlobEncoding_Impl(); pTmp->Free(this); } return result; @@ -1138,7 +1138,7 @@ class MemoryStream : public AbstractMemoryStream, public IDxcBlob { ULONG result = (ULONG)--m_dwRef; if (result == 0) { CComPtr pTmp(m_pMalloc); - this->~MemoryStream(); + this->MemoryStream::~MemoryStream(); pTmp->Free(this); } return result; diff --git a/lib/DxcSupport/WinAdapter.cpp b/lib/DxcSupport/WinAdapter.cpp index 27bd7c3c7e..cd65346e07 100644 --- a/lib/DxcSupport/WinAdapter.cpp +++ b/lib/DxcSupport/WinAdapter.cpp @@ -25,8 +25,6 @@ ULONG IUnknown::Release() { } return result; } -IUnknown::~IUnknown() {} - //===--------------------------- IMalloc ----------------------------------===// void *IMalloc::Alloc(size_t size) { return malloc(size); } From 794e21e1efef6dce742011100aa4265e25b3d84b Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Sat, 22 May 2021 22:59:49 +0200 Subject: [PATCH 2/4] 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/CompilerTest.cpp | 2 +- tools/clang/unittests/HLSL/ExtensionTest.cpp | 4 ++-- 9 files changed, 41 insertions(+), 51 deletions(-) diff --git a/include/dxc/Support/WinAdapter.h b/include/dxc/Support/WinAdapter.h index 31cfa7cd59..50936df51e 100644 --- a/include/dxc/Support/WinAdapter.h +++ b/include/dxc/Support/WinAdapter.h @@ -614,16 +614,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") @@ -631,10 +628,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 c4153ebf5c..23fdbb4632 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 20b4822bb0..5833cdf37f 100644 --- a/tools/clang/tools/dxclib/dxc.cpp +++ b/tools/clang/tools/dxclib/dxc.cpp @@ -605,7 +605,7 @@ int DxcContext::VerifyRootSignature() { } } -class DxcIncludeHandlerForInjectedSources : public IDxcIncludeHandler { +class DxcIncludeHandlerForInjectedSources final : public IDxcIncludeHandler { private: DXC_MICROCOM_REF_FIELD(m_dwRef) @@ -708,7 +708,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++) { @@ -716,7 +717,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/CompilerTest.cpp b/tools/clang/unittests/HLSL/CompilerTest.cpp index e2c9ee5fd4..4618598dde 100644 --- a/tools/clang/unittests/HLSL/CompilerTest.cpp +++ b/tools/clang/unittests/HLSL/CompilerTest.cpp @@ -61,7 +61,7 @@ using namespace std; using namespace hlsl_test; -class TestIncludeHandler : public IDxcIncludeHandler { +class TestIncludeHandler final : public IDxcIncludeHandler { DXC_MICROCOM_REF_FIELD(m_dwRef) public: DXC_MICROCOM_ADDREF_RELEASE_IMPL(m_dwRef) diff --git a/tools/clang/unittests/HLSL/ExtensionTest.cpp b/tools/clang/unittests/HLSL/ExtensionTest.cpp index 4a3760b1d1..f04032fda5 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; From 18b6edbcde30e335e328c57eec9bed6b902ccf4c Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Sun, 23 May 2021 01:05:08 +0200 Subject: [PATCH 3/4] WinAdapter: Add three missing virtual functions to `IMalloc` interface To prevent unexpected vtable breakage, add the missing functions from the [documentation]. Note that they are listed in the wrong order, the right order is retrieved from the `ObjIdl.h` header and implementations for `IMalloc` in DirectXShaderCompiler. All implementations are now properly using the `override` keyword too, to enforce virtual method existence in the base class. [documentation]: https://docs.microsoft.com/en-us/windows/win32/api/objidl/nn-objidl-imalloc --- include/dxc/Support/WinAdapter.h | 3 +++ lib/DxcSupport/FileIOHelper.cpp | 4 ++-- lib/DxcSupport/WinFunctions.cpp | 3 +++ tools/clang/unittests/HLSL/CompilerTest.cpp | 18 +++++++++--------- 4 files changed, 17 insertions(+), 11 deletions(-) diff --git a/include/dxc/Support/WinAdapter.h b/include/dxc/Support/WinAdapter.h index 50936df51e..bb2a51cb54 100644 --- a/include/dxc/Support/WinAdapter.h +++ b/include/dxc/Support/WinAdapter.h @@ -631,6 +631,9 @@ struct IMalloc : public IUnknown { virtual void *Alloc(size_t size) = 0; virtual void *Realloc(void *ptr, size_t size) = 0; virtual void Free(void *ptr) = 0; + virtual size_t GetSize(void *pv) = 0; + virtual int DidAlloc(void *pv) = 0; + virtual void HeapMinimize(void) = 0; }; CROSS_PLATFORM_UUIDOF(ISequentialStream, "0C733A30-2A1C-11CE-ADE5-00AA0044773D") diff --git a/lib/DxcSupport/FileIOHelper.cpp b/lib/DxcSupport/FileIOHelper.cpp index 23fdbb4632..cbfc05d14c 100644 --- a/lib/DxcSupport/FileIOHelper.cpp +++ b/lib/DxcSupport/FileIOHelper.cpp @@ -68,14 +68,14 @@ struct HeapMalloc final : public IMalloc { SIZE_T STDMETHODCALLTYPE GetSize( /* [annotation][in] */ - _In_opt_ _Post_writable_byte_size_(return) void *pv) + _In_opt_ _Post_writable_byte_size_(return) void *pv) override { return HeapSize(GetProcessHeap(), 0, pv); } int STDMETHODCALLTYPE DidAlloc( /* [annotation][in] */ - _In_opt_ void *pv) + _In_opt_ void *pv) override { return -1; // don't know } diff --git a/lib/DxcSupport/WinFunctions.cpp b/lib/DxcSupport/WinFunctions.cpp index 56dc08e06a..299d7197ba 100644 --- a/lib/DxcSupport/WinFunctions.cpp +++ b/lib/DxcSupport/WinFunctions.cpp @@ -167,6 +167,9 @@ struct CoMalloc final : public IMalloc { 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); } + size_t STDMETHODCALLTYPE GetSize(void *pv) override { return -1; } + int STDMETHODCALLTYPE DidAlloc(void *pv) override { return -1; } + void STDMETHODCALLTYPE HeapMinimize(void) override {} private: DXC_MICROCOM_REF_FIELD(m_dwRef) diff --git a/tools/clang/unittests/HLSL/CompilerTest.cpp b/tools/clang/unittests/HLSL/CompilerTest.cpp index 4618598dde..6003cbcac6 100644 --- a/tools/clang/unittests/HLSL/CompilerTest.cpp +++ b/tools/clang/unittests/HLSL/CompilerTest.cpp @@ -3073,17 +3073,17 @@ struct InstrumentedHeapMalloc : public IMalloc { m_FailAlloc = index; } - ULONG STDMETHODCALLTYPE AddRef() { + ULONG STDMETHODCALLTYPE AddRef() override { return ++m_RefCount; } - ULONG STDMETHODCALLTYPE Release() { + ULONG STDMETHODCALLTYPE Release() override { if (m_RefCount == 0) VERIFY_FAIL(); return --m_RefCount; } - STDMETHODIMP QueryInterface(REFIID iid, void** ppvObject) { + STDMETHODIMP QueryInterface(REFIID iid, void** ppvObject) override { return DoBasicQueryInterface(this, iid, ppvObject); } - virtual void *STDMETHODCALLTYPE Alloc(_In_ SIZE_T cb) { + virtual void *STDMETHODCALLTYPE Alloc(_In_ SIZE_T cb) override { ++m_AllocCount; if (m_FailAlloc && m_AllocCount >= m_FailAlloc) { return nullptr; // breakpoint for i failure - m_FailAlloc == 1+VAL @@ -3107,7 +3107,7 @@ struct InstrumentedHeapMalloc : public IMalloc { return P + 1; } - virtual void *STDMETHODCALLTYPE Realloc(_In_opt_ void *pv, _In_ SIZE_T cb) { + virtual void *STDMETHODCALLTYPE Realloc(_In_opt_ void *pv, _In_ SIZE_T cb) override { SIZE_T priorSize = pv == nullptr ? (SIZE_T)0 : GetSize(pv); void *R = Alloc(cb); if (!R) @@ -3118,7 +3118,7 @@ struct InstrumentedHeapMalloc : public IMalloc { return R; } - virtual void STDMETHODCALLTYPE Free(_In_opt_ void *pv) { + virtual void STDMETHODCALLTYPE Free(_In_opt_ void *pv) override { if (!pv) return; PtrData *P = DataFromPtr(pv); @@ -3136,18 +3136,18 @@ struct InstrumentedHeapMalloc : public IMalloc { virtual SIZE_T STDMETHODCALLTYPE GetSize( /* [annotation][in] */ - _In_opt_ _Post_writable_byte_size_(return) void *pv) + _In_opt_ _Post_writable_byte_size_(return) void *pv) override { if (pv == nullptr) return 0; return DataFromPtr(pv)->Size; } virtual int STDMETHODCALLTYPE DidAlloc( - _In_opt_ void *pv) { + _In_opt_ void *pv) override { return -1; // don't know } - virtual void STDMETHODCALLTYPE HeapMinimize(void) {} + virtual void STDMETHODCALLTYPE HeapMinimize(void) override {} void DumpLeaks() { PtrData *ptr = (PtrData*)AllocList.Flink;; From 322b18194cec047787c979c690b3b54d7b3ed797 Mon Sep 17 00:00:00 2001 From: Greg Roth Date: Mon, 24 Jan 2022 19:34:54 -0700 Subject: [PATCH 4/4] Make all WinAdapter destructions explicit This prevents warnings about non-virtual destructor usage that trip up the Linux build. It represents status quo on Windows. --- include/dxc/Support/microcom.h | 16 ++++++++------- include/dxc/Test/CompilationResult.h | 21 +++++++++----------- lib/DxcSupport/FileIOHelper.cpp | 2 +- lib/DxcSupport/WinFunctions.cpp | 2 +- tools/clang/tools/dxclib/dxc.cpp | 7 +++---- tools/clang/tools/libclang/dxcisenseimpl.cpp | 6 ++++-- tools/clang/unittests/HLSL/CompilerTest.cpp | 2 +- tools/clang/unittests/HLSL/ExtensionTest.cpp | 4 ++-- 8 files changed, 30 insertions(+), 30 deletions(-) diff --git a/include/dxc/Support/microcom.h b/include/dxc/Support/microcom.h index a3c6865d80..5d80d252f5 100644 --- a/include/dxc/Support/microcom.h +++ b/include/dxc/Support/microcom.h @@ -76,6 +76,11 @@ class CComInterfaceArray { } }; +template +void DxcCallDestructor(T *obj) { + obj->T::~T(); +} + #define DXC_MICROCOM_REF_FIELD(m_dwRef) \ volatile std::atomic m_dwRef = {0}; #define DXC_MICROCOM_ADDREF_IMPL(m_dwRef) \ @@ -86,8 +91,10 @@ class CComInterfaceArray { DXC_MICROCOM_ADDREF_IMPL(m_dwRef) \ ULONG STDMETHODCALLTYPE Release() override { \ ULONG result = (ULONG)--m_dwRef; \ - if (result == 0) \ - delete this; \ + if (result == 0) { \ + DxcCallDestructor(this); \ + operator delete(this); \ + } \ return result; \ } @@ -99,11 +106,6 @@ inline T *CreateOnMalloc(IMalloc * pMalloc, Args&&... args) { return (T *)P; } -template -void DxcCallDestructor(T *obj) { - obj->T::~T(); -} - // The "TM" version keep an IMalloc field that, if not null, indicate // ownership of 'this' and of any allocations used during release. #define DXC_MICROCOM_TM_REF_FIELDS() \ diff --git a/include/dxc/Test/CompilationResult.h b/include/dxc/Test/CompilationResult.h index 630e46d603..fdc193c522 100644 --- a/include/dxc/Test/CompilationResult.h +++ b/include/dxc/Test/CompilationResult.h @@ -20,6 +20,7 @@ #include #include "dxc/Support/WinIncludes.h" +#include "dxc/Support/microcom.h" #include "dxc/dxcapi.h" #include "dxc/dxcisense.h" @@ -49,14 +50,15 @@ inline HRESULT GetFirstChildFromCursor(IDxcCursor *cursor, return hr; } -class TrivialDxcUnsavedFile final : IDxcUnsavedFile +class TrivialDxcUnsavedFile : public IDxcUnsavedFile { private: - volatile std::atomic m_dwRef; + DXC_MICROCOM_REF_FIELD(m_dwRef) LPCSTR m_fileName; LPCSTR m_contents; unsigned m_length; public: + DXC_MICROCOM_ADDREF_RELEASE_IMPL(m_dwRef) TrivialDxcUnsavedFile(LPCSTR fileName, LPCSTR contents) : m_dwRef(0), m_fileName(fileName), m_contents(contents) { @@ -68,13 +70,8 @@ class TrivialDxcUnsavedFile final : IDxcUnsavedFile CComPtr pNewValue = new TrivialDxcUnsavedFile(fileName, contents); return pNewValue.QueryInterface(pResult); } - ULONG STDMETHODCALLTYPE AddRef() { return (ULONG)++m_dwRef; } - ULONG STDMETHODCALLTYPE Release() { - ULONG result = (ULONG)--m_dwRef; - if (result == 0) delete this; - return result; - } - HRESULT STDMETHODCALLTYPE QueryInterface(REFIID iid, void** ppvObject) + + HRESULT STDMETHODCALLTYPE QueryInterface(REFIID iid, void** ppvObject) override { if (ppvObject == nullptr) return E_POINTER; if (IsEqualIID(iid, __uuidof(IUnknown)) || @@ -88,19 +85,19 @@ class TrivialDxcUnsavedFile final : IDxcUnsavedFile return E_NOINTERFACE; } - HRESULT STDMETHODCALLTYPE GetFileName(LPSTR* pFileName) + HRESULT STDMETHODCALLTYPE GetFileName(LPSTR* pFileName) override { *pFileName = (LPSTR)CoTaskMemAlloc(1 + strlen(m_fileName)); strcpy(*pFileName, m_fileName); return S_OK; } - HRESULT STDMETHODCALLTYPE GetContents(LPSTR* pContents) + HRESULT STDMETHODCALLTYPE GetContents(LPSTR* pContents) override { *pContents = (LPSTR)CoTaskMemAlloc(m_length + 1); memcpy(*pContents, m_contents, m_length + 1); return S_OK; } - HRESULT STDMETHODCALLTYPE GetLength(unsigned* pLength) + HRESULT STDMETHODCALLTYPE GetLength(unsigned* pLength) override { *pLength = m_length; return S_OK; diff --git a/lib/DxcSupport/FileIOHelper.cpp b/lib/DxcSupport/FileIOHelper.cpp index cbfc05d14c..dee74841ed 100644 --- a/lib/DxcSupport/FileIOHelper.cpp +++ b/lib/DxcSupport/FileIOHelper.cpp @@ -37,7 +37,7 @@ // Alias for CP_UTF16LE, which is the only one we actually handle. #define CP_UTF16 CP_UTF16LE -struct HeapMalloc final : public IMalloc { +struct HeapMalloc : public IMalloc { public: ULONG STDMETHODCALLTYPE AddRef() override { return 1; } ULONG STDMETHODCALLTYPE Release() override { return 1; } diff --git a/lib/DxcSupport/WinFunctions.cpp b/lib/DxcSupport/WinFunctions.cpp index 299d7197ba..9f55c86604 100644 --- a/lib/DxcSupport/WinFunctions.cpp +++ b/lib/DxcSupport/WinFunctions.cpp @@ -155,7 +155,7 @@ unsigned char _BitScanForward(unsigned long * Index, unsigned long Mask) { return 1; } -struct CoMalloc final : public IMalloc { +struct CoMalloc : public IMalloc { CoMalloc() : m_dwRef(0) {}; DXC_MICROCOM_ADDREF_RELEASE_IMPL(m_dwRef) diff --git a/tools/clang/tools/dxclib/dxc.cpp b/tools/clang/tools/dxclib/dxc.cpp index 5833cdf37f..20b4822bb0 100644 --- a/tools/clang/tools/dxclib/dxc.cpp +++ b/tools/clang/tools/dxclib/dxc.cpp @@ -605,7 +605,7 @@ int DxcContext::VerifyRootSignature() { } } -class DxcIncludeHandlerForInjectedSources final : public IDxcIncludeHandler { +class DxcIncludeHandlerForInjectedSources : public IDxcIncludeHandler { private: DXC_MICROCOM_REF_FIELD(m_dwRef) @@ -708,8 +708,7 @@ void DxcContext::Recompile(IDxcBlob *pSource, IDxcLibrary *pLibrary, IFT(pPdbUtils->GetEntryPoint(&pEntryPoint)); CComPtr pCompileSource; - DxcIncludeHandlerForInjectedSources *pIncludeHandlerForInjectedSources = new DxcIncludeHandlerForInjectedSources(); - CComPtr pIncludeHandler = pIncludeHandlerForInjectedSources; + CComPtr pIncludeHandler = new DxcIncludeHandlerForInjectedSources(); UINT32 uSourceCount = 0; IFT(pPdbUtils->GetSourceCount(&uSourceCount)); for (UINT32 i = 0; i < uSourceCount; i++) { @@ -717,7 +716,7 @@ void DxcContext::Recompile(IDxcBlob *pSource, IDxcLibrary *pLibrary, CComBSTR pFileName; IFT(pPdbUtils->GetSource(i, &pSourceFile)); IFT(pPdbUtils->GetSourceName(i, &pFileName)); - IFT(pIncludeHandlerForInjectedSources->insertIncludeFile(pFileName, pSourceFile, 0)); + IFT(pIncludeHandler->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 1b0bd000bb..6e02240956 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 final : public IDxcUnsavedFile +class DxcBasicUnsavedFile : public IDxcUnsavedFile { private: DXC_MICROCOM_TM_REF_FIELDS() @@ -581,7 +581,9 @@ HRESULT DxcBasicUnsavedFile::Create( HRESULT hr = newValue->Initialize(fileName, contents, contentLength); if (FAILED(hr)) { - delete newValue; + CComPtr pTmp(newValue->m_pMalloc); + newValue->DxcBasicUnsavedFile::~DxcBasicUnsavedFile(); + pTmp->Free(newValue); return hr; } newValue->AddRef(); diff --git a/tools/clang/unittests/HLSL/CompilerTest.cpp b/tools/clang/unittests/HLSL/CompilerTest.cpp index 6003cbcac6..2743d41ee1 100644 --- a/tools/clang/unittests/HLSL/CompilerTest.cpp +++ b/tools/clang/unittests/HLSL/CompilerTest.cpp @@ -61,7 +61,7 @@ using namespace std; using namespace hlsl_test; -class TestIncludeHandler final : public IDxcIncludeHandler { +class TestIncludeHandler : public IDxcIncludeHandler { DXC_MICROCOM_REF_FIELD(m_dwRef) public: DXC_MICROCOM_ADDREF_RELEASE_IMPL(m_dwRef) diff --git a/tools/clang/unittests/HLSL/ExtensionTest.cpp b/tools/clang/unittests/HLSL/ExtensionTest.cpp index f04032fda5..4a3760b1d1 100644 --- a/tools/clang/unittests/HLSL/ExtensionTest.cpp +++ b/tools/clang/unittests/HLSL/ExtensionTest.cpp @@ -291,7 +291,7 @@ class IntrinsicTable { } }; -class TestIntrinsicTable final : public IDxcIntrinsicTable { +class TestIntrinsicTable : public IDxcIntrinsicTable { private: DXC_MICROCOM_REF_FIELD(m_dwRef) std::vector m_tables; @@ -395,7 +395,7 @@ class TestIntrinsicTable final : 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 final : public IDxcSemanticDefineValidator { +class TestSemanticDefineValidator : public IDxcSemanticDefineValidator { private: DXC_MICROCOM_REF_FIELD(m_dwRef) std::vector m_errorDefines;