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

Implement by-value object safety #54183

Merged
merged 19 commits into from
Oct 27, 2018
Merged

Conversation

qnighy
Copy link
Contributor

@qnighy qnighy commented Sep 13, 2018

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 13, 2018
@qnighy qnighy force-pushed the by-value-object-safety branch 2 times, most recently from 7a494a3 to 74e4171 Compare September 22, 2018 04:04
@TimNN
Copy link
Contributor

TimNN commented Sep 25, 2018

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

assert!(arg_ty.is_self());
local_decls[rcvr_arg].ty = tcx.mk_mut_ptr(arg_ty);

Operand::Copy(rcvr_l.deref())
Copy link
Member

@eddyb eddyb Sep 26, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to Operand::Move.


if instance.is_vtable_shim() {
buf.push("{{vtable-shim}}");
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@@ -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);
Copy link
Member

@eddyb eddyb Sep 26, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -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>(
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@arielb1 arielb1 Oct 1, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@eddyb
Copy link
Member

eddyb commented Sep 28, 2018

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

@michaelwoerister
Copy link
Member

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

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);
Copy link
Contributor

@arielb1 arielb1 Oct 1, 2018

Choose a reason for hiding this comment

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

Could is_vtable_shim become some sort of enum?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@@ -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>(
Copy link
Contributor

@arielb1 arielb1 Oct 1, 2018

Choose a reason for hiding this comment

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

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
Copy link
Member

r? @arielb1

@bors
Copy link
Contributor

bors commented Oct 5, 2018

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

@bors
Copy link
Contributor

bors commented Oct 11, 2018

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

@bors
Copy link
Contributor

bors commented Oct 14, 2018

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

@TimNN
Copy link
Contributor

TimNN commented Oct 23, 2018

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, done.

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

Choose a reason for hiding this comment

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

remove the copyright header here and in all other new tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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



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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, added three tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

@oli-obk
Copy link
Contributor

oli-obk commented Oct 27, 2018

@bors r+

@bors
Copy link
Contributor

bors commented Oct 27, 2018

📌 Commit 2f7ea4a has been approved by oli-obk

@bors 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
Copy link
Contributor

bors commented Oct 27, 2018

⌛ Testing commit 2f7ea4a with merge cae6efc...

bors added a commit that referenced this pull request Oct 27, 2018
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
Copy link
Contributor

bors commented Oct 27, 2018

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

@bors bors merged commit 2f7ea4a into rust-lang:master Oct 27, 2018
@qnighy qnighy deleted the by-value-object-safety branch October 27, 2018 22:25
bors added a commit that referenced this pull request Oct 13, 2019
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
Copy link
Member

RalfJung commented Nov 7, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants