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

drop_in_place: weaken the claim of equivalence with drop(ptr.read()) #125739

Merged
merged 2 commits into from
May 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 6 additions & 1 deletion library/core/src/ptr/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -450,8 +450,13 @@ mod mut_ptr;

/// Executes the destructor (if any) of the pointed-to value.
///
/// This is semantically equivalent to calling [`ptr::read`] and discarding
/// This is almost the same as calling [`ptr::read`] and discarding
/// the result, but has the following advantages:
Comment on lines +453 to 454
Copy link
Member

Choose a reason for hiding this comment

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

Could this be something like

Suggested change
/// This is almost the same as calling [`ptr::read`] and discarding
/// the result, but has the following advantages:
/// For most cases, this is equivalent to calling [`ptr::read`] and discarding
/// the result, but it is different in the following ways:

or is there a reason we should not prefer that more precise statement?

Copy link
Member Author

Choose a reason for hiding this comment

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

See the discussion in #112015 -- it's not fully clear whether we really want it to be equivalent, e.g. when calling read on a *mut &T we definitely require the reference to be dereferenceable, but when calling drop_in_place maybe we don't.

Copy link
Member

Choose a reason for hiding this comment

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

Hm.

Can we include an internal comment here that line 453 is a bluff, then, and there are additional unresolved operational semantics questions?

Copy link
Member

Choose a reason for hiding this comment

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

somewhere around line 540, I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a FIXME and a Miri test.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! Digging deep into histories is inevitable if someone wants full context, but it's nice to be able to see a plain-text hint in the source.

// FIXME: say something more useful than "almost the same"?
// There are open questions here: `read` requires the value to be fully valid, e.g. if `T` is a
// `bool` it must be 0 or 1, if it is a reference then it must be dereferenceable. `drop_in_place`
// only requires that `*to_drop` be "valid for dropping" and we have not defined what that means. In
// Miri it currently (May 2024) requires nothing at all for types without drop glue.
///
/// * It is *required* to use `drop_in_place` to drop unsized types like
/// trait objects, because they can't be read out onto the stack and
Expand Down
12 changes: 12 additions & 0 deletions src/tools/miri/tests/pass/drop_in_place.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// Miri currently doesn't require types without drop glue to be
// valid when dropped. This test confirms that behavior.
// This is not a stable guarantee!

use std::ptr;

fn main() {
let mut not_a_bool = 13u8;
unsafe {
ptr::drop_in_place(&mut not_a_bool as *mut u8 as *mut bool)
};
}
Loading