From 9711e97b26422b601df8ffedddc9c252ac5a3895 Mon Sep 17 00:00:00 2001 From: David Machaj <46852402+dmachaj@users.noreply.github.com.> Date: Sat, 26 Oct 2024 12:12:33 -0700 Subject: [PATCH 1/8] Squash to a single commit --- strings/base_implements.h | 6 +++ strings/base_meta.h | 2 +- strings/base_windows.h | 27 +++++++++++ test/test/missing_required_interfaces.cpp | 56 +++++++++++++++++++++++ test/test/test.vcxproj | 1 + test/test_component/test_component.idl | 7 +++ 6 files changed, 98 insertions(+), 1 deletion(-) create mode 100644 test/test/missing_required_interfaces.cpp diff --git a/strings/base_implements.h b/strings/base_implements.h index d0bb09012..aba869eff 100644 --- a/strings/base_implements.h +++ b/strings/base_implements.h @@ -788,6 +788,12 @@ namespace winrt::impl return m_inner.try_as(); } + template + auto as_or_failfast() const noexcept + { + return m_inner.as_or_failfast(); + } + explicit operator bool() const noexcept { return m_inner.operator bool(); diff --git a/strings/base_meta.h b/strings/base_meta.h index 2c1796e9c..de9493289 100644 --- a/strings/base_meta.h +++ b/strings/base_meta.h @@ -159,7 +159,7 @@ namespace winrt::impl { operator I() const noexcept { - return static_cast(this)->template try_as(); + return static_cast(this)->template as_or_failfast(); } }; diff --git a/strings/base_windows.h b/strings/base_windows.h index 41030d368..7bbe912b2 100644 --- a/strings/base_windows.h +++ b/strings/base_windows.h @@ -131,6 +131,27 @@ namespace winrt::impl ptr->QueryInterface(guid_of(), &result); return wrap_as_result(result); } + + template , int> = 0> + com_ref as_or_failfast(From* ptr) noexcept + { +#ifdef WINRT_DIAGNOSTICS + get_diagnostics_info().add_query(); +#endif + + if (!ptr) + { + return nullptr; + } + + void* result{}; + hresult code = ptr->QueryInterface(guid_of(), &result); + if (code < 0) + { + WINRT_IMPL_RoFailFastWithErrorContext(code); + } + return wrap_as_result(result); + } } WINRT_EXPORT namespace winrt::Windows::Foundation @@ -205,6 +226,12 @@ WINRT_EXPORT namespace winrt::Windows::Foundation return impl::try_as(m_ptr); } + template + auto as_or_failfast() const noexcept + { + return impl::as_or_failfast(m_ptr); + } + template void as(To& to) const { diff --git a/test/test/missing_required_interfaces.cpp b/test/test/missing_required_interfaces.cpp new file mode 100644 index 000000000..0649bc86b --- /dev/null +++ b/test/test/missing_required_interfaces.cpp @@ -0,0 +1,56 @@ +#include "pch.h" + +// Unset lean and mean so we can implement a type from the test_component namespace +#undef WINRT_LEAN_AND_MEAN +#include + +namespace +{ + struct LiesAboutInheritance : public winrt::implements + { + LiesAboutInheritance() = default; + void StubMethod() {} + }; +} + +bool g_failfastCalled = false; + +void __stdcall WINRT_IMPL_RoFailFastWithErrorContext(int32_t) noexcept +{ + g_failfastCalled = true; +} + +void DoTheUncatcheable(winrt::test_component::LiesAboutInheritance& lies) noexcept +{ + lies.ToString(); +} + +bool CatchTheUncatcheable(winrt::test_component::LiesAboutInheritance& lies) noexcept +{ + bool caughtCrash = false; + __try + { + // We verify that our replacement version of WINRT_IMPL_RoFailFastWithErrorContext is + // called. In a real program that would exit the process. But this is a test so it + // continues execution and hits a nullptr dereference instead. Eat that known/expected + // error and allow execution to continue onto other test cases. + DoTheUncatcheable(lies); + } + __except (EXCEPTION_EXECUTE_HANDLER) + { + caughtCrash = true; + } + return caughtCrash; +} + +TEST_CASE("missing_required_interfaces") +{ + auto lies = winrt::make_self().as(); + REQUIRE(lies); + REQUIRE_NOTHROW(lies.StubMethod()); + + REQUIRE(!g_failfastCalled); + const bool caughtCrash = CatchTheUncatcheable(lies); + REQUIRE(caughtCrash); + REQUIRE(g_failfastCalled); +} \ No newline at end of file diff --git a/test/test/test.vcxproj b/test/test/test.vcxproj index b75f2e219..a82465149 100644 --- a/test/test/test.vcxproj +++ b/test/test/test.vcxproj @@ -387,6 +387,7 @@ NotUsing + NotUsing NotUsing diff --git a/test/test_component/test_component.idl b/test/test_component/test_component.idl index fb7444889..a027c394c 100644 --- a/test/test_component/test_component.idl +++ b/test/test_component/test_component.idl @@ -163,6 +163,13 @@ namespace test_component static void StaticMethodWithAsyncReturn(); } + // This class declares that it implements another interface but under the covers it actually does + // not. This allows us to test the behavior when QI's that should not fail, do fail. + runtimeclass LiesAboutInheritance : Windows.Foundation.IStringable + { + void StubMethod(); + } + namespace Structs { struct All From 0b3279b0b38fd7cce9bf09302d245f1aefb95ed8 Mon Sep 17 00:00:00 2001 From: David Machaj <46852402+dmachaj@users.noreply.github.com.> Date: Mon, 28 Oct 2024 10:40:10 -0700 Subject: [PATCH 2/8] Stow error context right before failfast --- strings/base_extern.h | 1 + strings/base_windows.h | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/strings/base_extern.h b/strings/base_extern.h index 92872d416..2412f9f2c 100644 --- a/strings/base_extern.h +++ b/strings/base_extern.h @@ -29,6 +29,7 @@ extern "C" int32_t __stdcall WINRT_IMPL_SetThreadpoolTimerEx(winrt::impl::ptp_timer, void*, uint32_t, uint32_t) noexcept WINRT_IMPL_LINK(SetThreadpoolTimerEx, 16); int32_t __stdcall WINRT_IMPL_SetThreadpoolWaitEx(winrt::impl::ptp_wait, void*, void*, void*) noexcept WINRT_IMPL_LINK(SetThreadpoolWaitEx, 16); int32_t __stdcall WINRT_IMPL_RoOriginateLanguageException(int32_t error, void* message, void* exception) noexcept WINRT_IMPL_LINK(RoOriginateLanguageException, 12); + int32_t __stdcall WINRT_IMPL_RoCaptureErrorContext(int32_t error) noexcept WINRT_IMPL_LINK(RoCaptureErrorContext, 4); void __stdcall WINRT_IMPL_RoFailFastWithErrorContext(int32_t) noexcept WINRT_IMPL_LINK(RoFailFastWithErrorContext, 4); int32_t __stdcall WINRT_IMPL_RoTransformError(int32_t, int32_t, void*) noexcept WINRT_IMPL_LINK(RoTransformError, 12); diff --git a/strings/base_windows.h b/strings/base_windows.h index 7bbe912b2..9ef2f1907 100644 --- a/strings/base_windows.h +++ b/strings/base_windows.h @@ -148,6 +148,10 @@ namespace winrt::impl hresult code = ptr->QueryInterface(guid_of(), &result); if (code < 0) { + // Capture an error context immediately before the failfast call. The failfast gives the best + // output when there is a "stowed" exception. Capturing the error context will stow it enough + // for the failfast to find it and use it when creating an exception context record. + WINRT_IMPL_RoCaptureErrorContext(code); WINRT_IMPL_RoFailFastWithErrorContext(code); } return wrap_as_result(result); From 83c846b2806fc18e1f38396a0ad73cca7d654214 Mon Sep 17 00:00:00 2001 From: David Machaj <46852402+dmachaj@users.noreply.github.com.> Date: Mon, 28 Oct 2024 15:48:09 -0700 Subject: [PATCH 3/8] Change from failfast to throwing approach. Code gen size is not a problem --- cppwinrt/code_writers.h | 12 ++++++-- strings/base_error.h | 22 ++++++++++++++ strings/base_extern.h | 2 ++ strings/base_implements.h | 6 ---- strings/base_meta.h | 2 +- strings/base_windows.h | 27 ----------------- test/test/missing_required_interfaces.cpp | 37 ++--------------------- 7 files changed, 37 insertions(+), 71 deletions(-) diff --git a/cppwinrt/code_writers.h b/cppwinrt/code_writers.h index 3c77269cd..05b78bf62 100644 --- a/cppwinrt/code_writers.h +++ b/cppwinrt/code_writers.h @@ -1131,7 +1131,9 @@ namespace cppwinrt // we intentionally ignore errors when unregistering event handlers to be consistent with event_revoker format = R"( template auto consume_%::%(%) const noexcept {% - WINRT_IMPL_SHIM(%)->%(%);% + const auto castedResult = WINRT_IMPL_SHIM(%); + check_cast_result(castedResult); + castedResult->%(%);% } )"; } @@ -1139,7 +1141,9 @@ namespace cppwinrt { format = R"( template auto consume_%::%(%) const noexcept {% - WINRT_VERIFY_(0, WINRT_IMPL_SHIM(%)->%(%));% + const auto castedResult = WINRT_IMPL_SHIM(%); + check_cast_result(castedResult); + WINRT_VERIFY_(0, castedResult->%(%));% } )"; } @@ -1148,7 +1152,9 @@ namespace cppwinrt { format = R"( template auto consume_%::%(%) const {% - check_hresult(WINRT_IMPL_SHIM(%)->%(%));% + const auto castedResult = WINRT_IMPL_SHIM(%); + check_cast_result(castedResult); + check_hresult(castedResult->%(%));% } )"; } diff --git a/strings/base_error.h b/strings/base_error.h index 6e0aa103a..38b1ff344 100644 --- a/strings/base_error.h +++ b/strings/base_error.h @@ -529,6 +529,28 @@ WINRT_EXPORT namespace winrt return pointer; } + template + WINRT_IMPL_NOINLINE void check_cast_result(T const& from WINRT_IMPL_SOURCE_LOCATION_ARGS) + { + if (!from) + { + com_ptr restrictedError; + if (WINRT_IMPL_GetRestrictedErrorInfo(restrictedError.put_void()) == 0) + { + WINRT_IMPL_SetRestrictedErrorInfo(restrictedError.get()); + + int32_t code; + impl::bstr_handle description; + impl::bstr_handle restrictedDescription; + impl::bstr_handle capabilitySid; + if (restrictedError->GetErrorDetails(description.put(), &code, restrictedDescription.put(), capabilitySid.put()) == 0) + { + check_hresult(code WINRT_IMPL_SOURCE_LOCATION_FORWARD); + } + } + } + } + [[noreturn]] inline void terminate() noexcept { WINRT_IMPL_RoFailFastWithErrorContext(to_hresult()); diff --git a/strings/base_extern.h b/strings/base_extern.h index 2412f9f2c..c0e83e75a 100644 --- a/strings/base_extern.h +++ b/strings/base_extern.h @@ -32,6 +32,8 @@ extern "C" int32_t __stdcall WINRT_IMPL_RoCaptureErrorContext(int32_t error) noexcept WINRT_IMPL_LINK(RoCaptureErrorContext, 4); void __stdcall WINRT_IMPL_RoFailFastWithErrorContext(int32_t) noexcept WINRT_IMPL_LINK(RoFailFastWithErrorContext, 4); int32_t __stdcall WINRT_IMPL_RoTransformError(int32_t, int32_t, void*) noexcept WINRT_IMPL_LINK(RoTransformError, 12); + int32_t __stdcall WINRT_IMPL_GetRestrictedErrorInfo(void**) noexcept WINRT_IMPL_LINK(GetRestrictedErrorInfo, 4); + int32_t __stdcall WINRT_IMPL_SetRestrictedErrorInfo(void*) noexcept WINRT_IMPL_LINK(SetRestrictedErrorInfo, 4); void* __stdcall WINRT_IMPL_LoadLibraryExW(wchar_t const* name, void* unused, uint32_t flags) noexcept WINRT_IMPL_LINK(LoadLibraryExW, 12); int32_t __stdcall WINRT_IMPL_FreeLibrary(void* library) noexcept WINRT_IMPL_LINK(FreeLibrary, 4); diff --git a/strings/base_implements.h b/strings/base_implements.h index aba869eff..d0bb09012 100644 --- a/strings/base_implements.h +++ b/strings/base_implements.h @@ -788,12 +788,6 @@ namespace winrt::impl return m_inner.try_as(); } - template - auto as_or_failfast() const noexcept - { - return m_inner.as_or_failfast(); - } - explicit operator bool() const noexcept { return m_inner.operator bool(); diff --git a/strings/base_meta.h b/strings/base_meta.h index de9493289..2c1796e9c 100644 --- a/strings/base_meta.h +++ b/strings/base_meta.h @@ -159,7 +159,7 @@ namespace winrt::impl { operator I() const noexcept { - return static_cast(this)->template as_or_failfast(); + return static_cast(this)->template try_as(); } }; diff --git a/strings/base_windows.h b/strings/base_windows.h index 9ef2f1907..f5230e1c8 100644 --- a/strings/base_windows.h +++ b/strings/base_windows.h @@ -127,32 +127,11 @@ namespace winrt::impl return nullptr; } - void* result{}; - ptr->QueryInterface(guid_of(), &result); - return wrap_as_result(result); - } - - template , int> = 0> - com_ref as_or_failfast(From* ptr) noexcept - { -#ifdef WINRT_DIAGNOSTICS - get_diagnostics_info().add_query(); -#endif - - if (!ptr) - { - return nullptr; - } - void* result{}; hresult code = ptr->QueryInterface(guid_of(), &result); if (code < 0) { - // Capture an error context immediately before the failfast call. The failfast gives the best - // output when there is a "stowed" exception. Capturing the error context will stow it enough - // for the failfast to find it and use it when creating an exception context record. WINRT_IMPL_RoCaptureErrorContext(code); - WINRT_IMPL_RoFailFastWithErrorContext(code); } return wrap_as_result(result); } @@ -230,12 +209,6 @@ WINRT_EXPORT namespace winrt::Windows::Foundation return impl::try_as(m_ptr); } - template - auto as_or_failfast() const noexcept - { - return impl::as_or_failfast(m_ptr); - } - template void as(To& to) const { diff --git a/test/test/missing_required_interfaces.cpp b/test/test/missing_required_interfaces.cpp index 0649bc86b..8bc557944 100644 --- a/test/test/missing_required_interfaces.cpp +++ b/test/test/missing_required_interfaces.cpp @@ -13,44 +13,13 @@ namespace }; } -bool g_failfastCalled = false; - -void __stdcall WINRT_IMPL_RoFailFastWithErrorContext(int32_t) noexcept -{ - g_failfastCalled = true; -} - -void DoTheUncatcheable(winrt::test_component::LiesAboutInheritance& lies) noexcept -{ - lies.ToString(); -} - -bool CatchTheUncatcheable(winrt::test_component::LiesAboutInheritance& lies) noexcept -{ - bool caughtCrash = false; - __try - { - // We verify that our replacement version of WINRT_IMPL_RoFailFastWithErrorContext is - // called. In a real program that would exit the process. But this is a test so it - // continues execution and hits a nullptr dereference instead. Eat that known/expected - // error and allow execution to continue onto other test cases. - DoTheUncatcheable(lies); - } - __except (EXCEPTION_EXECUTE_HANDLER) - { - caughtCrash = true; - } - return caughtCrash; -} - TEST_CASE("missing_required_interfaces") { auto lies = winrt::make_self().as(); REQUIRE(lies); REQUIRE_NOTHROW(lies.StubMethod()); - REQUIRE(!g_failfastCalled); - const bool caughtCrash = CatchTheUncatcheable(lies); - REQUIRE(caughtCrash); - REQUIRE(g_failfastCalled); + // The IStringable::ToString method does not exist on this type. In previous versions of cppwinrt + // this line would crash with a nullptr deference. It now throws an exception. + REQUIRE_THROWS_AS(lies.ToString(), winrt::hresult_error); } \ No newline at end of file From 4a1dd037f1247ea54ba5d60c777bb1688f2fa380 Mon Sep 17 00:00:00 2001 From: David Machaj <46852402+dmachaj@users.noreply.github.com.> Date: Mon, 28 Oct 2024 17:23:48 -0700 Subject: [PATCH 4/8] The tests discovered a real bug. The WINRT_IMPL_SHIM macro only keeps the temporary alive until the semicolon. Remove that macro from where I need to call the check method to fix this crash --- cppwinrt/code_writers.h | 22 +++++++++++++--------- strings/base_error.h | 2 +- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/cppwinrt/code_writers.h b/cppwinrt/code_writers.h index 05b78bf62..df507a5b3 100644 --- a/cppwinrt/code_writers.h +++ b/cppwinrt/code_writers.h @@ -1131,9 +1131,10 @@ namespace cppwinrt // we intentionally ignore errors when unregistering event handlers to be consistent with event_revoker format = R"( template auto consume_%::%(%) const noexcept {% - const auto castedResult = WINRT_IMPL_SHIM(%); - check_cast_result(castedResult); - castedResult->%(%);% + const auto castedResult = static_cast<% const&>(static_cast(*this)); + const auto abiType = *(abi_t<%>**)&castedResult; + check_cast_result(abiType); + abiType->%(%);% } )"; } @@ -1141,9 +1142,10 @@ namespace cppwinrt { format = R"( template auto consume_%::%(%) const noexcept {% - const auto castedResult = WINRT_IMPL_SHIM(%); - check_cast_result(castedResult); - WINRT_VERIFY_(0, castedResult->%(%));% + const auto castedResult = static_cast<% const&>(static_cast(*this)); + const auto abiType = *(abi_t<%>**)&castedResult; + check_cast_result(abiType); + WINRT_VERIFY_(0, abiType->%(%));% } )"; } @@ -1152,9 +1154,10 @@ namespace cppwinrt { format = R"( template auto consume_%::%(%) const {% - const auto castedResult = WINRT_IMPL_SHIM(%); - check_cast_result(castedResult); - check_hresult(castedResult->%(%));% + const auto castedResult = static_cast<% const&>(static_cast(*this)); + const auto abiType = *(abi_t<%>**)&castedResult; + check_cast_result(abiType); + check_hresult(abiType->%(%));% } )"; } @@ -1167,6 +1170,7 @@ namespace cppwinrt bind(signature), bind(signature, false), type, + type, get_abi_name(method), bind(signature), bind(signature)); diff --git a/strings/base_error.h b/strings/base_error.h index 38b1ff344..a3fe77208 100644 --- a/strings/base_error.h +++ b/strings/base_error.h @@ -530,7 +530,7 @@ WINRT_EXPORT namespace winrt } template - WINRT_IMPL_NOINLINE void check_cast_result(T const& from WINRT_IMPL_SOURCE_LOCATION_ARGS) + WINRT_IMPL_NOINLINE void check_cast_result(T* from WINRT_IMPL_SOURCE_LOCATION_ARGS) { if (!from) { From 8b91053e4f97d65f6253224b20d99745217dd8a0 Mon Sep 17 00:00:00 2001 From: David Machaj <46852402+dmachaj@users.noreply.github.com.> Date: Tue, 29 Oct 2024 14:01:52 -0700 Subject: [PATCH 5/8] Defragment scratch.vcxproj and get it to use cppwinrt props files as if it were a nuget package --- cppwinrt.sln | 3 + scratch/scratch.vcxproj | 255 ++++------------------------------------ 2 files changed, 26 insertions(+), 232 deletions(-) diff --git a/cppwinrt.sln b/cppwinrt.sln index 43e96ffab..e2ac95942 100644 --- a/cppwinrt.sln +++ b/cppwinrt.sln @@ -88,6 +88,9 @@ EndProject Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "fast_fwd", "fast_fwd\fast_fwd.vcxproj", "{A63B3AD1-AB7B-461E-9FFF-2447F5BCD459}" EndProject Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "scratch", "scratch\scratch.vcxproj", "{E893622C-47DE-4F83-B422-0A26711590A4}" + ProjectSection(ProjectDependencies) = postProject + {D613FB39-5035-4043-91E2-BAB323908AF4} = {D613FB39-5035-4043-91E2-BAB323908AF4} + EndProjectSection EndProject Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "test_module_lock_none", "test\test_module_lock_none\test_module_lock_none.vcxproj", "{D48A96C2-8512-4CC3-B6E4-7CFF07ED8ED3}" ProjectSection(ProjectDependencies) = postProject diff --git a/scratch/scratch.vcxproj b/scratch/scratch.vcxproj index 4a26617b4..45122c13d 100644 --- a/scratch/scratch.vcxproj +++ b/scratch/scratch.vcxproj @@ -1,5 +1,6 @@ + Debug @@ -35,277 +36,67 @@ + true + true + true + true 16.0 {E893622C-47DE-4F83-B422-0A26711590A4} scratch scratch - - - - Application - true - - - Application - true - - Application - true + + ..\_build\$(Platform)\$(Configuration) + false - - Application - false - true - - - Application - false - true - - - Application - false - true - - - Application + + true - - Application + false true - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - MaxSpeed - true - true - $(OutputPath);Generated Files;..\..\..\library - NOMINMAX;_MBCS;%(PreprocessorDefinitions) - %(AdditionalOptions) - MultiThreaded - - - Console - true - true - - - - - - - - - - - - - Disabled - $(OutputPath);Generated Files;..\..\..\library - _MBCS;%(PreprocessorDefinitions) - %(AdditionalOptions) - MultiThreadedDebug - - - Console - - - - - - - - - - - + - Disabled - $(OutputPath);Generated Files;..\..\..\library - _MBCS;%(PreprocessorDefinitions) - %(AdditionalOptions) - MultiThreadedDebug + $(OutputPath);Generated Files; Console - - - - - - - - - - - Disabled - $(OutputPath);Generated Files;..\..\..\library - _MBCS;%(PreprocessorDefinitions) - %(AdditionalOptions) - MultiThreadedDebug - - - Console - - - - - - - - - - - - - Disabled - $(OutputPath);Generated Files;..\..\..\library - _MBCS;%(PreprocessorDefinitions) - %(AdditionalOptions) - MultiThreadedDebug - - - Console - - - - - - - - - - - + MaxSpeed true true - $(OutputPath);Generated Files;..\..\..\library NOMINMAX;_MBCS;%(PreprocessorDefinitions) - %(AdditionalOptions) MultiThreaded - Console true true - - - - - - - - - + - MaxSpeed - true - true - $(OutputPath);Generated Files;..\..\..\library - NOMINMAX;_MBCS;%(PreprocessorDefinitions) - %(AdditionalOptions) - MultiThreaded - - - Console - true - true - - - - - - - - - - - - - MaxSpeed - true - true - $(OutputPath);Generated Files;..\..\..\library - NOMINMAX;_MBCS;%(PreprocessorDefinitions) - %(AdditionalOptions) - MultiThreaded + Disabled + _MBCS;%(PreprocessorDefinitions) + MultiThreadedDebug - - Console - true - true - - - - - - - - - - NotUsing - Use - Use - Use - Use - Use - Use - Use - Use + Use - Create - Create - Create - Create - Create - Create - Create - Create + Create + + + From 46f64de7ee5a69eeaa12dfc8a45ad7bff1fad492 Mon Sep 17 00:00:00 2001 From: David Machaj <46852402+dmachaj@users.noreply.github.com.> Date: Tue, 29 Oct 2024 14:24:15 -0700 Subject: [PATCH 6/8] Move check_cast_result to impl namespace. Throw an hresult_error directly taking ownership of error info instead of re-originating --- strings/base_error.h | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/strings/base_error.h b/strings/base_error.h index a3fe77208..8a3a3a088 100644 --- a/strings/base_error.h +++ b/strings/base_error.h @@ -529,6 +529,24 @@ WINRT_EXPORT namespace winrt return pointer; } + [[noreturn]] inline void terminate() noexcept + { + WINRT_IMPL_RoFailFastWithErrorContext(to_hresult()); + abort(); + } +} + +namespace winrt::impl +{ + inline hresult check_hresult_allow_bounds(hresult const result WINRT_IMPL_SOURCE_LOCATION_ARGS) + { + if (result != impl::error_out_of_bounds && result != impl::error_fail && result != impl::error_file_not_found) + { + check_hresult(result WINRT_IMPL_SOURCE_LOCATION_FORWARD); + } + return result; + } + template WINRT_IMPL_NOINLINE void check_cast_result(T* from WINRT_IMPL_SOURCE_LOCATION_ARGS) { @@ -545,29 +563,11 @@ WINRT_EXPORT namespace winrt impl::bstr_handle capabilitySid; if (restrictedError->GetErrorDetails(description.put(), &code, restrictedDescription.put(), capabilitySid.put()) == 0) { - check_hresult(code WINRT_IMPL_SOURCE_LOCATION_FORWARD); + throw hresult_error(code, take_ownership_from_abi WINRT_IMPL_SOURCE_LOCATION_FORWARD); } } } } - - [[noreturn]] inline void terminate() noexcept - { - WINRT_IMPL_RoFailFastWithErrorContext(to_hresult()); - abort(); - } -} - -namespace winrt::impl -{ - inline hresult check_hresult_allow_bounds(hresult const result WINRT_IMPL_SOURCE_LOCATION_ARGS) - { - if (result != impl::error_out_of_bounds && result != impl::error_fail && result != impl::error_file_not_found) - { - check_hresult(result WINRT_IMPL_SOURCE_LOCATION_FORWARD); - } - return result; - } } #undef WINRT_IMPL_RETURNADDRESS From 6a71ee185b74a36d429ef8d918a73062c8b3c9fc Mon Sep 17 00:00:00 2001 From: David Machaj <46852402+dmachaj@users.noreply.github.com.> Date: Tue, 29 Oct 2024 15:33:40 -0700 Subject: [PATCH 7/8] Use a reference for the cast result to avoid a spurious addref/release --- cppwinrt/code_writers.h | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/cppwinrt/code_writers.h b/cppwinrt/code_writers.h index 9e329f551..78538af98 100644 --- a/cppwinrt/code_writers.h +++ b/cppwinrt/code_writers.h @@ -1129,9 +1129,13 @@ namespace cppwinrt if (is_remove_overload(method)) { // we intentionally ignore errors when unregistering event handlers to be consistent with event_revoker + // + // The `noexcept` versions will crash if check_cast_result throws but that is no different than previous + // behavior where it would not check the cast result and nullptr crash. At least the exception will terminate + // immediately while preserving the error code and local variables. format = R"( template auto consume_%::%(%) const noexcept {% - const auto castedResult = static_cast<% const&>(static_cast(*this)); + const auto& castedResult = static_cast<% const&>(static_cast(*this)); const auto abiType = *(abi_t<%>**)&castedResult; check_cast_result(abiType); abiType->%(%);% @@ -1142,7 +1146,7 @@ namespace cppwinrt { format = R"( template auto consume_%::%(%) const noexcept {% - const auto castedResult = static_cast<% const&>(static_cast(*this)); + const auto& castedResult = static_cast<% const&>(static_cast(*this)); const auto abiType = *(abi_t<%>**)&castedResult; check_cast_result(abiType); WINRT_VERIFY_(0, abiType->%(%));% @@ -1154,7 +1158,7 @@ namespace cppwinrt { format = R"( template auto consume_%::%(%) const {% - const auto castedResult = static_cast<% const&>(static_cast(*this)); + const auto& castedResult = static_cast<% const&>(static_cast(*this)); const auto abiType = *(abi_t<%>**)&castedResult; check_cast_result(abiType); check_hresult(abiType->%(%));% From 978e8f5a4f1f72c4b3fa94cf14032563d58bf5a1 Mon Sep 17 00:00:00 2001 From: David Machaj <46852402+dmachaj@users.noreply.github.com.> Date: Tue, 29 Oct 2024 15:54:27 -0700 Subject: [PATCH 8/8] East const, add newline to end of file --- cppwinrt/code_writers.h | 12 ++++++------ test/test/missing_required_interfaces.cpp | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/cppwinrt/code_writers.h b/cppwinrt/code_writers.h index 78538af98..0e516b528 100644 --- a/cppwinrt/code_writers.h +++ b/cppwinrt/code_writers.h @@ -1135,8 +1135,8 @@ namespace cppwinrt // immediately while preserving the error code and local variables. format = R"( template auto consume_%::%(%) const noexcept {% - const auto& castedResult = static_cast<% const&>(static_cast(*this)); - const auto abiType = *(abi_t<%>**)&castedResult; + auto const& castedResult = static_cast<% const&>(static_cast(*this)); + auto const abiType = *(abi_t<%>**)&castedResult; check_cast_result(abiType); abiType->%(%);% } @@ -1146,8 +1146,8 @@ namespace cppwinrt { format = R"( template auto consume_%::%(%) const noexcept {% - const auto& castedResult = static_cast<% const&>(static_cast(*this)); - const auto abiType = *(abi_t<%>**)&castedResult; + auto const& castedResult = static_cast<% const&>(static_cast(*this)); + auto const abiType = *(abi_t<%>**)&castedResult; check_cast_result(abiType); WINRT_VERIFY_(0, abiType->%(%));% } @@ -1158,8 +1158,8 @@ namespace cppwinrt { format = R"( template auto consume_%::%(%) const {% - const auto& castedResult = static_cast<% const&>(static_cast(*this)); - const auto abiType = *(abi_t<%>**)&castedResult; + auto const& castedResult = static_cast<% const&>(static_cast(*this)); + auto const abiType = *(abi_t<%>**)&castedResult; check_cast_result(abiType); check_hresult(abiType->%(%));% } diff --git a/test/test/missing_required_interfaces.cpp b/test/test/missing_required_interfaces.cpp index 8bc557944..237e4ff41 100644 --- a/test/test/missing_required_interfaces.cpp +++ b/test/test/missing_required_interfaces.cpp @@ -22,4 +22,4 @@ TEST_CASE("missing_required_interfaces") // The IStringable::ToString method does not exist on this type. In previous versions of cppwinrt // this line would crash with a nullptr deference. It now throws an exception. REQUIRE_THROWS_AS(lies.ToString(), winrt::hresult_error); -} \ No newline at end of file +}