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

[ldionne]

Summary

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.

[jdoerrie]

Hi both, PTAL. I was surprised to find out that both std::variant<int> and std::optional<int> have a sizeof 12 in my environment, this is my attempt to fix it. Please do let me know if there are other places I need to change, or how I would write a test to prevent future regressions.

[ldionne]

In D120064#3329840, @jdoerrie wrote:

Hi both, PTAL. I was surprised to find out that both std::variant<int> and std::optional<int> have a sizeof 12 in my environment, this is my attempt to fix it. Please do let me know if there are other places I need to change, or how I would write a test to prevent future regressions.

This is an ABI break on Windows, pinging @mstorsjo.

Regarding writing a test, you can add one in the appropriate subdirectory of libcxx/test/libcxx so we only check the property when testing libc++ itself, not other standard library implementations.

[mstorsjo]

In D120064#3351148, @ldionne wrote:

In D120064#3329840, @jdoerrie wrote:

Hi both, PTAL. I was surprised to find out that both std::variant<int> and std::optional<int> have a sizeof 12 in my environment, this is my attempt to fix it. Please do let me know if there are other places I need to change, or how I would write a test to prevent future regressions.

This is an ABI break on Windows, pinging @mstorsjo.

While I do act as a vendor for the library on Windows platforms, I only do that for the MinGW configuration (which isn't affected here), so this ABI break doesn't affect me. I don't think we've had other users of the MSVC setup speak up about ABI stability though.

[mstorsjo]

In D120064#3351148, @ldionne wrote:

In D120064#3329840, @jdoerrie wrote:

Hi both, PTAL. I was surprised to find out that both std::variant<int> and std::optional<int> have a sizeof 12 in my environment, this is my attempt to fix it. Please do let me know if there are other places I need to change, or how I would write a test to prevent future regressions.

This is an ABI break on Windows, pinging @mstorsjo.

@rnk @thakis @hans Is such an ABI break in libc++ for the clang-cl case tolerable for you guys?

[rnk]

In D120064#3352256, @mstorsjo wrote:

@rnk @thakis @hans Is such an ABI break in libc++ for the clang-cl case tolerable for you guys?

Yes, we can tolerate it, but that's not a great proxy for project policy.

I think, in practice, libc++ is not the system-standard C++ library in MSVC environments. It would be reasonable for libc++ to proclaim that the MSVC libc++ ABI is unstable, for now. I expect we will discover more issues like this in the near future. We may wish to re-evaluate the placement of dllexport annotations, for example, since @smeenai informed me that they interact poorly with inlining.

[ldionne]

In D120064#3352402, @rnk wrote:

In D120064#3352256, @mstorsjo wrote:

@rnk @thakis @hans Is such an ABI break in libc++ for the clang-cl case tolerable for you guys?

Yes, we can tolerate it, but that's not a great proxy for project policy.

I think, in practice, libc++ is not the system-standard C++ library in MSVC environments. It would be reasonable for libc++ to proclaim that the MSVC libc++ ABI is unstable, for now. I expect we will discover more issues like this in the near future. We may wish to re-evaluate the placement of dllexport annotations, for example, since @smeenai informed me that they interact poorly with inlining.

Yeah, I agree with that. We could update the Windows row in https://libcxx.llvm.org/#platform-and-compiler-support to say that the ABI is unstable -- would that be reasonable?

[hans]

In D120064#3352471, @ldionne wrote:

In D120064#3352402, @rnk wrote:

In D120064#3352256, @mstorsjo wrote:

@rnk @thakis @hans Is such an ABI break in libc++ for the clang-cl case tolerable for you guys?

Yes, we can tolerate it, but that's not a great proxy for project policy.

I think, in practice, libc++ is not the system-standard C++ library in MSVC environments. It would be reasonable for libc++ to proclaim that the MSVC libc++ ABI is unstable, for now. I expect we will discover more issues like this in the near future. We may wish to re-evaluate the placement of dllexport annotations, for example, since @smeenai informed me that they interact poorly with inlining.

Yeah, I agree with that. We could update the Windows row in https://libcxx.llvm.org/#platform-and-compiler-support to say that the ABI is unstable -- would that be reasonable?

Sounds reasonable to me.

[mstorsjo]

In D120064#3359254, @hans wrote:

In D120064#3352471, @ldionne wrote:

Yeah, I agree with that. We could update the Windows row in https://libcxx.llvm.org/#platform-and-compiler-support to say that the ABI is unstable -- would that be reasonable?

Sounds reasonable to me.

At least for the MSVC/MSABI variant of it - most of these issues don't hit the mingw configuration. In the mingw configuration, there's also more use of the library as a platform library - where we'd preferrably avoid continuous ABI breakage if possible (but where it probably still can be tolerated if deemed necessary).

[rnk]

In D120064#3359263, @mstorsjo wrote:

In D120064#3359254, @hans wrote:

In D120064#3352471, @ldionne wrote:

Yeah, I agree with that. We could update the Windows row in https://libcxx.llvm.org/#platform-and-compiler-support to say that the ABI is unstable -- would that be reasonable?

Sounds reasonable to me.

At least for the MSVC/MSABI variant of it - most of these issues don't hit the mingw configuration. In the mingw configuration, there's also more use of the library as a platform library - where we'd preferrably avoid continuous ABI breakage if possible (but where it probably still can be tolerated if deemed necessary).

That's fine with me, and that's what I had in mind. The way that MSVC handles dllexport inline functions (IMO) makes it harder to stabilize the C++ ABI, and GCC and mingw-mode Clang handle those similarly to the way -fvisibility-inlines-hidden works on non-COFF targets.

The dllimport vs. exclude_from_explicit_instantiation conflict comes to mind as an example of one of the challenges.

[davidben]

Picking this up again, I uploaded https://reviews.llvm.org/D146190 which takes @jdoerrie's patch and adds the test and documentation change. (Should I have made a new revision or updated this one? I assumed new one since uploading over someone else's CL seemed rude. I'm surprised there even is an "Update Diff" button for me in the corner here! 😄)

[ldionne]

^ Thanks! @jdoerrie let's abandon this review since we'll move forward with D146190!

[ldionne]

[Github PR transition cleanup]

Abandoning since this has been done.

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