[libcxx-commits] [libcxx] [libc++] Ensure that `std::expected` has no tail padding (PR #69673)
via libcxx-commits
libcxx-commits at lists.llvm.org
Fri Jan 19 03:06:07 PST 2024
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/69673 at github.com>
================
@@ -88,8 +88,353 @@ _LIBCPP_HIDE_FROM_ABI void __throw_bad_expected_access(_Arg&& __arg) {
# endif
}
+struct __conditional_no_unique_address_invoke_tag {};
+
+// This class implements an object with `[[no_unique_address]]` conditionally applied to it,
+// based on the value of `_NoUnique`.
+//
+// A member of this class must always have `[[no_unique_address]]` applied to
+// it. Otherwise, the `[[no_unique_address]]` in the "`_NoUnique == true`" case
+// would not have any effect. In the `false` case, the `__v` is not
+// `[[no_unique_address]]`, so nullifies the effects of the "outer"
+// `[[no_unique_address]]` regarding data layout.
+//
+// If we had a language feature, this class would basically be replaced by `[[no_unique_address(condition)]]`.
+template <bool _NoUnique, class _Tp>
+struct __conditional_no_unique_address;
+
+template <class _Tp>
+struct __conditional_no_unique_address<true, _Tp> {
+ template <class... _Args>
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit __conditional_no_unique_address(in_place_t, _Args&&... __args)
+ : __v(std::forward<_Args>(__args)...) {}
+
+ template <class _Func, class... _Args>
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit __conditional_no_unique_address(
+ __conditional_no_unique_address_invoke_tag, _Func&& __f, _Args&&... __args)
+ : __v(std::invoke(std::forward<_Func>(__f), std::forward<_Args>(__args)...)) {}
+
+ _LIBCPP_NO_UNIQUE_ADDRESS _Tp __v;
+};
+
+template <class _Tp>
+struct __conditional_no_unique_address<false, _Tp> {
+ template <class... _Args>
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit __conditional_no_unique_address(in_place_t, _Args&&... __args)
+ : __v(std::forward<_Args>(__args)...) {}
+
+ template <class _Func, class... _Args>
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit __conditional_no_unique_address(
+ __conditional_no_unique_address_invoke_tag, _Func&& __f, _Args&&... __args)
+ : __v(std::invoke(std::forward<_Func>(__f), std::forward<_Args>(__args)...)) {}
+
+ _Tp __v;
+};
+
+// This function returns whether the type `_Second` can be stuffed into the tail padding
+// of the `_First` type if both of them are given `[[no_unique_address]]`.
+template <class _First, class _Second>
+inline constexpr bool __fits_in_tail_padding = []() {
+ struct __x {
+ _LIBCPP_NO_UNIQUE_ADDRESS _First __first;
+ _LIBCPP_NO_UNIQUE_ADDRESS _Second __second;
+ };
+ return sizeof(__x) == sizeof(_First);
+}();
+
+// This class implements the storage used by `std::expected`. We have a few
+// goals for this storage:
+// 1. Whenever the underlying {_Tp | _Unex} combination has free bytes in its
+// tail padding, we should reuse it to store the bool discriminator of the
+// expected, so as to save space.
+// 2. Whenever the `expected<_Tp, _Unex>` as a whole has free bytes in its tail
+// padding, we should allow an object following the expected to be stored in
+// its tail padding.
+// 3. However, we never want a user object (say `X`) that would follow an
+// `expected<_Tp, _Unex>` to be stored in the padding bytes of the
+// underlying {_Tp | _Unex} union, if any. That is because we use
+// `construct_at` on that union, which would end up overwriting the `X`
+// member if it is stored in the tail padding of the union.
+//
+// To achieve this, `__expected_base`'s logic is implemented in an inner
+// `__repr` class. `__expected_base` holds one `__repr` member which is
+// conditionally `[[no_unique_address]]`. The `__repr` class holds the
+// underlying {_Tp | _Unex} union and a boolean "has value" flag.
+//
+// Which one of the `__repr_`/`__union_` members is `[[no_unique_address]]`
+// depends on whether the "has value" boolean fits into the tail padding of
+// the underlying {_Tp | _Unex} union:
+//
+// - In case the "has value" bool fits into the tail padding of the union, the
+// whole `__repr_` member is _not_ `[[no_unique_address]]` as it needs to be
+// transparently replaced on `emplace()`/`swap()` etc.
+// - In case the "has value" bool does not fit into the tail padding of the
+// union, only the union member must be transparently replaced (therefore is
+// _not_ `[[no_unique_address]]`) and the "has value" flag must be adjusted
+// manually.
+//
+// This way, the member that is transparently replaced on mutating operations
+// is never `[[no_unique_address]]`, satisfying the requirements from
+// "[basic.life]" in the standard.
+//
+// Stripped away of all superfluous elements, the layout of `__expected_base`
+// then looks like this:
+//
+// template <class Tp, class Err>
+// class expected_base {
+// union union_t {
+// [[no_unique_address]] Tp val;
+// [[no_unique_address]] Err unex;
+// };
+//
+// struct repr {
+// private:
+// // If "has value" fits into the tail, this should be
+// // `[[no_unique_address]]`, otherwise not.
+// [[no_unique_address]] conditional_no_unique_address<
+// fits_in_tail_padding<union_t, bool>,
+// union_t>::type union_;
+// [[no_unique_address]] bool has_val_;
+// };
+//
+// protected:
+// // If "has value" fits into the tail, this must _not_ be
+// // `[[no_unique_address]]` so that we fill out the
+// // complete `expected` object.
+// [[no_unique_address]] conditional_no_unique_address<
+// !fits_in_tail_padding<union_t, bool>,
+// repr>::type repr_;
+// };
+//
template <class _Tp, class _Err>
-class expected {
+class __expected_base {
+ // use named union because [[no_unique_address]] cannot be applied to an unnamed union,
+ // also guaranteed elision into a potentially-overlapping subobject is unsettled (and
+ // it's not clear that it's implementable, given that the function is allowed to clobber
+ // the tail padding) - see https://github.com/itanium-cxx-abi/cxx-abi/issues/107.
+ union __union_t {
+ _LIBCPP_HIDE_FROM_ABI constexpr __union_t(const __union_t&) = delete;
+ _LIBCPP_HIDE_FROM_ABI constexpr __union_t(const __union_t&)
+ requires(is_copy_constructible_v<_Tp> && is_copy_constructible_v<_Err> &&
+ is_trivially_copy_constructible_v<_Tp> && is_trivially_copy_constructible_v<_Err>)
+ = default;
+ _LIBCPP_HIDE_FROM_ABI constexpr __union_t(__union_t&&) = delete;
+ _LIBCPP_HIDE_FROM_ABI constexpr __union_t(__union_t&&)
+ requires(is_move_constructible_v<_Tp> && is_move_constructible_v<_Err> &&
+ is_trivially_move_constructible_v<_Tp> && is_trivially_move_constructible_v<_Err>)
+ = default;
+ _LIBCPP_HIDE_FROM_ABI constexpr __union_t& operator=(const __union_t&) = delete;
+ _LIBCPP_HIDE_FROM_ABI constexpr __union_t& operator=(__union_t&&) = delete;
+
+ template <class... _Args>
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(in_place_t, _Args&&... __args)
+ : __val_(std::forward<_Args>(__args)...) {}
+
+ template <class... _Args>
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(unexpect_t, _Args&&... __args)
+ : __unex_(std::forward<_Args>(__args)...) {}
+
+ template <class _Func, class... _Args>
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(
+ std::__expected_construct_in_place_from_invoke_tag, _Func&& __f, _Args&&... __args)
+ : __val_(std::invoke(std::forward<_Func>(__f), std::forward<_Args>(__args)...)) {}
+
+ template <class _Func, class... _Args>
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(
+ std::__expected_construct_unexpected_from_invoke_tag, _Func&& __f, _Args&&... __args)
+ : __unex_(std::invoke(std::forward<_Func>(__f), std::forward<_Args>(__args)...)) {}
+
+ _LIBCPP_HIDE_FROM_ABI constexpr ~__union_t()
+ requires(is_trivially_destructible_v<_Tp> && is_trivially_destructible_v<_Err>)
+ = default;
+
+ // __repr's destructor handles this
+ _LIBCPP_HIDE_FROM_ABI constexpr ~__union_t() {}
+
+ _LIBCPP_NO_UNIQUE_ADDRESS _Tp __val_;
+ _LIBCPP_NO_UNIQUE_ADDRESS _Err __unex_;
+ };
+
+ static constexpr bool __put_flag_in_tail = __fits_in_tail_padding<__union_t, bool>;
+
+ struct __repr {
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit __repr() = delete;
+
+ template <class... _Args>
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit __repr(in_place_t __tag, _Args&&... __args)
+ : __union_(in_place, __tag, std::forward<_Args>(__args)...), __has_val_(true) {}
+
+ template <class... _Args>
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit __repr(unexpect_t __tag, _Args&&... __args)
+ : __union_(in_place, __tag, std::forward<_Args>(__args)...), __has_val_(false) {}
+
+ template <class... _Args>
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit __repr(std::__expected_construct_in_place_from_invoke_tag __tag,
+ _Args&&... __args)
+ : __union_(in_place, __tag, std::forward<_Args>(__args)...), __has_val_(true) {}
+
+ template <class... _Args>
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit __repr(std::__expected_construct_unexpected_from_invoke_tag __tag,
+ _Args&&... __args)
+ : __union_(in_place, __tag, std::forward<_Args>(__args)...), __has_val_(false) {}
+
+ template <class _OtherUnion>
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit __repr(bool __has_val, _OtherUnion&& __other)
+ requires(!__put_flag_in_tail)
+ : __union_(__conditional_no_unique_address_invoke_tag{},
+ [&] { return __make_union(__has_val, std::forward<_OtherUnion>(__other)); }),
+ __has_val_(__has_val) {}
+
+ _LIBCPP_HIDE_FROM_ABI constexpr __repr(const __repr&) = delete;
+ _LIBCPP_HIDE_FROM_ABI constexpr __repr(const __repr&)
+ requires(is_copy_constructible_v<_Tp> && is_copy_constructible_v<_Err> &&
+ is_trivially_copy_constructible_v<_Tp> && is_trivially_copy_constructible_v<_Err>)
+ = default;
+ _LIBCPP_HIDE_FROM_ABI constexpr __repr(__repr&&) = delete;
+ _LIBCPP_HIDE_FROM_ABI constexpr __repr(__repr&&)
+ requires(is_move_constructible_v<_Tp> && is_move_constructible_v<_Err> &&
+ is_trivially_move_constructible_v<_Tp> && is_trivially_move_constructible_v<_Err>)
+ = default;
+
+ _LIBCPP_HIDE_FROM_ABI constexpr __repr& operator=(const __repr&) = delete;
+ _LIBCPP_HIDE_FROM_ABI constexpr __repr& operator=(__repr&&) = delete;
+
+ _LIBCPP_HIDE_FROM_ABI constexpr ~__repr()
+ requires(is_trivially_destructible_v<_Tp> && is_trivially_destructible_v<_Err>)
+ = default;
+
+ _LIBCPP_HIDE_FROM_ABI constexpr ~__repr()
+ requires(!is_trivially_destructible_v<_Tp> || !is_trivially_destructible_v<_Err>)
+ {
+ __destroy_union_member();
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr void __destroy_union()
+ requires(!__put_flag_in_tail && (is_trivially_destructible_v<_Tp> && is_trivially_destructible_v<_Err>))
+ {
+ std::destroy_at(&__union_.__v);
+ }
+
+ _LIBCPP_HIDE_FROM_ABI constexpr void __destroy_union()
+ requires(!__put_flag_in_tail && (!is_trivially_destructible_v<_Tp> || !is_trivially_destructible_v<_Err>))
+ {
+ __destroy_union_member();
+ std::destroy_at(&__union_.__v);
+ }
+
+ template <class... _Args>
+ _LIBCPP_HIDE_FROM_ABI constexpr void __construct_union(in_place_t, _Args&&... __args)
+ requires(!__put_flag_in_tail)
+ {
+ std::construct_at(&__union_.__v, in_place, std::forward<_Args>(__args)...);
+ __has_val_ = true;
+ }
+
+ template <class... _Args>
+ _LIBCPP_HIDE_FROM_ABI constexpr void __construct_union(unexpect_t, _Args&&... __args)
+ requires(!__put_flag_in_tail)
+ {
+ std::construct_at(&__union_.__v, unexpect, std::forward<_Args>(__args)...);
+ __has_val_ = false;
+ }
+
+ private:
+ template <class, class>
+ friend class __expected_base;
+
+ _LIBCPP_HIDE_FROM_ABI constexpr void __destroy_union_member()
+ requires(!is_trivially_destructible_v<_Tp> || !is_trivially_destructible_v<_Err>)
+ {
+ if (__has_val_) {
+ std::destroy_at(std::addressof(__union_.__v.__val_));
+ } else {
+ std::destroy_at(std::addressof(__union_.__v.__unex_));
+ }
+ }
+
+ template <class _OtherUnion>
+ _LIBCPP_HIDE_FROM_ABI static constexpr __union_t __make_union(bool __has_val, _OtherUnion&& __other)
+ requires(!__put_flag_in_tail)
+ {
+ if (__has_val)
+ return __union_t(in_place, std::forward<_OtherUnion>(__other).__val_);
+ else
+ return __union_t(unexpect, std::forward<_OtherUnion>(__other).__unex_);
+ }
+
+ _LIBCPP_NO_UNIQUE_ADDRESS __conditional_no_unique_address<__put_flag_in_tail, __union_t> __union_;
+ _LIBCPP_NO_UNIQUE_ADDRESS bool __has_val_;
+ };
+
+ template <class _OtherUnion>
+ _LIBCPP_HIDE_FROM_ABI static constexpr __repr __make_repr(bool __has_val, _OtherUnion&& __other)
+ requires(__put_flag_in_tail)
+ {
+ if (__has_val)
+ return __repr(in_place, std::forward<_OtherUnion>(__other).__val_);
+ else
+ return __repr(unexpect, std::forward<_OtherUnion>(__other).__unex_);
+ }
+
+protected:
+ template <class... _Args>
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit __expected_base(_Args&&... __args)
----------------
huixie90 wrote:
I was about to suggest to have some tag (e.g. `in_place_t`) as the first argument to prevent problems like this
https://godbolt.org/z/cn6d38vn4
However, this would make the caller bit weird , i.e. `__base(in_place, in_place, args...)`
what do you think?
https://github.com/llvm/llvm-project/pull/69673
More information about the libcxx-commits
mailing list