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

MarijnS95 · 2021-05-22T14:46:43Z

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

Tested on Clang 11.1.0 and GCC 11.1.0. Unfortunately GCC fails compiling on an irrelevant -Werror=stringop-truncation in SPIRV-Tools, irrespective of this PR.

On Clang this PR still results in quite a few warnings of the sort:

[605/1293] Building CXX object lib/DxcSupport/CMakeFiles/LLVMDxcSupport.dir/WinAdapter.cpp.o
../lib/DxcSupport/WinAdapter.cpp:24:5: warning: delete called on 'IUnknown' that is abstract but has non-virtual destructor [-Wdelete-abstract-non-virtual-dtor]
    delete this;
    ^
1 warning generated.

And it seems the same warning is named -Wdelete-non-virtual-dtor on GCC - but unfortunately surrounded by hundreds upon hundreds of these warnings.

CC @jenatali; thanks for checking this with me and pointing out the warning on MSVC too!

AppVeyorBot · 2021-05-22T15:45:05Z

❌ Build DirectXShaderCompiler 1.0.144 failed (commit 6c3eee293b by @MarijnS95)

jenatali · 2021-05-22T20:29:18Z

Huh. The IUnknown implementation should be pure virtual and shouldn't have the m_count member. I missed that last time I looked at it. I guess you could theoretically rely on that to implement a Linux-only IUnknown-derived class, but anything cross platform would need to define its own count and implement Release() on a more-derived class, since Windows defines IUnknown as pure-virtual with no virtual destructor.

Removing the implementation of AddRef and Release from WinAdapter.cpp will resolve the -Wdelete-non-virtual-dtor error/warning.

MarijnS95 · 2021-05-22T21:10:23Z

@jenatali thanks for the insight!

The IUnknown implementation should be pure virtual and shouldn't have the m_count member. I missed that last time I looked at it. I guess you could theoretically rely on that to implement a Linux-only IUnknown-derived class, but anything cross platform would need to define its own count and implement Release() on a more-derived class, since Windows defines IUnknown as pure-virtual with no virtual destructor.

I wonder if that's done on purpose, to have an easier time implementing other wrapper structs. Though it seems only IMalloc is affected:

../lib/DxcSupport/WinFunctions.cpp:158:19: error: allocating an object of abstract class type 'IMalloc'
  *ppMalloc = new IMalloc;
                  ^
../include/dxc/Support/WinAdapter.h:626:17: note: unimplemented pure virtual method 'AddRef' in 'IMalloc'
  virtual ULONG AddRef() = 0;
                ^
../include/dxc/Support/WinAdapter.h:627:17: note: unimplemented pure virtual method 'Release' in 'IMalloc'
  virtual ULONG Release() = 0;
                ^

That's simply solved by moving m_count to IMalloc, see the last commit.

Removing the implementation of AddRef and Release from WinAdapter.cpp will resolve the -Wdelete-non-virtual-dtor error/warning.

For completeness this error occurs on all other subclasses that contain virtual methods themselves. Expand this block to see all the errors.

[62/1040] Building CXX object lib/Support/CMakeFiles/LLVMSupport.dir/Process.cpp.o
In file included from ../lib/Support/Process.cpp:78:
../lib/Support/Unix/Process.inc:98:10: warning: 'mallinfo' is deprecated [-Wdeprecated-declarations]
  mi = ::mallinfo();
         ^
/usr/include/malloc.h:118:48: note: 'mallinfo' has been explicitly marked deprecated here
extern struct mallinfo mallinfo (void) __THROW __MALLOC_DEPRECATED;
                                               ^
/usr/include/malloc.h:31:30: note: expanded from macro '__MALLOC_DEPRECATED'
# define __MALLOC_DEPRECATED __attribute_deprecated__
                             ^
/usr/include/sys/cdefs.h:261:51: note: expanded from macro '__attribute_deprecated__'
# define __attribute_deprecated__ __attribute__ ((__deprecated__))
                                                  ^
1 warning generated.
[530/1040] Building CXX object lib/DxcSupport/CMakeFiles/LLVMDxcSupport.dir/dxcmem.cpp.o
../lib/DxcSupport/dxcmem.cpp:46:55: warning: ISO C++ requires the name after '::~' to be found in the same scope as the name before '::~' [-Wdtor-name]
    g_ThreadMallocTls->llvm::sys::ThreadLocal<IMalloc>::~ThreadLocal();
                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
                                                      ::ThreadLocal
1 warning generated.
[532/1040] Building CXX object lib/DxcSupport/CMakeFiles/LLVMDxcSupport.dir/WinAdapter.cpp.o
../lib/DxcSupport/WinAdapter.cpp:31:5: warning: delete called on non-final 'IMalloc' that has virtual functions but non-virtual destructor [-Wdelete-non-abstract-non-virtual-dtor]
    delete this;
    ^
