[libc++] Ensure that `std::expected` has no tail padding #69673

jiixyj · 2023-10-20T02:07:50Z

Currently std::expected can have some padding bytes in its tail due to [[no_unique_address]]. Those padding bytes can be used by other objects. For example, in the current implementation:

sizeof(std::expected<std::optional<int>, bool>) == sizeof(std::expected<std::expected<std::optional<int>, bool>, bool>);

...so the data layout of an std::expected<std::expected<std::optional<int>, bool>, bool> can look like this:

            +-- optional "has value" flag
            |        +--padding
/---int---\ |        |
00 00 00 00 01 00 00 00
               |  |
               |  +- "outer" expected "has value" flag
               |
               +- expected "has value" flag

This is problematic because emplace()ing the "inner" expected can not only overwrite the "inner" expected "has value" flag (issue #68552) but also the tail padding where other objects might live.

@philnik777 proposed to add a char __padding_[] array to the end of the expected to make sure that the expected itself never has any tail padding bytes that might get used by other objects.

This is an ABI breaking change because
sizeof(std::expected<std::optional<int>, bool>) < sizeof(std::expected<std::expected<std::optional<int>, bool>, bool>); afterwards. The data layout will change in the following cases where tail padding can be reused by other objects:

class foo : std::expected<std::optional<int>, bool> {
  bool b;
};

or using [[no_unique_address]]:

struct foo {
  [[no_unique_address]] std::expected<std::optional<int>, bool> e;
  bool b;
};

Fixes: #70494

jiixyj · 2023-10-20T02:17:11Z

Some questions / notes:

I zero initialize the new padding arrays (otherwise the compiler generated copy constructors would be UB I guess?). The bytes in those arrays might get overwritten by subsequent constructor/destructor calls in methods like emplace() but this should be OK.
Is there an easier way to calculate the required padding bytes?
The test(s) that assert sizeof(expected) == datasizeof(expected) should probably live in new files, maybe called test/std/utilities/expected/expected.expected/datasize.pass.cpp and expected.void/datasize.pass.cpp?

jiixyj · 2023-10-20T02:19:05Z

Also, since this would be an ABI break, I guess there is a bit more process involved to get something like this in?

jiixyj · 2023-10-20T03:20:49Z

GNU anonymous struct members seem to work great for this use case:

#include <memory>

//

struct bp {
   private:
    alignas(4) bool b_;
};
static_assert(sizeof(bp) == 4);
static_assert(std::__libcpp_datasizeof<bp>::value == 1);

//

struct s1 {
    s1() : bp_{}, b_{} {};

   private:
    [[no_unique_address]] bp bp_;
    bool b_;
};
static_assert(sizeof(s1) == 4);
static_assert(std::__libcpp_datasizeof<s1>::value == 2);  // bad :(

//

struct s2 {
    s2() : bp_{}, b_{} {};

   private:
    struct {
        [[no_unique_address]] bp bp_;
        bool b_;
    };
};
static_assert(sizeof(s2) == 4);
static_assert(std::__libcpp_datasizeof<s2>::value == 4);  // good :)

https://godbolt.org/z/EenY54E4r

Sadly this is not portable...

frederick-vs-ja · 2023-10-20T05:48:00Z

It seems that when __libcpp_datasizeof<__union_t<_Tp, _Err>>::value == sizeof(__union_t<_Tp, _Err>), there shouldn't be a padding array, because reconstruction of the union member won't overwrite the bool flag or a user-provided member.

Does an unnamed bit-field work?

jiixyj · 2023-10-20T06:02:46Z

Does an unnamed bit-field work?

It looks like it does!

struct s3 {
    s3() : bp_{}, b_{} {};

   private:
    [[no_unique_address]] bp bp_;
    bool b_;
    int : 0;
};
static_assert(sizeof(s3) == 4);
static_assert(std::__libcpp_datasizeof<s3>::value == 4);  // good :)

philnik777 · 2023-10-20T09:05:47Z

libcxx/include/__expected/expected.h

+      _LIBCPP_NO_UNIQUE_ADDRESS __union_t<_Tp, _Err> __union_;
+      bool __has_val_;
+    };
+    return sizeof(__calc_expected) - __libcpp_datasizeof<__calc_expected>::value;


