Elide bounds checks

[cesarb]

No description provided.

[burdges]

rust-lang/rust@6a7bc47

[valarauca]

Sounds good. It seems the nearby if statements should also be updated to this style.

The nearby panic around line 353 should become x = &x[..y.len()]; also. This won't change the guarantees of the interface at all (it'll still panic on unequal lengths) but we'll lose the fancier error message. So nothing of value lost.

The same change should be made on ct_slice_eq as well. This would change a failing condition to a panicing condition so the docs/comment would have to updated to reflect it.

If you can make those two changes I'll accept and update the docs.

[cesarb]

The nearby panic around line 353 should become x = &x[..y.len()]; also.

No, that completely misses the point of this change. The reason for let y = &y[..x_len] is that the loop is for i in 0..x_len, or after a trivial value propagation, for i in 0..x.len(). Within the loop, LLVM therefore knows that x[i] can never be out of bounds, since i < x.len() is always true; so LLVM treats the bounds check for x[i] as "always in-bounds" and discards the path where the bounds check fails as unreachable.

However, LLVM can't do that for y[i], since it's not smart enough to notice that, when entering that loop, y.len() >= x.len(), and that it means i < y.len() is also always true.

The trick works due to the fact that slices in Rust are actually a pair of variables: a pointer to the first element, and the length. What the let y = &y[..x_len] (let y = &y[..x.len()] after a trivial value propagation) does is to copy the "length" part of x to the "length" part of y, so within the loop i < y.len() becomes i < x.len() which it knows is always true.

Of course, that introduces another bounds check for y.len() >= x.len(), but we just asserted in the preceding block that y.len() == x.len() so it's trivially true and also removed.

Now consider what would happen if I added a x = &x[..y.len()];. Since the loop is still for i in 0..x_len, that would introduce a new bounds check for x[i].

This won't change the guarantees of the interface at all (it'll still panic on unequal lengths) but we'll lose the fancier error message.

We still need the test for x_len != y_len, otherwise there will still be bounds checks. I would prefer to instead change the test to assert_eq!(x_len, y_len, "Consistent Time: Attempted to copy between non-equal lens {} and {}", x_len, y_len) which gives an even fancier error message.

The same change should be made on ct_slice_eq as well. This would change a failing condition to a panicing condition so the docs/comment would have to updated to reflect it.

Same as above: it's better to keep the test as-is.

[valarauca]

Ah okay. Thank you for explaining a bit better.

Accepted