[PATCH] D150264: [libcxx] Add strict weak ordering checks to sorting algorithms

[Danila Kutenin via Phabricator via llvm-commits]

danlark added inline comments.


================
Comment at: libcxx/include/__algorithm/sort.h:704
   typedef typename iterator_traits<_RandomAccessIterator>::difference_type difference_type;
   using _Comp_ref = __comp_ref_type<_Compare>;
   // Upper bound for using insertion sort for sorting.
----------------
ldionne wrote:
> philnik wrote:
> > ldionne wrote:
> > > ldionne wrote:
> > > > Sorry for being so late to the party, but I think there's a problem here. We should decide whether we want to use these added facilities or `__comp_ref_type` to catch these kinds of issues. Right now, we have a weird situation where both can potentially be used at the same time. This means that we have the following mechanisms in place and enabled independently:
> > > > 
> > > > 1. The assertions I added that ensure that we would not dereference iterators out-of-bounds inside `std::sort`. Those are arguably hardening assertions, not only debug mode assertions.
> > > > 2. The existing `__comp_ref_type` mechanism which aims to catch invalid strict weak orders at the predicate level.
> > > > 3. The new mechanism introduced by this patch.
> > > > 
> > > > Is there a reason why we'd want to have these three severely overlapping mechanisms (and especially within the same algorithm)?
> > > CC @var-const 
> > I think they are complementary. All of them have their strengths and weaknesses.
> > 
> > - The assertions are relatively light weight, but miss a lot of the strict weak ordering issues. As you said yourself, these can be considered hardening asserts.
> > - The `__comp_ref_type` catches a lot more issues, but can't check across multiple pairs.
> > - This new check is //very// expensive, making it only suitable for a subrange and only in debug mode, but it catches a lot of the strict weak ordering issues.
> > 
> > I think the assertions aren't useful to combine with `__comp_ref_type` and the new checks, but also don't hurt. Having both for larger ranges makes sense, since the `__comp_ref_type` can check the whole range.
> Ok, that's something I could go with. However, in that case we need to reinstate the `invalid_comparator.pass.cpp` test to do what it did before. It was a hardening test that ensures that we don't go OOB inside these algorithms when the comparator is wrong. The goal was to ensure that we don't do that bad thing even when the debug mode is disabled. After this patch, however, that test became a test for the newly introduced strict weak order checks, which only make sense in debug mode.
> 
> @danlark Do you think you could pick that up?
I think we still check for what you ask in the comment:

Line 87:

```
TEST_LIBCPP_ASSERT_FAILURE(std::sort(copy.begin(), copy.end(), checked_predicate), "Would read out of bounds");
```

The test checks that out of bound reads check fires first. Then if it is not able to identify anything, the strict weak ordering check asserts.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150264/new/

https://reviews.llvm.org/D150264