Why can't we just use sizeof(expected) - __libcpp_datasizeof<expected>::value?

At this point the full definition of expected is not available yet, so I'm getting errors like:

__expected/expected.h:960:12: error: invalid application of 'sizeof' to an incomplete type 'expected<TracedBase<false, false>, TracedBase<true>>' # | 960 | return sizeof(expected) - __libcpp_datasizeof<expected>::value; # | | ^~~~~~~~~~~~~~~~

It seems we can determine the size of the padding via only sizeof(__union_t<_Tp, _Err>) and __libcpp_datasizeof<__union_t<_Tp, _Err>>::value without inventing a new type.

philnik777 · 2023-10-20T09:08:30Z

libcxx/include/__expected/expected.h

  _LIBCPP_NO_UNIQUE_ADDRESS __union_t<_Tp, _Err> __union_;
  bool __has_val_;
+  char __padding_[__calculate_padding()]{};


Could we try to allow tail padding inside expected if the subobject didn't have any? e.g. it would be fine if expected<int, int> has tail padding, since the subobject can't overwrite any of the expected tail padding.

ldionne

I zero initialize the new padding arrays (otherwise the compiler generated copy constructors would be UB I guess?). The bytes in those arrays might get overwritten by subsequent constructor/destructor calls in methods like emplace() but this should be OK.

Ah, good point. Maybe worth a one-line comment somewhere since I was actually about to ask a question about that.

The test(s) that assert sizeof(expected) == datasizeof(expected) should probably live in new files, maybe called test/std/utilities/expected/expected.expected/datasize.pass.cpp and expected.void/datasize.pass.cpp?

They should be in libcxx/test/libcxx/<...> since that is where we store our libc++ specific tests.

ldionne · 2023-10-20T16:20:30Z

libcxx/include/__expected/expected.h

@@ -955,8 +956,17 @@ class expected {
    _LIBCPP_NO_UNIQUE_ADDRESS _ErrorType __unex_;
  };

