Add DirectXShaderCompiler example #802

MarijnS95 · 2021-05-13T12:46:20Z

Depends on #747, #800

Following the discussion in https://github.com/microsoft/windows-rs/discussions/566#discussioncomment-467792 and the acknowledgement to have something in the tree that could possibly run on the Linux CI (more requirements in https://github.com/microsoft/windows-rs/discussions/566#discussioncomment-701806). Now there's a use for #747 too :)

TODO

Should I put dxcompiler.dll (and later libdxcompiler.so) in .windows/<arch>?
Should I remove the include handler (last commit) until proper COM overriding/implementing is finished? It's a complete hack anyway.

MarijnS95 · 2021-05-13T12:46:48Z

examples/dxc/Cargo.toml

+
+[dependencies]
+bindings = { package = "dxc_bindings", path = "bindings" }
+libloading = "0.7"


Unfortunately this adds a dependency on winapi :/

MarijnS95 · 2021-05-13T12:47:37Z

examples/dxc/src/main.rs

+// Only exists in newer DXC headers
+const DXC_CP_UTF8: u32 = CP_UTF8;


This will be cleaned/gone when microsoft/win32metadata#474 lands, like: MarijnS95@6e7d02d

MarijnS95 · 2021-05-13T12:47:58Z

examples/dxc/src/main.rs

+    // let mut compiler: Option<IDxcCompiler2> = None;
+    // let compiler = unsafe { create(&CLSID_DxcCompiler, &IDxcCompiler2::IID, compiler.set_abi()) }
+    //     .and_some(compiler)
+    //     .unwrap();
+    let compiler: IDxcCompiler2 = unsafe { DxcCreateInstanceProc(&create, &CLSID_DxcCompiler) }?;
+    let library: IDxcLibrary = unsafe { DxcCreateInstanceProc(&create, &CLSID_DxcLibrary) }?;


Is the thinking that the commented out code would replace the explicit calls to DxcCreateInstanceProc when 747 lands? Or is it the other way round?

The commented out code would disappear, in favour of a single call to DxcCreateInstanceProc.

MarijnS95 · 2021-05-13T12:48:14Z

examples/dxc/src/main.rs

