slices: fix ZST slice iterators making up pointers; debug_assert alignment in from_raw_parts #52206

RalfJung · 2018-07-10T07:08:12Z

This fixes the problem that we are fabricating pointers out of thin air. I also managed to share more code between the mutable and shared iterators, while reducing the amount of macros.

I am not sure how useful it really is to add a debug_assert! in libcore. Everybody gets a release version of that anyway, right? Is there at least a CI job that runs the test suite with a debug version?

Fixes #42789

rust-highfive · 2018-07-10T07:08:15Z

r? @shepmaster

(rust_highfive has picked a reviewer for you, use r? to override)

scottmcm · 2018-07-10T07:17:24Z

src/libcore/slice/mod.rs

@@ -581,7 +607,7 @@ impl<T> [T] {
    pub fn iter(&self) -> Iter<T> {
        unsafe {
            let p = if mem::size_of::<T>() == 0 {
-                1 as *const _
+                NonNull::dangling().as_ptr() as *const _


This makes me think that Iter::ptr should be a NonNull<T> instead of a *const T...

It could be. Same for end. (Though I have plans for #42789 that would need tweaking if end were to become a NonNull.)

Do you want me to change that in this PR?

I have no strong feelings about it. If it's easy and it's mostly lines that were changing anyway, might as well; if it's annoying or makes the PR a bunch longer, then probably not worth doing right now. (end not being NonNull is also totally fine by me.)

It's annoying because tons of code here does raw pointer operations, so there'd be a lot of to_ptr and new_unchecked and so on.

rust-highfive · 2018-07-10T07:21:41Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.


[00:03:59] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:03:59] tidy error: /checkout/src/libcore/tests/slice.rs:389: TODO is deprecated; use FIXME
[00:03:59] tidy error: /checkout/src/libcore/tests/slice.rs:416: TODO is deprecated; use FIXME
[00:04:01] some tidy checks failed
[00:04:01] 
[00:04:01] 
[00:04:01] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:01] 
[00:04:01] 
[00:04:01] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:01] Build completed unsuccessfully in 0:00:50
[00:04:01] Build completed unsuccessfully in 0:00:50
[00:04:01] make: *** [tidy] Error 1
[00:04:01] Makefile:79: recipe for target 'tidy' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:1216931d
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:055475b8:start=1531206865919441469,finish=1531206865925966084,duration=6524615
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:1d7bdf28
$ head -30 ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
head: cannot open ‘./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers’ for reading: No such file or directory
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:1898c4c7
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

RalfJung · 2018-07-10T07:46:26Z

I should add that -- given that ZST slices seem to have zero test coverage before my change -- I am somewhat worried I broke something somewhere...

scottmcm · 2018-07-10T21:30:28Z

src/libcore/slice/mod.rs

@@ -80,7 +80,7 @@ macro_rules! slice_offset {
    ($ptr:expr, $by:expr) => {{
        let ptr = $ptr;
        if size_from_ptr(ptr) == 0 {
-            (ptr as *mut i8).wrapping_offset($by) as _
+            (ptr as *mut i8).wrapping_offset($by.wrapping_mul(align_from_ptr(ptr) as isize)) as _


Is there enough address space to do this, given the capacities claimed by RawVec? I can apparently make a usize::MAX-length Vec of a align-8 ZST: https://play.rust-lang.org/?gist=c0b3d266bfbd34448d97e00f1408c946&version=stable&mode=release&edition=2015

Should the following change to !0 / mem::align_of::<T>()?

rust/src/liballoc/raw_vec.rs

Lines 215 to 221 in 2c1a715

pub fn cap(&self) -> usize {

if mem::size_of::<T>() == 0 {

!0

} else {

self.cap

}

}

Ah, good point.

Is it enough to change that in raw_vec or are there other ways to create huge slices?

Doesn't this problem disappear if we fix #42789 by using start as a real non-fabricated pointer and end as a counter? I assume that's the plan you mentioned, so if we're likely doing that anyway, maybe avoid some churn in user visible behavior by going straight to that solution.

Yes it does. In fact I am working on that next step already. I thought it would be easier for reviewers etc. to split this into two PRs, but if you prefer I can certainly finish that up and add it here (or make a new PR because there are going to be drastically more changes).

RalfJung · 2018-07-11T12:08:08Z

All right, I added "no longer make up pointers" to this PR. Instead of just putting the length into end, I put ptr+len into end so that ptr == end is uniformly the condition for the iterator being empty. That saves us a bunch of if mem::size_of::<T> == 0.

This is based on @bluss's work in #46223. In that PR, quite a few benchmarks were done to make sure this does not regress performance. How would I run these benchmarks?

scottmcm · 2018-07-11T16:19:20Z

src/libcore/slice/mod.rs

+                    // iterator, this works for both ZST and non-ZST.
+                    // For a ZST we would usually do `self.end = self.ptr`, but since
+                    // we will not give out an address any more after this there is no
+                    // way to observe the difference.


Couldn't I still get an address from .as_slice().as_ptr()?

Hm... okay it is observable but only as a raw pointer, not as a reference. So there's no soundness problem.

I can still change it, if you prefer.

Up to you; the comment tweak is probably fine since I'm not sure why anyone would be looking at the address of a post-iteration slice anyway.

Okay, then I'd prefer to keep it this way.

scottmcm · 2018-07-11T16:29:27Z

src/libcore/slice/mod.rs

+            #[inline(always)]
+            unsafe fn post_inc_start(&mut self, offset: isize) -> * $raw_mut T {
+                if mem::size_of::<T>() == 0 {
+                    self.end = (self.end as isize).wrapping_sub(offset) as * $raw_mut T;


wrapping_sub looks like a typo in a method named inc?

I thought so too at first, but no, for ZST slices the start pointer should not change as the iterator advances, and this wrapping_sub is decrementing the length. However, since it's apparently somewhat subtle, there should be a comment spelling this out.

Yeah in fact I used wrapping_add at first and wondered why everything exploded. ;)

I'll add a comment.

RalfJung · 2018-07-12T08:27:57Z

Should I be doing benchmarks before we land this? How would I go about that?

shepmaster · 2018-07-16T13:23:14Z

r? @alexcrichton

shepmaster · 2018-07-16T13:26:18Z

I think that checking the performance of slices would be a great idea, seeing as how fundamental they are. An accidental extra conditional or missed inline seems like it could have a big impact. I'm not sure if a regular perf run would be enough to feel good about it as I don't really know what that test suite looks like.

RalfJung · 2018-07-16T14:02:23Z

I've not done benchmarking like it was done for #46223 before (or any kind of benchmarking with Rust ever, really), so I'd need some pointers for how to do that.

EDIT: Hm, seems like they just ran some crate's bench suite? But how does that get the old/new numbers...?

Mark-Simulacrum · 2018-07-16T14:09:37Z

Generally speaking you can probably write up some #[bench] tests and compare against last nightly if you don't want to rebuild. We can also run perf.rlo, but that's more opaque since any difference here is likely quite minor.

RalfJung · 2018-07-16T14:14:33Z

I don't have any experience coming up with bench tests, is there something I should be on the watch for?

I noticed there is some stuff in libcore/benches/slice.rs, but not much. That produces output like

test slice::binary_search_l1                               ... bench:          55 ns/iter (+/- 2)
test slice::binary_search_l1_with_dups                     ... bench:          44 ns/iter (+/- 1)
test slice::binary_search_l2                               ... bench:          74 ns/iter (+/- 3)
test slice::binary_search_l2_with_dups                     ... bench:          74 ns/iter (+/- 3)
test slice::binary_search_l3                               ... bench:         162 ns/iter (+/- 11)
test slice::binary_search_l3_with_dups                     ... bench:         160 ns/iter (+/- 11)

How do I get these tables with a "+/- %" column?

shepmaster · 2018-07-16T14:26:16Z

How do I get these tables with a "+/- %" column?

Probably via cargo-benchcmp

alexcrichton · 2018-07-16T15:37:34Z

@bors: try

Can't hurt at least to have a try build!

bors · 2018-07-16T15:37:46Z

⌛ Trying commit f567aea with merge 3e9fa0ba8d51c2a5fe7cf53be75b7e4bf1370bd4...

alexcrichton · 2018-07-16T15:45:23Z

src/libcore/slice/mod.rs

+                    // For a ZST we would usually do `self.end = self.ptr`, but since
+                    // we will not give out an reference any more after this there is no
+                    // way to observe the difference except for raw pointers.
+                    self.ptr = self.end;


Previously in next there's an assume(!self.end.is_null()) but only if T has a nonzero size. That implies to me that if T is a ZST it's possible for self.end to be null (I think wraparound?). In this case, this is setting self.ptr to null, right? Wouldn't that break the in-memory representation of Option<&[T]> as well as the first assume above in the call to next?

Oh good point, I don't think we can rule that out.

I guess I'll hope for the optimizer to remove the conditional here as well, then. :)

alexcrichton · 2018-07-16T15:52:56Z

src/libcore/slice/mod.rs

+            assume(!ptr.is_null());
+
+            let end = if mem::size_of::<T>() == 0 {
+                (ptr as usize).wrapping_add(self.len()) as *mut _


Oh-so-long-ago the casting here between integers and pointers was found to inhibit optimizations when it comes to ZSTs, so would it be possible to use a similar strategy as that PR to avoid the pointer<->int conversions here?

I just tried that, and it makes the tests fail...?!?

EDIT: Ah, it still increments in multiples of size:of::<T>, which the docs don't really say...

bors · 2018-07-16T17:23:51Z

☀️ Test successful - status-travis
State: approved= try=True

alexcrichton · 2018-07-16T18:10:58Z

@rust-timer build 3e9fa0ba8d51c2a5fe7cf53be75b7e4bf1370bd4

rust-timer · 2018-07-16T18:11:00Z

Success: Queued 3e9fa0ba8d51c2a5fe7cf53be75b7e4bf1370bd4 with parent 3d5753f, comparison URL.

RalfJung · 2018-08-01T22:20:59Z

Heh, looks like there's actual code that which creates unaligned slices? This is in the rg test suite.

Should I back out the debug_assert! part of this PR? it's not really related anyway. I want to follow up on this but would prefer to land the rest first.

This also changes the IR for nth(), but the new IR actually looks nicer that the old (and it is one instruction shorter).

Also use ident, not expr, to avoid accidental side-effects

…regression

RalfJung · 2018-08-01T22:36:36Z

@bors r=alexcrichton

bors · 2018-08-01T22:36:37Z

📌 Commit 9fcf2c9 has been approved by alexcrichton

bors · 2018-08-02T00:14:30Z

⌛ Testing commit 9fcf2c9 with merge 1d9405f...

slices: fix ZST slice iterators making up pointers; debug_assert alignment in from_raw_parts This fixes the problem that we are fabricating pointers out of thin air. I also managed to share more code between the mutable and shared iterators, while reducing the amount of macros. I am not sure how useful it really is to add a `debug_assert!` in libcore. Everybody gets a release version of that anyway, right? Is there at least a CI job that runs the test suite with a debug version? Fixes #42789

bors · 2018-08-02T02:24:10Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 1d9405f to master...

gnzlbg · 2018-08-02T13:41:32Z

Is there at least a CI job that runs the test suite with a debug version?

So what's the answer to this? If there isn't there should be one.

kennytm · 2018-08-02T13:50:44Z

We could run some tests (up to 30 minutes) in the x86_64-gnu-debug job.

RalfJung · 2018-08-02T15:50:12Z

Give that I got a CI failure when I tried to land this with the debug_assert!, something is tested with debug-assertions enabled. ripgrep, at least. I'd assume then that this also runs the usual libcore, libstd, run-pass tests?

kennytm · 2018-08-03T00:24:41Z

Ah right. We do test with debug_assertions enabled, but they are disabled in dist.

rust/src/ci/run.sh

Lines 55 to 76 in 40e4b6e

    
           if [ "$DEPLOY$DEPLOY_ALT" != "" ]; then 
        
             RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --release-channel=$RUST_RELEASE_CHANNEL" 
        
             RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --enable-llvm-static-stdcpp" 
        
             if [ "$NO_LLVM_ASSERTIONS" = "1" ]; then 
        
               RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --disable-llvm-assertions" 
        
             elif [ "$DEPLOY_ALT" != "" ]; then 
        
               RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --enable-llvm-assertions" 
        
             fi 
        
           else 
        
             # We almost always want debug assertions enabled, but sometimes this takes too 
        
             # long for too little benefit, so we just turn them off. 
        
             if [ "$NO_DEBUG_ASSERTIONS" = "" ]; then 
        
               RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --enable-debug-assertions" 
        
             fi 
        
             # In general we always want to run tests with LLVM assertions enabled, but not 
        
             # all platforms currently support that, so we have an option to disable. 
        
             if [ "$NO_LLVM_ASSERTIONS" = "" ]; then 
        
               RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --enable-llvm-assertions" 
        
             fi 
        
           fi

. The x86_64-gnu-debug job enables debug symbols in additional to debug assertions.

nnethercote · 2018-08-17T01:17:42Z

This turned out to be a clear win on a few benchmarks, of up to 3.9%:
https://perf.rust-lang.org/compare.html?start=97085f9fb0736b322dc216db3655da780b4d8041&end=1d9405fb6caa5eac18e5a28685e4f30dcbde6d45&stat=instructions:u

Nice job!

RalfJung · 2018-08-17T07:05:39Z

Thanks! No idea how that happened, though. ;)

rust-highfive assigned shepmaster Jul 10, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 10, 2018

scottmcm reviewed Jul 10, 2018

View reviewed changes

RalfJung force-pushed the zst-slices branch from 38bd9d7 to 6b4ad90 Compare July 11, 2018 11:20

RalfJung changed the title ~~slices: fix alignment for ZST slices; debug_assert alignment in from_raw_parts~~ slices: fix ZST slice iterators making up pointers; debug_assert alignment in from_raw_parts Jul 11, 2018

RalfJung force-pushed the zst-slices branch from 6b4ad90 to bcf8375 Compare July 11, 2018 12:05

scottmcm reviewed Jul 11, 2018

View reviewed changes

RalfJung mentioned this pull request Jul 12, 2018

Iterators over slices of ZST behave strange #42789

Closed

rust-highfive assigned alexcrichton and unassigned shepmaster Jul 16, 2018

alexcrichton reviewed Jul 16, 2018

View reviewed changes

bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 1, 2018

RalfJung force-pushed the zst-slices branch from db8bdbc to 2f58f7c Compare August 1, 2018 22:25

RalfJung added 9 commits August 2, 2018 00:29

slice iterators: ZST iterators no longer just "make up" addresses

86369c3

comments

c7d90d1

use wrapping_offset; fix logic error in nth

cbdba2b

test nth better

60b0636

make the code for nth closer to what it used to be

1b3c6ba

macro-inline len() and is_empty() to fix performance regressions

3e3ff4b

This also changes the IR for nth(), but the new IR actually looks nicer that the old (and it is one instruction shorter).

simplify len macro: No longer require the type

b0a82d9

Also use ident, not expr, to avoid accidental side-effects

Introduce another way to compute the length, to fix position codegen …

e1471cf

…regression

use the same length computation everywhere

9fcf2c9

RalfJung force-pushed the zst-slices branch from 2f58f7c to 9fcf2c9 Compare August 1, 2018 22:35

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 Aug 1, 2018

bors merged commit 9fcf2c9 into rust-lang:master Aug 2, 2018

RalfJung deleted the zst-slices branch August 16, 2018 10:53

RalfJung mentioned this pull request May 13, 2019

Implement nth_back for slice::{Iter, IterMut} #60772

Merged

RalfJung mentioned this pull request Aug 15, 2019

clarify what is UB rust-lang/nomicon#149

Merged

	pub fn cap(&self) -> usize {
	if mem::size_of::<T>() == 0 {
	!0
	} else {
	self.cap
	}
	}

slices: fix ZST slice iterators making up pointers; debug_assert alignment in from_raw_parts #52206

slices: fix ZST slice iterators making up pointers; debug_assert alignment in from_raw_parts #52206

Conversation

RalfJung commented Jul 10, 2018 • edited Loading

rust-highfive commented Jul 10, 2018

Choose a reason for hiding this comment

RalfJung Jul 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

rust-highfive commented Jul 10, 2018

RalfJung commented Jul 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Jul 12, 2018

shepmaster commented Jul 16, 2018

shepmaster commented Jul 16, 2018

RalfJung commented Jul 16, 2018 • edited Loading

Mark-Simulacrum commented Jul 16, 2018

RalfJung commented Jul 16, 2018

shepmaster commented Jul 16, 2018

alexcrichton commented Jul 16, 2018

bors commented Jul 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Jul 18, 2018 • edited Loading

Choose a reason for hiding this comment

bors commented Jul 16, 2018

alexcrichton commented Jul 16, 2018

rust-timer commented Jul 16, 2018

RalfJung commented Aug 1, 2018

RalfJung commented Aug 1, 2018

bors commented Aug 1, 2018

bors commented Aug 2, 2018

bors commented Aug 2, 2018

gnzlbg commented Aug 2, 2018

kennytm commented Aug 2, 2018

RalfJung commented Aug 2, 2018 • edited Loading

kennytm commented Aug 3, 2018

nnethercote commented Aug 17, 2018

RalfJung commented Aug 17, 2018

RalfJung commented Jul 10, 2018 •

edited

Loading

RalfJung Jul 10, 2018 •

edited

Loading

RalfJung Jul 11, 2018 •

edited

Loading

RalfJung commented Jul 11, 2018 •

edited

Loading

RalfJung commented Jul 16, 2018 •

edited

Loading

RalfJung Jul 18, 2018 •

edited

Loading

RalfJung commented Aug 2, 2018 •

edited

Loading