Support libdxcompiler.so on Linux #5

MarijnS95 · 2020-07-20T09:11:02Z

Thanks @h3r2tic for doing the initial porting work!

Things to sort out in the feature:

Make vtables binary-compatible with Windows again (requires changes in the upstream project);
Update deprecated com-rs crate to Microsofts' com-rs. This is in the works but depends on minor changes in com-rs to compile on Linux. That project is furthermore going through an API refactor which makes it hard to justify switching just yet.

extern crate requires deprecated `#[macro_use]`, which should be replaced with `use` instead.

HRESULT is 32-bit: https://en.wikipedia.org/wiki/HRESULT

This assumes a DXC compiled without virtual destructor! Which - as it stands - is invalid with undefined behaviour, but "works". This needs followup in DXC itself. This includes a revert of commit 308dd86.

TODO: Might as well keep the IUnknown name and "hide" the COM one (less changes, as long as the right one is use'd.

get_buffer_pointer returns a mutable (non-const) void ptr, create_include_handler returns an IDxcIncludeHandler, and annotate some c_void types. All the remaining c_void uses are that way in the official API.

Using winapi on not(windows) is a no-op already, but saves pulling in this extra dependency.

examples/validate.rs

MarijnS95 · 2020-07-20T09:46:37Z

src/ffi.rs

-    malloc: *const c_void,
+    malloc: /* IMalloc */ *const c_void,


Perhaps we should make these pub type IMalloc = c_void; (and pub type IStream = c_void; below) instead?

They likely won't be used any time soon, but it's good to have all non-void types (in the actual DXC API) properly represented even if the backing object isn't implemented.

src/wrapper.rs

MarijnS95 · 2020-07-21T10:18:11Z

src/utils.rs

+    #[error("Error loading library")]
+    LoadLibraryError(#[from] libloading::Error),


Is the underlying error printed in this case, or is that only applicable to #[error(transparent)]?

Updated, because this also wouldn't print the filename of the DLL / SO that was being attempted.

…, makes the API slightly easier to use

MarijnS95 · 2020-07-21T10:20:32Z

src/lib.rs

-pub use crate::utils::{compile_hlsl, validate_dxil};
+pub use crate::utils::compile_hlsl;
+pub use crate::utils::validate_dxil;


We might as well undo this now 😉

MarijnS95 · 2020-07-21T10:27:08Z

src/wrapper.rs

+        #[cfg(not(windows))]
+        {
+            Err(HassleError::WindowsOnly(
+                "DXIL Signing is only supported on windows at the moment".to_string(),
+            ))
+        }
+
+        #[cfg(windows)]
+        {
+            let dxil_lib = Library::new("dxil.dll").map_err(|e| HassleError::LoadLibraryError {
+                filename: "dxil".to_string(),
+                inner: e,
+            })?;
+
+            Ok(Self { dxil_lib })
+        }


Since none of this depends on cfg-scoped symbols anymore:

Suggested change

#[cfg(not(windows))]

{

Err(HassleError::WindowsOnly(

"DXIL Signing is only supported on windows at the moment".to_string(),

))

}

#[cfg(windows)]

{

let dxil_lib = Library::new("dxil.dll").map_err(|e| HassleError::LoadLibraryError {

filename: "dxil".to_string(),

inner: e,

})?;

Ok(Self { dxil_lib })

}

if cfg!(windows) {

let dxil_lib = Library::new("dxil.dll").map_err(|e| HassleError::LoadLibraryError {

filename: "dxil".to_string(),

inner: e,

})?;

Ok(Self { dxil_lib })

} else {

Err(HassleError::WindowsOnly(

"DXIL Signing is only supported on windows at the moment".to_string(),

))

}

The same call to SysFreeString happens in the Windows counterpart. Solves the following leaks found by valgrind: ==213075== 40 bytes in 1 blocks are definitely lost in loss record 4 of 13 ==213075== at 0x483A77F: malloc (vg_replace_malloc.c:307) ==213075== by 0x5B03395: ??? ==213075== by 0x5AFC77E: ??? ==213075== by 0x12C766: hassle_rs::intellisense::ffi::IDxcCursor::get_display_name (macros.rs:108) ==213075== by 0x116373: hassle_rs::intellisense::wrapper::DxcCursor::get_display_name (wrapper.rs:236) ==213075== by 0x11286D: intellisense_tu::main (intellisense-tu.rs:33) ==213075== by 0x11371A: core::ops::function::FnOnce::call_once (function.rs:227) ==213075== by 0x11348D: std::sys_common::backtrace::__rust_begin_short_backtrace (backtrace.rs:137) ==213075== by 0x111E10: std::rt::lang_start::{{closure}} (rt.rs:66) ==213075== by 0x150A20: call_once<(),Fn<()>> (function.rs:259) ==213075== by 0x150A20: do_call<&Fn<()>,i32> (panicking.rs:373) ==213075== by 0x150A20: try<i32,&Fn<()>> (panicking.rs:337) ==213075== by 0x150A20: catch_unwind<&Fn<()>,i32> (panic.rs:379) ==213075== by 0x150A20: std::rt::lang_start_internal (rt.rs:51) ==213075== by 0x111DE6: std::rt::lang_start (rt.rs:65) ==213075== ==213075== 56 (16 direct, 40 indirect) bytes in 1 blocks are definitely lost in loss record 5 of 13 ==213075== at 0x483ADEF: operator new(unsigned long) (vg_replace_malloc.c:342) ==213075== by 0x4FD058D: ??? ==213075== by 0x4FC04D9: ??? ==213075== by 0x40112DD: call_init.part.0 (in /usr/lib/ld-2.32.so) ==213075== by 0x40113C7: _dl_init (in /usr/lib/ld-2.32.so) ==213075== by 0x4A100E4: _dl_catch_exception (in /usr/lib/libc-2.32.so) ==213075== by 0x4015704: dl_open_worker (in /usr/lib/ld-2.32.so) ==213075== by 0x4A10087: _dl_catch_exception (in /usr/lib/libc-2.32.so) ==213075== by 0x4014F3D: _dl_open (in /usr/lib/ld-2.32.so) ==213075== by 0x489434B: ??? (in /usr/lib/libdl-2.32.so) ==213075== by 0x4A10087: _dl_catch_exception (in /usr/lib/libc-2.32.so) ==213075== by 0x4A10152: _dl_catch_error (in /usr/lib/libc-2.32.so) ==213075== ==213075== 124 bytes in 3 blocks are definitely lost in loss record 9 of 13 ==213075== at 0x483A77F: malloc (vg_replace_malloc.c:307) ==213075== by 0x5B03395: ??? ==213075== by 0x5AFC77E: ??? ==213075== by 0x12C766: hassle_rs::intellisense::ffi::IDxcCursor::get_display_name (macros.rs:108) ==213075== by 0x116373: hassle_rs::intellisense::wrapper::DxcCursor::get_display_name (wrapper.rs:236) ==213075== by 0x112F63: intellisense_tu::main (intellisense-tu.rs:52) ==213075== by 0x11371A: core::ops::function::FnOnce::call_once (function.rs:227) ==213075== by 0x11348D: std::sys_common::backtrace::__rust_begin_short_backtrace (backtrace.rs:137) ==213075== by 0x111E10: std::rt::lang_start::{{closure}} (rt.rs:66) ==213075== by 0x150A20: call_once<(),Fn<()>> (function.rs:259) ==213075== by 0x150A20: do_call<&Fn<()>,i32> (panicking.rs:373) ==213075== by 0x150A20: try<i32,&Fn<()>> (panicking.rs:337) ==213075== by 0x150A20: catch_unwind<&Fn<()>,i32> (panic.rs:379) ==213075== by 0x150A20: std::rt::lang_start_internal (rt.rs:51) ==213075== by 0x111DE6: std::rt::lang_start (rt.rs:65) Fixes: 94fbad7 ("Support libdxcompiler.so on Linux (#5)")

Resulting strings were only one character long because the second byte for ASCII text is NULL. On Linux DXC properly uses wchar_t which is 32-bit instead of 16-bit. Fixes: 94fbad7 ("Support libdxcompiler.so on Linux (#5)")

…ide, not UTF-8; convert return_hr to expression (#10) * utils: Mark libloading error as source In case we want to walk the .source() trace and print all inner exceptions (when these are usually rewrapped in thiserror types in higher layers), this exception needs to be marked as source to be returned from that function. * intellisense: Simplify DxcCursor retrieval and free leaked allocation Solves the following Valgrind trace: ==168242== 24 bytes in 1 blocks are definitely lost in loss record 2 of 14 ==168242== at 0x483A77F: malloc (vg_replace_malloc.c:307) ==168242== by 0x5AFC3F4: ??? ==168242== by 0x5AFCFB3: ??? ==168242== by 0x12BD23: hassle_rs::intellisense::ffi::IDxcCursor::get_children (macros.rs:108) ==168242== by 0x1167E6: hassle_rs::intellisense::wrapper::DxcCursor::get_all_children (wrapper.rs:210) ==168242== by 0x111B8A: intellisense_tu::main (intellisense-tu.rs:43) ==168242== by 0x112F5A: core::ops::function::FnOnce::call_once (function.rs:227) ==168242== by 0x1132FD: std::sys_common::backtrace::__rust_begin_short_backtrace (backtrace.rs:137) ==168242== by 0x113510: std::rt::lang_start::{{closure}} (rt.rs:66) ==168242== by 0x14F810: call_once<(),Fn<()>> (function.rs:259) ==168242== by 0x14F810: do_call<&Fn<()>,i32> (panicking.rs:373) ==168242== by 0x14F810: try<i32,&Fn<()>> (panicking.rs:337) ==168242== by 0x14F810: catch_unwind<&Fn<()>,i32> (panic.rs:379) ==168242== by 0x14F810: std::rt::lang_start_internal (rt.rs:51) ==168242== by 0x1134E6: std::rt::lang_start (rt.rs:65) * Convert return_hr to expression Make return_hr more versatile by providing Ok or Err as expession instead of forcing a return out of the current function. This follows Rusts convention to provide a block result as an unterminated expression on the last line, and allows it to be used in an expression context including with the question mark operator. * Free allocated memory using the appropriate API (free/CoTaskMemFree) On Windows the right API needs to be used to free this pointer in the same way as it was allocated by DXC (using CoTaskMemAlloc). On non-Windows platforms this call is [delegated] to `free` from libc. [delegated]: https://github.com/microsoft/DirectXShaderCompiler/blob/a8d9780046cb64a1cea842fa6fc28a250e3e2c09/include/dxc/Support/WinAdapter.h#L46 * Linux: util: Free BSTR after conversion to String The same call to SysFreeString happens in the Windows counterpart. Solves the following leaks found by valgrind: ==213075== 40 bytes in 1 blocks are definitely lost in loss record 4 of 13 ==213075== at 0x483A77F: malloc (vg_replace_malloc.c:307) ==213075== by 0x5B03395: ??? ==213075== by 0x5AFC77E: ??? ==213075== by 0x12C766: hassle_rs::intellisense::ffi::IDxcCursor::get_display_name (macros.rs:108) ==213075== by 0x116373: hassle_rs::intellisense::wrapper::DxcCursor::get_display_name (wrapper.rs:236) ==213075== by 0x11286D: intellisense_tu::main (intellisense-tu.rs:33) ==213075== by 0x11371A: core::ops::function::FnOnce::call_once (function.rs:227) ==213075== by 0x11348D: std::sys_common::backtrace::__rust_begin_short_backtrace (backtrace.rs:137) ==213075== by 0x111E10: std::rt::lang_start::{{closure}} (rt.rs:66) ==213075== by 0x150A20: call_once<(),Fn<()>> (function.rs:259) ==213075== by 0x150A20: do_call<&Fn<()>,i32> (panicking.rs:373) ==213075== by 0x150A20: try<i32,&Fn<()>> (panicking.rs:337) ==213075== by 0x150A20: catch_unwind<&Fn<()>,i32> (panic.rs:379) ==213075== by 0x150A20: std::rt::lang_start_internal (rt.rs:51) ==213075== by 0x111DE6: std::rt::lang_start (rt.rs:65) ==213075== ==213075== 56 (16 direct, 40 indirect) bytes in 1 blocks are definitely lost in loss record 5 of 13 ==213075== at 0x483ADEF: operator new(unsigned long) (vg_replace_malloc.c:342) ==213075== by 0x4FD058D: ??? ==213075== by 0x4FC04D9: ??? ==213075== by 0x40112DD: call_init.part.0 (in /usr/lib/ld-2.32.so) ==213075== by 0x40113C7: _dl_init (in /usr/lib/ld-2.32.so) ==213075== by 0x4A100E4: _dl_catch_exception (in /usr/lib/libc-2.32.so) ==213075== by 0x4015704: dl_open_worker (in /usr/lib/ld-2.32.so) ==213075== by 0x4A10087: _dl_catch_exception (in /usr/lib/libc-2.32.so) ==213075== by 0x4014F3D: _dl_open (in /usr/lib/ld-2.32.so) ==213075== by 0x489434B: ??? (in /usr/lib/libdl-2.32.so) ==213075== by 0x4A10087: _dl_catch_exception (in /usr/lib/libc-2.32.so) ==213075== by 0x4A10152: _dl_catch_error (in /usr/lib/libc-2.32.so) ==213075== ==213075== 124 bytes in 3 blocks are definitely lost in loss record 9 of 13 ==213075== at 0x483A77F: malloc (vg_replace_malloc.c:307) ==213075== by 0x5B03395: ??? ==213075== by 0x5AFC77E: ??? ==213075== by 0x12C766: hassle_rs::intellisense::ffi::IDxcCursor::get_display_name (macros.rs:108) ==213075== by 0x116373: hassle_rs::intellisense::wrapper::DxcCursor::get_display_name (wrapper.rs:236) ==213075== by 0x112F63: intellisense_tu::main (intellisense-tu.rs:52) ==213075== by 0x11371A: core::ops::function::FnOnce::call_once (function.rs:227) ==213075== by 0x11348D: std::sys_common::backtrace::__rust_begin_short_backtrace (backtrace.rs:137) ==213075== by 0x111E10: std::rt::lang_start::{{closure}} (rt.rs:66) ==213075== by 0x150A20: call_once<(),Fn<()>> (function.rs:259) ==213075== by 0x150A20: do_call<&Fn<()>,i32> (panicking.rs:373) ==213075== by 0x150A20: try<i32,&Fn<()>> (panicking.rs:337) ==213075== by 0x150A20: catch_unwind<&Fn<()>,i32> (panic.rs:379) ==213075== by 0x150A20: std::rt::lang_start_internal (rt.rs:51) ==213075== by 0x111DE6: std::rt::lang_start (rt.rs:65) Fixes: 94fbad7 ("Support libdxcompiler.so on Linux (#5)") * Linux: util: BSTR contains a wide string instead of UTF-8 Resulting strings were only one character long because the second byte for ASCII text is NULL. On Linux DXC properly uses wchar_t which is 32-bit instead of 16-bit. Fixes: 94fbad7 ("Support libdxcompiler.so on Linux (#5)") * Free leaking DxcIncludeHandlerWrapper Valgrind shows that we are leaking the DxcIncludeHandlerWrapper allocation: ==96758== 485 (80 direct, 405 indirect) bytes in 1 blocks are definitely lost in loss record 13 of 17 ==96758== at 0x483A77F: malloc (vg_replace_malloc.c:307) ==96758== by 0x1A1E4B: alloc::alloc::alloc (alloc.rs:74) ==96758== by 0x1A1F09: alloc::alloc::Global::alloc_impl (alloc.rs:153) ==96758== by 0x1A2069: <alloc::alloc::Global as core::alloc::AllocRef>::alloc (alloc.rs:203) ==96758== by 0x1A1D9A: alloc::alloc::exchange_malloc (alloc.rs:281) ==96758== by 0x199A1A: new<hassle_rs::wrapper::DxcIncludeHandlerWrapper> (boxed.rs:175) ==96758== by 0x199A1A: hassle_rs::wrapper::DxcCompiler::prep_include_handler (wrapper.rs:248) ==96758== by 0x199D6F: hassle_rs::wrapper::DxcCompiler::compile (wrapper.rs:278) ==96758== by 0x195C70: hassle_rs::utils::compile_hlsl (utils.rs:115) ==96758== by 0x127BDF: include::main (autogen_ops.rs:0) ==96758== by 0x1275CA: core::ops::function::FnOnce::call_once (autogen_context.rs:1683) ==96758== by 0x127ADD: std::sys_common::backtrace::__rust_begin_short_backtrace (backtrace.rs:137) ==96758== by 0x127530: std::rt::lang_start::{{closure}} (rt.rs:66) This happens because of Box::into_raw, which moves the pointer out of the box. Taking this pointer out of a referenced Box makes it live on till the end of the function where it is appropriately dropped. Properly freeing DxcIncludeHandlerWrapper exposes a second issue: The blobs are now freed but have already been freed within DXC becaues of a missing refcount: we are supposed to increment it before writing the pointer to it to `include_source` [1]. Now that the blobs have a proper refcount and live on until DXC cleans them up when not needed, they do not need to be kept alive inside DxcIncludeHandlerWrapper anymore. Finally, clean up the unnecessary clone of `source`: This String is already retrieved by value and can be moved straight into the Rc. [1]: https://github.com/microsoft/DirectXShaderCompiler/blob/a8d9780046cb64a1cea842fa6fc28a250e3e2c09/tools/clang/unittests/dxc_batch/dxc_batch.cpp#L553-L554

…extra trait" This reverts commit c37cb03. This [commit] is squashed in the merge of PR #5, and does not exist in-tree. [commit]: c37cb03

…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

MarijnS95 requested a review from Jasper-Bekkers July 20, 2020 09:11

MarijnS95 and others added 21 commits July 20, 2020 11:34

Cargo: Update all dependencies

b34d4c1

wip

a45a973

well, uh, at least the spirv example doesn't crash...

c49acdb

include example works too

3c32237

Type include_handler as *const IDxcIncludeHandler instead of c_void

14a61ae

[LINUX] Fix warnings

ea093bb

Fix check/clippy warnings

123c99b

os: Add BSTR type on non-Windows

4b1706a

lib: Fix bitflags macro include

a9501bd

extern crate requires deprecated `#[macro_use]`, which should be replaced with `use` instead.

Make intellisense compile on Linux

2a5856e

[LINUX ONLY] intellisense: Insert the usual destructor dummies

d056a6d

[WINDOWS] Make OS wrapper types compatible with Windows again

7df6938

HRESULT is 32-bit: https://en.wikipedia.org/wiki/HRESULT

Delete dummy functions (virtual dtors) again

760d493

This assumes a DXC compiled without virtual destructor! Which - as it stands - is invalid with undefined behaviour, but "works". This needs followup in DXC itself. This includes a revert of commit 308dd86.

[WIN/LINUX] Implement vtable dummies (virtual dtors) through extra trait

c37cb03

TODO: Might as well keep the IUnknown name and "hide" the COM one (less changes, as long as the right one is use'd.

[LINUX] wrapper: Library is in PWD

0cdd306

Add error type

f8f1dc8

Fix return type of IDxcType::get_kind

a5f295d

ffi: Replace c_void with the respective interface type

b9c441a

get_buffer_pointer returns a mutable (non-const) void ptr, create_include_handler returns an IDxcIncludeHandler, and annotate some c_void types. All the remaining c_void uses are that way in the official API.

Cargo.toml: Only include winapi dependency on cfg(windows)

6400883

Using winapi on not(windows) is a no-op already, but saves pulling in this extra dependency.

[LINUX] Disable DXIL validation at compile-time

df99e09

README: Mention libdxcompiler.so requirement on Linux

20b9a9b

MarijnS95 force-pushed the linux branch from 10d88c8 to 72ad32f Compare July 20, 2020 09:35

MarijnS95 commented Jul 20, 2020

View reviewed changes

examples/validate.rs Show resolved Hide resolved

Fix most clippy warnings

af94d99

MarijnS95 force-pushed the linux branch from 72ad32f to af94d99 Compare July 20, 2020 09:40

MarijnS95 commented Jul 20, 2020

View reviewed changes

src/wrapper.rs Outdated Show resolved Hide resolved

Remove dead code

6f9f481

MarijnS95 commented Jul 21, 2020

View reviewed changes

Jasper-Bekkers approved these changes Jul 21, 2020

View reviewed changes

Jasper-Bekkers merged commit 94fbad7 into master Jul 21, 2020

Jasper-Bekkers deleted the linux branch July 21, 2020 10:24

Jasper-Bekkers added 8 commits July 21, 2020 11:29

Silence clippy warnings in toplevel project

808dea6

Fix clippy warnings in example projects

9d07358

Formatting

0272110

Improve error handling

1c9a760

Add more context about the .dll / .so we're trying to load

5c56948

Linux compilation fix

4a21203

Expose helper on Linux as well even though it always returns an error…

44a3196

…, makes the API slightly easier to use

And export it

4c6a388

MarijnS95 commented Jul 21, 2020

View reviewed changes

MarijnS95 mentioned this pull request Nov 8, 2022

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support libdxcompiler.so on Linux #5

Support libdxcompiler.so on Linux #5

MarijnS95 commented Jul 20, 2020

MarijnS95 Jul 20, 2020

MarijnS95 Jul 20, 2020

MarijnS95 Jul 21, 2020

Jasper-Bekkers Jul 21, 2020

MarijnS95 Jul 21, 2020

MarijnS95 Jul 21, 2020

		#[error("Error loading library")]
		LoadLibraryError(#[from] libloading::Error),

Support libdxcompiler.so on Linux #5

Support libdxcompiler.so on Linux #5

Conversation

MarijnS95 commented Jul 20, 2020

MarijnS95 Jul 20, 2020

Choose a reason for hiding this comment

MarijnS95 Jul 20, 2020

Choose a reason for hiding this comment

MarijnS95 Jul 21, 2020

Choose a reason for hiding this comment

Jasper-Bekkers Jul 21, 2020

Choose a reason for hiding this comment

MarijnS95 Jul 21, 2020

Choose a reason for hiding this comment

MarijnS95 Jul 21, 2020

Choose a reason for hiding this comment