Skip to content

Commit f92369f

Browse files
ldionnetstellar
authored andcommitted
[libc++] Replace __is_trivially_relocatable by is_trivially_copyable (llvm#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 accfbd4)
1 parent 898089b commit f92369f

File tree

3 files changed

+86
-7
lines changed

3 files changed

+86
-7
lines changed

libcxx/include/__type_traits/is_trivially_relocatable.h

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@
1111

1212
#include <__config>
1313
#include <__type_traits/enable_if.h>
14-
#include <__type_traits/integral_constant.h>
1514
#include <__type_traits/is_same.h>
1615
#include <__type_traits/is_trivially_copyable.h>
1716

@@ -23,8 +22,11 @@ _LIBCPP_BEGIN_NAMESPACE_STD
2322

2423
// A type is trivially relocatable if a move construct + destroy of the original object is equivalent to
2524
// `memcpy(dst, src, sizeof(T))`.
26-
27-
#if __has_builtin(__is_trivially_relocatable)
25+
//
26+
// Note that we don't use the __is_trivially_relocatable Clang builtin right now because it does not
27+
// implement the semantics of any current or future trivial relocation proposal and it can lead to
28+
// incorrect optimizations on some platforms (Windows) and supported compilers (AppleClang).
29+
#if __has_builtin(__is_trivially_relocatable) && 0
2830
template <class _Tp, class = void>
2931
struct __libcpp_is_trivially_relocatable : integral_constant<bool, __is_trivially_relocatable(_Tp)> {};
3032
#else

libcxx/test/libcxx/type_traits/is_trivially_relocatable.compile.pass.cpp

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -54,11 +54,17 @@ struct MoveOnlyTriviallyCopyable {
5454
MoveOnlyTriviallyCopyable(MoveOnlyTriviallyCopyable&&) = default;
5555
MoveOnlyTriviallyCopyable& operator=(MoveOnlyTriviallyCopyable&&) = default;
5656
};
57-
#ifndef _MSC_VER
5857
static_assert(std::__libcpp_is_trivially_relocatable<MoveOnlyTriviallyCopyable>::value, "");
59-
#else
60-
static_assert(!std::__libcpp_is_trivially_relocatable<MoveOnlyTriviallyCopyable>::value, "");
61-
#endif
58+
59+
struct NonTrivialMoveConstructor {
60+
NonTrivialMoveConstructor(NonTrivialMoveConstructor&&);
61+
};
62+
static_assert(!std::__libcpp_is_trivially_relocatable<NonTrivialMoveConstructor>::value, "");
63+
64+
struct NonTrivialDestructor {
65+
~NonTrivialDestructor() {}
66+
};
67+
static_assert(!std::__libcpp_is_trivially_relocatable<NonTrivialDestructor>::value, "");
6268

6369
// library-internal types
6470
// ----------------------
Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
//===----------------------------------------------------------------------===//
2+
//
3+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
4+
// See https://llvm.org/LICENSE.txt for license information.
5+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
6+
//
7+
//===----------------------------------------------------------------------===//
8+
9+
// <vector>
10+
11+
// Make sure we don't miscompile vector operations for types that shouldn't be considered
12+
// trivially relocatable.
13+
14+
#include <vector>
15+
#include <cassert>
16+
#include <cstddef>
17+
#include <type_traits>
18+
#include <utility>
19+
20+
#include "test_macros.h"
21+
22+
struct Tracker {
23+
std::size_t move_constructs = 0;
24+
};
25+
26+
struct [[clang::trivial_abi]] Inner {
27+
TEST_CONSTEXPR_CXX20 explicit Inner(Tracker* tracker) : tracker_(tracker) {}
28+
TEST_CONSTEXPR_CXX20 Inner(const Inner& rhs) : tracker_(rhs.tracker_) { tracker_->move_constructs += 1; }
29+
TEST_CONSTEXPR_CXX20 Inner(Inner&& rhs) : tracker_(rhs.tracker_) { tracker_->move_constructs += 1; }
30+
Tracker* tracker_;
31+
};
32+
33+
// Even though this type contains a trivial_abi type, it is not trivially move-constructible,
34+
// so we should not attempt to optimize its move construction + destroy using trivial relocation.
35+
struct NotTriviallyMovable {
36+
TEST_CONSTEXPR_CXX20 explicit NotTriviallyMovable(Tracker* tracker) : inner_(tracker) {}
37+
TEST_CONSTEXPR_CXX20 NotTriviallyMovable(NotTriviallyMovable&& other) : inner_(std::move(other.inner_)) {}
38+
Inner inner_;
39+
};
40+
static_assert(!std::is_trivially_copyable<NotTriviallyMovable>::value, "");
41+
LIBCPP_STATIC_ASSERT(!std::__libcpp_is_trivially_relocatable<NotTriviallyMovable>::value, "");
42+
43+
TEST_CONSTEXPR_CXX20 bool tests() {
44+
Tracker track;
45+
std::vector<NotTriviallyMovable> v;
46+
47+
// Fill the vector at its capacity, such that any subsequent push_back would require growing.
48+
v.reserve(5);
49+
std::size_t const capacity = v.capacity(); // could technically be more than 5
50+
while (v.size() < v.capacity()) {
51+
v.emplace_back(&track);
52+
}
53+
assert(track.move_constructs == 0);
54+
assert(v.capacity() == capacity);
55+
assert(v.size() == capacity);
56+
57+
// Force a reallocation of the buffer + relocalization of the elements.
58+
// All the existing elements of the vector should be move-constructed to their new location.
59+
v.emplace_back(&track);
60+
assert(track.move_constructs == capacity);
61+
62+
return true;
63+
}
64+
65+
int main(int, char**) {
66+
tests();
67+
#if TEST_STD_VER >= 20
68+
static_assert(tests());
69+
#endif
70+
return 0;
71+
}

0 commit comments

Comments
 (0)