More clone shim cleanups #48063

Manishearth · 2018-02-08T00:12:15Z

Manishearth · 2018-02-08T00:24:02Z

Bear in mind, I've done these patches at the instruction of @eddyb and don't yet 100% understand what's going on with the substs here. Please direct complex questions to him :)

eddyb · 2018-02-08T00:26:56Z

I'm just betting/hoping that everything lines up correctly 😅.

nikomatsakis

I left some comments. But in general I am confused about the "overall direction" of these changes -- they seem to be eliding type information when generating the clone impl, but don't we need that type information to know how big thing are and so forth? What am I missing?

nikomatsakis · 2018-02-08T17:50:42Z

src/librustc_mir/shim.rs

+            let self_ty = substs.type_at(0);
+            let mut builder = CloneShimBuilder::new(tcx, def_id, self_ty);
+            builder.copy_shim();
+            builder.into_mir()


I..don't understand this. How can we have just one shim for all monomorphizations? Don't their sizes depend on the specific types in use?

And what does this DefId refer to anyhow?

Shims are not per-monomorphization, they're pretty generic (look at some of the other ones). CloneShim (and now the CloneStructuralShim) was incorrectly too monomorphic (but the array/tuple cases are harder to fix).

What's happening now is that there's only one MIR body forreturn *self; and one per closure definition, which you monomorphize just like you'd monomorphize a generic closure definition.

"One shim for all monomorphizations" is the reason behind this PR. As for how it works, ask @eddyb

nikomatsakis · 2018-02-08T17:52:57Z

src/librustc/ty/instance.rs

