Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
4 of 9 tasks
saethlin opened this issue Feb 9, 2024 · 2 comments
Open
4 of 9 tasks
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. E-help-wanted Call for participation: Help is requested to fix this issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@saethlin
Copy link
Member

saethlin commented Feb 9, 2024

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

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Feb 9, 2024
@fmease fmease added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. E-help-wanted Call for participation: Help is requested to fix this issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Feb 9, 2024
@the8472
Copy link
Member

the8472 commented Feb 10, 2024

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
Copy link
Member Author

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.

bors added a commit to rust-lang-ci/rust that referenced this issue Feb 13, 2024
Use intrinsics::debug_assertions in debug_assert_nounwind

This is the first item in rust-lang#120848.

Based on the benchmarking in this PR, it looks like, for the programs in our benchmark suite, enabling all these additional checks does not introduce significant compile-time overhead, with the single exception of `Alignment::new_unchecked`. Therefore, I've added `#[cfg(debug_assertions)]` to that one call site, so that it remains compiled out in the distributed standard library.

The trailing commas in the previous calls to `debug_assert_nounwind!` were causing the macro to expand to `panic_nouwnind_fmt`, which requires more work to set up its arguments, and that overhead alone is measured between this perf run and the next: rust-lang#120863 (comment)
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 14, 2024
Add `#[rustc_no_mir_inline]` for standard library UB checks

should help with rust-lang#121110 and also with rust-lang#120848

I am not entirely sure whether this is the correct solution and I haven't validated it, I just quickly threw it together before going to sleep.

r? `@saethlin`
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 14, 2024
Use intrinsics::debug_assertions in debug_assert_nounwind

This is the first item in rust-lang#120848.

Based on the benchmarking in this PR, it looks like, for the programs in our benchmark suite, enabling all these additional checks does not introduce significant compile-time overhead, with the single exception of `Alignment::new_unchecked`. Therefore, I've added `#[cfg(debug_assertions)]` to that one call site, so that it remains compiled out in the distributed standard library.

The trailing commas in the previous calls to `debug_assert_nounwind!` were causing the macro to expand to `panic_nouwnind_fmt`, which requires more work to set up its arguments, and that overhead alone is measured between this perf run and the next: rust-lang#120863 (comment)
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 15, 2024
Use intrinsics::debug_assertions in debug_assert_nounwind

This is the first item in rust-lang#120848.

Based on the benchmarking in this PR, it looks like, for the programs in our benchmark suite, enabling all these additional checks does not introduce significant compile-time overhead, with the single exception of `Alignment::new_unchecked`. Therefore, I've added `#[cfg(debug_assertions)]` to that one call site, so that it remains compiled out in the distributed standard library.

The trailing commas in the previous calls to `debug_assert_nounwind!` were causing the macro to expand to `panic_nouwnind_fmt`, which requires more work to set up its arguments, and that overhead alone is measured between this perf run and the next: rust-lang#120863 (comment)
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 16, 2024
Use intrinsics::debug_assertions in debug_assert_nounwind

This is the first item in rust-lang#120848.

Based on the benchmarking in this PR, it looks like, for the programs in our benchmark suite, enabling all these additional checks does not introduce significant compile-time overhead, with the single exception of `Alignment::new_unchecked`. Therefore, I've added `#[cfg(debug_assertions)]` to that one call site, so that it remains compiled out in the distributed standard library.

The trailing commas in the previous calls to `debug_assert_nounwind!` were causing the macro to expand to `panic_nouwnind_fmt`, which requires more work to set up its arguments, and that overhead alone is measured between this perf run and the next: rust-lang#120863 (comment)
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 16, 2024
Use intrinsics::debug_assertions in debug_assert_nounwind

This is the first item in rust-lang#120848.

Based on the benchmarking in this PR, it looks like, for the programs in our benchmark suite, enabling all these additional checks does not introduce significant compile-time overhead, with the single exception of `Alignment::new_unchecked`. Therefore, I've added `#[cfg(debug_assertions)]` to that one call site, so that it remains compiled out in the distributed standard library.

The trailing commas in the previous calls to `debug_assert_nounwind!` were causing the macro to expand to `panic_nouwnind_fmt`, which requires more work to set up its arguments, and that overhead alone is measured between this perf run and the next: rust-lang#120863 (comment)
saethlin added a commit to saethlin/rust that referenced this issue Feb 20, 2024
…thlin

Make `is_nonoverlapping` `#[inline]`

It showed up with 3% execution time in a compiler profile.

backlink to rust-lang#120848

r? `@saethlin`
Noratrieb added a commit to Noratrieb/rust that referenced this issue Feb 20, 2024
…thlin

Make `is_nonoverlapping` `#[inline]`

It showed up with 3% execution time in a compiler profile.

backlink to rust-lang#120848

r? ``@saethlin``
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 20, 2024
Rollup merge of rust-lang#121311 - Nilstrieb:is-it-overlapping, r=saethlin

Make `is_nonoverlapping` `#[inline]`

It showed up with 3% execution time in a compiler profile.

backlink to rust-lang#120848

r? ``@saethlin``
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 20, 2024
Use intrinsics::debug_assertions in debug_assert_nounwind

This is the first item in rust-lang#120848.

Based on the benchmarking in this PR, it looks like, for the programs in our benchmark suite, enabling all these additional checks does not introduce significant compile-time overhead, with the single exception of `Alignment::new_unchecked`. Therefore, I've added `#[cfg(debug_assertions)]` to that one call site, so that it remains compiled out in the distributed standard library.

