[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
Fri Oct 20 10:57:15 PDT 2023


https://github.com/jiixyj updated https://github.com/llvm/llvm-project/pull/69673

>From 74b266db78accc3f23e0cc1d96aa7277eeccdb2f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jan=20Kokem=C3=BCller?= <jan.kokemueller at gmail.com>
Date: Sun, 15 Oct 2023 22:09:51 +0200
Subject: [PATCH 1/5] ensure that expected has no tail padding

---
 libcxx/include/__expected/expected.h          | 20 +++++++++++++++++++
 .../expected.expected/assign/emplace.pass.cpp | 16 +++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/libcxx/include/__expected/expected.h b/libcxx/include/__expected/expected.h
index 045370a486fae6b..463060702235742 100644
--- a/libcxx/include/__expected/expected.h
+++ b/libcxx/include/__expected/expected.h
@@ -18,6 +18,7 @@
 #include <__memory/addressof.h>
 #include <__memory/construct_at.h>
 #include <__type_traits/conjunction.h>
+#include <__type_traits/datasizeof.h>
 #include <__type_traits/disjunction.h>
 #include <__type_traits/integral_constant.h>
 #include <__type_traits/is_assignable.h>
@@ -955,8 +956,17 @@ class expected {
     _LIBCPP_NO_UNIQUE_ADDRESS _ErrorType __unex_;
   };
 
+  _LIBCPP_HIDE_FROM_ABI static constexpr auto __calculate_padding() {
+    struct __calc_expected {
+      _LIBCPP_NO_UNIQUE_ADDRESS __union_t<_Tp, _Err> __union_;
+      bool __has_val_;
+    };
+    return sizeof(__calc_expected) - __libcpp_datasizeof<__calc_expected>::value;
+  }
+
   _LIBCPP_NO_UNIQUE_ADDRESS __union_t<_Tp, _Err> __union_;
   bool __has_val_;
+  char __padding_[__calculate_padding()]{};
 };
 
 template <class _Tp, class _Err>
@@ -1552,8 +1562,18 @@ class expected<_Tp, _Err> {
     _LIBCPP_NO_UNIQUE_ADDRESS _ErrorType __unex_;
   };
 
+
+  _LIBCPP_HIDE_FROM_ABI static constexpr auto __calculate_padding() {
+    struct __calc_expected {
+      _LIBCPP_NO_UNIQUE_ADDRESS __union_t<_Err> __union_;
+      bool __has_val_;
+    };
+    return sizeof(__calc_expected) - __libcpp_datasizeof<__calc_expected>::value;
+  }
+
   _LIBCPP_NO_UNIQUE_ADDRESS __union_t<_Err> __union_;
   bool __has_val_;
+  char __padding_[__calculate_padding()]{};
 };
 
 _LIBCPP_END_NAMESPACE_STD
diff --git a/libcxx/test/std/utilities/expected/expected.expected/assign/emplace.pass.cpp b/libcxx/test/std/utilities/expected/expected.expected/assign/emplace.pass.cpp
index c62e6289350201a..5be8d2b5fb74e40 100644
--- a/libcxx/test/std/utilities/expected/expected.expected/assign/emplace.pass.cpp
+++ b/libcxx/test/std/utilities/expected/expected.expected/assign/emplace.pass.cpp
@@ -73,6 +73,22 @@ constexpr bool test() {
     assert(e.value() == 10);
   }
 
+  // Test that `expected` has no tail padding.
+  {
+    struct BoolWithPadding {
+      explicit operator bool() { return b; }
+
+    private:
+      alignas(4) bool b = false;
+    };
+
+    static_assert(sizeof(std::expected<BoolWithPadding, bool>) ==
+                  std::__libcpp_datasizeof<std::expected<BoolWithPadding, bool>>::value);
+
+    static_assert(sizeof(std::expected<void, BoolWithPadding>) ==
+                  std::__libcpp_datasizeof<std::expected<void, BoolWithPadding>>::value);
+  }
+
   return true;
 }
 