@@ -291,7 +300,16 @@ fn resolve_associated_item<'a, 'tcx>(
            if let Some(_) = tcx.lang_items().clone_trait() {
                let name = tcx.type_of(def_id);
                let def = if name == "clone" {


Wait, pre-existing but... how .. does this work? name = tcx.type_of(def_id); if name == "clone"?? Isn't name a Ty<'tcx>?

I guess that the travis failure answers that question (i.e., it doesn't work).

lol I broke this in a rebase by accident. It used to be item_name

nikomatsakis · 2018-02-08T17:54:43Z

src/librustc/ty/instance.rs

+    ///`<T as Clone>::clone` shim for arrays and tuples
+    CloneStructuralShim(DefId, Ty<'tcx>),
+    ///`<T as Clone>::clone` shim for closures
+    CloneNominalShim(DefId, Ty<'tcx>),


Why is this not CloneClosureShim?

And (in all cases) can you comment what the DefId refers to? (If you're feeling heroic, do the same for the other variants)

It used to be CloneClosureShim and eddyb had me change it to use a more generic name 😄

It's literally Clone::clone - all of the variants (have to) record the original DefId that was passed to Instance::resolve or something more specific (mostly for InstanceDef::Item.

nikomatsakis · 2018-02-08T17:55:42Z

src/librustc/ty/instance.rs

@@ -47,7 +47,7 @@ pub enum InstanceDef<'tcx> {
    ///`<T as Clone>::clone` shim for arrays and tuples
    CloneStructuralShim(DefId, Ty<'tcx>),
    ///`<T as Clone>::clone` shim for closures
-    CloneNominalShim(DefId, Ty<'tcx>),
+    CloneNominalShim { clone: DefId, ty: DefId },


comment about what ty is the def-id of...

The nominal type definition (closure, ADT, etc. - @Manishearth has a branch where enums also want this). Could use a better field name.

…Shim

… monomorphizations

Manishearth · 2018-02-09T00:58:49Z

Fails on run-pass/clone-closure.rs

@eddyb any idea what's going on?

[01:06:53] error: internal compiler error: librustc/ty/layout.rs:2184: TyLayout::field_type(TyLayout { ty: i16, details: LayoutDetails { variants: Single { index: 0 }, fields: Union(0), abi: Scalar(Scalar { value: Int(I16, true), valid_range: 0..=65535 }), align: Align { abi: 1, pref: 1 }, size: Size { raw: 2 } } }): not applicable

eddyb · 2018-02-09T02:38:05Z

@Manishearth You need a backtrace, I have no idea.

Manishearth · 2018-02-09T16:31:18Z

error: internal compiler error: librustc/ty/layout.rs:2184: TyLayout::field_type(TyLayout { ty: i16, details: LayoutDetails { variants: Single { index: 0 }, fields: Union(0), abi: Scalar(Scalar { value: Int(I16, true), valid_range: 0..=65535 }), align: Align { abi: 1, pref: 1 }, size: Size { raw: 2 } } }): not applicable

thread 'rustc' panicked at 'Box<Any>', librustc_errors/lib.rs:535:9
note: Run with `RUST_BACKTRACE=1` for a backtrace.

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.25.0-dev running on x86_64-apple-darwin

[08:30:53] मanishearth@ligo ~/mozilla/sand O_o
$ RUST_BACKTRACE=1 rustc +x1 test.rs
error: internal compiler error: librustc/ty/layout.rs:2184: TyLayout::field_type(TyLayout { ty: i16, details: LayoutDetails { variants: Single { index: 0 }, fields: Union(0), abi: Scalar(Scalar { value: Int(I16, true), valid_range: 0..=65535 }), align: Align { abi: 1, pref: 1 }, size: Size { raw: 2 } } }): not applicable

thread 'rustc' panicked at 'Box<Any>', librustc_errors/lib.rs:535:9
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
   1: std::sys_common::backtrace::print
   2: std::panicking::default_hook::{{closure}}
   3: std::panicking::default_hook
   4: std::panicking::rust_panic_with_hook
   5: std::panicking::begin_panic
   6: rustc_errors::Handler::bug
   7: <std::thread::local::LocalKey<T>>::with
   8: rustc::ty::context::tls::with_opt
   9: rustc::session::opt_span_bug_fmt
  10: rustc::session::bug_fmt
  11: rustc::ty::layout::TyLayout::field
  12: rustc_trans::mir::place::PlaceRef::project_field
  13: rustc_trans::mir::place::<impl rustc_trans::mir::FunctionCx<'a, 'tcx>>::trans_place
  14: rustc_trans::mir::rvalue::<impl rustc_trans::mir::FunctionCx<'a, 'tcx>>::trans_rvalue_operand
  15: rustc_trans::mir::trans_mir
  16: rustc_trans::base::trans_instance
  17: rustc_trans::trans_item::MonoItemExt::define
  18: rustc_trans::base::compile_codegen_unit
  19: rustc::ty::maps::<impl rustc::ty::maps::queries::compile_codegen_unit<'tcx>>::compute_result
  20: rustc::dep_graph::graph::DepGraph::with_task_impl
  21: rustc_errors::Handler::track_diagnostics
  22: rustc::ty::maps::plumbing::<impl rustc::ty::context::TyCtxt<'a, 'gcx, 'tcx>>::cycle_check
  23: rustc::ty::maps::<impl rustc::ty::maps::queries::compile_codegen_unit<'tcx>>::force
  24: rustc::ty::maps::<impl rustc::ty::maps::queries::compile_codegen_unit<'tcx>>::try_get
  25: rustc::ty::maps::TyCtxtAt::compile_codegen_unit
  26: rustc::ty::maps::<impl rustc::ty::context::TyCtxt<'a, 'tcx, 'lcx>>::compile_codegen_unit
  27: rustc_trans::base::trans_crate
  28: <rustc_trans::LlvmTransCrate as rustc_trans_utils::trans_crate::TransCrate>::trans_crate
  29: rustc::util::common::time
  30: rustc_driver::driver::phase_4_translate_to_llvm
  31: rustc_driver::driver::compile_input::{{closure}}
  32: <std::thread::local::LocalKey<T>>::with

Manishearth · 2018-02-09T16:31:59Z

If I make the closure not capture variables, I get

Function return type does not match operand type of return inst!
  ret %"main::{{closure}}" undef
 i8LLVM ERROR: Broken function found, compilation aborted!

nikomatsakis · 2018-02-14T19:50:07Z

I've kind of lost the thread here. What is the status? @Manishearth do you need guidance?

Waiting to figure out what is going on =)

Manishearth · 2018-02-14T20:09:15Z

I haven't had the time to investigate the crash, but if you have a guess as to where it's coming from I'd love to know.

arazabishov · 2018-02-22T20:47:55Z

@Manishearth friendly ping from (release team) triage :) What is the status of this PR?

Manishearth · 2018-02-22T20:50:50Z

Still haven't had the time.

shepmaster · 2018-03-02T19:25:02Z

It's PR triage time again, @Manishearth! How's it going?

Manishearth · 2018-03-02T19:26:59Z

Nope. Feel free to close if you wish, but if someone else wants to take it over that works too

(@eddyb ?)

bors · 2018-03-08T11:38:08Z

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

pietroalbini · 2018-03-12T10:33:17Z

Closing for inactivity. @Manishearth, if you find the time to work on this again feel free to reopen this PR!

rust-highfive assigned nikomatsakis Feb 8, 2018

nikomatsakis previously requested changes Feb 8, 2018

View reviewed changes

Manishearth added 4 commits February 8, 2018 11:48

Use InstanceDef::Item for clone_from shims

8bdb10d

Split CloneShim into CloneCopyShim, CloneStructuralShim, CloneNominal…

d45a70c

…Shim

Remove ty from CloneCopyShim so that it creates a single shim for all…

9898cff

… monomorphizations

Pass down the self def id instead of the type

2c33d2f

Manishearth force-pushed the clone-shim-split branch from 2b830b2 to 2c33d2f Compare February 8, 2018 19:52

arazabishov added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 8, 2018

Manishearth mentioned this pull request Feb 9, 2018

[WIP] Make derive(Clone) memcpy on enum variants containing only Copy types #47867

Closed

pietroalbini closed this Mar 12, 2018

nox mentioned this pull request Mar 30, 2018

Salvage clone shim cleanup and carry it to completion #49507

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More clone shim cleanups #48063

More clone shim cleanups #48063

Manishearth commented Feb 8, 2018

Manishearth commented Feb 8, 2018

eddyb commented Feb 8, 2018

nikomatsakis left a comment

nikomatsakis Feb 8, 2018

nikomatsakis Feb 8, 2018

eddyb Feb 8, 2018

Manishearth Feb 8, 2018

nikomatsakis Feb 8, 2018

nikomatsakis Feb 8, 2018

Manishearth Feb 8, 2018

nikomatsakis Feb 8, 2018

nikomatsakis Feb 8, 2018

Manishearth Feb 8, 2018

eddyb Feb 8, 2018

nikomatsakis Feb 8, 2018

eddyb Feb 8, 2018

Manishearth commented Feb 9, 2018

eddyb commented Feb 9, 2018

Manishearth commented Feb 9, 2018

Manishearth commented Feb 9, 2018

nikomatsakis commented Feb 14, 2018

Manishearth commented Feb 14, 2018

arazabishov commented Feb 22, 2018

Manishearth commented Feb 22, 2018

shepmaster commented Mar 2, 2018

Manishearth commented Mar 2, 2018

bors commented Mar 8, 2018

pietroalbini commented Mar 12, 2018

More clone shim cleanups #48063

More clone shim cleanups #48063

Conversation

Manishearth commented Feb 8, 2018

Manishearth commented Feb 8, 2018

eddyb commented Feb 8, 2018

nikomatsakis left a 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

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

Manishearth commented Feb 9, 2018

eddyb commented Feb 9, 2018

Manishearth commented Feb 9, 2018

Manishearth commented Feb 9, 2018

nikomatsakis commented Feb 14, 2018

Manishearth commented Feb 14, 2018

arazabishov commented Feb 22, 2018

Manishearth commented Feb 22, 2018

shepmaster commented Mar 2, 2018

Manishearth commented Mar 2, 2018

bors commented Mar 8, 2018

pietroalbini commented Mar 12, 2018