Add debug assertions to some unsafe functions #92686

saethlin · 2022-01-09T05:03:45Z

As suggested by #51713

~~Some similar code calls abort() instead of panic!() but aborting doesn't work in a const fn, and the intrinsic for doing dispatch based on whether execution is in a const is unstable.~~

This picked up some invalid uses of get_unchecked in the compiler, and fixes them.

I can confirm that they do in fact pick up invalid uses of get_unchecked in the wild, though the user experience is less-than-awesome:

     Running unittests (target/x86_64-unknown-linux-gnu/debug/deps/rle_decode_fast-04b7918da2001b50)

running 6 tests
error: test failed, to rerun pass '--lib'

Caused by:
  process didn't exit successfully: `/home/ben/rle-decode-helper/target/x86_64-unknown-linux-gnu/debug/deps/rle_decode_fast-04b7918da2001b50` (signal: 4, SIGILL: illegal instruction)

~~As best I can tell these changes produce a 6% regression in the runtime of ./x.py test when [rust] debug = true is set.~~
Latest commit (6894d55) brings the additional overhead from this PR down to 0.5%, while also adding a few more assertions. I think this actually covers all the places in core that it is reasonable to check for safety requirements at runtime.

Thoughts?

rust-highfive · 2022-01-09T05:03:49Z

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

the8472 · 2022-01-09T05:48:17Z

and the intrinsic for doing dispatch based on whether execution is in a const is unstable.

It already does get used in the standard library.

rust/library/core/src/intrinsics.rs

Lines 2077 to 2083 in 02fe61b

    
           unsafe { 
        
               const_eval_select((src, dst, count), compiletime_check, runtime_check); 
        
           } 
        
           // SAFETY: the safety contract for `copy_nonoverlapping` must be 
        
           // upheld by the caller. 
        
           unsafe { copy_nonoverlapping(src, dst, count) }

joshtriplett · 2022-01-09T08:22:22Z

@bors try @rust-timer queue

rust-timer · 2022-01-09T08:22:24Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-01-09T08:22:30Z

⌛ Trying commit 2ecff4e203cc4d32cc0c9f94f3099e01736c9c28 with merge 90cd1c65761dc97f1b34902129d8ffd8a10b5226...

joshtriplett · 2022-01-09T08:22:46Z

(The perf run is for the non-debug-assertion-related changes.)

bors · 2022-01-09T08:27:54Z

💔 Test failed - checks-actions

joshtriplett · 2022-01-09T08:44:47Z

Ah, oops. Please do kick off a perf run once CI passes.

saethlin · 2022-01-09T15:19:22Z

@rustbot ready

Aaron1011 · 2022-01-09T15:22:41Z

@bors try @rust-timer queue

rust-timer · 2022-01-09T15:22:42Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-01-09T15:22:51Z

⌛ Trying commit 55a9a6e7b3137824cd5b046a3029b8c24782ceda with merge 2e0638c4c955492789a8401d1e2ed15779d55dba...

bors · 2022-01-09T17:02:59Z

☀️ Try build successful - checks-actions
Build commit: 2e0638c4c955492789a8401d1e2ed15779d55dba (2e0638c4c955492789a8401d1e2ed15779d55dba)

rust-timer · 2022-01-09T17:03:01Z

Queued 2e0638c4c955492789a8401d1e2ed15779d55dba with parent e19ca1d, future comparison URL.

rust-timer · 2022-01-09T18:58:48Z

Finished benchmarking commit (2e0638c4c955492789a8401d1e2ed15779d55dba): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

Amanieu · 2022-04-03T15:54:22Z

I ran some performance tests on debug builds of a local codebase that uses -Zbuild-std and there was no measurable performance impact.

@bors r+

bors · 2022-04-03T15:54:23Z

📌 Commit 6e6d0cb has been approved by Amanieu

bors · 2022-04-03T16:04:55Z

⌛ Testing commit 6e6d0cb with merge 168a020...

bors · 2022-04-03T18:30:49Z

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing 168a020 to master...

rust-timer · 2022-04-03T19:43:58Z

Finished benchmarking commit (168a020): comparison url.

Summary:

