Implement by-value object safety #54183

qnighy · 2018-09-13T13:40:34Z

This PR implements by-value object safety, which is part of unsized rvalues #48055. That means, with #![feature(unsized_locals)], you can call a method fn foo(self, ...) on trait objects. One aim of this is to enable Box<FnOnce> in the near future.

The difficulty here is this: when constructing a vtable for a trait Foo, we can't just put the function <T as Foo>::foo into the table. If T is no larger than usize, self is usually passed directly. However, as the caller of the vtable doesn't know the concrete Self type, we want a variant of <T as Foo>::foo where self is always passed by reference.

Therefore, when the compiler encounters such a method to be generated as a vtable entry, it produces a newly introduced instance called InstanceDef::VtableShim(def_id) (that wraps the original instance). the shim just derefs the receiver and calls the original method. We give different symbol names for the shims by appending ::{{vtable-shim}} to the symbol path (and also adding vtable-shimness as an ingredient to the symbol hash).

r? @eddyb

TimNN · 2018-09-25T17:56:33Z

Ping from triage @eddyb: This PR requires your review.

eddyb · 2018-09-26T07:43:42Z

src/librustc_mir/shim.rs

+            assert!(arg_ty.is_self());
+            local_decls[rcvr_arg].ty = tcx.mk_mut_ptr(arg_ty);
+
+            Operand::Copy(rcvr_l.deref())


This doesn't seem right, should probably be Operand::Move. cc @nikomatsakis

Changed to Operand::Move.

eddyb · 2018-09-26T07:44:41Z

src/librustc_codegen_utils/symbol_names.rs

+
+    if instance.is_vtable_shim() {
+        buf.push("{{vtable-shim}}");
+    }


Is this needed?

It's unnecessary for symbol distinction. It's useful when reading the generated LLVM IR or assembly.

eddyb · 2018-09-26T07:45:30Z

src/librustc_codegen_utils/symbol_names.rs

@@ -219,6 +219,9 @@ fn get_symbol_hash<'a, 'tcx>(
                .hash_stable(&mut hcx, &mut hasher);
            (&tcx.crate_disambiguator(instantiating_crate)).hash_stable(&mut hcx, &mut hasher);
        }
+
+        let is_vtable_shim = instance.is_vtable_shim();
+        is_vtable_shim.hash_stable(&mut hcx, &mut hasher);


Shouldn't we be hashing something from the Instance anyway? I thought we already had overlaps in (DefId, Substs) between possible Instances, did I imagine that?

cc @michaelwoerister

My understanding is that, aside from VtableShim that I've just introduced, there are no overlaps in (DefId, Substs). Intrinsics just differs from Item in its ABI. FnPtrShim, Virtual, ClosureOnceShim, and CloneShim all refers to trait methods, and these instances do not have manual implementations. DropGlue refers to core::ptr::drop_in_palce, but for that function DropGlue is specifically generated, so it won't collide with Item.

With that said, hashing InstanceDef's discriminator seems good to me.

src/librustc_codegen_llvm/mir/block.rs

eddyb · 2018-09-26T07:48:04Z

src/librustc_codegen_llvm/meth.rs

