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

Don't use NonNull::dangling as sentinel value in Rc, Arc #52637

Merged
merged 2 commits into from
Jul 24, 2018

Conversation

RalfJung
Copy link
Member

Instead, rely on alignment and use usize::MAX as sentinel.

Cc #52508

r? @joshtriplett

Instead, rely on alignment and use usize::MAX as sentinel.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 23, 2018
// `Weak::new` sets this to a dangling pointer so that it doesn’t need
// to allocate space on the heap.
// `Weak::new` sets this to `usize::MAX` so that it doesn’t need
// to allocate space on the heap. That's not a value a real poiner
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo, poiner -> pointer

@@ -449,6 +450,8 @@ impl<T: ?Sized> Rc<T> {
#[stable(feature = "rc_weak", since = "1.4.0")]
pub fn downgrade(this: &Self) -> Weak<T> {
this.inc_weak();
// Make sure we do not create a dangling Weak
debug_assert!(!is_dangling(this.ptr));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this assert is not gonna show up in libstd for users. Libstd is compiled in release mode

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have tests using the debug profile in the CI, so keeping this is fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I didn't want to risk an actual perf hit (though it should get all optimized out, because LLVM knows the pointer to be aligned).

// to allocate space on the heap.
// `Weak::new` sets this to `usize::MAX` so that it doesn’t need
// to allocate space on the heap. That's not a value a real poiner
// will ever have because RcBox has alignment at least 4.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically at least 2 due to 16-bit targets

@@ -1185,15 +1189,14 @@ impl<T> Weak<T> {
#[stable(feature = "downgraded_weak", since = "1.10.0")]
pub fn new() -> Weak<T> {
Weak {
ptr: NonNull::dangling(),
ptr: NonNull::new(usize::MAX as *mut RcBox<T>).expect("MAX is not 0"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better than unsafe code, I figured. :P

@RalfJung
Copy link
Member Author

Fixed typos.

@joshtriplett
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jul 23, 2018

📌 Commit a303741 has been approved by joshtriplett

@bors 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 Jul 23, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Jul 24, 2018
Don't use NonNull::dangling as sentinel value in Rc, Arc

Instead, rely on alignment and use usize::MAX as sentinel.

Cc rust-lang#52508

r? @joshtriplett
bors added a commit that referenced this pull request Jul 24, 2018
Rollup of 10 pull requests

Successful merges:

 - #52538 (Remove obsolete flags in the i586_musl Dockerfile)
 - #52548 (Cursor: update docs to clarify Cursor only works with in-memory buffers)
 - #52605 (Do not suggest using `to_owned()` on `&str += &str`)
 - #52621 (Fix color detection for Windows msys terminals.)
 - #52622 (Use MultiSpan in E0707 and E709)
 - #52627 (Compile rustc before building tests for rustdoc)
 - #52637 (Don't use NonNull::dangling as sentinel value in Rc, Arc)
 - #52640 (Forget Waker when cloning LocalWaker)
 - #52641 (Simplify 2 functions in rustc_mir/dataflow)
 - #52642 (Replace a few expect+format combos with unwrap_or_else+panic)

Failed merges:

r? @ghost
@bors bors merged commit a303741 into rust-lang:master Jul 24, 2018
@SimonSapin
Copy link
Contributor

Is it ok to use a misaligned pointer?

@hanna-kruppe
Copy link
Contributor

Of course! NonNull only rules out null and thus only has a niche of 0..=0, so unaligned pointers are fine storage-wise (the choice of !0 here is only made with an eye towards enlarging that niche in the future, otherwise we could just use 1) and we never dereference it so that's fine too.

@SimonSapin
Copy link
Contributor

Then why did we change zero-size Box<T> and Vec<T> from using pointer address 1 to address align_of::<T>()?

@hanna-kruppe
Copy link
Contributor

Containers of ZSTs can be iterated over by reference (Vec directly, Box e.g. via slice::from_ref). These references must be aligned. Weak doesn't contain any value, so you can't possibly get a reference out of it.

@hanna-kruppe
Copy link
Contributor

Uh, iteration is probably an unnecessary complication. Just Deref'ing them will already give you a reference that has to be aligned.

@SimonSapin
Copy link
Contributor

I see, thanks for the explanation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants