[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>
This commit is contained in:
Jan Kokemüller 2024-01-22 15:05:39 +01:00 committed by GitHub
parent bf7b8dae06
commit 4f4690530e
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
20 changed files with 1210 additions and 410 deletions

View File

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

File diff suppressed because it is too large Load Diff

View File

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

View File

@ -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)}}{{.*}}}}

View File

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

View File

@ -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)}}{{.*}}}}

View File

@ -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;
}

View File

@ -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;
}

View File

@ -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;
}

View File

@ -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;
}

View File

@ -9,8 +9,8 @@
// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
// GCC has a issue for `Guaranteed copy elision for potentially-overlapping non-static data members`,
// please refer to: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108333, but we have a workaround to
// avoid this issue.
// please refer to: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108333
// XFAIL: gcc-13
// <expected>

View File

@ -9,8 +9,8 @@
// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
// GCC has a issue for `Guaranteed copy elision for potentially-overlapping non-static data members`,
// please refer to: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108333, but we have a workaround to
// avoid this issue.
// please refer to: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108333.
// XFAIL: gcc-13
// <expected>

View File

@ -227,6 +227,28 @@ constexpr bool test() {
}
}
// CheckForInvalidWrites
{
{
CheckForInvalidWrites<true> x(std::unexpect);
CheckForInvalidWrites<true> y;
x.swap(y);
assert(x.check());
assert(y.check());
}
{
CheckForInvalidWrites<false> x(std::unexpect);
CheckForInvalidWrites<false> y;
x.swap(y);
assert(x.check());
assert(y.check());
}
}
return true;
}

View File

@ -99,6 +99,28 @@ constexpr bool test() {
assert(state.copyAssignCalled);
}
// CheckForInvalidWrites
{
{
CheckForInvalidWrites<true, true> e1;
CheckForInvalidWrites<true, true> e2(std::unexpect);
e1 = e2;
assert(e1.check());
assert(e2.check());
}
{
CheckForInvalidWrites<false, true> e1;
CheckForInvalidWrites<false, true> e2(std::unexpect);
e1 = e2;
assert(e1.check());
assert(e2.check());
}
}
return true;
}

View File

@ -125,6 +125,28 @@ constexpr bool test() {
assert(state.moveAssignCalled);
}
// CheckForInvalidWrites
{
{
CheckForInvalidWrites<true, true> e1;
CheckForInvalidWrites<true, true> e2(std::unexpect);
e1 = std::move(e2);
assert(e1.check());
assert(e2.check());
}
{
CheckForInvalidWrites<false, true> e1;
CheckForInvalidWrites<false, true> e2(std::unexpect);
e1 = std::move(e2);
assert(e1.check());
assert(e2.check());
}
}
return true;
}

View File

@ -91,6 +91,22 @@ constexpr bool test() {
assert(state1.copyAssignCalled);
}
// CheckForInvalidWrites
{
{
CheckForInvalidWrites<true, true> e;
std::unexpected<int> un(std::in_place, 42);
e = un;
assert(e.check());
}
{
CheckForInvalidWrites<false, true> e;
std::unexpected<bool> un(std::in_place, true);
e = un;
assert(e.check());
}
}
return true;
}

View File

@ -172,6 +172,23 @@ constexpr bool test() {
assert(e1.error().data_ == 10);
assert(oldState.moveAssignCalled);
}
// CheckForInvalidWrites
{
{
CheckForInvalidWrites<true, true> e;
std::unexpected<int> un(std::in_place, 42);
e = std::move(un);
assert(e.check());
}
{
CheckForInvalidWrites<false, true> e;
std::unexpected<bool> un(std::in_place, true);
e = std::move(un);
assert(e.check());
}
}
return true;
}

View File

@ -9,8 +9,8 @@
// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
// GCC has a issue for `Guaranteed copy elision for potentially-overlapping non-static data members`,
// please refer to: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108333, but we have a workaround to
// avoid this issue.
// please refer to: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108333
// XFAIL: gcc-13
// <expected>

View File

@ -139,6 +139,28 @@ constexpr bool test() {
assert(y.has_value());
}
// CheckForInvalidWrites
{
{
CheckForInvalidWrites<true, true> x(std::unexpect);
CheckForInvalidWrites<true, true> y;
x.swap(y);
assert(x.check());
assert(y.check());
}
{
CheckForInvalidWrites<false, true> x(std::unexpect);
CheckForInvalidWrites<false, true> y;
x.swap(y);
assert(x.check());
assert(y.check());
}
}
return true;
}

View File

