From 3a7dfc850fab2ca45780654892542ce881dc85ee Mon Sep 17 00:00:00 2001 From: Xuehan Xu Date: Sun, 25 Feb 2024 12:06:18 +0800 Subject: [PATCH] crimson/common/errorator: disallow void-returning error handlers Fixes: https://tracker.ceph.com/issues/64556 Signed-off-by: Xuehan Xu (cherry picked from commit 513257cdd67ec126539f617256d73e4e8c65cf31) --- src/crimson/common/errorator.h | 87 ++++++++++++----------- src/crimson/common/interruptible_future.h | 30 +++++--- 2 files changed, 67 insertions(+), 50 deletions(-) diff --git a/src/crimson/common/errorator.h b/src/crimson/common/errorator.h index a9d743fcb84fe..2eeb9e49bbace 100644 --- a/src/crimson/common/errorator.h +++ b/src/crimson/common/errorator.h @@ -156,6 +156,8 @@ public: } }; +struct no_touch_error_marker {}; + // unthrowable_wrapper ensures compilation failure when somebody // would like to `throw make_error<...>)()` instead of returning. // returning allows for the compile-time verification of future's @@ -211,12 +213,13 @@ struct unthrowable_wrapper : error_t> { } assert_failure() = default; - void operator()(const unthrowable_wrapper&) { + no_touch_error_marker operator()(const unthrowable_wrapper&) { if (msg) { ceph_abort(msg); } else { ceph_abort(); } + return no_touch_error_marker{}; } }; @@ -320,44 +323,43 @@ public: void handle() { static_assert(std::is_invocable::value, "provided Error Visitor is not exhaustive"); - // In C++ throwing an exception isn't the sole way to signal - // error with it. This approach nicely fits cold, infrequent cases - // but when applied to a hot one, it will likely hurt performance. - // - // Alternative approach is to create `std::exception_ptr` on our - // own and place it in the future via `make_exception_future()`. - // When it comes to handling, the pointer can be interrogated for - // pointee's type with `__cxa_exception_type()` instead of costly - // re-throwing (via `std::rethrow_exception()`) and matching with - // `catch`. The limitation here is lack of support for hierarchies - // of exceptions. The code below checks for exact match only while - // `catch` would allow to match against a base class as well. - // However, this shouldn't be a big issue for `errorator` as Error - // Visitors are already checked for exhaustiveness at compile-time. - // - // NOTE: `__cxa_exception_type()` is an extension of the language. - // It should be available both in GCC and Clang but a fallback - // (based on `std::rethrow_exception()` and `catch`) can be made - // to handle other platforms if necessary. - if (type_info == ErrorT::error_t::get_exception_ptr_type_info()) { - // set `state::invalid` in internals of `seastar::future` to not - // call `report_failed_future()` during `operator=()`. - [[maybe_unused]] auto&& ep = std::move(result).get_exception(); - - using return_t = std::invoke_result_t; - if constexpr (std::is_assignable_v) { - result = std::invoke(std::forward(errfunc), - ErrorT::error_t::from_exception_ptr(std::move(ep))); - } else if constexpr (std::is_same_v) { - // void denotes explicit discarding - // execute for the sake a side effects. Typically this boils down - // to throwing an exception by the handler. - std::invoke(std::forward(errfunc), - ErrorT::error_t::from_exception_ptr(std::move(ep))); - } else { - result = FuturatorT::invoke( - std::forward(errfunc), - ErrorT::error_t::from_exception_ptr(std::move(ep))); + using return_t = std::invoke_result_t; + static_assert(!std::is_same_v, + "error handlers mustn't return void"); + if constexpr (std::is_same_v) { + return; + } else { + // In C++ throwing an exception isn't the sole way to signal + // error with it. This approach nicely fits cold, infrequent cases + // but when applied to a hot one, it will likely hurt performance. + // + // Alternative approach is to create `std::exception_ptr` on our + // own and place it in the future via `make_exception_future()`. + // When it comes to handling, the pointer can be interrogated for + // pointee's type with `__cxa_exception_type()` instead of costly + // re-throwing (via `std::rethrow_exception()`) and matching with + // `catch`. The limitation here is lack of support for hierarchies + // of exceptions. The code below checks for exact match only while + // `catch` would allow to match against a base class as well. + // However, this shouldn't be a big issue for `errorator` as Error + // Visitors are already checked for exhaustiveness at compile-time. + // + // NOTE: `__cxa_exception_type()` is an extension of the language. + // It should be available both in GCC and Clang but a fallback + // (based on `std::rethrow_exception()` and `catch`) can be made + // to handle other platforms if necessary. + if (type_info == ErrorT::error_t::get_exception_ptr_type_info()) { + // set `state::invalid` in internals of `seastar::future` to not + // call `report_failed_future()` during `operator=()`. + [[maybe_unused]] auto &&ep = std::move(result).get_exception(); + if constexpr (std::is_assignable_v) { + result = std::invoke(std::forward(errfunc), + ErrorT::error_t::from_exception_ptr(std::move(ep))); + } else { + result = FuturatorT::invoke( + std::forward(errfunc), + ErrorT::error_t::from_exception_ptr(std::move(ep))); + } } } } @@ -389,6 +391,7 @@ static constexpr auto composer(FuncHead&& head, FuncTail&&... tail) { std::is_invocable_v || (sizeof...(FuncTail) > 0), "composition is not exhaustive"); + return no_touch_error_marker{}; } }; } @@ -911,7 +914,7 @@ public: assert_all() = default; template ...> - void operator()(ErrorT&&) { + no_touch_error_marker operator()(ErrorT&&) { static_assert(contains_once_v>, "discarding disallowed ErrorT"); if (msg) { @@ -919,6 +922,7 @@ public: } else { ceph_abort(); } + return no_touch_error_marker{}; } }; @@ -1256,12 +1260,13 @@ namespace ct_error { assert_all() = default; template - void operator()(ErrorT&&) { + no_touch_error_marker operator()(ErrorT&&) { if (msg) { ceph_abort(msg); } else { ceph_abort(); } + return no_touch_error_marker{}; } }; diff --git a/src/crimson/common/interruptible_future.h b/src/crimson/common/interruptible_future.h index 27467f2396265..405d9c3c05d26 100644 --- a/src/crimson/common/interruptible_future.h +++ b/src/crimson/common/interruptible_future.h @@ -821,13 +821,17 @@ public: }, [func=std::move(errfunc), interrupt_condition=interrupt_cond.interrupt_cond] (auto&& err) mutable -> decltype(auto) { - constexpr bool return_void = std::is_void_v< + static_assert(!std::is_void_v< std::invoke_result_t>>; + std::decay_t>>); + constexpr bool is_assert = std::is_same_v< + std::decay_t>>, + ::crimson::no_touch_error_marker>; constexpr bool return_err = ::crimson::is_error_v< std::decay_t>>>; - if constexpr (return_err || return_void) { + if constexpr (return_err || is_assert) { return non_futurized_call_with_interruption( interrupt_condition, std::move(func), @@ -857,13 +861,17 @@ public: }, [func=std::move(errfunc), interrupt_condition=interrupt_cond.interrupt_cond] (auto&& err) mutable -> decltype(auto) { - constexpr bool return_void = std::is_void_v< + static_assert(!std::is_void_v< std::invoke_result_t>>; + std::decay_t>>); + constexpr bool is_assert = std::is_same_v< + std::decay_t>>, + ::crimson::no_touch_error_marker>; constexpr bool return_err = ::crimson::is_error_v< std::decay_t>>>; - if constexpr (return_err || return_void) { + if constexpr (return_err || is_assert) { return non_futurized_call_with_interruption( interrupt_condition, std::move(func), @@ -985,13 +993,17 @@ public: [errfunc=std::move(errfunc), interrupt_condition=interrupt_cond.interrupt_cond] (auto&& err) mutable -> decltype(auto) { - constexpr bool return_void = std::is_void_v< + static_assert(!std::is_void_v< std::invoke_result_t>>; + std::decay_t>>); + constexpr bool is_assert = std::is_same_v< + std::decay_t>>, + ::crimson::no_touch_error_marker>; constexpr bool return_err = ::crimson::is_error_v< std::decay_t>>>; - if constexpr (return_err || return_void) { + if constexpr (return_err || is_assert) { return non_futurized_call_with_interruption( interrupt_condition, std::move(errfunc), -- 2.39.5