Page MenuHomePhabricator

[libcxx] Introduce the mechanism for fixing -fno-exceptions test failures.
AbandonedPublic

Authored by rmaprath on Nov 13 2015, 6:10 AM.

Details

Summary

Summary:

r252598 and r252870 added XFAILs to all those tests that are currently
failing on the -fno-exceptions libc++ library variant. This patch introduces
a mechanism to fix those tests.

Details:

Most of the failing tests have checks of the following nature:

// [1] setup some global state
try {
  do_something_bad();
  assert(false);
} catch (some_exception e) {
  // [2] check some (mostly global) state
  // for consistency.
}
// [3] check some more state
// [4] possibly more try/catch clauses

These tests have been written with the -fexceptions library in mind and does
not compile with -fno-exceptions, and hence cannot be used as-is for testing
the -fno-exceptions library variant.

The current patch attempts to improve the testing of the -fno-exceptions library
variant by leveraging as much as possible of these existing tests, without having
to change the tests themselves massively.

We have the following goals in mind:

  1. do_something_bad() should call abort() in the -fno-exceptions library variant. The tests should be able to verify this.

    Note: The -fno-exceptions library variant is a custom extension, the behaviour of this library when responding to error states does not fall into the scope of the C++ standard (which always assumes exceptions). Here we have made the decision to call abort() because:

    I. It captures the intention of -fno-exceptions: when the library has entered an error state and has no means of notifying it to user code, it makes sense to abort the program rather than continue.

    II. This behaviour is in-line with the IA64 CXX ABI: https://mentorembedded.github.io/cxx-abi/abi-eh.html#base-framework Note the unwinding process, where it says: "If the search phase reports failure, e.g. because no handler was found, it will call terminate() rather than commence phase 2"
  2. The abort() call in 1) should not terminate the test itself, as that would prevent any follow-up tests (in the same source file) from running.
  3. We should leverage as much of the existing tests as possible, without having to change them drastically.

These goals lead us to the following approach:

We introduce a special support header file (noexcept.h) which when included in
the test file, re-writes code segments like the one above with something along
the lines of:

iflabel:
// [1] setup some global state
if (!abort_called()) {
  do_something_bad();
  assert(false);
} else {
  // [2] check some (mostly global) state
  // for consistency.
}
// [3] check some more state
// [4] possibly more try/catch clauses

The custom abort() method provided in the support header file makes a jump to
"iflabel" when called, causing the else branch to be taken after the abort()
call. This mechanism allows us to achieve all of the above goals without having
to modify the test cases greatly (in most cases, simply including the noexcept.h
header is all that is needed).

Are there alternatives approaches to improve the testing? without this kind of
re-writing of try/catch clauses?

Two approaches come to mind:
  1. Conditionalize try/catch/throw statements on _LIBCPP_NO_EXCEPTIONS
     and do the necessary adjustments case by case. This requires more effort
     and modifies the existing tests a lot.
  2. Keep the current XFAILS, add separate tests for the -fno-exceptions
     library variant. Again, this is quite an involved task.

In sum, the approach above gives us the most benefit (able to catch quite a lot
of defects in the -fno-exceptions library) with significantly less effort. Of
course, additional tests can be added that specifically test the -fno-exceptions
library variant. Such tests may use _LIBCPP_NO_EXCEPTIONS explicitly.

Aside from the tests, we have also introduced one configuration header file to
the library itself (__noexcept). This header file introduces utility functions
that can be used to simplify the -fno-exceptions code path. The following
example shows how the library handles the -fno-exceptions as of today:

if (some_bad_state()) {
 #ifndef _LIBCPP_NO_EXCEPTIONS
   throw some_exception("some message");
 #else
   // Do one of:
   //  - assert(false)
   //  - abort()
   //  - ignore
 #endif
}

Note: In the sample test fix (.../array/at.pass.cpp and include/array) attached
to this patch, an assert() is used in the -fno-exceptions code path. The use of
assert() is unsafe here as compiling with NDEBUG will get rid of them, resulting
in code that essentially ignores the erroneous state.