The trailing commas in the previous calls to `debug_assert_nounwind!` were causing the macro to expand to `panic_nouwnind_fmt`, which requires more work to set up its arguments, and that overhead alone is measured between this perf run and the next: rust-lang#120863 (comment)
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 22, 2024
Avoid lowering code under dead SwitchInt targets

r? `@ghost`

Reviving rust-lang#91222 per rust-lang#120848
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 22, 2024
Avoid lowering code under dead SwitchInt targets

r? `@ghost`

Reviving rust-lang#91222 per rust-lang#120848
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 22, 2024
Avoid lowering code under dead SwitchInt targets

r? `@ghost`

Reviving rust-lang#91222 per rust-lang#120848
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 22, 2024
Add `#[rustc_no_mir_inline]` for standard library UB checks

should help with rust-lang#121110 and also with rust-lang#120848

I am not entirely sure whether this is the correct solution and I haven't validated it, I just quickly threw it together before going to sleep.

r? `@saethlin`
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 24, 2024
Avoid lowering code under dead SwitchInt targets

r? `@ghost`

Reviving rust-lang#91222 per rust-lang#120848
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 25, 2024
Add `#[rustc_no_mir_inline]` for standard library UB checks

should help with rust-lang#121110 and also with rust-lang#120848

Because the MIR inliner cannot know whether the checks are enabled or not, so inlining is an unnecessary compile time pessimization when debug assertions are disabled. LLVM knows whether they are enabled or not, so it can optimize accordingly without wasting time.

r? `@saethlin`
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 25, 2024
Avoid lowering code under dead SwitchInt targets

The objective of this PR is to detect and eliminate code which is guarded by an `if false`, even if that `false` is a constant which is not known until monomorphization, or is `intrinsics::debug_assertions()`.

The effect of this is that we generate no LLVM IR the standard library's unsafe preconditions, when they are compiled in a build where they should be immediately optimized out. This mono-time optimization ensures that builds which disable debug assertions do not grow a linkage requirement against `core`, which compiler-builtins currently needs: rust-lang#121552

This revives the codegen side of rust-lang#91222 as planned in rust-lang#120848.
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 13, 2024
Avoid lowering code under dead SwitchInt targets

The objective of this PR is to detect and eliminate code which is guarded by an `if false`, even if that `false` is a constant which is not known until monomorphization, or is `intrinsics::debug_assertions()`.

The effect of this is that we generate no LLVM IR the standard library's unsafe preconditions, when they are compiled in a build where they should be immediately optimized out. This mono-time optimization ensures that builds which disable debug assertions do not grow a linkage requirement against `core`, which compiler-builtins currently needs: rust-lang#121552

This revives the codegen side of rust-lang#91222 as planned in rust-lang#120848.
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Apr 7, 2024
Use intrinsics::debug_assertions in debug_assert_nounwind

This is the first item in rust-lang/rust#120848.

Based on the benchmarking in this PR, it looks like, for the programs in our benchmark suite, enabling all these additional checks does not introduce significant compile-time overhead, with the single exception of `Alignment::new_unchecked`. Therefore, I've added `#[cfg(debug_assertions)]` to that one call site, so that it remains compiled out in the distributed standard library.

The trailing commas in the previous calls to `debug_assert_nounwind!` were causing the macro to expand to `panic_nouwnind_fmt`, which requires more work to set up its arguments, and that overhead alone is measured between this perf run and the next: rust-lang/rust#120863 (comment)
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 12, 2024
Avoid more NonNull-raw-NonNull roundtrips in Vec

r? the8472

The standard library in general has a lot of these round-trips from niched types to their raw innards and back. Such round-trips have overhead in debug builds since rust-lang#120594. I removed some such round-trips in that initial PR and I've been meaning to come back and hunt down more such examples (this is the last item on rust-lang#120848).
jhpratt added a commit to jhpratt/rust that referenced this issue Apr 13, 2024
Avoid more NonNull-raw-NonNull roundtrips in Vec

r? the8472

The standard library in general has a lot of these round-trips from niched types to their raw innards and back. Such round-trips have overhead in debug builds since rust-lang#120594. I removed some such round-trips in that initial PR and I've been meaning to come back and hunt down more such examples (this is the last item on rust-lang#120848).
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Apr 13, 2024
Rollup merge of rust-lang#123835 - saethlin:vec-from-nonnull, r=the8472

Avoid more NonNull-raw-NonNull roundtrips in Vec

r? the8472

The standard library in general has a lot of these round-trips from niched types to their raw innards and back. Such round-trips have overhead in debug builds since rust-lang#120594. I removed some such round-trips in that initial PR and I've been meaning to come back and hunt down more such examples (this is the last item on rust-lang#120848).
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
Use intrinsics::debug_assertions in debug_assert_nounwind

This is the first item in rust-lang/rust#120848.

Based on the benchmarking in this PR, it looks like, for the programs in our benchmark suite, enabling all these additional checks does not introduce significant compile-time overhead, with the single exception of `Alignment::new_unchecked`. Therefore, I've added `#[cfg(debug_assertions)]` to that one call site, so that it remains compiled out in the distributed standard library.

The trailing commas in the previous calls to `debug_assert_nounwind!` were causing the macro to expand to `panic_nouwnind_fmt`, which requires more work to set up its arguments, and that overhead alone is measured between this perf run and the next: rust-lang/rust#120863 (comment)
@workingjubilee workingjubilee added C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. and removed C-totally-not-a-tracking-issue labels Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. E-help-wanted Call for participation: Help is requested to fix this issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants