[libcxx-commits] [libcxx] [libc++][hardening] Check bounds on arithmetic in __bounded_iter (PR #78876)

[David Benjamin via libcxx-commits]

davidben wrote:

> Nit: in the description, there seems to be an unfinished sentence: [...]

Thanks! Rephrased.

> Consider the following (bad) code: [...]
> I think this is technically UB, but it's likely to be "benign" UB (I call these hobbit iterators because they go there and back again). The current implementation wouldn't reject this case, but with this patch, it will start trapping if get_offset returns a value beyond bounds.

I actually disagree that this is "benign" UB. When this function is inlined at a point where the compiler can see the bounds of the allocation, the out-of-bounds pointer arithmetic is very visible to the compiler. The consequence of this analysis is that the compiler learns bounds on the pointers, which would then allow it to delete the bounds-checks we wanted to add.

Moreover, by assuming this doesn't happen, we put ourselves in a very precarious position. We _want_ the optimizer to delete redundant bounds checks, and we want to avoid, as you say, tying the implementation too much to one particular optimizer. Those pressures mean we will want optimizers to get increasingly good at bounds analysis. But if we exhibit this UB, we will simultaneously want them to not get _too_ good at it. I don't think that's a good place to be in.

Indeed, see https://github.com/llvm/llvm-project/issues/78784. That is the missing observation that would have allowed Clang to optimize the current `__bounded_iter`. That observation relies on pointer arithmetic UB.

I would also argue that the additional checks are actually *more* optimizer-agnostic than the current ones, but I'll discuss that below.

> I think part of the reason __bounded_iter is currently implemented this way is a concern that projects out in the wild might inadvertently rely on "hobbit" iterators, and rejecting them would cause unnecessary friction on adoption. I don't have any actual data on this -- this might be a largely hypothetical concern. But I want to make sure that if we want to break code like this, we do so consciously.

Absolutely. I think should consciously break code like this:

1. I expect hobbit iterators are pretty rare, much rarer than hobbit pointers.
2. I really don't think this UB is benign (see above), so hobbit iterators and pointers are dubiously compatible with bounds-checking.
3. Not only is the new `__bounded_iter` better optimized _in practice_, it is better optimized because it aligns better with C++ iterator patterns. That is, it is inherently easier to optimize, not specific to one optimizer (see below).

I think those three are strong evidence that this change is worth the risk of impacting hobbit iterators.

> In general, while it's very important for this type to be performant, I'm a bit concerned if we start to tie the implementation "too much" to the optimizer. I want to be pragmatic here, so obviously some level of attention to the optimizer is necessary. What I'd like to avoid is relying on code that has subtle implicit interactions with the optimizer.

Could you elaborate a bit more on this concern? To me, assertions that are more straightforwardly deletable are _less_ tied to the optimizer, because it's more likely to be universal. Whereas assertions that rely on more complex reasoning that our optimizer happen to handle would be more tied to it. This PR _improves_ the situation by this criteria!

> an internal assertion that happened to improve performance by providing additional information to the optimizer.

Hmm, I'm not quite following this one. Could you elaborate? I suspect we may not be on the same page about why this new version is easier to optimize, so let me continue on and perhaps that'll help?

> IMO, from a higher-level (logical?) perspective, it never makes sense that having an additional check could improve performance (at best, it could have a negligible negative impact).

I would push back a bit on both of these. I don't think this higher-level intuition is correct. The additional checks *move the assertions closer to their preconditions*. That is inherently easier for any optimizer to handle.

> If we ever want to pass some information to the optimizer, we need to write it in a way that makes it explicit (something similar in spirit to _LIBCPP_ASSUME). It might expand to anything, including an assertion, under the hood, but the API must make it clear that the intention is to help the optimizer, not e.g. abort the program if the condition doesn't hold.

So, as discussed above, we specifically want to abort the program because otherwise we'll trigger non-benign UB and get ourselves into problems. But also `_LIBCPP_ASSUME` is currently a no-op because of an LLVM bug. :-) I do think we need a `_LIBCPP_ACTUALLY_ASSUME` to make other parts of bounded iterators 

> the != comparison optimizing better than <= comparison. I don't know if this is a fundamental property that the former optimizes better or it just happens to be so with the current implementation of the optimizer.

(The comparison is actually `<`, not `<=`. We want to reject `iter == end`.)

This is a fundamental property, both of how C++ is used and just what it means to optimize a check out. In order to delete a check, be it `!=` or `<`, the compiler must prove it _always_ holds. Providing `a != b` is inherently easier than proving `a < b` because it's a weaker statement to prove.

Moreover, the way C++ iterators work, `!=` is the natural theorem to prove. When you use iterators, in a range-for loop or elsewhere, the calling code writes `iter != end`, not `iter < end`. Think about what a ranged-for loop expands to:

```
auto it = c.begin();
while (it != c.end()) {
  f(*it);
  ++it;
}
```

In the current `__bounded_iter`, this becomes:

```
auto it = c.begin();
while (it != c.end()) {
  assert(c.begin() <= it && it < c.end());
  f(*it);
  ++it;
}
```

This is not an easy theorem to prove. In order to delete the check, the compiler must know that `c.begin() <= c.end()` and it doesn't know that right now. In order to know that... it must exploit pointer arithmetic UB (`c.end()` is `data + size`. Again, see https://github.com/llvm/llvm-project/issues/78784. I keep pointing at that one for a reason), the very pointer arithmetic UB that the current `__bounded_iter` incorrectly assumes is benign!

Now consider the `__bounded_iter`:

```
auto it = c.begin();
while (it != c.end()) {
  assert(it != c.end());
  f(*it);
  assert(it != c.end());
  ++it;
}
```

This is trivial to optimize. The compiler does not need any complex analysis at all, because we've the assertions exactly match the preconditions that the caller is *already* expected to use.

Now, in more complicated cases, we need the compiler to infer `<` too. That is, however, harder for the compiler and requires it know something about the relationship between `begin` and `end`. I think we should give it that relationship, with perhaps with `_LIBCPP_ACTUALLY_ASSUME`. But that does not change the fact that our iterators should work with `!=`, for all the reasons discussed here. That is, this PR makes sense independent of any future smarter optimization work.

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