[libcxx] Fix using the vcruntime ABI with _HAS_EXCEPTIONS=0 defined

[paulkirth]

Summary

_HAS_EXCEPTIONS=0 allows disabling the exception parts of the MS STL
and vcruntime, and e.g. compiler-rt/lib/fuzzer sets this define (to
work around issues with MS STL). If using libc++ instead of MS STL,
this define previously broke the libc++ headers.

If _HAS_EXCEPTIONS is set to 0, the vcruntime_exception.h header
doesn't define the ABI base class std::exception. If no exceptions
are going to be thrown, this probably is fine (although it also
breaks using subclasses of it as regular objects that aren't thrown),
but it requires ifdeffing out all subclasses of all exception/error
derived objects (which are sprinkled throughout the headers).

Instead, libc++ will supply an ABI compatible definition when
_HAS_EXCEPTIONS is set to 0, which will make the class hierarchies
complete.

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
one may argue that's a pathological/self-imposed problem in that case.

[mstorsjo]

In D103947#3492146, @paulkirth wrote:

In D103947#3492009, @mstorsjo wrote:

I think you should extend test_macros.h to define TEST_HAS_EXCEPTIONS like this:

#if (!TEST_HAS_FEATURE(cxx_exceptions) && !defined(__cpp_exceptions) \
     && !defined(__EXCEPTIONS)) || \
    (defined(_HAS_EXCEPTIONS) && _HAS_EXCEPTIONS == 0) 
#define TEST_HAS_NO_EXCEPTIONS
#endif

As the _HAS_EXCEPTIONS=0 flag practically means that we should skip exception tests, so the no-exceptions testsuite feature flag should be set. But on the compiler level, exceptions aren't really disabled, so this test macro doesn't pick it up, but we maybe can make the test macro look at _HAS_EXCEPTIONS too. Or does that break other tests, if TEST_HAS_NO_EXCEPTIONS suddenly is defined in all tests?

We already define the macro in that way. clang-format modified the layout slightly, but they look identical to me. I haven't noticed any tests failing because of that change.

Oh, sorry, I looked at the failure too quickly, now I see what's going on. Yeah I guess it'd make most sense to XFAIL this test, somehow. Wrapping the try/catch in an #if !defined(_HAS_EXCEPTIONS) || _HAS_EXCEPTIONS != 0 could work too but isn't quite pretty (but I don't know of a neat way of XFAILing this test either, other than adding yet another test feature flag for this specific configuration.)

[paulkirth]

Use preprocessor to avoid incorrect diagnostic checking when __HAS_EXCEPTIONS == 0

[paulkirth]

@mstorsjo Thanks for that idea, and you're right, it is very ugly, more so after formatting. Hopefully this will allow the final test to pass.

BTW you've been an incredible resource for someone who "wasn't planning to work on this". Thanks for all your feedback. I'm sadly very lost when it comes to Windows, and you've been very generous with your time.

[paulkirth]

ok, that broke this test in other configs. _HAS_EXCEPTIONS will also be set when we use -fno-exceptions so I should have thought of that issue.

I'll need to either think of another way to disable the test that one configuration, or look into a more complex solution, like marking the test XFAIL.

[paulkirth]

Detect when exceptions are disabled in the compiler, and disambiguate from the case when _HAS_EXCEPTIONS ==0

Adds a new preprocessor define that is defined when the old TEST_HAS_NO_EXCEPTIONS conditions would have been true.
We use this as a proxy test to determine if no-exceptions is really in force. If not, then we don't expect the diagnostics to be emitted.

[mstorsjo]

libcxx/test/configs/llvm-libc++-shared-no-exception-clangcl.cfg.in

22 ↗