Primary benchmarks: 🎉 relevant improvements found
Secondary benchmarks: no relevant changes found. 19 results were found to be statistically significant but too small to be relevant.

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count¹	1	0	17	19	18
mean²	0.6%	N/A	-0.5%	-0.4%	-0.4%
max	0.6%	N/A	-1.4%	-0.8%	0.0%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

number of relevant changes ↩
the arithmetic mean of the percent change ↩

…homcc Add a forgotten check for NonNull::new_unchecked's precondition Looks like I forgot this function a while ago in rust-lang#92686 r? `@thomcc`

…homcc Add a forgotten check for NonNull::new_unchecked's precondition Looks like I forgot this function a while ago in rust-lang#92686 r? ``@thomcc``

…homcc Add a forgotten check for NonNull::new_unchecked's precondition Looks like I forgot this function a while ago in rust-lang#92686 r? ```@thomcc```

940: Fix get_unchecked panic by raw pointer calculation r=taiki-e a=sticnarf It is related to #878 but doesn't fix it. Miri still complains with this PR. It fixes the panic on recent toolchains (after rust-lang/rust#92686) when using `-Zbuild-std`. ``` $ RUST_BACKTRACE=1 cargo test -p crossbeam-skiplist -Zbuild-std --target x86_64-unknown-linux-gnu --test map -- smoke Finished test [unoptimized + debuginfo] target(s) in 0.03s Running tests/map.rs (target/x86_64-unknown-linux-gnu/debug/deps/map-509dc855f6125f07) running 1 test thread 'smoke' panicked at 'unsafe precondition(s) violated: slice::get_unchecked requires that the index is within the slice', /home/yilin/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/panicking.rs:87:58 stack backtrace: 0: rust_begin_unwind at /home/yilin/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:575:5 1: core::panicking::panic_str_nounwind at /home/yilin/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/panicking.rs:90:14 2: <usize as core::slice::index::SliceIndex<[T]>>::get_unchecked::runtime at /home/yilin/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/intrinsics.rs:2284:21 3: <usize as core::slice::index::SliceIndex<[T]>>::get_unchecked at /home/yilin/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/index.rs:236:13 4: core::slice::<impl [T]>::get_unchecked at /home/yilin/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/mod.rs:399:20 5: <crossbeam_skiplist::base::Tower<K,V> as core::ops::index::Index<usize>>::index at ./src/base.rs:39:18 6: crossbeam_skiplist::base::SkipList<K,V>::search_position at ./src/base.rs:781:24 7: crossbeam_skiplist::base::SkipList<K,V>::insert_internal at ./src/base.rs:871:26 8: crossbeam_skiplist::base::SkipList<K,V>::insert at ./src/base.rs:1085:9 9: crossbeam_skiplist::map::SkipMap<K,V>::insert at ./src/map.rs:375:20 10: map::smoke at ./tests/map.rs:9:5 11: map::smoke::{{closure}} at ./tests/map.rs:7:12 12: core::ops::function::FnOnce::call_once at /home/yilin/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:507:5 13: core::ops::function::FnOnce::call_once at /home/yilin/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:507:5 note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace. thread panicked while panicking. aborting. error: test failed, to rerun pass `-p crossbeam-skiplist --test map` Caused by: process didn't exit successfully: `/home/yilin/Repos/crossbeam/target/x86_64-unknown-linux-gnu/debug/deps/map-509dc855f6125f07 smoke` (signal: 6, SIGABRT: process abort signal) ``` 941: Fix newly added clippy warnings r=taiki-e a=taiki-e Co-authored-by: Yilin Chen <[email protected]> Co-authored-by: Taiki Endo <[email protected]>

Add a forgotten check for NonNull::new_unchecked's precondition Looks like I forgot this function a while ago in rust-lang/rust#92686 r? ```@thomcc```

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 9, 2022

rust-highfive assigned joshtriplett Jan 9, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 9, 2022

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 9, 2022

This comment has been minimized.

Sign in to view

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 9, 2022

saethlin force-pushed the unsafe-debug-asserts branch from 2ecff4e to 131a0d1 Compare January 9, 2022 14:13

This comment has been minimized.

Sign in to view