1 warning generated.
[602/1040] Building CXX object lib/HLSL/CMakeFiles/LLVMHLSL.dir/HLMatrixLowerPass.cpp.o
../lib/HLSL/HLMatrixLowerPass.cpp:1582:18: warning: unused variable 'ValMatTy' [-Wunused-variable]
    HLMatrixType ValMatTy = HLMatrixType::cast(TrueMat->getType());
                 ^
1 warning generated.
[861/1040] Building CXX object tools/clang/lib/CodeGen/CMakeFiles/clangCodeGen.dir/CGHLSLMSFinishCodeGen.cpp.o
../tools/clang/lib/CodeGen/CGHLSLMSFinishCodeGen.cpp:164:6: warning: unused function 'IsHLSLBufferViewType' [-Wunused-function]
bool IsHLSLBufferViewType(llvm::Type *Ty) {
     ^
1 warning generated.
[972/1040] Building CXX object tools/clang/tools/libclang/CMakeFiles/libclang.dir/dxcisenseimpl.cpp.o
../tools/clang/tools/libclang/dxcisenseimpl.cpp:584:5: warning: delete called on non-final 'DxcBasicUnsavedFile' that has virtual functions but non-virtual destructor [-Wdelete-non-abstract-non-virtual-dtor]
    delete newValue;
    ^
1 warning generated.
[991/1040] Building CXX object tools/clang/tools/dxclib/CMakeFiles/dxclib.dir/dxc.cpp.o
../tools/clang/tools/dxclib/dxc.cpp:591:3: warning: delete called on non-final 'DxcIncludeHandlerForInjectedSources' that has virtual functions but non-virtual destructor [-Wdelete-non-abstract-non-virtual-dtor]
  DXC_MICROCOM_ADDREF_RELEASE_IMPL(m_dwRef)
  ^
../include/dxc/Support/microcom.h:90:7: note: expanded from macro 'DXC_MICROCOM_ADDREF_RELEASE_IMPL'
      delete this;                                                             \
      ^
1 warning generated.
[994/1040] Building CXX object tools/clang/unittests/HLSL/CMakeFiles/clang-hlsl-tests.dir/Objects.cpp.o
In file included from ../tools/clang/unittests/HLSL/Objects.cpp:11:
../include/dxc/Test/CompilationResult.h:74:22: warning: delete called on non-final 'TrivialDxcUnsavedFile' that has virtual functions but non-virtual destructor [-Wdelete-non-abstract-non-virtual-dtor]
    if (result == 0) delete this;
                     ^
1 warning generated.
[995/1040] Building CXX object tools/clang/unittests/HLSL/CMakeFiles/clang-hlsl-tests.dir/VerifierTest.cpp.o
In file included from ../tools/clang/unittests/HLSL/VerifierTest.cpp:14:
../include/dxc/Test/CompilationResult.h:74:22: warning: delete called on non-final 'TrivialDxcUnsavedFile' that has virtual functions but non-virtual destructor [-Wdelete-non-abstract-non-virtual-dtor]
    if (result == 0) delete this;
                     ^
1 warning generated.
[1003/1040] Building CXX object tools/clang/unittests/HLSL/CMakeFiles/clang-hlsl-tests.dir/FunctionTest.cpp.o
In file included from ../tools/clang/unittests/HLSL/FunctionTest.cpp:16:
../include/dxc/Test/CompilationResult.h:74:22: warning: delete called on non-final 'TrivialDxcUnsavedFile' that has virtual functions but non-virtual destructor [-Wdelete-non-abstract-non-virtual-dtor]
    if (result == 0) delete this;
                     ^
1 warning generated.
[1006/1040] Building CXX object tools/clang/unittests/HLSL/CMakeFiles/clang-hlsl-tests.dir/ExtensionTest.cpp.o
In file included from ../tools/clang/unittests/HLSL/ExtensionTest.cpp:10:
../include/dxc/Test/CompilationResult.h:74:22: warning: delete called on non-final 'TrivialDxcUnsavedFile' that has virtual functions but non-virtual destructor [-Wdelete-non-abstract-non-virtual-dtor]
    if (result == 0) delete this;
                     ^
../tools/clang/unittests/HLSL/ExtensionTest.cpp:306:3: warning: delete called on non-final 'TestIntrinsicTable' that has virtual functions but non-virtual destructor [-Wdelete-non-abstract-non-virtual-dtor]
  DXC_MICROCOM_ADDREF_RELEASE_IMPL(m_dwRef)
  ^
../include/dxc/Support/microcom.h:90:7: note: expanded from macro 'DXC_MICROCOM_ADDREF_RELEASE_IMPL'
      delete this;                                                             \
      ^
../tools/clang/unittests/HLSL/ExtensionTest.cpp:406:3: warning: delete called on non-final 'TestSemanticDefineValidator' that has virtual functions but non-virtual destructor [-Wdelete-non-abstract-non-virtual-dtor]
  DXC_MICROCOM_ADDREF_RELEASE_IMPL(m_dwRef)
  ^
../include/dxc/Support/microcom.h:90:7: note: expanded from macro 'DXC_MICROCOM_ADDREF_RELEASE_IMPL'
      delete this;                                                             \
      ^
3 warnings generated.
[1007/1040] Building CXX object tools/clang/unittests/HLSLTestLib/CMakeFiles/HLSLTestLib.dir/DxcTestUtils.cpp.o
In file included from ../tools/clang/unittests/HLSLTestLib/DxcTestUtils.cpp:12:
../include/dxc/Test/CompilationResult.h:74:22: warning: delete called on non-final 'TrivialDxcUnsavedFile' that has virtual functions but non-virtual destructor [-Wdelete-non-abstract-non-virtual-dtor]
    if (result == 0) delete this;
                     ^
1 warning generated.
[1010/1040] Building CXX object tools/clang/unittests/HLSL/CMakeFiles/clang-hlsl-tests.dir/DXIsenseTest.cpp.o
In file included from ../tools/clang/unittests/HLSL/DXIsenseTest.cpp:12:
../include/dxc/Test/CompilationResult.h:74:22: warning: delete called on non-final 'TrivialDxcUnsavedFile' that has virtual functions but non-virtual destructor [-Wdelete-non-abstract-non-virtual-dtor]
    if (result == 0) delete this;
                     ^
1 warning generated.
[1013/1040] Building CXX object tools/clang/unittests/HLSL/CMakeFiles/clang-hlsl-tests.dir/DxilModuleTest.cpp.o
In file included from ../tools/clang/unittests/HLSL/DxilModuleTest.cpp:10:
../include/dxc/Test/CompilationResult.h:74:22: warning: delete called on non-final 'TrivialDxcUnsavedFile' that has virtual functions but non-virtual destructor [-Wdelete-non-abstract-non-virtual-dtor]
    if (result == 0) delete this;
                     ^
1 warning generated.
[1040/1040] Linking CXX executable bin/clang-spirv-tests

jenatali · 2021-05-22T21:34:14Z

That's simply solved by moving m_count to IMalloc, see the last commit.

Note that IMalloc is supposed to be a pure-virtual class as well, since that's also an interface in the SDK: https://docs.microsoft.com/en-us/windows/win32/api/objidl/nn-objidl-imalloc

So you'd run into the same problem if you tried to have cross-platform usage of IMalloc. The solution is to make anything which implements IMalloc use the same DXC_MICROCOM_ADDREF_RELEASE_IMPL as everything else which inherits from IUnknown instead of trying to bake this behavior into the interface definition itself.

all other subclasses

Nah, looks like these are all in tests. I suspect core code suppresses this warning already. There's plenty of classes which inherit from IUnknown which don't produce this warning.

AppVeyorBot · 2021-05-22T22:07:07Z

❌ Build DirectXShaderCompiler 1.0.147 failed (commit af7fc15f3c by @MarijnS95)

MarijnS95 · 2021-05-22T23:36:10Z

Note that IMalloc is supposed to be a pure-virtual class as well, since that's also an interface in the SDK: https://docs.microsoft.com/en-us/windows/win32/api/objidl/nn-objidl-imalloc

Ah I could have figured :)

