[libcxx-commits] [libcxx] [libc++] Replace __is_trivially_relocatable by is_trivially_copyable (PR #124970)

[via libcxx-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

<details>
<summary>Changes</summary>

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.

---
Full diff: https://github.com/llvm/llvm-project/pull/124970.diff


3 Files Affected:

- (modified) libcxx/include/__type_traits/is_trivially_relocatable.h (-7) 
- (modified) libcxx/test/libcxx/type_traits/is_trivially_relocatable.compile.pass.cpp (+10) 
- (added) libcxx/test/std/containers/sequences/vector/trivial_relocation.pass.cpp (+67) 


``````````diff
diff --git a/libcxx/include/__type_traits/is_trivially_relocatable.h b/libcxx/include/__type_traits/is_trivially_relocatable.h
index c0871731cc0016..ecc97a41dfbdcb 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,14 +22,8 @@ _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,
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 674df1d0219057..e43f38aa26f7d0 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
@@ -60,6 +60,16 @@ static_assert(std::__libcpp_is_trivially_relocatable<MoveOnlyTriviallyCopyable>:
 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 00000000000000..e109a5034032dd
--- /dev/null
+++ b/libcxx/test/std/containers/sequences/vector/trivial_relocation.pass.cpp
@@ -0,0 +1,67 @@
+//===----------------------------------------------------------------------===//
+//
+// 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 "test_macros.h"
+
+struct Tracker {
+  std::size_t move_constructs = 0;
+};
+
+struct [[clang::trivial_abi]] Inner {
+  TEST_CONSTEXPR explicit Inner(Tracker* tracker) : tracker_(tracker) {}
+  TEST_CONSTEXPR Inner(const Inner& rhs) : tracker_(rhs.tracker_) { tracker_->move_constructs += 1; }
+  TEST_CONSTEXPR 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 explicit NotTriviallyMovable(Tracker* tracker) : inner_(tracker) {}
+  TEST_CONSTEXPR 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);
+  for (std::size_t i = 0; i != 5; ++i) {
+    v.emplace_back(&track);
+  }
+  assert(track.move_constructs == 0);
+  assert(v.size() == 5);
+
+  // 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 == 5);
+
+  return true;
+}
+
+int main(int, char**) {
+  tests();
+#if TEST_STD_VER >= 20
+  static_assert(tests());
+#endif
+  return 0;
+}

``````````

</details>


https://github.com/llvm/llvm-project/pull/124970