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.
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.
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.
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?
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.
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! 😄)