This makes exception handling a lot simpler, since we don't have to convert any exceptions this way. Is also properly handles all the user-thrown exceptions.
Details
- Reviewers
ldionne - Group Reviewers
Restricted Project - Commits
- rGaade74675c15: [libc++][PSTL] Overhaul exceptions handling
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
libcxx/include/__algorithm/pstl_backends/cpu_backends/any_of.h | ||
---|---|---|
69 | We should document how backends are expected to handle exceptions in libcxx/include/__algorithm/pstl_backend.h and in libcxx/include/__algorithm/pstl_backends/cpu_backend.h. | |
libcxx/include/__algorithm/pstl_backends/cpu_backends/find_if.h | ||
100 | Notes from our 1:1 session: // Option 1: // Pros: // - CPU sub-backends don't have to handle user exceptions in any special way. They only have to care // about any exception that might be thrown in their own setup code (aka std::bad_alloc). // - Handling of excetpions at the `__pstl_foo` level is uniform across all algorithms. // - We can write all code inside the PSTL with the mindset that exceptions are disabled, // i.e. we never throw and we use appropriate types to report errors instead. This might be // a nice systematic approach? // Cons: // - Requires introducing a custom std::expected-like type for internal use. // - Hardcodes the fact that we can only throw `std::bad_alloc` from backends. // - We can't elide the move construction of the return value out of `__pstl_find_if`. template <class _ExecutionPolicy, class _ForwardIterator, class _Predicate> _LIBCPP_HIDE_FROM_ABI _ForwardIterator __pstl_find_if(__cpu_backend_tag, _ForwardIterator __first, _ForwardIterator __last, _Predicate __pred) { auto __maybe_result = std::__terminate_on_exception([&] -> __funky_expected<_ForwardIterator, std::bad_alloc> { if constexpr (__is_parallel_execution_policy_v<_ExecutionPolicy> && __has_random_access_iterator_category<_ForwardIterator>::value) { return std::__parallel_find( __first, __last, [&__pred](_ForwardIterator __brick_first, _ForwardIterator __brick_last) { return std::__pstl_find_if<__remove_parallel_policy_t<_ExecutionPolicy>>( __cpu_backend_tag{}, __brick_first, __brick_last, __pred); }, less<>{}, true); } else if constexpr (__is_unsequenced_execution_policy_v<_ExecutionPolicy> && __has_random_access_iterator_category<_ForwardIterator>::value) { using __diff_t = __iter_diff_t<_ForwardIterator>; return std::__simd_first(__first, __diff_t(0), __last - __first, [&__pred](_ForwardIterator __iter, __diff_t __i) { return __pred(__iter[__i]); }); } else { return std::find_if(__first, __last, __pred); } }); return *std::move(__maybe_result).or_else([](auto&& __exception) -> __funky_expected<_ForwardIterator, void> { std::__throw_bad_alloc(); }); } // Option 2: // Pros: // - Each layer fully handles all of its exceptions independently. That can be simpler or more complicated depending // on how you look at it. // Cons: // - When you're writing a CPU backend, you need to explicitly handle user-thrown exceptions and carve out the // setup-thrown exceptions from the `terminate_on_exception` code paths. template <class _ExecutionPolicy, class _ForwardIterator, class _Predicate> _LIBCPP_HIDE_FROM_ABI _ForwardIterator __pstl_find_if(__cpu_backend_tag, _ForwardIterator __first, _ForwardIterator __last, _Predicate __pred) { if constexpr (__is_parallel_execution_policy_v<_ExecutionPolicy> && __has_random_access_iterator_category<_ForwardIterator>::value) { return std::__parallel_find( // we would document that CPU sub-backends need to handle exceptions appropriately __first, __last, [&__pred](_ForwardIterator __brick_first, _ForwardIterator __brick_last) { return std::__pstl_find_if<__remove_parallel_policy_t<_ExecutionPolicy>>( __cpu_backend_tag{}, __brick_first, __brick_last, __pred); }, less<>{}, true); } else if constexpr (__is_unsequenced_execution_policy_v<_ExecutionPolicy> && __has_random_access_iterator_category<_ForwardIterator>::value) { using __diff_t = __iter_diff_t<_ForwardIterator>; return std::__terminate_on_exception([&] { return std::__simd_first(__first, __diff_t(0), __last - __first, [&__pred](_ForwardIterator __iter, __diff_t __i) { return __pred(__iter[__i]); }); }); } else { return std::__terminate_on_exception([&] { return std::find_if(__first, __last, __pred); }); } } At the end, we both agreed that option 2 is the way to go for now, largely because implementing an expected-like type in C++17 drives the cost of option 1 up a lot. | |
119–121 | If an exception is thrown in here, we would also need to terminate. This needs to be fixed + a test. | |
123 | If an exception is thrown in here, we should also terminate even though this is actually running serial. This needs fix + tests. |
libcxx/include/__algorithm/pstl_backend.h | ||
---|---|---|
166–167 | And also below. | |
libcxx/include/__algorithm/pstl_backends/cpu_backend.h | ||
52 | ||
libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/pstl.fill.pass.cpp | ||
27 | This include is not necessary. | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/pstl.fill.terminate.pass.cpp | ||
30 ↗ | (On Diff #537895) | Can we also test iterators that throw when some operation (e.g. increment) is applied to them? I think this might catch issues in backends when they e.g. compute the size of the range. For this algorithms but others as well. Or throwing on copy of the iterator. Actually, this raises the question of how to handle throwing iterators at all. I think this approach doesn't work, since an iterator could technically throw even in one of the frontend functions. It's possible that the only approach that will work is to actually make the top-level function noexcept (but still throw std::bad_alloc in a few cases). |
libcxx/include/__algorithm/pstl_backends/cpu_backends/libdispatch.h | ||
---|---|---|
110 | noexcept? | |
148–149 | We don't need __terminate_on_exception anymore here. | |
libcxx/include/__algorithm/pstl_backends/cpu_backends/merge.h | ||
43 | noexcept? | |
libcxx/include/__algorithm/pstl_backends/pstl_expected.h | ||
1 ↗ | (On Diff #548302) | As discussed, let's go for std::optional. It handles triviality properly and it resolves a lot of complexity that we run into if we try to do expected. |
libcxx/include/__algorithm/pstl_merge.h | ||
77–80 | We need a test that we behave properly if the iterators throw when they are moved here. Also note that it would be possible to pass by rvalue ref into __pstl_merge and drop __terminate_on_exception altogether here (and in fact I don't think we'd need it anywhere anymore, which would be great). But I'm neutral on that, what I care about most is to add tests for this. | |
libcxx/include/__numeric/pstl_transform_reduce.h | ||
78–82 | Same thing for moving iterators applies here. | |
88 | This could be: *std::__terminate_on_exception(...).or_else([]{ std::__throw_bad_alloc(); }); We only need to use some pre-C++23 or_else version. |
libcxx/include/__algorithm/pstl_backends/cpu_backend.h | ||
---|---|---|
52 | Actually, I think we want to say: CPU backends are expected to report errors (i.e. failure to allocate) by returning a disengaged `optional` from their implementation. Exceptions shouldn't be used to report an internal failure-to-allocate, since all exceptions are turned into a program termination at the front-end level. When a backend returns a disengaged `optional` to the frontend, the frontend will turn that into a call to `std::__throw_bad_alloc();` to report the internal failure to the user. | |
libcxx/include/__algorithm/pstl_backends/cpu_backends/find_if.h | ||
109 | Let's add an internal assertion here to document why __pstl_find_if will never return nullopt (the reason is that unseq isn't allowed to allocate, assuming that's true). If that's not true, then this is a bug. | |
libcxx/include/__algorithm/pstl_backends/cpu_backends/libdispatch.h | ||
67 | Per our discussion just now, we should actually remove noexcept here! | |
libcxx/include/__algorithm/pstl_backends/cpu_backends/transform_reduce.h | ||
166 | Same, internal assertion. | |
libcxx/include/__algorithm/pstl_for_each.h | ||
65–66 | As discussed, it looks like we actually need to do this (otherwise we'd terminate in case std::for_each(policy, ...) throws bad_alloc): template <class _ExecutionPolicy, class _ForwardIterator, class _Size, class _Function, class _RawPolicy = __remove_cvref_t<_ExecutionPolicy>, enable_if_t<is_execution_policy_v<_RawPolicy>, int> = 0> _LIBCPP_HIDE_FROM_ABI void __for_each_n(_ExecutionPolicy&& __policy, _ForwardIterator __first, _Size __size, _Function __func) noexcept { _LIBCPP_REQUIRE_CPP17_FORWARD_ITERATOR(_ForwardIterator); return std::__pstl_frontend_dispatch( _LIBCPP_PSTL_CUSTOMIZATION_POINT(__pstl_for_each_n), [&](_ForwardIterator __g_first, _Size __g_size, _Function __g_func) -> optional<__empty> { if constexpr (__has_random_access_iterator_category_or_concept<_ForwardIterator>::value) { std::for_each(__policy, std::move(__g_first), __g_first + __g_size, std::move(__g_func)); return __empty{}; } else { std::for_each_n(std::move(__g_first), __g_size, std::move(__g_func)); return __empty{}; } }, __first, __size, std::move(__func)); } template <class _ExecutionPolicy, class _ForwardIterator, class _Size, class _Function, class _RawPolicy = __remove_cvref_t<_ExecutionPolicy>, enable_if_t<is_execution_policy_v<_RawPolicy>, int> = 0> _LIBCPP_HIDE_FROM_ABI void for_each_n(_ExecutionPolicy&& __policy, _ForwardIterator __first, _Size __size, _Function __func) { auto __res = std::__for_each_n(...); if (!__res) std::__throw_bad_alloc(); } And then we should use the noexcept version from fallback implementations. | |
libcxx/include/__utility/terminate_on_exception.h | ||
0 | I think this is just not needed at all anymore. |
libcxx/include/__algorithm/pstl_any_all_none_of.h | ||
---|---|---|
14 | Missing <optional> include. Please make a pass throughout. | |
libcxx/include/__algorithm/pstl_backends/cpu_backends/any_of.h | ||
22–23 | <__utility/move.h> | |
libcxx/include/__algorithm/pstl_backends/cpu_backends/find_if.h | ||
39 | This seems like a leftover, we decided to only mark the top-level __find algo noexcept. | |
libcxx/include/__algorithm/pstl_backends/cpu_backends/thread.h | ||
38 | noexcept leftover | |
libcxx/include/__algorithm/pstl_backends/cpu_backends/transform.h | ||
48 | noexcept leftover | |
95 | noexcept leftover | |
libcxx/include/__algorithm/pstl_is_partitioned.h | ||
61 | In another patch, can you please make a pass to make sure you're not missing _LLIBCPP_REQUIRE_CPP17_FOO from any of them? | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/pstl.fill.terminate.pass.cpp | ||
1 ↗ | (On Diff #548774) | Let's name this test pstl.exception_handling.pass.cpp since this isn't only specific to std::fill. WDYT? |
13 ↗ | (On Diff #548774) | You can adjust all the comments |
13 ↗ | (On Diff #548774) | Also you should mention that you're testing std::fill_n here (this applies to all these tests). |
40 ↗ | (On Diff #548774) | everywhere |
43 ↗ | (On Diff #548774) | Please add a comment like // test std::fill_n to separate each algorithm you're testing. This is kinda dumb here cause there's 2, but when there's more it's useful to have a visual marker |
libcxx/test/std/algorithms/alg.nonmodifying/alg.foreach/pstl.for_each.terminate.pass.cpp | ||
1 ↗ | (On Diff #548774) | I think you need to UNSUPPORT these tests when -fno-exceptions |
Missing <optional> include. Please make a pass throughout.