[libcxx-commits] [libcxx] [libc++] Replace __is_trivially_relocatable by is_trivially_copyable (PR #124970)
Louis Dionne via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Feb 5 10:58:46 PST 2025
https://github.com/ldionne updated https://github.com/llvm/llvm-project/pull/124970
>From 6da2fc4d827722f0adc5e445232d62fb191d3a61 Mon Sep 17 00:00:00 2001
From: Louis Dionne <ldionne.2 at gmail.com>
Date: Wed, 29 Jan 2025 13:18:11 -0500
Subject: [PATCH 1/7] [libc++] Replace __is_trivially_relocatable by
is_trivially_copyable
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.
---
.../__type_traits/is_trivially_relocatable.h | 7 --
.../is_trivially_relocatable.compile.pass.cpp | 10 +++
.../vector/trivial_relocation.pass.cpp | 67 +++++++++++++++++++
3 files changed, 77 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 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;
+}
>From 798bb8067126208fc665d23a0d1b46f5820c9e59 Mon Sep 17 00:00:00 2001
From: Louis Dionne <ldionne.2 at gmail.com>
Date: Tue, 4 Feb 2025 14:41:28 -0500
Subject: [PATCH 2/7] Use && 0 and add a comment
---
libcxx/include/__type_traits/is_trivially_relocatable.h | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/libcxx/include/__type_traits/is_trivially_relocatable.h b/libcxx/include/__type_traits/is_trivially_relocatable.h
index ecc97a41dfbdcb..9b0e240de55f4e 100644
--- a/libcxx/include/__type_traits/is_trivially_relocatable.h
+++ b/libcxx/include/__type_traits/is_trivially_relocatable.h
@@ -22,8 +22,17 @@ _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))`.
+//
+// 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
template <class _Tp, class = void>
struct __libcpp_is_trivially_relocatable : is_trivially_copyable<_Tp> {};
+#endif
template <class _Tp>
struct __libcpp_is_trivially_relocatable<_Tp,
>From 55eaf470a98736e6ac2cfa7eb68d3a812bb0d233 Mon Sep 17 00:00:00 2001
From: Louis Dionne <ldionne.2 at gmail.com>
Date: Tue, 4 Feb 2025 14:45:04 -0500
Subject: [PATCH 3/7] Make test portable
---
.../sequences/vector/trivial_relocation.pass.cpp | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/libcxx/test/std/containers/sequences/vector/trivial_relocation.pass.cpp b/libcxx/test/std/containers/sequences/vector/trivial_relocation.pass.cpp
index e109a5034032dd..d093ae8b877165 100644
--- a/libcxx/test/std/containers/sequences/vector/trivial_relocation.pass.cpp
+++ b/libcxx/test/std/containers/sequences/vector/trivial_relocation.pass.cpp
@@ -44,16 +44,18 @@ TEST_CONSTEXPR_CXX20 bool tests() {
// 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) {
+ 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.size() == 5);
+ 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 == 5);
+ assert(track.move_constructs == capacity);
return true;
}
>From dd56c13bc292c971b310e70e928bfa84db40c246 Mon Sep 17 00:00:00 2001
From: Louis Dionne <ldionne.2 at gmail.com>
Date: Tue, 4 Feb 2025 14:59:02 -0500
Subject: [PATCH 4/7] Missing include
---
.../std/containers/sequences/vector/trivial_relocation.pass.cpp | 2 ++
1 file changed, 2 insertions(+)
diff --git a/libcxx/test/std/containers/sequences/vector/trivial_relocation.pass.cpp b/libcxx/test/std/containers/sequences/vector/trivial_relocation.pass.cpp
index d093ae8b877165..3d775d3604630e 100644
--- a/libcxx/test/std/containers/sequences/vector/trivial_relocation.pass.cpp
+++ b/libcxx/test/std/containers/sequences/vector/trivial_relocation.pass.cpp
@@ -14,6 +14,8 @@
#include <vector>
#include <cassert>
#include <cstddef>
+#include <type_traits>
+#include <utility>
#include "test_macros.h"
>From f846c2afabe7cf3d3da286a35c946238cbfa906f Mon Sep 17 00:00:00 2001
From: Louis Dionne <ldionne.2 at gmail.com>
Date: Wed, 5 Feb 2025 08:56:03 -0500
Subject: [PATCH 5/7] Poke CI
>From 63ebc857854171d82637f3da0b9ccbfb3e9aee78 Mon Sep 17 00:00:00 2001
From: Louis Dionne <ldionne.2 at gmail.com>
Date: Wed, 5 Feb 2025 13:56:57 -0500
Subject: [PATCH 6/7] Adjust TEST_CONSTEXPR
---
.../sequences/vector/trivial_relocation.pass.cpp | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/libcxx/test/std/containers/sequences/vector/trivial_relocation.pass.cpp b/libcxx/test/std/containers/sequences/vector/trivial_relocation.pass.cpp
index 3d775d3604630e..fbd597d07d6e32 100644
--- a/libcxx/test/std/containers/sequences/vector/trivial_relocation.pass.cpp
+++ b/libcxx/test/std/containers/sequences/vector/trivial_relocation.pass.cpp
@@ -24,17 +24,17 @@ struct Tracker {
};
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; }
+ 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 explicit NotTriviallyMovable(Tracker* tracker) : inner_(tracker) {}
- TEST_CONSTEXPR NotTriviallyMovable(NotTriviallyMovable&& other) : inner_(std::move(other.inner_)) {}
+ 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, "");
>From d614f2e7531f14e197dbeadd023bbe2da6f33c9d Mon Sep 17 00:00:00 2001
From: Louis Dionne <ldionne.2 at gmail.com>
Date: Wed, 5 Feb 2025 13:58:31 -0500
Subject: [PATCH 7/7] Fix test on Windows
---
.../type_traits/is_trivially_relocatable.compile.pass.cpp | 4 ----
1 file changed, 4 deletions(-)
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 e43f38aa26f7d0..213d06d314a075 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,7 @@ 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&&);
More information about the libcxx-commits
mailing list