Totally-not-a-tracking-issue for UB-detecting debug assertions in the standard library

saethlin commented 9 months ago

What's this?

For a long time, we've been trickling support into the standard library for detecting at runtime calls to unsafe functions that violate their documented preconditions. These checks have become way more interesting with https://github.com/rust-lang/rust/pull/120594 because they can now trip in default cargo run/test. Some nice people recently prompted me to list what improvements I want to make on top of that PR, so I'm going to track them here.

If you're interested in working on this topic, this issue can be a hub for coordinating that effort. I am not offering mentoring per se but I'd be happy to discuss work on this topic.

Things to do next

[x] Convert debug_assert_nounwind to use the new intrinsic. https://github.com/rust-lang/rust/pull/120863
- Where reasonable, we should deduplicate checks by hand. For example get_unchecked should use intrinsics::unreachable instead of hint::unreachable_unchecked and comment why.
- Assess compile-time impact, and find hot checks by building regressed benchmarks with --emit=llvm-ir and searching the IR for the check calls.
[x] Unify debug_assert_nounwind and assert_unsafe_precondition, while ensuring that Library UB checks are checked in Miri/const-eval as described in this comment: https://github.com/rust-lang/rust/pull/121583#issuecomment-1963168186 PR is up at https://github.com/rust-lang/rust/pull/121662
[ ] Try to replace the intrinsic function with a lang-item const. This might produce less MIR (and thus improve compile times) but the implementation complexity inside the compiler might not be worth it. (This idea has been obviated by lowering the intrinsic function to a Mir::NullOp, which has very nearly the same effect)
[ ] Improve error messages when assertions are hit. These checks are currently done in outlined functions, so code size is not as big of a concern as it was when all the checks were inlinable. (This has become a dubious proposition in light the desire to have optimized + debug assertions builds. Maybe we can have good error messages in the cold checks?)
[x] The actual checks are hidden behind #[inline(never)] to prevent monomorphizing them many times, but that attribute also means LLVM cannot see what is being checked and optimize on it. Try changing the monomorphic check function from #[inline(never)] to some new attribute that makes them inlinable by LLVM, but not by the MIR inliner. Perhaps we call this #[inline(only_post_mono)]? https://github.com/rust-lang/rust/pull/121114
[ ] Try to deduplicate checks in a MIR pass or codegen. It's possible that after GVN we end up with MIR where a call dominates another call with the same argument(s).
[x] Branching on a const not known until monomorphization produces some CFGs which would be fixed by the MIR pass SimplifyCfg but aren't. https://github.com/rust-lang/rust/pull/120650 targets this pattern, but we should try to revive the strategy in https://github.com/rust-lang/rust/pull/91222 as well: https://github.com/rust-lang/rust/pull/121421
[ ] It might be nice to have these checks enabled for dependencies but not the top-level crate, which is tricky if the top-level crate monomorphizes code from a dependency that makes an invalid call. Can we use caller location information to check if the monomorphizing crate or the caller crate has debug assertions enabled?
[ ] Search the standard library for uses of functions that have one of these delayed debug asserts, and see if they can be replaced with safe code. I found quite a few of these with NonNull::new_unchecked, I'm sure there are others.

Implementation history

the8472 commented 9 months ago

Try changing the monomorphic check function from #[inline(never)] to some new attribute that makes them inlinable by LLVM, but not by the MIR inliner. Perhaps we call this #[inline(only_post_mono)]?

There is the rust-cold calling convention. That probably shouldn't be inlined by the mir inliner since we want to tell LLVM that's coldcc.

saethlin commented 9 months ago

The LLVM LangRef documents this about coldcc:

Furthermore the inliner doesn’t consider such function calls for inlining.

So I don't think that does the post-mono-only inlining that I described.

rust-lang / rust