Skip to content

Reapply "[libc++] Optimize vector growing of trivially relocatable types" #80558

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 3, 2024

Conversation

philnik777
Copy link
Contributor

This reapplies #76657. Non-trivial elements didn't get destroyed previously. This fixes the bug and adds tests for all the vector insertion functions.

@philnik777 philnik777 requested a review from a team as a code owner February 3, 2024 19:32
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Feb 3, 2024
@llvmbot
Copy link
Member

llvmbot commented Feb 3, 2024

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

This reapplies #76657. Non-trivial elements didn't get destroyed previously. This fixes the bug and adds tests for all the vector insertion functions.


Patch is 29.05 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/80558.diff

14 Files Affected:

  • (modified) libcxx/benchmarks/ContainerBenchmarks.h (+1-1)
  • (modified) libcxx/benchmarks/vector_operations.bench.cpp (+17-1)
  • (modified) libcxx/docs/ReleaseNotes/19.rst (+1-1)
  • (modified) libcxx/include/CMakeLists.txt (+1)
  • (modified) libcxx/include/__memory/uninitialized_algorithms.h (+42-44)
  • (modified) libcxx/include/__memory/unique_ptr.h (+24)
  • (added) libcxx/include/__type_traits/is_trivially_relocatable.h (+42)
  • (modified) libcxx/include/libcxx.imp (+1)
  • (modified) libcxx/include/module.modulemap.in (+1)
  • (modified) libcxx/include/string (+16)
  • (modified) libcxx/include/vector (+28-11)
  • (added) libcxx/test/libcxx/type_traits/is_trivially_relocatable.compile.pass.cpp (+115)
  • (added) libcxx/test/std/containers/sequences/vector/vector.modifiers/destory_elements.pass.cpp (+70)
  • (modified) libcxx/test/support/count_new.h (+7-4)
diff --git a/libcxx/benchmarks/ContainerBenchmarks.h b/libcxx/benchmarks/ContainerBenchmarks.h
index 9a9abfd3b0d0f..744505b439985 100644
--- a/libcxx/benchmarks/ContainerBenchmarks.h
+++ b/libcxx/benchmarks/ContainerBenchmarks.h
@@ -80,7 +80,7 @@ void BM_ConstructFromRange(benchmark::State& st, Container, GenInputs gen) {
 }
 
 template <class Container>
