From 73789eb0689071b0b02b00cb3daed9f224ff1a10 Mon Sep 17 00:00:00 2001 From: Abseil Team Date: Tue, 11 Oct 2022 10:27:17 -0700 Subject: Detects accidental multiple invocations of AnyInvocable::operator()&& by producing an error in debug mode, and clarifies that the behavior is undefined in the general case. PiperOrigin-RevId: 480392976 Change-Id: I2d4c6f213fa7c8747f125c9735272a8e47b9214b --- absl/functional/any_invocable.h | 3 ++ absl/functional/any_invocable_test.cc | 49 +++++++++++++++++++++++++------- absl/functional/internal/any_invocable.h | 24 ++++++++++++++-- 3 files changed, 64 insertions(+), 12 deletions(-) (limited to 'absl/functional') diff --git a/absl/functional/any_invocable.h b/absl/functional/any_invocable.h index 040418d4..3e783c87 100644 --- a/absl/functional/any_invocable.h +++ b/absl/functional/any_invocable.h @@ -148,6 +148,9 @@ ABSL_NAMESPACE_BEGIN // // rvalue-reference qualified. // std::move(continuation)(result_of_foo); // } +// +// Attempting to call `absl::AnyInvocable` multiple times in such a case +// results in undefined behavior. template class AnyInvocable : private internal_any_invocable::Impl { private: diff --git a/absl/functional/any_invocable_test.cc b/absl/functional/any_invocable_test.cc index dabaae9b..1ed85407 100644 --- a/absl/functional/any_invocable_test.cc +++ b/absl/functional/any_invocable_test.cc @@ -16,6 +16,7 @@ #include #include +#include #include #include @@ -1156,9 +1157,6 @@ TYPED_TEST_P(AnyInvTestMovable, ConversionConstructionUserDefinedType) { EXPECT_TRUE(static_cast(fun)); EXPECT_EQ(29, TypeParam::ToThisParam(fun)(7, 8, 9).value); - - EXPECT_TRUE(static_cast(fun)); - EXPECT_EQ(38, TypeParam::ToThisParam(fun)(10, 11, 12).value); } TYPED_TEST_P(AnyInvTestMovable, ConversionConstructionVoidCovariance) { @@ -1179,9 +1177,6 @@ TYPED_TEST_P(AnyInvTestMovable, ConversionAssignUserDefinedTypeEmptyLhs) { EXPECT_TRUE(static_cast(fun)); EXPECT_EQ(29, TypeParam::ToThisParam(fun)(7, 8, 9).value); - - EXPECT_TRUE(static_cast(fun)); - EXPECT_EQ(38, TypeParam::ToThisParam(fun)(10, 11, 12).value); } TYPED_TEST_P(AnyInvTestMovable, ConversionAssignUserDefinedTypeNonemptyLhs) { @@ -1193,9 +1188,6 @@ TYPED_TEST_P(AnyInvTestMovable, ConversionAssignUserDefinedTypeNonemptyLhs) { EXPECT_TRUE(static_cast(fun)); EXPECT_EQ(29, TypeParam::ToThisParam(fun)(7, 8, 9).value); - - EXPECT_TRUE(static_cast(fun)); - EXPECT_EQ(38, TypeParam::ToThisParam(fun)(10, 11, 12).value); } TYPED_TEST_P(AnyInvTestMovable, ConversionAssignVoidCovariance) { @@ -1414,6 +1406,41 @@ TYPED_TEST_P(AnyInvTestRvalue, ConversionAssignReferenceWrapper) { std::is_assignable>::value)); } +TYPED_TEST_P(AnyInvTestRvalue, NonConstCrashesOnSecondCall) { + using AnyInvType = typename TypeParam::AnyInvType; + using AddType = typename TypeParam::AddType; + + AnyInvType fun(absl::in_place_type, 5); + + EXPECT_TRUE(static_cast(fun)); + std::move(fun)(7, 8, 9); + + // Ensure we're still valid + EXPECT_TRUE(static_cast(fun)); // NOLINT(bugprone-use-after-move) + +#if !defined(NDEBUG) || ABSL_OPTION_HARDENED == 1 + EXPECT_DEATH_IF_SUPPORTED(std::move(fun)(7, 8, 9), ""); +#endif +} + +// Ensure that any qualifiers (in particular &&-qualifiers) do not affect +// when the destructor is actually run. +TYPED_TEST_P(AnyInvTestRvalue, QualifierIndependentObjectLifetime) { + using AnyInvType = typename TypeParam::AnyInvType; + + auto refs = std::make_shared(); + { + AnyInvType fun([refs](auto&&...) noexcept { return 0; }); + EXPECT_FALSE(refs.unique()); + + std::move(fun)(7, 8, 9); + + // Ensure destructor hasn't run even if rref-qualified + EXPECT_FALSE(refs.unique()); + } + EXPECT_TRUE(refs.unique()); +} + // NOTE: This test suite originally attempted to enumerate all possible // combinations of type properties but the build-time started getting too large. // Instead, it is now assumed that certain parameters are orthogonal and so @@ -1670,7 +1697,9 @@ INSTANTIATE_TYPED_TEST_SUITE_P(NonRvalueCallNothrow, AnyInvTestNonRvalue, REGISTER_TYPED_TEST_SUITE_P(AnyInvTestRvalue, ConversionConstructionReferenceWrapper, NonMoveableResultType, - ConversionAssignReferenceWrapper); + ConversionAssignReferenceWrapper, + NonConstCrashesOnSecondCall, + QualifierIndependentObjectLifetime); INSTANTIATE_TYPED_TEST_SUITE_P(RvalueCallMayThrow, AnyInvTestRvalue, TestParameterListRvalueQualifiersCallMayThrow); diff --git a/absl/functional/internal/any_invocable.h b/absl/functional/internal/any_invocable.h index 35b389d1..8fce4bf6 100644 --- a/absl/functional/internal/any_invocable.h +++ b/absl/functional/internal/any_invocable.h @@ -809,11 +809,31 @@ using CanAssignReferenceWrapper = TrueAlias< : Core(absl::in_place_type inv_quals>, \ std::forward(args)...) {} \ \ + InvokerType* ExtractInvoker() cv { \ + using QualifiedTestType = int cv ref; \ + auto* invoker = this->invoker_; \ + if (!std::is_const::value && \ + std::is_rvalue_reference::value) { \ + ABSL_HARDENING_ASSERT([this]() { \ + /* We checked that this isn't const above, so const_cast is safe */ \ + const_cast(this)->invoker_ = \ + [](TypeErasedState*, \ + ForwardedParameterType

...) noexcept(noex) -> ReturnType { \ + ABSL_HARDENING_ASSERT(false && "AnyInvocable use-after-move"); \ + std::terminate(); \ + }; \ + return this->HasValue(); \ + }()); \ + } \ + return invoker; \ + } \ + \ /*The actual invocation operation with the proper signature*/ \ ReturnType operator()(P... args) cv ref noexcept(noex) { \ assert(this->invoker_ != nullptr); \ - return this->invoker_(const_cast(&this->state_), \ - static_cast>(args)...); \ + return this->ExtractInvoker()( \ + const_cast(&this->state_), \ + static_cast>(args)...); \ } \ } -- cgit v1.2.3