[libcxx-commits] [PATCH] D146421: [libc++] Disable the __insertion_sort_unguarded optimization

[Louis Dionne via Phabricator via libcxx-commits]

ldionne created this revision.
Herald added a subscriber: mikhail.ramalho.
Herald added a project: All.
ldionne requested review of this revision.
Herald added a project: libc++.
Herald added a subscriber: libcxx-commits.
Herald added a reviewer: libc++.

The difference between __insertion_sort_unguarded and __insertion_sort
is that the former doesn't ensure that we don't run off the beginning
of the range. This assumes that the comparison predicate is a strict
weak order (which is a pre-condition to the algorithm). However, in
practice, some predicates may not satisfy the pre-condition in subtle
ways and the result is currently that we'll access elements outside of
the range.

We should obviously not weaken the preconditions of the algorithm,
however it seems like letting this situation happen without even a
way of diagnosing the issue is pretty bad. Hence, this patch disables
the optimization until we at least have a way to assert that we're not
going out of bounds for folks that would want to enable such assertions.

This is essentially a hotfix targeted for the release/16.x branch.
I plan to then revert this and apply a proper fix onto main so that
we can have this optimization (and also the choice to assert on it)
for LLVM 17.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146421

Files:
  libcxx/include/__algorithm/sort.h


Index: libcxx/include/__algorithm/sort.h
===================================================================
--- libcxx/include/__algorithm/sort.h
+++ libcxx/include/__algorithm/sort.h
@@ -737,7 +737,10 @@
     }
     // Use insertion sort if the length of the range is below the specified limit.
     if (__len < __limit) {
-      if (__leftmost) {
+      // TODO: Invalid comparison predicates can cause __insertion_sort_unguarded to go out-of-bounds, which is
+      //       highly undesirable. Disable this optimization until we at least have a way to assert that we're
+      //       not going out of bounds.
+      if (__leftmost || true) {
         std::__insertion_sort<_AlgPolicy, _Compare>(__first, __last, __comp);
       } else {
         std::__insertion_sort_unguarded<_AlgPolicy, _Compare>(__first, __last, __comp);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D146421.506570.patch
Type: text/x-patch
Size: 833 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20230320/b6508d00/attachment.bin>