change the type of the argument of `drop_in_place` lang item to `&mut _` by WaffleLapkin · Pull Request #154327 · rust-lang/rust

WaffleLapkin · 2026-03-24T17:44:25Z

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

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 · 2026-03-24T17:47:02Z

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 · 2026-04-06T13:52:26Z

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+.

…s, r=RalfJung Slightly refactor mplace<->ptr conversions split off of rust-lang#154327 r? RalfJung

…ottmcm Don't try to remove `drop_in_place` calls in `RemoveUnneededDrops` As per my justification in #154327 (comment) r? scottmcm

…ottmcm Don't try to remove `drop_in_place` calls in `RemoveUnneededDrops` As per my justification in rust-lang/rust#154327 (comment) r? scottmcm

WaffleLapkin · 2026-04-14T10:18:33Z

(#155044 is merged, ci is passing, this PR is ready for review again :3)

scottmcm

I skimmed over this and it looks good, but TBH I don't really feel like I understand the CTFE and async implications that are the only non-obvious bits to feel comfortable signing-off.

View changes since this review

RalfJung · 2026-04-15T09:01:58Z

The rustc_const_eval and rustc_ty_utils changes LGTM.

But yeah it'd be good to get someone who has seen the async drop stuff before to look at that. Cc @oli-obk @azhogin

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

…ottmcm Don't try to remove `drop_in_place` calls in `RemoveUnneededDrops` As per my justification in rust-lang/rust#154327 (comment) r? scottmcm

rust-bors · 2026-04-17T07:55:24Z

☔ The latest upstream changes (presumably #155207) made this pull request unmergeable. Please resolve the merge conflicts.

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 · 2026-04-20T16:51:28Z

@@ -8,6 +8,8 @@
 pub fn start(_: isize, _: *const *const u8) -> isize {
    // No item produced for this, it's a no-op drop and so is removed.
    unsafe {
+        // FIXME: is not removed on opt-level=0 anymore...
+        //~ MONO_ITEM fn std::ptr::drop_in_place::<u32> @@ drop_glue_noop-cgu.0[External]


So the crucial part that is still tested is that we don't see any drop_glue::<u32>, right? Maybe add a comment explaining that (instead of the FIXME which seems expected with the given mir-opt-level).

RalfJung · 2026-04-20T16:52:06Z

+        // FIXME: drop_in_place isn't an intrinsic....
        // This is the interesting thing in this test case: Normally we would
        // not have drop-glue for the unsized [StructWithDtor]. This has to be
        // generated though when the drop_in_place() intrinsic is used.


Seems like you can just update the comment then?

RalfJung · 2026-04-20T16:54:24Z

-        );
+        println!("AsyncUnion::Dropper::poll: {}, {}", unsafe { self.signed }, unsafe {
+            self.unsigned
+        },);


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 · 2026-04-21T01:02:22Z

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 · 2026-04-21T13:48:19Z

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

RalfJung reviewed Mar 24, 2026

View reviewed changes

Comment thread library/core/src/ptr/mod.rs Outdated

This comment has been minimized.

Sign in to view

RalfJung reviewed Mar 25, 2026

View reviewed changes

Comment thread compiler/rustc_const_eval/src/interpret/place.rs

Comment thread library/core/src/ptr/mod.rs Outdated

WaffleLapkin force-pushed the drop_in_place_ref branch from 281786e to 4842194 Compare April 2, 2026 11:19

This comment has been minimized.

Sign in to view

RalfJung reviewed Apr 4, 2026

View reviewed changes

Comment thread compiler/rustc_const_eval/src/interpret/place.rs

WaffleLapkin force-pushed the drop_in_place_ref branch 2 times, most recently from 794fcf5 to ea0c913 Compare April 5, 2026 18:31

rustbot added A-run-make Area: port run-make Makefiles to rmake.rs A-test-infra-minicore Area: `minicore` test auxiliary and `//@ add-core-stubs` T-rust-analyzer Relevant to the rust-analyzer team, which will review and decide on the PR/issue. labels Apr 5, 2026

This comment has been minimized.

Sign in to view

WaffleLapkin force-pushed the drop_in_place_ref branch from ea0c913 to 3dda404 Compare April 5, 2026 18:47

This comment has been minimized.

Sign in to view

WaffleLapkin force-pushed the drop_in_place_ref branch from 3dda404 to 32510c2 Compare April 6, 2026 12:22

This comment has been minimized.

Sign in to view

RalfJung reviewed Apr 6, 2026

View reviewed changes

Comment thread compiler/rustc_const_eval/src/interpret/place.rs Outdated

WaffleLapkin force-pushed the drop_in_place_ref branch from 32510c2 to 487fcc4 Compare April 6, 2026 14:00

This comment has been minimized.

Sign in to view

WaffleLapkin mentioned this pull request Apr 6, 2026

Slightly refactor mplace<->ptr conversions #154894

Merged

WaffleLapkin force-pushed the drop_in_place_ref branch from 487fcc4 to 6e43096 Compare April 6, 2026 15:56

This comment has been minimized.

Sign in to view

JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Apr 6, 2026

Rollup merge of rust-lang#154894 - WaffleLapkin:mplace_ptr_conversion…

d1e186f

…s, r=RalfJung Slightly refactor mplace<->ptr conversions split off of rust-lang#154327 r? RalfJung

WaffleLapkin mentioned this pull request Apr 9, 2026

Don't try to remove drop_in_place calls in RemoveUnneededDrops #155044

Merged

N1ark mentioned this pull request Apr 9, 2026

Bug: panic with --precise-drops AeneasVerif/charon#1089

Closed

WaffleLapkin added 3 commits April 10, 2026 09:33

change type argument of drop_in_place lang item to &mut _

45a31f2

bless miri/codegen-llvm/ui tests

2be11ff

remove retag from drop shim

18c69ba

WaffleLapkin force-pushed the drop_in_place_ref branch from 9030bb9 to 3bb21d4 Compare April 10, 2026 07:34

This comment has been minimized.

Sign in to view

rust-bors Bot pushed a commit that referenced this pull request Apr 11, 2026

Auto merge of #155044 - WaffleLapkin:dont-remove-unneeded-drops, r=sc…

c7c14d4

…ottmcm Don't try to remove `drop_in_place` calls in `RemoveUnneededDrops` As per my justification in #154327 (comment) r? scottmcm

scottmcm reviewed Apr 12, 2026

View reviewed changes

Comment thread tests/mir-opt/inline/inline_empty_drop_glue.slice_in_place.Inline.diff Outdated

This comment has been minimized.

Sign in to view

scottmcm mentioned this pull request Apr 12, 2026

Have arrays' drop_glue just unsize and call the slice version #155184

Draft

scottmcm reviewed Apr 12, 2026

View reviewed changes

Comment thread compiler/rustc_codegen_cranelift/example/mini_core.rs Outdated

WaffleLapkin added 6 commits April 13, 2026 18:59

rename drop_in_place lang item to drop_glue

80592a3

finish up renaming drop_in_place lang item => drop_glue

528bb1b

change type of async_drop_in_place to accept &mut _

5392b63

bless mir-opt tests

ca36661

bless codegen-units tests

1350a74

make NonNull::drop_in_place call drop_glue directly

e3aaa43

WaffleLapkin force-pushed the drop_in_place_ref branch from b8934ff to e3aaa43 Compare April 13, 2026 17:00

scottmcm approved these changes Apr 15, 2026

View reviewed changes

RalfJung reviewed Apr 15, 2026

View reviewed changes

Comment thread library/core/src/ptr/mod.rs Outdated

improve the doc comment on drop_glue

222a3a5

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

RalfJung reviewed Apr 20, 2026

View reviewed changes

	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))

	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.
	}

Uh oh!

Conversation

WaffleLapkin commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RalfJung commented Mar 24, 2026

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Uh oh!

RalfJung commented Apr 6, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Uh oh!

This comment has been minimized.

Uh oh!

WaffleLapkin commented Apr 14, 2026

Uh oh!

scottmcm left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

rust-bors Bot commented Apr 17, 2026

Uh oh!

RalfJung left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

RalfJung Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

RalfJung Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

saethlin commented Apr 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RalfJung commented Apr 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

WaffleLapkin commented Mar 24, 2026 •

edited

Loading

scottmcm left a comment •

edited by rustbot

Loading

RalfJung commented Apr 15, 2026 •

edited

Loading

RalfJung left a comment •

edited by rustbot

Loading

saethlin commented Apr 21, 2026 •

edited

Loading