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

[davidben]

Summary

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.

[Mordante]

Thanks for the patch!

I'm not really fond of declaring the MSVC ABI as unstable. If there is no other way, please add this information in the commit message. That way we can find back why it was decided to declare the ABI on a platform as unstable.

libcxx/include/optional

641

Why not guard this attribute with a _LIBCPP_ABI_FOO_FLAG instead of breaking the ABI unconditionally?

[mstorsjo]

In D146190#4199844, @Mordante wrote:

I'm not really fond of declaring the MSVC ABI as unstable. If there is no other way, please add this information in the commit message. That way we can find back why it was decided to declare the ABI on a platform as unstable.

FWIW, treating the libc++ ABI as unstable in MSVC environments has been considered a couple times before, but without formally spelling it out like this. See e.g. D134912 where we’d be more or less agreed upon the same already (even if that change hasn’t been landed yet).

[davidben]

Happy to do this however you all think is appropriate. I was just basing this on the discussion in https://reviews.llvm.org/D120064, which suggested folks preferred treating the MSVC ABI as unstable. (I'll update the description to link to the discussion.)

[EricWF]

libcxx/include/optional

641

Because that would prevent most users from ever getting value from it.

If we don't have any libc++ users who care about ABI compatibility with MSVC, then we should do this unconditionally.

[davidben]

libcxx/include/optional

641

Unconditional is also my preference, but I'll do whatever the project prefers. Currently assuming that's to leave it at is. (What's the process for driving that question to a conclusion?)

[Mordante]

Since @EricWF feels this is fine for Windows I don't have objections either. However I really feel we need to document this better.

libcxx/include/optional

641

When that is the rationale for forcing an ABI break on all Windows users, then let's document that in the commit message.

I think it would be good to add this rationale to the index.rst, since this is against the normal libc++ policy. I really want to avoid people thinking an unstable ABI is acceptable on all platforms.

libcxx/test/libcxx/utilities/variant/variant.variant/variant_size.pass.cpp

81

This is not needed in C++17, the same for the static_asserts below.

[davidben]

Address review comments

[davidben]

libcxx/include/optional

641

Updated the commit message.

Couldn't find a good place to work it into index.rst, as it doesn't talk about ABI much in the first place, but I added a paragraph to ABIVersioning.rst. This what you had in mind?

libcxx/test/libcxx/utilities/variant/variant.variant/variant_size.pass.cpp

81

Done. I've left the existing instances in the file alone.

[davidben]

Are folks satisfied with the state of this? What's needed to move forward with this change?

[philnik]

LGTM. I'll leave final approval to @Mordante, since he had comments.

[EricWF]

I think this is ready. It LGTM. Lets just get the CI passing.

libcxx/docs/DesignDocs/ABIVersioning.rst

26

I agree with this. Is there anyway to call it out more prominently?

[philnik]

In D146190#4256333, @EricWF wrote:

I think this is ready. It LGTM. Lets just get the CI passing.

Please wait for other people who have already commented. It's really not nice to overrule people like that, and this isn't exactly time sensitive.

[davidben]

CI failure seems unrelated. Rebasing (changed test to use std:size_t since it seems you all have started qualifying it) and reuploading in hopes that clears.

[davidben]

libcxx/docs/DesignDocs/ABIVersioning.rst

26

Happy to put it wherever you all like. :-) Any place in particular? I put it in index.rst, with the supported platforms, and this file, which talks about ABI considerations, since those seemed the natural places.

[EricWF]

In D146190#4256345, @philnik wrote:

In D146190#4256333, @EricWF wrote:

I think this is ready. It LGTM. Lets just get the CI passing.

Please wait for other people who have already commented. It's really not nice to overrule people like that, and this isn't exactly time sensitive.

Which issues are you talking about? They all seem to be resolved. There doesn't appear to be any blockers. If someone want's changes before this proceeds, there's a way to do that.
Otherwise, I feel comfortable that the objections have been addressed and this is ready to land. There's no need to wait.

[philnik]

In D146190#4256667, @EricWF wrote:

In D146190#4256345, @philnik wrote:

In D146190#4256333, @EricWF wrote:

I think this is ready. It LGTM. Lets just get the CI passing.

Please wait for other people who have already commented. It's really not nice to overrule people like that, and this isn't exactly time sensitive.

Which issues are you talking about? They all seem to be resolved. There doesn't appear to be any blockers. If someone want's changes before this proceeds, there's a way to do that.
Otherwise, I feel comfortable that the objections have been addressed and this is ready to land. There's no need to wait.

