Skip to content

Commit 7237470

Browse files
danakjdanakj
committed
Check for aliasing in swap and clone_from impls
1 parent 8c48bc9 commit 7237470

File tree

6 files changed

+146
-25
lines changed

6 files changed

+146
-25
lines changed

‎subspace/containers/array.h

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -146,12 +146,14 @@ class Array final {
146146
return ar;
147147
}
148148

149-
constexpr void clone_from(const Array& other) & noexcept
149+
constexpr void clone_from(const Array& source) & noexcept
150150
requires(::sus::mem::Clone<T>)
151151
{
152+
if (&source == this) [[unlikely]]
153+
return;
152154
for (auto i = size_t{0}; i < N; ++i) {
153155
::sus::clone_into(mref(get_unchecked_mut(::sus::marker::unsafe_fn, i)),
154-
other.get_unchecked(::sus::marker::unsafe_fn, i));
156+
source.get_unchecked(::sus::marker::unsafe_fn, i));
155157
}
156158
}
157159

‎subspace/containers/vec.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,8 @@ class Vec {
157157
{
158158
check(!is_moved_from());
159159
check(!source.is_moved_from());
160+
if (&source == this) [[unlikely]]
161+
return;
160162
if (source.capacity_ == 0_usize) {
161163
destroy_storage_objects();
162164
free_storage();

‎subspace/mem/swap.h

Lines changed: 40 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
#include <type_traits>
2020

21+
#include "subspace/marker/unsafe.h"
2122
#include "subspace/mem/addressof.h"
2223
#include "subspace/mem/move.h"
2324
#include "subspace/mem/mref.h"
@@ -26,21 +27,54 @@
2627

2728
namespace sus::mem {
2829

30+
/// Swaps the objects `lhs` and `rhs`.
31+
///
32+
/// If both inputs point to the same object, no swap takes place, so no move
33+
/// constructor/operator is called.
2934
template <class T>
3035
requires(sus::mem::Move<T>)
3136
constexpr void swap(T& lhs, T& rhs) noexcept {
37+
if constexpr (::sus::mem::relocate_by_memcpy<T>) {
38+
// memcpy() and memmove() are not constexpr so we can't use them in
39+
// constexpr evaluation.
40+
if (!std::is_constant_evaluated()) {
41+
constexpr auto data_size = ::sus::mem::data_size_of<T>();
42+
char temp[data_size];
43+
memcpy(temp, ::sus::mem::addressof(lhs), data_size);
44+
memmove(::sus::mem::addressof(lhs), ::sus::mem::addressof(rhs),
45+
data_size);
46+
memcpy(::sus::mem::addressof(rhs), temp, data_size);
47+
return;
48+
}
49+
}
50+
if (::sus::mem::addressof(lhs) != ::sus::mem::addressof(rhs)) [[likely]] {
51+
auto temp = T(::sus::move(lhs));
52+
lhs = ::sus::move(rhs);
53+
rhs = ::sus::move(temp);
54+
}
55+
}
56+
57+
/// Swaps the objects `lhs` and `rhs`.
58+
///
59+
/// # Safety
60+
/// The inputs must not both refer to the same object, or Undefined Behaviour
61+
/// may result.
62+
template <class T>
63+
requires(sus::mem::Move<T>)
64+
constexpr void swap_no_alias_unchecked(::sus::marker::UnsafeFnMarker, T& lhs,
65+
T& rhs) noexcept {
3266
if constexpr (::sus::mem::relocate_by_memcpy<T>) {
3367
// memcpy() is not constexpr so we can't use it in constexpr evaluation.
3468
if (!std::is_constant_evaluated()) {
35-
char temp[::sus::mem::data_size_of<T>()];
36-
memcpy(temp, ::sus::mem::addressof(lhs), ::sus::mem::data_size_of<T>());
37-
memcpy(::sus::mem::addressof(lhs), ::sus::mem::addressof(rhs),
38-
::sus::mem::data_size_of<T>());
39-
memcpy(::sus::mem::addressof(rhs), temp, ::sus::mem::data_size_of<T>());
69+
constexpr auto data_size = ::sus::mem::data_size_of<T>();
70+
char temp[data_size];
71+
memcpy(temp, ::sus::mem::addressof(lhs), data_size);
72+
memcpy(::sus::mem::addressof(lhs), ::sus::mem::addressof(rhs), data_size);
73+
memcpy(::sus::mem::addressof(rhs), temp, data_size);
4074
return;
4175
}
4276
}
43-
T temp(::sus::move(lhs));
77+
auto temp = T(::sus::move(lhs));
4478
lhs = ::sus::move(rhs);
4579
rhs = ::sus::move(temp);
4680
}

‎subspace/mem/swap_unittest.cc

Lines changed: 96 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -17,26 +17,28 @@
1717
#include <type_traits>
1818

1919
#include "googletest/include/gtest/gtest.h"
20+
#include "subspace/containers/array.h"
2021
#include "subspace/macros/builtin.h"
2122
#include "subspace/mem/relocate.h"
23+
#include "subspace/ops/range_literals.h"
24+
#include "subspace/prelude.h"
2225

23-
namespace sus::mem {
2426
namespace {
2527

2628
TEST(Swap, ConstexprTrivialRelocate) {
2729
using T = int;
28-
static_assert(relocate_by_memcpy<T>, "");
30+
static_assert(sus::mem::relocate_by_memcpy<T>, "");
2931

3032
auto i = []() constexpr {
3133
T i(2);
3234
T j(5);
33-
::sus::mem::swap(mref(i), mref(j));
35+
sus::mem::swap(mref(i), mref(j));
3436
return i;
3537
};
3638
auto j = []() constexpr {
3739
T i(2);
3840
T j(5);
39-
::sus::mem::swap(mref(i), mref(j));
41+
sus::mem::swap(mref(i), mref(j));
4042
return j;
4143
};
4244
static_assert(i() == T(5), "");
@@ -55,18 +57,19 @@ TEST(Swap, ConstexprTrivialAbi) {
5557
// [[sus_trivial_abi]].
5658
static_assert(!std::is_trivially_move_constructible_v<S>, "");
5759
static_assert(
58-
relocate_by_memcpy<S> == __has_extension(trivially_relocatable), "");
60+
sus::mem::relocate_by_memcpy<S> == __has_extension(trivially_relocatable),
61+
"");
5962

6063
auto i = []() constexpr {
6164
S i(2);
6265
S j(5);
63-
::sus::mem::swap(mref(i), mref(j));
66+
sus::mem::swap(mref(i), mref(j));
6467
return i;
6568
};
6669
auto j = []() constexpr {
6770
S i(2);
6871
S j(5);
69-
::sus::mem::swap(mref(i), mref(j));
72+
sus::mem::swap(mref(i), mref(j));
7073
return j;
7174
};
7275
static_assert(i().num == 5, "");
@@ -84,18 +87,18 @@ TEST(Swap, ConstexprNonTrivial) {
8487
int num;
8588
int moves = 0;
8689
};
87-
static_assert(!relocate_by_memcpy<S>, "");
90+
static_assert(!sus::mem::relocate_by_memcpy<S>, "");
8891

8992
auto i = []() constexpr {
9093
S i(2);
9194
S j(5);
92-
::sus::mem::swap(mref(i), mref(j));
95+
sus::mem::swap(mref(i), mref(j));
9396
return i;
9497
};
9598
auto j = []() constexpr {
9699
S i(2);
97100
S j(5);
98-
::sus::mem::swap(mref(i), mref(j));
101+
sus::mem::swap(mref(i), mref(j));
99102
return j;
100103
};
101104
static_assert(i().num == 5, "");
@@ -107,11 +110,11 @@ TEST(Swap, ConstexprNonTrivial) {
107110

108111
TEST(Swap, TrivialRelocate) {
109112
using T = int;
110-
static_assert(relocate_by_memcpy<T>, "");
113+
static_assert(sus::mem::relocate_by_memcpy<T>, "");
111114

112115
T i(2);
113116
T j(5);
114-
::sus::mem::swap(mref(i), mref(j));
117+
sus::mem::swap(mref(i), mref(j));
115118
EXPECT_EQ(i, T(5));
116119
EXPECT_EQ(j, T(2));
117120
}
@@ -131,11 +134,12 @@ TEST(Swap, TrivialAbi) {
131134
// [[sus_trivial_abi]].
132135
static_assert(!std::is_trivially_move_constructible_v<S>, "");
133136
static_assert(
134-
relocate_by_memcpy<S> == __has_extension(trivially_relocatable), "");
137+
sus::mem::relocate_by_memcpy<S> == __has_extension(trivially_relocatable),
138+
"");
135139

136140
S i(2);
137141
S j(5);
138-
::sus::mem::swap(mref(i), mref(j));
142+
sus::mem::swap(mref(i), mref(j));
139143
EXPECT_EQ(i.num, 5);
140144
EXPECT_EQ(j.num, 2);
141145
#if __has_extension(trivially_relocatable)
@@ -158,17 +162,92 @@ TEST(Swap, NonTrivial) {
158162
int num;
159163
int moves = 0;
160164
};
161-
static_assert(!relocate_by_memcpy<S>, "");
165+
static_assert(!sus::mem::relocate_by_memcpy<S>, "");
162166

163167
S i(2);
164168
S j(5);
165-
::sus::mem::swap(mref(i), mref(j));
169+
sus::mem::swap(mref(i), mref(j));
166170
EXPECT_EQ(i.num, 5);
167171
EXPECT_EQ(j.num, 2);
168172
// The swap was done by move operations.
169173
EXPECT_GE(i.moves, 1);
170174
EXPECT_GE(j.moves, 1);
171175
}
172176

177+
struct Trivial {
178+
explicit Trivial(sus::Array<i32, 100> i) : num(sus::move(i)) {}
179+
Trivial(Trivial&&) { moves += 1u; }
180+
Trivial& operator=(Trivial&&) {
181+
moves += 1u;
182+
return *this;
183+
}
184+
sus::Array<i32, 100> num;
185+
186+
static usize moves;
187+
188+
sus_class_trivially_relocatable_unchecked(unsafe_fn);
189+
};
190+
191+
usize Trivial::moves;
192+
193+
TEST(Swap, Alias) {
194+
static usize moves;
195+
struct S {
196+
explicit S(i32 i) : num(i) {}
197+
S(S&&) { moves += 1u; }
198+
S& operator=(S&&) {
199+
moves += 1u;
200+
return *this;
201+
}
202+
i32 num;
203+
};
204+
205+
S i(2);
206+
sus::mem::swap(mref(i), mref(i));
207+
EXPECT_EQ(moves, 0u);
208+
209+
Trivial::moves = 0u;
210+
Trivial t(sus::Array<i32, 100>::with_initializer(
211+
[i = 0_i32]() mutable { return sus::mem::replace(i, i + 1); }));
212+
sus::mem::swap(mref(t), mref(t));
213+
for (usize j : "0..100"_r) {
214+
EXPECT_EQ(t.num[j], i32::from(j));
215+
}
216+
EXPECT_EQ(Trivial::moves, 0u);
217+
}
218+
219+
TEST(Swap, NoAliasUnchecked) {
220+
static usize moves;
221+
struct S {
222+
explicit S(i32 i) : num(i) {}
223+
S(S&&) { moves += 1u; }
224+
S& operator=(S&&) {
225+
moves += 1u;
226+
return *this;
227+
}
228+
i32 num;
229+
};
230+
231+
S i(2);
232+
sus::mem::swap_no_alias_unchecked(unsafe_fn, mref(i), mref(i));
233+
// Luckily S doesn't do anything bad when assigned to itself, and we can
234+
// verify the number of times a move happened.
235+
EXPECT_EQ(moves, 3u);
236+
237+
Trivial::moves = 0u;
238+
Trivial t1(sus::Array<i32, 100>::with_initializer(
239+
[i = 0_i32]() mutable { return sus::mem::replace(i, i + 1); }));
240+
Trivial t2(sus::Array<i32, 100>::with_initializer(
241+
[i = 10_i32]() mutable { return sus::mem::replace(i, i + 1); }));
242+
sus::mem::swap_no_alias_unchecked(unsafe_fn, mref(t1), mref(t2));
243+
for (usize j : "0..100"_r) {
244+
EXPECT_EQ(t1.num[j], i32::from(j) + 10);
245+
EXPECT_EQ(t2.num[j], i32::from(j));
246+
}
247+
EXPECT_EQ(Trivial::moves, 0u);
248+
249+
sus::mem::swap_no_alias_unchecked(unsafe_fn, mref(t1), mref(t1));
250+
// As memcpy() was called in an invalid way we can't really check anything
251+
// here, but we can see we didn't check() anything.
252+
}
173253
} // namespace
174-
} // namespace sus::mem

‎subspace/option/option.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -278,6 +278,8 @@ class Option final {
278278
void clone_from(const Option& source) & noexcept
279279
requires(::sus::mem::Clone<T> && !::sus::mem::CopyOrRef<T>)
280280
{
281+
if (&source == this) [[unlikely]]
282+
return;
281283
if (t_.state() == Some && source.t_.state() == Some) {
282284
::sus::clone_into(mref(t_.val_mut()), source.t_.val());
283285
} else {

‎subspace/result/result.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -388,6 +388,8 @@ class [[nodiscard]] Result final {
388388
!(::sus::mem::Copy<T> && ::sus::mem::Copy<E>))
389389
{
390390
::sus::check(source.state_ != __private::ResultState::IsMoved);
391+
if (&source == this) [[unlikely]]
392+
return;
391393
if (state_ == source.state_) {
392394
switch (state_) {
393395
case __private::ResultState::IsOk:

0 commit comments

Comments
 (0)