The test changes presented above catches such situations where abort() is not
called. The throw_helper() functions in the __noexcept header does the right
thing depending on the availability of exceptions. So the library developers can
use these helper functions and simplify the code. With the use of helper
functions, above code snippet reduces to:

#include <__noexcept>
...
if (some_bad_state())
  throw_helper<some_exception>("some message");

Note that no significant loss of readability is introduced. This also has the
added benefit of being able to easily track down all the places in the library
that throws exceptions.

Diff Detail

Event Timeline

rmaprath updated this revision to Diff 40145.Nov 13 2015, 6:10 AM
rmaprath retitled this revision from to [libcxx] Introduce the mechanism for fixing -fno-exceptions test failures..
rmaprath updated this object.
rmaprath updated this object.
rmaprath added a subscriber: cfe-commits.
rmaprath updated this revision to Diff 40307.Nov 16 2015, 9:36 AM

Tidied up the patch a bit:

  • Got rid of the different variants of the throw_helper function, can do with one in almost all the cases (the exceptional case will be submitted for review separately).
  • Modified __config a little to make the compiler not complain about re-defining _LIBCPP_NO_EXCEPTIONS macro when compiling with -fno-exceptions (NFC).

It would be nice if the no-exceptions library called an intermediate helper, for example __libcxx_noexceptions_abort(), instead of calling abort() directly. Then the user and the tests could provide a replacement for __libcxx_noexceptions_abort() instead of abort() which has other uses.

