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.

Sign up for GitHub

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

Clarify drop_in_place safety #108684

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+7 −2
Open

Clarify drop_in_place safety #108684

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
23d4d77
Clarify drop_in_place safety
Manishearth Mar 3, 2023
f6747b0
Remove note on references
Manishearth Mar 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions library/core/src/ptr/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,10 @@ mod mut_ptr;
/// done automatically by the compiler. This means the fields of packed structs
/// are not dropped in-place.
///
/// [`drop_in_place()`] does not modify the pointed-to value beyond any changes
/// performed by [`Drop::drop()`]. As far as the compiler is concerned, the value
Copy link
Member

Choose a reason for hiding this comment

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

What is the point of this guarantee? drop can still mutate the value pretty much arbitrarily, right?
Or is it intended to say something about the auto-generated drop glue, in case I control the drop being called? That seems odd to state like this though, instead we should give a somewhat operational description of the auto-generated drop glue:

  • for a struct, it will first call the user-defined drop if it exists, and then call the drop glue of each field
  • for an enum, it will first call the user-defined drop if it exists, and then determine the active variant and call the drop glue for all fields of that variant
  • for an array, it will call the drop glue for each field
  • ...

Also IMO we shouldn't duplicate the same thing between ManuallyDrop::drop and here. drop_in_place is probably the more canonical location, but then ManuallyDrop::drop should just reference this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or is it intended to say something about the auto-generated drop glue, in case I control the drop being called?

Yes.

/// will still contain a valid bit pattern for type `T`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// will still contain a valid bit pattern for type `T`.
/// must still contain a valid bit pattern for type `T`.

I feel like "will" is a little odd here, this reads better to me -- but in general I find the sentence strange; typically we don't I think say things like "As far as the compiler ...", rather that something is UB or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

You'd say that except that party of the wording was lifted straight from the MaybeUninit docs :)

As Jakob says it doesn't make much sense though, so we should probably tighten up the wording on both

Copy link
Member

Choose a reason for hiding this comment

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

You mean the ManuallyDrop docs, I assume. But yeah that wording doesn't make a ton of sense. I don't know what that sentence is even trying to achieve, can we just remove it?

///
/// [`ptr::read`]: self::read
/// [`ptr::read_unaligned`]: self::read_unaligned
/// [pinned]: crate::pin
Expand All @@ -446,10 +450,11 @@ mod mut_ptr;
/// additional invariants - this is type-dependent.
///
/// Additionally, if `T` is not [`Copy`], using the pointed-to value after
/// calling `drop_in_place` can cause undefined behavior. Note that `*to_drop =
/// calling `drop_in_place` may cause undefined behavior. Note that `*to_drop =
/// foo` counts as a use because it will cause the value to be dropped
/// again. [`write()`] can be used to overwrite data without causing it to be
/// dropped.
/// dropped. Read operations may be UB based on library invariants of that type,
/// for example reading the value pointed to by a dropped `Box<T>` is a use-after-free.
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence seems useful, but can you clarify: Read operations on what? Ie, I have code like:

let p: *mut T = some_pointer_from_somewhere;
drop_in_place(p);

I initially interpreted read operations as being about *p, but that doesn't seem to agree with the rest of the sentence, which seems to be about **p in the case where T is Box<S>.

///
/// Note that even if `T` has size `0`, the pointer must be non-null and properly aligned.
///
Expand Down