[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
Sat Jan 13 00:41:20 PST 2024
================
@@ -88,8 +88,289 @@ _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(_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(_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);
+}();
+
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_(__tag, std::forward<_Args>(__args)...), __has_val_(true) {}
+
+ template <class... _Args>
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit __repr(unexpect_t __tag, _Args&&... __args)
+ : __union_(__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_(__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_(__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_;
----------------
jiixyj wrote:
In commit <https://github.com/llvm/llvm-project/pull/69673/commits/fe55535db87b8de529bd43a8f45c961f5c1dfbdd> I made the `__union_` member unconditionally `[[no_unique_address]]` to see if the CI complains. Sadly, GCC 13 fails the following tests:
```
llvm-libc++-shared-gcc.cfg.in :: std/utilities/expected/expected.expected/ctor/ctor.convert.move.pass.cpp
llvm-libc++-shared-gcc.cfg.in :: std/utilities/expected/expected.expected/ctor/ctor.copy.pass.cpp
llvm-libc++-shared-gcc.cfg.in :: std/utilities/expected/expected.expected/ctor/ctor.move.pass.cpp
```
As GCC won't do guaranteed copy elision into a `[[no_unique_address]]` member it tries to fall back to move construction, but the move constructor of the `union_t` is deleted:
```
# | /home/runner/_work/llvm-project/llvm-project/build/generic-gcc/include/c++/v1/__expected/expected.h:115:9: error: use of deleted function ‘constexpr std::__1::__expected_base<_Tp, _Err>::__union_t::__union_t(std::__1::__expected_base<_Tp, _Err>::__union_t&&) [with _Tp = Data; _Err = int]’
# | 115 | : __v(std::invoke(std::forward<_Func>(__f), std::forward<_Args>(__args)...)) {}
# | | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | /home/runner/_work/llvm-project/llvm-project/build/generic-gcc/include/c++/v1/__expected/expected.h:157:37: note: declared here
# | 157 | _LIBCPP_HIDE_FROM_ABI constexpr __union_t(__union_t&&) = delete;
# | | ^~~~~~~~~
```
This is because in the current design the copy/move constructors of `expected` actually rely on guaranteed copy elision to work properly: <https://github.com/jiixyj/llvm-project/blob/fe55535db87b8de529bd43a8f45c961f5c1dfbdd/libcxx/include/__expected/expected.h#L323>
Of course, I could refactor this back to make use of `std::construct_at` instead of relying on guaranteed copy elision -- but then we would just trade one complexity for another... :/
Any suggestions or design ideas?
https://github.com/llvm/llvm-project/pull/69673
More information about the libcxx-commits
mailing list