Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Dec 17, 2022

Conversation

MarijnS95
Copy link
Member

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.


Note that releasing hassle-rs with this change relies on a DXC built with at least that PR, which was meged on October 13th.

@MarijnS95
Copy link
Member Author

I have unfortunately not found a clear way to detect the version of libdxcompiler.so at runtime or some other way to inform the user about this discrepancy... We'll have to release this as a breaking change and clearly document since what commit / version DXC is becoming incompatible I think.

…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
@MarijnS95 MarijnS95 force-pushed the no-dtor-in-vtable branch 2 times, most recently from 4b4de09 to a61fe02 Compare November 9, 2022 18:38
@MarijnS95
Copy link
Member Author

MarijnS95 commented Nov 9, 2022

We've previously done this by stating all the many commits that we landed upstream, together with DXC release tags.

I've converted those overly verbose and hard-to-scan-through paragraphs to a simple table, describing exactly what DXC version is going to be needed for a given hassle-rs release: the only breaks happened with 0.5.1 (when using intellisense) and are going to happen with 0.10.0 where this PR is supposed to land. Besides that it's just been flaky/broken IID support on MacOS+clang and gcc compilers before a given DXC commit/release.

@MarijnS95 MarijnS95 changed the title Revert "[WIN/LINUX] Implement vtable dummies (virtual dtors) through extra trait" **Breaking:** Revert "[WIN/LINUX] Implement vtable dummies (virtual dtors) through extra trait" Nov 9, 2022
@MarijnS95 MarijnS95 changed the title **Breaking:** Revert "[WIN/LINUX] Implement vtable dummies (virtual dtors) through extra trait" Breaking: Revert "[WIN/LINUX] Implement vtable dummies (virtual dtors) through extra trait" Nov 9, 2022
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.
@MarijnS95
Copy link
Member Author

CC @cwfitzgerald, such breaking changes in DXC releases and hassle-rs may be extra relevant for you now that you're using DXC in wgpu :)

@cwfitzgerald
Copy link

Thanks for the heads up! What are the consequences of loading an old version?

@MarijnS95
Copy link
Member Author

MarijnS95 commented Nov 9, 2022

@cwfitzgerald It'll segfault, because the COM vtables changed (for the better, mind you!).

And only on Linux, if you use DXC to compile SPIR-V (or for precompiling DXIL on a Linux server).

@cwfitzgerald
Copy link

It only segfaults on linux? If so, that's spooky but tolerable as we will only use DXC on windows.

@MarijnS95
Copy link
Member Author

It's spooky but only because we previously worked around clearly-wrong vtable layouts, and it quite frankly took well over a year to fix upstream :/

@MarijnS95
Copy link
Member Author

DXC v1.7.2212 just released, seems like a great moment to merge this and make a new release to crates.io 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants