Add note about PhantomData and dropck #432

KodrAus · 2020-09-07T00:51:16Z

Based on a soundness issue with SyncOnceCell introduced when we added a #[may_dangle] attribute without also informing dropck that we logically own the value: rust-lang/rust#76367

cc @matklad @m-ou-se

r? @RalfJung

RalfJung · 2020-09-07T07:38:32Z

src/libs/maintaining-std.md

@@ -189,6 +189,23 @@ Changes to collection internals may affect the order their items are dropped in.

 Generic types that manually implement `Drop` should consider whether a `#[may_dangle]` attribute is appropriate. The [Nomicon][dropck] has some details on what `#[may_dangle]` is all about.

+If the type pretends to store a value that may dangle, but doesn't do so directly (perhaps through `MaybeUninit<T>` or a raw pointer) then make sure there's an appropriate use of `PhantomData` to support dropck. As a [real-world example][rust/issues/76367], adding a `#[may_dangle]` attribute to an `OptionCell<T>` that internally stores its value as a `MaybeUninit<T>` requires both a `PhantomData` marker and a `#[may_dangle]` attribute:


When reading this paragraph, I wouldn't know what "pretends to store a value that may dangle" means -- specifically the "may dangle" part.

The key point here is that if dropping Type<T> means that a T could be dropped, then dropck needs to know this. If the drop occurs through the normal auto-generated drop glue, no action is required as dropck know what that one does. But if the drop occurs through a manual drop impl, then there needs to be a PhantomData<T>.

KodrAus · 2020-09-07T09:06:29Z

Thanks for the review @RalfJung! That first cut was clearly sketchy and deserved more consideration 🙂

How does this look now? I've linked to the appropriate RFC and I've tried to suggest that our original implementation of OptionCell<T> without a PhantomData<T> field was problematic even before we added #[may_dangle]. Would you agree?

m-ou-se

Ah nice, an example makes it a lot clearer.

Few comments:

I've tried to suggest that our original implementation of OptionCell<T> without a PhantomData<T> field was problematic even before we added #[may_dangle]. Would you agree?

Hm, not sure. Without #[may_dangle], dropck assumes we're going to access some potential T regardless, so that's safe. Although it might be good to add it just in case a #[may_dangle] is added later, I think it's better to just say that only adding #[may_dangle] requires thinking about dropck and related PhantomData.

m-ou-se · 2020-09-07T09:10:29Z

src/libs/maintaining-std.md

-Generic types that manually implement `Drop` should consider whether a `#[may_dangle]` attribute is appropriate. The [Nomicon][dropck] has some details on what `#[may_dangle]` is all about.
+A generic `Type<T>` that manually implements `Drop` should consider whether a `#[may_dangle]` attribute is appropriate on `T`. The [Nomicon][dropck] has some details on what `#[may_dangle]` is all about.
+
+If a generic `Type<T>` has a manual drop implementation that may also involve dropping `T` then dropck needs to know about it. If `Type<T>`'s ownership of `T` is expressed indirectly through pointers, such as `*mut T` or `MaybeUninit<T>`, then `Type<T>` also [needs a `PhantomData<T>` field][RFC 0769 PhantomData] to tell dropck that `T` may be dropped. Types in the standard library that use the internal `Unique<T>` pointer type don't need a `PhantomData<T>` marker field. That's taken care of for them by `Unique<T>`.


If Type<T>'s ownership of T is expressed indirectly through pointers, such as *mut T or MaybeUninit<T>

This now suggests that MaybeUninit<T> owns a T through pointers, instead of directly.

Might be good to mention ManuallyDrop<T> as well. That one's more closely related to the problem, as it does exactly the one thing that makes this problem a problem.

src/libs/maintaining-std.md

Co-authored-by: Mara Bos <m-ou.se@m-ou.se>

RalfJung · 2020-09-07T09:45:50Z

Ah nice, an example makes it a lot clearer.

Few comments:

I've tried to suggest that our original implementation of OptionCell<T> without a PhantomData<T> field was problematic even before we added #[may_dangle]. Would you agree?

Hm, not sure. Without #[may_dangle], dropck assumes we're going to access some potential T regardless, so that's safe. Although it might be good to add it just in case a #[may_dangle] is added later, I think it's better to just say that only adding #[may_dangle] requires thinking about dropck and related PhantomData.

Yes, I was about to raise a similar concern. dropck by default has to assume that a manual drop impl does everything safe code can do, which includes dropping some T. Only when #[may_dangle] is added does it assume that no operation on the may-dangle-parameter is called, except for dropping it but only if that is reflected via a struct field (either a T is really owned or it is PhantomData-owned).

RalfJung · 2020-09-07T09:48:05Z

