debug_asserts within libstd are not hit unless using -Zbuild-std

[glandium]

STR:

$ cargo new testcase ; cd testcase
$ cat <<EOF > src/main.rs
fn main() {
    let a = [0; 2];
    let b = unsafe { a.get_unchecked(3) };
    println!("{}", b);
}
EOF
$ cargo +nightly run -q
32764
$ cargo +nightly run -q -Zbuild-std --target=x86_64-unknown-linux-gnu
thread 'main' panicked at /home/glandium/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/index.rs:228:9:
slice::get_unchecked requires that the index is within the slice
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread caused non-unwinding panic. aborting.

It would seem the intent is for the latter to always happen, but it doesn't.

$ rustc +nightly --version
rustc 1.77.0-nightly (cb4d9a190 2024-01-30)

[glandium]

Here is a reduced testcase (not minimal) from real (yes, broken, invoking UB) code, that, because the mentioned debug_assert is not hit, leads to a completely unrelated panic that "can't happen" because of the hint::assert_unchecked that was recently added:

#![allow(non_upper_case_globals, non_snake_case, dead_code)]

#[inline]
fn static_atoms() -> &'static [[u32; 3]; STATIC_ATOM_COUNT] {
    unsafe {
        let addr = &gGkAtoms as *const _ as usize + kGkAtomsArrayOffset as usize;
        &*(addr as *const _)
    }
}

#[inline]
fn valid_static_atom_addr(addr: usize) -> bool {
    unsafe {
        let atoms = static_atoms();
        let start = atoms.as_ptr();
        let end = atoms.get_unchecked(STATIC_ATOM_COUNT) as *const _;
        let in_range = addr >= start as usize && addr < end as usize;
        let aligned = addr % 4 == 0;
        in_range && aligned
    }
}

fn main() {
    println!("{:?}", valid_static_atom_addr(0));
}

(it requires optimizations to be enabled, build with RUSTFLAGS=-O cargo +nightly run ; funnily enough, this code crashes with --release, and works with plain cargo +nightly run)

See https://glandium.org/blog/?p=4354 for the full story.

[KisaragiEffective]

Isn't it caused by -O? RUSTFLAGS=-O is synonym for -C opt-level=2, which does not implies cfg(debug_assertions) (debug_assert! is compiled into no-op if debug_assertions is turned off).
You should experiment by explicitly turn on debug-assertions.

[glandium]

Isn't it caused by -O? RUSTFLAGS=-O is synonym for -C opt-level=2, which does not implies cfg(debug_assertions) (debug_assert! is compiled into no-op if debug_assertions is turned off). You should experiment by explicitly turn on debug-assertions.

-O makes for a non-sensical error in a debug build. A non-optimized debug build actually doesn't fail at all (unless using -Z build-std, in which case the get_unchecked debug_assert is hit), and a release build crashes with an Illegal Instruction. I don't think this is a good position to be in to debug something like this.

[the8472]

It's not so much "intended" but a known limitation since we ship a pre-built std without debug asserts. The issue will be solved eventually if/when build-std becomes stable.

[glandium]

The problem is that those debug_assert are followed with hints to the compiler that what's being asserted doesn't happen. So we're in a situation where UB will lead to (rightful) crashes in release builds, but debug builds showing nothing for it at all (since they're not optimized by default), or worse, can show something completely unrelated with optimization enabled (which some might do because non-optimized code can be really slow).

Someone on r/rust mentioned miri. Yes, miri finds the UB. But there are a tons of cases where miri just can't be used.

It's not uncommon to ship separate debug-variants of standard libraries. Rust should probably do that, because I don't see build-std becoming usable with cargo vendor any time soon.

[the8472]

I don't see build-std becoming usable with cargo vendor any time soon.

Can you elaborate on that? I don't know how vendoring interacts with build-std.

[glandium]

I don't see build-std becoming usable with cargo vendor any time soon.

Can you elaborate on that? I don't know how vendoring interacts with build-std.

rust-src doesn't contain the dependencies for libstd, and if you have a cargo config with replace-with as per the output from cargo vendor, build-std fails to find those dependencies because they're not vendored. And they can't be vendored because the code might be built against different versions of rust, with different dependencies for libstd.

There were attempts to make it work in #78790 and rust-lang/cargo#8834 but they were reverted because they caused other problems.

[the8472]

Ah, that's unfortunate. Afaik we're currently not shipping more flavors of std because the combinatorial explosion would bloat rustup component sizes and build-std is the intended solution to all of those. I can't speak for all the teams but I suspect it'd take some rather strong arguments to change the position on that. Work on improving build-std would be preferable.

Well, building your own toolchain might be a heavyweight alternative.

[NCGThompson]

I think that if a debug assertion triggers as a result of user code, then it would be nice for it to always obey the callers debug-assertions flag, but if the debug assertion triggers only as a result of a bug in the standard library, then it should only obey the debug-assertions flag that the standard library was compiled with.

Currently, functions that are dependent on the callers debug-assertions flag are macros. I think more of these macros can be added, especially for the "unchecked" functions. e.g. debug_unwrap_unchecked!(example_option).

[saethlin]

As the author of many of std's UB-detecting debug assertions and the pointer alignment checks, I've thought about this problem a lot.

if the debug assertion triggers only as a result of a bug in the standard library, then it should only obey the debug-assertions flag that the standard library was compiled with.

We cannot distinguish if a standard library function is calling an unsafe _unchecked function because it has passed the precondition on to an external caller, or if the standard library is calling it because the standard library is sure some usize is zero (and maybe it isn't). In both case the call is coming from inside the standard library, but in one of those cases the check is for the end user and in the other it's for the correctness of the standard library itself.

I'm (slowly, very slowly) trying to migrate the pointer alignment checks and also runtime checks for niches (which would cover things like null slices) out of a MIR transform and into codegen: #117473. One exciting consequence of doing this in codegen is that the checks get inserted into standard library functions that are public and generic, or only lowered to MIR for some other reason.

So there is an alternative approach here which might work. We could make all the standard library UB check assertions not check cfg!(debug_assertions) but instead check an intrinsic that is swapped in for a constant in codegen. I'm sure this would have significant impacts on compile time, but it might be worth it. And unlike "improve build-std" which is a monumental task, this could be done by one person in not that much time.

[RalfJung]

It would seem the intent is for the latter to always happen, but it doesn't.

Not entirely; the intent is to have the debug assertions for the rare case where they are useful, but we are fully aware that most users do not benefit from them in their current shape. We just haven't found a good way to ship this yet.

Someone on r/rust mentioned miri. Yes, miri finds the UB. But there are a tons of cases where miri just can't be used.

Shameless plug, cargo-careful also enables these checks. No idea if that works with cargo-vendor though.

[glandium]

Shameless plug, cargo-careful also enables these checks. No idea if that works with cargo-vendor though.

It sounds like it only works on bin crates.

[saethlin]

cargo-careful works by building a sysroot with different settings than the distributed one, then invoking Cargo with that sysroot. It should work on most crate types; for libraries one would probably just want to run tests. cargo +nightly careful test is mentioned in the README.

[saethlin]

I've closed this because the linked PR should make the example program panic. I didn't fix all debug_asserts in libstd, and the panic message is not very informative. I hope to improve both in the future.