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

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

As a result, 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.

This patch fixes the problem by ensuring that std::expected has no tail
padding, which is achieved by conditional usage of [[no_unique_address]]
based on the tail padding that this would create.

This is an ABI breaking change because the following property changes:

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

Before the change, this relation didn't hold. After the change, the relation
does hold, which means that the size of std::expected in these cases increases
after this patch. 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;
  };

The vendor communication is handled in #70820.
Fixes: #70494

Co-authored-by: philnik777 <nikolasklauser@berlin.de>
Co-authored-by: Louis Dionne <ldionne.2@gmail.com>

Author: 3 people

libcxx/docs/ReleaseNotes/18.rst

@@ -284,6 +284,28 @@ ABI Affecting Changes
   against different configurations of it being used in different translation
   units.
 
+- The amount of padding bytes available for use at the end of certain ``std::expected`` instantiations has changed in this
+  release. This is an ABI break for any code that held a ``std::expected`` member with ``[[no_unique_address]]`` in an
+  ABI-facing type. In those cases, the layout of the enclosing type will change, breaking the ABI. However, the
+  ``std::expected<T, E>`` member requires a few characteristics in order to be affected by this change:
+
+  - A type equivalent to ``union {T ; E}`` needs to have more than one byte of padding available.
+  - The ``std::expected<T, E>`` member must have been in a situation where its padding bytes were previously reused by
+    another object, which can happen in a few cases (this is probably not exhaustive):
+
+    - It is a member with ``[[no_unique_address]]`` applied to it, and it is followed by another data member, or
+    - It is a member with ``[[no_unique_address]]`` applied to it, and it is the last member of the user-defined type,
+      and that user-defined type is used in ways that its padding bytes can be reused, or
+    - It is inherited from
+
+  We expect that this will not be a very frequent occurrence. However, there is unfortunately no technique we can use
+  in the library to catch such misuse. Indeed, even applying an ABI tag to ``std::expected`` would not help since ABI
+  tags are not propagated to containing types. As a result, if you notice very difficult to explain bugs around the
+  usage of a ``std::expected``, you should consider checking whether you are hitting this ABI break. This change was
+  done to fix `#70494 <https://github.com/llvm/llvm-project/issues/70494>`_ and the vendor communication is handled
+  in `#70820 <https://github.com/llvm/llvm-project/issues/70820>`_.
+
+
 Build System Changes
 --------------------
 

libcxx/include/__expected/expected.h

@@ -12,7 +12,10 @@
 
 // test [[no_unique_address]] is applied to the union
 
+#include <__type_traits/datasizeof.h>
 #include <expected>
+#include <optional>
+#include <memory>
 
 struct Empty {};
 
@@ -23,13 +26,49 @@ struct A {
 
 struct B : public A {
   int z_;
+  short z2_;
   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>));
+static_assert(sizeof(std::expected<Empty, B>) == sizeof(B));
 static_assert(sizeof(std::expected<A, Empty>) == 2 * sizeof(int) + alignof(std::expected<A, Empty>));
 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>));
