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

[saethlin]

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 #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

• Convert debug_assert_nounwind to use the new intrinsic. Use intrinsics::debug_assertions in debug_assert_nounwind #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.
• 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: rename debug_assert_nounwind → debug_assert_ubcheck #121583 (comment) PR is up at Distinguish between library and lang UB in assert_unsafe_precondition #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?)
• 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)]? Add #[rustc_no_mir_inline] for standard library UB checks #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).
• Branching on a const not known until monomorphization produces some CFGs which would be fixed by the MIR pass SimplifyCfg but aren't. Use br instead of a conditional when switching on a constant boolean #120650 targets this pattern, but we should try to revive the strategy in [EXPERIMENT] Don't monomorphize things that are unused due to if <T as Trait>::CONST #91222 as well: Avoid lowering code under dead SwitchInt targets #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

• Add debug asserts to some unsafe operations #51713
• Add debug assertions to write_bytes and copy* #62103
• debug_assert a few more raw pointer methods #69208
• debug-assert ptr sanity in ptr::write #69509
• Add debug assertions to some unsafe functions #92686
• nicer errors from assert_unsafe_precondition #102732
• Even nicer errors from assert_unsafe_precondition #103035
• Add a forgotten check for NonNull::new_unchecked's precondition #103329
• Add debug_assert_nounwind and convert assert_unsafe_precondition #110303
• Add #[inline] to core debug assertion helpers #113687
• Toggle assert_unsafe_precondition in codegen instead of expansion #120594
• Use intrinsics::debug_assertions in debug_assert_nounwind #120863
• Always inline check in assert_unsafe_precondition with cfg(debug_assertions) #121196
• Make is_nonoverlapping #[inline] #121311
• Add #[rustc_no_mir_inline] for standard library UB checks #121114
• refactor check_{lang,library}_ub: use a single intrinsic #122629
• Eliminate UbChecks for non-standard libraries #122975
• Put checks that detect UB under their own flag below debug_assertions #123411
• Avoid lowering code under dead SwitchInt targets #121421
• Only collect mono items from reachable blocks #123272
• Avoid more NonNull-raw-NonNull roundtrips in Vec #123835

[the8472]

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]

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.