[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#3417436, @paulkirth wrote:

We'd like to get this feature landed for Fuchsia support on Windows. If you don't plan to work on this soon, do you mind if I commandeer this patch and look into the approach proposed here by @ldionne?

Feel free to commandeer it - I don't plan to work on it.

[paulkirth]

Update the approach to use an ABI compatible version of the Exception classes typically supplied by MSVC when exceptions are disabled. This is an almost verbatim copy of @ldionne 's suggestion using MSVC's implementation as a guideline for ABI compatibility. I've also modified the summary with information about the new approach.

I am unsure of the testing approach that should be taken here and have largely reused the changes to bots and test configurations from the previous patch(@mstorsjo).

[mstorsjo]

libcxx/include/exception

92

Is this condition strictly necessary? Isn't it the case that if we include the header, under these circumstances, it doesn't provide the definitions we need? I.e. we could still keep including the header even if it doesn't give us anything of value.

(OTOH I guess the condition can be good for clarity?)

Are we sure that vcruntime.h which sets the default _HAS_EXCEPTIONS == 1 has been included at this point? (I guess cstddef and cstdlib boil down to including that transitively?) Or would we need to make this into #if defined(_LIBCPP_ABI_VCRUNTIME) && (!defined(_HAS_EXCEPTIONS) || _HAS_EXCEPTIONS != 0)?

104

Isn't the _HAS_EXCEPTIONS define strictly speaking independent of -fno-exceptions? As far as I can see, clang-cl doesn't predefine _HAS_EXCEPTIONS, and vcruntime.h defines _HAS_EXCEPTIONS to 1 unless it has been set by the user/caller.

I.e. I think the comment would be more correct by speaking of _HAS_EXCEPTIONS consistently instead of involving -fno-exceptions.

Secondly, I see that the structure of comments for the ifdef conditions here, is to have the comment preceding the condition. As there's more than one condition involved, the comment for the second condition #elif defined(_LIBCPP_ABI_VCRUNTIME) && _HAS_EXCEPTIONS == 0 ends up in the ifdef block for the first condition, so at least to me, it'd be more readable if the comment explaining the condition would be after the condition, i.e. within the ifdef block it explains.

129

Should this maybe be using _LIBCPP_NODISCARD instead? And __CLR_OR_THIS_CALL could maybe be omitted in all cases where libc++ is used... I guess it's a tradeoff between exactly matching the reference to the letter (easing compat if taking this to further untested configurations) vs making the code match libc++ conventions (where attribute macros may be defined slightly differently than in upstream vcruntime).

145

Shouldn't this be using _NOEXCEPT instead of plain noexcept?

147

As far as I can see, libc++ typically doesn't use override

libcxx/test/support/test_macros.h

189 ↗

(On Diff #419284)

Previous versions of this patch had parentheses around the first set of conditions, making it clearer wrt the mixed && and ||, i.e. #if (existing_condition && other_existing_condition) || (defined(_HAS_EXCEPTIONS && _HAS_EXCEPTIONS == 0)

libcxx/utils/ci/buildkite-pipeline.yml

595

Since earlier revisions of the patch, we now have both clang-cl and mingw CI configurations, so for clarity, please name this configuration Clang-cl (_HAS_EXCEPTIONS=0), or as @ldionne requested earlier, Clang-cl (no exceptions).

libcxx/utils/ci/run-buildbot

587

Unfortunately, after switching to the new style libcxx test configs, LIBCXX_TEST_COMPILER_FLAGS no longer has an effect. (Until the option is removed, maybe our cmake file could warn if it is set?) So currently, you'd need to duplicate the llvm-libc++-shared-clangcl.cfg.in test config file and make a new one which also passes this define.

[paulkirth]

Respond to comments.

• Update macros to libc++ versions, remove __CLR_OR_THIS_CALL
• add new test config file
• make preprocessor checks easier to read
• update comments for readability

[phosek]

libcxx/include/exception

101

The name of the class isn't capitalized.

111

Typo.

117–118

I'd make it a full sentence.

121

You can omit the argument name since it's unused.

123

You can omit the argument name since it's unused.

133–134

Nit

139

This is not necessary since you already have protected: above.

140

I wonder if we should annotate this with [[maybe_unused]] to avoid a compiler warning.

150–151

Nit

154

Nit

[paulkirth]

Fix typos and nits

[paulkirth]

libcxx/include/exception

92

I was trying to be explicit, but maybe my logic was off here. I believe that vcruntime.h should always have been pulled in by this point, but I will try and double check that will always be the case. Regardless, I think your suggestion of #if defined(_LIBCPP_ABI_VCRUNTIME) && (!defined(_HAS_EXCEPTIONS) || _HAS_EXCEPTIONS != 0) is the better approach to take here

104

Isn't the _HAS_EXCEPTIONS define strictly speaking independent of -fno-exceptions? As far as I can see, clang-cl doesn't predefine _HAS_EXCEPTIONS, and vcruntime.h defines _HAS_EXCEPTIONS to 1 unless it has been set by the user/caller.

My understanding here was that this was a problem because when -fno-exceptions is set, then MSVC sets _HAS_EXCEPTIONS=0, and thus does not provide the definitions for the Exception classes. But if it adds clarity, I'm happy to amend the wording here to focus on _HAS_EXCEPTIONS

Additionally, I'm happy to place the comments differently if you think it would improve readability, so thanks for the suggestion.

129

Oh, thanks for catching the error with _NODISCARD, I will update the patch to reflect this.

w.r.t __CLR_OR_THIS_CALL I erred on the side of caution, since I wasn't 100% sure it would be ABI compatible w/o it, but I was also hesitant about using it.

Given that you also think its a bit out of place, I plan to remove them. If we find out it does indeed cause a problem, we can always add them back in.

140

That may be a good idea. Grep didn't reveal a macro that I saw, but I may have missed it. I do see that this file has a _LIBCPP_DIAGNOISTIC_IGNORED(-Wunused-private-field) maybe that is the correct approach here w/ a change to the warning?

[paulkirth]

Fix formatting and _NOEXCEPT

[mstorsjo]

Overall I think this looks quite good now, just a couple minor nits left.

libcxx/include/exception

92

Here, you added the !defined(_HAS_EXCEPTIONS) in the next condition, but I think we should have it here too, on the very first ifdef.

Additionally, I think I'd rather check _HAS_EXCEPTIONS != 0 instead of _HAS_EXCEPTIONS == 1 as I believe the define is interpreted as a boolean, if someone were to set it to something else than 0 or 1.

100–101

I'd prefer _HAS_EXCEPTIONS != 0 instead of == 1.

[paulkirth]

Update preprocessor conditions

[phosek]

libcxx/include/exception

140

I'd prefer defining a new macro, e.g. _LIBCPP_MAYBE_UNUSED, akin to _NOEXCEPT which would expand to [[maybe_unused]] (or __attribute__((unused)) if [[maybe_unused]] is unavailable).

[philnik]

libcxx/include/exception

140

I'd rather avoid adding more macros. This shouldn't ever generate a warning, since the variable is protected and not private. Otherwise I would consider this a compiler bug.

[paulkirth]

Fix formatting

[paulkirth]

rename config file w/ correct extension

[paulkirth]

rebase

[paulkirth]

@ldionne I think this is probably ready for you to take a look at.

[paulkirth]

Use unused parameter and remove noreturn specifier.

I don't think either will impact ABI compatability, and should fix some errors.

[paulkirth]

Try to make preprocessor conditions consistant

[phosek]

libcxx/include/exception

101

Super minor nit

116

Super minor nit

[paulkirth]

Use different approach to handling missing defintions.

The more complex preprocessor conditions may not be necesary

[paulkirth]

Move include outside of namespace & clean up changes to preprocessor checks

[paulkirth]

change include to vcruntime.h to address bad definitions

[paulkirth]

Update class defs to match vcruntime_*.h definitions

I had used definitions base on the MSVC STL previously, which was incorrect.
While there remain a few linking issues, this approach allows most tests
to pass locally.

[phosek]

libcxx/include/exception

114

Nit

121

Nit

123

Nit

128

Nit

142

Nit

[paulkirth]

run clang-format

[paulkirth]

Remove LIBCXX_EXCEPTION_ABI specifiers.

It should have been obvious that this would make these definitions ABI incompatable w/ the vcruntime.

[paulkirth]

Rebase and fix accidental removal of LIBCPP_EXCEPTION_ABI macro outside of _HAS_EXCEPTIONS == 0 case

[paulkirth]

Re-run clang-format on code, since git clang-format fails to format it correctly

[paulkirth]

so this is down to one failure: support/test.support/test_macros_header.no_exceptions.verify.cpp (https://buildkite.com/llvm-project/libcxx-ci/builds/10551#b46abad1-6a39-4118-8a86-b50e1aaab001).

The failing test expects an error due to exceptions being disabled, so I'm not sure we should expect that to pass in this config.

_HAS_EXCEPTIONS = 0 disables exceptions by failing to provide definitions to exception classes, and is what gets set when fno-exceptions is used, but I don't think that compilation will fail with the expected message if you *only* set _HAS_EXCEPTIONS = 0, but don't also use fno-exceptions.

@phosek @mstorsjo @ldionne what's the course of action here? just maybe mark this as XFAIL? or disable this test if in this config?

[mstorsjo]

In D103947#3487191, @paulkirth wrote:

so this is down to one failure: support/test.support/test_macros_header.no_exceptions.verify.cpp (https://buildkite.com/llvm-project/libcxx-ci/builds/10551#b46abad1-6a39-4118-8a86-b50e1aaab001).

The failing test expects an error due to exceptions being disabled, so I'm not sure we should expect that to pass in this config.

_HAS_EXCEPTIONS = 0 disables exceptions by failing to provide definitions to exception classes, and is what gets set when fno-exceptions is used, but I don't think that compilation will fail with the expected message if you *only* set _HAS_EXCEPTIONS = 0, but don't also use fno-exceptions.

@phosek @mstorsjo @ldionne what's the course of action here? just maybe mark this as XFAIL? or disable this test if in this config?

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?

[paulkirth]

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.

[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.

libcxx/include/exception

 #include <__availability>
 #include <__config>
 #include <__memory/addressof.h>
 #include <cstddef>
 #include <cstdlib>
 #include <type_traits>
 #include <version>
 
+// <vcruntime_exception.h> defines its own std::exception and std::bad_exception types,
+// which we use in order to be ABI-compatible with other STLs on Windows.
 #if defined(_LIBCPP_ABI_VCRUNTIME)
-#include <vcruntime_exception.h>
+#  include <vcruntime_exception.h>
 #endif
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 namespace std  // purposefully not using versioning namespace
 {
 
-#if !defined(_LIBCPP_ABI_VCRUNTIME)
-class _LIBCPP_EXCEPTION_ABI exception
-{
+#if defined(_LIBCPP_ABI_VCRUNTIME) && (!defined(_HAS_EXCEPTIONS) || _HAS_EXCEPTIONS != 0)
+// The std::exception class was already included above, but we're explicit about this condition here for clarity.
+
+#elif defined(_LIBCPP_ABI_VCRUNTIME) && _HAS_EXCEPTIONS == 0
+// However, <vcruntime_exception.h> does not define std::exception and std::bad_exception
+// when _HAS_EXCEPTIONS == 0.
+//
+// Since libc++ still wants to provide the std::exception hierarchy even when _HAS_EXCEPTIONS == 0
+// (after all those are simply types like any other), we define an ABI-compatible version
+// of the VCRuntime std::exception and std::bad_exception types in that mode.
+
+struct __std_exception_data {
+  char const* _What;
+  bool _DoFree;
+};
+
+class exception { // base of all library exceptions
+public:
+  exception() _NOEXCEPT : _Data() {}
+
+  explicit exception(char const* __message) _NOEXCEPT : _Data() {
+    _Data._What = __message;
+    _Data._DoFree = true;
+  }
+
+  exception(exception const&) _NOEXCEPT {}
+
+  exception& operator=(exception const&) _NOEXCEPT { return *this; }
+
+  virtual ~exception() _NOEXCEPT {}
+
+  virtual char const* what() const _NOEXCEPT { return _Data._What ? _Data._What : "Unknown exception"; }
+
+private:
+  __std_exception_data _Data;
+};
+
+class bad_exception : public exception {
+public:
+  bad_exception() _NOEXCEPT : exception("bad exception") {}
+};
+
+#else // !defined(_LIBCPP_ABI_VCRUNTIME)
+// On all other platforms, we define our own std::exception and std::bad_exception types
+// regardless of whether exceptions are turned on as a language feature.
+
+class _LIBCPP_EXCEPTION_ABI exception {
 public:
-    _LIBCPP_INLINE_VISIBILITY exception() _NOEXCEPT {}
-    _LIBCPP_INLINE_VISIBILITY exception(const exception&) _NOEXCEPT = default;
 
-    virtual ~exception() _NOEXCEPT;
-    virtual const char* what() const _NOEXCEPT;
 };
 
-class _LIBCPP_EXCEPTION_ABI bad_exception
-    : public exception
-{
+class _LIBCPP_EXCEPTION_ABI bad_exception : public exception {
 public:
-    _LIBCPP_INLINE_VISIBILITY bad_exception() _NOEXCEPT {}
-    virtual ~bad_exception() _NOEXCEPT;
-    virtual const char* what() const _NOEXCEPT;
 };
 #endif // !_LIBCPP_ABI_VCRUNTIME
 
 #if _LIBCPP_STD_VER <= 14 \
     || defined(_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS) \
     || defined(_LIBCPP_BUILDING_LIBRARY)
 typedef void (*unexpected_handler)();
 _LIBCPP_FUNC_VIS unexpected_handler set_unexpected(unexpected_handler) _NOEXCEPT;

libcxx/include/new

public:
     virtual ~bad_array_new_length() _NOEXCEPT;
     virtual const char* what() const _NOEXCEPT;
 };
 
 typedef void (*new_handler)();
 _LIBCPP_FUNC_VIS new_handler set_new_handler(new_handler) _NOEXCEPT;
 _LIBCPP_FUNC_VIS new_handler get_new_handler() _NOEXCEPT;
 
-#endif // !_LIBCPP_ABI_VCRUNTIME
+#elif defined(_HAS_EXCEPTIONS) && _HAS_EXCEPTIONS == 0 // !_LIBCPP_ABI_VCRUNTIME
+
+// When _HAS_EXCEPTIONS == 0, these complete definitions are needed,
+// since they would normally be provided in vcruntime_exception.h
+class bad_alloc : public exception {
+public:
+  bad_alloc() noexcept : exception("bad allocation") {}
+
+private:
+  friend class bad_array_new_length;
+
+  bad_alloc(char const* const __message) noexcept : exception(__message) {}
+};
+
+class bad_array_new_length : public bad_alloc {
+public:
+  bad_array_new_length() noexcept : bad_alloc("bad array new length") {}
+};
+#endif // defined(_LIBCPP_ABI_VCRUNTIME) && defined(_HAS_EXCEPTIONS) &&_HAS_EXCEPTIONS == 0 
 
 _LIBCPP_NORETURN _LIBCPP_FUNC_VIS void __throw_bad_alloc();  // not in C++ spec
 
 _LIBCPP_NORETURN inline _LIBCPP_INLINE_VISIBILITY
 void __throw_bad_array_new_length()
 {
 #ifndef _LIBCPP_NO_EXCEPTIONS
     throw bad_array_new_length();
 #else
     _VSTD::abort();
 #endif
 }
 

libcxx/include/typeinfo

 #endif
 };
 #endif // defined(_LIBCPP_ABI_MICROSOFT)
 
 class _LIBCPP_EXCEPTION_ABI bad_cast
     : public exception
 {
  public:
   bad_cast() _NOEXCEPT;
   bad_cast(const bad_cast&) _NOEXCEPT = default;
   virtual ~bad_cast() _NOEXCEPT;
   virtual const char* what() const _NOEXCEPT;
 };
 
 class _LIBCPP_EXCEPTION_ABI bad_typeid
     : public exception
 {
  public:
   bad_typeid() _NOEXCEPT;
   virtual ~bad_typeid() _NOEXCEPT;
   virtual const char* what() const _NOEXCEPT;
 };
 
 } // namespace std
 
 #endif // defined(_LIBCPP_ABI_VCRUNTIME)
 
+#if defined(_LIBCPP_ABI_VCRUNTIME) && _HAS_EXCEPTIONS == 0
+
+namespace std {
+
+class bad_cast : public exception {
+public:
+  bad_cast() _NOEXCEPT : exception("bad cast") {}
+
+private:
+  bad_cast(const char* const __message) _NOEXCEPT : exception(__message) {}
+};
+
+class bad_typeid : public exception {
+public:
+  bad_typeid() _NOEXCEPT : exception("bad typeid") {}
+
+private:
+  bad_typeid(const char* const __message) _NOEXCEPT : exception(__message) {}
+};
+
+} // namespace std
+
+#endif // defined(_LIBCPP_ABI_VCRUNTIME) && _HAS_EXCEPTIONS == 0
+
 _LIBCPP_BEGIN_NAMESPACE_STD
 _LIBCPP_NORETURN inline _LIBCPP_INLINE_VISIBILITY
 void __throw_bad_cast()
 {
 #ifndef _LIBCPP_NO_EXCEPTIONS
     throw bad_cast();
 #else
     _VSTD::abort();
 #endif
 }
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // __LIBCPP_TYPEINFO

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

+# This testing configuration handles running the test suite against LLVM's libc++
+# using a DLL, with Clang-cl on Windows. This variant sets _HAS_EXCEPTIONS = 0
+# which removes exception class definitions  from the vcruntime.
+
+lit_config.load_config(config, '@CMAKE_CURRENT_BINARY_DIR@/cmake-bridge.cfg')
+
+config.substitutions.append(('%{flags}', '--driver-mode=g++'))
+config.substitutions.append(('%{compile_flags}',
+    '-nostdinc++ -I %{include} -I %{target-include} -I %{libcxx}/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX -D_HAS_EXCEPTIONS=0'
+))
+config.substitutions.append(('%{link_flags}',
+    '-nostdlib -L %{lib} -lc++ -lmsvcrt -lmsvcprt -loldnames'
+))
+config.substitutions.append(('%{exec}',
+    '%{executor} --execdir %T --env PATH=%{lib} -- '
+))
+
+import os, site
+site.addsitedir(os.path.join('@LIBCXX_SOURCE_DIR@', 'utils'))
+import libcxx.test.params, libcxx.test.newconfig
+libcxx.test.newconfig.configure(
+    libcxx.test.params.DEFAULT_PARAMETERS,
+    libcxx.test.features.DEFAULT_FEATURES,
+    config,
+    lit_config
+)

libcxx/utils/ci/buildkite-pipeline.yml

- label: "No exceptions"
         queue: "libcxx-builders"
         os: "linux"
       retry:
         automatic:
           - exit_status: -1  # Agent was lost
             limit: 2
       timeout_in_minutes: 120
 
   - label: "Unstable ABI"
     command: "libcxx/utils/ci/run-buildbot generic-abi-unstable"
     artifact_paths:
       - "**/test-results.xml"
       - "**/*.abilist"
     agents:
       queue: "libcxx-builders"
       os: "linux"
     retry:
- label: "Clang-cl (Static)"
       agents:
         queue: "windows"
       retry:
         automatic:
           - exit_status: -1  # Agent was lost
             limit: 2
       timeout_in_minutes: 120
 
+    - label: "Clang-cl (no vcruntime exceptions)"
+      command: "bash libcxx/utils/ci/run-buildbot clang-cl-no-vcruntime"
+      artifact_paths:
+        - "**/test-results.xml"
+        - "**/*.abilist"
+      agents:
+        queue: "windows"
+      retry:
+        automatic:
+          - exit_status: -1  # Agent was lost
+            limit: 2
+
     - label: "MinGW (DLL, x86_64)"
       command: "bash libcxx/utils/ci/run-buildbot mingw-dll"
       artifact_paths:
         - "**/test-results.xml"
         - "**/*.abilist"
       agents:
         queue: "windows"
       retry:

libcxx/utils/ci/run-buildbot

clang-cl-dll)
     ${NINJA} -vC "${BUILD_DIR}" check-cxx
 ;;
 clang-cl-static)
     clean
     generate-cmake-libcxx-win -DLIBCXX_ENABLE_SHARED=OFF
     echo "+++ Running the libc++ tests"
     ${NINJA} -vC "${BUILD_DIR}" check-cxx
 ;;
+clang-cl-no-vcruntime)
+    clean
+    # Building libc++ in the same way as in clang-cl-dll above, but running
+    # tests with -D_HAS_EXCEPTIONS=0, which users might set in certain
+    # translation units while using libc++, even if libc++ is built with
+    # exceptions enabled.
+    generate-cmake-libcxx-win -DLIBCXX_TEST_PARAMS="enable_experimental=False" \
+                              -DLIBCXX_TEST_CONFIG="llvm-libc++-shared-no-vcruntime-clangcl.cfg.in"
+    echo "+++ Running the libc++ tests"
+    ${NINJA} -vC "${BUILD_DIR}" check-cxx
+;;
 mingw-dll)
     clean
     # Explicitly specify the compiler with a triple prefix. The CI
     # environment has got two installations of Clang; the default one
     # defaults to MSVC mode, while there's an installation of llvm-mingw
     # further back in PATH. By calling the compiler with an explicit
     # triple prefix, we use the one that is bundled with a mingw sysroot.
     generate-cmake \