Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[libc++][PSTL] Overhaul exceptions handling

Authored by philnik on Jun 30 2023, 10:18 AM.


Group Reviewers
Restricted Project

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ldionne added inline comments.Aug 7 2023, 10:00 AM
159–160 ↗(On Diff #537895)

And also below.

51 ↗(On Diff #537895)
27 ↗(On Diff #537895)

This include is not necessary.

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 updated this revision to Diff 548302.Aug 8 2023, 11:48 AM

Use an expected type

ldionne added inline comments.Aug 8 2023, 2:59 PM
101 ↗(On Diff #548302)


140 ↗(On Diff #548302)

We don't need __terminate_on_exception anymore here.



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.

44 ↗(On Diff #548302)

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.

46 ↗(On Diff #548302)

Same thing for moving iterators applies here.

56 ↗(On Diff #548302)

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 updated this revision to Diff 548422.Aug 8 2023, 5:57 PM
philnik marked an inline comment as done.

Address comments

ldionne requested changes to this revision.Aug 9 2023, 9:27 AM
ldionne added inline comments.
78 ↗(On Diff #548422)

These should be updated for optional<foo>

163 ↗(On Diff #548422)

a disengaged optional. Same below.

51 ↗(On Diff #537895)

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.

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.

66 ↗(On Diff #548422)

Per our discussion just now, we should actually remove noexcept here!


Same, internal assertion.

58 ↗(On Diff #548422)

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>
__for_each_n(_ExecutionPolicy&& __policy, _ForwardIterator __first, _Size __size, _Function __func) noexcept {
  return std::__pstl_frontend_dispatch(
      [&](_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{};

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>
for_each_n(_ExecutionPolicy&& __policy, _ForwardIterator __first, _Size __size, _Function __func) {
  auto __res = std::__for_each_n(...);
  if (!__res)

And then we should use the noexcept version from fallback implementations.

25 ↗(On Diff #548422)

I think this is just not needed at all anymore.

This revision now requires changes to proceed.Aug 9 2023, 9:27 AM
philnik updated this revision to Diff 548774.Aug 9 2023, 2:41 PM
philnik marked 16 inline comments as done.

Address comments

ldionne added inline comments.Aug 10 2023, 8:58 AM
14 ↗(On Diff #548774)

Missing <optional> include. Please make a pass throughout.




This seems like a leftover, we decided to only mark the top-level __find algo noexcept.


noexcept leftover


noexcept leftover


noexcept leftover

60 ↗(On Diff #548774)

In another patch, can you please make a pass to make sure you're not missing _LLIBCPP_REQUIRE_CPP17_FOO from any of them?

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)


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

1 ↗(On Diff #548774)

I think you need to UNSUPPORT these tests when -fno-exceptions

philnik updated this revision to Diff 549081.Aug 10 2023, 9:38 AM
philnik marked 13 inline comments as done.

Address comments

ldionne accepted this revision.Aug 10 2023, 10:10 AM

LGTM, thanks!

This revision is now accepted and ready to land.Aug 10 2023, 10:10 AM
philnik updated this revision to Diff 549993.Aug 14 2023, 9:35 AM

Try to fix CI

philnik updated this revision to Diff 550018.Aug 14 2023, 10:27 AM

Try to fix CI

philnik updated this revision to Diff 550132.Aug 14 2023, 4:38 PM

Try to fix CI

philnik updated this revision to Diff 550429.Aug 15 2023, 12:23 PM

Fix formatting

philnik retitled this revision from [libc++][PSTL] Move exception handling to the CPU backends to [libc++][PSTL] Overhaul exceptions handling.Aug 15 2023, 2:53 PM
philnik edited the summary of this revision. (Show Details)
philnik updated this revision to Diff 550833.Aug 16 2023, 11:55 AM

Try to fix CI

philnik updated this revision to Diff 555638.Sun, Sep 3, 3:33 PM

Try to fix CI

philnik updated this revision to Diff 556472.Mon, Sep 11, 11:59 AM

Try to fix CI

philnik updated this revision to Diff 556491.Mon, Sep 11, 3:18 PM

Try to fix CI

philnik updated this revision to Diff 556725.Wed, Sep 13, 3:18 PM

Try to fix CI

philnik updated this revision to Diff 556928.Mon, Sep 18, 12:11 AM

Try to fix CI

philnik updated this revision to Diff 556953.Mon, Sep 18, 8:37 AM

Try to fix CI

philnik updated this revision to Diff 556993.Mon, Sep 18, 8:46 PM

Try to fix CI

philnik updated this revision to Diff 557002.Mon, Sep 18, 11:04 PM

Try to fix CTry to fix CII