In https://reviews.llvm.org/D146190#inline-1415505 Mark voiced concerns about the documentation and he didn't have a look whether the extended documentation suffices. That's why I'd like him to have a look. While I consider them to be resolved, I think there is nothing wrong with waiting a few days until he had a look. Again, there is no need to get this in as soon as possible.

[EricWF]

@philnik I believe your actions here are unbecoming. I would ask you to reconsider the way you interact in the community.

[davidben]

Re a few days, I added more notes to the documentation March 21st, which was about three weeks ago. :-) Also the original patch is over a year old now.

Anyway, I'm happy to tweak this however folks prefer, but I'd need to know your preferences to do that. If folks are agreed on the direction and it's just documentation, I'm also happy to upload a follow-up patch to make further documentation changes if feedback comes in after the change lands.

Timing-wise, this is a blocker for us using libc++'s std::variant and std::optional in Chromium. Right now we're using the Abseil polyfills, and I'm interested in moving to the STL implementation. We also have some code we're looking to extract into a standalone library with fewer dependencies, so STL types are ideal. But without this issue fixed, the libc++ types aren't usable for us, which leaves us in a bind.

[EricWF]

OK, lets try this again. Hopefully with cooler heads.

I've reviewed the LLVM code review guidance, and am comfortable that moving forward with this change today falls well within them.
@Mordante Feel free to leave additional feedback, and we'll make sure it's address post-commit.

[philnik]

In D146190#4256881, @davidben wrote:

Re a few days, I added more notes to the documentation March 21st, which was about three weeks ago. :-) Also the original patch is over a year old now.

It's easy to sometimes miss a patch. Feel free ping after a week if you don't get feedback (for smaller patches; larger ones unfortunately always take a long time).

Anyway, I'm happy to tweak this however folks prefer, but I'd need to know your preferences to do that. If folks are agreed on the direction and it's just documentation, I'm also happy to upload a follow-up patch to make further documentation changes if feedback comes in after the change lands.

Timing-wise, this is a blocker for us using libc++'s std::variant and std::optional in Chromium. Right now we're using the Abseil polyfills, and I'm interested in moving to the STL implementation. We also have some code we're looking to extract into a standalone library with fewer dependencies, so STL types are ideal. But without this issue fixed, the libc++ types aren't usable for us, which leaves us in a bind.

I'm OK with landing this now if you commit to addressing any feedback Mark might have, since this is actually a blocker for you.

[davidben]

In D146190#4261827, @philnik wrote:

I'm OK with landing this now if you commit to addressing any feedback Mark might have, since this is actually a blocker for you.

Sure! More than happy to tweak the documentation however you all like. Also happy to tweak the code based on feedback, though my sense is folks were happy with the direction (don't promise ABI for MSVC to avoid gating this on ABI versioning), and the concerns were just about where to say it.

(But if I'm wrong and the project's consensus ends up on using ABI versioning here, happy to sort that out too.)

[ldionne]

Based on the discussion we had in D146190 where we agreed that the ABI on MSVC was not to be considered stable, I think this patch does the right thing. There are a few things I think we can improve in the patch, but the overall goal sounds good to me.

libcxx/test/libcxx/utilities/optional/optional.object/optional_size.pass.cpp

24

libcxx/test/libcxx/utilities/variant/variant.variant/variant_size.pass.cpp

67

Can't we always use unsigned char? It would be nice to avoid checking an internal macro like _LIBCPP_ABI_VARIANT_INDEX_TYPE_OPTIMIZATION from the test suite if we can.

80

[davidben]

Update bug URL

[davidben]

libcxx/test/libcxx/utilities/optional/optional.object/optional_size.pass.cpp

24

Done.

libcxx/test/libcxx/utilities/variant/variant.variant/variant_size.pass.cpp

67

I don't think that works. This is trying to model the expected size of std::variant<T> but, when _LIBCPP_ABI_VARIANT_INDEX_TYPE_OPTIMIZATION is unset, the index is unsigned int instead of unsigned char. You all, I gather, couldn't apply the optimization unconditionally due to ABI stability.

This isn't the only place in the file that needs to handle this. See ExpectEqual a few lines up in the file.

80

Done.

[Mordante]

A few documentation comments.

libcxx/docs/DesignDocs/ABIVersioning.rst

26

Can you make this a section MSVC Envirionments to make it slightly more explicit this is MSVC only.

libcxx/docs/index.rst

123

How about making unstable a link to the ABI document explaining why this ABI is unstable?

[davidben]

Documentation tweaks

[davidben]

libcxx/docs/DesignDocs/ABIVersioning.rst

26

Done. Did it in sentence case, rather than title case, to match the heading in the file, though I've noticed files are a bit inconsistent here.

libcxx/docs/index.rst

123

Done. Hopefully I got the syntax right?

[Mordante]

LGTM modulo the documentation issue. Please make sure the CI is green before landing. Note the sanitizers CI is currently broken and the AIX may also be offline. So feel free to ignore errors from these runners.

libcxx/docs/DesignDocs/ABIVersioning.rst

25

libcxx/docs/index.rst

123

123

Done. Hopefully I got the syntax right?

CI says no. I have two suggestions that I've tested locally this fixes the link.

[davidben]

Fix link

libcxx/docs/index.rst

123

That didn't work (looks like :ref: is for links within a document), but I repro'd it and fixed it. Just was missing the DesignDocs path.

[davidben]

In D146190#4278224, @Mordante wrote:

LGTM modulo the documentation issue. Please make sure the CI is green before landing. Note the sanitizers CI is currently broken and the AIX may also be offline. So feel free to ignore errors from these runners.

Ack. Though I don't think I have bits to land things, so I'll need to wait for one of you all to do it anyway. :-)

