Avoid unreachable integer underflow check in CStr::count_bytes()

[Darksonn]

Follow up to #162930.

cc @clarfonthey, @hanna-kruppe

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: libs
• libs expanded to 12 candidates
• Random selection from JohnTitor, LawnGnome, Mark-Simulacrum, clarfonthey, jhpratt

[hanna-kruppe]

This looks sound, but can you say a bit about what the benefit is? In #162912 you mentioned "A colleague of mine was trying to remove all panics in a Linux kernel driver" -- is this also intended to help eliminate an overflow check? I'm asking both because we should have something more reason than "we can" for using unsafe and because we should have a codegen test capturing that reason (or at least a reasonable proxy for it).

[clarfonthey]

FWIW, the technically-valid functionality of NonZero - 1 is something I've used a lot and have wanted to add a dedicated method for. But in this case it wouldn't help since we're dealing with slice lengths.

[Darksonn]

This is not some random function. CStr::from_ptr(ptr).count_bytes() is the way to invoke strlen from Rust code.

I do in general think it's worth it for core vocabulary types such as CStr to avoid useless panicking paths. As I mentioned in #162912, one reason is that they make it easier to find bugs in your code by listing all places where the final binary calls a panicking function.

I suppose there is a safe implementation:

self.to_bytes().len()

Of course since to_bytes_with_nul already contains an equivalent hint::assert_unchecked, it's not any safer.

[Darksonn]

Well, the codegen test is a good call. Interestingly enough, neither computing the length .unchecked_sub(1) or via self.to_bytes().len() lets LLVM know that the length is less than isize::MAX, and the tests/codegen-llvm/cstr-len-plus-one.rs test added by #162930 fails if modified to use s.count_bytes() instead of s.bytes().count().

Of course the isize::MAX bound is a separate concern from the subtraction underflow.

[hanna-kruppe]

I do in general think it's worth it for core vocabulary types such as CStr to avoid useless panicking paths.

I am sympathetic to this, as long as it doesn't impose undue maintenance burden. But for overflow checks in particular, I'm a bit skeptical because we don't ship the standard library with overflow checks enabled. It's possible to build the standard library like this (and maybe it'll become more common as build-std matures), but today it's a rare configuration that is rarely benchmarked and rarely optimized for.

I guess this PR and earlier ones are evidence that this is starting to change, but today there's probably many other places that have overflow checks. So if this is going to lead to a steady stream of PRs to find and eliminate such overflow checks, that seems like a bigger change that we should have a discussion in the libs team about at some point (e.g., how much do we care about this vs. maintainability, how do we catch regressions and prioritize them, should we have tests and CI jobs that exercise this directly, etc.).

[Darksonn]

Huh. I had not realized that he ran into this overflow check due to how Linux builds core. I thought this was how it worked for everyone. Back when I was working on the target modifiers RFC, it was explained to me that the stdlib has a way to inherit overflow checks, and that this is why -Coverflow-checks works without build-std.

[hanna-kruppe]

Sorry for my sloppy phrasing, the standard library does have ways to inherit overflow checks, but they're opt-in and fairly targeted (see overflow_checks intrinsic, and I think there's also something magic about using the primitive's Add etc. impls in generic code also used by impl Add for $Int etc). By volume, most arithmetic operators under library/* do not inherit overflow checks, including #[inline] methods like CStr::count_bytes: https://rust.godbolt.org/z/Mr8frq5cc

[Darksonn]

I opened a discussion about overflow checks on zulip:

#t-libs > Future of overflow checks in the standard library @ 💬

[Mark-Simulacrum]

@bors r+ rollup

View changes since this review

[Mark-Simulacrum]

I think I'm OK with this. An alternative could be to try to_bytes().len(), which technically should/could avoid the panic (to_bytes_with_nul, which it calls, has an assert_unchecked(!empty)).

But the added indirection doesn't seem like it has a ton of value and probably hurts optimization (at least in terms of how long it takes to compile).

[clarfonthey]

assert_unchecked does actually hurt a lot more than it helps, so, the unchecked sub should just help convey the length information properly, honestly.

[rust-bors]

📌 Commit ae2ecf5 has been approved by Mark-Simulacrum

It is now in the queue for this repository.