>From 71bc5da89260fd31a284651ccf870d00f8bc41db Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jan=20Kokem=C3=BCller?= <jan.kokemueller at gmail.com>
Date: Fri, 20 Oct 2023 19:39:15 +0200
Subject: [PATCH 2/5] remove tests from emplace.pass.cpp

---
 .../expected.expected/assign/emplace.pass.cpp    | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/libcxx/test/std/utilities/expected/expected.expected/assign/emplace.pass.cpp b/libcxx/test/std/utilities/expected/expected.expected/assign/emplace.pass.cpp
index 5be8d2b5fb74e40..c62e6289350201a 100644
--- a/libcxx/test/std/utilities/expected/expected.expected/assign/emplace.pass.cpp
+++ b/libcxx/test/std/utilities/expected/expected.expected/assign/emplace.pass.cpp
@@ -73,22 +73,6 @@ constexpr bool test() {
     assert(e.value() == 10);
   }
 
-  // Test that `expected` has no tail padding.
-  {
-    struct BoolWithPadding {
-      explicit operator bool() { return b; }
-
-    private:
-      alignas(4) bool b = false;
-    };
-
-    static_assert(sizeof(std::expected<BoolWithPadding, bool>) ==
-                  std::__libcpp_datasizeof<std::expected<BoolWithPadding, bool>>::value);
-
-    static_assert(sizeof(std::expected<void, BoolWithPadding>) ==
-                  std::__libcpp_datasizeof<std::expected<void, BoolWithPadding>>::value);
-  }
-
   return true;
 }
 

>From f07fd5f821e292d49e30cd97ddfc019c8df97474 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jan=20Kokem=C3=BCller?= <jan.kokemueller at gmail.com>
Date: Fri, 20 Oct 2023 19:40:18 +0200
Subject: [PATCH 3/5] handle GCC's warning in __libcpp_datasizeof

---
 libcxx/include/__type_traits/datasizeof.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libcxx/include/__type_traits/datasizeof.h b/libcxx/include/__type_traits/datasizeof.h
index 5688e3293a69eb5..96404ad15a73376 100644
--- a/libcxx/include/__type_traits/datasizeof.h
+++ b/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")
   static const size_t value = offsetof(_FirstPaddingByte<>, __first_padding_byte_);
   _LIBCPP_DIAGNOSTIC_POP
 };

>From a17243c8d78e8ab09d60665ef08087f81e4ffd98 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jan=20Kokem=C3=BCller?= <jan.kokemueller at gmail.com>
Date: Fri, 20 Oct 2023 19:53:22 +0200
Subject: [PATCH 4/5] implement different approach to padding calculation

This tries to fix two issues:

- There should be no artificial padding if the parameter type(s) don't
  need it, for example if they have no tail padding
- Prevent the padding array being zero length
---
 libcxx/include/__expected/expected.h | 53 +++++++++++++++++-----------
 1 file changed, 32 insertions(+), 21 deletions(-)

diff --git a/libcxx/include/__expected/expected.h b/libcxx/include/__expected/expected.h
index 463060702235742..0728d3af7bb47ca 100644
--- a/libcxx/include/__expected/expected.h
+++ b/libcxx/include/__expected/expected.h
@@ -89,6 +89,34 @@ _LIBCPP_HIDE_FROM_ABI void __throw_bad_expected_access(_Arg&& __arg) {
 #  endif
 }
 