saethlin force-pushed the unsafe-debug-asserts branch from 131a0d1 to 55a9a6e Compare January 9, 2022 14:23

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 9, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 3, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 3, 2022

bors merged commit 168a020 into rust-lang:master Apr 3, 2022

rustbot added this to the 1.61.0 milestone Apr 3, 2022

bors mentioned this pull request Apr 3, 2022

Stabilize const versions of ptr::slice_from_raw_parts and slice::from_raw_parts. #94946

Closed

yaahc mentioned this pull request Apr 7, 2022

Should we add an panic interface that reports the error via the panic handler but unconditionally aborts? rust-lang/project-error-handling#34

Open

saethlin deleted the unsafe-debug-asserts branch May 16, 2022 04:37

saethlin mentioned this pull request May 16, 2022

Fetch dependencies for -Zbuild-std before entering the sandbox rust-lang/rustwide#66

Merged

5225225 mentioned this pull request Jun 13, 2022

Add runtime validity checks inside MaybeUninit::assume_init #98073

Closed

4 tasks

thomcc mentioned this pull request Jul 3, 2022

Add assertion that transmute_copy's U is not larger than T #98839

Merged

5225225 mentioned this pull request Aug 17, 2022

Overhaul ThinVec usage #100666

Closed

saethlin mentioned this pull request Sep 17, 2022

[crater experiment] Turn on debug assertions #101929

Closed

saethlin mentioned this pull request Oct 21, 2022

Add a forgotten check for NonNull::new_unchecked's precondition #103329

Merged

pitaj mentioned this pull request Dec 1, 2022

Add debug asserts to some unsafe operations #51713

Closed

saethlin mentioned this pull request Dec 1, 2022

Add more debug assertions to unsafe functions #105117

Merged

sticnarf mentioned this pull request Dec 15, 2022

Fix get_unchecked panic by raw pointer calculation crossbeam-rs/crossbeam#940

Merged

saethlin mentioned this pull request Feb 9, 2024

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

Open

9 tasks

dtolnay removed the I-libs-nominated Nominated for discussion during a libs team meeting. label Sep 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add debug assertions to some unsafe functions #92686

Add debug assertions to some unsafe functions #92686

saethlin commented Jan 9, 2022 •

edited

Loading

rust-highfive commented Jan 9, 2022

This comment has been minimized.

the8472 commented Jan 9, 2022

joshtriplett commented Jan 9, 2022

rust-timer commented Jan 9, 2022

bors commented Jan 9, 2022

joshtriplett commented Jan 9, 2022

This comment has been minimized.

bors commented Jan 9, 2022

joshtriplett commented Jan 9, 2022

This comment has been minimized.

saethlin commented Jan 9, 2022

Aaron1011 commented Jan 9, 2022

rust-timer commented Jan 9, 2022

bors commented Jan 9, 2022

bors commented Jan 9, 2022

rust-timer commented Jan 9, 2022

rust-timer commented Jan 9, 2022

Amanieu commented Apr 3, 2022

bors commented Apr 3, 2022

bors commented Apr 3, 2022

bors commented Apr 3, 2022

rust-timer commented Apr 3, 2022

Add debug assertions to some unsafe functions #92686

Add debug assertions to some unsafe functions #92686

Conversation

saethlin commented Jan 9, 2022 • edited Loading

rust-highfive commented Jan 9, 2022

This comment has been minimized.

the8472 commented Jan 9, 2022

joshtriplett commented Jan 9, 2022

rust-timer commented Jan 9, 2022

bors commented Jan 9, 2022

joshtriplett commented Jan 9, 2022

This comment has been minimized.

bors commented Jan 9, 2022

joshtriplett commented Jan 9, 2022

This comment has been minimized.

saethlin commented Jan 9, 2022

Aaron1011 commented Jan 9, 2022

rust-timer commented Jan 9, 2022

bors commented Jan 9, 2022

bors commented Jan 9, 2022

rust-timer commented Jan 9, 2022

rust-timer commented Jan 9, 2022

Amanieu commented Apr 3, 2022

bors commented Apr 3, 2022

bors commented Apr 3, 2022

bors commented Apr 3, 2022

rust-timer commented Apr 3, 2022

Footnotes

saethlin commented Jan 9, 2022 •

edited

Loading