[Mordante]

In D146190#4278503, @davidben wrote:

In D146190#4278224, @Mordante wrote:

LGTM modulo the documentation issue. Please make sure the CI is green before landing. Note the sanitizers CI is currently broken and the AIX may also be offline. So feel free to ignore errors from these runners.

Ack. Though I don't think I have bits to land things, so I'll need to wait for one of you all to do it anyway. :-)

Can you provide "your name <[e-mail]>" then somebody can commit it on your behalf.

libcxx/docs/index.rst

123

Odd it works for me locally and I have used this in the past. However the CI is happy so no need to change.

[davidben]

In D146190#4287960, @Mordante wrote:

In D146190#4278503, @davidben wrote:

In D146190#4278224, @Mordante wrote:

LGTM modulo the documentation issue. Please make sure the CI is green before landing. Note the sanitizers CI is currently broken and the AIX may also be offline. So feel free to ignore errors from these runners.

Ack. Though I don't think I have bits to land things, so I'll need to wait for one of you all to do it anyway. :-)

Can you provide "your name <[e-mail]>" then somebody can commit it on your behalf.

David Benjamin <[e-mail]>

[davidben]

Friendly ping

[EricWF]

Committed in db381405ced7b65ad1cf8fc60bbdfb505e1ed3de

libcxx/docs/DesignDocs/ABIVersioning.rst

 To always use the most cutting-edge, most unstable ABI (which is currently ``2``
 but at some point will become ``3``), set the CMake option ``LIBCXX_ABI_UNSTABLE``.
 
 Internally, each ABI-changing feature is placed under its own C++ macro,
 ``_LIBCPP_ABI_XXX``. These macros' definitions are controlled by the 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.
+
+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.

libcxx/docs/index.rst

 Libc++ also supports common platforms and architectures:
 
 =============== ========================= ============================
 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 unstable
 AIX 7.2TL5+     powerpc, powerpc64
 =============== ========================= ============================
 
 Generally speaking, libc++ should work on any platform that provides a fairly complete
 C Standard Library. It is also possible to turn off parts of the library for use on
 systems that provide incomplete support.
 
 However, libc++ aims to provide a high-quality implementation of the C++ Standard

libcxx/include/optional

 
 template<class _Tp>
 class optional;
 template <class _Tp>
 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>
 {
     using __base = __optional_move_assign_base<_Tp>;
 public:
     using value_type = _Tp;
 

libcxx/include/variant

 
 template <class _Tp, class... _Types>
 using __best_match_t =
     typename invoke_result_t<_MakeOverloads<_Types...>, _Tp, _Tp>::type;
 
 } // 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>,
       private __sfinae_assign_base<
           __all<(is_copy_constructible_v<_Types> &&
                  is_copy_assignable_v<_Types>)...>::value,
           __all<(is_move_constructible_v<_Types> &&
                  is_move_assignable_v<_Types>)...>::value> {

libcxx/test/libcxx/utilities/optional/optional.object/optional_size.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// 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://github.com/llvm/llvm-project/issues/61095.
+  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;
+}

libcxx/test/libcxx/utilities/variant/variant.variant/variant_size.pass.cpp

static_assert(std::is_same_v<
       std::__variant_index_t<Lim::max()-1>,
       std::__variant_index_t<Lim::max()>
     > == ExpectEqual, "");
   using IndexT = std::__variant_index_t<Lim::max()-1>;
   using IndexLim = std::numeric_limits<IndexT>;
   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.
   //test_index_type<unsigned short>();
   test_index_internals<unsigned char>();
   test_index_internals<unsigned short>();
 
+  // Test that std::variant achieves the expected size. See https://github.com/llvm/llvm-project/issues/61095.
+  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;
 }