[PATCH] D14653: [libcxx] Introduce the mechanism for fixing -fno-exceptions test failures.

[Eric Fiselier via cfe-commits]

EricWF added a reviewer: EricWF.
EricWF added a comment.

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?




http://reviews.llvm.org/D14653