rmaprath added a comment. In http://reviews.llvm.org/D14653#306675, @EricWF wrote:
> - 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 `XFAIL`s 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 http://reviews.llvm.org/D14653 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
