From 81d48b3e436c1d1d47649ccd8ceb5e38d77a6a69 Mon Sep 17 00:00:00 2001 From: Abseil Team Date: Fri, 10 Jan 2025 11:42:36 -0800 Subject: [PATCH] Annotate some Abseil container methods with [[clang::lifetime_capture_by(...)]] This will allow catching some lifetime issues such as the following: ``` absl::flat_hash_set s; s.add(std::string(...)); // dangling ``` PiperOrigin-RevId: 714123078 Change-Id: I67f382e97fa8d5e2dc2b58a1ccb8cc013819e4b3 --- absl/base/attributes.h | 11 ++ absl/container/BUILD.bazel | 1 + absl/container/CMakeLists.txt | 1 + .../container/internal/common_policy_traits.h | 9 ++ absl/container/internal/raw_hash_set.h | 135 ++++++++++++------ absl/meta/type_traits.h | 6 +- 6 files changed, 117 insertions(+), 46 deletions(-) diff --git a/absl/base/attributes.h b/absl/base/attributes.h index 0b94ae43b79..95b102e52bc 100644 --- a/absl/base/attributes.h +++ b/absl/base/attributes.h @@ -831,6 +831,17 @@ #define ABSL_ATTRIBUTE_LIFETIME_BOUND #endif +// Internal attribute; name and documentation TBD. +// +// See the upstream documentation: +// https://clang.llvm.org/docs/AttributeReference.html#lifetime_capture_by +#if ABSL_HAVE_CPP_ATTRIBUTE(clang::lifetime_capture_by) +#define ABSL_INTERNAL_ATTRIBUTE_CAPTURED_BY(Owner) \ + [[clang::lifetime_capture_by(Owner)]] +#else +#define ABSL_INTERNAL_ATTRIBUTE_CAPTURED_BY(Owner) +#endif + // ABSL_ATTRIBUTE_VIEW indicates that a type is solely a "view" of data that it // points to, similarly to a span, string_view, or other non-owning reference // type. diff --git a/absl/container/BUILD.bazel b/absl/container/BUILD.bazel index 776a516b614..19dc91de1db 100644 --- a/absl/container/BUILD.bazel +++ b/absl/container/BUILD.bazel @@ -682,6 +682,7 @@ cc_library( linkopts = ABSL_DEFAULT_LINKOPTS, deps = [ ":common", + ":common_policy_traits", ":compressed_tuple", ":container_memory", ":hash_function_defaults", diff --git a/absl/container/CMakeLists.txt b/absl/container/CMakeLists.txt index b9152d24f67..85a4ce219ec 100644 --- a/absl/container/CMakeLists.txt +++ b/absl/container/CMakeLists.txt @@ -749,6 +749,7 @@ absl_cc_library( ${ABSL_DEFAULT_COPTS} DEPS absl::bits + absl::common_policy_traits absl::compressed_tuple absl::config absl::container_common diff --git a/absl/container/internal/common_policy_traits.h b/absl/container/internal/common_policy_traits.h index c521f6122ee..bbf54750f94 100644 --- a/absl/container/internal/common_policy_traits.h +++ b/absl/container/internal/common_policy_traits.h @@ -28,6 +28,15 @@ namespace absl { ABSL_NAMESPACE_BEGIN namespace container_internal { +template +struct policy_trait_element_is_owner : std::false_type {}; + +template +struct policy_trait_element_is_owner< + Policy, + std::enable_if_t::value>> + : Policy::element_is_owner {}; + // Defines how slots are initialized/destroyed/moved. template struct common_policy_traits { diff --git a/absl/container/internal/raw_hash_set.h b/absl/container/internal/raw_hash_set.h index 5cfa98bd5dc..24f60118ce9 100644 --- a/absl/container/internal/raw_hash_set.h +++ b/absl/container/internal/raw_hash_set.h @@ -209,6 +209,7 @@ #include "absl/base/port.h" #include "absl/base/prefetch.h" #include "absl/container/internal/common.h" // IWYU pragma: export // for node_handle +#include "absl/container/internal/common_policy_traits.h" #include "absl/container/internal/compressed_tuple.h" #include "absl/container/internal/container_memory.h" #include "absl/container/internal/hash_function_defaults.h" @@ -2436,23 +2437,14 @@ class raw_hash_set { static_assert(std::is_lvalue_reference::value, "Policy::element() must return a reference"); - template - struct SameAsElementReference - : std::is_same::type>::type, - typename std::remove_cv< - typename std::remove_reference::type>::type> {}; - // An enabler for insert(T&&): T must be convertible to init_type or be the // same as [cv] value_type [ref]. - // Note: we separate SameAsElementReference into its own type to avoid using - // reference unless we need to. MSVC doesn't seem to like it in some - // cases. template - using RequiresInsertable = typename std::enable_if< - absl::disjunction, - SameAsElementReference>::value, - int>::type; + using Insertable = absl::disjunction< + std::is_same, absl::remove_cvref_t>, + std::is_convertible>; + template + using IsNotBitField = std::is_pointer; // RequiresNotInit is a workaround for gcc prior to 7.1. // See https://godbolt.org/g/Y4xsUh. @@ -2463,6 +2455,17 @@ class raw_hash_set { template using IsDecomposable = IsDecomposable; + template + using IsDecomposableAndInsertable = + IsDecomposable::value, T>>; + + // Evaluates to true if an assignment from the given type would require the + // source object to remain alive for the life of the element. + template + using IsLifetimeBoundAssignmentFrom = std::conditional_t< + policy_trait_element_is_owner::value, std::false_type, + type_traits_internal::IsLifetimeBoundAssignment>; + public: static_assert(std::is_same::value, "Allocators with custom pointer types are not supported"); @@ -2706,7 +2709,8 @@ class raw_hash_set { // absl::flat_hash_set a, b{a}; // // RequiresNotInit is a workaround for gcc prior to 7.1. - template = 0, RequiresInsertable = 0> + template = 0, + std::enable_if_t::value, int> = 0> raw_hash_set(std::initializer_list init, size_t bucket_count = 0, const hasher& hash = hasher(), const key_equal& eq = key_equal(), const allocator_type& alloc = allocator_type()) @@ -2717,7 +2721,8 @@ class raw_hash_set { const allocator_type& alloc = allocator_type()) : raw_hash_set(init.begin(), init.end(), bucket_count, hash, eq, alloc) {} - template = 0, RequiresInsertable = 0> + template = 0, + std::enable_if_t::value, int> = 0> raw_hash_set(std::initializer_list init, size_t bucket_count, const hasher& hash, const allocator_type& alloc) : raw_hash_set(init, bucket_count, hash, key_equal(), alloc) {} @@ -2726,7 +2731,8 @@ class raw_hash_set { const hasher& hash, const allocator_type& alloc) : raw_hash_set(init, bucket_count, hash, key_equal(), alloc) {} - template = 0, RequiresInsertable = 0> + template = 0, + std::enable_if_t::value, int> = 0> raw_hash_set(std::initializer_list init, size_t bucket_count, const allocator_type& alloc) : raw_hash_set(init, bucket_count, hasher(), key_equal(), alloc) {} @@ -2735,7 +2741,8 @@ class raw_hash_set { const allocator_type& alloc) : raw_hash_set(init, bucket_count, hasher(), key_equal(), alloc) {} - template = 0, RequiresInsertable = 0> + template = 0, + std::enable_if_t::value, int> = 0> raw_hash_set(std::initializer_list init, const allocator_type& alloc) : raw_hash_set(init, 0, hasher(), key_equal(), alloc) {} @@ -2951,15 +2958,26 @@ class raw_hash_set { // // flat_hash_map m; // m.insert(std::make_pair("abc", 42)); - // TODO(cheshire): A type alias T2 is introduced as a workaround for the nvcc - // bug. - template = 0, class T2 = T, - typename std::enable_if::value, int>::type = 0, - T* = nullptr> + template ::value && + IsNotBitField::value && + !IsLifetimeBoundAssignmentFrom::value, + int> = 0> std::pair insert(T&& value) ABSL_ATTRIBUTE_LIFETIME_BOUND { return emplace(std::forward(value)); } + template ::value && + IsNotBitField::value && + IsLifetimeBoundAssignmentFrom::value, + int> = 0> + std::pair insert( + T&& value ABSL_INTERNAL_ATTRIBUTE_CAPTURED_BY(this)) + ABSL_ATTRIBUTE_LIFETIME_BOUND { + return emplace(std::forward(value)); + } + // This overload kicks in when the argument is a bitfield or an lvalue of // insertable and decomposable type. // @@ -2971,13 +2989,23 @@ class raw_hash_set { // const char* p = "hello"; // s.insert(p); // - template < - class T, RequiresInsertable = 0, - typename std::enable_if::value, int>::type = 0> + template ::value && + !IsLifetimeBoundAssignmentFrom::value, + int> = 0> std::pair insert(const T& value) ABSL_ATTRIBUTE_LIFETIME_BOUND { return emplace(value); } + template ::value && + IsLifetimeBoundAssignmentFrom::value, + int> = 0> + std::pair insert( + const T& value ABSL_INTERNAL_ATTRIBUTE_CAPTURED_BY(this)) + ABSL_ATTRIBUTE_LIFETIME_BOUND { + return emplace(value); + } // This overload kicks in when the argument is an rvalue of init_type. Its // purpose is to handle brace-init-list arguments. @@ -2985,22 +3013,43 @@ class raw_hash_set { // flat_hash_map s; // s.insert({"abc", 42}); std::pair insert(init_type&& value) - ABSL_ATTRIBUTE_LIFETIME_BOUND { + ABSL_ATTRIBUTE_LIFETIME_BOUND +#if __cplusplus >= 202002L + requires(!IsLifetimeBoundAssignmentFrom::value) +#endif + { return emplace(std::move(value)); } +#if __cplusplus >= 202002L + std::pair insert( + init_type&& value ABSL_INTERNAL_ATTRIBUTE_CAPTURED_BY(this)) + ABSL_ATTRIBUTE_LIFETIME_BOUND + requires(IsLifetimeBoundAssignmentFrom::value) + { + return emplace(std::move(value)); + } +#endif - // TODO(cheshire): A type alias T2 is introduced as a workaround for the nvcc - // bug. - template = 0, class T2 = T, - typename std::enable_if::value, int>::type = 0, - T* = nullptr> + template ::value && + IsNotBitField::value && + !IsLifetimeBoundAssignmentFrom::value, + int> = 0> iterator insert(const_iterator, T&& value) ABSL_ATTRIBUTE_LIFETIME_BOUND { return insert(std::forward(value)).first; } + template ::value && + IsNotBitField::value && + IsLifetimeBoundAssignmentFrom::value, + int> = 0> + iterator insert(const_iterator, T&& value ABSL_INTERNAL_ATTRIBUTE_CAPTURED_BY( + this)) ABSL_ATTRIBUTE_LIFETIME_BOUND { + return insert(std::forward(value)).first; + } - template < - class T, RequiresInsertable = 0, - typename std::enable_if::value, int>::type = 0> + template ::value, int> = 0> iterator insert(const_iterator, const T& value) ABSL_ATTRIBUTE_LIFETIME_BOUND { return insert(value).first; @@ -3016,7 +3065,8 @@ class raw_hash_set { for (; first != last; ++first) emplace(*first); } - template = 0, RequiresInsertable = 0> + template = 0, + std::enable_if_t::value, int> = 0> void insert(std::initializer_list ilist) { insert(ilist.begin(), ilist.end()); } @@ -3055,8 +3105,8 @@ class raw_hash_set { // flat_hash_map m = {{"abc", "def"}}; // // Creates no std::string copies and makes no heap allocations. // m.emplace("abc", "xyz"); - template ::value, int>::type = 0> + template ::value, int> = 0> std::pair emplace(Args&&... args) ABSL_ATTRIBUTE_LIFETIME_BOUND { return PolicyTraits::apply(EmplaceDecomposable{*this}, @@ -3066,8 +3116,8 @@ class raw_hash_set { // This overload kicks in if we cannot deduce the key from args. It constructs // value_type unconditionally and then either moves it into the table or // destroys. - template ::value, int>::type = 0> + template ::value, int> = 0> std::pair emplace(Args&&... args) ABSL_ATTRIBUTE_LIFETIME_BOUND { alignas(slot_type) unsigned char raw[sizeof(slot_type)]; @@ -3259,9 +3309,8 @@ class raw_hash_set { return node; } - template < - class K = key_type, - typename std::enable_if::value, int>::type = 0> + template ::value, int> = 0> node_type extract(const key_arg& key) { auto it = find(key); return it == end() ? node_type() : extract(const_iterator{it}); diff --git a/absl/meta/type_traits.h b/absl/meta/type_traits.h index d52c49ef526..02da0674a88 100644 --- a/absl/meta/type_traits.h +++ b/absl/meta/type_traits.h @@ -658,9 +658,9 @@ struct IsView> : std::true_type {}; // Until then, we consider an assignment from an "owner" (such as std::string) // to a "view" (such as std::string_view) to be a lifetime-bound assignment. template -using IsLifetimeBoundAssignment = - std::integral_constant>::value && - IsOwner>::value>; +using IsLifetimeBoundAssignment = absl::conjunction< + std::integral_constant::value>, + IsOwner>, IsView>>; } // namespace type_traits_internal