jroelofs added inline comments.Nov 19 2015, 8:18 AM
include/__noexcept
22 ↗(On Diff #40307)

We're not allowed to pollute the global namespace. This must be prefixed with __.

rmaprath updated this revision to Diff 40669.Nov 19 2015, 9:58 AM

Addressing review comments:

  • Renamed throw_helper as __throw_helper in order to not pollute the global namespace (@jroelofs)
  • Made try_buf a thread_local in order to be able to support those tests the launch multiple threads.
  • Introduced __libcxx_noexceptions_abort() as an intermediary between the no-exceptions library and the c library's abort() (@scott-0).
    • This had the side-benefit that we now don't have to hack around assert() in noexcept.h test support header; neat!
jroelofs added inline comments.Nov 19 2015, 10:21 AM
include/__noexcept
19 ↗(On Diff #40669)

Make this:

void __attribute__((weak)) __libcxx_noexceptions_abort();
36 ↗(On Diff #40669)

Then this should be:

if (__libcxx_noexceptions_abort)
  __libcxx_noexceptions_abort();
else
  abort();
src/noexcept.cpp
1 ↗(On Diff #40669)

And delete this whole file.

jroelofs added inline comments.Nov 19 2015, 10:25 AM
include/__noexcept
19 ↗(On Diff #40669)

I meant to write:

void _LIBCPP_WEAK __libcxx_noexceptions_abort();
rmaprath updated this revision to Diff 40707.Nov 19 2015, 2:36 PM

Addressing review comments by @jroelofs:

  • Got rid of the default implementation for __libcxx_noexceptions_abort by making it a weak reference.
jroelofs edited edge metadata.Nov 19 2015, 3:00 PM
jroelofs added a subscriber: EricWF.

I don't have any more comments, but I don't feel comfortable LGTM-ing this. I'll save that for Eric or Marshall.

test/support/noexcept.h
15

s/</ </

21

I believe there are platforms that don't support thread_local, but which are not _LIBCPP_HAS_NO_THREADS platforms. I don't know of a good way to test for that :/ maybe @EricWF has an idea?

rmaprath updated this revision to Diff 40767.Nov 20 2015, 4:19 AM
rmaprath edited edge metadata.

Addressing review comments:

  • Fixed a couple of typos.
  • Made the use of the thread-local storage specifier a bit more flexible to allow testing on few other configurations.

Gentle ping.

rmaprath updated this revision to Diff 42175.EditedDec 8 2015, 7:06 AM

Addressing review comments by @mclow.lists:

Got rid of the __noexcept header by moving the __throw_helper function into the __config header. The __libcxx_noexceptions_abort function now takes in an extra parameter containing the error message.

I removed the default call to abort() as it would be unwise (and impossible?) to call it from __config given its use across many headers. The current expectation is that the users of the no-exceptions library need to provide an implementation of __libcxx_noexceptions_abort if they wish to handle exceptional situations occurring within the no-exceptions library, otherwise such states will be simply ignored by the library.

jroelofs added inline comments.Dec 8 2015, 7:10 AM
test/support/noexcept.h
11

You should also add a header guard so that this doesn't get confused with one.

rmaprath updated this revision to Diff 42178.Dec 8 2015, 7:34 AM

Added a header guard to the noexcept.h test support header (review comment by @jroelofs).

Sorry I'm late to this party. Please let me know if my concerns have already been discussed.

First, thanks for taking on all this work. Getting the test suite passing without exceptions is a tall order.

My two cents:

  • I like the approach taken in the library. __throw_helper and __libcxx_noexceptions_abort(). However I don't like that they are in their own header. We should find another header for these to live in. <type_traits> tends to be the dumping ground for these sorts of things.
  • I'm concerned about the cost of the call to __throw_helper() when exceptions are turned on. Mostly because it copies the exception given to it but also because it's an unneeded layer of indirection. Perhaps we could have a macro called _LIBCPP_THROW(E, MSG) that is simply defined as throw E when exceptions are turned on and __throw_helper(E, MSG) otherwise.
  • I would really like to avoid #define try and #define catch. Replacing keywords has serious code smell and hides the fact that the test does something useful with exceptions turned off. It also has the slightest possibility of hiding bugs in libc++ (ex it might hide a 'try' keyword within a _LIBCPP_HAS_NO_EXCEPTIONS block). I would be happy simply using TEST_TRY and TEST_CATCH macros or similar. This communicates the intent of the test (w/o exceptions) to the reader.
  • While your setjmp/longjmp solution is quite clever it concerns me that longjmp doesn't respect destructors (AFAIK). This means that the tests may leak resources with -fno-exceptions. I would like to avoid this because I think it will prevent the use of sanitizers. Unfortunately I think this means that we will need to actually modify each test.
  • The #define try and #define catch only work when the expression only has one catch block and also when that catch block does not reference the caught exception by name. It seems like this would greatly limit the effectiveness of your approach. How many tests begin to pass after applying your patch?
  • We should document libc++'s support -fno-exceptions in general but we should also document each function that now has well defined behavior with -fno-exceptions as we update the tests.
  • Is __libcpp_noexceptions_abort() is intended to be a customization point for users of -fno-exceptions?
  • I like the approach taken in the library. __throw_helper and __libcxx_noexceptions_abort(). However I don't like that they are in their own header. We should find another header for these to live in. <type_traits> tends to be the dumping ground for these sorts of things.

I've moved these into the __config header after Marshall's comments. I chose __config as any standard header needing to throw exceptions would have to include the header which contains __throw_helper, which means the user of that standard header would end up (unknowingly) including the header which contains __throw_helper. __config seemed like a good option in this regard, but I can see that type_traits is also included by almost all the standard headers, so that should be fine as well.

  • I'm concerned about the cost of the call to __throw_helper() when exceptions are turned on. Mostly because it copies the exception given to it but also because it's an unneeded layer of indirection. Perhaps we could have a macro called _LIBCPP_THROW(E, MSG) that is simply defined as throw E when exceptions are turned on and __throw_helper(E, MSG) otherwise.

Agreed, will do this.

  • I would really like to avoid #define try and #define catch. Replacing keywords has serious code smell and hides the fact that the test does something useful with exceptions turned off. It also has the slightest possibility of hiding bugs in libc++ (ex it might hide a 'try' keyword within a _LIBCPP_HAS_NO_EXCEPTIONS block). I would be happy simply using TEST_TRY and TEST_CATCH macros or similar. This communicates the intent of the test (w/o exceptions) to the reader.

Note that a try statement within a _LIBCPP_NO_EXCEPTIONS guard will lead to a compilation failure (not allowed when compiling with -fno-exceptions). But I agree about the readability aspect. Our intention was to make the change as less intrusive as possible, so that contributors to the standard (with exceptions) library tests wouldn't have to worry about the no-exceptions case that much. I will update the patch to use TEST_TRY and TEST_CATCH explicitly.

  • While your setjmp/longjmp solution is quite clever it concerns me that longjmp doesn't respect destructors (AFAIK). This means that the tests may leak resources with -fno-exceptions. I would like to avoid this because I think it will prevent the use of sanitizers. Unfortunately I think this means that we will need to actually modify each test.

This is a tricky one. The issue here is, when the library encounters an exceptional situation and does not stop the control flow (either by throwing, aborting or longjumping) it will continue to execute ignoring the error. This means that whatever the follow-up asserts in the respective test cases will fail. So, if we are not going to really abort (as the test itself will terminate if we do this) the closest option is to longjmp. The alternative of course is to develop separate tests for the no-exceptions variant, which will check for the actual abort call and assert(false) if the test doesn't terminate. This requires a significant amount of effort.

  • The #define try and #define catch only work when the expression only has one catch block and also when that catch block does not reference the caught exception by name. It seems like this would greatly limit the effectiveness of your approach. How many tests begin to pass after applying your patch?

In my full patch, I encountered about may be ~10 cases where the catch blocks reference the thrown exception object. The solution was to simply disable those asserts that reference the exception object. I think this is acceptable from the no-exceptions library perspective.

There are few more tests which are irrelevant for the no-exceptions library variant. For example, those under tests/std/language.support/support.exception/ which specifically test the exception handling behaviour (IIUC). Also, tests related to thread futures where an exception thrown from a future is saved and later re-thrown into the get() calling thread. I think these tests do not fall into the scope of the no-exceptions library variant. I've left the XFAILs in for these tests.

Apart from those, I was able to fix all the remaining failures. In total, I think there are around 120 new passes. More importantly, I was able to dig out and fix quite a few places in the library sources where the -fno-exceptions library variant does the wrong thing (either assert or completely ignore the error state).

  • We should document libc++'s support -fno-exceptions in general but we should also document each function that now has well defined behavior with -fno-exceptions as we update the tests.

Agreed. I suppose this would be a separate section in http://libcxx.llvm.org/docs/?

Think it would be along the lines of:

+ -fno-exceptions library support
++ std::array
---- at(int n): calls __libcpp_noexceptions_abort() when n is invalid
---- ...
  • Is __libcpp_noexceptions_abort() is intended to be a customization point for users of -fno-exceptions?

Yes. Currently, if you use -fno-exceptions and link against the default (with-exceptions) library, bad things will happen. For starters, the headers will either assert or completely ignore the error and continue. Furthermore, when the library throws, I think the unwinder has to terminate the program as the exception cannot be propagated. On the other hand, if you use -fno-exceptions with the new no-exceptions library variant, you can be notified of error states through __libcxx_noexceptions_abort() and take appropriate action.

Marshall was thinking about some way to make the program fault at startup if you attempt to use -fno-exceptions alongside the default (with-exceptions) library build. I'm not sure if this is easily achievable though.

Thanks for the comments!

Best,

  • Asiri
EricWF edited edge metadata.Dec 10 2015, 5:08 AM

Before you spend time changing the tests to use TEST_TRY/TEST_CATCH I would like somebody else to weigh in. I don't really think my solution is that great either. @jroelofs, @mclow any thoughts?

A couple of more thoughts:

  • Forget what I said about putting it in <type_traits>. I think it's fine in __config
  • Because throw_helper no longer throws it should probably be renamed to something more appropriate.
  • __throw_helper and __libcpp_noexceptions_abort should probably be marked _LIBCPP_NORETURN. I can't think of a sane case where you would want to return.
rmaprath updated this revision to Diff 42440.Dec 10 2015, 10:04 AM
rmaprath edited edge metadata.

Addressing review comments by @EricWF:

  • Introduced _LIBCPP_THROW macro to appropriately throw or call the no-exceptions report routine to handle the exceptional situation (avoid the overhead of a function call in the default library). The compiler may be able to inline the call to __throw_helper to achieve the same effect, but wouldn't hurt to make it explicit.
  • Renamed __throw_helper to __libcxx_noexceptions_report as appropriate.
  • Added _LIBCPP_NORETURN qualifiers to both __libcxx_noexceptions_report and __libcxx_noexceptions_abort.
rmaprath updated this revision to Diff 42694.Dec 14 2015, 2:29 AM

Simplified the changes to __config header.

rmaprath updated this revision to Diff 42701.EditedDec 14 2015, 3:47 AM

Further refined the changes to __config header:

  • Now there's only one _LIBCPP_THROW macro, which does the right thing depending on the availability of exceptions.
  • Made __libcxx_noexceptions_report not take in the exception object as an argument. The only argument for this was that, with the exception object, we can produce a better error message. But this has the downside that the caller will have to instantiate an exception object even in the no-exceptions mode. Since the LIBCPP_THROW macro now requires an explicit error message for the no-exception variant, there's really no need to fiddle with the exception object.
rmaprath updated this revision to Diff 42977.Dec 16 2015, 2:51 AM

Two minor additions:

  • Introduced a TEST_THROW macro for those tests that need to throw exceptions (Note:- not using the _LIBCPP_THROW macro here as we want to keep the tests independent of the standard library implementation).
  • Updated tests/support/test_allocator.h to use the _LIBCPP_THROW macro rather than terminate the test when exceptions are disabled.
rmaprath marked 2 inline comments as done.Dec 16 2015, 2:52 AM
mclow.lists added inline comments.Dec 17 2015, 10:19 AM
test/support/noexcept.h
44

This is explicitly forbidden by the standard.
[macro.names]/2:
A translation unit shall not #define or #undef names lexically identical to keywords, to the identifiers listed in Table 2, or to the attribute-tokens described in 7.6.

rmaprath updated this revision to Diff 43237.Dec 18 2015, 9:13 AM

Addressing review comments by @mclow.lists:

  • Introduced TEST_TRY and TEST_CATCH macros to avoid the non-standard #define try/catch.
rmaprath marked an inline comment as done.Dec 18 2015, 9:14 AM
rmaprath updated this revision to Diff 45868.Jan 25 2016, 8:11 AM

Re-based on trunk. Trivial.

mclow.lists added inline comments.Jan 27 2016, 8:49 AM
include/__config
847

I don't care for having to specify something twice. (E, MSG). Maybe "stringify" E and make that the message.

test/support/noexcept.h
23

I don't care for this; I think that "implementing a mechanism for throwing exceptions in the test suite for when we've disabled exceptions" seems like something that we'll have to revisit time and time again.

I wonder if it would be better to just split some tests into multiple tests (some parts that test exception handling, some that don't), and then XFAIL: no-exceptions the ones that test exception handling.

rmaprath added inline comments.Jan 28 2016, 2:19 AM
test/support/noexcept.h
23

Not sure if I follow. This mechanism is not about throwing exceptions in the test suite; it is to check that the library calls __libcxx_noexceptions_abort() where it previously (with-exceptions) used to throw.

I don't see how splitting the tests would allow us to check that the library calls __libcxx_noexceptions_abort() as appropriate. We can XFAIL the tests that does exception handling, but then we won't be checking the library's behaviour in exceptional situations (i.e. how does it react to exceptional situations in the absence of exceptions?).

If we define that the no-exceptions library's behaviour in exceptional situations is *undefined*, then your suggestion makes sense, as then we don't have to worry about calling __libcxx_noexceptions_abort(). But this is not what we agreed upon in the dev list.

WDYT?

rmaprath abandoned this revision.Sep 13 2016, 3:44 AM