Skip to content

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)

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