src/libs/maintaining-std.md

+impl<T> Drop for OptionCell<T> {
+    fn drop(&mut self) {
+        // Safety: The cell is being dropped, so it can't be accessed again.
+        unsafe { self.take_inner() };


Might be good to at least state the type signature for take_inner. I assume it returns Option<T>.

Or just write out the code:

if self.is_init { let _ = unsafe { self.value.read() }; }

I'd find drop(unsafe { self.value.read() }) clearer, after all the point is that this drops the value.

RalfJung

This is the most clear explanation of the problem that I have seen so far. :)

RalfJung · 2020-09-07T09:51:12Z

We should reference these docs from rust-lang/rust#34761 once the PR lands.

KodrAus · 2020-09-07T10:41:01Z

Or just write out the code

Good idea!

Thanks @RalfJung and @m-ou-se for helping shape this up! ❤️

I’ll drop a link in the tracking issue when it’s all deployed.

src/libs/maintaining-std.md

Co-authored-by: Ralf Jung <post@ralfj.de>

KodrAus added 2 commits September 7, 2020 10:49

add note about PhantomData and dropck

4fd1f93

Update maintaining-std.md

7861c36

RalfJung reviewed Sep 7, 2020

View reviewed changes

rework may_dangle guidance

b3b2e92

m-ou-se reviewed Sep 7, 2020

View reviewed changes

KodrAus and others added 3 commits September 7, 2020 19:25

Update src/libs/maintaining-std.md

3ac1ab3

Co-authored-by: Mara Bos <m-ou.se@m-ou.se>

Update src/libs/maintaining-std.md

fc26bba

Co-authored-by: Mara Bos <m-ou.se@m-ou.se>

clarify may_dangle docs

bbdeb5f

RalfJung reviewed Sep 7, 2020

View reviewed changes

RalfJung approved these changes Sep 7, 2020

View reviewed changes

inline take_inner impl

68fdc6c

RalfJung reviewed Sep 7, 2020

View reviewed changes

src/libs/maintaining-std.md Outdated Show resolved Hide resolved

RalfJung reviewed Sep 7, 2020

View reviewed changes

src/libs/maintaining-std.md Show resolved Hide resolved

KodrAus and others added 2 commits September 7, 2020 21:15

Update src/libs/maintaining-std.md

f85d006

Co-authored-by: Ralf Jung <post@ralfj.de>

clarify safety comments in Drop impl

d81ddc8

KodrAus merged commit 537aea0 into master Sep 7, 2020

KodrAus deleted the KodrAus-patch-1 branch September 7, 2020 22:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add note about PhantomData and dropck #432

Add note about PhantomData and dropck #432

KodrAus commented Sep 7, 2020

RalfJung Sep 7, 2020

KodrAus commented Sep 7, 2020

m-ou-se left a comment

m-ou-se Sep 7, 2020

RalfJung commented Sep 7, 2020 •

edited

Loading

RalfJung Sep 7, 2020

m-ou-se Sep 7, 2020

RalfJung Sep 7, 2020

RalfJung left a comment

RalfJung commented Sep 7, 2020

KodrAus commented Sep 7, 2020

		@@ -189,6 +189,23 @@ Changes to collection internals may affect the order their items are dropped in.

		Generic types that manually implement `Drop` should consider whether a `#[may_dangle]` attribute is appropriate. The [Nomicon][dropck] has some details on what `#[may_dangle]` is all about.

		If the type pretends to store a value that may dangle, but doesn't do so directly (perhaps through `MaybeUninit<T>` or a raw pointer) then make sure there's an appropriate use of `PhantomData` to support dropck. As a [real-world example][rust/issues/76367], adding a `#[may_dangle]` attribute to an `OptionCell<T>` that internally stores its value as a `MaybeUninit<T>` requires both a `PhantomData` marker and a `#[may_dangle]` attribute:

Add note about PhantomData and dropck #432

Add note about PhantomData and dropck #432

Conversation

KodrAus commented Sep 7, 2020

RalfJung Sep 7, 2020

Choose a reason for hiding this comment

KodrAus commented Sep 7, 2020

m-ou-se left a comment

Choose a reason for hiding this comment

m-ou-se Sep 7, 2020

Choose a reason for hiding this comment

RalfJung commented Sep 7, 2020 • edited Loading

RalfJung Sep 7, 2020

Choose a reason for hiding this comment

m-ou-se Sep 7, 2020

Choose a reason for hiding this comment

RalfJung Sep 7, 2020

Choose a reason for hiding this comment

RalfJung left a comment

Choose a reason for hiding this comment

RalfJung commented Sep 7, 2020

KodrAus commented Sep 7, 2020

RalfJung commented Sep 7, 2020 •

edited

Loading