@@ -98,7 +98,7 @@ pub fn get_vtable(
        let methods = tcx.vtable_methods(trait_ref);
        let methods = methods.iter().cloned().map(|opt_mth| {
            opt_mth.map_or(nullptr, |(def_id, substs)| {
-                callee::resolve_and_get_fn(cx, def_id, substs)
+                callee::resolve_and_get_fn_for_vtable(cx, def_id, substs)


You probably need to do something similar in miri, but it might be harder.

eddyb · 2018-09-26T07:49:23Z

src/librustc_codegen_llvm/common.rs

@@ -456,3 +456,22 @@ pub fn ty_fn_sig<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>,
        _ => bug!("unexpected type {:?} to ty_fn_sig", ty)
    }
 }
+
+pub fn ty_fn_sig_vtable<'a, 'tcx>(


I would expect this to be on Instance itself. Same for ty_fn_sig above, I guess. Feel free to leave FIXME comments here instead.

Should I introduce InstanceExt in codegen_llvm, or just move them into librustc?

This feels like it would fit in librustc as a function of instance. It would also allow us to get rid of the ugly is_vtable_shim boolean passed around.

Moved ty_fn_sig(_vtable) under Instance as Instance::fn_sig. Now is_vtable_shim is seen in only a few places.

eddyb · 2018-09-28T10:53:10Z

r? @michaelwoerister or @arielb1 on the instance/symbol changes.

michaelwoerister · 2018-10-01T14:56:07Z

@eddyb The changes to the symbol names and hashes look good to me.

arielb1 · 2018-10-01T19:52:47Z

src/librustc_codegen_llvm/declare.rs

-    let sig = common::ty_fn_sig(cx, fn_type);
+    debug!("declare_rust_fn(name={:?}, fn_type={:?}, is_vtable_shim={:?})",
+           name, fn_type, is_vtable_shim);
+    let sig = common::ty_fn_sig_vtable(cx, fn_type, is_vtable_shim);


Could is_vtable_shim become some sort of enum?

As a result of the ty_fn_sig refactoring, we now have only a reasonable amount of occurrences of is_vtable_shim.

arielb1 · 2018-10-01T19:54:25Z

src/librustc_codegen_llvm/common.rs

@@ -456,3 +456,22 @@ pub fn ty_fn_sig<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>,
        _ => bug!("unexpected type {:?} to ty_fn_sig", ty)
    }
 }
+
+pub fn ty_fn_sig_vtable<'a, 'tcx>(


This feels like it would fit in librustc as a function of instance. It would also allow us to get rid of the ugly is_vtable_shim boolean passed around.

michaelwoerister · 2018-10-04T08:47:07Z

r? @arielb1

bors · 2018-10-05T22:26:50Z

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

bors · 2018-10-11T15:15:52Z

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

bors · 2018-10-14T20:44:16Z

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

TimNN · 2018-10-23T17:53:03Z

Ping from triage @arielb1 / @rust-lang/compiler: This PR requires your review.

oli-obk · 2018-10-23T18:11:02Z

src/test/compile-fail/unsized-locals/by-value-trait-object-safety.rs

@@ -0,0 +1,30 @@
+// Copyright 2018 The Rust Project Developers. See the COPYRIGHT


remove the copyright header and move the test to src/test/ui

Sure, done.

oli-obk · 2018-10-23T18:11:19Z

src/test/run-pass-valgrind/unsized-locals/by-value-trait-objects-rust-call.rs

@@ -0,0 +1,65 @@
+// Copyright 2018 The Rust Project Developers. See the COPYRIGHT


remove the copyright header here and in all other new tests

Sure, removed these headers for all unsized-locals tests including those from #51131.

oli-obk · 2018-10-23T18:14:23Z

src/test/run-pass/unsized-locals/by-value-trait-object-safety.rs

+
+
+fn main() {
+    let x = *(Box::new(A) as Box<dyn Foo>);


Are there preexisting tests for calling such a method directly on a Box<dyn Foo> without first dereferencing the box? If not, please add a ui test for that.

I've not tested that, and it turned out to compile! I added this as a run-pass test (just below here).

awesome!

The RFC isn't very clear on this. But I would see that as a natural consequence.

Can you add more tests (also failure tests ensuring that we can't use the box anymore after having called a method on it?)

Sure, added three tests.

oli-obk · 2018-10-27T11:33:10Z

@bors r+

bors · 2018-10-27T11:33:11Z

📌 Commit 2f7ea4a has been approved by oli-obk

bors · 2018-10-27T19:29:41Z

⌛ Testing commit 2f7ea4a with merge cae6efc...

@eddyb

Implement by-value object safety This PR implements **by-value object safety**, which is part of unsized rvalues #48055. That means, with `#![feature(unsized_locals)]`, you can call a method `fn foo(self, ...)` on trait objects. One aim of this is to enable `Box<FnOnce>` in the near future. The difficulty here is this: when constructing a vtable for a trait `Foo`, we can't just put the function `<T as Foo>::foo` into the table. If `T` is no larger than `usize`, `self` is usually passed directly. However, as the caller of the vtable doesn't know the concrete `Self` type, we want a variant of `<T as Foo>::foo` where `self` is always passed by reference. Therefore, when the compiler encounters such a method to be generated as a vtable entry, it produces a newly introduced instance called `InstanceDef::VtableShim(def_id)` (that wraps the original instance). the shim just derefs the receiver and calls the original method. We give different symbol names for the shims by appending `::{{vtable-shim}}` to the symbol path (and also adding vtable-shimness as an ingredient to the symbol hash). r? @eddyb

bors · 2018-10-27T22:16:55Z

☀️ Test successful - status-appveyor, status-travis
Approved by: oli-obk
Pushing cae6efc to master...

Add `Instance::resolve_for_fn_ptr` (RFC 2091 #2/N) Supercedes: #65082 Depends on: #65037 Tracking issue: #47809 [RFC text](https://github.com/rust-lang/rfcs/blob/master/text/2091-inline-semantic.md) steps taken: * [x] add a `ReifyShim` that is similar to `VirtualShim` in behavior (see #54183) * [x] add `ty::Instance::resolve_for_fn_ptr` (leave `ty::Instance::resolve_vtable` alone), migrate appropriate callers * [x] `resolve_for_fn_ptr` returns the shim if calling a `#[track_caller]` function

RalfJung · 2019-11-07T07:50:48Z

I just realized that Miri doesn't have a good self-contained test of unsized method receivers yet. So I wanted to copy one from this PR, but all I found here are compile-fail tests. Where could I find run-pass tests for this feature?

EDIT: ah, the autoderef test looks good. :)

rust-highfive assigned eddyb Sep 13, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 13, 2018

qnighy force-pushed the by-value-object-safety branch 2 times, most recently from 7a494a3 to 74e4171 Compare September 22, 2018 04:04

eddyb reviewed Sep 26, 2018

View reviewed changes

src/librustc_codegen_llvm/mir/block.rs Show resolved Hide resolved

eddyb reviewed Sep 26, 2018

View reviewed changes

qnighy force-pushed the by-value-object-safety branch from 74e4171 to 0d5e048 Compare September 26, 2018 15:15

rust-highfive assigned michaelwoerister and unassigned eddyb Sep 28, 2018

arielb1 reviewed Oct 1, 2018

View reviewed changes

arielb1 suggested changes Oct 1, 2018

View reviewed changes

rust-highfive assigned arielb1 and unassigned michaelwoerister Oct 4, 2018

qnighy force-pushed the by-value-object-safety branch from 0d5e048 to 326e3c0 Compare October 6, 2018 14:52

qnighy force-pushed the by-value-object-safety branch from 326e3c0 to cd844f5 Compare October 12, 2018 14:03

qnighy force-pushed the by-value-object-safety branch from cd844f5 to 8e27255 Compare October 15, 2018 15:22

oli-obk requested changes Oct 23, 2018

View reviewed changes

Add InstanceDef::VtableShim.

0ad4c6f

qnighy added 3 commits October 24, 2018 22:55

Move compile-fail/unsized-locals under ui as per rust-lang#44844.

b3dce87

Add autoderef tests for by-value trait objects.

4fd895a

Remove license headers from unsized-locals tests.

1c48647

qnighy force-pushed the by-value-object-safety branch from 8e27255 to 1c48647 Compare October 24, 2018 14:27

Add more tests on unsized locals autoderef and borrowck.

2f7ea4a

oli-obk approved these changes Oct 27, 2018

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 27, 2018

bors merged commit 2f7ea4a into rust-lang:master Oct 27, 2018

This was referenced Oct 27, 2018

Take 2: Implement object-safety and dynamic dispatch for arbitrary_self_types #54383

Merged

Make librustc_codegen_llvm aware of LLVM address spaces. #51576

Closed

qnighy deleted the by-value-object-safety branch October 27, 2018 22:25

qnighy mentioned this pull request Oct 27, 2018

Tracking issue for RFC #1909: Unsized Rvalues (unsized_locals, unsized_fn_params) #48055

Open

13 tasks

SimonSapin mentioned this pull request Oct 28, 2018

Tracking issue for FnBox() #28796

Closed

cramertj mentioned this pull request Nov 20, 2018

Fuchsia + lld page-faulting on FnOnce::call_once vtable shim #56089

Closed

eddyb mentioned this pull request Dec 2, 2018

Tracking issue for RFC 2091: Implicit caller location #47809

Closed

3 tasks

RalfJung mentioned this pull request Dec 2, 2018

Teach miri about unsized locals rust-lang/miri#449

Closed

bjorn3 mentioned this pull request Apr 7, 2019

"cargo miri run" fails: cannot alloc memory for unsized type rust-lang/miri#675

Closed

This was referenced Oct 3, 2019

#[track_caller] feature gate (RFC 2091 1/N) #65037

Merged

WIP Add a shim for functions annotated with #[track_caller] (RFC 2091 #2/N) #65082

Closed

Add Instance::resolve_for_fn_ptr (RFC 2091 #2/N) #65182

Merged

		@@ -0,0 +1,30 @@
		// Copyright 2018 The Rust Project Developers. See the COPYRIGHT

		@@ -0,0 +1,65 @@
		// Copyright 2018 The Rust Project Developers. See the COPYRIGHT

Implement by-value object safety #54183

Implement by-value object safety #54183

Conversation

qnighy commented Sep 13, 2018

TimNN commented Sep 25, 2018

eddyb Sep 26, 2018 • edited Loading

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 Sep 26, 2018 • edited Loading

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

arielb1 Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb commented Sep 28, 2018

michaelwoerister commented Oct 1, 2018

arielb1 Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

michaelwoerister commented Oct 4, 2018

bors commented Oct 5, 2018

bors commented Oct 11, 2018

bors commented Oct 14, 2018

TimNN commented Oct 23, 2018

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

oli-obk commented Oct 27, 2018

bors commented Oct 27, 2018

bors commented Oct 27, 2018

bors commented Oct 27, 2018

RalfJung commented Nov 7, 2019 • edited Loading

eddyb Sep 26, 2018 •

edited

Loading

eddyb Sep 26, 2018 •

edited

Loading

arielb1 Oct 1, 2018 •

edited

Loading

arielb1 Oct 1, 2018 •

edited

Loading

arielb1 Oct 1, 2018 •

edited

Loading

RalfJung commented Nov 7, 2019 •

edited

Loading