[llvm-branch-commits] [libcxx] release/20.x: [libc++] Replace __is_trivially_relocatable by is_trivially_copyable (#124970) (PR #125996)

[via llvm-branch-commits]

https://github.com/llvmbot created https://github.com/llvm/llvm-project/pull/125996

Backport accfbd4cb327411ad66c0109ba1841482b871967

Requested by: @ldionne

>From bb87c8e305e2c91f28c402f8c83d861d6cdc7876 Mon Sep 17 00:00:00 2001
From: Louis Dionne <ldionne.2 at gmail.com>
Date: Wed, 5 Feb 2025 22:32:51 -0500
Subject: [PATCH] [libc++] Replace __is_trivially_relocatable by
 is_trivially_copyable (#124970)

The __is_trivially_relocatable builtin has semantics that do not
correspond to any current or future notion of trivial relocation.
Furthermore, it currently leads to incorrect optimizations for some
types on supported compilers:
- Clang on Windows where types with non-trivial destructors get
  incorrectly optimized
- AppleClang where types with non-trivial move constructors get
  incorrectly optimized

Until there is an agreed upon and bugfree implementation of what it
means to be trivially relocatable, it is safer to simply use trivially
copyable instead. This doesn't leave a lot of types behind and is
definitely correct.

(cherry picked from commit accfbd4cb327411ad66c0109ba1841482b871967)
---
 .../__type_traits/is_trivially_relocatable.h  |  8 ++-
 .../is_trivially_relocatable.compile.pass.cpp | 14 ++--
 .../vector/trivial_relocation.pass.cpp        | 71 +++++++++++++++++++
 3 files changed, 86 insertions(+), 7 deletions(-)
 create mode 100644 libcxx/test/std/containers/sequences/vector/trivial_relocation.pass.cpp

diff --git a/libcxx/include/__type_traits/is_trivially_relocatable.h b/libcxx/include/__type_traits/is_trivially_relocatable.h
index c0871731cc0016b..9b0e240de55f4ec 100644
--- a/libcxx/include/__type_traits/is_trivially_relocatable.h
+++ b/libcxx/include/__type_traits/is_trivially_relocatable.h
@@ -11,7 +11,6 @@
 
 #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>
 
@@ -23,8 +22,11 @@ _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)
+//
+// Note that we don't use the __is_trivially_relocatable Clang builtin right now because it does not
+// implement the semantics of any current or future trivial relocation proposal and it can lead to
+// incorrect optimizations on some platforms (Windows) and supported compilers (AppleClang).
+#if __has_builtin(__is_trivially_relocatable) && 0
 template <class _Tp, class = void>
 struct __libcpp_is_trivially_relocatable : integral_constant<bool, __is_trivially_relocatable(_Tp)> {};
 #else
diff --git a/libcxx/test/libcxx/type_traits/is_trivially_relocatable.compile.pass.cpp b/libcxx/test/libcxx/type_traits/is_trivially_relocatable.compile.pass.cpp
index 674df1d0219057d..213d06d314a075f 100644
--- a/libcxx/test/libcxx/type_traits/is_trivially_relocatable.compile.pass.cpp
+++ b/libcxx/test/libcxx/type_traits/is_trivially_relocatable.compile.pass.cpp
@@ -54,11 +54,17 @@ struct MoveOnlyTriviallyCopyable {
   MoveOnlyTriviallyCopyable(MoveOnlyTriviallyCopyable&&)                 = default;
   MoveOnlyTriviallyCopyable& operator=(MoveOnlyTriviallyCopyable&&)      = default;
 };
-#ifndef _MSC_VER
 static_assert(std::__libcpp_is_trivially_relocatable<MoveOnlyTriviallyCopyable>::value, "");
-#else
-static_assert(!std::__libcpp_is_trivially_relocatable<MoveOnlyTriviallyCopyable>::value, "");
-#endif
+
+struct NonTrivialMoveConstructor {
+  NonTrivialMoveConstructor(NonTrivialMoveConstructor&&);
+};
+static_assert(!std::__libcpp_is_trivially_relocatable<NonTrivialMoveConstructor>::value, "");
+
+struct NonTrivialDestructor {
+  ~NonTrivialDestructor() {}
+};
+static_assert(!std::__libcpp_is_trivially_relocatable<NonTrivialDestructor>::value, "");
 
 // library-internal types
 // ----------------------
diff --git a/libcxx/test/std/containers/sequences/vector/trivial_relocation.pass.cpp b/libcxx/test/std/containers/sequences/vector/trivial_relocation.pass.cpp
new file mode 100644
index 000000000000000..fbd597d07d6e32e
--- /dev/null
+++ b/libcxx/test/std/containers/sequences/vector/trivial_relocation.pass.cpp
@@ -0,0 +1,71 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// <vector>
+
+// Make sure we don't miscompile vector operations for types that shouldn't be considered
+// trivially relocatable.
+
+#include <vector>
+#include <cassert>
+#include <cstddef>
+#include <type_traits>
+#include <utility>
+
+#include "test_macros.h"
+
+struct Tracker {
+  std::size_t move_constructs = 0;
+};
+
+struct [[clang::trivial_abi]] Inner {
+  TEST_CONSTEXPR_CXX20 explicit Inner(Tracker* tracker) : tracker_(tracker) {}
+  TEST_CONSTEXPR_CXX20 Inner(const Inner& rhs) : tracker_(rhs.tracker_) { tracker_->move_constructs += 1; }
+  TEST_CONSTEXPR_CXX20 Inner(Inner&& rhs) : tracker_(rhs.tracker_) { tracker_->move_constructs += 1; }
+  Tracker* tracker_;
+};
+
+// Even though this type contains a trivial_abi type, it is not trivially move-constructible,
+// so we should not attempt to optimize its move construction + destroy using trivial relocation.
+struct NotTriviallyMovable {
+  TEST_CONSTEXPR_CXX20 explicit NotTriviallyMovable(Tracker* tracker) : inner_(tracker) {}
+  TEST_CONSTEXPR_CXX20 NotTriviallyMovable(NotTriviallyMovable&& other) : inner_(std::move(other.inner_)) {}
+  Inner inner_;
+};
+static_assert(!std::is_trivially_copyable<NotTriviallyMovable>::value, "");
+LIBCPP_STATIC_ASSERT(!std::__libcpp_is_trivially_relocatable<NotTriviallyMovable>::value, "");
+
+TEST_CONSTEXPR_CXX20 bool tests() {
+  Tracker track;
+  std::vector<NotTriviallyMovable> v;
+
+  // Fill the vector at its capacity, such that any subsequent push_back would require growing.
+  v.reserve(5);
+  std::size_t const capacity = v.capacity(); // could technically be more than 5
+  while (v.size() < v.capacity()) {
+    v.emplace_back(&track);
+  }
+  assert(track.move_constructs == 0);
+  assert(v.capacity() == capacity);
+  assert(v.size() == capacity);
+
+  // Force a reallocation of the buffer + relocalization of the elements.
+  // All the existing elements of the vector should be move-constructed to their new location.
+  v.emplace_back(&track);
+  assert(track.move_constructs == capacity);
+
+  return true;
+}
+
+int main(int, char**) {
+  tests();
+#if TEST_STD_VER >= 20
+  static_assert(tests());
+#endif
+  return 0;
+}