So you'd run into the same problem if you tried to have cross-platform usage of IMalloc. The solution is to make anything which implements IMalloc use the same DXC_MICROCOM_ADDREF_RELEASE_IMPL as everything else which inherits from IUnknown instead of trying to bake this behavior into the interface definition itself.

This is again a default implementation that's only used from CoGetMalloc. The IMalloc interface has now been converted to pure-virtual, with the default implementation in a local class. Couple observations:

DXC_MICROCOM_ADDREF_RELEASE_IMPL is pretty much providing m_count in much the same way, cleaner to use that indeed;

The implementation still gives off the warning:

[546/1039] Building CXX object lib/DxcSupport/CMakeFiles/LLVMDxcSupport.dir/WinFunctions.cpp.o
../lib/DxcSupport/WinFunctions.cpp:162:3: warning: delete called on non-final 'CoMalloc' that has virtual functions but non-virtual destructor [-Wdelete-non-abstract-non-virtual-dtor]
DXC_MICROCOM_ADDREF_RELEASE_IMPL(m_dwRef)
^
../include/dxc/Support/microcom.h:90:7: note: expanded from macro 'DXC_MICROCOM_ADDREF_RELEASE_IMPL'
    delete this;                                                             \
    ^
1 warning generated.

Should perhaps use a single, static instance just like struct HeapMalloc? (static HeapMalloc g_HeapMalloc;, AddRef and Release return 1). This struct doesn't store anything but passes through to system allocation functions, and could just as well be a static global.
However, statics don't initialize vtables properly:

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff68b47f5 in DxcInitThreadMalloc () at ../lib/DxcSupport/dxcmem.cpp:32
32	  g_ThreadMallocTls = (llvm::sys::ThreadLocal<IMalloc>*)g_pDefaultMalloc->Alloc(sizeof(llvm::sys::ThreadLocal<IMalloc>));
(gdb) bt
#0  0x00007ffff68b47f5 in DxcInitThreadMalloc () at ../lib/DxcSupport/dxcmem.cpp:32
#1  0x00007ffff67d2597 in DllMain() () from ./libdxcompiler.so
...
(gdb) p g_pDefaultMalloc
$1 = (IMalloc *) 0x7ffff7d76c80 <g_CoMalloc>
(gdb) info vtbl g_pDefaultMalloc
vtable for 'IMalloc' @ 0x0 (subobject @ 0x7ffff7d76c80):
[0]: <error: Cannot access memory at address 0x0>
[1]: <error: Cannot access memory at address 0x8>
[2]: <error: Cannot access memory at address 0x10>
[3]: <error: Cannot access memory at address 0x18>
[4]: <error: Cannot access memory at address 0x20>
[5]: <error: Cannot access memory at address 0x28>
[6]: <error: Cannot access memory at address 0x30>
[7]: <error: Cannot access memory at address 0x38>
[8]: <error: Cannot access memory at address 0x40>