+template <size_t __Padding>
+struct __expected_padding {
+  using type = char[__Padding];
+};
+
+template <>
+struct __expected_padding<0> {
+  using type = struct {};
+};
+
+template <class _Union>
+_LIBCPP_HIDE_FROM_ABI constexpr size_t __expected_calculate_padding() {
+  struct __calc_expected {
+    _LIBCPP_NO_UNIQUE_ADDRESS _Union __union_;
+    bool __has_val_;
+  };
+
+  size_t __datasize = __libcpp_datasizeof<__calc_expected>::value;
+  return sizeof(_Union) < __datasize ? 0 : sizeof(_Union) - __datasize;
+}
+
+// An object of this type must be the last member of the `expected` class to
+// ensure `expected`'s datasize is large enough to fit the parameter type(s).
+// It should be value-initialized so it is safe to copy when `expected`'s
+// copy operators are invoked.
+template <typename _Union>
+using __expected_padding_t = __expected_padding<__expected_calculate_padding<_Union>()>::type;
+
 template <class _Tp, class _Err>
 class expected {
   static_assert(
@@ -956,17 +984,9 @@ class expected {
     _LIBCPP_NO_UNIQUE_ADDRESS _ErrorType __unex_;
   };
 
-  _LIBCPP_HIDE_FROM_ABI static constexpr auto __calculate_padding() {
-    struct __calc_expected {
-      _LIBCPP_NO_UNIQUE_ADDRESS __union_t<_Tp, _Err> __union_;
-      bool __has_val_;
-    };
-    return sizeof(__calc_expected) - __libcpp_datasizeof<__calc_expected>::value;
-  }
-
   _LIBCPP_NO_UNIQUE_ADDRESS __union_t<_Tp, _Err> __union_;
-  bool __has_val_;
-  char __padding_[__calculate_padding()]{};
+  _LIBCPP_NO_UNIQUE_ADDRESS bool __has_val_;
+  _LIBCPP_NO_UNIQUE_ADDRESS __expected_padding_t<__union_t<_Tp, _Err>> __padding_{};
 };
 
 template <class _Tp, class _Err>
@@ -1562,18 +1582,9 @@ class expected<_Tp, _Err> {
     _LIBCPP_NO_UNIQUE_ADDRESS _ErrorType __unex_;
   };
 
-
-  _LIBCPP_HIDE_FROM_ABI static constexpr auto __calculate_padding() {
-    struct __calc_expected {
-      _LIBCPP_NO_UNIQUE_ADDRESS __union_t<_Err> __union_;
-      bool __has_val_;
-    };
-    return sizeof(__calc_expected) - __libcpp_datasizeof<__calc_expected>::value;
-  }
-
   _LIBCPP_NO_UNIQUE_ADDRESS __union_t<_Err> __union_;
-  bool __has_val_;
-  char __padding_[__calculate_padding()]{};
+  _LIBCPP_NO_UNIQUE_ADDRESS bool __has_val_;
+  _LIBCPP_NO_UNIQUE_ADDRESS __expected_padding_t<__union_t<_Err>> __padding_{};
 };
 
 _LIBCPP_END_NAMESPACE_STD

>From cc8cb38f9d21262af9225d2644c52f550b0594c0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jan=20Kokem=C3=BCller?= <jan.kokemueller at gmail.com>
Date: Fri, 20 Oct 2023 19:56:59 +0200
Subject: [PATCH 5/5] add tests

---
 .../no_unique_address.compile.pass.cpp          | 17 +++++++++++++++++
 .../no_unique_address.compile.pass.cpp          | 15 +++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/libcxx/test/libcxx/utilities/expected/expected.expected/no_unique_address.compile.pass.cpp b/libcxx/test/libcxx/utilities/expected/expected.expected/no_unique_address.compile.pass.cpp
index c8c217054ae6c43..06f597d54871ee1 100644
--- a/libcxx/test/libcxx/utilities/expected/expected.expected/no_unique_address.compile.pass.cpp
+++ b/libcxx/test/libcxx/utilities/expected/expected.expected/no_unique_address.compile.pass.cpp
@@ -26,6 +26,13 @@ struct B : public A {
   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>));
@@ -33,3 +40,13 @@ static_assert(sizeof(std::expected<A, Empty>) == 2 * sizeof(int) + alignof(std::
 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>));
+
+// 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);
diff --git a/libcxx/test/libcxx/utilities/expected/expected.void/no_unique_address.compile.pass.cpp b/libcxx/test/libcxx/utilities/expected/expected.void/no_unique_address.compile.pass.cpp
index dc59a6228386bcd..29cf8dff0d2b288 100644
--- a/libcxx/test/libcxx/utilities/expected/expected.void/no_unique_address.compile.pass.cpp
+++ b/libcxx/test/libcxx/utilities/expected/expected.void/no_unique_address.compile.pass.cpp
@@ -26,6 +26,21 @@ struct B : public A {
   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>));
+
+// 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);



More information about the libcxx-commits mailing list