DirectXShaderCompiler icon indicating copy to clipboard operation
DirectXShaderCompiler copied to clipboard

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

Open MarijnS95 opened this issue 4 years ago • 34 comments

Fixes #3783

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!

MarijnS95 avatar May 22 '21 14:05 MarijnS95

:x: Build DirectXShaderCompiler 1.0.144 failed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/6c3eee293b by @MarijnS95)

AppVeyorBot avatar May 22 '21 15:05 AppVeyorBot

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.

jenatali avatar May 22 '21 20:05 jenatali

@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

MarijnS95 avatar May 22 '21 21:05 MarijnS95

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.

jenatali avatar May 22 '21 21:05 jenatali

:x: Build DirectXShaderCompiler 1.0.147 failed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/af7fc15f3c by @MarijnS95)

AppVeyorBot avatar May 22 '21 22:05 AppVeyorBot

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.

MarijnS95 avatar May 22 '21 23:05 MarijnS95

:x: Build DirectXShaderCompiler 1.0.148 failed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/0da7b95d8f by @MarijnS95)

AppVeyorBot avatar May 23 '21 00:05 AppVeyorBot

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!

jaebaek avatar May 25 '21 05:05 jaebaek

:x: Build DirectXShaderCompiler 1.0.157 failed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/e1296dab7c by @jaebaek)

AppVeyorBot avatar May 25 '21 05:05 AppVeyorBot

:x: Build DirectXShaderCompiler 1.0.158 failed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/f8585c5083 by @jaebaek)

AppVeyorBot avatar May 25 '21 06:05 AppVeyorBot

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 :grinning:

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 https://github.com/MarijnS95/DirectXShaderCompiler/compare/winadapter-iunknown-no-virtual-dtor...static-global for turning CoMalloc into a static global like HeapMalloc and showing the vtable issue on both.

MarijnS95 avatar May 25 '21 07:05 MarijnS95

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.

MarijnS95 avatar May 25 '21 07:05 MarijnS95

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 :sweat_smile: (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! :+1:

jaebaek avatar May 25 '21 08:05 jaebaek

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 :joy:). Looks like we can keep it simple without extra virtual dtors :+1:

MarijnS95 avatar May 25 '21 08:05 MarijnS95

:x: Build DirectXShaderCompiler 1.0.159 failed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/0b3ffdee87 by @MarijnS95)

AppVeyorBot avatar May 25 '21 08:05 AppVeyorBot

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.

MarijnS95 avatar May 25 '21 08:05 MarijnS95

:white_check_mark: Build DirectXShaderCompiler 1.0.160 completed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/c9e73d6030 by @MarijnS95)

AppVeyorBot avatar May 25 '21 09:05 AppVeyorBot

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

jaebaek avatar May 25 '21 12:05 jaebaek

@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 :grin:

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

MarijnS95 avatar May 25 '21 12:05 MarijnS95

@pow2clk thanks for self-requesting a review; is this something you can look at? It'd be nice to get this finalized before I completely forget all the implementation details, and it'll take a few coordinated downstream changes, too.

EDIT: Please also consider the discussion above about making CoMalloc static, and fixing HeapMalloc, with placement-new (will land that as a separate PR).

MarijnS95 avatar Jul 08 '21 08:07 MarijnS95

:white_check_mark: Build DirectXShaderCompiler 1.0.380 completed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/fa1a285800 by @MarijnS95)

AppVeyorBot avatar Jul 16 '21 09:07 AppVeyorBot

@pow2clk Is this something you can review? I understand this is not of highest priority but it's been almost three months since you self-requested a review. I'd like to get this in so that we don't have to spread Linux-specific hacks around any longer; it's only making the migration back worse over time.

@jenatali Do you have any more comments that could further this PR?

MarijnS95 avatar Aug 20 '21 12:08 MarijnS95

:white_check_mark: Build DirectXShaderCompiler 1.0.520 completed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/d8c364ba5f by @MarijnS95)

AppVeyorBot avatar Sep 20 '21 14:09 AppVeyorBot

@pow2clk Would it help review if this PR is split up? That is, remove everything but the first commit that actually addresses #3783, and submit the other patches separately for improved definitions of pure-virtual interfaces?

MarijnS95 avatar Dec 09 '21 12:12 MarijnS95

:white_check_mark: Build DirectXShaderCompiler 1.0.974 completed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/864366becc by @MarijnS95)

AppVeyorBot avatar Dec 09 '21 13:12 AppVeyorBot

@pow2clk Thanks a lot for tackling more Linux issues and extended testing! I'll get right to rebasing this on top of #3888 / master and resolving the remaining issue :+1:!

MarijnS95 avatar Jan 24 '22 11:01 MarijnS95

@pow2clk Things got a bit mixed up and lost in the comments above, but as a followup do you think looking into these are useful:

  • Convert CoMalloc to a static singleton just like HeapMalloc - requires placement new to initialize the vtable?
  • GetGlobalHeapMalloc currently returns such a static singleton that has an uninitialized vtable;
  • Calling GetGlobalHeapMalloc()->Alloc for testing (it doesn't appear to be used otherwise) segfaults deep in std::map<...>::operator[].

MarijnS95 avatar Jan 24 '22 11:01 MarijnS95

https://dev.azure.com/DirectXShaderCompiler/public/_build/results?buildId=895&view=logs&j=24c9fe99-3b1b-58cc-6a06-beff22cc45c7&t=21badf4d-2659-5245-8ca3-8874367569d5&l=1852 it looks like we need to come up with the same pointer duplication as https://github.com/microsoft/DirectXShaderCompiler/pull/3793/files#diff-ff426fac7f90d369f6c137698730989cf30ad094c6b1c37e5978563cf82f06ebR710-R711 so that the pure-virtual interface can be used inside CComPtr, to fix https://github.com/microsoft/DirectXShaderCompiler/pull/3793#issuecomment-847679213 :(

Is there something better we can do here to fix this?

MarijnS95 avatar Jan 24 '22 11:01 MarijnS95

:x: Build DirectXShaderCompiler 1.0.1100 failed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/86e6552415 by @MarijnS95)

AppVeyorBot avatar Jan 24 '22 12:01 AppVeyorBot

:white_check_mark: Build DirectXShaderCompiler 1.0.1103 completed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/3de5ae798e by @pow2clk)

AppVeyorBot avatar Jan 25 '22 12:01 AppVeyorBot