Same applies to GetGlobalHeapMalloc() - I guess that codepath is never called?

By the way that documentation shows more functions are available, and that was never caught because implementations like struct HeapMalloc : public IMalloc lack the override keyword. Fixed that now too just so that our vtables are in-check again.

all other subclasses

Nah, looks like these are all in tests. I suspect core code suppresses this warning already. There's plenty of classes which inherit from IUnknown which don't produce this warning.

Not sure why I wrote all... Seems there's only one, for DxcIncludeHandlerForInjectedSources outside of tests.

Final nit, looks like the API in WinAdapter isn't using STDMETHODCALLTYPE/STDMETHODIMP like the rest. The former is defined to nothing so it doesn't make a difference, but might be more consistent to use.

AppVeyorBot · 2021-05-23T00:36:14Z

❌ Build DirectXShaderCompiler 1.0.148 failed (commit 0da7b95d8f by @MarijnS95)

jaebaek · 2021-05-25T05:02:47Z

Thank you for this change!
I am excited this will fix the virtual table issue.
I tried to fix the build failure on Linux with the clang++. Please let me know if the change I made is wrong!

AppVeyorBot · 2021-05-25T05:54:39Z

❌ Build DirectXShaderCompiler 1.0.157 failed (commit e1296dab7c by @jaebaek)

AppVeyorBot · 2021-05-25T06:44:53Z

❌ Build DirectXShaderCompiler 1.0.158 failed (commit f8585c5083 by @jaebaek)

lib/DxcSupport/dxcmem.cpp

MarijnS95 · 2021-05-25T07:50:39Z

Thank you for this change!
I am excited this will fix the virtual table issue.

Yes it's going to be awesome and we can finally drop our hacks! Unfortunately this means the applications we're using libdxcompiler.so in are not backwards- nor forwards-compatible (on Linux), but that's a problem of our own creation for allowing said hack in the first place instead of fixing it in here straight away 😀

I tried to fix the build failure on Linux with the clang++. Please let me know if the change I made is wrong!

Thanks! Yes final was the next stop to fix this issue, including the removal of final. I've adopted your change and fixed the CComPtr<DxcIncludeHandlerForInjectedSources> -> CComPtr<IDxcIncludeHandler> so that the windows build should succeed now too. The empty destructors have been removed, those should not be necessary (and had inconsistent formatting).

Finally, HeapMalloc has been marked as final as well, even though it doesn't show up in the warnings (there might be more such classes?).
Any comment on the VTable issue though? GetGlobalHeapMalloc() returns garbage, but it might be fine because this is never called? See MarijnS95/DirectXShaderCompiler@winadapter-iunknown-no-virtual-dtor...static-global for turning CoMalloc into a static global like HeapMalloc and showing the vtable issue on both.

MarijnS95 · 2021-05-25T07:58:41Z

