debug-assert ptr sanity in ptr::write #69509

RalfJung · 2020-02-27T09:09:08Z

This is a re-submission of the parts that we removed from #69208 due to "interesting" test failures.

RalfJung · 2020-02-27T09:10:54Z

As @eddyb suggested I tried ./x.py test --stage 1 src/test/codegen --target i686-unknown-linux-gnu, and was indeed able to reproduce the failure:

/home/r/src/rust/rustc.3/src/test/codegen/repeat-trusted-len.rs:16:11: error: CHECK: expected string not found in input
// CHECK: call void @llvm.memset.p0i8.[[USIZE]](i8* {{(nonnull )?}}align 1{{.*}} %{{[0-9]+}}, i8 42, [[USIZE]] 100000, i1 false)
          ^
/home/r/src/rust/rustc.3/build/x86_64-unknown-linux-gnu/test/codegen/repeat-trusted-len/repeat-trusted-len.ll:17:33: note: scanning from here
define void @repeat_take_collect(%"alloc::vec::Vec<u8>"* noalias nocapture sret dereferenceable(12)) unnamed_addr #1 personality i32 (i32, i32, i64, %"unwind::libunwind::_Unwind_Exception"*, %"unwind::libunwind::_Unwind_Context"*)* @rust_eh_personality {
                                ^
/home/r/src/rust/rustc.3/build/x86_64-unknown-linux-gnu/test/codegen/repeat-trusted-len/repeat-trusted-len.ll:17:33: note: with "USIZE" equal to "i32"
define void @repeat_take_collect(%"alloc::vec::Vec<u8>"* noalias nocapture sret dereferenceable(12)) unnamed_addr #1 personality i32 (i32, i32, i64, %"unwind::libunwind::_Unwind_Exception"*, %"unwind::libunwind::_Unwind_Context"*)* @rust_eh_personality {
                                ^
/home/r/src/rust/rustc.3/build/x86_64-unknown-linux-gnu/test/codegen/repeat-trusted-len/repeat-trusted-len.ll:17:33: note: with "USIZE" equal to "i32"
define void @repeat_take_collect(%"alloc::vec::Vec<u8>"* noalias nocapture sret dereferenceable(12)) unnamed_addr #1 personality i32 (i32, i32, i64, %"unwind::libunwind::_Unwind_Exception"*, %"unwind::libunwind::_Unwind_Context"*)* @rust_eh_personality {
                                ^
/home/r/src/rust/rustc.3/build/x86_64-unknown-linux-gnu/test/codegen/repeat-trusted-len/repeat-trusted-len.ll:30:2: note: possible intended match here
 tail call void @llvm.memset.p0i8.i64(i8* nonnull align 1 %1, i8 42, i64 100000, i1 false) #4, !noalias !11
 ^

If I read this correctly, then the codegen test expected memset.p0i8.i32 but we actually get memset.p0i8.i64. That's odd, for sure, but shouldn't be bad -- right?

With the fix I just pushed, this locally passes both on 64bit and 32bit (Linux each time; the wasm32 target doesn't work as I am missing emcc at least).

eddyb · 2020-02-27T09:43:41Z

Maybe we should ignore that distinction in the test? Seems really weird though.
Any chance you're using u64 at any point instead of usize in the check you're adding?

cc @nagisa @nikic @hanna-kruppe

RalfJung · 2020-02-27T09:51:53Z

Maybe we should ignore that distinction in the test?

Yeah, I think that's what I am doing in the last commit I pushed here.

Any chance you're using u64 at any point instead of usize in the check you're adding?

I can't see a u64 anywhere:

https://github.com/rust-lang/rust/blob/693b4d3075e7d48d9402876b12bef376d7aa2f1f/src/libcore/intrinsics.rs#L1421-L1423

https://github.com/rust-lang/rust/blob/693b4d3075e7d48d9402876b12bef376d7aa2f1f/src/libcore/ptr/const_ptr.rs#L28-L32

eddyb · 2020-02-27T10:08:54Z

Yeah, I think that's what I am doing in the last commit I pushed here

You're hardcoding the (weird) i64, I'm saying we can just ignore those parts of the line with {{.*}}. Oh and you removed [[USIZE]], so you can probably remove the part of the test that only exists to bind USIZE to a pointer-sized integer.

RalfJung · 2020-02-27T10:37:10Z

Oh I see. Well let's see what others think before I do further edits here.

Oh and you removed [[USIZE]], so you can probably remove the part of the test that only exists to bind USIZE to a pointer-sized integer.

Ah, that's how this works. :D

src/libcore/ptr/mod.rs

eddyb · 2020-02-27T11:32:52Z

Apropos of nothing, I just realized I want a way in config.toml to only enable debug assertions in the compiler, but not libcore.
I was wondering how my local build has become slower relative to nightly, and these kinds of assertions seem responsible.

I don't even want debug assertions (IMO rustc itself should never use them, just in case they affect soundness, but that's orthogonal), it's just that debug logging is gated on it.

RalfJung · 2020-02-27T12:19:46Z

these kinds of assertions seem responsible.

