From a2fbd102010a20ac35ff0d950e44f193cd032751 Mon Sep 17 00:00:00 2001 From: Kenny Kerr Date: Sat, 27 Jan 2024 09:03:11 -0600 Subject: [PATCH] Optimize error propagation (#2819) --- crates/libs/core/src/error.rs | 101 +++++++++--------- crates/libs/core/src/imp/bindings.rs | 2 + crates/libs/core/src/imp/com_bindings.rs | 14 --- crates/libs/core/src/interface.rs | 5 +- crates/tests/core/Cargo.toml | 2 + crates/tests/core/tests/error.rs | 38 ++++++- .../tests/debugger_visualizer/tests/test.rs | 4 +- crates/tests/implement/tests/data_object.rs | 2 +- crates/tools/core/bindings.txt | 2 + crates/tools/core/com_bindings.txt | 2 - 10 files changed, 102 insertions(+), 70 deletions(-) diff --git a/crates/libs/core/src/error.rs b/crates/libs/core/src/error.rs index 91eccbc040..ef798193ff 100644 --- a/crates/libs/core/src/error.rs +++ b/crates/libs/core/src/error.rs @@ -1,23 +1,27 @@ +#![allow(missing_docs)] + use super::*; /// An error object consists of both an error code as well as detailed error information for debugging. #[derive(Clone, PartialEq, Eq)] pub struct Error { pub(crate) code: HRESULT, - pub(crate) info: Option, + pub(crate) info: Option, } +unsafe impl Send for Error {} +unsafe impl Sync for Error {} + impl Error { /// An error object without any failure information. pub const OK: Self = Self { code: HRESULT(0), info: None }; - /// This creates a new WinRT error object, capturing the stack and other information about the + /// This creates a new error object, capturing the stack and other information about the /// point of failure. pub fn new(code: HRESULT, message: HSTRING) -> Self { unsafe { crate::imp::RoOriginateError(code.0, std::mem::transmute_copy(&message)); - let info = GetErrorInfo().and_then(|e| e.cast()).ok(); - Self { code, info } + Self { code, info: GetErrorInfo() } } } @@ -31,43 +35,66 @@ impl Error { self.code } - /// The error information describing the error. - pub const fn info(&self) -> &Option { - &self.info + /// The error object describing the error. + pub fn info(&self) -> Option { + self.info.as_ref().and_then(|info| info.cast::().ok()) } /// The error message describing the error. pub fn message(&self) -> HSTRING { - // First attempt to retrieve the restricted error information. if let Some(info) = &self.info { - let mut fallback = BSTR::default(); let mut message = BSTR::default(); - let mut code = HRESULT(0); - unsafe { - let _ = info.GetErrorDetails(&mut fallback, &mut code, &mut message, &mut BSTR::default()); + // First attempt to retrieve the restricted error information. + if let Ok(info) = info.cast::() { + let mut fallback = BSTR::default(); + let mut code = HRESULT(0); + + unsafe { + // The vfptr is called directly to avoid the default error propagation logic. + _ = (info.vtable().GetErrorDetails)(info.as_raw(), &mut fallback as *mut _ as _, &mut code, &mut message as *mut _ as _, &mut BSTR::default() as *mut _ as _); + } + + if message.is_empty() { + message = fallback + }; } - if self.code == code { - let message = if !message.is_empty() { message } else { fallback }; - return HSTRING::from_wide(crate::imp::wide_trim_end(message.as_wide())).unwrap_or_default(); + // Next attempt to retrieve the regular error information. + if message.is_empty() { + unsafe { + // The vfptr is called directly to avoid the default error propagation logic. + _ = (info.vtable().GetDescription)(info.as_raw(), &mut message as *mut _ as _); + } } + + return HSTRING::from_wide(crate::imp::wide_trim_end(message.as_wide())).unwrap_or_default(); } + // Otherwise fallback to a generic error code description. self.code.message() } } impl From for HRESULT { fn from(error: Error) -> Self { - let code = error.code; - let info: Option = error.info.and_then(|info| info.cast().ok()); - - unsafe { - let _ = crate::imp::SetErrorInfo(0, info.as_ref()); + if error.info.is_some() { + unsafe { + crate::imp::SetErrorInfo(0, std::mem::transmute_copy(&error.info)); + } } - code + error.code + } +} + +impl From for Error { + fn from(code: HRESULT) -> Self { + let info = GetErrorInfo(); + + // Call CapturePropagationContext here if a use case presents itself. Otherwise, we can avoid the overhead for error propagation. + + Self { code, info } } } @@ -105,32 +132,6 @@ impl From for Error { } } -impl From for Error { - fn from(code: HRESULT) -> Self { - let info: Option = GetErrorInfo().and_then(|e| e.cast()).ok(); - - if let Some(info) = info { - // If it does (and therefore running on a recent version of Windows) - // then capture_propagation_context adds a breadcrumb to the error - // info to make debugging easier. - if let Ok(capture) = info.cast::() { - unsafe { - let _ = capture.CapturePropagationContext(None); - } - } - - return Self { code, info: Some(info) }; - } - - if let Ok(info) = GetErrorInfo() { - let message = unsafe { info.GetDescription().unwrap_or_default() }; - Self::new(code, HSTRING::from_wide(message.as_wide()).unwrap_or_default()) - } else { - Self { code, info: None } - } - } -} - impl std::fmt::Debug for Error { fn fmt(&self, fmt: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { let mut debug = fmt.debug_struct("Error"); @@ -151,6 +152,8 @@ impl std::fmt::Display for Error { impl std::error::Error for Error {} -fn GetErrorInfo() -> Result { - unsafe { crate::imp::GetErrorInfo(0) } +fn GetErrorInfo() -> Option { + let mut info = None; + unsafe { crate::imp::GetErrorInfo(0, &mut info as *mut _ as _) }; + info } diff --git a/crates/libs/core/src/imp/bindings.rs b/crates/libs/core/src/imp/bindings.rs index 05b0371cbd..3887120f5b 100644 --- a/crates/libs/core/src/imp/bindings.rs +++ b/crates/libs/core/src/imp/bindings.rs @@ -21,6 +21,8 @@ ::windows_targets::link!("ole32.dll" "system" fn CoTaskMemFree(pv : *const ::core::ffi::c_void)); ::windows_targets::link!("ole32.dll" "system" fn PropVariantClear(pvar : *mut PROPVARIANT) -> HRESULT); ::windows_targets::link!("ole32.dll" "system" fn PropVariantCopy(pvardest : *mut PROPVARIANT, pvarsrc : *const PROPVARIANT) -> HRESULT); +::windows_targets::link!("oleaut32.dll" "system" fn GetErrorInfo(dwreserved : u32, pperrinfo : *mut * mut::core::ffi::c_void) -> HRESULT); +::windows_targets::link!("oleaut32.dll" "system" fn SetErrorInfo(dwreserved : u32, perrinfo : * mut::core::ffi::c_void) -> HRESULT); ::windows_targets::link!("oleaut32.dll" "system" fn SysAllocStringLen(strin : PCWSTR, ui : u32) -> BSTR); ::windows_targets::link!("oleaut32.dll" "system" fn SysFreeString(bstrstring : BSTR)); ::windows_targets::link!("oleaut32.dll" "system" fn SysStringLen(pbstr : BSTR) -> u32); diff --git a/crates/libs/core/src/imp/com_bindings.rs b/crates/libs/core/src/imp/com_bindings.rs index 699fe0f21d..1c92f715fd 100644 --- a/crates/libs/core/src/imp/com_bindings.rs +++ b/crates/libs/core/src/imp/com_bindings.rs @@ -16,20 +16,6 @@ where let mut result__ = ::std::mem::zeroed(); RoGetAgileReference(options, riid, punk.into_param().abi(), &mut result__).from_abi(result__) } -#[inline] -pub unsafe fn GetErrorInfo(dwreserved: u32) -> ::windows_core::Result { - ::windows_targets::link!("oleaut32.dll" "system" fn GetErrorInfo(dwreserved : u32, pperrinfo : *mut * mut::core::ffi::c_void) -> ::windows_core::HRESULT); - let mut result__ = ::std::mem::zeroed(); - GetErrorInfo(dwreserved, &mut result__).from_abi(result__) -} -#[inline] -pub unsafe fn SetErrorInfo(dwreserved: u32, perrinfo: P0) -> ::windows_core::Result<()> -where - P0: ::windows_core::IntoParam, -{ - ::windows_targets::link!("oleaut32.dll" "system" fn SetErrorInfo(dwreserved : u32, perrinfo : * mut::core::ffi::c_void) -> ::windows_core::HRESULT); - SetErrorInfo(dwreserved, perrinfo.into_param().abi()).ok() -} pub const AGILEREFERENCE_DEFAULT: AgileReferenceOptions = AgileReferenceOptions(0i32); #[repr(transparent)] #[derive(::core::cmp::PartialEq, ::core::cmp::Eq, ::core::marker::Copy, ::core::clone::Clone, ::core::default::Default)] diff --git a/crates/libs/core/src/interface.rs b/crates/libs/core/src/interface.rs index 9fbe7d7ce9..39f3706d27 100644 --- a/crates/libs/core/src/interface.rs +++ b/crates/libs/core/src/interface.rs @@ -79,10 +79,13 @@ pub unsafe trait Interface: Sized + Clone { /// named cast. fn cast(&self) -> Result { let mut result = None; + // SAFETY: `result` is valid for writing an interface pointer and it is safe // to cast the `result` pointer as `T` on success because we are using the `IID` tied // to `T` which the implementor of `Interface` has guaranteed is correct - unsafe { self.query(&T::IID, &mut result as *mut _ as _).and_some(result) } + unsafe { _ = self.query(&T::IID, &mut result as *mut _ as _) }; + + result.ok_or_else(|| Error { code: crate::imp::E_NOINTERFACE, info: None }) } /// Attempts to create a [`Weak`] reference to this object. diff --git a/crates/tests/core/Cargo.toml b/crates/tests/core/Cargo.toml index 0c105caa45..1cedb18ed3 100644 --- a/crates/tests/core/Cargo.toml +++ b/crates/tests/core/Cargo.toml @@ -10,6 +10,8 @@ features = [ "implement", "Win32_Foundation", "Win32_System_WinRT", + "Win32_System_Ole", + "Win32_System_Com", "Win32_Media_Audio", ] diff --git a/crates/tests/core/tests/error.rs b/crates/tests/core/tests/error.rs index 00d3cb6503..baa5ad44a1 100644 --- a/crates/tests/core/tests/error.rs +++ b/crates/tests/core/tests/error.rs @@ -1,4 +1,7 @@ -use windows::{core::*, Win32::Foundation::*, Win32::Media::Audio::*}; +use windows::{ + core::*, Win32::Foundation::*, Win32::Media::Audio::*, Win32::System::Com::*, + Win32::System::Ole::*, Win32::System::WinRT::*, +}; #[test] fn display_debug() { @@ -29,3 +32,36 @@ fn hresult_last_error() { assert_eq!(e.code(), CRYPT_E_NOT_FOUND); } } + +// Checks that non-restricted error info is reported. +#[test] +fn set_error_info() -> Result<()> { + unsafe { + let creator = CreateErrorInfo()?; + creator.SetDescription(w!("message"))?; + SetErrorInfo(0, &creator.cast::()?)?; + + assert_eq!(Error::from(E_FAIL).message(), "message"); + SetErrorInfo(0, None)?; + assert_eq!(Error::from(E_FAIL).message(), "Unspecified error"); + + Ok(()) + } +} + +// https://github.com/microsoft/cppwinrt/pull/1386 +#[test] +fn suppressed_error_info() -> Result<()> { + unsafe { RoSetErrorReportingFlags(RO_ERROR_REPORTING_SUPPRESSSETERRORINFO.0 as u32)? }; + + assert_eq!( + Error::new(E_FAIL, "message".into()).message(), + "Unspecified error" + ); + + unsafe { RoSetErrorReportingFlags(RO_ERROR_REPORTING_USESETERRORINFO.0 as u32)? }; + + assert_eq!(Error::new(E_FAIL, "message".into()).message(), "message"); + + Ok(()) +} diff --git a/crates/tests/debugger_visualizer/tests/test.rs b/crates/tests/debugger_visualizer/tests/test.rs index b4dce61d12..4f90ba2851 100644 --- a/crates/tests/debugger_visualizer/tests/test.rs +++ b/crates/tests/debugger_visualizer/tests/test.rs @@ -175,11 +175,11 @@ hstring : "This is an HSTRING" [Type: windows_core::strings::hstring::H out_of_memory_error : 0x8007000e (Not enough memory resources are available to complete this operation.) [Type: windows_core::error::Error] [] [Type: windows_core::error::Error] - [info] : Some [Type: enum2$ >] + [info] : Some [Type: enum2$ >] invalid_argument_error : 0x80070057 (The parameter is incorrect.) [Type: windows_core::error::Error] [] [Type: windows_core::error::Error] - [info] : Some [Type: enum2$ >] + [info] : Some [Type: enum2$ >] "# )] fn test_debugger_visualizer() { diff --git a/crates/tests/implement/tests/data_object.rs b/crates/tests/implement/tests/data_object.rs index 6c7ea48636..8d28433f9d 100644 --- a/crates/tests/implement/tests/data_object.rs +++ b/crates/tests/implement/tests/data_object.rs @@ -100,7 +100,7 @@ fn test() -> Result<()> { assert!(r.is_err()); let e = r.unwrap_err(); assert!(e.code() == S_OK); - assert!(e.info().is_none()); + assert!(e.info::().is_none()); d.DAdvise(&Default::default(), 0, None)?; diff --git a/crates/tools/core/bindings.txt b/crates/tools/core/bindings.txt index b459faecb0..5107f2ff2f 100644 --- a/crates/tools/core/bindings.txt +++ b/crates/tools/core/bindings.txt @@ -70,3 +70,5 @@ Windows.Win32.System.Variant.VT_UNKNOWN Windows.Win32.System.WinRT.RoGetActivationFactory Windows.Win32.System.WinRT.RoOriginateError + Windows.Win32.System.Com.GetErrorInfo + Windows.Win32.System.Com.SetErrorInfo diff --git a/crates/tools/core/com_bindings.txt b/crates/tools/core/com_bindings.txt index 6c506ece7f..ad68b374bb 100644 --- a/crates/tools/core/com_bindings.txt +++ b/crates/tools/core/com_bindings.txt @@ -17,10 +17,8 @@ Windows.Win32.Foundation.RPC_E_DISCONNECTED Windows.Win32.Foundation.TYPE_E_TYPEMISMATCH Windows.Win32.System.Com.CoCreateGuid - Windows.Win32.System.Com.GetErrorInfo Windows.Win32.System.Com.IAgileObject Windows.Win32.System.Com.IErrorInfo - Windows.Win32.System.Com.SetErrorInfo Windows.Win32.System.WinRT.AGILEREFERENCE_DEFAULT Windows.Win32.System.WinRT.IAgileReference Windows.Win32.System.WinRT.ILanguageExceptionErrorInfo2