-void BM_Pushback(benchmark::State& state, Container c) {
+void BM_Pushback_no_grow(benchmark::State& state, Container c) {
   int count = state.range(0);
   c.reserve(count);
   while (state.KeepRunningBatch(count)) {
diff --git a/libcxx/benchmarks/vector_operations.bench.cpp b/libcxx/benchmarks/vector_operations.bench.cpp
index 38b14c56756fb..da21d1806c012 100644
--- a/libcxx/benchmarks/vector_operations.bench.cpp
+++ b/libcxx/benchmarks/vector_operations.bench.cpp
@@ -1,6 +1,7 @@
 #include <cstdint>
 #include <cstdlib>
 #include <cstring>
+#include <deque>
 #include <functional>
 #include <vector>
 
@@ -39,6 +40,21 @@ BENCHMARK_CAPTURE(BM_ConstructFromRange, vector_size_t, std::vector<size_t>{}, g
 BENCHMARK_CAPTURE(BM_ConstructFromRange, vector_string, std::vector<std::string>{}, getRandomStringInputs)
     ->Arg(TestNumInputs);
 
-BENCHMARK_CAPTURE(BM_Pushback, vector_int, std::vector<int>{})->Arg(TestNumInputs);
+BENCHMARK_CAPTURE(BM_Pushback_no_grow, vector_int, std::vector<int>{})->Arg(TestNumInputs);
+
+template <class T>
+void bm_grow(benchmark::State& state) {
+  for (auto _ : state) {
+    std::vector<T> vec;
+    benchmark::DoNotOptimize(vec);
+    for (size_t i = 0; i != 2048; ++i)
+      vec.emplace_back();
+    benchmark::DoNotOptimize(vec);
+  }
+}
+BENCHMARK(bm_grow<int>);
+BENCHMARK(bm_grow<std::string>);
+BENCHMARK(bm_grow<std::unique_ptr<int>>);
+BENCHMARK(bm_grow<std::deque<int>>);
 
 BENCHMARK_MAIN();
diff --git a/libcxx/docs/ReleaseNotes/19.rst b/libcxx/docs/ReleaseNotes/19.rst
index db731de2e4399..908d46b711a5a 100644
--- a/libcxx/docs/ReleaseNotes/19.rst
+++ b/libcxx/docs/ReleaseNotes/19.rst
@@ -44,8 +44,8 @@ Implemented Papers
 
 Improvements and New Features
 -----------------------------
-TODO
 
+- The performance of growing ``std::vector`` has been improved for trivially relocatable types.
 
 Deprecations and Removals
 -------------------------
diff --git a/libcxx/include/CMakeLists.txt b/libcxx/include/CMakeLists.txt
index 6a845c813dc33..d8ee598dc2633 100644
--- a/libcxx/include/CMakeLists.txt
+++ b/libcxx/include/CMakeLists.txt
@@ -808,6 +808,7 @@ set(files
   __type_traits/is_trivially_lexicographically_comparable.h
   __type_traits/is_trivially_move_assignable.h
   __type_traits/is_trivially_move_constructible.h
+  __type_traits/is_trivially_relocatable.h
   __type_traits/is_unbounded_array.h
   __type_traits/is_union.h
   __type_traits/is_unsigned.h
diff --git a/libcxx/include/__memory/uninitialized_algorithms.h b/libcxx/include/__memory/uninitialized_algorithms.h
index 9aff93a896486..9733bb748f665 100644
--- a/libcxx/include/__memory/uninitialized_algorithms.h
+++ b/libcxx/include/__memory/uninitialized_algorithms.h
@@ -29,6 +29,7 @@
 #include <__type_traits/is_trivially_copy_constructible.h>
 #include <__type_traits/is_trivially_move_assignable.h>
 #include <__type_traits/is_trivially_move_constructible.h>
+#include <__type_traits/is_trivially_relocatable.h>
 #include <__type_traits/is_unbounded_array.h>
 #include <__type_traits/negation.h>
 #include <__type_traits/remove_const.h>
@@ -594,60 +595,57 @@ __uninitialized_allocator_copy(_Alloc& __alloc, _Iter1 __first1, _Sent1 __last1,
   return std::__rewrap_iter(__first2, __result);
 }
 
-// Move-construct the elements [__first1, __last1) into [__first2, __first2 + N)
-// if the move constructor is noexcept, where N is distance(__first1, __last1).
-//
-// Otherwise try to copy all elements. If an exception is thrown the already copied
-// elements are destroyed in reverse order of their construction.
-template <class _Alloc, class _Iter1, class _Sent1, class _Iter2>
-_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _Iter2
-__uninitialized_allocator_move_if_noexcept(_Alloc& __alloc, _Iter1 __first1, _Sent1 __last1, _Iter2 __first2) {
-  static_assert(__is_cpp17_move_insertable<_Alloc>::value,
-                "The specified type does not meet the requirements of Cpp17MoveInsertable");
-  auto __destruct_first = __first2;
-  auto __guard =
-      std::__make_exception_guard(_AllocatorDestroyRangeReverse<_Alloc, _Iter2>(__alloc, __destruct_first, __first2));
-  while (__first1 != __last1) {
-#ifndef _LIBCPP_HAS_NO_EXCEPTIONS
-    allocator_traits<_Alloc>::construct(__alloc, std::__to_address(__first2), std::move_if_noexcept(*__first1));
-#else
-    allocator_traits<_Alloc>::construct(__alloc, std::__to_address(__first2), std::move(*__first1));
-#endif
-    ++__first1;
-    ++__first2;
-  }
-  __guard.__complete();
-  return __first2;
-}
-
 template <class _Alloc, class _Type>
 struct __allocator_has_trivial_move_construct : _Not<__has_construct<_Alloc, _Type*, _Type&&> > {};
 
 template <class _Type>
 struct __allocator_has_trivial_move_construct<allocator<_Type>, _Type> : true_type {};
 
-#ifndef _LIBCPP_COMPILER_GCC
-template <
-    class _Alloc,
-    class _Iter1,
-    class _Iter2,
-    class _Type = typename iterator_traits<_Iter1>::value_type,
-    class = __enable_if_t<is_trivially_move_constructible<_Type>::value && is_trivially_move_assignable<_Type>::value &&
-                          __allocator_has_trivial_move_construct<_Alloc, _Type>::value> >
-_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _Iter2
-__uninitialized_allocator_move_if_noexcept(_Alloc&, _Iter1 __first1, _Iter1 __last1, _Iter2 __first2) {
-  if (__libcpp_is_constant_evaluated()) {
-    while (__first1 != __last1) {
-      std::__construct_at(std::__to_address(__first2), std::move(*__first1));
-      ++__first1;
-      ++__first2;
+template <class _Alloc, class _Tp>
+struct __allocator_has_trivial_destroy : _Not<__has_destroy<_Alloc, _Tp*> > {};
+
+template <class _Tp, class _Up>
+struct __allocator_has_trivial_destroy<allocator<_Tp>, _Up> : true_type {};
+
+// __uninitialized_allocator_relocate relocates the objects in [__first, __last) into __result.
+// Relocation means that the objects in [__first, __last) are placed into __result as-if by move-construct and destroy,
+// except that the move constructor and destructor may never be called if they are known to be equivalent to a memcpy.
+//
+// Preconditions:  __result doesn't contain any objects and [__first, __last) contains objects
+// Postconditions: __result contains the objects from [__first, __last) and
+//                 [__first, __last) doesn't contain any objects
+//
+// The strong exception guarantee is provided if any of the following are true:
+// - is_nothrow_move_constructible<_Tp>
+// - is_copy_constructible<_Tp>
+// - __libcpp_is_trivially_relocatable<_Tp>
+template <class _Alloc, class _Tp>
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 void
+__uninitialized_allocator_relocate(_Alloc& __alloc, _Tp* __first, _Tp* __last, _Tp* __result) {
+  static_assert(__is_cpp17_move_insertable<_Alloc>::value,
+                "The specified type does not meet the requirements of Cpp17MoveInsertable");
+  if (__libcpp_is_constant_evaluated() || !__libcpp_is_trivially_relocatable<_Tp>::value ||
+      !__allocator_has_trivial_move_construct<_Alloc, _Tp>::value ||
+      !__allocator_has_trivial_destroy<_Alloc, _Tp>::value) {
+    auto __destruct_first = __result;
+    auto __guard =
+        std::__make_exception_guard(_AllocatorDestroyRangeReverse<_Alloc, _Tp*>(__alloc, __destruct_first, __result));
+    auto __iter = __first;
+    while (__iter != __last) {
+#ifndef _LIBCPP_HAS_NO_EXCEPTIONS
+      allocator_traits<_Alloc>::construct(__alloc, __result, std::move_if_noexcept(*__iter));
+#else
+      allocator_traits<_Alloc>::construct(__alloc, __result, std::move(*__iter));
+#endif
+      ++__iter;
+      ++__result;
     }
-    return __first2;
+    __guard.__complete();
+    std::__allocator_destroy(__alloc, __first, __last);
   } else {
-    return std::move(__first1, __last1, __first2);
+    __builtin_memcpy(__result, __first, sizeof(_Tp) * (__last - __first));
   }
 }
-#endif // _LIBCPP_COMPILER_GCC
 
 _LIBCPP_END_NAMESPACE_STD
 
diff --git a/libcxx/include/__memory/unique_ptr.h b/libcxx/include/__memory/unique_ptr.h
index db473eaa50a6b..a505dab8dd749 100644
--- a/libcxx/include/__memory/unique_ptr.h
+++ b/libcxx/include/__memory/unique_ptr.h
@@ -21,6 +21,7 @@
 #include <__memory/compressed_pair.h>
 #include <__type_traits/add_lvalue_reference.h>
 #include <__type_traits/common_type.h>
+#include <__type_traits/conditional.h>
 #include <__type_traits/dependent_type.h>
 #include <__type_traits/integral_constant.h>
 #include <__type_traits/is_array.h>
@@ -33,6 +34,7 @@
 #include <__type_traits/is_reference.h>
 #include <__type_traits/is_same.h>
 #include <__type_traits/is_swappable.h>
+#include <__type_traits/is_trivially_relocatable.h>
 #include <__type_traits/is_void.h>
 #include <__type_traits/remove_extent.h>
 #include <__type_traits/type_identity.h>
@@ -129,6 +131,17 @@ class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr {
 
   static_assert(!is_rvalue_reference<deleter_type>::value, "the specified deleter type cannot be an rvalue reference");
 
+  // A unique_ptr contains the following members which may be trivially relocatable:
+  // - pointer : this may be trivially relocatable, so it's checked
+  // - deleter_type: this may be trivially relocatable, so it's checked
+  //
+  // This unique_ptr implementation only contains a pointer to the unique object and a deleter, so there are no
+  // references to itself. This means that the entire structure is trivially relocatable if its members are.
+  using __trivially_relocatable = __conditional_t<
+      __libcpp_is_trivially_relocatable<pointer>::value && __libcpp_is_trivially_relocatable<deleter_type>::value,
+      unique_ptr,
+      void>;
+
 private:
   __compressed_pair<pointer, deleter_type> __ptr_;
 
@@ -276,6 +289,17 @@ class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr<_Tp[], _Dp>
   typedef _Dp deleter_type;
   typedef typename __pointer<_Tp, deleter_type>::type pointer;
 
+  // A unique_ptr contains the following members which may be trivially relocatable:
+  // - pointer : this may be trivially relocatable, so it's checked
+  // - deleter_type: this may be trivially relocatable, so it's checked
+  //
+  // This unique_ptr implementation only contains a pointer to the unique object and a deleter, so there are no
+  // references to itself. This means that the entire structure is trivially relocatable if its members are.
+  using __trivially_relocatable = __conditional_t<
+      __libcpp_is_trivially_relocatable<pointer>::value && __libcpp_is_trivially_relocatable<deleter_type>::value,
+      unique_ptr,
+      void>;
+
 private:
   __compressed_pair<pointer, deleter_type> __ptr_;
 
diff --git a/libcxx/include/__type_traits/is_trivially_relocatable.h b/libcxx/include/__type_traits/is_trivially_relocatable.h
new file mode 100644
index 0000000000000..c0871731cc001
--- /dev/null
+++ b/libcxx/include/__type_traits/is_trivially_relocatable.h
@@ -0,0 +1,42 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef _LIBCPP___TYPE_TRAITS_IS_TRIVIALLY_RELOCATABLE_H
+#define _LIBCPP___TYPE_TRAITS_IS_TRIVIALLY_RELOCATABLE_H
+
+#include <__config>
+#include <__type_traits/enable_if.h>
+#include <__type_traits/integral_constant.h>
+#include <__type_traits/is_same.h>
+#include <__type_traits/is_trivially_copyable.h>
+
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+#  pragma GCC system_header
+#endif
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+// A type is trivially relocatable if a move construct + destroy of the original object is equivalent to
+// `memcpy(dst, src, sizeof(T))`.
+
+#if __has_builtin(__is_trivially_relocatable)
+template <class _Tp, class = void>
+struct __libcpp_is_trivially_relocatable : integral_constant<bool, __is_trivially_relocatable(_Tp)> {};
+#else
+template <class _Tp, class = void>
+struct __libcpp_is_trivially_relocatable : is_trivially_copyable<_Tp> {};
+#endif
+
+template <class _Tp>
+struct __libcpp_is_trivially_relocatable<_Tp,
+                                         __enable_if_t<is_same<_Tp, typename _Tp::__trivially_relocatable>::value> >
+    : true_type {};
+
+_LIBCPP_END_NAMESPACE_STD
+
+#endif // _LIBCPP___TYPE_TRAITS_IS_TRIVIALLY_RELOCATABLE_H
diff --git a/libcxx/include/libcxx.imp b/libcxx/include/libcxx.imp
index 13c0dfeb2bc98..8509c7e39a666 100644
--- a/libcxx/include/libcxx.imp
+++ b/libcxx/include/libcxx.imp
@@ -796,6 +796,7 @@
   { include: [ "<__type_traits/is_trivially_lexicographically_comparable.h>", "private", "<type_traits>", "public" ] },
   { include: [ "<__type_traits/is_trivially_move_assignable.h>", "private", "<type_traits>", "public" ] },
   { include: [ "<__type_traits/is_trivially_move_constructible.h>", "private", "<type_traits>", "public" ] },
+  { include: [ "<__type_traits/is_trivially_relocatable.h>", "private", "<type_traits>", "public" ] },
   { include: [ "<__type_traits/is_unbounded_array.h>", "private", "<type_traits>", "public" ] },
   { include: [ "<__type_traits/is_union.h>", "private", "<type_traits>", "public" ] },
   { include: [ "<__type_traits/is_unsigned.h>", "private", "<type_traits>", "public" ] },
diff --git a/libcxx/include/module.modulemap.in b/libcxx/include/module.modulemap.in
index 03f71290b6603..ddb0f4b70e02b 100644
--- a/libcxx/include/module.modulemap.in
+++ b/libcxx/include/module.modulemap.in
@@ -1970,6 +1970,7 @@ module std_private_type_traits_is_trivially_destructible                 [system
 module std_private_type_traits_is_trivially_lexicographically_comparable [system] { header "__type_traits/is_trivially_lexicographically_comparable.h" }
 module std_private_type_traits_is_trivially_move_assignable              [system] { header "__type_traits/is_trivially_move_assignable.h" }
 module std_private_type_traits_is_trivially_move_constructible           [system] { header "__type_traits/is_trivially_move_constructible.h" }
+module std_private_type_traits_is_trivially_relocatable                  [system] { header "__type_traits/is_trivially_relocatable.h" }
 module std_private_type_traits_is_unbounded_array                        [system] { header "__type_traits/is_unbounded_array.h" }
 module std_private_type_traits_is_union                                  [system] { header "__type_traits/is_union.h" }
 module std_private_type_traits_is_unsigned                               [system] { header "__type_traits/is_unsigned.h" }
diff --git a/libcxx/include/string b/libcxx/include/string
index efdff3dd42da0..ed4fdbe6864c2 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -599,6 +599,7 @@ basic_string<char32_t> operator""s( const char32_t *str, size_t len );
 #include <__ranges/size.h>
 #include <__string/char_traits.h>
 #include <__string/extern_template_lists.h>
+#include <__type_traits/conditional.h>
 #include <__type_traits/is_allocator.h>
 #include <__type_traits/is_array.h>
 #include <__type_traits/is_convertible.h>
@@ -607,6 +608,7 @@ basic_string<char32_t> operator""s( const char32_t *str, size_t len );
 #include <__type_traits/is_same.h>
 #include <__type_traits/is_standard_layout.h>
 #include <__type_traits/is_trivial.h>
+#include <__type_traits/is_trivially_relocatable.h>
 #include <__type_traits/noexcept_move_assign_container.h>
 #include <__type_traits/remove_cvref.h>
 #include <__type_traits/void_t.h>
@@ -724,6 +726,20 @@ public:
   typedef typename __alloc_traits::pointer pointer;
   typedef typename __alloc_traits::const_pointer const_pointer;
 
+  // A basic_string contains the following members which may be trivially relocatable:
+  // - pointer: is currently assumed to be trivially relocatable, but is still checked in case that changes
+  // - size_type: is always trivially relocatable, since it has to be an integral type
+  // - value_type: is always trivially relocatable, since it has to be trivial
+  // - unsigned char: is a fundamental type, so it's trivially relocatable
+  // - allocator_type: may or may not be trivially relocatable, so it's checked
+  //
+  // This string implementation doesn't contain any references into itself. It only contains a bit that says whether
+  // it is in small or large string mode, so the entire structure is trivially relocatable if its members are.
+  using __trivially_relocatable = __conditional_t<
+      __libcpp_is_trivially_relocatable<allocator_type>::value && __libcpp_is_trivially_relocatable<pointer>::value,
+      basic_string,
+      void>;
+
   static_assert((!is_array<value_type>::value), "Character type of basic_string must not be an array");
   static_assert((is_standard_layout<value_type>::value), "Character type of basic_string must be standard-layout");
   static_assert((is_trivial<value_type>::value), "Character type of basic_string must be trivial");
diff --git a/libcxx/include/vector b/libcxx/include/vector
index 5e2027ea21a39..e9615ab4c9a30 100644
--- a/libcxx/include/vector
+++ b/libcxx/include/vector
@@ -982,14 +982,18 @@ template <ranges::input_range _Range,
 vector(from_range_t, _Range&&, _Alloc = _Alloc()) -> vector<ranges::range_value_t<_Range>, _Alloc>;
 #endif
 
+// __swap_out_circular_buffer relocates the objects in [__begin_, __end_) into the front of __v and swaps the buffers of
+// *this and __v. It is assumed that __v provides space for exactly (__end_ - __begin_) objects in the front. This
+// function has a strong exception guarantee.
 template <class _Tp, class _Allocator>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 void
 vector<_Tp, _Allocator>::__swap_out_circular_buffer(__split_buffer<value_type, allocator_type&>& __v) {
   __annotate_delete();
-  using _RevIter = std::reverse_iterator<pointer>;
-  __v.__begin_   = std::__uninitialized_allocator_move_if_noexcept(
-                     __alloc(), _RevIter(__end_), _RevIter(__begin_), _RevIter(__v.__begin_))
-                     .base();
+  auto __new_begin = __v.__begin_ - (__end_ - __begin_);
+  std::__uninitialized_allocator_relocate(
+      __alloc(), std::__to_address(__begin_), std::__to_address(__end_), std::__to_address(__new_begin));
+  __v.__begin_ = __new_begin;
+  __end_       = __begin_; // All the objects have been destroyed by relocating them.
   std::swap(this->__begin_, __v.__begin_);
   std::swap(this->__end_, __v.__end_);
   std::swap(this->__end_cap(), __v.__end_cap());
@@ -997,22 +1001,35 @@ vector<_Tp, _Allocator>::__swap_out_circular_buffer(__split_buffer<value_type, a
   __annotate_new(size());
 }
 
+// __swap_out_circular_buffer relocates the objects in [__begin_, __p) into the front of __v, the objects in
+// [__p, __end_) into the back of __v and swaps the buffers of *this and __v. It is assumed that __v provides space for
+// exactly (__p - __begin_) objects in the front and space for at least (__end_ - __p) objects in the back. This
+// function has a strong exception guarantee if __begin_ == __p || __end_ == __p.
 template <class _Tp, class _Allocator>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 typename vector<_Tp, _Allocator>::pointer
 vector<_Tp, _Allocator>::__swap_out_circular_buffer(__split_buffer<value_type, allocator_type&>& __v, pointer __p) {
   __annotate_delete();
-  pointer __r    = __v.__begin_;
-  using _RevIter = std::reverse_iterator<pointer>;
-  __v.__begin_   = std::__uninitialized_allocator_move_if_noexcept(
-                     __alloc(), _RevIter(__p), _RevIter(__begin_), _RevIter(__v.__begin_))
-                     .base();
-  __v.__end_ = std::__uninitialized_allocator_move_if_noexcept(__alloc(), __p, __end_, __v.__end_);
+  pointer __ret = __v.__begin_;
+
+  // Relocate [__p, __end_) first to avoid having a hole in [__begin_, __end_)
+  // in case something in [__begin_, __p) throws.
+  std::__uninitialized_allocator_relocate(
+      __alloc(), std::__to_address(__p), std::__to_address(__end_), std::__to_address(__v.__end_));
+  __v.__end_ += (__end_ - __p);
+  __end_           = __p; // The objects in [__p, __end_) have b...
[truncated]

@philnik777
Copy link
Contributor Author

I'll land this when the CI is happy.

Copy link

github-actions bot commented Feb 3, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@philnik777 philnik777 force-pushed the uninitilaized_allocator_relocate branch 3 times, most recently from 7c737cb to 0e0ccde Compare February 3, 2024 23:25
@philnik777 philnik777 force-pushed the uninitilaized_allocator_relocate branch from 0e0ccde to e4c10a6 Compare February 3, 2024 23:25
@philnik777 philnik777 merged commit 4e112e5 into llvm:main Feb 3, 2024
@philnik777 philnik777 deleted the uninitilaized_allocator_relocate branch February 3, 2024 23:28
@nico
Copy link
Contributor

nico commented Feb 4, 2024

We're seeing compile errors after this, e.g.

In file included from ../../cc/metrics/frame_sorter_unittest.cc:5:
In file included from ../../cc/metrics/frame_sorter.h:13:
In file included from ../../base/containers/circular_deque.h:14:
In file included from ../../base/check.h:17:
In file included from ../../base/memory/raw_ptr.h:11:
In file included from ../../base/allocator/partition_allocator/src/partition_alloc/pointers/raw_ptr.h:10:
In file included from ../../third_party/libc++/src/include/functional:526:
In file included from ../../third_party/libc++/src/include/__functional/boyer_moore_searcher.h:22:
In file included from ../../third_party/libc++/src/include/__memory/shared_ptr.h:32:
../../third_party/libc++/src/include/__memory/uninitialized_algorithms.h:646:22: error: cannot initialize a parameter of type 'void *' with an lvalue of type 'const viz::BeginFrameArgs *'
  646 |     __builtin_memcpy(__result, __first, sizeof(_Tp) * (__last - __first));
      |                      ^~~~~~~~
../../third_party/libc++/src/include/vector:993:8: note: in instantiation of function template specialization 'std::__uninitialized_allocator_relocate<std::allocator<const viz::BeginFrameArgs>, const viz::BeginFrameArgs>' requested here
  993 |   std::__uninitialized_allocator_relocate(
      |        ^
../../third_party/libc++/src/include/vector:1472:3: note: in instantiation of member function 'std::vector<const viz::BeginFrameArgs>::__swap_out_circular_buffer' requested here
 1472 |   __swap_out_circular_buffer(__v);
      |   ^
../../third_party/libc++/src/include/vector:1496:13: note: in instantiation of function template specialization 'std::vector<const viz::BeginFrameArgs>::__push_back_slow_path<const viz::BeginFrameArgs>' requested here
 1496 |     __end = __push_back_slow_path(std::move(__x));
      |             ^
../../cc/metrics/frame_sorter_unittest.cc:63:15: note: in instantiation of member function 'std::vector<const viz::BeginFrameArgs>::push_back' requested here
   63 |         args_.push_back(GetNextFrameArgs());
      |               ^

(https://source.chromium.org/chromium/chromium/src/+/main:cc/metrics/frame_sorter_unittest.cc;l=63?q=frame_sorter_unittest)

Is this expected?

@gribozavr
Copy link
Collaborator

gribozavr commented Feb 5, 2024

@nico args_ is a std::vector<const T>; const elements are not allowed. See also #73664.

@philnik777
Copy link
Contributor Author

@nico @gribozavr is correct that this is because of vector<const T>. I think I'd consider this a bug for now, but IMO we should just remove the allocator<const T> extension that allows this. It has bitten at least me way too often when working on vector, and isn't supported by any other major vendor: https://godbolt.org/z/8MPaP94cz. @ldionne Any thoughts on this?

@dwblaikie
Copy link
Collaborator

Agreed that they aren't allowed - but they have been (defacto, or intentionally - given the mention of the allocator<const T> extension) for some time. Removing them is probably good (though understanding why they were intentionally supported would probably be good to do before that) - but may need a longer migration plan so as not to break lots of code?

(might be worth reverting this patch to consider the fallout/whether there's a smoother way to stage this transition/removal?)

@dwblaikie
Copy link
Collaborator

(oh, the "I think I'd consider this a bug for now" - a bug in this patch? (yeah, for now, but it'd be good to remove the functionality/expectation eventually) - OK, makes sense, guess we're on the same page already - excuse the extra verbiage, then)

Quuxplusone added a commit to Quuxplusone/llvm-project that referenced this pull request Feb 5, 2024
agozillon pushed a commit to agozillon/llvm-project that referenced this pull request Feb 5, 2024
…pes" (llvm#80558)

This reapplies llvm#76657. Non-trivial elements didn't get destroyed
previously. This fixes the bug and adds tests for all the vector
insertion functions.
pranavk pushed a commit that referenced this pull request Feb 5, 2024
#80558 introduced code that assumed that the element type of `vector` is
never const. This fixes it and adds a test. Eventually we should remove
the `allocator<const T>` extension.
@MaskRay
Copy link
Member

MaskRay commented Feb 6, 2024

@nico @gribozavr is correct that this is because of vector<const T>. I think I'd consider this a bug for now, but IMO we should just remove the allocator<const T> extension that allows this. It has bitten at least me way too often when working on vector, and isn't supported by any other major vendor: godbolt.org/z/8MPaP94cz. @ldionne Any thoughts on this?

The clang-tidy check portability-std-allocator-const patch (https://reviews.llvm.org/D123655) contains some notes and can be useful if your code base has a lot of offending code.


We internally do use portability-std-allocator-const and the offending code does gets fewer and fewer, but there is still huge amount of offending code... (the root issue is that if vector<const T> occurs in a header file and the header file is used by many .cc files, it's difficult to do large-scale changes to remove them at once). So I really appreciate that #80711 worked around the problem.

@nico
Copy link
Contributor

nico commented Jul 12, 2024

@ldionne Any thoughts on this?

In case anyone else happens to find this PR here: This has now happened, over in #96319.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants