change the type of the argument of drop_in_place lang item to &mut _

[WaffleLapkin]

View all comments

We used to special case core::ptr::drop_in_place when computing LLVM argument attributes with this hack:

rust/compiler/rustc_ty_utils/src/abi.rs

Lines 383 to 392 in db5e2dc

	let drop_target_pointee_info = drop_target_pointee.and_then(|pointee| {
	assert_eq!(pointee, layout.ty.builtin_deref(true).unwrap());
	assert_eq!(offset, Size::ZERO);
	// The argument to `drop_in_place` is semantically equivalent to a mutable reference.
	let mutref = Ty::new_mut_ref(tcx, tcx.lifetimes.re_erased, pointee);
	let layout = cx.layout_of(mutref).unwrap();
	layout.pointee_info_at(&cx, offset)
	});
	if let Some(pointee) = drop_target_pointee_info.or_else(|| layout.pointee_info_at(&cx, offset))

This is because even though drop_in_place takes a *mut T it is semantically a &mut T (remember how &mut Self is passed to Drop::drop). This is apparently relevant for perf.

This PR replaces this hack with a simpler solution -- it makes drop_in_place a thin wrapper around newly added core::ptr::drop_glue, which is the actual lang item and takes a &mut T:

rust/library/core/src/ptr/mod.rs

Lines 810 to 833 in d2563d5

	pub const unsafe fn drop_in_place<T: PointeeSized>(to_drop: *mut T)
	where
	T: [const] Destruct,
	{
	// Due to historic reasons, `drop_in_place` takes a pointer rather than a reference,
	// which results in worse codegen since we don't apply noalias/dereferenceable llvm
	// attributes to pointer arguments. To workaround this without breaking public
	// interface, `drop_in_place` calls the lang item, rather than being one directly.
	// SAFETY:
	// - compiler glue has the same safety requirements as this function
	// - the pointer must be valid as per the safety requirement of this function
	unsafe { drop_glue(&mut *to_drop) }
	}
	/// Helper function for `drop_in_place`.
	#[lang = "drop_glue"]
	const unsafe fn drop_glue<T: PointeeSized>(_: &mut T)
	where
	T: [const] Destruct,
	{
	// Code here does not matter - this is replaced by the
	// real drop glue by the compiler.
	}

---

The rest of the PR is blessing tests and cleaning up things which are not necessary after this change.

One thing that is a bit awkward is that now that drop_glue is the actual lang item, a lot of the comments referring to drop_in_place are outdated. Should I try fixing that?

I've also changed async_drop_in_place to take a &mut T, and it simplified the code handling it a bit. (since it's unstable we don't need to introduce a wrapper)

---

cc @RalfJung
Closes #154274

[RalfJung]

Oh wow I hadn't expected my wish to be fulfilled before I could even finish my PR that motivated me to express the wish in the first place. :-) ❤️

[RalfJung]

I like the interpreter changes and renames. Not sure what the status of the rest of the PR is, but if you submit just those as a separate PR I'll r+.

[RalfJung]

Looking over the changes in the async part again -- they seem fairly mechanical, and I assume there's some sort of MIR validation / typeck running which ensures that the types all match. So I'd be happy to sign off on these.

What I really don't know how to review are the gigantic mir-opt test suite diffs. How do you all usually deal with that? @rust-lang/wg-mir-opt @scottmcm @saethlin

View changes since this review

[RalfJung]

It'd be better to avoid formatting-only changes... and in particular this one here doesn't like it is properly formatted now? This looked a lot better before.

[saethlin]

What I really don't know how to review are the gigantic mir-opt test suite diffs. How do you all usually deal with that?

mir-opt diffs look good to me.

Most of the diff here is from tests that check that drop-related code is inlined (it is) and then there's a bunch of diff in the specifics of the SourceScope data for PreCodegen tests, and those tests aren't worried about debuginfo. If you can visually exclude those cases, the diff isn't too bad.

Also for what it's worth I don't have a lot of patience for these mir-opt diffs for this reason, we have support for FileCheck annotations in mir-opt tests. If something is really that critical, it should be tested for with FileCheck.

[RalfJung]

If something is really that critical, it should be tested for with FileCheck.

It should, yeah, but is it?^^

Thanks for taking a look. :)

@WaffleLapkin looks like we're just one rebase and a few nits away from landing this then. :D

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[WaffleLapkin]

@RalfJung rebased + addressed the nits :)

[RalfJung]

@bors r=RalfJung,scottmcm,saethlin rollup=never

[rust-bors]

📌 Commit f1a1b8e has been approved by RalfJung,scottmcm,saethlin

It is now in the queue for this repository.

[RalfJung]

Actually...
@bors r-
Please squash the history a bit to remove "fix" commits.

[rust-bors]

This pull request was unapproved.

View changes since this unapproval