Would be interesting to see this confirmed. There aren't that many, really, and I feel like LLVM ought to be able to optimize most of them away -- but maybe it is not as good at that as it could be.

IMO rustc itself should never use them, just in case they affect soundness, but that's orthogonal.

We use them in the Miri engine for some checks that have non-trivial cost, so we don't want to slow down release builds with those sanity checks.

eddyb · 2020-02-27T17:41:21Z

There aren't that many, really, and I feel like LLVM ought to be able to optimize most of them away

Not if e.g. functions operating on raw pointers aren't inlined into functions that operate on references, or if said inlining happens to lose some information.
Not to mention that the checks themselves may impede inlining - it's a whole rat's nest.

For the record the worst I've seen so far is a slowdown of 2.4x for one crate, between nightly and a local build (using the times in the cargo -Z timings report), but it's not uniform, suggesting it affects some parts of rustc worse (or that the culprit is actually debug! calls having their arguments evaluated, because that was sadly changed at some point - this makes me think we should consider forking log).

We use them in the Miri engine for some checks that have non-trivial cost, so we don't want to slow down release builds with those sanity checks.

Hopefully they're really hard to trigger by screwing up elsewhere, especially from other parts of the compiler.
Or maybe they're more relevant to runtime miri than CTFE, I guess.

However, if you do get the chance to measure any of them, and it happens to be not that impactful perf-wise, please change it to a regular assert!.

RalfJung · 2020-02-27T22:38:24Z

For the record the worst I've seen so far is a slowdown of 2.4x for one crate, between nightly and a local build

Ouch. Well if you think these raw-ptr related debug assertions have significant impact on this (would be good to get that explicitly tested), I'm okay removing them. I figured they would be a nice safety net but it might just be too expensive.

If you give me a benchmark, I might find some time to test this.

eddyb · 2020-02-27T22:50:53Z

Ouch. Well if you think these raw-ptr related debug assertions have significant impact on this (would be good to get that explicitly tested), I'm okay removing them. I figured they would be a nice safety net but it might just be too expensive.

I'd prefer to keep them but for me to be able to turn debug-assertions off in libstd, just so I get as close as possible to the expected nightly performance.

But due to the variation in slowdown between different crates being compiled I'm starting to suspect debug! logging which doesn't check RUSTC_LOG before computing the format args (but is completely gated by cfg(debug_assertions), so when they're disabled it doesn't compute the format args).

eddyb · 2020-02-27T22:58:00Z

If you give me a benchmark, I might find some time to test this.

Just found a non-project-specific crate in those builds: serde, with a ~1.9x slowdown.

Mark-Simulacrum · 2020-02-28T01:42:59Z

r? @eddyb on this one I think

RalfJung · 2020-03-12T19:14:13Z

@eddyb

You're hardcoding the (weird) i64, I'm saying we can just ignore those parts of the line with {{.*}}. Oh and you removed [[USIZE]], so you can probably remove the part of the test that only exists to bind USIZE to a pointer-sized integer.

I did these adjustments. I hope the test looks good now. :)

eddyb · 2020-03-12T19:33:25Z

src/test/ui/issues/issue-40883.rs

+    // give space for 2 copies of `Big` + 256 "misc" bytes.
+    if stack_usage > mem::size_of::<Big>() * 2 + 256 {


Could this constant be binary searched so it's less than double the previous value?

I thought powers of two were nice, but sure. ;)

208 is the smallest value that works for me.
But I am a bit worried it might be larger for other platforms / architectures.

We don't support a target with larger sizes/alignments than x64 I don't think.

Oh nevermind, they could less efficient in their stack usage even if the types aren't larger.

Yeah that's what I meant.

eddyb · 2020-03-12T19:36:34Z

Pinging @nagisa @nikic @hanna-kruppe again, any reason we'd end up with a @llvm.memset.p0i8.i64 call on 32-bit target? See #69509 (comment).

I thought the llvm.memset size parameter was always a pointer-sized integer.

EDIT: we can merge this PR without solving that but I'm curious why it might even happen.

hanna-kruppe · 2020-03-13T10:25:03Z

No plausible reason for that comes to mind. It's not just very weird in itself, it's also very spooky that the changes in this PR have such an effect.

hanna-kruppe · 2020-03-13T10:39:58Z

I don't have the bandwidth for it currently, but it would be great if someone could:

