Document that Unique::empty() and NonNull::dangling() aren't sentinel values #52508

joshtriplett · 2018-07-18T19:54:06Z

The documentation of Unique::empty() and NonNull::dangling() could
potentially suggest that they work as sentinel values indicating a
not-yet-initialized pointer. However, they both declare a non-null
pointer equal to the alignment of the type, which could potentially
reference a valid value of that type (specifically, the first such valid
value in memory). Explicitly document that the return value of these
functions does not work as a sentinel value.

rust-highfive · 2018-07-18T19:54:16Z

r? @Mark-Simulacrum

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

Mark-Simulacrum

r=me with the nit fixed

Mark-Simulacrum · 2018-07-18T19:55:27Z

src/libcore/ptr.rs

+ ///
+ /// Note that the pointer value may potentially represent a valid pointer to
+ /// a `T`, which means this must not be used as a "not yet initialized"
+ /// sentinel value. Types that lazily allocate must track initialized cells


nit: s/initialized cells/initialization

… values The documentation of Unique::empty() and NonNull::dangling() could potentially suggest that they work as sentinel values indicating a not-yet-initialized pointer. However, they both declare a non-null pointer equal to the alignment of the type, which could potentially reference a valid value of that type (specifically, the first such valid value in memory). Explicitly document that the return value of these functions does not work as a sentinel value.

kennytm · 2018-07-18T20:02:02Z