+        let this = &mut *(this as *mut IncludeHandlerData);
+        dbg!(&this, pfilename, ppincludesource);
+
+        if pfilename != "./foo/bar/copy.hlsl" {


This comparison needs #800.

MarijnS95 · 2021-05-13T12:49:31Z

examples/dxc/src/main.rs

+        // Virtual destructors breaking the vtable, hack for !windows DXC
+        #[cfg(not(windows))]
+        dtor,
+        #[cfg(not(windows))]
+        dtor,


This should probably be removed - I'll have to take this up with DXC but it's a tricky one. For more details: Traverse-Research/hassle-rs@c37cb03#diff-8202c69531938d4c80a8f8a9190be8b98a026d2bd0364b97a0c7a32e8ae21ac6

I'll ask around to find out the history here. Looks like IUnknown explicitly has a virtual destructor on non-Windows.

Thanks, that'd be nice. Non-Windows support is fairly recent and it looks mostly used for the ./dxc binary, not the library. I've already submitted some fixes over time to make the library workable, and this seems to be the final step to make it truly cross-compatible. I mentioned this issue a while ago but didn't get to report a standalone issue yet nor dig and fix it myself.

Looks like IUnknown explicitly has a virtual destructor on non-Windows.

Correct: https://github.com/microsoft/DirectXShaderCompiler/blob/aa4f5176ebba010a860a214d73f33bdc5b5042ce/include/dxc/Support/WinAdapter.h#L627

I already tried removing it but that results in tons of compiler warnings, though it should work since on Windows these objects aren't cleaned up through a virtual DTOR either... I'll revisit this again on both GCC and Clang, and see what happens. My C++ knowledge isn't up there so I don't know if we can simply disable the warning without breaking anything. Note that Release calls delete on itself which would call the destructor and deallocate: https://github.com/microsoft/DirectXShaderCompiler/blob/aa4f5176ebba010a860a214d73f33bdc5b5042ce/lib/DxcSupport/WinAdapter.cpp#L24. Aside, the vtable clearly shows a destructor specific to DxcLibrary even though it has no members that need destructing - that already happens the release function. I vaguely recall reading something about passing the pointer and size to delete under the hood, that might be why DxcLibrary has its own deleting destructor in place.

Finally found some time to report this at 2AM: microsoft/DirectXShaderCompiler#3783

@damyanp For the record (in case you're still asking around) there's now a PR open: microsoft/DirectXShaderCompiler#3793

Removing the destructor is totally fine, and even MSVC complains about missing virtual dtors on the "official" IUnknown from the Windows headers, if enough warnings are enabled: https://godbolt.org/z/hKPT6ThEf

examples/dxc/Cargo.toml

damyanp · 2021-05-13T20:48:40Z

examples/dxc/src/main.rs

+    // let mut compiler: Option<IDxcCompiler2> = None;
+    // let compiler = unsafe { create(&CLSID_DxcCompiler, &IDxcCompiler2::IID, compiler.set_abi()) }
+    //     .and_some(compiler)
+    //     .unwrap();
+    let compiler: IDxcCompiler2 = unsafe { DxcCreateInstanceProc(&create, &CLSID_DxcCompiler) }?;
+    let library: IDxcLibrary = unsafe { DxcCreateInstanceProc(&create, &CLSID_DxcLibrary) }?;


Is the thinking that the commented out code would replace the explicit calls to DxcCreateInstanceProc when 747 lands? Or is it the other way round?

damyanp · 2021-05-13T20:53:01Z

examples/dxc/src/main.rs

+        // Virtual destructors breaking the vtable, hack for !windows DXC
+        #[cfg(not(windows))]
+        dtor,
+        #[cfg(not(windows))]
+        dtor,


I'll ask around to find out the history here. Looks like IUnknown explicitly has a virtual destructor on non-Windows.

kennykerr · 2021-05-18T18:43:26Z

Should I put dxcompiler.dll (and later libdxcompiler.so) in .windows/?

@damyanp are these Microsoft binaries your team distributes?

damyanp · 2021-05-18T19:03:21Z

They're distributed by a team I work closely with.

Can you explain more what it means to put them in "./windows/"? Does this involve somehow redistributing them? Or is this something that would happen at build time?

MarijnS95 · 2021-05-18T19:07:54Z

Can you explain more what it means to put them in "./windows/"? Does this involve somehow redistributing them? Or is this something that would happen at build time?

Redistributing in the .windows/ directory in this repository so that the CI can run the example for testing, preferably both Linux and Windows. It's not distrbuted with the crate itself on crates.io however.

kennykerr · 2021-05-18T19:15:02Z

The Windows crate doesn't distribute these but any crate may place dependencies in the '.windows' folder of their crate or workspace and the Windows crate will copy them to the target directory for convenience.

damyanp · 2021-05-18T19:17:18Z

Would this only be used for the tests? End users would still need to obtain the dll through the official distribution? What we want to avoid doing is making this crate become another vector for distributing the compiler dll. We should expect users to want control over which version of the dll they're using, and not have the Windows crate dictate which version they use.

kennykerr · 2021-05-18T19:29:40Z

Yes, this is only to enable examples/tests in this repo. You can see existing binaries here. I'm not saying I want to add these necessarily. I'm just explaining how it works.

damyanp · 2021-05-18T19:33:26Z

Would an option be to have the CI build pick up the binaries that are built from the shader compiler project?

kennykerr · 2021-05-18T19:38:55Z

That's an option. I'd rather not complicate the tests and examples with such build dependencies here.

MarijnS95 · 2021-05-18T19:52:38Z

That would also complicate local testing if a script is necessary to fetch and unpack the sources. Note that the GH releases don't include Linux binaries, those are only provided on AppVeyor (microsoft/DirectXShaderCompiler#3686).

On the other hand these binaries would add quite a few megabytes to this repo.

kennykerr · 2021-05-18T20:04:34Z

Probably not worth adding such an example to this repo. It's also not something I have any experience with so I'm a little hesitant to take it on.

damyanp · 2021-05-18T20:14:02Z

I'm going to work with the dxcompiler team to try and figure out how they work with these efforts. Although a version of the compiler is included in the SDK, most users get their version from the github project.

MarijnS95 · 2021-05-18T23:30:02Z

Probably not worth adding such an example to this repo. It's also not something I have any experience with so I'm a little hesitant to take it on.

We can have the example without the library checked in, assuming those wanting to test it need the dll from the SDK or downloaded from GH releases/AppVeyor.

kennykerr · 2021-05-19T14:48:58Z

Feel free to share this example in a dedicate repo. I'm sure it will benefit developers in interested in DirectX.

MarijnS95 · 2021-05-19T14:52:38Z

No further discussion whether this can make it in without having the library checked in?

kennykerr · 2021-05-19T14:54:54Z

As I'm not a DirectX expert, I don't want to carry examples that I can't verify myself simply by running cargo run.

MarijnS95 · 2021-05-19T19:25:49Z

As I'm not a DirectX expert, I don't want to carry examples that I can't verify myself simply by running cargo run.

No-one else to carry that effort? As said the library should be included with the SDK thus available on most developer machines?

I had plans to use DXC through windows-rs but all my efforts thus far have been nullified 😞

MarijnS95 added 3 commits May 13, 2021 14:03

pwstr: Use widestring crate for 32-bit wchar on Linux

acc5b7e

bstr: Implement using widestring crate

b78dff9

Generate QueryInterface-like helper function for callbacks

3b9262d

MarijnS95 commented May 13, 2021

View reviewed changes

MarijnS95 mentioned this pull request May 13, 2021

Generate QueryInterface-like helper function for callbacks #747

Closed

damyanp reviewed May 13, 2021

View reviewed changes

MarijnS95 added 2 commits May 14, 2021 09:13

examples: Add DirectXShaderCompiler

698199f

examples/dxc: Implement include handler

48daca0

MarijnS95 force-pushed the dxc branch from 2503e84 to 48daca0 Compare May 14, 2021 07:13

MarijnS95 mentioned this pull request May 19, 2021

[Linux] WinAdapter: IUnknown vtable includes destructors resulting in ABI mismatch microsoft/DirectXShaderCompiler#3783

Closed

kennykerr closed this May 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DirectXShaderCompiler example #802

Add DirectXShaderCompiler example #802

MarijnS95 commented May 13, 2021

MarijnS95 May 13, 2021

MarijnS95 May 13, 2021

MarijnS95 May 13, 2021

damyanp May 13, 2021

MarijnS95 May 14, 2021

MarijnS95 May 13, 2021

MarijnS95 May 13, 2021

damyanp May 13, 2021

MarijnS95 May 14, 2021

MarijnS95 May 19, 2021

MarijnS95 May 22, 2021

damyanp May 13, 2021

damyanp May 13, 2021

kennykerr commented May 18, 2021

damyanp commented May 18, 2021

MarijnS95 commented May 18, 2021 •

edited

Loading

kennykerr commented May 18, 2021

damyanp commented May 18, 2021

kennykerr commented May 18, 2021

damyanp commented May 18, 2021

kennykerr commented May 18, 2021

MarijnS95 commented May 18, 2021

kennykerr commented May 18, 2021

damyanp commented May 18, 2021

MarijnS95 commented May 18, 2021

kennykerr commented May 19, 2021

MarijnS95 commented May 19, 2021

kennykerr commented May 19, 2021

MarijnS95 commented May 19, 2021

		// Only exists in newer DXC headers
		const DXC_CP_UTF8: u32 = CP_UTF8;

Add DirectXShaderCompiler example #802

Add DirectXShaderCompiler example #802

Conversation

MarijnS95 commented May 13, 2021

TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennykerr commented May 18, 2021

damyanp commented May 18, 2021

MarijnS95 commented May 18, 2021 • edited Loading

kennykerr commented May 18, 2021

damyanp commented May 18, 2021

kennykerr commented May 18, 2021

damyanp commented May 18, 2021

kennykerr commented May 18, 2021

MarijnS95 commented May 18, 2021

kennykerr commented May 18, 2021

damyanp commented May 18, 2021

MarijnS95 commented May 18, 2021

kennykerr commented May 19, 2021

MarijnS95 commented May 19, 2021

kennykerr commented May 19, 2021

MarijnS95 commented May 19, 2021

MarijnS95 commented May 18, 2021 •

edited

Loading