[Linux] WinAdapter: IUnknown vtable includes destructors resulting in ABI mismatch

[MarijnS95]

The vtable for IUnknown from WinAdapter.h contains five instead of three function pointers, shifting vtable entries for every subclass. Two pointers following the three virutal functions in IUnknown are reserved for a complete object and deleting destructor:

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**)>
[7]: 0x7ffff6a56d70 <DxcLibrary::CreateBlobFromFile(wchar_t const*, unsigned int*, IDxcBlobEncoding**)>
[8]: 0x7ffff6a56d80 <DxcLibrary::CreateBlobWithEncodingFromPinned(void const*, unsigned int, unsigned int, IDxcBlobEncoding**)>
[9]: 0x7ffff6a56d90 <DxcLibrary::CreateBlobWithEncodingOnHeapCopy(void const*, unsigned int, unsigned int, IDxcBlobEncoding**)>
[10]: 0x7ffff6a56da0 <DxcLibrary::CreateBlobWithEncodingOnMalloc(void const*, IMalloc*, unsigned int, unsigned int, IDxcBlobEncoding**)>
[11]: 0x7ffff6a56db0 <DxcLibrary::CreateIncludeHandler(IDxcIncludeHandler**)>
[12]: 0x7ffff6a56dc0 <DxcLibrary::CreateStreamFromBlobReadOnly(IDxcBlob*, IStream**)>
[13]: 0x7ffff6a56dd0 <DxcLibrary::GetBlobAsUtf8(IDxcBlob*, IDxcBlobEncoding**)>
[14]: 0x7ffff6a56e90 <DxcLibrary::GetBlobAsUtf16(IDxcBlob*, IDxcBlobEncoding**)>

This makes it annoying or impossible to interact with the interfaces from Linux and Windows in a generic way.

Simply removing it results in compiler warnings from both Clang and GCC:

