[libcxx-commits] [PATCH] D103947: [libcxx] Fix using the vcruntime ABI with _HAS_EXCEPTIONS=0 defined

Wed Jul 28 13:21:35 PDT 2021

mstorsjo added a comment.

In D103947#2910425 <https://reviews.llvm.org/D103947#2910425>, @ldionne wrote:

> Sorry if some of my questions are naive, but I am not familiar with how things are layered when building on Windows. My understanding is that we're building on top of the C Standard Library part of the Visual C++ Runtime, and also using bits and pieces of their C++ STL (i.e. the `std::exception` class). Is that correct? What else do we use from their C++ support? Do we reuse other classes (e.g. more exception classes)?

I'm not familiar with all the specifics, but everything corresponding to the C++ ABI library is imported from there.

>> In this build configuration, one can still create instances of exception subclasses, and those objects will be ABI incompatible with the ones from when _HAS_EXCEPTIONS isn't defined to 0 - but that's IMO a pathological/self-imposed problem in that case.
>
> To avoid this, would it be possible to provide an ABI-compatible `std::exception` class, and use that unconditionally when building on top of the VC runtime?

Maybe that's possible - I haven't studied if their regular `std::exception` class does something funky or not.

> Edit: It looks like the Microsoft STL does exactly that? https://github.com/microsoft/STL/blob/main/stl/inc/exception#L81

Note that this is within an `#if !_HAS_EXCEPTIONS`, see https://github.com/microsoft/STL/blob/main/stl/inc/exception#L30-L33. I'm not sure if their fallback is ABI compatible or just the simplest possible stub.

================
Comment at: libcxx/include/typeinfo:349
+
+#if !defined(_LIBCPP_ABI_VCRUNTIME_EXCEPTION)
+
----------------
ldionne wrote:
> Why are we not providing those when building on top of the vcruntime exceptions?
Normally, vcruntime provides `bad_cast` too.

================
Comment at: libcxx/test/support/test_macros.h:255

-#if !TEST_HAS_FEATURE(cxx_exceptions) && !defined(__cpp_exceptions) \
-     && !defined(__EXCEPTIONS)
+#if (!TEST_HAS_FEATURE(cxx_exceptions) && !defined(__cpp_exceptions) \
+      && !defined(__EXCEPTIONS)) \
----------------
ldionne wrote:
> I don't understand why we're passing/checking `_HAS_EXCEPTIONS=0` around everywhere like that. Doesn't the compiler set `__has_feature(cxx_exceptions)` correctly when we use `-fno-exceptions` (or the MSVC equivalent)?
The situation isn't that a user explicitly disables exceptions with the equivalent of `-fno-exceptions`.

The users will have defined `_HAS_EXCEPTIONS=0` manually for some translation units (for whatever reason - apparently this is something that is pretty common, so we can't just fix the one case where it is set), which just omits those declarations but doesn't do anything else like affecting code generation.

================
Comment at: libcxx/utils/libcxx/test/features.py:34-40
+  # Normally, the feature 'no-exceptions' is set in params.py if
+  # running tests with enable_exceptions=false, but if we didn't set
+  # that and only defined _HAS_EXCEPTIONS=0, pick up on that and set the
+  # no-exceptions feature so certain tests are omitted.
+  # It would probably be fine to just run that test configuration with
+  # enable_exceptions=false too.
+  Feature(name='no-exceptions',                 when=lambda cfg: '_HAS_EXCEPTIONS' in compilerMacros(cfg) and compilerMacros(cfg)['_HAS_EXCEPTIONS'] == '0'),
----------------
ldionne wrote:
> This feels like it shouldn't be necessary, but I'd like to understand.
> 
> Why would you run the test suite with `_HAS_EXCEPTIONS=0` without passing `enable_exceptions=False`?
So this is the core issue to understand here: We don't intentionally build a setup of libc++ with exceptions disabled, we build a totally regular libc++ build. Then when this libc++ is used to build other projects, a small fraction of the translation units that include libc++ headers will be built with `_HAS_EXCEPTIONS=0`, which pulls out the rug under us, removing a bunch of the definitions we regularly rely on.

For testing purposes, we could build libc++ without exceptions support, but that wouldn't actually be testing the same scenario that we're going to hit in the wild. The same goes for unix platforms - you have libc++ built with exceptions enabled, but some amount of the code built using libc++ will be built with `-fno-exceptions`, and that should work even if the lib itself was built with exceptions enabled. Except `-fno-exceptions` has less brutal effect than `HAS_EXCEPTIONS=0`.

================
Comment at: libcxx/utils/libcxx/test/params.py:40
+
+  # PR50676, affects MSVC mode without exceptions. This is only a temporary
+  # hack - ideally we'd at most add this flag in that particular configuration,
----------------
ldionne wrote:
> Hans said in https://bugs.llvm.org/show_bug.cgi?id=50676 that `-fno-delayed-template-parsing` fixed the issue in MSVC mode. Is there a reason why we're not compiling with that flag? Are we trying to make libc++ work while emulating the old and broken MSVC template parsing machinery (which has been fixed now IIUC)?
I haven't considered that (I think that conclusion came after I posted this version of the patch) and I'm not familiar with that setting in general. For building libc++ I guess we can use whatever, but we can't assume the option to be set in a specific way when user's code includes libc++. I guess we could set it while running tests if it just silences a minor incovenience (warnings) that would just be hitting the particular compilation units that define `_HAS_EXCEPTIONS=0` anyway.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103947/new/

https://reviews.llvm.org/D103947