[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
Wed Dec 20 07:10:30 PST 2023
https://github.com/huixie90 updated https://github.com/llvm/llvm-project/pull/71314
>From 9302957bdf951eefc963f4475b6d534b8c26a2be 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/4] [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 6615533d374349..13e28954fd862f 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 e21191f98dea3e..6a33dab96280af 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 32fed6f8caa7d0..cfe394e8918323 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 1c1b6f099873449017fdb4ee3eaec17e85029177 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/4] 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 13e28954fd862f..a03cd70fa09ed9 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 6a33dab96280af..c425a67351bfb6 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;
}
>From 1557cde95c016b2439c6c2e1dfb921ecbe2a9dad Mon Sep 17 00:00:00 2001
From: Hui <hui.xie0621 at gmail.com>
Date: Mon, 6 Nov 2023 15:47:39 +0000
Subject: [PATCH 3/4] fix no_unique_address ignored in the tests on win
---
.../range.adaptors/range.move.wrap/no_unique_address.pass.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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 c425a67351bfb6..b7dee217e4945e 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
@@ -20,7 +20,7 @@
template <class T, bool ExpectNoUniqueAddress>
void test_no_unique_address() {
struct Test {
- [[no_unique_address]] std::ranges::__movable_box<T> box_;
+ _LIBCPP_NO_UNIQUE_ADDRESS std::ranges::__movable_box<T> box_;
bool b2;
};
>From 7a5be34b412cfcb9cf433d9dd711beff6b5ebf28 Mon Sep 17 00:00:00 2001
From: Hui <hui.xie0621 at gmail.com>
Date: Wed, 20 Dec 2023 15:10:01 +0000
Subject: [PATCH 4/4] address feedback and rebase
---
libcxx/docs/ReleaseNotes/18.rst | 4 ++
libcxx/include/__config | 12 ++++
libcxx/include/__ranges/chunk_by_view.h | 3 +-
libcxx/include/__ranges/drop_while_view.h | 3 +-
libcxx/include/__ranges/filter_view.h | 2 +-
libcxx/include/__ranges/movable_box.h | 49 +++++++-------
libcxx/include/__ranges/repeat_view.h | 4 +-
libcxx/include/__ranges/single_view.h | 4 +-
libcxx/include/__ranges/take_while_view.h | 3 +-
libcxx/include/__ranges/transform_view.h | 3 +-
.../no_unique_address.pass.cpp | 5 +-
.../no_unique_address.compile.pass.cpp | 21 ++++++
.../no_unique_address.compile.pass.cpp | 21 ++++++
..._robust_against_no_unique_address.pass.cpp | 64 +++++++++++++++++++
14 files changed, 166 insertions(+), 32 deletions(-)
create mode 100644 libcxx/test/libcxx/ranges/range.factories/range.repeat.view/no_unique_address.compile.pass.cpp
create mode 100644 libcxx/test/libcxx/ranges/range.factories/range.single.view/no_unique_address.compile.pass.cpp
create mode 100644 libcxx/test/std/ranges/range_robust_against_no_unique_address.pass.cpp
diff --git a/libcxx/docs/ReleaseNotes/18.rst b/libcxx/docs/ReleaseNotes/18.rst
index 79608c631f1e62..2a8f2a0fd07260 100644
--- a/libcxx/docs/ReleaseNotes/18.rst
+++ b/libcxx/docs/ReleaseNotes/18.rst
@@ -166,6 +166,10 @@ ABI Affecting Changes
to throw a different exception when attempting allocations that are too large
(``std::bad_alloc`` vs ``std::length_error``).
+- The ABI of some classes inside `std::ranges` that use `movable-box` have changed in order to fix a bug which could
+ result in overwriting user data followed by the `movable-box`. The affected views are `take_while_view`, `filter_view`,
+ `single_view`, `drop_while_view`, `repeat_view`, `transform_view`, `chunk_by_view`
+
Build System Changes
--------------------
diff --git a/libcxx/include/__config b/libcxx/include/__config
index adff13e714cb64..04abcb9cda8dda 100644
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -200,6 +200,18 @@
# define _LIBCPP_ABI_BAD_FUNCTION_CALL_KEY_FUNCTION
# endif
+# ifndef _LIBCPP_NO_ABI_TAG
+// We had some bugs where we use [[no_unique_address]] together with construct_at,
+// which causes UB as the call on construct_at could write to overlapping subobjects
+//
+// https://github.com/llvm/llvm-project/issues/70506
+// https://github.com/llvm/llvm-project/issues/70494
+//
+// To fix the bug we had to change the ABI of some classes to remove [[no_unique_address]] under certain conditions.
+// The below macro is used for all classes that have ABI changed as part of the bug fix
+# define _LIBCPP_ABI_2023_OVERLAPPING_SUBOBJECT_FIX_TAG __attribute__((__abi_tag__("2023_subobj_fix")))
+# endif
+
// Changes the iterator type of select containers (see below) to a bounded iterator that keeps track of whether it's
// within the bounds of the original container and asserts it on every dereference.
//
diff --git a/libcxx/include/__ranges/chunk_by_view.h b/libcxx/include/__ranges/chunk_by_view.h
index e4699868764698..3ecc018cac9d91 100644
--- a/libcxx/include/__ranges/chunk_by_view.h
+++ b/libcxx/include/__ranges/chunk_by_view.h
@@ -54,7 +54,8 @@ namespace ranges {
template <forward_range _View, indirect_binary_predicate<iterator_t<_View>, iterator_t<_View>> _Pred>
requires view<_View> && is_object_v<_Pred>
-class chunk_by_view : public view_interface<chunk_by_view<_View, _Pred>> {
+class _LIBCPP_ABI_2023_OVERLAPPING_SUBOBJECT_FIX_TAG chunk_by_view
+ : public view_interface<chunk_by_view<_View, _Pred>> {
_LIBCPP_NO_UNIQUE_ADDRESS _View __base_ = _View();
_LIBCPP_NO_UNIQUE_ADDRESS __movable_box<_Pred> __pred_;
diff --git a/libcxx/include/__ranges/drop_while_view.h b/libcxx/include/__ranges/drop_while_view.h
index 677b5bc66d442a..eb3783eb42f146 100644
--- a/libcxx/include/__ranges/drop_while_view.h
+++ b/libcxx/include/__ranges/drop_while_view.h
@@ -45,7 +45,8 @@ namespace ranges {
template <view _View, class _Pred>
requires input_range<_View> && is_object_v<_Pred> && indirect_unary_predicate<const _Pred, iterator_t<_View>>
-class drop_while_view : public view_interface<drop_while_view<_View, _Pred>> {
+class _LIBCPP_ABI_2023_OVERLAPPING_SUBOBJECT_FIX_TAG drop_while_view
+ : public view_interface<drop_while_view<_View, _Pred>> {
public:
_LIBCPP_HIDE_FROM_ABI drop_while_view()
requires default_initializable<_View> && default_initializable<_Pred>
diff --git a/libcxx/include/__ranges/filter_view.h b/libcxx/include/__ranges/filter_view.h
index 08d50ab011042b..868ad128e8944a 100644
--- a/libcxx/include/__ranges/filter_view.h
+++ b/libcxx/include/__ranges/filter_view.h
@@ -51,7 +51,7 @@ _LIBCPP_BEGIN_NAMESPACE_STD
namespace ranges {
template <input_range _View, indirect_unary_predicate<iterator_t<_View>> _Pred>
requires view<_View> && is_object_v<_Pred>
-class filter_view : public view_interface<filter_view<_View, _Pred>> {
+class _LIBCPP_ABI_2023_OVERLAPPING_SUBOBJECT_FIX_TAG filter_view : public view_interface<filter_view<_View, _Pred>> {
_LIBCPP_NO_UNIQUE_ADDRESS _View __base_ = _View();
_LIBCPP_NO_UNIQUE_ADDRESS __movable_box<_Pred> __pred_;
diff --git a/libcxx/include/__ranges/movable_box.h b/libcxx/include/__ranges/movable_box.h
index a03cd70fa09ed9..80773927bfd488 100644
--- a/libcxx/include/__ranges/movable_box.h
+++ b/libcxx/include/__ranges/movable_box.h
@@ -135,9 +135,16 @@ concept __doesnt_need_empty_state =
// 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
+// When _Tp doesn't have an assignment operator, we must implement __movable_box's assignment operator
+// by doing destroy_at followed by construct_at. However, that implementation strategy leads to UB if the nested
+// _Tp is potentially overlapping. As it is doing a non-transparent replacement of the sub-object, which means that
+// we're not considered "nested" inside the movable-box anymore, and since we're not nested within it, [basic.life]/1.5
+// says that we essentially just reused the storage of the movable-box for a completely unrelated object and ended the
+// movable-box's lifetime.
+// https://github.com/llvm/llvm-project/issues/70494#issuecomment-1845646490
+//
+// Hence, when the _Tp doesn't have an assignment operator, we can't risk making it a potentially-overlapping
+// subobject because of the above, and we don't use [[no_unique_address]] in that case.
template <class _Tp>
concept __can_use_no_unique_address = (copy_constructible<_Tp> ? copyable<_Tp> : movable<_Tp>);
@@ -157,41 +164,39 @@ concept __can_use_no_unique_address = copyable<_Tp>;
# endif
template <class _Tp>
-struct __movable_box_base {
+struct __movable_box_holder {
_Tp __val_;
template <class... _Args>
- requires is_constructible_v<_Tp, _Args&&...>
- _LIBCPP_HIDE_FROM_ABI constexpr explicit __movable_box_base(_Args&&... __args)
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit __movable_box_holder(in_place_t, _Args&&... __args)
: __val_(std::forward<_Args>(__args)...) {}
};
template <class _Tp>
requires __can_use_no_unique_address<_Tp>
-struct __movable_box_base<_Tp> {
+struct __movable_box_holder<_Tp> {
_LIBCPP_NO_UNIQUE_ADDRESS _Tp __val_;
template <class... _Args>
- requires is_constructible_v<_Tp, _Args&&...>
- _LIBCPP_HIDE_FROM_ABI constexpr explicit __movable_box_base(_Args&&... __args)
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit __movable_box_holder(in_place_t, _Args&&... __args)
: __val_(std::forward<_Args>(__args)...) {}
};
template <__movable_box_object _Tp>
requires __doesnt_need_empty_state<_Tp>
-class __movable_box<_Tp> : private __movable_box_base<_Tp> {
- using __base = __movable_box_base<_Tp>;
+class __movable_box<_Tp> {
+ _LIBCPP_NO_UNIQUE_ADDRESS __movable_box_holder<_Tp> __holder_;
public:
template <class... _Args>
requires is_constructible_v<_Tp, _Args...>
- _LIBCPP_HIDE_FROM_ABI constexpr explicit __movable_box(in_place_t, _Args&&... __args) noexcept(
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit __movable_box(in_place_t __inplace, _Args&&... __args) noexcept(
is_nothrow_constructible_v<_Tp, _Args...>)
- : __base(std::forward<_Args>(__args)...) {}
+ : __holder_(__inplace, std::forward<_Args>(__args)...) {}
_LIBCPP_HIDE_FROM_ABI constexpr __movable_box() noexcept(is_nothrow_default_constructible_v<_Tp>)
requires default_initializable<_Tp>
- : __base() {}
+ : __holder_(in_place_t{}) {}
_LIBCPP_HIDE_FROM_ABI __movable_box(__movable_box const&) = default;
_LIBCPP_HIDE_FROM_ABI __movable_box(__movable_box&&) = default;
@@ -209,8 +214,8 @@ class __movable_box<_Tp> : private __movable_box_base<_Tp> {
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_);
+ std::destroy_at(std::addressof(__holder_.__val_));
+ std::construct_at(std::addressof(__holder_.__val_), __other.__holder_.__val_);
}
return *this;
}
@@ -219,17 +224,17 @@ class __movable_box<_Tp> : private __movable_box_base<_Tp> {
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_));
+ std::destroy_at(std::addressof(__holder_.__val_));
+ std::construct_at(std::addressof(__holder_.__val_), std::move(__other.__holder_.__val_));
}
return *this;
}
- _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 _Tp const& operator*() const noexcept { return __holder_.__val_; }
+ _LIBCPP_HIDE_FROM_ABI constexpr _Tp& operator*() noexcept { return __holder_.__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 const _Tp* operator->() const noexcept { return std::addressof(__holder_.__val_); }
+ _LIBCPP_HIDE_FROM_ABI constexpr _Tp* operator->() noexcept { return std::addressof(__holder_.__val_); }
_LIBCPP_HIDE_FROM_ABI constexpr bool __has_value() const noexcept { return true; }
};
diff --git a/libcxx/include/__ranges/repeat_view.h b/libcxx/include/__ranges/repeat_view.h
index 459a1e229613ac..479eca96acb098 100644
--- a/libcxx/include/__ranges/repeat_view.h
+++ b/libcxx/include/__ranges/repeat_view.h
@@ -68,7 +68,7 @@ struct __fn;
template <move_constructible _Tp, semiregular _Bound = unreachable_sentinel_t>
requires(is_object_v<_Tp> && same_as<_Tp, remove_cv_t<_Tp>> &&
(__integer_like_with_usable_difference_type<_Bound> || same_as<_Bound, unreachable_sentinel_t>))
-class repeat_view : public view_interface<repeat_view<_Tp, _Bound>> {
+class _LIBCPP_ABI_2023_OVERLAPPING_SUBOBJECT_FIX_TAG repeat_view : public view_interface<repeat_view<_Tp, _Bound>> {
friend struct views::__take::__fn;
friend struct views::__drop::__fn;
class __iterator;
@@ -119,7 +119,7 @@ class repeat_view : public view_interface<repeat_view<_Tp, _Bound>> {
}
private:
- __movable_box<_Tp> __value_;
+ _LIBCPP_NO_UNIQUE_ADDRESS __movable_box<_Tp> __value_;
_LIBCPP_NO_UNIQUE_ADDRESS _Bound __bound_ = _Bound();
};
diff --git a/libcxx/include/__ranges/single_view.h b/libcxx/include/__ranges/single_view.h
index b0b2c1d9f3c062..0ae2036a66a92b 100644
--- a/libcxx/include/__ranges/single_view.h
+++ b/libcxx/include/__ranges/single_view.h
@@ -37,8 +37,8 @@ template <move_constructible _Tp>
template <copy_constructible _Tp>
# endif
requires is_object_v<_Tp>
-class single_view : public view_interface<single_view<_Tp>> {
- __movable_box<_Tp> __value_;
+class _LIBCPP_ABI_2023_OVERLAPPING_SUBOBJECT_FIX_TAG single_view : public view_interface<single_view<_Tp>> {
+ _LIBCPP_NO_UNIQUE_ADDRESS __movable_box<_Tp> __value_;
public:
_LIBCPP_HIDE_FROM_ABI single_view()
diff --git a/libcxx/include/__ranges/take_while_view.h b/libcxx/include/__ranges/take_while_view.h
index a6f7f80ca76bfb..4534210d97943e 100644
--- a/libcxx/include/__ranges/take_while_view.h
+++ b/libcxx/include/__ranges/take_while_view.h
@@ -43,7 +43,8 @@ namespace ranges {
template <view _View, class _Pred>
requires input_range<_View> && is_object_v<_Pred> && indirect_unary_predicate<const _Pred, iterator_t<_View>>
-class take_while_view : public view_interface<take_while_view<_View, _Pred>> {
+class _LIBCPP_ABI_2023_OVERLAPPING_SUBOBJECT_FIX_TAG take_while_view
+ : public view_interface<take_while_view<_View, _Pred>> {
template <bool>
class __sentinel;
diff --git a/libcxx/include/__ranges/transform_view.h b/libcxx/include/__ranges/transform_view.h
index 55c6ce587bd69e..744f597ccef593 100644
--- a/libcxx/include/__ranges/transform_view.h
+++ b/libcxx/include/__ranges/transform_view.h
@@ -67,7 +67,8 @@ template <input_range _View, move_constructible _Fn>
template <input_range _View, copy_constructible _Fn>
# endif
requires __transform_view_constraints<_View, _Fn>
-class transform_view : public view_interface<transform_view<_View, _Fn>> {
+class _LIBCPP_ABI_2023_OVERLAPPING_SUBOBJECT_FIX_TAG transform_view
+ : public view_interface<transform_view<_View, _Fn>> {
template <bool>
class __iterator;
template <bool>
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 b7dee217e4945e..e8916059ac2456 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
@@ -8,6 +8,9 @@
// UNSUPPORTED: c++03, c++11, c++14, c++17
+// clang-cl and cl currently don't support [[no_unique_address]]
+// XFAIL: msvc
+
// This test ensures that <copyable-box> behaves correctly when it holds an empty type.
#include <ranges>
@@ -20,7 +23,7 @@
template <class T, bool ExpectNoUniqueAddress>
void test_no_unique_address() {
struct Test {
- _LIBCPP_NO_UNIQUE_ADDRESS std::ranges::__movable_box<T> box_;
+ [[no_unique_address]] std::ranges::__movable_box<T> box_;
bool b2;
};
diff --git a/libcxx/test/libcxx/ranges/range.factories/range.repeat.view/no_unique_address.compile.pass.cpp b/libcxx/test/libcxx/ranges/range.factories/range.repeat.view/no_unique_address.compile.pass.cpp
new file mode 100644
index 00000000000000..f6e0e25494daca
--- /dev/null
+++ b/libcxx/test/libcxx/ranges/range.factories/range.repeat.view/no_unique_address.compile.pass.cpp
@@ -0,0 +1,21 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+// XFAIL: msvc
+
+#include <ranges>
+
+struct Empty {};
+
+struct Test {
+ [[no_unique_address]] std::ranges::repeat_view<Empty> v;
+ bool b;
+};
+
+static_assert(sizeof(Test) == sizeof(bool));
diff --git a/libcxx/test/libcxx/ranges/range.factories/range.single.view/no_unique_address.compile.pass.cpp b/libcxx/test/libcxx/ranges/range.factories/range.single.view/no_unique_address.compile.pass.cpp
new file mode 100644
index 00000000000000..8106131d3c54b2
--- /dev/null
+++ b/libcxx/test/libcxx/ranges/range.factories/range.single.view/no_unique_address.compile.pass.cpp
@@ -0,0 +1,21 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+// XFAIL: msvc
+
+#include <ranges>
+
+struct Empty {};
+
+struct Test {
+ [[no_unique_address]] std::ranges::single_view<Empty> v;
+ bool b;
+};
+
+static_assert(sizeof(Test) == sizeof(bool));
diff --git a/libcxx/test/std/ranges/range_robust_against_no_unique_address.pass.cpp b/libcxx/test/std/ranges/range_robust_against_no_unique_address.pass.cpp
new file mode 100644
index 00000000000000..a35ecf1ed72e59
--- /dev/null
+++ b/libcxx/test/std/ranges/range_robust_against_no_unique_address.pass.cpp
@@ -0,0 +1,64 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+
+// Test that views that use __movable_box do not overwrite overlapping subobjects.
+
+#include <cassert>
+#include <ranges>
+
+struct Pred {
+ alignas(128) bool a{};
+
+ Pred() noexcept = default;
+ Pred(const Pred&) noexcept = default;
+ Pred(Pred&&) noexcept = default;
+
+ Pred& operator=(const Pred&) = delete;
+ Pred& operator=(Pred&&) = delete;
+
+ constexpr bool operator()(const auto&...) const { return true; }
+};
+
+struct View : std::ranges::view_base {
+ constexpr int* begin() const { return nullptr; }
+ constexpr int* end() const { return nullptr; }
+};
+
+template <class T>
+struct S {
+ [[no_unique_address]] T t{};
+ char c = 42;
+};
+
+template <class T>
+constexpr void testOne() {
+ S<T> s1;
+ assert(s1.c == 42);
+ s1.t = T{};
+ assert(s1.c == 42);
+}
+
+constexpr bool test() {
+ testOne<std::ranges::transform_view<View, Pred>>();
+ testOne<std::ranges::filter_view<View, Pred>>();
+ testOne<std::ranges::chunk_by_view<View, Pred>>();
+ testOne<std::ranges::drop_while_view<View, Pred>>();
+ testOne<std::ranges::take_while_view<View, Pred>>();
+ testOne<std::ranges::repeat_view<Pred>>();
+ testOne<std::ranges::single_view<Pred>>();
+ return true;
+}
+
+int main(int, char**) {
+ static_assert(test());
+ test();
+
+ return 0;
+}
More information about the libcxx-commits
mailing list