+  _LIBCPP_HIDE_FROM_ABI static constexpr auto __calculate_padding() {


This won't work if there is no tail padding, since we'll end up creating a char __padding_[0]; below. This is the case for e.g. std::expected<char, char>. Needs tests.

I implemented an alternative approach to calculate the padding type that avoids zero-length arrays (still wondering if there is a more elegant way, though). I also added some tests.

zygoloid · 2023-10-20T17:58:28Z

We could consider adding a Clang extension to support this case, to avoid the ABI change here and maintain the compact layout. Would that be useful to libc++, or would you need (at least) for GCC to implement the same extension before you could make use of it?

jiixyj · 2023-10-20T18:01:38Z

Does an unnamed bit-field work?

It looks like it does!

I couldn't get the unnamed bit-field idea to work sadly. I couldn't reliably calculate the needed bits in char : __calculate_padding() * 8;. Something was always off a few bytes...

zygoloid

I don't think that filling in the tail padding is sufficient here. Once we fix Clang to properly implement the "transparent replacement" rule for std::construct_at, constant evaluation of expected::emplace is going to stop working, and this change doesn't help there.

If you don't want to rely on a language extension, the rule you need to conform to is that you can't transparently replace a potentially overlapping subboject -- you can't use construct_at on a base class or a [[no_unique_address]] member.

zygoloid · 2023-10-20T18:23:55Z

If you don't want to rely on a language extension, the rule you need to conform to is that you can't transparently replace a potentially overlapping subboject -- you can't use construct_at on a base class or a [[no_unique_address]] member.

Here's one approach you could take:

template<typename T, typename E> struct expected {
  struct repr_type {
    [[no_unique_address]] union {
      [[no_unique_address]] T value;
      [[no_unique_address]] E error;
    };
    bool is_error;
  } repr;
};

... then apply std::construct_at to the repr member from emplace.

huixie90 · 2023-10-20T18:29:30Z

I had more thought about it and still a bit unsure about manually adding char arrays as paddings.

Imagine one day we have std::atomic<std::expected<int, int>> (today it is not working because std::expected<int,int>::operator=(const std::expected&) is not trivial, but maybe one day in the future standard it could be made trivial). atomic::compare_exchange_weak is going to do the memory comparison (instead of ==). As per P0528R3 (merged in c++20), the implementation needs to make objects with padding work. If we don't have these char arrays, if we implement the paper and it should just work but with char arrays, these bytes are no longer considered as padding and compare_exchange_week needs to compare these bytes too, which could be problematic.

Although this is a hypothetical problem which does not exist yet, but things like this could exist somewhere and we are just not aware of.

Had discussion with Louis today and he had an idea like this

template <class T, class U>
struct compact_pair{
    [[no_unique_address]] T t;
    [[no_unique_address]] U u;
};

template <class T, class U>
struct expected{

    union union_t{
        [[no_unique_address]] T t;
        [[no_unique_address]] U u;
    };

    compact_pair<union_t, bool> compact_pair_;
};

static_assert(sizeof(std::optional<int>) == 8);
static_assert(sizeof(expected<std::optional<int>, int>) == 8);
static_assert(sizeof(expected<expected<std::optional<int>, int>, int>) == 12);

This way we can achieve has_value can be packed into T's tail padding, and expected's tail padding won't be used by anyone else

philnik777 · 2023-10-20T18:56:48Z

@zygoloid s repr_type looks really promising. That should allow us to have a nice ABI and fix the bugs we currently have. @jiixyj could you try that version and check whether it actually fixes everything and gives us the nice ABI? (Note that you have to name the union to apply [[no_unique_address]])

jiixyj · 2023-10-20T20:20:39Z

@zygoloid s repr_type looks really promising. That should allow us to have a nice ABI and fix the bugs we currently have. @jiixyj could you try that version and check whether it actually fixes everything and gives us the nice ABI? (Note that you have to name the union to apply [[no_unique_address]])

I had a shot at the repr/compact_pair approach and at looks good! The changes compared to #68733 are in commit 7f3144c and overall pretty mechanical.

The only downside I see is that expected's padding can never be used by another object. In other words, sizeof(expected) == datasizeof(expected) is always true.

... then apply std::construct_at to the repr member from emplace.

Wouldn't std::construct_at still be invalid, technically, when called on __repr_.__union_.__val_? Isn't that still a partially overlapping subobject?

jiixyj · 2023-10-20T20:26:42Z

The only downside I see is that expected's padding can never be used by another object. In other words, sizeof(expected) == datasizeof(expected) is always true.

So right now, this test fails:

https://github.com/jiixyj/llvm-project/blob/5691b27a2dbe8ed49a328c0cd0619d5af4b930a1/libcxx/test/libcxx/utilities/expected/expected.expected/no_unique_address.compile.pass.cpp#L53

zygoloid · 2023-10-20T22:35:07Z

... then apply std::construct_at to the repr member from emplace.

Wouldn't std::construct_at still be invalid, technically, when called on __repr_.__union_.__val_? Isn't that still a partially overlapping subobject?

Yes, that would be invalid. You need to apply std::construct_at to the repr member itself, not to the value inside the union.

jiixyj · 2023-10-21T07:35:47Z

Yes, that would be invalid. You need to apply std::construct_at to the repr member itself, not to the value inside the union.

Ahh, so when emplace()ing the expected, you would always transparently replace the whole repr struct? Would the destructor of repr still be allowed to call std::destroy_at on its union member like this, even though they are partially overlapping?

~repr() {
  if (__has_val_) {
    std::destroy_at(std::addressof(__union_.__val_));
  } else {
    std::destroy_at(std::addressof(__union_.__unex_));
  }
}

jiixyj · 2023-10-21T09:56:10Z

I've tried to implement the "repr struct" strategy. On mutating operations, the whole repr struct is teared down and recreated if needed. This has also the nice side effect that the __has_value_ flag is never manually mutated any longer. The state of the union and the value of the __has_value_ flag are guaranteed to be kept in sync by repr's constructors.

Internally, the repr struct still needs to call std::construct_at/std::destroy_at sometimes on its [[no_unique_address]] members. Not sure if that is OK...

frederick-vs-ja · 2023-10-21T10:11:59Z

If you don't want to rely on a language extension, the rule you need to conform to is that you can't transparently replace a potentially overlapping subboject -- you can't use construct_at on a base class or a [[no_unique_address]] member.

Here's one approach you could take:
template<typename T, typename E> struct expected {
  struct repr_type {
    [[no_unique_address]] union {
      [[no_unique_address]] T value;
      [[no_unique_address]] E error;
    };
    bool is_error;
  } repr;
};
... then apply std::construct_at to the repr member from emplace.

It seems that such strategy will make the tail padding of expected<int, int> unreusable...

Perhaps we should use a mixed strategy:

when the bool flag lives in the tail padding of the union, the repr strategy should be used, and expected shouldn't have reusable tail padding,
otherwise, however, we should make the tail padding of expected (if any) reusable.

It's painful that we have no [[no_unique_address(condition)]] yet.

libcxx/include/__expected/expected.h

philnik777 · 2023-10-21T11:18:18Z

libcxx/include/__expected/expected.h

-  _LIBCPP_NO_UNIQUE_ADDRESS __union_t<_Tp, _Err> __union_;
-  bool __has_val_;
+  struct __expected_repr {
+    _LIBCPP_HIDE_FROM_ABI constexpr explicit __expected_repr() = delete;


Why do we need an overload set here? If we remove that and simply directly initialize the members that would make this thing a lot simpler.

Do you mean the constructor overload set? I thought since __repr needs a destructor anyways I'd create some constructors for easier use and more foolproof initialization of the __has_val_ flag. But those are not strong feelings.

What can certainly be removed from __repr and union constructors are the std::__expected_construct_in_place_from_invoke_tag/std::__expected_construct_unexpected_from_invoke_tag overloads. What do you think?

What can certainly be removed from __repr and union constructors are the std::__expected_construct_in_place_from_invoke_tag/std::__expected_construct_unexpected_from_invoke_tag overloads. What do you think?

Well, that didn't work: It seems the function really has to be threaded down into the union and only std::invoked down there...

# | In file included from /freebsd/data/linux/git-net/llvm-project/libcxx/test/std/utilities/expected/expected.expected/monadic/transform.pass.cpp:22: # | In file included from /freebsd/data/linux/git-net/llvm-project/build/include/c++/v1/expected:44: # | /freebsd/data/linux/git-net/llvm-project/build/include/c++/v1/__expected/expected.h:837:11: error: call to deleted constructor of 'NonCopy' # | 837 | : __val_(std::forward<_Args>(__args)...) {} # | | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~ # | /freebsd/data/linux/git-net/llvm-project/build/include/c++/v1/__expected/expected.h:906:11: note: in instantiation of function template specialization 'std::expected<NonCopy, int>::__union _t<NonCopy, int>::__union_t<NonCopy>' requested here # | 906 | : __union_(__tag, std::forward<_Args>(__args)...), __has_val_(true) {} # | | ^ # | /freebsd/data/linux/git-net/llvm-project/build/include/c++/v1/__expected/expected.h:173:9: note: in instantiation of function template specialization 'std::expected<NonCopy, int>::__repr:: __repr<NonCopy>' requested here # | 173 | : __repr_(in_place, std::invoke(std::forward<_Func>(__f), std::forward<_Args>(__args)...)) {} # | | ^ # | /freebsd/data/linux/git-net/llvm-project/build/include/c++/v1/__expected/expected.h:703:14: note: in instantiation of function template specialization 'std::expected<NonCopy, int>::expecte d<(lambda at /freebsd/data/linux/git-net/llvm-project/libcxx/test/std/utilities/expected/expected.expected/monadic/transform.pass.cpp:220:16) &, int &>' requested here # | 703 | return expected<_Up, _Err>(__expected_construct_in_place_from_invoke_tag{}, std::forward<_Func>(__f), __repr_.__union_.__val_);

philnik777 · 2023-10-21T11:20:51Z

libcxx/include/__expected/expected.h

@@ -909,11 +850,19 @@ class expected {
        std::__expected_construct_unexpected_from_invoke_tag, _Func&& __f, _Args&&... __args)
        : __unex_(std::invoke(std::forward<_Func>(__f), std::forward<_Args>(__args)...)) {}

+    template <class _Union>
+    _LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(bool __has_val, _Union&& __other) {


I think we should move the construct_ats outside the union and instead construct_at the __expected_repr.

Hm, then the union would need to be default constructible and have an __empty_ member again. Also, ordering issues among the union and __has_val_ flag initialization would become possible once more. What do you think?

libcxx/include/__expected/expected.h

philnik777 · 2023-10-21T11:24:12Z

libcxx/include/__expected/expected.h

@@ -1552,8 +1570,56 @@ class expected<_Tp, _Err> {
    _LIBCPP_NO_UNIQUE_ADDRESS _ErrorType __unex_;
  };

-  _LIBCPP_NO_UNIQUE_ADDRESS __union_t<_Err> __union_;
-  bool __has_val_;
+  struct __expected_repr {


Let's just name this __repr. It's already inside expected.

philnik777 · 2023-10-21T11:24:37Z

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")


Where does this change come from?

I noticed from earlier when playing around with __libcpp_datasizeof that GCC errors out on the offsetof (the CI build failed). It seems that with those diagnostic macros you need to suppress for both Clang and GCC separately because their flags may be different.

Anyway, this should probably a separate issue/PR, so I'll remove it for now.

zygoloid · 2023-10-21T23:25:12Z

Internally, the repr struct still needs to call std::construct_at/std::destroy_at sometimes on its [[no_unique_address]] members. Not sure if that is OK...

destroy_at (or a manual destructor call) is OK, but construct_at (or placement new) is not.

jiixyj · 2023-10-24T05:44:17Z

Perhaps we should use a mixed strategy:

* when the `bool` flag lives in the tail padding of the union, the repr strategy should be used, and `expected` shouldn't have reusable tail padding,

* otherwise, however, we should make the tail padding of `expected` (if any) reusable.

It's painful that we have no [[no_unique_address(condition)]] yet.

What do you think about this:

#include <expected>
#include <iostream>
#include <memory>
#include <optional>

template <typename Val, typename Err>
union expected_union {
    [[no_unique_address]] Val val;
    [[no_unique_address]] Err unex;
};

template <typename Val, typename Err, bool StuffTail = false>
struct expected_repr {
   private:
    expected_union<Val, Err> union_;
    [[no_unique_address]] bool has_val_;
};

template <typename Val, typename Err>
struct expected_repr<Val, Err, true> {
   private:
    [[no_unique_address]] expected_union<Val, Err> union_;
    [[no_unique_address]] bool has_val_;
};

template <typename Val, typename Err,  //
          typename Repr = expected_repr<Val, Err, true>,
          typename Union = expected_union<Val, Err>>
concept tail_stuffable = sizeof(Repr) == sizeof(Union);

template <typename Val, typename Err>
struct expected_base {
   protected:
    [[no_unique_address]] expected_repr<Val, Err, false> repr_;
};

template <typename Val, typename Err>
    requires tail_stuffable<Val, Err>
struct expected_base<Val, Err> {
   protected:
    expected_repr<Val, Err, true> repr_;
};

template <typename Val, typename Err>
struct expected : private expected_base<Val, Err> {};

https://godbolt.org/z/5oj6TjTGd

You would have two repr types, depending on whether the bool flag can live in the union's tail padding or not, and switch between them with inheritance (sort of emulating conditional [[no_unique_address]]).

What I like about this is that you don't even need something like std::__libcpp_datasize. You can just ask the first repr type: "Can the bool be stuffed in your tail?"

In the "tail padding" case you would std::destruct_at/std::construct_at the whole repr, including the "has value" flag. In the "normal" case you would only std::destruct_at/std::construct_at the union and set the "has value" flag manually.

I'll try to implement this to see how bad it is regarding code complexity. But I'm optimistic!

jiixyj · 2023-10-25T05:44:50Z

I've implemented the layout from comment #69673 (comment) and I think it works pretty well!

There are now two layouts:

In case the "has value" bool is stuffable into the tail padding of the union, the whole "repr" class needs to be transparently replaced on emplace()/swap() etc. Let's call it the "TRR" (transparently replaceable "repr") layout.
In case the "has value" bool is not stuffable into the tail padding of the union, only the union member must be transparently replaced and the "has value" flag must be adjusted manually. Let's call that layout "TRU" (transparently replaceable "union")

Some note:

I moved all "union" and "repr" types out of the "expected" class.
Sadly, there is some code duplication going on since there are now two repr classes instead of just one. :(
For "TRU", the "repr" class offers some member functions to help with union destruction/construction: __destroy_union and __construct_union. Those member functions also do the adjustment of the "has bool" flag.

GCC does not like to do guaranteed copy elision into [[no_unique_address]] members (Guaranteed copy elision for potentially-overlapping non-static data members itanium-cxx-abi/cxx-abi#107), so to implement expected's copy constructors there need to be two helper functions that do guaranteed copy elision, which may look weird at first:

TRU:

template <class _OtherUnion>
_LIBCPP_HIDE_FROM_ABI static constexpr __expected_union_t<_Tp, _Err>
__make_union(bool __has_val, _OtherUnion&& __other)
{
  if (__has_val)
    return __expected_union_t<_Tp, _Err>(in_place, std::forward<_OtherUnion>(__other).__val_);
  else
    return __expected_union_t<_Tp, _Err>(unexpect, std::forward<_OtherUnion>(__other).__unex_);
}

TRR:

template <class _OtherUnion>
_LIBCPP_HIDE_FROM_ABI static constexpr __repr __make_repr(bool __has_val, _OtherUnion&& __other)
{
  if (__has_val)
    return __repr(in_place, std::forward<_OtherUnion>(__other).__val_);
  else
    return __repr(unexpect, std::forward<_OtherUnion>(__other).__unex_);
}

__expected_base provides two helper functions __destroy and __construct which are called by expected's members instead of std::destroy_at and std::construct_at. The helpers then either transparently replace the repr (for TRR) or the union (for TRU).

jiixyj · 2023-10-25T06:01:37Z

One think I'm still unsure about is the layout of the union itself. Currently, it looks like this:

template <class Val, class Err>
union {
  Val val_;
  Err unex_;
};

template <class Val, class Err>
requires(is_trivially_move_constructible_v<Val> && is_trivially_move_constructible_v<Err>)
union {
  [[no_unique_address]] Val val_;
  [[no_unique_address]] Err unex_;
};

...i.e. tail padding is only ever reused if both types are trivially move constructible. I think this was done to make the layout compatible with GCC (and possibly future changes to the standard/Itanium ABI). itanium-cxx-abi/cxx-abi#107 has some good discussion.

But I wonder, why do the types need to be trivially move constructible? Wouldn't nothrow move constructible be enough? The problematic case seems to be this one: https://godbolt.org/z/1xEo1njGP There, type Foo satisfies all requirements (https://eel.is/c++draft/expected#object.assign-9.3) to be assignable to expected<Bar>, but GCC doesn't to the guaranteed copy elision into the [[no_unique_address]] member so the operation throws (!).

To fix this, wouldn't this work instead of the current approach?

template <class Val, class Err>
union u {
  Val val_;
  Err unex_;
};

template <class Val, class Err>
requires is_nothrow_move_constructible_v<Val>
union<Val, Err> u {
  [[no_unique_address]] Val val_;
  Err unex_;
};

template <class Val, class Err>
requires is_nothrow_move_constructible_v<Err>
union<Val, Err> u {
  Val val_;
  [[no_unique_address]] Err unex_;
};

template <class Val, class Err>
requires(is_nothrow_move_constructible_v<Val> && is_nothrow_move_constructible_v<Err>)
union<Val, Err> u {
  [[no_unique_address]] Val val_;
  [[no_unique_address]] Err unex_;
};

There is a bit of combinatorial explosion going on, but this the best layout I could come up with that is still compatible with GCC.

jiixyj · 2023-10-27T16:50:27Z

One think I'm still unsure about is the layout of the union itself.

I have convinced myself that making the union members unconditionally [[no_unique_address]] is the way to go. Pessimizing expected's data layout because of GCC's decision to prevent guaranteed copy elision into partially overlapping subobjects (for which there are good reasons!) is not worth it, I think. In expected's case, we control all tail padding bytes, so guaranteed copy elision into the union members is perfectly safe. Making the layout depend on move constructor triviality seems also a bit brittle to me. I have asked at the GCC bug if there can be a way for a [[no_unique_address]] member to declare "yes, please allow guaranteed copy elision into me": https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98995#c13

jiixyj · 2023-10-27T17:09:23Z

I moved all "union" and "repr" types out of the "expected" class.

Sadly, there is some code duplication going on since there are now two repr classes instead of just one. :(

I managed to fix the code duplication by making [[no_unique_address]] conditional with std::conditional. So the current layout looks like this:

template <bool NoUnique, class Tp>
class conditional_no_unique_address {
  struct unique {
    Tp v;
  };
  struct no_unique {
    [[no_unique_address]] Tp v;
  };

public:
  using type = std::conditional<NoUnique, no_unique, unique>::type;
}

// Returns true iff "has value" can be stuffed into the tail of the union.
template <class Union>
constexpr bool can_stuff_tail();

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" can be stuffed into the tail, this should be
    // `[[no_unique_address]]`, otherwise not.
    [[no_unique_address]] conditional_no_unique_address<
        can_stuff_tail<union_t>(), union_t>::type union_;
    [[no_unique_address]] bool has_val_;
  };

protected:
  // If "has value" can be stuffed 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<
      !can_stuff_tail<union_t>(), repr>::type repr_;
};

template <class Tp, class Err>
class expected : private expected_base<Tp, Err> {};

…union

…works

…form_error.pass.cpp Co-authored-by: Louis Dionne <ldionne.2@gmail.com>

… test

…rything works" This reverts commit 4bb1d3e.

…address.compile.pass.cpp Co-authored-by: Louis Dionne <ldionne.2@gmail.com>

…_put_flag_in_tail for better readability

…p test

jiixyj · 2024-01-21T04:41:54Z

Do you have a link to the failed check?

For example this one: https://buildkite.com/llvm-project/libcxx-ci/builds/32948

Ah, I forgot to put an "XFAIL: msvc" on the "expected.void" test of "transform_error.mandates.verify.cpp". It should work now, hopefully.

ldionne · 2024-01-22T13:34:35Z

The CI failure is due to FreeBSD and is unrelated to this change. Merging.

jiixyj mentioned this pull request Oct 20, 2023

[libc++] Fix UB in <expected> related to "has value" flag (#68552) #68733

Merged

philnik777 requested changes Oct 20, 2023

View reviewed changes

ldionne requested changes Oct 20, 2023

View reviewed changes

zygoloid reviewed Oct 20, 2023

View reviewed changes

philnik777 requested changes Oct 21, 2023

View reviewed changes

jiixyj force-pushed the expected-remove-tail-padding-rebased branch from b5073bb to 7b67afc Compare October 24, 2023 16:21

ldionne assigned ldionne and huixie90 Oct 27, 2023

jiixyj and others added 20 commits January 20, 2024 12:19

add comment detailing the distinction between __make_repr and __make_…

cb7a0b9

…union

add tests that check for invalid writes to potential tail padding

6cf3893

fix GCC warning about multiline comments

f6c6735

try to fix transform_error.mandates.verify.cpp test on clang-cl

41b9459

add in_place_t tag to __conditional_no_unique_address constructor

e00e21e

make __union_ unconditionally no_unique_address to see if everything …

2778371

…works

Update libcxx/test/std/utilities/expected/expected.void/monadic/trans…

6363349

…form_error.pass.cpp Co-authored-by: Louis Dionne <ldionne.2@gmail.com>

fix typo

e25009e

try to workaround clang-cl CI failure by excluding clang-cl from this…

3dafea3

… test

Revert "make __union_ unconditionally no_unique_address to see if eve…

d9f648e

…rything works" This reverts commit 4bb1d3e.

add issue link to comment on libcxx/test/std/utilities/expected/types.h

6e65e00

replace precondition comments with _LIBCPP_ASSERT_INTERNAL

1d90c61

add comment to the __expected_base class

ba2d4cd

Update libcxx/test/libcxx/utilities/expected/expected.void/no_unique_…

b6727b6

…address.compile.pass.cpp Co-authored-by: Louis Dionne <ldionne.2@gmail.com>

add comment to __conditional_no_unique_address_invoke_tag

e37d5d8

add a comment to the __repr constructor that calls __make_union

c46e26f

add comment to __destroy_union

a5ad48f

introduce bool __allow_reusing_expected_tail_padding as negation of _…

bd65b2a

…_put_flag_in_tail for better readability

use put_flag_in_tail/allow_reusing_expected_tail_padding in synopsis

b5a1798

autoformat

9664d71

jiixyj force-pushed the expected-remove-tail-padding-rebased branch from cc96bac to 9664d71 Compare January 20, 2024 11:22

add 'XFAIL: msvc' to expected.void/transform_error.mandates.verify.cp…

e82f622

…p test

use 'XFAIL: gcc-13' instead of 'XFAIL: gcc'

f4df3f9

ldionne added 2 commits January 22, 2024 09:01

Add release note

89290d5

Merge branch 'main' into expected-remove-tail-padding-rebased

3d1bcb1

ldionne merged commit 4f46905 into llvm:main Jan 22, 2024

jiixyj mentioned this pull request Mar 9, 2024

[libcxx] makes expected trivially assignable when both members are … #74768

Open

[libc++] Ensure that std::expected has no tail padding #69673

[libc++] Ensure that std::expected has no tail padding #69673

Uh oh!

Conversation

jiixyj commented Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jiixyj commented Oct 20, 2023

Uh oh!

jiixyj commented Oct 20, 2023

Uh oh!

jiixyj commented Oct 20, 2023

Uh oh!

frederick-vs-ja commented Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jiixyj commented Oct 20, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldionne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zygoloid commented Oct 20, 2023

Uh oh!

jiixyj commented Oct 20, 2023

Uh oh!

zygoloid left a comment

Choose a reason for hiding this comment

Uh oh!

zygoloid commented Oct 20, 2023

Uh oh!

huixie90 commented Oct 20, 2023

Uh oh!

philnik777 commented Oct 20, 2023

Uh oh!

jiixyj commented Oct 20, 2023

Uh oh!

jiixyj commented Oct 20, 2023

Uh oh!

zygoloid commented Oct 20, 2023

Uh oh!

jiixyj commented Oct 21, 2023

Uh oh!

jiixyj commented Oct 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frederick-vs-ja commented Oct 21, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

[libc++] Ensure that `std::expected` has no tail padding #69673

[libc++] Ensure that `std::expected` has no tail padding #69673

jiixyj commented Oct 20, 2023 •

edited

Loading

frederick-vs-ja commented Oct 20, 2023 •

edited

Loading

jiixyj commented Oct 21, 2023 •

edited

Loading