This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by paulkirth on Jun 9 2021, 1:29 AM.

Details

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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 commandeered this revision.Mar 30 2022, 1:23 PM
paulkirth added a reviewer: mstorsjo.
paulkirth updated this revision to Diff 419284.Mar 30 2022, 4:57 PM
paulkirth edited the summary of this revision. (Show Details)

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 added inline comments.Mar 30 2022, 11:39 PM
libcxx/include/exception
89

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)?

102

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.

127

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

143

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

145

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
568 ↗(On Diff #419284)

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
602 ↗(On Diff #419284)

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 updated this revision to Diff 423501.Apr 18 2022, 6:35 PM

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 added inline comments.Apr 18 2022, 6:54 PM
libcxx/include/exception
99

The name of the class isn't capitalized.

109

Typo.

115–116

I'd make it a full sentence.

119

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

121

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

131–132

Nit

137

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

138

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

148–149

Nit

152

Nit

paulkirth updated this revision to Diff 423504.Apr 18 2022, 7:09 PM

Fix typos and nits

paulkirth added inline comments.Apr 18 2022, 7:15 PM
libcxx/include/exception
89

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

102

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.

127

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.

138

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 updated this revision to Diff 423646.Apr 19 2022, 9:04 AM
paulkirth marked 3 inline comments as done and an inline comment as not done.

Fix formatting and _NOEXCEPT

paulkirth marked 9 inline comments as done.Apr 19 2022, 9:07 AM

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

libcxx/include/exception
89

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.

98

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

paulkirth updated this revision to Diff 424023.Apr 20 2022, 2:10 PM

Update preprocessor conditions

paulkirth marked 2 inline comments as done.Apr 20 2022, 2:11 PM
phosek added inline comments.Apr 20 2022, 4:17 PM
libcxx/include/exception
138

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 added inline comments.
libcxx/include/exception
138

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 updated this revision to Diff 424060.Apr 20 2022, 5:15 PM

Fix formatting

paulkirth updated this revision to Diff 424061.Apr 20 2022, 5:20 PM

rename config file w/ correct extension

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

paulkirth updated this revision to Diff 424682.Apr 22 2022, 7:07 PM

Use unused parameter and remove noreturn specifier.

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

paulkirth updated this revision to Diff 424751.Apr 23 2022, 2:31 PM

Try to make preprocessor conditions consistant

phosek added inline comments.Apr 23 2022, 3:07 PM
libcxx/include/exception
99

Super minor nit

114

Super minor nit

paulkirth updated this revision to Diff 425357.Apr 26 2022, 5:26 PM

Use different approach to handling missing defintions.

The more complex preprocessor conditions may not be necesary

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

change include to vcruntime.h to address bad definitions

paulkirth updated this revision to Diff 426197.Apr 29 2022, 5:30 PM

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 added inline comments.Apr 29 2022, 6:36 PM
libcxx/include/exception
112

Nit

119

Nit

121

Nit

126

Nit

140

Nit

paulkirth updated this revision to Diff 426204.Apr 29 2022, 6:57 PM

run clang-format

paulkirth marked 5 inline comments as done.Apr 29 2022, 6:59 PM
paulkirth updated this revision to Diff 426457.May 2 2022, 10:50 AM

Remove LIBCXX_EXCEPTION_ABI specifiers.

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

paulkirth updated this revision to Diff 426519.May 2 2022, 2:18 PM

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

paulkirth updated this revision to Diff 426522.May 2 2022, 2:34 PM

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

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?

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?

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.

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 updated this revision to Diff 427139.May 4 2022, 2:27 PM

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

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

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 updated this revision to Diff 427164.May 4 2022, 5:06 PM

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 added inline comments.May 4 2022, 10:48 PM
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 updated this revision to Diff 427376.May 5 2022, 10:04 AM

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

paulkirth updated this revision to Diff 427453.May 5 2022, 2:26 PM

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

paulkirth updated this revision to Diff 427454.May 5 2022, 2:30 PM

Don't reintroduce LIBCXX-WINDOWS-FIXME

paulkirth marked an inline comment as done.May 5 2022, 2:30 PM

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

ldionne requested changes to this revision.May 18 2022, 8:25 AM

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
117–133

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
399 ↗(On Diff #427454)

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.

This revision now requires changes to proceed.May 18 2022, 8:25 AM
mstorsjo added inline comments.May 18 2022, 9:05 AM
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.)

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

libcxx/include/exception
117–133

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
399 ↗(On Diff #427454)

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 updated this revision to Diff 430524.May 18 2022, 4:03 PM

Address review comments

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

friendly ping.

ldionne added inline comments.Jun 8 2022, 2:17 PM
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 added inline comments.Jun 8 2022, 2:24 PM
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 added inline comments.Jun 9 2022, 1:39 AM
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 requested changes to this revision.Jun 22 2022, 9:54 AM
ldionne added inline comments.
libcxx/include/exception
116

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
154 ↗(On Diff #434520)

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
377–379 ↗(On Diff #434520)

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
568 ↗(On Diff #419284)

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?

This revision now requires changes to proceed.Jun 22 2022, 9:54 AM
paulkirth updated this revision to Diff 439562.Jun 23 2022, 4:06 PM

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 marked an inline comment as done.Jun 24 2022, 10:35 AM
paulkirth added inline comments.
libcxx/include/exception
116

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
154 ↗(On Diff #434520)

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 updated this revision to Diff 439889.Jun 24 2022, 2:18 PM
paulkirth marked an inline comment as done.

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

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

thieta added a subscriber: thieta.Aug 9 2022, 4:47 AM

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

paulkirth updated this revision to Diff 451339.Aug 9 2022, 8:02 PM

Disable experimental in buildbot config & Rebase

paulkirth updated this revision to Diff 451340.Aug 9 2022, 8:09 PM

Fix diagnostic from clang-tidy

paulkirth updated this revision to Diff 451485.Aug 10 2022, 8:30 AM

Properly fix formatting for clang_tidy.

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

ldionne accepted this revision.Aug 17 2022, 12:23 PM

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
3 ↗(On Diff #451485)
This revision is now accepted and ready to land.Aug 17 2022, 12:23 PM
paulkirth updated this revision to Diff 453422.Aug 17 2022, 2:06 PM

Address nit

paulkirth updated this revision to Diff 453423.Aug 17 2022, 2:10 PM
paulkirth edited the summary of this revision. (Show Details)

Rebase and update summary

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

This revision was landed with ongoing or failed builds.Aug 17 2022, 2:14 PM
This revision was automatically updated to reflect the committed changes.