Make drop glue for unsized value pass two arguments instead of *(data, meta) #38511

Mark-Simulacrum · 2016-12-21T14:01:50Z

eddyb · 2016-12-21T14:07:27Z

src/librustc_trans/adt.rs

@@ -87,6 +87,14 @@ impl MaybeSizedValue {
    pub fn has_meta(&self) -> bool {
        !self.meta.is_null()
    }
+
+    pub fn to_args(&self) -> Vec<ValueRef> {


Unnecessary allocation IMO. I'd rather have some helper that takes a MaybeSizedValue and drops it.

Actually, there's only two places so I'd rather just have allocation-less logic in both of them, this might work:

let args = &[x.value, x.meta][..1 + x.has_meta() as usize];

You could also make a helper for "argument count".

eddyb · 2016-12-21T14:16:48Z

src/librustc_trans/glue.rs

    let t = g.ty();

+    let args = if ccx.shared().type_is_sized(t) {
+        MaybeSizedValue::sized(get_param(llfn, 0))


I'd move the first argument out (data), and also call this v or value or ptr, not args.

eddyb · 2016-12-21T14:19:29Z

src/librustc_trans/glue.rs

+                    |bb, vv| drop_ty(bb, MaybeSizedValue::sized(vv), unit_ty));
+            } else {
+                cx = tvec::slice_for_each(&cx, value.value, unit_ty, value.meta,
+                    |bb, _vv| drop_ty(bb, value, unit_ty));


This can't be right. You should assert that the unit_ty is sized (or don't, really, it can never be right now).

eddyb · 2016-12-21T14:20:35Z

src/librustc_trans/mir/block.rs

-                    bcx.store(lvalue.llextra, base::get_meta(&bcx, scratch));
-                    scratch
+                    unsized_args = [lvalue.llval, lvalue.llextra];
+                    &unsized_args[..]


I'd rather have what I suggested before here too, one line turning an lvalue into an argument slice.

eddyb · 2016-12-21T14:22:16Z

src/librustc_trans/mir/block.rs

                    } else {
-                        bcx.call(drop_fn, &[llvalue], cleanup_bundle);
+                        bcx.call(drop_fn, args, cleanup_bundle);


Here's the cool part: since the ABI is identical for the drop glue and drop_in_place, you can just change callee.data to Fn(drop_fn) and mutate intrinsic to None. You can replace the pointer cast of an argument with a cast of drop_fn to the LLVM type of fn_ty (which means the if has to be moved lower down to where fn_ty is declared).

eddyb · 2016-12-21T14:26:16Z

src/librustc_trans/trans_item.rs

-        let sig = tcx.mk_fn_sig(iter::once(tcx.mk_mut_ptr(tcx.types.i8)), tcx.mk_nil(), false);
+        let sig = tcx.mk_fn_sig(iter::once(tcx.mk_mut_ptr(t)), tcx.mk_nil(), false);
+
+        debug!("predefine_drop_glue: sig={}", sig);

        // Create a FnType for fn(*mut i8) and substitute the real type in
        // later - that prevents FnType from splitting fat pointers up.


Outdated comment.

eddyb · 2016-12-21T17:02:57Z

src/librustc_trans/glue.rs

    let t = g.ty();

+    let value = get_param(llfn, 0);


Oh this meant you started using value below noo 😞, I'd rather you change it back.

eddyb · 2016-12-21T17:04:24Z

src/librustc_trans/glue.rs

-                        scratch
-                    };
-                    drop_ty(&cx, val, field_ty);
+                        drop_ty(&cx, MaybeSizedValue::unsized_(llfld_a, value.meta), field_ty);


You should really create a MaybeSizedValue in a variable (I'd copy the original and only change the data pointer) then have a separate single drop_ty call.

eddyb · 2016-12-21T17:05:45Z

src/librustc_trans/mir/block.rs

@@ -458,65 +467,12 @@ impl<'a, 'tcx> MirContext<'a, 'tcx> {
                    }

                    let ptr = self.trans_operand(&bcx, &args[0]);


This needs to go away.

eddyb · 2016-12-21T17:57:05Z

src/librustc_trans/glue.rs

+                bcx = normal_bcx;
+            } else {
+                llret = bcx.call(callee.reify(bcx.ccx),
+                    &[ptr.value, ptr.meta][..1 + ptr.has_meta() as usize], None);


Can you lift the arguments out of the if-let/else?

eddyb · 2016-12-21T18:43:07Z

@bors r+

bors · 2016-12-21T18:43:08Z

📌 Commit 5ee90f5 has been approved by eddyb

…, meta)

