[libc++] Replace __is_trivially_relocatable by is_trivially_copyable

[ldionne]

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.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

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 --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;
+}

[ldionne]

/cherry-pick accfbd4

[llvmbot]

Failed to create pull request for issue124970 https://github.com/llvm/llvm-project/actions/runs/13171297029

[ldionne]

/cherry-pick accfbd4

[llvmbot]

/pull-request #125996

[zmodem]

We saw a slight binary increase from this, which I suppose was expected.

Could you add some color on why this change was made now, and what were the incorrect optimizations. I guess some destructors which were supposed to run didn't?

Do you think we'll be able to get this back in some form later?

[ldionne]

We saw a slight binary increase from this, which I suppose was expected.

I am a bit surprised about this, I didn't expect much noticeable impact, but it's definitely not impossible. If you have details on which types are now generating less optimal std::vector operations, I could investigate.

Could you add some color on why this change was made now, and what were the incorrect optimizations. I guess some destructors which were supposed to run didn't?

You can refer to the tests added by this PR: https://github.com/llvm/llvm-project/pull/124970/files#diff-55515a02488b32e9ba3c8cf44b5f5bc73c240db8e5b1a013aba93101cfb0ba21

I have been aware of this issue for a long time, maybe 6 months, but I was struggling to come up with a good test I could check-in so I punted on the issue. I recently revived this as part of resuming involvement in the trivial relocation proposals for WG21.

Since this is effectively a bug fix, I decided to backport it to LLVM 20.

Do you think we'll be able to get this back in some form later?

Yes, definitely. We are working towards a proper notion of trivial relocation and library utilities to take advantage of it in http://wg21.link/p3516 and http://wg21.link/p2786. Once the dust has settled on the approach we're taking in WG21, the hope is that the Clang builtin will be updated to mirror the Standard property and then we can switch back to it (perhaps with some opt-outs until we drop support for "broken" compilers).

[zmodem]

If you have details on which types are now generating less optimal std::vector operations, I could investigate.

It's a small increase spread across the binary, so it's hard to say for sure, but it's likely our raw_ptr type which has a test checking for exactly this and gets used a lot.

Do you think we'll be able to get this back in some form later?

Yes, [...]

Sounds great, we can just wait then. Thank you!

[ldionne]

If you have details on which types are now generating less optimal std::vector operations, I could investigate.

It's a small increase spread across the binary, so it's hard to say for sure, but it's likely our raw_ptr type which has a test checking for exactly this and gets used a lot.

Ack, thanks, the link is super useful. FYI we voted trivial relocation into the working draft for C++26 this morning, so this should be coming sooner rather than later.

What we voted in is not P1144 which you seemed to be expecting, though, it's http://wg21.link/P2786. There are some differences, so you should make sure that your type is compatible with it.