@ -199,4 +199,141 @@ static_assert(!std::is_trivially_move_constructible_v<TailClobbererNonTrivialMov
static_assert(std::is_nothrow_move_constructible_v<TailClobbererNonTrivialMove<0, true>>);
static_assert(!std::is_nothrow_move_constructible_v<TailClobbererNonTrivialMove<0, false>>);
// The `CheckForInvalidWrites` class recreates situations where other objects
// may be placed into a `std::expected`'s tail padding (see
// https://github.com/llvm/llvm-project/issues/70494). With a template
// parameter `WithPaddedExpected` two cases can be tested:
//
// 1. The `std::expected<T, E>` itself has padding, because `T`/`E` _don't_
// have tail padding. This is modelled by `CheckForInvalidWrites<true>`
// which has a (potential) data layout like this:
//
// +- `expected`'s "has value" flag
// |
// | +- `please_dont_overwrite_me`
// | |
// /---int---\ | /----------^-------\ //
// 00 00 00 00 01 01 01 01 01 01 01 01
// \--v---/
// |
// |
// +- `expected`'s tail padding which
// gets repurposed by `please_dont_overwrite_me`
//
// 2. There is tail padding in the union of `T` and `E` which means the
// "has value" flag can be put into this tail padding. In this case, the
// `std::expected` itself _must not_ have any tail padding as it may get
// overwritten on mutating operations such as `emplace()`. This case is
// modelled by `CheckForInvalidWrites<false>` with a (potential) data
// layout like this:
//
// +- bool
// | +- please_dont_overwrite_me
// | +- "has value" flag |
// | | /--------^---------\ //
// 00 00 00 00 00 00 00 00 01 01 01 01 01 01 01 00
// \---padding-----/ |
// +- `CheckForInvalidWrites`
// padding
//
// Note that other implementation strategies are viable, including one that
// doesn't make use of `[[no_unique_address]]`. But if an implementation uses
// the strategy above, it must make sure that those tail padding bytes are not
// overwritten improperly on operations such as `emplace()`.
struct BoolWithPadding {
constexpr explicit BoolWithPadding() noexcept : BoolWithPadding(false) {}
constexpr BoolWithPadding(bool val) noexcept {
if (!std::is_constant_evaluated()) {
std::memset(this, 0, sizeof(*this));
}
val_ = val;
}
constexpr BoolWithPadding(const BoolWithPadding& other) noexcept : BoolWithPadding(other.val_) {}
constexpr BoolWithPadding& operator=(const BoolWithPadding& other) noexcept {
val_ = other.val_;
return *this;
}
// The previous data layout of libc++'s `expected` required `T` to be
// trivially move constructible to employ the `[[no_unique_address]]`
// optimization. To trigger bugs with the old implementation, make
// `BoolWithPadding` trivially move constructible.
constexpr BoolWithPadding(BoolWithPadding&&) = default;
private:
alignas(8) bool val_;
};
struct IntWithoutPadding {
constexpr explicit IntWithoutPadding() noexcept : IntWithoutPadding(0) {}
constexpr IntWithoutPadding(int val) noexcept {
if (!std::is_constant_evaluated()) {
std::memset(this, 0, sizeof(*this));
}
val_ = val;
}
constexpr IntWithoutPadding(const IntWithoutPadding& other) noexcept : IntWithoutPadding(other.val_) {}
constexpr IntWithoutPadding& operator=(const IntWithoutPadding& other) noexcept {
val_ = other.val_;
return *this;
}
// See comment on `BoolWithPadding`.
constexpr IntWithoutPadding(IntWithoutPadding&&) = default;
private:
int val_;
};
template <bool WithPaddedExpected, bool ExpectedVoid>
struct CheckForInvalidWritesBaseImpl;
template <>
struct CheckForInvalidWritesBaseImpl<true, false> {
using type = std::expected<IntWithoutPadding, bool>;
};
template <>
struct CheckForInvalidWritesBaseImpl<false, false> {
using type = std::expected<BoolWithPadding, bool>;
};
template <>
struct CheckForInvalidWritesBaseImpl<true, true> {
using type = std::expected<void, IntWithoutPadding>;
};
template <>
struct CheckForInvalidWritesBaseImpl<false, true> {
using type = std::expected<void, BoolWithPadding>;
};
template <bool WithPaddedExpected, bool ExpectedVoid>
using CheckForInvalidWritesBase = typename CheckForInvalidWritesBaseImpl<WithPaddedExpected, ExpectedVoid>::type;
template <bool WithPaddedExpected, bool ExpectedVoid = false>
struct CheckForInvalidWrites : public CheckForInvalidWritesBase<WithPaddedExpected, ExpectedVoid> {
constexpr CheckForInvalidWrites() = default;
constexpr CheckForInvalidWrites(std::unexpect_t)
: CheckForInvalidWritesBase<WithPaddedExpected, ExpectedVoid>(std::unexpect) {}
constexpr CheckForInvalidWrites& operator=(const CheckForInvalidWrites& other) {
CheckForInvalidWritesBase<WithPaddedExpected, ExpectedVoid>::operator=(other);
return *this;
}
constexpr CheckForInvalidWrites& operator=(CheckForInvalidWrites&& other) {
CheckForInvalidWritesBase<WithPaddedExpected, ExpectedVoid>::operator=(std::move(other));
return *this;
}
using CheckForInvalidWritesBase<WithPaddedExpected, ExpectedVoid>::operator=;
const bool please_dont_overwrite_me[7] = {true, true, true, true, true, true, true};
constexpr bool check() {
for (bool i : please_dont_overwrite_me) {
if (!i) {
return false;
}
}
return true;
}
};
#endif // TEST_STD_UTILITIES_EXPECTED_TYPES_H