eddyb · 2016-12-21T22:40:55Z

@bors r+

bors · 2016-12-21T22:40:56Z

📌 Commit afc2dcd has been approved by eddyb

bors · 2016-12-22T00:27:49Z

⌛ Testing commit afc2dcd with merge 94bb66d...

bors · 2016-12-22T00:29:48Z

💔 Test failed - auto-mac-32-opt

bluss · 2016-12-22T01:04:40Z

@bors retry force clean

bors · 2016-12-22T01:04:47Z

⌛ Testing commit afc2dcd with merge c44e5d5...

bors · 2016-12-22T01:10:29Z

💔 Test failed - auto-mac-32-opt

eddyb · 2016-12-22T23:40:57Z

@bors retry

bors · 2016-12-23T11:56:44Z

⌛ Testing commit afc2dcd with merge ce4461f...

@eddyb

Make drop glue for unsized value pass two arguments instead of *(data, meta) Fixes #36457 r? @eddyb

bors · 2016-12-23T14:42:44Z

rust-highfive assigned eddyb Dec 21, 2016

eddyb suggested changes Dec 21, 2016

View reviewed changes

Mark-Simulacrum force-pushed the drop-glue branch 2 times, most recently from 942f015 to 303db8b Compare December 21, 2016 16:57

eddyb reviewed Dec 21, 2016

View reviewed changes

Mark-Simulacrum force-pushed the drop-glue branch from b547921 to 0c45a4d Compare December 21, 2016 17:49

eddyb reviewed Dec 21, 2016

View reviewed changes

Mark-Simulacrum force-pushed the drop-glue branch 2 times, most recently from 72c048f to 5ee90f5 Compare December 21, 2016 18:34

eddyb approved these changes Dec 21, 2016

View reviewed changes

Make drop glue for unsized value pass two arguments instead of *(data…

afc2dcd

…, meta)

Mark-Simulacrum force-pushed the drop-glue branch from 5ee90f5 to afc2dcd Compare December 21, 2016 19:02

bors added a commit that referenced this pull request Dec 23, 2016

Auto merge of #38511 - Mark-Simulacrum:drop-glue, r=eddyb

ce4461f

Make drop glue for unsized value pass two arguments instead of *(data, meta) Fixes #36457 r? @eddyb

bors merged commit afc2dcd into rust-lang:master Dec 23, 2016

Mark-Simulacrum deleted the drop-glue branch December 27, 2016 00:26

Mark-Simulacrum mentioned this pull request Jun 7, 2018

Missing tokens in custom derive input generated by macro #51331

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make drop glue for unsized value pass two arguments instead of *(data, meta) #38511

Make drop glue for unsized value pass two arguments instead of *(data, meta) #38511

Mark-Simulacrum commented Dec 21, 2016

eddyb Dec 21, 2016

eddyb Dec 21, 2016

eddyb Dec 21, 2016

eddyb Dec 21, 2016

eddyb Dec 21, 2016

eddyb Dec 21, 2016

eddyb Dec 21, 2016

eddyb Dec 21, 2016

eddyb Dec 21, 2016

eddyb Dec 21, 2016

eddyb Dec 21, 2016

eddyb commented Dec 21, 2016

bors commented Dec 21, 2016

eddyb commented Dec 21, 2016

bors commented Dec 21, 2016

bors commented Dec 22, 2016

bors commented Dec 22, 2016

bluss commented Dec 22, 2016

bors commented Dec 22, 2016

bors commented Dec 22, 2016

eddyb commented Dec 22, 2016

bors commented Dec 23, 2016

bors commented Dec 23, 2016

		@@ -458,65 +467,12 @@ impl<'a, 'tcx> MirContext<'a, 'tcx> {
		}

		let ptr = self.trans_operand(&bcx, &args[0]);

Make drop glue for unsized value pass two arguments instead of *(data, meta) #38511

Make drop glue for unsized value pass two arguments instead of *(data, meta) #38511

Conversation

Mark-Simulacrum commented Dec 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb commented Dec 21, 2016

bors commented Dec 21, 2016

eddyb commented Dec 21, 2016

bors commented Dec 21, 2016

bors commented Dec 22, 2016

bors commented Dec 22, 2016

bluss commented Dec 22, 2016

bors commented Dec 22, 2016

bors commented Dec 22, 2016

eddyb commented Dec 22, 2016

bors commented Dec 23, 2016

bors commented Dec 23, 2016