[libcxx-commits] [libcxx] [libc++] Ensure that `std::expected` has no tail padding (PR #69673)
Jan Kokemüller via libcxx-commits
libcxx-commits at lists.llvm.org
Fri Oct 20 10:57:15 PDT 2023
https://github.com/jiixyj updated https://github.com/llvm/llvm-project/pull/69673
>From 74b266db78accc3f23e0cc1d96aa7277eeccdb2f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jan=20Kokem=C3=BCller?= <jan.kokemueller at gmail.com>
Date: Sun, 15 Oct 2023 22:09:51 +0200
Subject: [PATCH 1/5] ensure that expected has no tail padding
---
libcxx/include/__expected/expected.h | 20 +++++++++++++++++++
.../expected.expected/assign/emplace.pass.cpp | 16 +++++++++++++++
2 files changed, 36 insertions(+)
diff --git a/libcxx/include/__expected/expected.h b/libcxx/include/__expected/expected.h
index 045370a486fae6b..463060702235742 100644
--- a/libcxx/include/__expected/expected.h
+++ b/libcxx/include/__expected/expected.h
@@ -18,6 +18,7 @@
#include <__memory/addressof.h>
#include <__memory/construct_at.h>
#include <__type_traits/conjunction.h>
+#include <__type_traits/datasizeof.h>
#include <__type_traits/disjunction.h>
#include <__type_traits/integral_constant.h>
#include <__type_traits/is_assignable.h>
@@ -955,8 +956,17 @@ class expected {
_LIBCPP_NO_UNIQUE_ADDRESS _ErrorType __unex_;
};
+ _LIBCPP_HIDE_FROM_ABI static constexpr auto __calculate_padding() {
+ struct __calc_expected {
+ _LIBCPP_NO_UNIQUE_ADDRESS __union_t<_Tp, _Err> __union_;
+ bool __has_val_;
+ };
+ return sizeof(__calc_expected) - __libcpp_datasizeof<__calc_expected>::value;
+ }
+
_LIBCPP_NO_UNIQUE_ADDRESS __union_t<_Tp, _Err> __union_;
bool __has_val_;
+ char __padding_[__calculate_padding()]{};
};
template <class _Tp, class _Err>
@@ -1552,8 +1562,18 @@ class expected<_Tp, _Err> {
_LIBCPP_NO_UNIQUE_ADDRESS _ErrorType __unex_;
};
+
+ _LIBCPP_HIDE_FROM_ABI static constexpr auto __calculate_padding() {
+ struct __calc_expected {
+ _LIBCPP_NO_UNIQUE_ADDRESS __union_t<_Err> __union_;
+ bool __has_val_;
+ };
+ return sizeof(__calc_expected) - __libcpp_datasizeof<__calc_expected>::value;
+ }
+
_LIBCPP_NO_UNIQUE_ADDRESS __union_t<_Err> __union_;
bool __has_val_;
+ char __padding_[__calculate_padding()]{};
};
_LIBCPP_END_NAMESPACE_STD
diff --git a/libcxx/test/std/utilities/expected/expected.expected/assign/emplace.pass.cpp b/libcxx/test/std/utilities/expected/expected.expected/assign/emplace.pass.cpp
index c62e6289350201a..5be8d2b5fb74e40 100644
--- a/libcxx/test/std/utilities/expected/expected.expected/assign/emplace.pass.cpp
+++ b/libcxx/test/std/utilities/expected/expected.expected/assign/emplace.pass.cpp
@@ -73,6 +73,22 @@ constexpr bool test() {
assert(e.value() == 10);
}
+ // Test that `expected` has no tail padding.
+ {
+ struct BoolWithPadding {
+ explicit operator bool() { return b; }
+
+ private:
+ alignas(4) bool b = false;
+ };
+
+ static_assert(sizeof(std::expected<BoolWithPadding, bool>) ==
+ std::__libcpp_datasizeof<std::expected<BoolWithPadding, bool>>::value);
+
+ static_assert(sizeof(std::expected<void, BoolWithPadding>) ==
+ std::__libcpp_datasizeof<std::expected<void, BoolWithPadding>>::value);
+ }
+
return true;
}
>From 71bc5da89260fd31a284651ccf870d00f8bc41db Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jan=20Kokem=C3=BCller?= <jan.kokemueller at gmail.com>
Date: Fri, 20 Oct 2023 19:39:15 +0200
Subject: [PATCH 2/5] remove tests from emplace.pass.cpp
---
.../expected.expected/assign/emplace.pass.cpp | 16 ----------------
1 file changed, 16 deletions(-)
diff --git a/libcxx/test/std/utilities/expected/expected.expected/assign/emplace.pass.cpp b/libcxx/test/std/utilities/expected/expected.expected/assign/emplace.pass.cpp
index 5be8d2b5fb74e40..c62e6289350201a 100644
--- a/libcxx/test/std/utilities/expected/expected.expected/assign/emplace.pass.cpp
+++ b/libcxx/test/std/utilities/expected/expected.expected/assign/emplace.pass.cpp
@@ -73,22 +73,6 @@ constexpr bool test() {
assert(e.value() == 10);
}
- // Test that `expected` has no tail padding.
- {
- struct BoolWithPadding {
- explicit operator bool() { return b; }
-
- private:
- alignas(4) bool b = false;
- };
-
- static_assert(sizeof(std::expected<BoolWithPadding, bool>) ==
- std::__libcpp_datasizeof<std::expected<BoolWithPadding, bool>>::value);
-
- static_assert(sizeof(std::expected<void, BoolWithPadding>) ==
- std::__libcpp_datasizeof<std::expected<void, BoolWithPadding>>::value);
- }
-
return true;
}
>From f07fd5f821e292d49e30cd97ddfc019c8df97474 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jan=20Kokem=C3=BCller?= <jan.kokemueller at gmail.com>
Date: Fri, 20 Oct 2023 19:40:18 +0200
Subject: [PATCH 3/5] handle GCC's warning in __libcpp_datasizeof
---
libcxx/include/__type_traits/datasizeof.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/libcxx/include/__type_traits/datasizeof.h b/libcxx/include/__type_traits/datasizeof.h
index 5688e3293a69eb5..96404ad15a73376 100644
--- a/libcxx/include/__type_traits/datasizeof.h
+++ b/libcxx/include/__type_traits/datasizeof.h
@@ -51,6 +51,7 @@ struct __libcpp_datasizeof {
// the use as an extension.
_LIBCPP_DIAGNOSTIC_PUSH
_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Winvalid-offsetof")
+ _LIBCPP_GCC_DIAGNOSTIC_IGNORED("-Winvalid-offsetof")
static const size_t value = offsetof(_FirstPaddingByte<>, __first_padding_byte_);
_LIBCPP_DIAGNOSTIC_POP
};
>From a17243c8d78e8ab09d60665ef08087f81e4ffd98 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jan=20Kokem=C3=BCller?= <jan.kokemueller at gmail.com>
Date: Fri, 20 Oct 2023 19:53:22 +0200
Subject: [PATCH 4/5] implement different approach to padding calculation
This tries to fix two issues:
- There should be no artificial padding if the parameter type(s) don't
need it, for example if they have no tail padding
- Prevent the padding array being zero length
---
libcxx/include/__expected/expected.h | 53 +++++++++++++++++-----------
1 file changed, 32 insertions(+), 21 deletions(-)
diff --git a/libcxx/include/__expected/expected.h b/libcxx/include/__expected/expected.h
index 463060702235742..0728d3af7bb47ca 100644
--- a/libcxx/include/__expected/expected.h
+++ b/libcxx/include/__expected/expected.h
@@ -89,6 +89,34 @@ _LIBCPP_HIDE_FROM_ABI void __throw_bad_expected_access(_Arg&& __arg) {
# endif
}
+template <size_t __Padding>
+struct __expected_padding {
+ using type = char[__Padding];
+};
+
+template <>
+struct __expected_padding<0> {
+ using type = struct {};
+};
+
+template <class _Union>
+_LIBCPP_HIDE_FROM_ABI constexpr size_t __expected_calculate_padding() {
+ struct __calc_expected {
+ _LIBCPP_NO_UNIQUE_ADDRESS _Union __union_;
+ bool __has_val_;
+ };
+
+ size_t __datasize = __libcpp_datasizeof<__calc_expected>::value;
+ return sizeof(_Union) < __datasize ? 0 : sizeof(_Union) - __datasize;
+}
+
+// An object of this type must be the last member of the `expected` class to
+// ensure `expected`'s datasize is large enough to fit the parameter type(s).
+// It should be value-initialized so it is safe to copy when `expected`'s
+// copy operators are invoked.
+template <typename _Union>
+using __expected_padding_t = __expected_padding<__expected_calculate_padding<_Union>()>::type;
+
template <class _Tp, class _Err>
class expected {
static_assert(
@@ -956,17 +984,9 @@ class expected {
_LIBCPP_NO_UNIQUE_ADDRESS _ErrorType __unex_;
};
- _LIBCPP_HIDE_FROM_ABI static constexpr auto __calculate_padding() {
- struct __calc_expected {
- _LIBCPP_NO_UNIQUE_ADDRESS __union_t<_Tp, _Err> __union_;
- bool __has_val_;
- };
- return sizeof(__calc_expected) - __libcpp_datasizeof<__calc_expected>::value;
- }
-
_LIBCPP_NO_UNIQUE_ADDRESS __union_t<_Tp, _Err> __union_;
- bool __has_val_;
- char __padding_[__calculate_padding()]{};
+ _LIBCPP_NO_UNIQUE_ADDRESS bool __has_val_;
+ _LIBCPP_NO_UNIQUE_ADDRESS __expected_padding_t<__union_t<_Tp, _Err>> __padding_{};
};
template <class _Tp, class _Err>
@@ -1562,18 +1582,9 @@ class expected<_Tp, _Err> {
_LIBCPP_NO_UNIQUE_ADDRESS _ErrorType __unex_;
};
-
- _LIBCPP_HIDE_FROM_ABI static constexpr auto __calculate_padding() {
- struct __calc_expected {
- _LIBCPP_NO_UNIQUE_ADDRESS __union_t<_Err> __union_;
- bool __has_val_;
- };
- return sizeof(__calc_expected) - __libcpp_datasizeof<__calc_expected>::value;
- }
-
_LIBCPP_NO_UNIQUE_ADDRESS __union_t<_Err> __union_;
- bool __has_val_;
- char __padding_[__calculate_padding()]{};
+ _LIBCPP_NO_UNIQUE_ADDRESS bool __has_val_;
+ _LIBCPP_NO_UNIQUE_ADDRESS __expected_padding_t<__union_t<_Err>> __padding_{};
};
_LIBCPP_END_NAMESPACE_STD
>From cc8cb38f9d21262af9225d2644c52f550b0594c0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jan=20Kokem=C3=BCller?= <jan.kokemueller at gmail.com>
Date: Fri, 20 Oct 2023 19:56:59 +0200
Subject: [PATCH 5/5] add tests
---
.../no_unique_address.compile.pass.cpp | 17 +++++++++++++++++
.../no_unique_address.compile.pass.cpp | 15 +++++++++++++++
2 files changed, 32 insertions(+)
diff --git a/libcxx/test/libcxx/utilities/expected/expected.expected/no_unique_address.compile.pass.cpp b/libcxx/test/libcxx/utilities/expected/expected.expected/no_unique_address.compile.pass.cpp
index c8c217054ae6c43..06f597d54871ee1 100644
--- a/libcxx/test/libcxx/utilities/expected/expected.expected/no_unique_address.compile.pass.cpp
+++ b/libcxx/test/libcxx/utilities/expected/expected.expected/no_unique_address.compile.pass.cpp
@@ -26,6 +26,13 @@ struct B : public A {
virtual ~B() = default;
};
+struct BoolWithPadding {
+ explicit operator bool() { return b; }
+
+private:
+ alignas(1024) bool b = false;
+};
+
static_assert(sizeof(std::expected<Empty, Empty>) == sizeof(bool));
static_assert(sizeof(std::expected<Empty, A>) == 2 * sizeof(int) + alignof(std::expected<Empty, A>));
static_assert(sizeof(std::expected<Empty, B>) == sizeof(B) + alignof(std::expected<Empty, B>));
@@ -33,3 +40,13 @@ static_assert(sizeof(std::expected<A, Empty>) == 2 * sizeof(int) + alignof(std::
static_assert(sizeof(std::expected<A, A>) == 2 * sizeof(int) + alignof(std::expected<A, A>));
static_assert(sizeof(std::expected<B, Empty>) == sizeof(B) + alignof(std::expected<B, Empty>));
static_assert(sizeof(std::expected<B, B>) == sizeof(B) + alignof(std::expected<B, B>));
+
+// Check that `expected`'s datasize is large enough for the parameter type(s).
+static_assert(sizeof(std::expected<BoolWithPadding, Empty>) ==
+ std::__libcpp_datasizeof<std::expected<BoolWithPadding, Empty>>::value);
+static_assert(sizeof(std::expected<Empty, BoolWithPadding>) ==
+ std::__libcpp_datasizeof<std::expected<Empty, BoolWithPadding>>::value);
+
+// In this case, there should be tail padding in the `expected` because `A`
+// itself does _not_ have tail padding.
+static_assert(sizeof(std::expected<A, A>) > std::__libcpp_datasizeof<std::expected<A, A>>::value);
diff --git a/libcxx/test/libcxx/utilities/expected/expected.void/no_unique_address.compile.pass.cpp b/libcxx/test/libcxx/utilities/expected/expected.void/no_unique_address.compile.pass.cpp
index dc59a6228386bcd..29cf8dff0d2b288 100644
--- a/libcxx/test/libcxx/utilities/expected/expected.void/no_unique_address.compile.pass.cpp
+++ b/libcxx/test/libcxx/utilities/expected/expected.void/no_unique_address.compile.pass.cpp
@@ -26,6 +26,21 @@ struct B : public A {
virtual ~B() = default;
};
+struct BoolWithPadding {
+ explicit operator bool() { return b; }
+
+private:
+ alignas(1024) bool b = false;
+};
+
static_assert(sizeof(std::expected<void, Empty>) == sizeof(bool));
static_assert(sizeof(std::expected<void, A>) == 2 * sizeof(int) + alignof(std::expected<void, A>));
static_assert(sizeof(std::expected<void, B>) == sizeof(B) + alignof(std::expected<void, B>));
+
+// Check that `expected`'s datasize is large enough for the parameter type(s).
+static_assert(sizeof(std::expected<void, BoolWithPadding>) ==
+ std::__libcpp_datasizeof<std::expected<void, BoolWithPadding>>::value);
+
+// In this case, there should be tail padding in the `expected` because `A`
+// itself does _not_ have tail padding.
+static_assert(sizeof(std::expected<void, A>) > std::__libcpp_datasizeof<std::expected<void, A>>::value);
More information about the libcxx-commits
mailing list