[libcxx-commits] [libcxx] Suppress a redundant hardening check in basic_string_view::substr (PR #91804)

[David Benjamin via libcxx-commits]

davidben wrote:

> I expect there are redundant checks in other places too, do we want to eliminate all of them?

Yes, we _want_ to eliminate all _redundant_ safety checks. Whether we _can_ in each instance is a engineer tradeoff between performance and risk.

The point of safety checks isn't to check just because. It should generally be a goal that, when a safety check is _truly_ redundant (i.e. one can _prove_ that they are _always_ true) that it is eliminated. Such a check is pure overhead, and there are a lot of such instances. Most code is mostly correct, and so most naively-applied safety checks are redundant.

That doesn't mean we don't safety checks. The key word here is "most". Humans are fallible and, as we scale out to large C++ programs _using_ the STL, there will be failed preconditions. When those preconditions fail even once, the results are _catastrophic_ for users. We _need_ safety checks.

However, this balance of extremes means there are complex tradeoffs here. Safety checks are not expensive but, in aggregate and in hot paths, the costs of safety checks can add up. That is why it is important that most redundant ones be eliminated, so that projects have the performance budget for the ones that are either not redundant, or at least not obviously so. The best way to eliminate safety checks is via automatic compiler optimizations. That is one piece of code that we need to get right once, and then a whole class of redundant checks go away. Because usually the redundancy is locally obvious. E.g.

```
   std::optional<T> whatever = ...;
   if (whatever) {
     whatever->do_thing();
  }
```

It is totally fine that `operator->` has a safety check because the compiler can easily see the check is redundant and delete it.

Other times, however, that is not possible. When that happens, we have a choice to make:

1. Improve the compiler so that it does see it.
2. Find another way to structure the code such that the compiler can see it.
3. Rely on fallible humans to assert that the invariant holds (i.e. take on a risk) and use an unsafe pattern that suppresses the check.
4. Live with this check and take on the performance cost.

Now, the invariant here is such that neither (1) nor (2) is not currently viable. Our language does not have the tools to capture that `size_ < PTRDIFF_MAX`. We have to pick between (3) and (4). This is an engineering tradeoff. We can choose to take on slightly more risk for better performance, or we can choose to lose performance and reduce the risk.

In these cases, ideally the balance would tilt towards (4) as much as possible. The consequences of getting these wrong as so extreme that we generally don't want to take on that risk. But, for _this function_ and for _this check_, I think (3) is the clear winner:

First, we're _within the STL_, and in `string_view::substr` no less. This is a function we can expect to be called throughout C++ code, within parsers and whatnot to peel off substrings of your bigger structure. Performances costs here scale.

Second, we must look at the nature of the risk. This is _not_ a safety check for bugs in the calling code. (If it were, I would pretty much always argue for (4).) This is a safety check for _bugs in libc++_. While that is indeed possible, I think it's quite obvious why the precondition holds here. Moreover, that safety check is to catch things like -1 accidentally making its way into a `string_view` constructor and wrapping around to `SIZE_MAX`. That's not a concern here. Therefore the risk we take on is minimal.

Finally, we look at the second-order effects. Because this is specifically `string_view::substr`, avoiding the performance hit relieves some performance budget in our callers for them to then spend on safer constructs elsewhere, where the tradeoff between (3) and (4) may be less clear. That is overall a safety-positive outcome.

https://github.com/llvm/llvm-project/pull/91804