Fix EBO on std::optional and std::variant when targeting the MSVC ABI

Committing on behalf of davidben. Reviewed as D146190

Patch originally by Jan Dörrie in https://reviews.llvm.org/D120064. I've just updated it to include tests, and update documentation that MSVC ABI is not stable.

In the current implementation both `std::optional` and `std::variant` don't perform the EBO on MSVC's ABI. This is because both classes inherit from multiple empty base classes, which breaks the EBO for MSVC. This patch fixes this issue by applying the `empty_bases` declspec attribute, which is already used to fix a similar issue for `std::tuple`.

See https://reviews.llvm.org/D120064 for discussion on MSVC ABI stability. From the discussion, libc++ doesn't have users that expect a stable ABI on MSVC. The fix is thus applied unconditionally to benefit more users. Documentation has been updated to reflect this.

Fixes https://github.com/llvm/llvm-project/issues/61095.

diff --git a/libcxx/docs/DesignDocs/ABIVersioning.rst b/libcxx/docs/DesignDocs/ABIVersioning.rst
index 3b82f3c..ad72186 100644 (file)
--- a/libcxx/docs/DesignDocs/ABIVersioning.rst
+++ b/libcxx/docs/DesignDocs/ABIVersioning.rst
@@ -22,3 +22,11 @@ Internally, each ABI-changing feature is placed under its own C++ macro,
 ``_LIBCPP_ABI_VERSION``, which is controlled by the ``LIBCXX_ABI_VERSION`` set
 at build time. Libc++ does not intend users to interact with these C++ macros
 directly.
+
+-----------------
+MSVC environments
+-----------------
+
+The exception to this is MSVC environments. Libc++ does not currently have users
+that require a stable ABI in MSVC environments, so MSVC-only changes may be
+applied unconditionally.

diff --git a/libcxx/docs/index.rst b/libcxx/docs/index.rst
index be89c17..6fa35d4 100644 (file)
--- a/libcxx/docs/index.rst
+++ b/libcxx/docs/index.rst
@@ -120,7 +120,7 @@ Target platform Target architecture       Notes
 macOS 10.9+     i386, x86_64, arm64       Building the shared library itself requires targetting macOS 10.13+
 FreeBSD 12+     i386, x86_64, arm
 Linux           i386, x86_64, arm, arm64  Only glibc-2.24 and later and no other libc is officially supported
-Windows         i386, x86_64              Both MSVC and MinGW style environments
+Windows         i386, x86_64              Both MSVC and MinGW style environments, ABI in MSVC environments is :doc:`unstable <DesignDocs/ABIVersioning>`
 AIX 7.2TL5+     powerpc, powerpc64
 =============== ========================= ============================
 

diff --git a/libcxx/include/optional b/libcxx/include/optional
index 77f52b7..a387de4 100644 (file)
--- a/libcxx/include/optional
+++ b/libcxx/include/optional
@@ -638,7 +638,7 @@ struct __is_std_optional : false_type {};
 template <class _Tp> struct __is_std_optional<optional<_Tp>> : true_type {};
 
 template <class _Tp>
-class optional
+class _LIBCPP_DECLSPEC_EMPTY_BASES optional
     : private __optional_move_assign_base<_Tp>
     , private __optional_sfinae_ctor_base_t<_Tp>
     , private __optional_sfinae_assign_base_t<_Tp>

diff --git a/libcxx/include/variant b/libcxx/include/variant
index 7dcbe1f..2ee7f48 100644 (file)
--- a/libcxx/include/variant
+++ b/libcxx/include/variant
@@ -1294,7 +1294,7 @@ using __best_match_t =
 } // namespace __variant_detail
 
 template <class... _Types>
-class _LIBCPP_TEMPLATE_VIS variant
+class _LIBCPP_TEMPLATE_VIS _LIBCPP_DECLSPEC_EMPTY_BASES variant
     : private __sfinae_ctor_base<
           __all<is_copy_constructible_v<_Types>...>::value,
           __all<is_move_constructible_v<_Types>...>::value>,

diff --git a/libcxx/test/libcxx/utilities/optional/optional.object/optional_size.pass.cpp b/libcxx/test/libcxx/utilities/optional/optional.object/optional_size.pass.cpp
new file mode 100644 (file)
index 0000000..b928ed7
--- /dev/null
+++ b/libcxx/test/libcxx/utilities/optional/optional.object/optional_size.pass.cpp
@@ -0,0 +1,31 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14
+
+// <optional>
+
+// template <class T> class optional;
+
+#include <optional>
+
+template <class T>
+struct type_with_bool {
+  T value;
+  bool has_value;
+};
+
+int main(int, char**) {
+  // Test that std::optional achieves the expected size. See https://llvm.org/PR61095.
+  static_assert(sizeof(std::optional<char>) == sizeof(type_with_bool<char>));
+  static_assert(sizeof(std::optional<int>) == sizeof(type_with_bool<int>));
+  static_assert(sizeof(std::optional<long>) == sizeof(type_with_bool<long>));
+  static_assert(sizeof(std::optional<std::size_t>) == sizeof(type_with_bool<std::size_t>));
+
+  return 0;
+}

diff --git a/libcxx/test/libcxx/utilities/variant/variant.variant/variant_size.pass.cpp b/libcxx/test/libcxx/utilities/variant/variant.variant/variant_size.pass.cpp
index a2dc58b..9011e61 100644 (file)
--- a/libcxx/test/libcxx/utilities/variant/variant.variant/variant_size.pass.cpp
+++ b/libcxx/test/libcxx/utilities/variant/variant.variant/variant_size.pass.cpp
@@ -60,6 +60,16 @@ void test_index_internals() {
   static_assert(std::__variant_npos<IndexT> == IndexLim::max(), "");
 }
 
+template <class LargestType>
+struct type_with_index {
+  LargestType largest;
+#ifdef _LIBCPP_ABI_VARIANT_INDEX_TYPE_OPTIMIZATION
+  unsigned char index;
+#else
+  unsigned int index;
+#endif
+};
+
 int main(int, char**) {
   test_index_type<unsigned char>();
   // This won't compile due to template depth issues.
@@ -67,5 +77,12 @@ int main(int, char**) {
   test_index_internals<unsigned char>();
   test_index_internals<unsigned short>();
 
+  // Test that std::variant achieves the expected size. See https://llvm.org/PR61095.
+  static_assert(sizeof(std::variant<char, char, char>) == sizeof(type_with_index<char>));
+  static_assert(sizeof(std::variant<int, int, int>) == sizeof(type_with_index<int>));
+  static_assert(sizeof(std::variant<long, long, long>) == sizeof(type_with_index<long>));
+  static_assert(sizeof(std::variant<char, int, long>) == sizeof(type_with_index<long>));
+  static_assert(sizeof(std::variant<std::size_t, std::size_t, std::size_t>) == sizeof(type_with_index<std::size_t>));
+
   return 0;
 }