[libc++][PSTL] Overhaul exceptions handling

[philnik]

Summary

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.

[ldionne]

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

[philnik]

Use an expected type

[ldionne]

libcxx/include/__algorithm/pstl_backends/cpu_backends/libdispatch.h

103

noexcept?

141–142

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.

[philnik]

Address comments

[ldionne]

libcxx/include/__algorithm/pstl_backend.h

80

These should be updated for optional<foo>

167

a disengaged optional. Same below.

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

66

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.

[philnik]

Address comments

[ldionne]

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

[philnik]

Address comments

[ldionne]

LGTM, thanks!

[philnik]

Try to fix CI

[philnik]

Try to fix CI

[philnik]

Try to fix CI

[philnik]

Fix formatting

[philnik]

Try to fix CI

[philnik]

Rebased

[philnik]

Try to fix CI

[philnik]

Try to fix CI

[philnik]

Try to fix CI

[philnik]

Try to fix CI

[philnik]

Try to fix CI

[philnik]

Try to fix CI

[philnik]

Try to fix CI

[philnik]

Try to fix CTry to fix CII