../include/dxc/Support/WinAdapter.h:628:8: warning: 'IUnknown' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
struct IUnknown {
       ^

No idea if ignoring them locally (#pragma clang/GCC diagnostic ignored "-Wnon-virtual-dtor") or globally is the right way forward though. Members owned by subclasses of IUnknown should be cleaned up through the Release() function anyway otherwise this would leak memory and other resources on Windows where no virtual destructors are used.

I recall reading something about operator delete (called as delete this in IUnknown::Release) requiring object size to call the appropriate memory deallocation function which could be facilitated through this destructor. The deleting destructor could possibly pass a size to one of the operator delete variants.

@pow2clk Hope you have some suggestions since you've been working on Linux compatibility - apart this issue everything runs flawlessly on Linux on-par with our Windows builds and that's simply awesome!

[jenatali]

It should be possible to just remove the virtual destructor and suppress the warning. All destruction of objects is done via the Release() implementation which is declared as part of the concrete class using a macro (DXC_MICROCOM_ADDREF_RELEASE_IMPL). So, when the code runs delete this it is deleting the class, not the IUnknown interface. If the class which declares Release() wants to have a virtual destructor to support further inheritance, that'd be fine.

[MarijnS95]

All destruction of objects is done via the Release() implementation which is declared as part of the concrete class using a macro (DXC_MICROCOM_ADDREF_RELEASE_IMPL). So, when the code runs delete this it is deleting the class, not the IUnknown interface.

Yes Release is used as "virtual destructor" which calls the appropriate concrete destructor and deallocator using delete this in DXC_MICROCOM_ADDREF_RELEASE_IMPL or the concrete destructor and IMalloc free function in DXC_MICROCOM_TM_ADDREF_RELEASE_IMPL. The few classes not providing a custom Release function (ie. IMalloc) don't hold any extra members that can't be cleaned up through IUnknown's default Release() implementation.

If the class which declares Release() wants to have a virtual destructor to support further inheritance, that'd be fine.

If explicitly defined in the class and vtable that's okay, yes. It isn't for IUnknown on Windows though and shouldn't be on Linux either :)

It should be possible to just remove the virtual destructor and suppress the warning.

My thought as well, but it looks pretty horrible: master...MarijnS95:winadapter-iunknown-no-virtual-dtor

Unless we want to disable this warning project-wide, but that's ugly too when it really only concerns IUnknown and descendants.

Furthermore DXC_MICROCOM_ADDREF_RELEASE_IMPL isn't too happy when calling the dtor:

Error is now irrelevant

```cpp ../include/dxc/Support/microcom.h:104:3: warning: destructor called on non-final 'DxcRewriter' that has virtual functions but non-virtual destructor [-Wdelete-non-abstract-non-virtual-dtor] obj->~T(); ^ ../tools/clang/tools/libclang/dxcrewriteunused.cpp:1274:3: note: in instantiation of function template specialization 'DxcCallDestructor' requested here DXC_MICROCOM_TM_ADDREF_RELEASE_IMPL() ^ ../include/dxc/Support/microcom.h:120:7: note: expanded from macro 'DXC_MICROCOM_TM_ADDREF_RELEASE_IMPL' DxcCallDestructor(this); \ ^ ../include/dxc/Support/microcom.h:104:9: note: qualify call to silence this warning obj->~T(); ^ DxcRewriter:: ```

Thought about using obj->~T::T(); or obj->typename T::T() but that's invalid, it's been too long since I've last used C++. Suggestions that don't involve disabling this warning locally?

And so I realized I should have written obj->T::~T() seconds after posting. Good morning 😴.

I'll work on this some more and open a PR later. The sooner this lands the better, I'm already not looking forward to removing our Linux hacks from hassle-rs and dealing with very strict version requirements, as this is not forwards nor backwards compatible 😬

[jenatali]

From peeking at the commits you linked, you probably don't want to explicitly scope to not _WIN32, since Windows also doesn't define a virtual destructor for IUnknown, and you can use Clang to target Windows, and even GCC via MinGW.

I also don't entirely understand why this warning is popping up. I don't see it with either GCC or Clang with -Wall -Wpedantic unless I explicitly turn on this warning. Is the Linux build doing that?

[MarijnS95]

From peeking at the commits you linked, you probably don't want to explicitly scope to not _WIN32, since Windows also doesn't define a virtual destructor for IUnknown, and you can use Clang to target Windows, and even GCC via MinGW.

That's completely on purpose since building with Clang on Windows should not use WinAdapter.h (see how I placed the diagnostics carefully inside the same #ifdef _WIN32 block as #include "dxc/Support/WinAdapter.h" in dxcapi.h). Only IUnknown from WinAdapter.h is affeced by this issue.

I also don't entirely understand why this warning is popping up. I don't see it with either GCC or Clang with -Wall -Wpedantic unless I explicitly turn on this warning. Is the Linux build doing that?

I guess you're building on Windows where WinAdapter.h is not used? I'm curious though what magic Windows uses under the hood to define its IUnknown without this issue, we could learn a thing or two from that.

[jenatali]

I should clarify, I was just playing around with Clang/GCC on Godbolt, targeting Linux, defining a structure with a virtual method and no virtual destructor. Compiling with -Wnon-virtual-dtor I see the warning, but without explicitly adding that warning, I'm not seeing it. See https://godbolt.org/z/M4eYYYePh.

So, even though you wouldn't be using WinAdapter.h targeting Windows, you can still use Clang/GCC, which still support having that warning turned on, but it should be suppressed when implementing IUnknown-based classes, since Windows IUnknown definition already doesn't have a virtual destructor, so it'd trigger the warning there too (I expect).

[MarijnS95]

I should clarify, I was just playing around with Clang/GCC on Godbolt, targeting Linux, defining a structure with a virtual method and no virtual destructor. Compiling with -Wnon-virtual-dtor I see the warning, but without explicitly adding that warning, I'm not seeing it. See https://godbolt.org/z/M4eYYYePh.

I wonder what flag causes this to turn on. There are mentions of -Werror=non-virtual-dtor in the LLVM part of this codebase but I doubt those leak to DXC compile commands or are otherwise translated back into -Wno-error?

but it should be suppressed when implementing IUnknown-based classes, since Windows IUnknown definition already doesn't have a virtual destructor, so it'd trigger the warning there too (I expect).

I'd really like to get confirmation for this though, having a hunch that Microsoft might have done something special to their definition to prevent triggering it - I doubt MSVC doesn't have a warning for this class of errors, or is it not explicitly turned on there at all? Guess I should go hunt for the source declaration of IUnknown somewhere in Windows headers :)

[jenatali]

I doubt MSVC doesn't have a warning for this class of errors

Indeed, MSVC does, but it's only enabled with /Wall, which includes a lot of warnings that are very verbose, and so as I understand, it's almost never used. Even the Windows SDK isn't clean with /Wall: https://godbolt.org/z/hKPT6ThEf

[MarijnS95]

Indeed, MSVC does, but it's only enabled with /Wall, which includes a lot of warnings that are very verbose, and so as I understand, it's almost never used. Even the Windows SDK isn't clean with /Wall: https://godbolt.org/z/hKPT6ThEf

Cool, so this isn't even an issue with the WinAdapter implementation per-se! Would it be acceptable to just -Wno-non-virtual-dtor this globally from CMake and call it a day? There are a few more warnings remaining but I can sort through those and either fix them (obj->T::~T()) or disable the warning too.