From 2222e569d5b8fb76c3b60f5d136691f8771c6798 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Fri, 30 Oct 2020 12:29:28 +0100 Subject: [PATCH 1/7] 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. --- src/utils.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/utils.rs b/src/utils.rs index 675b362..e473f77 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -69,9 +69,10 @@ pub enum HassleError { CompileError(String), #[error("Validation error: {0}")] ValidationError(String), - #[error("Error loading library {filename:?} / {inner:?}")] + #[error("Failed to load library {filename:?}: {inner:?}")] LoadLibraryError { filename: String, + #[source] inner: libloading::Error, }, #[error("LibLoading error: {0:?}")] From 8706e37ce59c2f0edaa71413b3249b9fd8bd117a Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Fri, 30 Oct 2020 14:57:06 +0100 Subject: [PATCH 2/7] 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> (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) --- src/intellisense/wrapper.rs | 84 +++++++++++-------------------------- 1 file changed, 25 insertions(+), 59 deletions(-) diff --git a/src/intellisense/wrapper.rs b/src/intellisense/wrapper.rs index 0a42080..1201acd 100644 --- a/src/intellisense/wrapper.rs +++ b/src/intellisense/wrapper.rs @@ -177,64 +177,33 @@ impl DxcCursor { let mut result: *mut *mut IDxcCursor = std::ptr::null_mut(); let mut result_length: u32 = 0; - let mut children = vec![]; return_hr!( self.inner .get_children(skip, max_count, &mut result_length, &mut result), - { - for i in 0..result_length { + // get_children allocates a buffer to pass the result in. Vec will + // free it on Drop. + Vec::from_raw_parts(result, result_length as usize, result_length as usize) + .into_iter() + .map(|ptr| { let mut childcursor = ComPtr::::new(); - - let ptr: *mut *mut IDxcCursor = childcursor.as_mut_ptr(); - - *ptr = (*result).offset(i as isize); - - children.push(DxcCursor::new(childcursor)); - } - children - } + *childcursor.as_mut_ptr() = ptr; + DxcCursor::new(childcursor) + }) + .collect::>() ); } } pub fn get_all_children(&self) -> Result, HRESULT> { - let max_children_count = 10; - let mut current_children_count = 0; + const MAX_CHILDREN_PER_CHUNK: u32 = 10; let mut children = vec![]; - unsafe { - let mut result: *mut *mut IDxcCursor = std::ptr::null_mut(); - let mut result_length: u32 = 0; - - loop { - let hr = self.inner.get_children( - current_children_count, - max_children_count, - &mut result_length, - &mut result, - ); - - if hr != 0 { - return Err(hr); - } - - for i in 0..result_length { - let mut childcursor = ComPtr::::new(); - - let ptr: &mut *mut IDxcCursor = childcursor.as_mut_ptr(); - - *ptr = *(result.offset(i as isize)); - - let dxc_cursor = DxcCursor::new(childcursor); - - children.push(dxc_cursor); - } - - if result_length < max_children_count { - return Ok(children); - } - - current_children_count += result_length; + loop { + let res = self.get_children(children.len() as u32, MAX_CHILDREN_PER_CHUNK)?; + let res_len = res.len(); + children.extend(res); + if res_len < MAX_CHILDREN_PER_CHUNK as usize { + break Ok(children); } } } @@ -384,7 +353,6 @@ impl DxcCursor { let mut result: *mut *mut IDxcCursor = std::ptr::null_mut(); let mut result_length: u32 = 0; - let mut children = vec![]; return_hr!( self.inner.find_references_in_file( file.inner.as_ptr(), @@ -393,18 +361,16 @@ impl DxcCursor { &mut result_length, &mut result ), - { - for i in 0..result_length { + // find_references_in_file allocates a buffer to pass the result in. + // Vec will free it on Drop. + Vec::from_raw_parts(result, result_length as usize, result_length as usize) + .into_iter() + .map(|ptr| { let mut childcursor = ComPtr::::new(); - - let ptr: *mut *mut IDxcCursor = childcursor.as_mut_ptr(); - - *ptr = (*result).offset(i as isize); - - children.push(DxcCursor::new(childcursor)); - } - children - } + *childcursor.as_mut_ptr() = ptr; + DxcCursor::new(childcursor) + }) + .collect::>() ); } } From ff28be085b29d9414573474a103e02fcbc44840a Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Tue, 3 Nov 2020 19:34:23 +0100 Subject: [PATCH 3/7] 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. --- src/intellisense/wrapper.rs | 116 ++++++++++++++++++------------------ src/wrapper.rs | 56 ++++++++--------- 2 files changed, 85 insertions(+), 87 deletions(-) diff --git a/src/intellisense/wrapper.rs b/src/intellisense/wrapper.rs index 1201acd..89a44f3 100644 --- a/src/intellisense/wrapper.rs +++ b/src/intellisense/wrapper.rs @@ -18,20 +18,20 @@ impl DxcIntellisense { pub fn get_default_editing_tu_options(&self) -> Result { let mut options: DxcTranslationUnitFlags = DxcTranslationUnitFlags::NONE; unsafe { - return_hr!( + check_hr!( self.inner.get_default_editing_tu_options(&mut options), options - ); + ) } } pub fn create_index(&self) -> Result { let mut index: ComPtr = ComPtr::new(); unsafe { - return_hr!( + check_hr!( self.inner.create_index(index.as_mut_ptr()), DxcIndex::new(index) - ); + ) } } @@ -45,7 +45,7 @@ impl DxcIntellisense { let mut file: ComPtr = ComPtr::new(); unsafe { - return_hr!( + check_hr!( self.inner.create_unsaved_file( c_file_name.as_ptr(), c_contents.as_ptr(), @@ -53,7 +53,7 @@ impl DxcIntellisense { file.as_mut_ptr() ), DxcUnsavedFile::new(file) - ); + ) } } } @@ -97,7 +97,7 @@ impl DxcIndex { } let mut tu: ComPtr = ComPtr::new(); - return_hr!( + check_hr!( self.inner.parse_translation_unit( c_source_filename.as_ptr() as *const u8, cliargs.as_ptr(), @@ -108,7 +108,7 @@ impl DxcIndex { tu.as_mut_ptr() ), DxcTranslationUnit::new(tu) - ); + ) } } } @@ -121,9 +121,7 @@ pub struct DxcUnsavedFile { impl DxcUnsavedFile { pub fn get_length(&self) -> Result { let mut length: u32 = 0; - unsafe { - return_hr!(self.inner.get_length(&mut length), length); - } + unsafe { check_hr!(self.inner.get_length(&mut length), length) } } fn new(inner: ComPtr) -> Self { @@ -144,20 +142,20 @@ impl DxcTranslationUnit { pub fn get_file(&self, name: &[u8]) -> Result { let mut file: ComPtr = ComPtr::new(); unsafe { - return_hr!( + check_hr!( self.inner.get_file(name.as_ptr(), file.as_mut_ptr()), DxcFile::new(file) - ); + ) } } pub fn get_cursor(&self) -> Result { let mut cursor: ComPtr = ComPtr::new(); unsafe { - return_hr!( + check_hr!( self.inner.get_cursor(cursor.as_mut_ptr()), DxcCursor::new(cursor) - ); + ) } } } @@ -177,7 +175,7 @@ impl DxcCursor { let mut result: *mut *mut IDxcCursor = std::ptr::null_mut(); let mut result_length: u32 = 0; - return_hr!( + check_hr!( self.inner .get_children(skip, max_count, &mut result_length, &mut result), // get_children allocates a buffer to pass the result in. Vec will @@ -190,7 +188,7 @@ impl DxcCursor { DxcCursor::new(childcursor) }) .collect::>() - ); + ) } } @@ -211,135 +209,135 @@ impl DxcCursor { pub fn get_extent(&self) -> Result { unsafe { let mut range: ComPtr = ComPtr::new(); - return_hr!( + check_hr!( self.inner.get_extent(range.as_mut_ptr()), DxcSourceRange::new(range) - ); + ) } } pub fn get_location(&self) -> Result { unsafe { let mut location: ComPtr = ComPtr::new(); - return_hr!( + check_hr!( self.inner.get_location(location.as_mut_ptr()), DxcSourceLocation::new(location) - ); + ) } } pub fn get_display_name(&self) -> Result { unsafe { let mut name: BSTR = std::ptr::null_mut(); - return_hr!( + check_hr!( self.inner.get_display_name(&mut name), crate::utils::from_bstr(name) - ); + ) } } pub fn get_formatted_name(&self, formatting: DxcCursorFormatting) -> Result { unsafe { let mut name: BSTR = std::ptr::null_mut(); - return_hr!( + check_hr!( self.inner.get_formatted_name(formatting, &mut name), crate::utils::from_bstr(name) - ); + ) } } pub fn get_qualified_name(&self, include_template_args: bool) -> Result { unsafe { let mut name: BSTR = std::ptr::null_mut(); - return_hr!( + check_hr!( self.inner .get_qualified_name(include_template_args, &mut name), crate::utils::from_bstr(name) - ); + ) } } pub fn get_kind(&self) -> Result { unsafe { let mut cursor_kind: DxcCursorKind = DxcCursorKind::UNEXPOSED_DECL; - return_hr!(self.inner.get_kind(&mut cursor_kind), cursor_kind); + check_hr!(self.inner.get_kind(&mut cursor_kind), cursor_kind) } } pub fn get_kind_flags(&self) -> Result { unsafe { let mut cursor_kind_flags: DxcCursorKindFlags = DxcCursorKindFlags::NONE; - return_hr!( + check_hr!( self.inner.get_kind_flags(&mut cursor_kind_flags), cursor_kind_flags - ); + ) } } pub fn get_semantic_parent(&self) -> Result { unsafe { let mut inner = ComPtr::::new(); - return_hr!( + check_hr!( self.inner.get_semantic_parent(inner.as_mut_ptr()), DxcCursor::new(inner) - ); + ) } } pub fn get_lexical_parent(&self) -> Result { unsafe { let mut inner = ComPtr::::new(); - return_hr!( + check_hr!( self.inner.get_lexical_parent(inner.as_mut_ptr()), DxcCursor::new(inner) - ); + ) } } pub fn get_cursor_type(&self) -> Result { unsafe { let mut inner = ComPtr::::new(); - return_hr!( + check_hr!( self.inner.get_cursor_type(inner.as_mut_ptr()), DxcType::new(inner) - ); + ) } } pub fn get_num_arguments(&self) -> Result { unsafe { let mut result: i32 = 0; - return_hr!(self.inner.get_num_arguments(&mut result), result); + check_hr!(self.inner.get_num_arguments(&mut result), result) } } pub fn get_argument_at(&self, index: i32) -> Result { unsafe { let mut inner = ComPtr::::new(); - return_hr!( + check_hr!( self.inner.get_argument_at(index, inner.as_mut_ptr()), DxcCursor::new(inner) - ); + ) } } pub fn get_referenced_cursor(&self) -> Result { unsafe { let mut inner = ComPtr::::new(); - return_hr!( + check_hr!( self.inner.get_referenced_cursor(inner.as_mut_ptr()), DxcCursor::new(inner) - ); + ) } } pub fn get_definition_cursor(&self) -> Result { unsafe { let mut inner = ComPtr::::new(); - return_hr!( + check_hr!( self.inner.get_definition_cursor(inner.as_mut_ptr()), DxcCursor::new(inner) - ); + ) } } @@ -353,7 +351,7 @@ impl DxcCursor { let mut result: *mut *mut IDxcCursor = std::ptr::null_mut(); let mut result_length: u32 = 0; - return_hr!( + check_hr!( self.inner.find_references_in_file( file.inner.as_ptr(), skip, @@ -371,52 +369,52 @@ impl DxcCursor { DxcCursor::new(childcursor) }) .collect::>() - ); + ) } } pub fn get_spelling(&self) -> Result { unsafe { let mut spelling: LPSTR = std::ptr::null_mut(); - return_hr!( + check_hr!( self.inner.get_spelling(&mut spelling), crate::utils::from_lpstr(spelling) - ); + ) } } pub fn is_equal_to(&self, other: &DxcCursor) -> Result { unsafe { let mut result: bool = false; - return_hr!( + check_hr!( self.inner.is_equal_to(other.inner.as_ptr(), &mut result), result - ); + ) } } pub fn is_null(&mut self) -> Result { unsafe { let mut result: bool = false; - return_hr!(IDxcCursor::is_null(&self.inner, &mut result), result); + check_hr!(IDxcCursor::is_null(&self.inner, &mut result), result) } } pub fn is_definition(&self) -> Result { unsafe { let mut result: bool = false; - return_hr!(self.inner.is_definition(&mut result), result); + check_hr!(self.inner.is_definition(&mut result), result) } } pub fn get_snapped_child(&self, location: &DxcSourceLocation) -> Result { unsafe { let mut inner = ComPtr::::new(); - return_hr!( + check_hr!( self.inner .get_snapped_child(location.inner.as_ptr(), inner.as_mut_ptr()), DxcCursor::new(inner) - ); + ) } } @@ -447,10 +445,10 @@ impl DxcType { pub fn get_spelling(&self) -> Result { unsafe { let mut spelling: LPSTR = std::ptr::null_mut(); - return_hr!( + check_hr!( self.inner.get_spelling(&mut spelling), crate::utils::from_lpstr(spelling) - ); + ) } } } @@ -482,13 +480,13 @@ impl DxcSourceRange { unsafe { let mut start_offset: u32 = 0; let mut end_offset: u32 = 0; - return_hr!( + check_hr!( self.inner.get_offsets(&mut start_offset, &mut end_offset), DxcSourceOffsets { start_offset, end_offset } - ); + ) } } } @@ -513,13 +511,13 @@ impl DxcFile { impl Dxc { pub fn create_intellisense(&self) -> Result { let mut intellisense: ComPtr = ComPtr::new(); - return_hr_wrapped!( + check_hr_wrapped!( self.get_dxc_create_instance()?( &CLSID_DxcIntelliSense, &IID_IDxcIntelliSense, intellisense.as_mut_ptr(), ), DxcIntellisense::new(intellisense) - ); + ) } } diff --git a/src/wrapper.rs b/src/wrapper.rs index 2091cf1..a0f732f 100644 --- a/src/wrapper.rs +++ b/src/wrapper.rs @@ -14,26 +14,26 @@ use std::ffi::c_void; use std::rc::Rc; #[macro_export] -macro_rules! return_hr { - ($hr:expr, $v: expr) => { +macro_rules! check_hr { + ($hr:expr, $v: expr) => {{ let hr = $hr; if hr == 0 { - return Ok($v); + Ok($v) } else { - return Err(hr); + Err(hr) } - }; + }}; } -macro_rules! return_hr_wrapped { - ($hr:expr, $v: expr) => { +macro_rules! check_hr_wrapped { + ($hr:expr, $v: expr) => {{ let hr = $hr; if hr == 0 { - return Ok($v); + Ok($v) } else { - return Err(HassleError::Win32Error(hr)); + Err(HassleError::Win32Error(hr)) } - }; + }}; } #[derive(Debug)] @@ -90,23 +90,23 @@ impl DxcOperationResult { pub fn get_status(&self) -> Result { let mut status: u32 = 0; - return_hr!(unsafe { self.inner.get_status(&mut status) }, status); + check_hr!(unsafe { self.inner.get_status(&mut status) }, status) } pub fn get_result(&self) -> Result { let mut blob: ComPtr = ComPtr::new(); - return_hr!( + check_hr!( unsafe { self.inner.get_result(blob.as_mut_ptr()) }, DxcBlob::new(blob) - ); + ) } pub fn get_error_buffer(&self) -> Result { let mut blob: ComPtr = ComPtr::new(); - return_hr!( + check_hr!( unsafe { self.inner.get_error_buffer(blob.as_mut_ptr()) }, DxcBlobEncoding::new(blob) - ); + ) } } @@ -408,13 +408,13 @@ impl DxcCompiler { pub fn disassemble(&self, blob: &DxcBlob) -> Result { let mut result_blob: ComPtr = ComPtr::new(); - return_hr!( + check_hr!( unsafe { self.inner .disassemble(blob.inner.as_ptr(), result_blob.as_mut_ptr()) }, DxcBlobEncoding::new(result_blob) - ); + ) } } @@ -430,7 +430,7 @@ impl DxcLibrary { pub fn create_blob_with_encoding(&self, data: &[u8]) -> Result { let mut blob: ComPtr = ComPtr::new(); - return_hr!( + check_hr!( unsafe { self.inner.create_blob_with_encoding_from_pinned( data.as_ptr() as *const c_void, @@ -440,7 +440,7 @@ impl DxcLibrary { ) }, DxcBlobEncoding::new(blob) - ); + ) } pub fn create_blob_with_encoding_from_str( @@ -450,7 +450,7 @@ impl DxcLibrary { let mut blob: ComPtr = ComPtr::new(); const CP_UTF8: u32 = 65001; // UTF-8 translation - return_hr!( + check_hr!( unsafe { self.inner.create_blob_with_encoding_from_pinned( text.as_ptr() as *const c_void, @@ -460,7 +460,7 @@ impl DxcLibrary { ) }, DxcBlobEncoding::new(blob) - ); + ) } pub fn get_blob_as_string(&self, blob: &DxcBlobEncoding) -> String { @@ -521,26 +521,26 @@ impl Dxc { pub fn create_compiler(&self) -> Result { let mut compiler: ComPtr = ComPtr::new(); - return_hr_wrapped!( + check_hr_wrapped!( self.get_dxc_create_instance()?( &CLSID_DxcCompiler, &IID_IDxcCompiler2, compiler.as_mut_ptr(), ), DxcCompiler::new(compiler, self.create_library()?) - ); + ) } pub fn create_library(&self) -> Result { let mut library: ComPtr = ComPtr::new(); - return_hr_wrapped!( + check_hr_wrapped!( self.get_dxc_create_instance()?( &CLSID_DxcLibrary, &IID_IDxcLibrary, library.as_mut_ptr(), ), DxcLibrary::new(library) - ); + ) } } @@ -571,7 +571,7 @@ impl DxcValidator { let mut major = 0; let mut minor = 0; - return_hr! { + check_hr! { unsafe { version.get_version(&mut major, &mut minor) }, (major, minor) } @@ -629,13 +629,13 @@ impl Dxil { pub fn create_validator(&self) -> Result { let mut validator: ComPtr = ComPtr::new(); - return_hr_wrapped!( + check_hr_wrapped!( self.get_dxc_create_instance()?( &CLSID_DxcValidator, &IID_IDxcValidator, validator.as_mut_ptr(), ), DxcValidator::new(validator) - ); + ) } } From 75eb94f73e2cede3eafca47b2a46c75cbccf8f17 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Tue, 10 Nov 2020 11:44:35 +0100 Subject: [PATCH 4/7] 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 --- Cargo.toml | 5 +++- src/intellisense/wrapper.rs | 48 +++++++++++++++++++++---------------- src/os.rs | 8 +++++++ 3 files changed, 39 insertions(+), 22 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 2020aed..75bdcc7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -31,7 +31,10 @@ widestring = "0.4.2" thiserror = "1.0" [target.'cfg(windows)'.dependencies] -winapi = { version = "0.3.9", features = ["wtypes", "oleauto"] } +winapi = { version = "0.3.9", features = ["wtypes", "oleauto", "combaseapi"] } + +[target.'cfg(not(windows))'.dependencies] +libc = "0.2" [dev-dependencies] rspirv = "0.6.0" diff --git a/src/intellisense/wrapper.rs b/src/intellisense/wrapper.rs index 89a44f3..f240118 100644 --- a/src/intellisense/wrapper.rs +++ b/src/intellisense/wrapper.rs @@ -1,5 +1,5 @@ use crate::intellisense::ffi::*; -use crate::os::{BSTR, HRESULT, LPSTR}; +use crate::os::{CoTaskMemFree, BSTR, HRESULT, LPSTR}; use crate::utils::HassleError; use crate::wrapper::Dxc; use com_rs::ComPtr; @@ -178,16 +178,19 @@ impl DxcCursor { check_hr!( self.inner .get_children(skip, max_count, &mut result_length, &mut result), - // get_children allocates a buffer to pass the result in. Vec will - // free it on Drop. - Vec::from_raw_parts(result, result_length as usize, result_length as usize) - .into_iter() - .map(|ptr| { - let mut childcursor = ComPtr::::new(); - *childcursor.as_mut_ptr() = ptr; - DxcCursor::new(childcursor) - }) - .collect::>() + { + // get_children allocates a buffer to pass the result in. + let child_cursors = std::slice::from_raw_parts(result, result_length as usize) + .iter() + .map(|&ptr| { + let mut childcursor = ComPtr::::new(); + *childcursor.as_mut_ptr() = ptr; + DxcCursor::new(childcursor) + }) + .collect::>(); + CoTaskMemFree(result as *mut _); + child_cursors + } ) } } @@ -359,16 +362,19 @@ impl DxcCursor { &mut result_length, &mut result ), - // find_references_in_file allocates a buffer to pass the result in. - // Vec will free it on Drop. - Vec::from_raw_parts(result, result_length as usize, result_length as usize) - .into_iter() - .map(|ptr| { - let mut childcursor = ComPtr::::new(); - *childcursor.as_mut_ptr() = ptr; - DxcCursor::new(childcursor) - }) - .collect::>() + { + // find_references_in_file allocates a buffer to pass the result in. + let child_cursors = std::slice::from_raw_parts(result, result_length as usize) + .iter() + .map(|&ptr| { + let mut childcursor = ComPtr::::new(); + *childcursor.as_mut_ptr() = ptr; + DxcCursor::new(childcursor) + }) + .collect::>(); + CoTaskMemFree(result as *mut _); + child_cursors + } ) } } diff --git a/src/os.rs b/src/os.rs index bda56b8..b27a01e 100644 --- a/src/os.rs +++ b/src/os.rs @@ -4,6 +4,8 @@ mod os_defs { ntdef::{HRESULT, LPCSTR, LPCWSTR, LPSTR, LPWSTR, WCHAR}, wtypes::BSTR, }; + + pub use winapi::um::combaseapi::CoTaskMemFree; } #[cfg(not(windows))] @@ -18,6 +20,12 @@ mod os_defs { pub type BSTR = *mut OLECHAR; pub type LPBSTR = *mut BSTR; pub type HRESULT = i32; + + #[allow(non_snake_case)] + pub unsafe fn CoTaskMemFree(p: *mut libc::c_void) { + // https://github.com/microsoft/DirectXShaderCompiler/blob/a8d9780046cb64a1cea842fa6fc28a250e3e2c09/include/dxc/Support/WinAdapter.h#L46 + libc::free(p) + } } pub use os_defs::*; From aeba1b6e4eb7881114f16f57070722d15a65e0b3 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Tue, 10 Nov 2020 13:29:51 +0100 Subject: [PATCH 5/7] 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> (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> (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)") --- src/os.rs | 7 +++++++ src/utils.rs | 8 +++++--- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/os.rs b/src/os.rs index b27a01e..d46c25e 100644 --- a/src/os.rs +++ b/src/os.rs @@ -6,6 +6,7 @@ mod os_defs { }; pub use winapi::um::combaseapi::CoTaskMemFree; + pub use winapi::um::oleauto::SysFreeString; } #[cfg(not(windows))] @@ -26,6 +27,12 @@ mod os_defs { // https://github.com/microsoft/DirectXShaderCompiler/blob/a8d9780046cb64a1cea842fa6fc28a250e3e2c09/include/dxc/Support/WinAdapter.h#L46 libc::free(p) } + + #[allow(non_snake_case)] + pub unsafe fn SysFreeString(p: BSTR) { + // https://github.com/microsoft/DirectXShaderCompiler/blob/a8d9780046cb64a1cea842fa6fc28a250e3e2c09/include/dxc/Support/WinAdapter.h#L48-L50 + libc::free(p as _) + } } pub use os_defs::*; diff --git a/src/utils.rs b/src/utils.rs index e473f77..db504cd 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -1,9 +1,9 @@ -use crate::os::{BSTR, HRESULT, LPSTR, LPWSTR, WCHAR}; +use crate::os::{SysFreeString, BSTR, HRESULT, LPSTR, LPWSTR, WCHAR}; use crate::wrapper::*; use thiserror::Error; #[cfg(windows)] -use winapi::um::oleauto::{SysFreeString, SysStringLen}; +use winapi::um::oleauto::SysStringLen; pub(crate) fn to_wide(msg: &str) -> Vec { widestring::WideCString::from_str(msg).unwrap().into_vec() @@ -33,7 +33,9 @@ pub(crate) fn from_bstr(string: BSTR) -> String { // TODO (Marijn): This does NOT cover embedded NULLs: // https://docs.microsoft.com/en-us/windows/win32/api/oleauto/nf-oleauto-sysstringlen#remarks // Fortunately BSTRs are only used in names currently, which _likely_ don't include NULL characters (like binary data) - from_lpstr(string as LPSTR) + let result = from_lpstr(string as LPSTR); + unsafe { SysFreeString(string) }; + result } pub(crate) fn from_lpstr(string: LPSTR) -> String { From f1ad454f57d559af5b2c1f46f1f8cd72a7409c7c Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Tue, 10 Nov 2020 13:39:26 +0100 Subject: [PATCH 6/7] 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)") --- examples/intellisense-tu.rs | 14 ++++++++++++++ src/utils.rs | 23 +++++++++++++++-------- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/examples/intellisense-tu.rs b/examples/intellisense-tu.rs index fcdd8f5..974972c 100644 --- a/examples/intellisense-tu.rs +++ b/examples/intellisense-tu.rs @@ -32,6 +32,7 @@ fn main() { let name = cursor.get_display_name().unwrap(); println!("Name {:?}", name); + assert_eq!(name, "copy.hlsl"); let cursor_kind = cursor.get_kind().unwrap(); println!("CursorKind {:?}", cursor_kind); @@ -42,6 +43,19 @@ fn main() { let child_cursors = cursor.get_all_children().unwrap(); + assert_eq!( + child_cursors[0].get_display_name(), + Ok("g_input".to_owned()) + ); + assert_eq!( + child_cursors[1].get_display_name(), + Ok("g_output".to_owned()) + ); + assert_eq!( + child_cursors[2].get_display_name(), + Ok("copyCs(uint3)".to_owned()) + ); + for child_cursor in child_cursors { let range = child_cursor.get_extent().unwrap(); println!("Child Range {:?}", range); diff --git a/src/utils.rs b/src/utils.rs index db504cd..eff0f9c 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -13,16 +13,19 @@ pub(crate) fn from_wide(wide: LPWSTR) -> String { unsafe { widestring::WideCStr::from_ptr_str(wide) .to_string() - .expect("utf16 decode failed") + .expect("widestring decode failed") } } #[cfg(windows)] pub(crate) fn from_bstr(string: BSTR) -> String { unsafe { - let len = SysStringLen(string); - let slice: &[WCHAR] = ::std::slice::from_raw_parts(string, len as usize); - let result = String::from_utf16(slice).unwrap(); + let len = SysStringLen(string) as usize; + + let result = widestring::WideCStr::from_ptr_with_nul(string, len) + .to_string() + .expect("widestring decode failed"); + SysFreeString(string); result } @@ -30,10 +33,14 @@ pub(crate) fn from_bstr(string: BSTR) -> String { #[cfg(not(windows))] pub(crate) fn from_bstr(string: BSTR) -> String { - // TODO (Marijn): This does NOT cover embedded NULLs: - // https://docs.microsoft.com/en-us/windows/win32/api/oleauto/nf-oleauto-sysstringlen#remarks - // Fortunately BSTRs are only used in names currently, which _likely_ don't include NULL characters (like binary data) - let result = from_lpstr(string as LPSTR); + // TODO (Marijn): This does NOT cover embedded NULLs + + // BSTR contains its size in the four bytes preceding the pointer, in order to contain NULL bytes: + // https://docs.microsoft.com/en-us/previous-versions/windows/desktop/automat/bstr + // DXC on non-Windows does not adhere to that and simply allocates a buffer without prepending the size: + // https://github.com/microsoft/DirectXShaderCompiler/blob/a8d9780046cb64a1cea842fa6fc28a250e3e2c09/include/dxc/Support/WinAdapter.h#L49-L50 + let result = from_wide(string as LPWSTR); + unsafe { SysFreeString(string) }; result } From 4afa35662bdbd980ac14637a4c94651dce41668a Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Tue, 10 Nov 2020 17:16:07 +0100 Subject: [PATCH 7/7] 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.rs:203) ==96758== by 0x1A1D9A: alloc::alloc::exchange_malloc (alloc.rs:281) ==96758== by 0x199A1A: new (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 --- src/wrapper.rs | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/wrapper.rs b/src/wrapper.rs index a0f732f..8896204 100644 --- a/src/wrapper.rs +++ b/src/wrapper.rs @@ -135,7 +135,6 @@ struct DxcIncludeHandlerWrapperVtbl { struct DxcIncludeHandlerWrapper<'a> { vtable: Box, handler: Box, - blobs: Vec, pinned: Vec>, library: &'a DxcLibrary, } @@ -162,22 +161,22 @@ impl<'a> DxcIncludeHandlerWrapper<'a> { let filename = crate::utils::from_wide(filename as *mut _); - let source = unsafe { &(*me).handler.load_source(filename) }; + let source = unsafe { (*me).handler.load_source(filename) }; if let Some(source) = source { - let pinned_source = Rc::new(source.clone()); + let pinned_source = Rc::new(source); let mut blob = unsafe { (*me) .library - .create_blob_with_encoding_from_str(&*pinned_source) + .create_blob_with_encoding_from_str(pinned_source.as_str()) .unwrap() }; unsafe { + blob.inner.add_ref(); *include_source = *blob.inner.as_mut_ptr(); - (*me).blobs.push(blob); - (*me).pinned.push(Rc::clone(&pinned_source)); + (*me).pinned.push(pinned_source); } 0 @@ -248,7 +247,6 @@ impl DxcCompiler { Some(Box::new(DxcIncludeHandlerWrapper { vtable: Box::new(vtable), handler: include_handler, - blobs: vec![], library, pinned: vec![], })) @@ -288,7 +286,9 @@ impl DxcCompiler { dxc_args.len() as u32, dxc_defines.as_ptr(), dxc_defines.len() as u32, - handler_wrapper.map_or(std::ptr::null(), |v| Box::into_raw(v) as _), + handler_wrapper + .as_ref() + .map_or(std::ptr::null(), |v| &**v as *const _ as *const _), result.as_mut_ptr(), ) }; @@ -339,7 +339,9 @@ impl DxcCompiler { dxc_args.len() as u32, dxc_defines.as_ptr(), dxc_defines.len() as u32, - handler_wrapper.map_or(std::ptr::null(), |v| Box::into_raw(v) as _), + handler_wrapper + .as_ref() + .map_or(std::ptr::null(), |v| &**v as *const _ as *const _), result.as_mut_ptr(), &mut debug_filename, debug_blob.as_mut_ptr(), @@ -389,7 +391,9 @@ impl DxcCompiler { dxc_args.len() as u32, dxc_defines.as_ptr(), dxc_defines.len() as u32, - handler_wrapper.map_or(std::ptr::null(), |v| Box::into_raw(v) as _), + handler_wrapper + .as_ref() + .map_or(std::ptr::null(), |v| &**v as *const _ as *const _), result.as_mut_ptr(), ) };