From ba405c6be59bc77905602d25d7dd4fb9685e6e04 Mon Sep 17 00:00:00 2001 From: Crystal Jin Date: Sat, 8 May 2021 13:16:53 -0700 Subject: [PATCH] Back out "exception_wrapper thrown variant via abi/runtime" Summary: D26331579 (https://github.com/facebook/folly/commit/aa7f74aaf68ab0a7eb834f49a864c277dbe5fe09) was the offending diff that caused an insta crash on Messenger Android and Instagram Android. Backing out for now. Reviewed By: JunqiWang Differential Revision: D28308563 fbshipit-source-id: 87a547d9ba7cb3b33a4ddee9e273943f8379d990 --- folly/ExceptionWrapper-inl.h | 86 +++++++++++++++++++++++------ folly/ExceptionWrapper.cpp | 41 +++++++++++--- folly/ExceptionWrapper.h | 52 ++++++++++++----- folly/test/ExceptionWrapperTest.cpp | 44 +++++++++------ 4 files changed, 166 insertions(+), 57 deletions(-) diff --git a/folly/ExceptionWrapper-inl.h b/folly/ExceptionWrapper-inl.h index c82bd261ee2..d9b392982a2 100644 --- a/folly/ExceptionWrapper-inl.h +++ b/folly/ExceptionWrapper-inl.h @@ -127,6 +127,56 @@ inline std::exception const* exception_wrapper::as_exception_or_null_( return nullptr; } +static_assert( + !kMicrosoftAbiVer || (kMicrosoftAbiVer >= 1900 && kMicrosoftAbiVer <= 2000), + "exception_wrapper is untested and possibly broken on your version of " + "MSVC"); + +inline std::uintptr_t exception_wrapper::ExceptionPtr::as_int_( + std::exception_ptr const& ptr, std::exception const& e) noexcept { + if (!kMicrosoftAbiVer) { + return reinterpret_cast(&e); + } else { + // On Windows, as of MSVC2017, all thrown exceptions are copied to the stack + // first. Thus, we cannot depend on exception references associated with an + // exception_ptr to be live for the duration of the exception_ptr. We need + // to directly access the heap allocated memory inside the exception_ptr. + // + // std::exception_ptr is an opaque reinterpret_cast of + // std::shared_ptr<__ExceptionPtr> + // __ExceptionPtr is a non-virtual class with two members, a union and a + // bool. The union contains the now-undocumented EHExceptionRecord, which + // contains a struct which contains a void* which points to the heap + // allocated exception. + // We derive the offset to pExceptionObject via manual means. + FOLLY_PACK_PUSH + struct Win32ExceptionPtr { + char offset[8 + 4 * sizeof(void*)]; + void* exceptionObject; + } FOLLY_PACK_ATTR; + FOLLY_PACK_POP + + auto* win32ExceptionPtr = + reinterpret_cast const*>(&ptr) + ->get(); + return reinterpret_cast(win32ExceptionPtr->exceptionObject); + } +} +inline std::uintptr_t exception_wrapper::ExceptionPtr::as_int_( + std::exception_ptr const&, AnyException e) noexcept { + return reinterpret_cast(e.typeinfo_) + 1; +} +inline bool exception_wrapper::ExceptionPtr::has_exception_() const { + return 0 == exception_or_type_ % 2; +} +inline std::exception const* exception_wrapper::ExceptionPtr::as_exception_() + const { + return reinterpret_cast(exception_or_type_); +} +inline std::type_info const* exception_wrapper::ExceptionPtr::as_type_() const { + return reinterpret_cast(exception_or_type_ - 1); +} + inline void exception_wrapper::ExceptionPtr::copy_( exception_wrapper const* from, exception_wrapper* to) { ::new (static_cast(&to->eptr_)) ExceptionPtr(from->eptr_); @@ -146,11 +196,14 @@ inline void exception_wrapper::ExceptionPtr::delete_(exception_wrapper* that) { } inline std::type_info const* exception_wrapper::ExceptionPtr::type_( exception_wrapper const* that) { - return exception_ptr_get_type(that->eptr_.ptr_); + if (auto e = get_exception_(that)) { + return &typeid(*e); + } + return that->eptr_.as_type_(); } inline std::exception const* exception_wrapper::ExceptionPtr::get_exception_( exception_wrapper const* that) { - return exception_ptr_get_object(that->eptr_.ptr_); + return that->eptr_.has_exception_() ? that->eptr_.as_exception_() : nullptr; } inline exception_wrapper exception_wrapper::ExceptionPtr::get_exception_ptr_( exception_wrapper const* that) { @@ -195,8 +248,8 @@ inline exception_wrapper exception_wrapper::InPlace::get_exception_ptr_( exception_wrapper const* that) { try { throw_(that); - } catch (...) { - return exception_wrapper{std::current_exception()}; + } catch (Ex const& ex) { + return exception_wrapper{std::current_exception(), ex}; } } @@ -215,8 +268,8 @@ inline exception_wrapper exception_wrapper::SharedPtr::Impl::get_exception_ptr_() const noexcept { try { throw_(); - } catch (...) { - return exception_wrapper{std::current_exception()}; + } catch (Ex& ex) { + return exception_wrapper{std::current_exception(), ex}; } } inline void exception_wrapper::SharedPtr::copy_( @@ -254,7 +307,7 @@ inline exception_wrapper exception_wrapper::SharedPtr::get_exception_ptr_( template inline exception_wrapper::exception_wrapper( ThrownTag, in_place_type_t, As&&... as) - : eptr_{std::make_exception_ptr(Ex(std::forward(as)...))}, + : eptr_{std::make_exception_ptr(Ex(std::forward(as)...)), reinterpret_cast(std::addressof(typeid(Ex))) + 1u}, vptr_(&ExceptionPtr::ops_) {} template @@ -302,17 +355,9 @@ inline exception_wrapper::~exception_wrapper() { template inline exception_wrapper::exception_wrapper( - std::exception_ptr const& ptr, Ex& ex) noexcept - : exception_wrapper{folly::copy(ptr), ex} {} - -template -inline exception_wrapper::exception_wrapper( - std::exception_ptr&& ptr, Ex& ex) noexcept - : eptr_{std::move(ptr)}, vptr_(&ExceptionPtr::ops_) { + std::exception_ptr ptr, Ex& ex) noexcept + : eptr_{ptr, ExceptionPtr::as_int_(ptr, ex)}, vptr_(&ExceptionPtr::ops_) { assert(eptr_.ptr_); - (void)ex; - assert(exception_ptr_get_object(eptr_.ptr_)); - assert(exception_ptr_get_object(eptr_.ptr_) == &ex || kIsWindows); } namespace exception_wrapper_detail { @@ -412,6 +457,9 @@ inline std::exception_ptr exception_wrapper::to_exception_ptr() const noexcept { inline std::type_info const& exception_wrapper::none() noexcept { return typeid(void); } +inline std::type_info const& exception_wrapper::unknown() noexcept { + return typeid(Unknown); +} inline std::type_info const& exception_wrapper::type() const noexcept { return *vptr_->type_(this); @@ -426,7 +474,9 @@ inline folly::fbstring exception_wrapper::what() const { inline folly::fbstring exception_wrapper::class_name() const { auto& ti = type(); - return ti == none() ? "" : folly::demangle(ti); + return ti == none() ? "" + : ti == unknown() ? "" + : folly::demangle(ti); } template diff --git a/folly/ExceptionWrapper.cpp b/folly/ExceptionWrapper.cpp index a827672da1a..2263e16195f 100644 --- a/folly/ExceptionWrapper.cpp +++ b/folly/ExceptionWrapper.cpp @@ -37,6 +37,18 @@ exception_wrapper::VTable const exception_wrapper::ExceptionPtr::ops_{ exception_wrapper::VTable const exception_wrapper::SharedPtr::ops_{ copy_, move_, delete_, throw_, type_, get_exception_, get_exception_ptr_}; +namespace { +std::exception const* get_std_exception_(std::exception_ptr eptr) noexcept { + try { + std::rethrow_exception(eptr); + } catch (const std::exception& ex) { + return &ex; + } catch (...) { + return nullptr; + } +} +} // namespace + exception_wrapper exception_wrapper::from_exception_ptr( std::exception_ptr const& ptr) noexcept { return from_exception_ptr(folly::copy(ptr)); @@ -44,16 +56,31 @@ exception_wrapper exception_wrapper::from_exception_ptr( exception_wrapper exception_wrapper::from_exception_ptr( std::exception_ptr&& ptr) noexcept { - return !ptr ? exception_wrapper() : exception_wrapper(std::move(ptr)); + if (!ptr) { + return exception_wrapper(); + } + try { + std::rethrow_exception(std::move(ptr)); + } catch (std::exception& e) { + return exception_wrapper(std::current_exception(), e); + } catch (...) { + return exception_wrapper(std::current_exception()); + } } -exception_wrapper::exception_wrapper(std::exception_ptr const& ptr) noexcept - : exception_wrapper{folly::copy(ptr)} {} - -exception_wrapper::exception_wrapper(std::exception_ptr&& ptr) noexcept { +exception_wrapper::exception_wrapper(std::exception_ptr ptr) noexcept + : exception_wrapper{} { if (ptr) { - ::new (&eptr_) ExceptionPtr{std::move(ptr)}; - vptr_ = &ExceptionPtr::ops_; + if (auto e = get_std_exception_(ptr)) { + LOG(DFATAL) + << "Performance error: Please construct exception_wrapper with a " + "reference to the std::exception along with the " + "std::exception_ptr."; + *this = exception_wrapper{std::move(ptr), *e}; + } else { + Unknown uk; + *this = exception_wrapper{ptr, uk}; + } } } diff --git a/folly/ExceptionWrapper.h b/folly/ExceptionWrapper.h index 6fc8c085a14..d9f5e94a268 100644 --- a/folly/ExceptionWrapper.h +++ b/folly/ExceptionWrapper.h @@ -31,7 +31,6 @@ #include #include -#include #include #include #include @@ -39,7 +38,6 @@ #include #include #include -#include #ifdef __GNUC__ #pragma GCC diagnostic push @@ -179,7 +177,10 @@ class exception_wrapper final { // 1. An small object stored in-situ. // 2. A larger object stored on the heap and referenced with a // std::shared_ptr. - // 3. A std::exception_ptr. + // 3. A std::exception_ptr, together with either: + // a. A pointer to the referenced std::exception object, or + // b. A pointer to a std::type_info object for the referenced exception, + // or for an unspecified type if the type is unknown. // This is accomplished with the help of a union and a pointer to a hand- // rolled virtual table. This virtual table contains pointers to functions // that know which field of the union is active and do the proper action. @@ -211,6 +212,8 @@ class exception_wrapper final { using IsCatchAll = std::is_same>, AnyException>; + struct Unknown {}; + // Sadly, with the gcc-4.9 platform, std::logic_error and std::runtime_error // do not fit here. They also don't have noexcept copy-ctors, so the internal // storage wouldn't be used anyway. For the gcc-5 platform, both logic_error @@ -252,7 +255,19 @@ class exception_wrapper final { struct ExceptionPtr { std::exception_ptr ptr_; - + std::uintptr_t exception_or_type_; // odd for type_info + static_assert( + 1 < alignof(std::exception) && 1 < alignof(std::type_info), + "Surprise! std::exception and std::type_info don't have alignment " + "greater than one. as_int_ below will not work!"); + + static std::uintptr_t as_int_( + std::exception_ptr const& ptr, std::exception const& e) noexcept; + static std::uintptr_t as_int_( + std::exception_ptr const& ptr, AnyException e) noexcept; + bool has_exception_() const; + std::exception const* as_exception_() const; + std::type_info const* as_type_() const; static void copy_(exception_wrapper const* from, exception_wrapper* to); static void move_(exception_wrapper* from, exception_wrapper* to); static void delete_(exception_wrapper* that); @@ -393,19 +408,19 @@ class exception_wrapper final { ~exception_wrapper(); + //! \pre `ptr` is empty, or it holds a reference to an exception that is not + //! derived from `std::exception`. //! \post `!ptr || bool(*this)` - //! \post `hasThrownException() == bool(ptr)` - explicit exception_wrapper(std::exception_ptr const& ptr) noexcept; - explicit exception_wrapper(std::exception_ptr&& ptr) noexcept; + //! \post `hasThrownException() == true` + //! \post `type() == unknown()` + explicit exception_wrapper(std::exception_ptr ptr) noexcept; //! \pre `ptr` holds a reference to `ex`. //! \post `hasThrownException() == true` //! \post `bool(*this)` //! \post `type() == typeid(ex)` template - exception_wrapper(std::exception_ptr const& ptr, Ex& ex) noexcept; - template - exception_wrapper(std::exception_ptr&& ptr, Ex& ex) noexcept; + exception_wrapper(std::exception_ptr ptr, Ex& ex) noexcept; //! \pre `typeid(ex) == typeid(typename decay::type)` //! \post `bool(*this)` @@ -491,16 +506,23 @@ class exception_wrapper final { //! \return the `typeid` of an unspecified type used by //! `exception_wrapper::type()` to denote an empty `exception_wrapper`. static std::type_info const& none() noexcept; + //! \return the `typeid` of an unspecified type used by + //! `exception_wrapper::type()` to denote an `exception_wrapper` that + //! holds an exception of unknown type. + static std::type_info const& unknown() noexcept; //! Returns the `typeid` of the wrapped exception object. If there is no - //! wrapped exception object, returns `exception_wrapper::none()`. + //! wrapped exception object, returns `exception_wrapper::none()`. If + //! this instance wraps an exception of unknown type not derived from + //! `std::exception`, returns `exception_wrapper::unknown()`. std::type_info const& type() const noexcept; //! \return If `get_exception() != nullptr`, `class_name() + ": " + //! get_exception()->what()`; otherwise, `class_name()`. folly::fbstring what() const; - //! \return If `!*this`, the empty string; otherwise, + //! \return If `!*this`, the empty string; otherwise, if + //! `type() == unknown()`, the string `""`; otherwise, //! the result of `type().name()` after demangling. folly::fbstring class_name() const; @@ -636,8 +658,8 @@ template inline exception_wrapper try_and_catch_(F&& f) { try { return try_and_catch_(std::forward(f)); - } catch (Ex&) { - return exception_wrapper(std::current_exception()); + } catch (Ex& ex) { + return exception_wrapper(std::current_exception(), ex); } } } // namespace detail @@ -687,6 +709,8 @@ exception_wrapper try_and_catch(F&& fn) noexcept { try { static_cast(fn)(); return exception_wrapper{}; + } catch (std::exception const& ex) { + return exception_wrapper{std::current_exception(), ex}; } catch (...) { return exception_wrapper{std::current_exception()}; } diff --git a/folly/test/ExceptionWrapperTest.cpp b/folly/test/ExceptionWrapperTest.cpp index cf303ecb8d5..44310259f04 100644 --- a/folly/test/ExceptionWrapperTest.cpp +++ b/folly/test/ExceptionWrapperTest.cpp @@ -239,13 +239,13 @@ TEST(ExceptionWrapper, get_or_make_exception_ptr_test) { TEST(ExceptionWrapper, from_exception_ptr_empty) { auto ep = std::exception_ptr(); - auto ew = exception_wrapper{ep}; + auto ew = exception_wrapper::from_exception_ptr(ep); EXPECT_FALSE(bool(ew)); } TEST(ExceptionWrapper, from_exception_ptr_exn) { auto ep = std::make_exception_ptr(std::runtime_error("foo")); - auto ew = exception_wrapper{ep}; + auto ew = exception_wrapper::from_exception_ptr(ep); EXPECT_TRUE(bool(ew)); EXPECT_EQ(ep, folly::as_const(ew).to_exception_ptr()); EXPECT_EQ(ep, ew.to_exception_ptr()); @@ -254,7 +254,7 @@ TEST(ExceptionWrapper, from_exception_ptr_exn) { TEST(ExceptionWrapper, from_exception_ptr_any) { auto ep = std::make_exception_ptr(12); - auto ew = exception_wrapper{ep}; + auto ew = exception_wrapper::from_exception_ptr(ep); EXPECT_TRUE(bool(ew)); EXPECT_EQ(ep, folly::as_const(ew).to_exception_ptr()); EXPECT_EQ(ep, ew.to_exception_ptr()); @@ -420,7 +420,7 @@ TEST(ExceptionWrapper, with_non_std_exception_test) { TEST(ExceptionWrapper, with_exception_ptr_any_nil_test) { auto ep = std::make_exception_ptr(12); - auto ew = exception_wrapper(ep); + auto ew = exception_wrapper(ep); // concrete type is erased EXPECT_TRUE(bool(ew)); EXPECT_EQ(nullptr, ew.get_exception()); EXPECT_EQ(nullptr, ew.get_exception()); @@ -428,8 +428,9 @@ TEST(ExceptionWrapper, with_exception_ptr_any_nil_test) { EXPECT_EQ(12, *ew.get_exception()); EXPECT_EQ(ep, folly::as_const(ew).to_exception_ptr()); EXPECT_EQ(ep, ew.to_exception_ptr()); - EXPECT_EQ("int", ew.class_name()); - EXPECT_EQ("int", ew.what()); + EXPECT_EQ("", ew.class_name()); // because concrete type is + // erased + EXPECT_EQ("", ew.what()); EXPECT_FALSE(ew.is_compatible_with()); EXPECT_FALSE(ew.is_compatible_with()); EXPECT_TRUE(ew.is_compatible_with()); @@ -538,16 +539,23 @@ TEST(ExceptionWrapper, implicitConstruction) { } namespace { -struct BaseNonStdException { - virtual ~BaseNonStdException() {} +struct BaseException { + virtual ~BaseException() {} }; -struct DerivedNonStdException : BaseNonStdException {}; +struct DerivedException : BaseException {}; +exception_wrapper testNonStdException() { + try { + throw DerivedException{}; + } catch (const BaseException& e) { + return exception_wrapper{std::current_exception(), e}; + } +} } // namespace TEST(ExceptionWrapper, base_derived_non_std_exception_test) { - exception_wrapper ew{std::make_exception_ptr(DerivedNonStdException{})}; - EXPECT_TRUE(ew.type() == typeid(DerivedNonStdException)); - EXPECT_TRUE(ew.with_exception([](const DerivedNonStdException&) {})); + auto ew = testNonStdException(); + EXPECT_TRUE(ew.type() == typeid(DerivedException)); + EXPECT_TRUE(ew.with_exception([](const DerivedException&) {})); } namespace { @@ -834,26 +842,26 @@ TEST(ExceptionWrapper, handle_non_std_exception_big) { } TEST(ExceptionWrapper, handle_non_std_exception_rethrow_base_derived) { - exception_wrapper ew{std::make_exception_ptr(DerivedNonStdException{})}; + auto ew = testNonStdException(); bool handled = false; EXPECT_THROW( ew.handle( - [&](const DerivedNonStdException& e) { + [&](const DerivedException& e) { handled = true; throw e; }, - [](const BaseNonStdException&) { ADD_FAILURE(); }), - DerivedNonStdException); + [](const BaseException&) { ADD_FAILURE(); }), + DerivedException); EXPECT_TRUE(handled); handled = false; EXPECT_THROW( ew.handle( - [&](const DerivedNonStdException& e) { + [&](const DerivedException& e) { handled = true; throw e; }, [](...) { ADD_FAILURE(); }), - DerivedNonStdException); + DerivedException); EXPECT_TRUE(handled); }