reproduce this with a plain rustc invocation (no test suite shenanigans)
try to minimize the test case (so that it still contains a memset with i64 size on a 32b target)
check where the memset with i64 size is introduced (rustc's initial IR or during optimizations)

Centril · 2020-03-19T09:19:40Z

@bors p=3

bors · 2020-03-19T22:42:02Z

⌛ Testing commit c95f08a with merge 7202bc73630dad1231bfee91ad11aaec8f537ef5...

bors · 2020-03-20T01:43:01Z

💔 Test failed - checks-azure

Dylan-DPC-zz · 2020-03-20T01:50:33Z

Looks spurious to me

@bors retry

bors · 2020-03-20T05:11:03Z

⌛ Testing commit c95f08a with merge fc65ff9d482d99e59c4274c00865bbe924fbfa04...

bors · 2020-03-20T08:12:11Z

💔 Test failed - checks-azure

RalfJung · 2020-03-20T08:38:55Z

extracting D:\a\1\s\build\cache\2020-01-31\rustfmt-nightly-i686-pc-windows-gnu.tar.gz
    Updating crates.io index
error: failed to get `cc` as a dependency of package `bootstrap v0.0.0 (D:\a\1\s\src\bootstrap)`

Caused by:
  failed to fetch `https://github.com/rust-lang/crates.io-index`

@bors retry

bors · 2020-03-20T12:59:15Z

⌛ Testing commit c95f08a with merge ba06223814d736f19b8a4853d708eebb6a67feb0...

Centril · 2020-03-20T13:06:20Z

@bors retry yielding

bors · 2020-03-20T15:56:37Z

⌛ Testing commit c95f08a with merge dd53a2af944fe834c7c8b15b3cebaa513fc47af6...

JohnTitor · 2020-03-20T15:58:27Z

@bors retry yield rollup

bors · 2020-03-20T19:02:38Z

⌛ Testing commit c95f08a with merge 1057dc9...

bors · 2020-03-20T22:21:04Z

☀️ Test successful - checks-azure
Approved by: eddyb
Pushing 1057dc9 to master...

rust-highfive assigned Mark-Simulacrum Feb 27, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 27, 2020

RalfJung commented Feb 27, 2020

View reviewed changes

src/libcore/ptr/mod.rs Show resolved Hide resolved

rust-highfive assigned eddyb and unassigned Mark-Simulacrum Feb 28, 2020

RalfJung force-pushed the debug-assert-write branch from 693b4d3 to 6cc3d7a Compare February 29, 2020 08:24

RalfJung force-pushed the debug-assert-write branch 2 times, most recently from 7f53150 to 9d43ebf Compare March 12, 2020 19:13

RalfJung force-pushed the debug-assert-write branch from 9d43ebf to b324136 Compare March 12, 2020 19:15

eddyb reviewed Mar 12, 2020

View reviewed changes

RalfJung added 3 commits March 14, 2020 14:05

debug-assert ptr sanity in ptr::write

058ff72

increase slack for stack size test

59350e4

fix a codegen test

139a8c3

RalfJung force-pushed the debug-assert-write branch from 5e09c13 to 5fbd0cb Compare March 14, 2020 13:06

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 18, 2020

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

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 Mar 20, 2020

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

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 Mar 20, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 20, 2020

bors merged commit 1057dc9 into rust-lang:master Mar 20, 2020

RalfJung deleted the debug-assert-write branch March 21, 2020 09:41

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

		// give space for 2 copies of `Big` + 256 "misc" bytes.
		if stack_usage > mem::size_of::<Big>() * 2 + 256 {

debug-assert ptr sanity in ptr::write #69509

debug-assert ptr sanity in ptr::write #69509

Conversation

RalfJung commented Feb 27, 2020 • edited Loading

RalfJung commented Feb 27, 2020 • edited Loading

eddyb commented Feb 27, 2020

RalfJung commented Feb 27, 2020

eddyb commented Feb 27, 2020

RalfJung commented Feb 27, 2020 • edited Loading

eddyb commented Feb 27, 2020

RalfJung commented Feb 27, 2020

eddyb commented Feb 27, 2020

RalfJung commented Feb 27, 2020

eddyb commented Feb 27, 2020 • edited Loading

eddyb commented Feb 27, 2020

Mark-Simulacrum commented Feb 28, 2020

RalfJung commented Mar 12, 2020

eddyb Mar 12, 2020 • edited Loading

Choose a reason for hiding this comment

RalfJung Mar 12, 2020

Choose a reason for hiding this comment

RalfJung Mar 12, 2020

Choose a reason for hiding this comment

eddyb Mar 12, 2020

Choose a reason for hiding this comment

eddyb Mar 12, 2020 • edited Loading

Choose a reason for hiding this comment

RalfJung Mar 12, 2020

Choose a reason for hiding this comment

eddyb commented Mar 12, 2020 • edited Loading

hanna-kruppe commented Mar 13, 2020

hanna-kruppe commented Mar 13, 2020

Centril commented Mar 19, 2020

bors commented Mar 19, 2020

bors commented Mar 20, 2020

Dylan-DPC-zz commented Mar 20, 2020

bors commented Mar 20, 2020

bors commented Mar 20, 2020

RalfJung commented Mar 20, 2020

bors commented Mar 20, 2020

Centril commented Mar 20, 2020

bors commented Mar 20, 2020

JohnTitor commented Mar 20, 2020

bors commented Mar 20, 2020

bors commented Mar 20, 2020

RalfJung commented Feb 27, 2020 •

edited

Loading

RalfJung commented Feb 27, 2020 •

edited

Loading

RalfJung commented Feb 27, 2020 •

edited

Loading

eddyb commented Feb 27, 2020 •

edited

Loading

eddyb Mar 12, 2020 •

edited

Loading

eddyb Mar 12, 2020 •

edited

Loading

eddyb commented Mar 12, 2020 •

edited

Loading