[Linux] WinAdapter: Remove virtual dtors from IUnknown to fix vtable ABI (#3793)

* 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]: https://github.com/microsoft/DirectXShaderCompiler/issues/3783#issuecomment-844189358
[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]: https://github.com/microsoft/DirectXShaderCompiler/pull/3793#issuecomment-846459741

* 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

* 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.

Co-authored-by: Greg Roth <[email protected]>

cmake/modules/HandleLLVMOptions.cmake

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

@@ -614,17 +614,13 @@ 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")
@@ -632,10 +628,12 @@ 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

@@ -76,6 +76,11 @@ public:
   }
 };
 
+template<typename T>
+void DxcCallDestructor(T *obj) {
+  obj->T::~T();
+}
+
 #define DXC_MICROCOM_REF_FIELD(m_dwRef)                                        \
   volatile std::atomic<llvm::sys::cas_flag> m_dwRef = {0};
 #define DXC_MICROCOM_ADDREF_IMPL(m_dwRef)                                      \
@@ -86,8 +91,10 @@ public:
   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<typename T>
-void DxcCallDestructor(T *obj) {
-  obj->~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()                                           \

include/dxc/Test/CompilationResult.h

@@ -20,6 +20,7 @@
 #include <atomic>
 
 #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 : IDxcUnsavedFile
+class TrivialDxcUnsavedFile : public IDxcUnsavedFile
 {
 private:
-  volatile std::atomic<llvm::sys::cas_flag> 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 @@ public:
     CComPtr<TrivialDxcUnsavedFile> 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 @@ public:
 
     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;

lib/DxcSupport/FileIOHelper.cpp

@@ -44,13 +44,13 @@ public:
   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 @@ public:
     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;
@@ -321,7 +319,7 @@ public:
     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;
@@ -1138,7 +1136,7 @@ public:
     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 : 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/libclang/dxcisenseimpl.cpp

@@ -581,7 +581,9 @@ HRESULT DxcBasicUnsavedFile::Create(
   HRESULT hr = newValue->Initialize(fileName, contents, contentLength);
   if (FAILED(hr))
   {
-    delete newValue;
+    CComPtr<IMalloc> pTmp(newValue->m_pMalloc);
+    newValue->DxcBasicUnsavedFile::~DxcBasicUnsavedFile();
+    pTmp->Free(newValue);
     return hr;
   }
   newValue->AddRef();

tools/clang/unittests/HLSL/CompilerTest.cpp

@@ -3038,17 +3038,17 @@ public:
     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
@@ -3072,7 +3072,7 @@ public:
     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)
@@ -3083,7 +3083,7 @@ public:
     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);
@@ -3101,18 +3101,18 @@ public:
 
   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;;