(On Diff #427164)

Oh, btw, the whole block with LIBCXX-WINDOWS-FIXME was finally removed in upstream libcxx in D123145, so let's not reintroduce it in this new test config :-)

[paulkirth]

Try to set XFAIL to stop bot failure when _HAS_EXCEPTIONS=0. Worked locally ...

[paulkirth]

Go back to previous macros, but ensure that diagnostics checks are on the same line as the catch block

[paulkirth]

Don't reintroduce LIBCXX-WINDOWS-FIXME

[paulkirth]

@ldionne I think this brings everything up correctly on the windows front. thoughts?

[ldionne]

I'm mostly happy with this, except for the fact that we seem to allow _HAS_EXCEPTIONS=0 when the compiler is still passed -fexceptions. That seems either wrong or not worth supporting to me, but I'm all ears.

libcxx/include/exception

118–134

I think we want to mark all of those as _LIBCPP_HIDE_FROM_ABI (and similar for other exception classes defined in this patch).

And then, we can get rid of the weird exception(char const* _Message, int) constructor in favour of something more elegant, like passing a boolean directly or using a named tag like __dont_free_tag{} (because we don't care about matching the exact ABI of the constructor defined by VCRuntime, which I assume is why you're using the funky int overload here).

libcxx/include/typeinfo

403

What is this class?

libcxx/test/configs/llvm-libc++-shared-no-exception-clangcl.cfg.in

1 ↗

(On Diff #427454)

Why is this not handled by passing -fno-exceptions on the command-line when running the test suite? In other words, why don't we use the existing shared-clangcl.cfg.in configuration and add enable_exceptions=False?

libcxx/utils/libcxx/test/features.py

42–47 ↗

(On Diff #427454)

I think this is the cause of most of my confusion. Why would someone ever define _HAS_EXCEPTIONS=0 manually instead of using -fno-exceptions? That seems like a small quirk that causes a lot of complexity in this patch.

[mstorsjo]

libcxx/test/configs/llvm-libc++-shared-no-exception-clangcl.cfg.in

1 ↗

(On Diff #427454)

I think the main point is that this isn’t a configuration that someone intentionally chooses, as a project wide configuration. For various historic reasons, some projects seem to be building smaller subsets of their code (e.g. a subset of their source files) with this define (_HAS_EXCEPTIONS=0) set. They don’t build with -fno-exceptions (or the MSVC/clang-cl equivalents). I.e. the end user code is built with exception handling codegen enabled, but vcruntime’s classes hidden.

(Also, building with -fno-exceptions doesn’t define _HAS_EXCEPTIONS=0. They’re totally independent toggles.)

If building with LIBCXX_ENABLE_EXCEPTIONS=OFF, you’d have the libc++ built with vcruntime exception classes hidden - but that’s not the situation that end users use. Testing that way would probably cover many aspects of the code, but it wouldn’t test what end users actually end up using.

End users have a libc++ built entirely normally, but some fraction of end users code is built with vcruntime exception classes hidden.

(The fact that the library is built differently than end user code is the same situation as -fexception vs -fno-exceptions on unix. The library will be built with exceptions enabled, while some end user object files may be built with -fno-exceptions.)

[paulkirth]

Thanks for the feedback, and glad most of it seems to be in order.

libcxx/include/exception

118–134

Adding the _LIBCPP_HIDE_FROM_ABI is a good suggestion. Thank you.

Re removing the odd constructor, I think I had some trouble getting that to work IIRC, but let me give it another shot.

libcxx/include/typeinfo

403

oh, you're right. I think this is unnecessary.

libcxx/test/configs/llvm-libc++-shared-no-exception-clangcl.cfg.in

1 ↗

(On Diff #427454)

Here we're testing the case when the user hasn't passed -fno-exceptions but the has disabled exceptions through _HAS_EXCEPTIONS=0. From reading Martin's comments about this in earlier revisions it seems to be an important case to handle on Windows.

I'm happy to include the other approach, but I'm not sure we should forgo this configuration, unless libc++ wants to document it as unsupported.

[paulkirth]

Address review comments

• add _LIBCPP_HIDE_FROM_ABI to exception classes
• remove extra exception constructor
• remove unused friend class

[paulkirth]

ping

[paulkirth]

Rebase

[paulkirth]

friendly ping.

[ldionne]

libcxx/test/configs/llvm-libc++-shared-no-exception-clangcl.cfg.in

1 ↗

(On Diff #427454)

I understand. What you want to test is the combination of:

• libc++ built with support for exceptions
• user code built without support for exceptions

I'm OK with that -- I think that is useful and important to test. However, for me, that equates to the combination of LIBCXX_ENABLE_EXCEPTIONS=ON when building the library, and --param enable_exceptions=False when running the test suite. For me, the crazy part that we should not support is the fact that _HAS_EXCEPTIONS=0 is not the same as -fno-exceptions -- there's no place for that distinction in the test suite, IMO. It is okay if users build some TUs with exceptions enabled and others disabled, as long as they use -fno-exceptions and -fexceptions to do so.

So I might be thick, but I still think we should be using shared-clangcl.cfg.in with enable_exceptions=False, and building the library with LIBCXX_ENABLE_EXCEPTIONS=ON. Please let me know if you think I'm misunderstanding something and this is somehow not reasonable. With my understanding, the only users we'd be "not supporting" with that path forward are those that set _HAS_EXCEPTIONS=0 instead of passing -fno-exceptions in the TUs that shouldn't have exceptions, and I'm fine with that.

[mstorsjo]

libcxx/test/configs/llvm-libc++-shared-no-exception-clangcl.cfg.in

1 ↗

(On Diff #427454)

I get that you want that - however the exact situation where this was encountered in the wild, which prompted this whole patch, is that existing user code does exactly that - build with _HAS_EXCEPTIONS=0, without -fno-exceptions (or the clang-cl equivalent).

Just building with -fno-exceptions alone does not trigger the whole thing that we try to fix here. Building with -fno-exceptions still keeps the vcruntime exception base classes visible in headers, but just disables exception handling codegen.

[phosek]

libcxx/test/configs/llvm-libc++-shared-no-exception-clangcl.cfg.in

1 ↗

(On Diff #427454)

/EH and _HAS_EXCEPTIONS are technically orthogonal. /EH is the equivalent of -f[no]-exceptions. _HAS_EXCEPTIONS controls the use of exceptions in Microsoft STL including vcruntime which we use as a C++ ABI library on Windows. I don't know if there's a direct analogue on UNIX platforms, but on Windows we need to support both because they are being used in practice.

[ldionne]

libcxx/include/exception

117

No classes should ever be marked with _LIBCPP_HIDE_FROM_ABI, it's meant for functions exclusively. Otherwise, we'll give improper visibility/linkage to the RTTI of the class, and you won't be able to catch it across shared library (and even TU) boundary.

You should mark the individual member functions you want to hide from the ABI instead. Sorry, that's not obvious at all from the macro name.

libcxx/include/new

172

I am confused -- why do we have a different definition for those than above? In other words, why don't we simply change the #if !defined(_LIBCPP_ABI_VCRUNTIME) above to #if !defined(_LIBCPP_ABI_VCRUNTIME) || (defined(_HAS_EXCEPTIONS) && _HAS_EXCEPTIONS == 0)?

Are we trying to match the vcruntime layout here too?

libcxx/include/typeinfo

381–383

What is this? I don't see it used anywhere, why is it necessary?

libcxx/test/support/test.support/test_macros_header.no_exceptions.verify.cpp

12 ↗

(On Diff #434520)

If you didn't set the no-exceptions feature when _HAS_EXCEPTIONS=0 (which is misleading IMO, see other comment), you wouldn't need to touch this test at all IIUC. You also would not need to introduce TEST_COMPILING_WITH_NO_EXCEPTIONS.

libcxx/test/support/test_macros.h

184 ↗

(On Diff #434520)

TEST_HAS_NO_EXCEPTIONS is used to represent -fno-exceptions. Let's not overload it to mean "either -fno-exceptions or no exception classes provided by VCRuntime". In fact, I don't understand why we even need to detect that no exception classes are provided by vcruntime, since we are defining them in their place. So from the test suite's perspective, my understanding is that we should not even need any special code to handle _HAS_EXCEPTIONS=0.

libcxx/utils/ci/buildkite-pipeline.yml

595

Actually, in light of the latest comments, I think this should either be (no exception classes), (no vcruntime exceptions) or (_HAS_EXCEPTIONS=0). (no exceptions) is misleading, since one would expect that it's basically -fno-exceptions. I think my vote would go for (no vcruntime exceptions), and make the build job become clang-cl-no-vcruntime-exceptions.

Since this has apparently nothing to do with -fno-exceptions, let's make it clear.

libcxx/utils/libcxx/test/features.py

34–40 ↗

(On Diff #351387)

In that case, no-exceptions is not the right Lit feature name. It's already used for -fno-exceptions (i.e. no support for exceptions in the runtime), and this is something entirely different.

In fact, why do you even need to set a Lit feature here?

[paulkirth]

Address comments

• Remove usage of _LIBCPP_HIDE_FROM_ABI, I don't see any remaining functions that need them after moving away from strict layout conformance w/ vcruntime.
• Remove unused fuction.
• Make the `_HAS_EXCEPTIONS == 0' definitions for bad_alloc mutually exclusive with the existing preprocessor directives.
• Rename test configurations and files for CI

I still need to figure out if the lit feature can be removed.

[paulkirth]

libcxx/include/exception

117

That makes so much more sense. Thanks for the tip. I think there is very little left to hide anymore, since a lot of the initial changes I made to try and be compatible w/ vcruntime are no longer necessary.

libcxx/include/new

172

I tried to reuse those definitions, and ran into some other issues related to the weirdness that _HAS_EXCEPTIONS == 0 presents.

Since the vcruntime provides the full defs for these classes, and libcxx is compiled w/o _HAS_EXCEPTIONS == 0, we need to provide the implementation somewhere because it won't be availible in the library. I don't know of a good way to fallback unless I provide an implementation in the header, but providing a separate implementation from the class def was more challenging that I thought due to the visibility annotations used for those classes.

My first attempt was to only provide the missing function implementations, which were just empty and were along the lines of:

#if !defined(_LIBCPP_ABI_VCRUNTIME) || _HAS_EXCEPTIONS == 0
//.. existing defs
#endif
#if defined(_LIBCPP_ABI_VCRUNTIME) || _HAS_EXCEPTIONS == 0
bad_alloc::bad_alloc() _NOEXCEPT {}
// more implementations ...
#endif

That seemed like a clean solution until the build complained about redefining the functions w/o dllimport, and I don't see a good way to provide out of line definitions for the classes w/in the header file. If there's an option I overlooked, I'm happy to go back to this approach, but didn't find a suitable method to make that work.

[paulkirth]

Address outstanding review comments.

• Remove lit feature for making _HAS_EXCEPTIONS = 0 match no-exceptions. I was under the impression that this feature was required so that we would avoid testing unsupported configurations. I think this may no longer be necessary, and have removed it in light of @ldionne's question. In my testing this configuration seesm to work fine.

• Fix buildbot command
• Add documentation to test config file regarding _HAS_EXCEPTIONS = 0 useage
• Fix typos & whitespace

[phosek]

@ldionne Would it be possible to take a look again? This change is a blocker for D101479 which may be important for D121141.

[paulkirth]

Rebase.

[paulkirth]

Oh, I think I need to update the buildbot config to not build experimental, since that changed. I'll update tomorrow

[paulkirth]

Disable experimental in buildbot config & Rebase

[paulkirth]

Fix diagnostic from clang-tidy

[paulkirth]

Properly fix formatting for clang_tidy.

[paulkirth]

I think this is probably ready for review again. Presubmit is green and I think all of @ldionne 's feedback has been addressed.

[ldionne]

LGTM with a nitpick. Also, please make sure to update the commit message, it mentions defining _LIBCPP_NO_EXCEPTIONS but that's not the case anymore.

Thanks everyone for your patience with this patch.

libcxx/test/configs/llvm-libc++-shared-no-vcruntime-clangcl.cfg.in

4

[paulkirth]

Address nit

[paulkirth]

Rebase and update summary

[paulkirth]

That's great news! Thanks for the feedback @ldionne and @mstorsjo, I probably wouldn't have done this so easily w/o your input.