std::rc::Weak uses NonNull::dangling though (#50357, #51901)

joshtriplett · 2018-07-18T20:26:25Z

@kennytm is_dangling looks like a bug.

kennytm · 2018-07-18T20:36:37Z

Should we revert #50357 and #51901, or just // FIXME them at the moment?

joshtriplett · 2018-07-18T21:03:34Z

@kennytm If we don't have an immediate fix, we should revert them.

hanna-kruppe · 2018-07-18T21:08:30Z

I disagree with reverting, at least #51901 fixed a much more real bug.

joshtriplett · 2018-07-18T21:11:33Z

Ah, fair enough. But we do need a fix for them.

@kennytm Would the solution I proposed of using usize::MAX work for now?

kennytm · 2018-07-18T21:18:49Z

@joshtriplett If the compiler doesn't assume NonNull<RcBox<T>> must be aligned then it should work.

(A properly allocated RcBox<T> obtained from Rc::downgrade must be aligned to usize-boundary, so usize::MAX being an odd number will never point to valid RcBox<T>, making it usable as sentinel)

kennytm · 2018-07-21T16:05:31Z

@joshtriplett Ping, are you going to do this usize::MAX thing in this PR or in a different PR? If latter I think you could r+.

joshtriplett · 2018-07-21T19:11:37Z

@kennytm I'd be happy to review someone's PR for that.

@bors r=Mark-Simulacrum

bors · 2018-07-21T19:11:38Z

📌 Commit ce75632 has been approved by Mark-Simulacrum

kennytm · 2018-07-21T19:29:04Z

@bors rollup

… r=Mark-Simulacrum Document that Unique::empty() and NonNull::dangling() aren't sentinel values The documentation of Unique::empty() and NonNull::dangling() could potentially suggest that they work as sentinel values indicating a not-yet-initialized pointer. However, they both declare a non-null pointer equal to the alignment of the type, which could potentially reference a valid value of that type (specifically, the first such valid value in memory). Explicitly document that the return value of these functions does not work as a sentinel value.

RalfJung · 2018-07-21T23:22:03Z

Which advantage does usize::MAX have over using 1?

joshtriplett · 2018-07-21T23:35:44Z

@RalfJung Weak (and perhaps other things) contains a NonNull<RcBox<T>>, and RcBox has an alignment of at least the alignment of usize, which will typically be 4 or 8. Given that, there's a niche in a NonNull<RcBox<T>> that extends from 0..=3 or 0..=7. Having Weak use usize::MAX to represent the dangling value avoids using up that niche, and allows Weak to have the same niche.

RalfJung · 2018-07-22T08:03:50Z

@joshtriplett I assume you mean 0..4 and 0..8?
AFAIK we only consider NonNull to have a niche of 0..1. It seems unlikely to me that we can ever change that.
OTOH using usize::MAX is probably not more complicated either.

Cc @eddyb

hanna-kruppe · 2018-07-22T08:58:53Z

We definitely can't extend the niche of NonNull, but we can make Weak specifically (and some other types) opt into a larger niche.

RalfJung · 2018-07-22T10:31:16Z

I'd be happy to review someone's PR for that.

I can prepare a PR today or tmr.

… r=Mark-Simulacrum Document that Unique::empty() and NonNull::dangling() aren't sentinel values The documentation of Unique::empty() and NonNull::dangling() could potentially suggest that they work as sentinel values indicating a not-yet-initialized pointer. However, they both declare a non-null pointer equal to the alignment of the type, which could potentially reference a valid value of that type (specifically, the first such valid value in memory). Explicitly document that the return value of these functions does not work as a sentinel value.

Rollup of 11 pull requests Successful merges: - #51807 (Deprecation of str::slice_unchecked(_mut)) - #52051 (mem::swap the obvious way for types smaller than the SIMD optimization's block size) - #52465 (Add CI test harness for `thumb*` targets. [IRR-2018-embedded]) - #52507 (Reword when `_` couldn't be inferred) - #52508 (Document that Unique::empty() and NonNull::dangling() aren't sentinel values) - #52521 (Fix links in rustdoc book.) - #52581 (Avoid using `#[macro_export]` for documenting builtin macros) - #52582 (Typo) - #52587 (Add missing backtick in UniversalRegions doc comment) - #52594 (Run the error index tool against the sysroot libdir) - #52615 (Added new lines to .gitignore.)

RalfJung · 2018-07-23T09:34:04Z

Done: #52637

cramertj · 2018-07-23T21:36:31Z

We definitely can't extend the niche of NonNull, but we can make Weak specifically (and some other types) opt into a larger niche.

Ah-- I had been under the assumption that NonNull was guaranteed to be aligned. To clarify: this isn't the case?

cramertj · 2018-07-23T21:38:03Z

Yes, this isn't the case: NonNull::new is documented to return Some for non-zero values.

@joshtriplett

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

SimonSapin · 2018-08-01T12:48:21Z

Sorry I’m only seeing this now, but I’m surprised by this change and not sure I agree with it. Vec has ~always used Unique::empty() as a sentinel for empty vectors. What changed?

Mark-Simulacrum · 2018-08-01T12:57:39Z

@SimonSapin A discussion on Discord led us to the conclusion that 0x(alignment) is potentially a valid pointer, so using it as a sentinel value may lead to incorrect behavior when such a pointer is legitimately passed.

RalfJung · 2018-08-01T13:06:05Z

So should we open an issue for Vec to also use usize::MAX like weak references now do?

If this is reoccurring, should Unique/NonNull have support for this?

hanna-kruppe · 2018-08-01T13:09:43Z

Vec never does something like if self.ptr == NonNull::dangling(), does it? It has the capacity field for that.

eddyb · 2018-08-01T13:12:19Z

Yes, because of the separate capacity you can use any non-null aligned(?) pointer.
Pretty much like how &[T; 0] can point (almost) anywhere.

SimonSapin · 2018-08-01T15:56:28Z

Ah ok. Branching on equality to dangling() rather than only using it to "fill in the blanks" in a struct is the difference that I’d missed. Vec has its capacity field, and Box has size_of::<T>() == 0.

Still, does the pointer in rc::Weak not need to be aligned? (This would rather be a question for #52637 but it feels related enough.)

rust-highfive assigned Mark-Simulacrum Jul 18, 2018

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

Mark-Simulacrum approved these changes Jul 18, 2018

View reviewed changes

joshtriplett force-pushed the dangling-not-sentinel branch from 31e97d1 to ce75632 Compare July 18, 2018 20:01

kennytm added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jul 18, 2018

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 21, 2018

kennytm mentioned this pull request Jul 21, 2018

Rollup of 11 pull requests #52588

Closed

kennytm mentioned this pull request Jul 22, 2018

Rollup of 11 pull requests #52616

Merged

RalfJung mentioned this pull request Jul 22, 2018

Untagged unions (tracking issue for RFC 1444) #32836

Closed

5 tasks

bors merged commit ce75632 into rust-lang:master Jul 22, 2018

RalfJung mentioned this pull request Jul 23, 2018

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

Merged

This was referenced Aug 30, 2018

fix some uses of pointer intrinsics with invalid pointers #53804

Merged

Review and improve HashMap's handling of an empty table #53853

Closed

joshtriplett deleted the dangling-not-sentinel branch August 24, 2022 12:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document that Unique::empty() and NonNull::dangling() aren't sentinel values #52508

Document that Unique::empty() and NonNull::dangling() aren't sentinel values #52508

joshtriplett commented Jul 18, 2018

rust-highfive commented Jul 18, 2018

Mark-Simulacrum left a comment

Mark-Simulacrum Jul 18, 2018

joshtriplett Jul 18, 2018

kennytm commented Jul 18, 2018

joshtriplett commented Jul 18, 2018

kennytm commented Jul 18, 2018

joshtriplett commented Jul 18, 2018

hanna-kruppe commented Jul 18, 2018

joshtriplett commented Jul 18, 2018

kennytm commented Jul 18, 2018 •

edited

Loading

kennytm commented Jul 21, 2018

joshtriplett commented Jul 21, 2018

bors commented Jul 21, 2018

kennytm commented Jul 21, 2018

RalfJung commented Jul 21, 2018

joshtriplett commented Jul 21, 2018 •

edited

Loading

RalfJung commented Jul 22, 2018

hanna-kruppe commented Jul 22, 2018

RalfJung commented Jul 22, 2018

RalfJung commented Jul 23, 2018

cramertj commented Jul 23, 2018

cramertj commented Jul 23, 2018

SimonSapin commented Aug 1, 2018

Mark-Simulacrum commented Aug 1, 2018

RalfJung commented Aug 1, 2018

hanna-kruppe commented Aug 1, 2018

eddyb commented Aug 1, 2018

SimonSapin commented Aug 1, 2018

Document that Unique::empty() and NonNull::dangling() aren't sentinel values #52508

Document that Unique::empty() and NonNull::dangling() aren't sentinel values #52508

Conversation

joshtriplett commented Jul 18, 2018

rust-highfive commented Jul 18, 2018

Mark-Simulacrum left a comment

Choose a reason for hiding this comment

Mark-Simulacrum Jul 18, 2018

Choose a reason for hiding this comment

joshtriplett Jul 18, 2018

Choose a reason for hiding this comment

kennytm commented Jul 18, 2018

joshtriplett commented Jul 18, 2018

kennytm commented Jul 18, 2018

joshtriplett commented Jul 18, 2018

hanna-kruppe commented Jul 18, 2018

joshtriplett commented Jul 18, 2018

kennytm commented Jul 18, 2018 • edited Loading

kennytm commented Jul 21, 2018

joshtriplett commented Jul 21, 2018

bors commented Jul 21, 2018

kennytm commented Jul 21, 2018

RalfJung commented Jul 21, 2018

joshtriplett commented Jul 21, 2018 • edited Loading

RalfJung commented Jul 22, 2018

hanna-kruppe commented Jul 22, 2018

RalfJung commented Jul 22, 2018

RalfJung commented Jul 23, 2018

cramertj commented Jul 23, 2018

cramertj commented Jul 23, 2018

SimonSapin commented Aug 1, 2018

Mark-Simulacrum commented Aug 1, 2018

RalfJung commented Aug 1, 2018

hanna-kruppe commented Aug 1, 2018

eddyb commented Aug 1, 2018

SimonSapin commented Aug 1, 2018

kennytm commented Jul 18, 2018 •

edited

Loading

joshtriplett commented Jul 21, 2018 •

edited

Loading