Any comment on the VTable issue though? GetGlobalHeapMalloc() returns garbage, but it might be fine because this is never called? See MarijnS95/DirectXShaderCompiler@winadapter-iunknown-no-virtual-dtor...static-global for turning CoMalloc into a static global like HeapMalloc and showing the vtable issue on both.

Self-reply: I've hack-fixed this by running placement-new (which is fine to run unconditionally because the classes don't contain any other data).

We're crashing inside HeapMalloc::Alloc elsewhere now, apparently HeapMalloc is not used/tested anywhere.

jaebaek · 2021-05-25T08:01:27Z

Nice :) I do not have any specific comment for now. I just thought using final might be not a good solution because of CComPtr. I prepared another commit to avoid the error with virtual dtor but I am not sure it is a good solution either 😅 (and it seems you already fixed the issue about CComPtr)

I attach the one with virtual dtor: diff.txt. I will just follow your decision about it.

I want to check my other projects with this change. I am just curious previous issues I met was this issue or not.

Thanks! 👍

MarijnS95 · 2021-05-25T08:07:23Z

Nice :) I do not have any specific comment for now.

Not sure if you noticed the ninja-edit, but it's "fixed" now with placement-new, let me know if that's an acceptable change (I'll add a comment on the placement-new). It's more concise to keep these stateless instances as static globals, but for that they have to work in the first place. Any idea if HeapMalloc through GetGlobalHeapMalloc is used anywhere, because Alloc crashes on it (after fixing the vtable)?

EDIT: It looks like GetGlobalHeapMalloc() is only called from codepaths used by tools that are disabled on Linux.

I just thought using final might be not a good solution because of CComPtr. I prepared another commit to avoid the error with virtual dtor but I am not sure it is a good solution either (and it seems you already fixed the issue about CComPtr)

I attach the one with virtual dtor: diff.txt. I will just follow your decision about it.

Oh yeah, it looks like this was one odd case where the class type instead of the (pure virtual) interface was used in CComPtr<> - I don't see that anywhere else in the code, everything uses CComPtr<I...>. The CI should succeed now, awaiting in suspense (someone please put a ThreadRipper in AppVeyor 😂). Looks like we can keep it simple without extra virtual dtors 👍

AppVeyorBot · 2021-05-25T08:39:40Z

❌ Build DirectXShaderCompiler 1.0.159 failed (commit 0b3ffdee87 by @MarijnS95)

MarijnS95 · 2021-05-25T08:46:31Z

Oh yikes, the explicit type was used on purpose to access DxcIncludeHandlerForInjectedSources::insertIncludeFile. Not sure what the best fix is, I don't want to add virtual dtors and remove final just to satisfy some weird CComPtr/ATL behaviour that I have yet to fully understand. Added an extra unmanaged pointer with the right type for now, alternatively we can cast in the call too.

AppVeyorBot · 2021-05-25T09:48:25Z

✅ Build DirectXShaderCompiler 1.0.160 completed (commit c9e73d6030 by @MarijnS95)

jaebaek · 2021-05-25T12:27:01Z

I am not versed on the code base of this change in terms of the purpose of classes. I am afraid that I am not a right person to make a decision for the direction of changes here. If there are no other issues, fixing the vtable issue with working code looks good to me.

Let's wait @jenatali 's code review

MarijnS95 · 2021-05-25T12:34:51Z

@jaebaek Thanks for the review and suggestions thus far anyway! If anything I'll submit the conversion to a static global (singleton) in a separate PR together with the vtable-pointer fix (for HeapMalloc) so that we can have a better discussion there; let's first get the dtor here out of the way 😁

(Assuming it is okay to leave CoMalloc as a refcounted but stateless object for the time being, like it was/is before this PR)

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

pow2clk

We discussed this internally and I thought about it some more and came up with basically two options:

Effectively disable the warnings about the possibility of bad destructor calls due to not being virtual
Make every final descendent from IUnknown final.

The first is simple to implement and maintain, but does carry some risk of undetected programmer mistakes.

The second is probably "right". In that it forces people to either make final classes or add virtual destructors. However, it is cumbersome and represents a retraction of capabilities as there are users out there who have created descendants from our classes and would have to change how they use them. Incidentally, you are only supposed to create CComPtrs with interface types, but DXC, D3D and pretty much every else seems to violate this rampantly 😅

As such, I'm asking that the additions of finals to leaf node classes be removed along with the workarounds that they necessitated and that the Release() implementation in DXC_MICROCOM_ADDREF_RELEASE_IMPL be modified to be more in line with the "TM" variant. This has the effect of disabling the warnings only where we want exceptions with IUnknown descended classes.

Since it's a bit hard to communicate all this in the abstract, I created a commit that includes my suggestions plus a few other additions that you can draw from or incorporate directly if you choose: pow2clk@ea34d70

edit: corrected commit

pow2clk · 2022-01-25T03:54:30Z

cmake/modules/HandleLLVMOptions.cmake

+    # append_if(CXX_WONT_WARN_ON_FINAL_NONVIRTUALDTOR
+    #           "-Wnon-virtual-dtor" CMAKE_CXX_FLAGS)
+
+    # HLSL Change Ends


I wish that this could be disabled only where it needs to be, but it needs to be in quite a lot of places and that would probably be more annoying than its worth

We'd effectively have to inline-disable the warning every time a pure virtual interface is defined it seems. I guess the main point for this was because MSVC doesn't warn on this either.

pow2clk · 2022-01-25T03:55:44Z

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


This effectively disables the warning because the warning is concerned that if there is a descendent of the T class that is referenced by a pointer to the T class, only the T destructor will be called. This does just that.

I can't comment on the specific line due to github limitations, but my preferred solution would be to replace delete this on line 90 with a call to DxcCallDestructor followed by an explicit call to the delete operator.

Disabling that warning in this way would have been fine with the final notion, but theoretically it should be possible to have subclasses of subclasses with virtual destructors beyond that ¹? This would then call the wrong destructor, though it wasn't removed in your commit?

For my understanding, as this has all been long ago and I don't write a lot of C++: calling operator delete(this) directly is different in that it doesn't perform a virtual dispatch to any of the destructor operators (since that's done already by DxcCallDestructor) but merely frees the memory allocated by new?
EDIT: Seems like it's for deallocation purposes only, yes: https://en.cppreference.com/w/cpp/memory/new/operator_delete

Footnotes

As long as it isn't intermixed with the pure-virtual interfaces that would otherwise get a skewed vtable. ↩

Perhaps this is fine after all, given that:

COM objects should only be freed through the Release virtual function;

Hence, every descendant should override it;

Which will call the "lowest" destructor through DxcCallDestructor: it's known statically here as T, hence doesn't need to be virtual.

include/dxc/Test/CompilationResult.h

tools/clang/tools/dxclib/dxc.cpp

MarijnS95 · 2022-01-25T11:29:49Z

tools/clang/tools/libclang/dxcisenseimpl.cpp

+    CComPtr<IMalloc> pTmp(newValue->m_pMalloc);
+    newValue->DxcBasicUnsavedFile::~DxcBasicUnsavedFile();
+    pTmp->Free(newValue);


@pow2clck what'd happen if we just call newValue->Release() here? Same for InternalDxcBlobEncoding_Impl/MemoryStream above that had to get the ->T::~T "fix" to get rid of the warning.

Oh that's not entirely correct: Those two types have their code located in Release() already ¹, and the code here also uses IMalloc. The only thing all three have in common: they use IMalloc (ie. not suitable for DXC_MICROCOM_ADDREF_RELEASE_IMPL) yet don't use DxcThreadMalloc (ie. not suitable for DXC_MICROCOM_TM_ADDREF_RELEASE_IMPL either).

Footnotes

My suggestion would lead to infinite recursion 😂 ↩

AppVeyorBot · 2022-01-25T12:35:28Z

✅ Build DirectXShaderCompiler 1.0.1103 completed (commit 3de5ae798e by @pow2clk)

We have a very peculiar workaround for DXC (pending upstream fix [microsoft/DirectXShaderCompiler#3793]) that requires us to conditionally provide function implementations for extra vtable "spacers": https://github.com/Traverse-Research/hassle-rs/blob/f5a090c70bbcf66f3bafd3d549716a414e873838/src/wrapper.rs#L130-L139. The spacers are conditionally defined in: https://github.com/Traverse-Research/hassle-rs/blob/f5a090c70bbcf66f3bafd3d549716a414e873838/src/unknown.rs. These need to be forwarded otherwise the initialization of these vtable members will happen unconditionally even when they don't exist: https://github.com/Traverse-Research/hassle-rs/actions/runs/1777800733 Fixes microsoft#166 [microsoft/DirectXShaderCompiler#3793]: microsoft/DirectXShaderCompiler#3793

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

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

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

This prevents warnings about non-virtual destructor usage that trip up the Linux build. It represents status quo on Windows.

MarijnS95 · 2022-03-15T08:40:59Z

@pow2clk This is once again rebased on master and tested to work as intended. Your recommended/requested changes have been pulled in directly, anything else that needs to be done? I don't want this to sit yet another ~8 months 😅

AppVeyorBot · 2022-03-15T09:44:40Z

✅ Build DirectXShaderCompiler 1.0.1366 completed (commit 1490e620ee by @pow2clk)

…-no-virtual-dtor

AppVeyorBot · 2022-06-20T20:49:03Z

✅ Build DirectXShaderCompiler 1.0.1796 completed (commit 5f84b800a1 by @MarijnS95)

MarijnS95 · 2022-06-21T08:08:54Z

@pow2clk It seems the CI still succeeds after a merge with master... 2 months later 🙂

pow2clk

A much-delayed approval. Sorry again :(

MarijnS95 · 2022-10-13T18:02:29Z

@pow2clk thank you so much, after 1.5 years this is finally allowing me to drop some hacks in downstream wrapper projects 👍

…extra trait" This reverts commit c37cb03. [This commit] is squashed in the merge of PR #5, and does not exist in-tree. microsoft/DirectXShaderCompiler#3793 has finally been merged on October 13th 2022 and now allows us to drop the vtable dummies for virtual destructors again, as those are not part of the ABI and shouldn't have been in the vtable in the first place. [This commit]: c37cb03

…) through extra trait" (#50) * Revert "[WIN/LINUX] Implement vtable dummies (virtual dtors) through extra trait" This reverts commit c37cb03. [This commit] is squashed in the merge of PR #5, and does not exist in-tree. microsoft/DirectXShaderCompiler#3793 has finally been merged on October 13th 2022 and now allows us to drop the vtable dummies for virtual destructors again, as those are not part of the ABI and shouldn't have been in the vtable in the first place. [This commit]: c37cb03 * README: Replace verbose compatibility "guide" with direct table It's been quite a while ago (about two years) since we landed a whole bunch of compatibility fixes for DXC, and I hope everyone is using (much) newer releases by now. I also doubt anyone is interested in detailed descriptions of what we fixed (including the how and why), and just wants a clear answer on the DXC release or commit they must minimally use for compatibility with a certain `hassle-rs` release. * README: Vtable changes have been published in DXC v1.7.2212

We have a very peculiar workaround for DXC (pending upstream fix [microsoft/DirectXShaderCompiler#3793]) that requires us to conditionally provide function implementations for extra vtable "spacers": https://github.com/Traverse-Research/hassle-rs/blob/f5a090c70bbcf66f3bafd3d549716a414e873838/src/wrapper.rs#L130-L139. The spacers are conditionally defined in: https://github.com/Traverse-Research/hassle-rs/blob/f5a090c70bbcf66f3bafd3d549716a414e873838/src/unknown.rs. These need to be forwarded otherwise the initialization of these vtable members will happen unconditionally even when they don't exist: https://github.com/Traverse-Research/hassle-rs/actions/runs/1777800733 Fixes microsoft#166 [microsoft/DirectXShaderCompiler#3793]: microsoft/DirectXShaderCompiler#3793

This was referenced May 22, 2021

Add DirectXShaderCompiler example microsoft/windows-rs#802

Closed

[Linux] Import some LLVM fixes for GCC warnings #3794

Merged

MarijnS95 force-pushed the winadapter-iunknown-no-virtual-dtor branch from d4a0948 to 83b15a2 Compare May 22, 2021 23:36

MarijnS95 commented May 25, 2021

View reviewed changes

lib/DxcSupport/dxcmem.cpp Outdated Show resolved Hide resolved

MarijnS95 force-pushed the winadapter-iunknown-no-virtual-dtor branch from f6eccd0 to 570a5f2 Compare May 25, 2021 07:39

MarijnS95 force-pushed the winadapter-iunknown-no-virtual-dtor branch from 570a5f2 to 53d18bc Compare May 25, 2021 08:44

pow2clk self-requested a review May 27, 2021 01:05

pow2clk reviewed Jan 25, 2022

View reviewed changes

MarijnS95 commented Jan 25, 2022

View reviewed changes

MarijnS95 mentioned this pull request Feb 1, 2022

Migrate to Microsoft's com-rs Traverse-Research/hassle-rs#35

Merged

MarijnS95 mentioned this pull request Feb 1, 2022

macros/class: Propagate cfg annotations to COM class methods microsoft/com-rs#234

Closed

MarijnS95 mentioned this pull request Feb 28, 2022

Moving from com-rs to tinycom Traverse-Research/hassle-rs#36

Closed

MarijnS95 and others added 4 commits March 15, 2022 09:14

Make all WinAdapter destructions explicit

322b181

This prevents warnings about non-virtual destructor usage that trip up the Linux build. It represents status quo on Windows.

MarijnS95 force-pushed the winadapter-iunknown-no-virtual-dtor branch from 3904a9b to 322b181 Compare March 15, 2022 08:37

Merge remote-tracking branch 'upstream/main' into winadapter-iunknown…

e0bf4dd

…-no-virtual-dtor

MarijnS95 mentioned this pull request Oct 13, 2022

DXC Release build pipeline - add Linux config, improve display strings #4719

Merged

pow2clk approved these changes Oct 13, 2022

View reviewed changes

pow2clk merged commit 47f3137 into microsoft:main Oct 13, 2022

MarijnS95 deleted the winadapter-iunknown-no-virtual-dtor branch October 13, 2022 17:26

MarijnS95 mentioned this pull request Nov 8, 2022

Breaking: Revert "[WIN/LINUX] Implement vtable dummies (virtual dtors) through extra trait" Traverse-Research/hassle-rs#50

Merged

MarijnS95 mentioned this pull request Nov 28, 2022

[linux] Enable Reflection on *nix platforms #4810

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

MarijnS95 commented May 22, 2021 •

edited

Loading

AppVeyorBot commented May 22, 2021

jenatali commented May 22, 2021

MarijnS95 commented May 22, 2021

jenatali commented May 22, 2021 •

edited

Loading

AppVeyorBot commented May 22, 2021

MarijnS95 commented May 22, 2021

AppVeyorBot commented May 23, 2021

jaebaek commented May 25, 2021

AppVeyorBot commented May 25, 2021

AppVeyorBot commented May 25, 2021

MarijnS95 commented May 25, 2021 •

edited

Loading

MarijnS95 commented May 25, 2021 •

edited

Loading

jaebaek commented May 25, 2021

MarijnS95 commented May 25, 2021 •

edited

Loading

AppVeyorBot commented May 25, 2021

MarijnS95 commented May 25, 2021

AppVeyorBot commented May 25, 2021

jaebaek commented May 25, 2021

MarijnS95 commented May 25, 2021 •

edited

Loading

pow2clk left a comment •

edited

Loading

pow2clk Jan 25, 2022

MarijnS95 Jan 25, 2022

pow2clk Jan 25, 2022

MarijnS95 Jan 25, 2022 •

edited

Loading

MarijnS95 Jan 25, 2022

MarijnS95 Jan 25, 2022

MarijnS95 Jan 25, 2022 •

edited

Loading

AppVeyorBot commented Jan 25, 2022

MarijnS95 commented Mar 15, 2022

AppVeyorBot commented Mar 15, 2022

AppVeyorBot commented Jun 20, 2022

MarijnS95 commented Jun 21, 2022

pow2clk left a comment

MarijnS95 commented Oct 13, 2022

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

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

Conversation

MarijnS95 commented May 22, 2021 • edited Loading

AppVeyorBot commented May 22, 2021

jenatali commented May 22, 2021

MarijnS95 commented May 22, 2021

jenatali commented May 22, 2021 • edited Loading

AppVeyorBot commented May 22, 2021

MarijnS95 commented May 22, 2021

AppVeyorBot commented May 23, 2021

jaebaek commented May 25, 2021

AppVeyorBot commented May 25, 2021

AppVeyorBot commented May 25, 2021

MarijnS95 commented May 25, 2021 • edited Loading

MarijnS95 commented May 25, 2021 • edited Loading

jaebaek commented May 25, 2021

MarijnS95 commented May 25, 2021 • edited Loading

AppVeyorBot commented May 25, 2021

MarijnS95 commented May 25, 2021

AppVeyorBot commented May 25, 2021

jaebaek commented May 25, 2021

MarijnS95 commented May 25, 2021 • edited Loading

pow2clk left a comment • edited Loading

Choose a reason for hiding this comment

pow2clk Jan 25, 2022

Choose a reason for hiding this comment

MarijnS95 Jan 25, 2022

Choose a reason for hiding this comment

pow2clk Jan 25, 2022

Choose a reason for hiding this comment

MarijnS95 Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

Footnotes

MarijnS95 Jan 25, 2022

Choose a reason for hiding this comment

MarijnS95 Jan 25, 2022

Choose a reason for hiding this comment

MarijnS95 Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

Footnotes

AppVeyorBot commented Jan 25, 2022

MarijnS95 commented Mar 15, 2022

AppVeyorBot commented Mar 15, 2022

AppVeyorBot commented Jun 20, 2022

MarijnS95 commented Jun 21, 2022

pow2clk left a comment

Choose a reason for hiding this comment

MarijnS95 commented Oct 13, 2022

MarijnS95 commented May 22, 2021 •

edited

Loading

jenatali commented May 22, 2021 •

edited

Loading

MarijnS95 commented May 25, 2021 •

edited

Loading

MarijnS95 commented May 25, 2021 •

edited

Loading

MarijnS95 commented May 25, 2021 •

edited

Loading

MarijnS95 commented May 25, 2021 •

edited

Loading

pow2clk left a comment •

edited

Loading

MarijnS95 Jan 25, 2022 •

edited

Loading

MarijnS95 Jan 25, 2022 •

edited

Loading