Winadapter iunknown no virtual dtor (#1)

* 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 <DxcLibrary::QueryInterface(_GUID const&, void**)>
    [1]: 0x7ffff6a56d20 <DxcLibrary::AddRef()>
    [2]: 0x7ffff6a56d30 <DxcLibrary::Release()>
    [3]: 0x7ffff6b36bc0 <IUnknown::~IUnknown()> // Complete object destructor
    [4]: 0x7ffff6a57130 <DxcLibrary::~DxcLibrary()> // Deleting destructor
    [5]: 0x7ffff6a56d50 <DxcLibrary::SetMalloc(IMalloc*)>
    [6]: 0x7ffff6a56d60 <DxcLibrary::CreateBlobFromBlob(IDxcBlob*, unsigned int, unsigned int, IDxcBlob**)>
    ... 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]: microsoft#3783 (comment)
[2]: https://godbolt.org/z/hKPT6ThEf

* 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]: microsoft#3793 (comment)

* 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

Co-authored-by: Marijn Suijten <marijns95@gmail.com>

cmake/modules/HandleLLVMOptions.cmake

@@ -399,19 +399,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.

include/dxc/Support/WinAdapter.h

@@ -615,28 +615,26 @@ template <typename T> 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 ~IUnknown();
+  virtual ULONG AddRef() = 0;
+  virtual ULONG Release() = 0;
   template <class Q> HRESULT QueryInterface(Q **pp) {
     return QueryInterface(__uuidof(Q), (void **)pp);
   }
-
-private:
-  std::atomic<unsigned long> m_count;
 };
 
 CROSS_PLATFORM_UUIDOF(INoMarshal, "ECC8691B-C1DB-4DC0-855E-65F6C551AF49")
 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;
+  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")

include/dxc/Support/microcom.h

@@ -101,7 +101,7 @@ inline T *CreateOnMalloc(IMalloc * pMalloc, Args&&... args) {
 
 template<typename T>
 void DxcCallDestructor(T *obj) {
-  obj->~T();
+  obj->T::~T();
 }
 
 // The "TM" version keep an IMalloc field that, if not null, indicate

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<llvm::sys::cas_flag> m_dwRef;

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<IMalloc>(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)
+    _In_opt_ _Post_writable_byte_size_(return)  void *pv) override
   {
     return HeapSize(GetProcessHeap(), 0, pv);
   }
 
-  virtual int STDMETHODCALLTYPE DidAlloc(
+  int STDMETHODCALLTYPE DidAlloc(
     /* [annotation][in] */
-    _In_opt_  void *pv)
+    _In_opt_  void *pv) override
   {
     return -1; // don't know
   }
 
-
-  virtual void STDMETHODCALLTYPE HeapMinimize(void) {}
+  void STDMETHODCALLTYPE HeapMinimize(void) override {}
 };
 
 static HeapMalloc g_HeapMalloc;
@@ -319,7 +317,7 @@ class InternalDxcBlobEncoding_Impl : public _T {
     ULONG result = (ULONG)--m_dwRef;
     if (result == 0) {
       CComPtr<IMalloc> pTmp(m_pMalloc);
-      this->~InternalDxcBlobEncoding_Impl();
+      this->InternalDxcBlobEncoding_Impl::~InternalDxcBlobEncoding_Impl();
       pTmp->Free(this);
     }
     return result;
@@ -1124,7 +1122,7 @@ class MemoryStream : public AbstractMemoryStream, public IDxcBlob {
     ULONG result = (ULONG)--m_dwRef;
     if (result == 0) {
       CComPtr<IMalloc> pTmp(m_pMalloc);
-      this->~MemoryStream();
+      this->MemoryStream::~MemoryStream();
       pTmp->Free(this);
     }
     return result;

lib/DxcSupport/WinAdapter.cpp

@@ -12,31 +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;
-}
-IUnknown::~IUnknown() {}
-
-//===--------------------------- 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() {

lib/DxcSupport/WinFunctions.cpp

@@ -20,6 +20,7 @@
 #include <unistd.h>
 
 #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,28 @@ 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); }
+  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)
+};
+
 HRESULT CoGetMalloc(DWORD dwMemContext, IMalloc **ppMalloc) {
-  *ppMalloc = new IMalloc;
+  *ppMalloc = new CoMalloc;
   (*ppMalloc)->AddRef();
   return S_OK;
 }

tools/clang/tools/dxclib/dxc.cpp

@@ -583,7 +583,7 @@ int DxcContext::VerifyRootSignature() {
   }
 }
 
-class DxcIncludeHandlerForInjectedSources : public IDxcIncludeHandler {
+class DxcIncludeHandlerForInjectedSources final : public IDxcIncludeHandler {
 private:
   DXC_MICROCOM_REF_FIELD(m_dwRef)
 
@@ -686,15 +686,16 @@ void DxcContext::Recompile(IDxcBlob *pSource, IDxcLibrary *pLibrary,
   IFT(pPdbUtils->GetEntryPoint(&pEntryPoint));
 
   CComPtr<IDxcBlobEncoding> pCompileSource;
-  CComPtr<DxcIncludeHandlerForInjectedSources> pIncludeHandler = new DxcIncludeHandlerForInjectedSources();
+  DxcIncludeHandlerForInjectedSources *pIncludeHandlerForInjectedSources = new DxcIncludeHandlerForInjectedSources();
+  CComPtr<IDxcIncludeHandler> pIncludeHandler = pIncludeHandlerForInjectedSources;
   UINT32 uSourceCount = 0;
   IFT(pPdbUtils->GetSourceCount(&uSourceCount));
   for (UINT32 i = 0; i < uSourceCount; i++) {
     CComPtr<IDxcBlobEncoding> pSourceFile;
     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);
     }

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()

tools/clang/unittests/HLSL/CompilerTest.cpp

@@ -2720,17 +2720,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<IMalloc>(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
@@ -2752,7 +2752,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)
@@ -2763,7 +2763,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);
@@ -2779,18 +2779,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;;

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<IntrinsicTable> 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<std::string> m_errorDefines;