[libcxx-commits] [libcxx] [libc++][ranges][abi-break] Fix `movable_box` overwriting memory of data that lives in the tail padding (PR #71314)
via libcxx-commits
libcxx-commits at lists.llvm.org
Mon Nov 6 05:26:01 PST 2023
https://github.com/huixie90 updated https://github.com/llvm/llvm-project/pull/71314
>From bc81245ad3c55560752b6bf4637e96be7db2e64c Mon Sep 17 00:00:00 2001
From: Hui <hui.xie0621 at gmail.com>
Date: Sun, 5 Nov 2023 10:48:04 +0000
Subject: [PATCH 1/2] [libc++][ranges][abi-break] Fix `movable_box` overwriting
memory of data that lives in the tail padding
---
libcxx/include/__ranges/movable_box.h | 45 ++++++++++++++-----
.../no_unique_address.pass.cpp | 14 ++++++
.../range.transform/general.pass.cpp | 17 +++++++
3 files changed, 64 insertions(+), 12 deletions(-)
diff --git a/libcxx/include/__ranges/movable_box.h b/libcxx/include/__ranges/movable_box.h
index 6615533d374349e..13e28954fd862f9 100644
--- a/libcxx/include/__ranges/movable_box.h
+++ b/libcxx/include/__ranges/movable_box.h
@@ -16,6 +16,7 @@
#include <__config>
#include <__memory/addressof.h>
#include <__memory/construct_at.h>
+#include <__type_traits/is_empty.h>
#include <__type_traits/is_nothrow_constructible.h>
#include <__type_traits/is_nothrow_copy_constructible.h>
#include <__type_traits/is_nothrow_default_constructible.h>
@@ -146,21 +147,41 @@ template <class _Tp>
concept __doesnt_need_empty_state = __doesnt_need_empty_state_for_copy<_Tp> && __doesnt_need_empty_state_for_move<_Tp>;
# endif
+template <class _Tp, bool _NoUniqueAddress>
+struct __no_unique_address_if {
+ _Tp __val_;
+
+ template <class... _Args>
+ requires is_constructible_v<_Tp, _Args&&...>
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit __no_unique_address_if(_Args&&... __args)
+ : __val_(std::forward<_Args>(__args)...) {}
+};
+
+template <class _Tp>
+struct __no_unique_address_if<_Tp, true> {
+ _LIBCPP_NO_UNIQUE_ADDRESS _Tp __val_;
+
+ template <class... _Args>
+ requires is_constructible_v<_Tp, _Args&&...>
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit __no_unique_address_if(_Args&&... __args)
+ : __val_(std::forward<_Args>(__args)...) {}
+};
+
template <__movable_box_object _Tp>
requires __doesnt_need_empty_state<_Tp>
-class __movable_box<_Tp> {
- _LIBCPP_NO_UNIQUE_ADDRESS _Tp __val_;
+class __movable_box<_Tp> : private __no_unique_address_if<_Tp, is_empty_v<_Tp>> {
+ using __base = __no_unique_address_if<_Tp, is_empty_v<_Tp>>;
public:
template <class... _Args>
requires is_constructible_v<_Tp, _Args...>
_LIBCPP_HIDE_FROM_ABI constexpr explicit __movable_box(in_place_t, _Args&&... __args) noexcept(
is_nothrow_constructible_v<_Tp, _Args...>)
- : __val_(std::forward<_Args>(__args)...) {}
+ : __base(std::forward<_Args>(__args)...) {}
_LIBCPP_HIDE_FROM_ABI constexpr __movable_box() noexcept(is_nothrow_default_constructible_v<_Tp>)
requires default_initializable<_Tp>
- : __val_() {}
+ : __base() {}
_LIBCPP_HIDE_FROM_ABI __movable_box(__movable_box const&) = default;
_LIBCPP_HIDE_FROM_ABI __movable_box(__movable_box&&) = default;
@@ -177,8 +198,8 @@ class __movable_box<_Tp> {
_LIBCPP_HIDE_FROM_ABI constexpr __movable_box& operator=(__movable_box const& __other) noexcept {
static_assert(is_nothrow_copy_constructible_v<_Tp>);
if (this != std::addressof(__other)) {
- std::destroy_at(std::addressof(__val_));
- std::construct_at(std::addressof(__val_), __other.__val_);
+ std::destroy_at(std::addressof(this->__val_));
+ std::construct_at(std::addressof(this->__val_), __other.__val_);
}
return *this;
}
@@ -186,17 +207,17 @@ class __movable_box<_Tp> {
_LIBCPP_HIDE_FROM_ABI constexpr __movable_box& operator=(__movable_box&& __other) noexcept {
static_assert(is_nothrow_move_constructible_v<_Tp>);
if (this != std::addressof(__other)) {
- std::destroy_at(std::addressof(__val_));
- std::construct_at(std::addressof(__val_), std::move(__other.__val_));
+ std::destroy_at(std::addressof(this->__val_));
+ std::construct_at(std::addressof(this->__val_), std::move(__other.__val_));
}
return *this;
}
- _LIBCPP_HIDE_FROM_ABI constexpr _Tp const& operator*() const noexcept { return __val_; }
- _LIBCPP_HIDE_FROM_ABI constexpr _Tp& operator*() noexcept { return __val_; }
+ _LIBCPP_HIDE_FROM_ABI constexpr _Tp const& operator*() const noexcept { return this->__val_; }
+ _LIBCPP_HIDE_FROM_ABI constexpr _Tp& operator*() noexcept { return this->__val_; }
- _LIBCPP_HIDE_FROM_ABI constexpr const _Tp* operator->() const noexcept { return std::addressof(__val_); }
- _LIBCPP_HIDE_FROM_ABI constexpr _Tp* operator->() noexcept { return std::addressof(__val_); }
+ _LIBCPP_HIDE_FROM_ABI constexpr const _Tp* operator->() const noexcept { return std::addressof(this->__val_); }
+ _LIBCPP_HIDE_FROM_ABI constexpr _Tp* operator->() noexcept { return std::addressof(this->__val_); }
_LIBCPP_HIDE_FROM_ABI constexpr bool __has_value() const noexcept { return true; }
};
diff --git a/libcxx/test/libcxx/ranges/range.adaptors/range.move.wrap/no_unique_address.pass.cpp b/libcxx/test/libcxx/ranges/range.adaptors/range.move.wrap/no_unique_address.pass.cpp
index e21191f98dea3e8..6a33dab96280afa 100644
--- a/libcxx/test/libcxx/ranges/range.adaptors/range.move.wrap/no_unique_address.pass.cpp
+++ b/libcxx/test/libcxx/ranges/range.adaptors/range.move.wrap/no_unique_address.pass.cpp
@@ -15,6 +15,20 @@
#include <cassert>
#include <utility>
+// movable_box should not use no_unique_address internally if T is not empty
+struct WithPadding {
+ WithPadding() {}
+ int i;
+ bool b;
+};
+
+struct Test {
+ [[no_unique_address]] std::ranges::__movable_box<WithPadding> box_;
+ bool b2;
+};
+
+static_assert(sizeof(Test) > sizeof(std::ranges::__movable_box<WithPadding>));
+
bool copied = false;
bool moved = false;
diff --git a/libcxx/test/std/ranges/range.adaptors/range.transform/general.pass.cpp b/libcxx/test/std/ranges/range.adaptors/range.transform/general.pass.cpp
index 32fed6f8caa7d02..cfe394e89183238 100644
--- a/libcxx/test/std/ranges/range.adaptors/range.transform/general.pass.cpp
+++ b/libcxx/test/std/ranges/range.adaptors/range.transform/general.pass.cpp
@@ -108,5 +108,22 @@ int main(int, char**) {
}
#endif
+ // GH issue #70506
+ // movable_box::operator= overwrites underlying view
+ {
+ auto f = [l = 0.0L, b = false](int i) {
+ (void)l;
+ (void)b;
+ return i;
+ };
+
+ auto v1 = std::vector{1, 2, 3, 4} | std::views::transform(f);
+ auto v2 = std::vector{1, 2, 3, 4} | std::views::transform(f);
+
+ v1 = std::move(v2);
+ int expected[] = {1, 2, 3, 4};
+ assert(std::equal(v1.begin(), v1.end(), expected, expected + 4));
+ }
+
return 0;
}
>From 0224c1782eb2c4a768ba02150b0da7e3e6735779 Mon Sep 17 00:00:00 2001
From: Hui <hui.xie0621 at gmail.com>
Date: Mon, 6 Nov 2023 13:25:38 +0000
Subject: [PATCH 2/2] only apply no_unique_address when we don't need to supply
assignment operator
---
libcxx/include/__ranges/movable_box.h | 28 ++++--
.../no_unique_address.pass.cpp | 89 ++++++++++---------
2 files changed, 68 insertions(+), 49 deletions(-)
diff --git a/libcxx/include/__ranges/movable_box.h b/libcxx/include/__ranges/movable_box.h
index 13e28954fd862f9..a03cd70fa09ed92 100644
--- a/libcxx/include/__ranges/movable_box.h
+++ b/libcxx/include/__ranges/movable_box.h
@@ -16,7 +16,6 @@
#include <__config>
#include <__memory/addressof.h>
#include <__memory/construct_at.h>
-#include <__type_traits/is_empty.h>
#include <__type_traits/is_nothrow_constructible.h>
#include <__type_traits/is_nothrow_copy_constructible.h>
#include <__type_traits/is_nothrow_default_constructible.h>
@@ -135,6 +134,13 @@ concept __doesnt_need_empty_state =
// 2. Otherwise, movable-box<T> should store only a T if either T models movable or
// is_nothrow_move_constructible_v<T> is true.
: movable<_Tp> || is_nothrow_move_constructible_v<_Tp>);
+
+// we can only use no_unique_address if _Tp has assignment operators,
+// so that we don't need to add our own assignment operator, which
+// contains problematic construct_at
+template <class _Tp>
+concept __can_use_no_unique_address = (copy_constructible<_Tp> ? copyable<_Tp> : movable<_Tp>);
+
# else
template <class _Tp>
@@ -145,32 +151,36 @@ concept __doesnt_need_empty_state_for_move = movable<_Tp> || is_nothrow_move_con
template <class _Tp>
concept __doesnt_need_empty_state = __doesnt_need_empty_state_for_copy<_Tp> && __doesnt_need_empty_state_for_move<_Tp>;
+
+template <class _Tp>
+concept __can_use_no_unique_address = copyable<_Tp>;
# endif
-template <class _Tp, bool _NoUniqueAddress>
-struct __no_unique_address_if {
+template <class _Tp>
+struct __movable_box_base {
_Tp __val_;
template <class... _Args>
requires is_constructible_v<_Tp, _Args&&...>
- _LIBCPP_HIDE_FROM_ABI constexpr explicit __no_unique_address_if(_Args&&... __args)
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit __movable_box_base(_Args&&... __args)
: __val_(std::forward<_Args>(__args)...) {}
};
template <class _Tp>
-struct __no_unique_address_if<_Tp, true> {
+ requires __can_use_no_unique_address<_Tp>
+struct __movable_box_base<_Tp> {
_LIBCPP_NO_UNIQUE_ADDRESS _Tp __val_;
template <class... _Args>
requires is_constructible_v<_Tp, _Args&&...>
- _LIBCPP_HIDE_FROM_ABI constexpr explicit __no_unique_address_if(_Args&&... __args)
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit __movable_box_base(_Args&&... __args)
: __val_(std::forward<_Args>(__args)...) {}
};
template <__movable_box_object _Tp>
requires __doesnt_need_empty_state<_Tp>
-class __movable_box<_Tp> : private __no_unique_address_if<_Tp, is_empty_v<_Tp>> {
- using __base = __no_unique_address_if<_Tp, is_empty_v<_Tp>>;
+class __movable_box<_Tp> : private __movable_box_base<_Tp> {
+ using __base = __movable_box_base<_Tp>;
public:
template <class... _Args>
@@ -197,6 +207,7 @@ class __movable_box<_Tp> : private __no_unique_address_if<_Tp, is_empty_v<_Tp>>
// Implementation of assignment operators in case we perform optimization (2)
_LIBCPP_HIDE_FROM_ABI constexpr __movable_box& operator=(__movable_box const& __other) noexcept {
static_assert(is_nothrow_copy_constructible_v<_Tp>);
+ static_assert(!__can_use_no_unique_address<_Tp>);
if (this != std::addressof(__other)) {
std::destroy_at(std::addressof(this->__val_));
std::construct_at(std::addressof(this->__val_), __other.__val_);
@@ -206,6 +217,7 @@ class __movable_box<_Tp> : private __no_unique_address_if<_Tp, is_empty_v<_Tp>>
_LIBCPP_HIDE_FROM_ABI constexpr __movable_box& operator=(__movable_box&& __other) noexcept {
static_assert(is_nothrow_move_constructible_v<_Tp>);
+ static_assert(!__can_use_no_unique_address<_Tp>);
if (this != std::addressof(__other)) {
std::destroy_at(std::addressof(this->__val_));
std::construct_at(std::addressof(this->__val_), std::move(__other.__val_));
diff --git a/libcxx/test/libcxx/ranges/range.adaptors/range.move.wrap/no_unique_address.pass.cpp b/libcxx/test/libcxx/ranges/range.adaptors/range.move.wrap/no_unique_address.pass.cpp
index 6a33dab96280afa..c425a67351bfb6e 100644
--- a/libcxx/test/libcxx/ranges/range.adaptors/range.move.wrap/no_unique_address.pass.cpp
+++ b/libcxx/test/libcxx/ranges/range.adaptors/range.move.wrap/no_unique_address.pass.cpp
@@ -15,56 +15,63 @@
#include <cassert>
#include <utility>
-// movable_box should not use no_unique_address internally if T is not empty
-struct WithPadding {
- WithPadding() {}
- int i;
- bool b;
-};
-
-struct Test {
- [[no_unique_address]] std::ranges::__movable_box<WithPadding> box_;
- bool b2;
-};
-
-static_assert(sizeof(Test) > sizeof(std::ranges::__movable_box<WithPadding>));
+#include "test_macros.h"
+
+template <class T, bool ExpectNoUniqueAddress>
+void test_no_unique_address() {
+ struct Test {
+ [[no_unique_address]] std::ranges::__movable_box<T> box_;
+ bool b2;
+ };
+
+ if constexpr (ExpectNoUniqueAddress) {
+ static_assert(sizeof(Test) == sizeof(bool));
+ } else {
+ static_assert(sizeof(Test) > sizeof(bool));
+ }
+}
-bool copied = false;
-bool moved = false;
+struct Copyable {};
-struct Empty {
- Empty() noexcept {}
- Empty(Empty const&) noexcept { copied = true; }
- Empty(Empty&&) noexcept { moved = true; }
- Empty& operator=(Empty const&) = delete;
- Empty& operator=(Empty&&) = delete;
+struct NotCopyAssignable {
+ constexpr NotCopyAssignable() = default;
+ constexpr NotCopyAssignable(const NotCopyAssignable&) = default;
+ NotCopyAssignable& operator=(const NotCopyAssignable&) = delete;
};
-using Box = std::ranges::__movable_box<Empty>;
+struct NotMoveAssignable {
+ constexpr NotMoveAssignable() = default;
+ constexpr NotMoveAssignable(const NotMoveAssignable&) = default;
+ NotMoveAssignable& operator=(const NotMoveAssignable&) = default;
+ constexpr NotMoveAssignable(NotMoveAssignable&&) = default;
+ NotMoveAssignable& operator=(NotMoveAssignable&&) = delete;
+};
-struct Inherit : Box {};
+struct MoveOnly {
+ constexpr MoveOnly() = default;
+ constexpr MoveOnly(const MoveOnly&) = delete;
+ MoveOnly& operator=(const MoveOnly&) = delete;
+ constexpr MoveOnly(MoveOnly&&) = default;
+ MoveOnly& operator=(MoveOnly&&) = default;
+};
-struct Hold : Box {
- [[no_unique_address]] Inherit member;
+struct MoveOnlyNotAssignable {
+ constexpr MoveOnlyNotAssignable() = default;
+ constexpr MoveOnlyNotAssignable(const MoveOnlyNotAssignable&) = delete;
+ MoveOnlyNotAssignable& operator=(const MoveOnlyNotAssignable&) = delete;
+ constexpr MoveOnlyNotAssignable(MoveOnlyNotAssignable&&) = default;
+ MoveOnlyNotAssignable& operator=(MoveOnlyNotAssignable&&) = delete;
};
int main(int, char**) {
- Hold box;
-
- Box& base = static_cast<Box&>(box);
- Box& member = static_cast<Box&>(box.member);
-
- // Despite [[no_unique_address]], the two objects have the same type so they
- // can't share the same address.
- assert(&base != &member);
-
- // Make sure that we do perform the copy-construction, which wouldn't be the
- // case if the two <copyable-box>s had the same address.
- base = member;
- assert(copied);
-
- base = std::move(member);
- assert(moved);
+ test_no_unique_address<Copyable, true>();
+ test_no_unique_address<NotCopyAssignable, false>();
+ test_no_unique_address<NotMoveAssignable, false>();
+
+#if TEST_STD_VER >= 23
+ test_no_unique_address<MoveOnly, true>();
+ test_no_unique_address<MoveOnlyNotAssignable, false>();
+#endif
return 0;
}
More information about the libcxx-commits
mailing list