[libcxx-commits] [libcxx] db38140 - Fix EBO on std::optional and std::variant when targeting the MSVC ABI
Eric Fiselier via libcxx-commits
libcxx-commits at lists.llvm.org
Thu Apr 27 18:05:33 PDT 2023
Author: Eric Fiselier
Date: 2023-04-27T21:04:04-04:00
New Revision: db381405ced7b65ad1cf8fc60bbdfb505e1ed3de
URL: https://github.com/llvm/llvm-project/commit/db381405ced7b65ad1cf8fc60bbdfb505e1ed3de
DIFF: https://github.com/llvm/llvm-project/commit/db381405ced7b65ad1cf8fc60bbdfb505e1ed3de.diff
LOG: 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.
Added:
libcxx/test/libcxx/utilities/optional/optional.object/optional_size.pass.cpp
Modified:
libcxx/docs/DesignDocs/ABIVersioning.rst
libcxx/docs/index.rst
libcxx/include/optional
libcxx/include/variant
libcxx/test/libcxx/utilities/variant/variant.variant/variant_size.pass.cpp
Removed:
################################################################################
diff --git a/libcxx/docs/DesignDocs/ABIVersioning.rst b/libcxx/docs/DesignDocs/ABIVersioning.rst
index 3b82f3cc60a44..ad7218687d55f 100644
--- 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 be89c177db3f0..6fa35d4ed40e8 100644
--- 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 77f52b757dc70..a387de473f30e 100644
--- 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 7dcbe1f8d0d49..2ee7f4891f1ab 100644
--- 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
index 0000000000000..b928ed7432791
--- /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 a2dc58bce1b64..9011e61e78808 100644
--- 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;
}
More information about the libcxx-commits
mailing list