+static_assert(sizeof(std::expected<B, Empty>) == sizeof(B));
+static_assert(sizeof(std::expected<B, B>) == sizeof(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);
+
+// Test with some real types.
+static_assert(sizeof(std::expected<std::optional<int>, int>) == 8);
+static_assert(std::__libcpp_datasizeof<std::expected<std::optional<int>, int>>::value == 8);
+
+static_assert(sizeof(std::expected<int, std::optional<int>>) == 8);
+static_assert(std::__libcpp_datasizeof<std::expected<int, std::optional<int>>>::value == 8);
+
+static_assert(sizeof(std::expected<int, int>) == 8);
+static_assert(std::__libcpp_datasizeof<std::expected<int, int>>::value == 5);
+
+// clang-format off
+static_assert(std::__libcpp_datasizeof<int>::value == 4);
+static_assert(std::__libcpp_datasizeof<std::expected<int, int>>::value == 5);
+static_assert(std::__libcpp_datasizeof<std::expected<std::expected<int, int>, int>>::value == 8);
+static_assert(std::__libcpp_datasizeof<std::expected<std::expected<std::expected<int, int>, int>, int>>::value == 9);
+static_assert(std::__libcpp_datasizeof<std::expected<std::expected<std::expected<std::expected<int, int>, int>, int>, int>>::value == 12);
+// clang-format on

libcxx/test/libcxx/utilities/expected/expected.expected/no_unique_address.compile.pass.cpp

@@ -7,11 +7,15 @@
 //===----------------------------------------------------------------------===//
 
 // Clang-18 fixed some spurious clang diagnostics. Once clang-18 is the
-// minumum required version these obsolete tests can be removed.
+// minimum required version these obsolete tests can be removed.
 // TODO(LLVM-20) remove spurious clang diagnostic tests.
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
 
+// With clang-cl, some warnings have a 'which is a Microsoft extension' suffix
+// which break the tests.
+// XFAIL: msvc
+
 // Test the mandates
 
 // template<class F> constexpr auto transform_error(F&& f) &;
@@ -52,6 +56,7 @@ void test() {
     e.transform_error(return_unexpected<int&>); // expected-error-re@*:* {{static assertion failed {{.*}}The result of {{.*}} must be a valid template argument for unexpected}}
     // expected-error-re@*:* 0-1 {{{{(excess elements in struct initializer|no matching constructor for initialization of)}}{{.*}}}}
     // expected-error-re@*:* {{static assertion failed {{.*}}[expected.object.general] A program that instantiates the definition of template expected<T, E> for {{.*}} is ill-formed.}}
+    // expected-error-re@*:* {{union member {{.*}} has reference type {{.*}}}}
 
     e.transform_error(return_no_object<int&>); // expected-error-re@*:* {{static assertion failed {{.*}}The result of {{.*}} must be a valid template argument for unexpected}}
     // expected-error-re@*:* 0-1 {{{{(excess elements in struct initializer|no matching constructor for initialization of)}}{{.*}}}}

libcxx/test/libcxx/utilities/expected/expected.expected/transform_error.mandates.verify.cpp

@@ -11,8 +11,13 @@
 // XFAIL: msvc
 
 // test [[no_unique_address]] is applied to the union
+// In particular, we ensure that we reuse tail padding in the T
+// to store the discriminator whenever we can.
 
+#include <__type_traits/datasizeof.h>
 #include <expected>
+#include <optional>
+#include <memory>
 
 struct Empty {};
 
@@ -23,9 +28,40 @@ struct A {
 
 struct B : public A {
   int z_;
+  short z2_;
   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>));
+static_assert(sizeof(std::expected<void, B>) == sizeof(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);
+
+// Test with some real types.
+static_assert(sizeof(std::expected<void, std::optional<int>>) == 8);
+static_assert(std::__libcpp_datasizeof<std::expected<void, std::optional<int>>>::value == 8);
+
+static_assert(sizeof(std::expected<void, int>) == 8);
+static_assert(std::__libcpp_datasizeof<std::expected<void, int>>::value == 5);
+
+// clang-format off
+static_assert(std::__libcpp_datasizeof<int>::value == 4);
+static_assert(std::__libcpp_datasizeof<std::expected<void, int>>::value == 5);
+static_assert(std::__libcpp_datasizeof<std::expected<void, std::expected<void, int>>>::value == 8);
+static_assert(std::__libcpp_datasizeof<std::expected<void, std::expected<void, std::expected<void, int>>>>::value == 9);
+static_assert(std::__libcpp_datasizeof<std::expected<void, std::expected<void, std::expected<void, std::expected<void, int>>>>>::value == 12);
+// clang-format on

libcxx/test/libcxx/utilities/expected/expected.void/no_unique_address.compile.pass.cpp

@@ -12,6 +12,10 @@
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
 
+// With clang-cl, some warnings have a 'which is a Microsoft extension' suffix
+// which break the tests.
+// XFAIL: msvc
+
 // Test the mandates
 
 // template<class F> constexpr auto transform_error(F&& f) &;
@@ -52,6 +56,8 @@ void test() {
     e.transform_error(return_unexpected<int&>); // expected-error-re@*:* {{static assertion failed {{.*}}The result of {{.*}} must be a valid template argument for unexpected}}
     // expected-error-re@*:* 0-1 {{{{(excess elements in struct initializer|no matching constructor for initialization of)}}{{.*}}}}
     // expected-error-re@*:* {{static assertion failed {{.*}}A program that instantiates expected<T, E> with a E that is not a valid argument for unexpected<E> is ill-formed}}
+    // expected-error-re@*:* {{call to deleted constructor of {{.*}}}}
+    // expected-error-re@*:* {{union member {{.*}} has reference type {{.*}}}}
 
     e.transform_error(return_no_object<int&>); // expected-error-re@*:* {{static assertion failed {{.*}}The result of {{.*}} must be a valid template argument for unexpected}}
     // expected-error-re@*:* 0-1 {{{{(excess elements in struct initializer|no matching constructor for initialization of)}}{{.*}}}}

libcxx/test/libcxx/utilities/expected/expected.void/transform_error.mandates.verify.cpp

@@ -310,6 +310,20 @@ constexpr bool test() {
     assert(e.value().j == 8);
   }
 
+  // CheckForInvalidWrites
+  {
+    {
+      CheckForInvalidWrites<true> e1(std::unexpect);
+      e1 = 42;
+      assert(e1.check());
+    }
+    {
+      CheckForInvalidWrites<false> e1(std::unexpect);
+      e1 = true;
+      assert(e1.check());
+    }
+  }
+
   return true;
 }
 

libcxx/test/std/utilities/expected/expected.expected/assign/assign.U.pass.cpp

@@ -240,6 +240,29 @@ constexpr bool test() {
     assert(e1.error().data_ == 10);
     assert(oldState.copyAssignCalled);
   }
+
+  // CheckForInvalidWrites
+  {
+    {
+      CheckForInvalidWrites<true> e1(std::unexpect);
+      CheckForInvalidWrites<true> e2;
+
+      e1 = e2;
+
+      assert(e1.check());
+      assert(e2.check());
+    }
+    {
+      CheckForInvalidWrites<false> e1(std::unexpect);
+      CheckForInvalidWrites<false> e2;
+
+      e1 = e2;
+
+      assert(e1.check());
+      assert(e2.check());
+    }
+  }
+
   return true;
 }
 

libcxx/test/std/utilities/expected/expected.expected/assign/assign.copy.pass.cpp

@@ -258,6 +258,29 @@ constexpr bool test() {
     assert(e1.error().data_ == 10);
     assert(oldState.moveAssignCalled);
   }
+
+  // CheckForInvalidWrites
+  {
+    {
+      CheckForInvalidWrites<true> e1(std::unexpect);
+      CheckForInvalidWrites<true> e2;
+
+      e1 = std::move(e2);
+
+      assert(e1.check());
+      assert(e2.check());
+    }
+    {
+      CheckForInvalidWrites<false> e1(std::unexpect);
+      CheckForInvalidWrites<false> e2;
+
+      e1 = std::move(e2);
+
+      assert(e1.check());
+      assert(e2.check());
+    }
+  }
+
   return true;
 }
 

libcxx/test/std/utilities/expected/expected.expected/assign/assign.move.pass.cpp

@@ -80,6 +80,20 @@ constexpr bool test() {
     assert(e.has_value());
   }
 
+  // CheckForInvalidWrites
+  {
+    {
+      CheckForInvalidWrites<true> e;
+      e.emplace();
+      assert(e.check());
+    }
+    {
+      CheckForInvalidWrites<false> e;
+      e.emplace();
+      assert(e.check());
+    }
+  }
+
   return true;
 }