Address review comments

tmandry · tmandry · commit 77a6d29f483c · 2019-05-03T19:25:35.000-07:00
diff --git a/src/librustc/mir/mod.rs b/src/librustc/mir/mod.rs
@@ -2995,7 +2995,7 @@ pub struct UnsafetyCheckResult {
 }
 
 newtype_index! {
-    pub struct GeneratorField {
+    pub struct GeneratorSavedLocal {
         derive [HashStable]
         DEBUG_FORMAT = "_{}",
     }
@@ -3005,18 +3005,18 @@ newtype_index! {
 #[derive(Clone, Debug, RustcEncodable, RustcDecodable, HashStable)]
 pub struct GeneratorLayout<'tcx> {
     /// The type of every local stored inside the generator.
-    pub field_tys: IndexVec<GeneratorField, Ty<'tcx>>,
+    pub field_tys: IndexVec<GeneratorSavedLocal, Ty<'tcx>>,
 
     /// Which of the above fields are in each variant. Note that one field may
     /// be stored in multiple variants.
-    pub variant_fields: IndexVec<VariantIdx, IndexVec<Field, GeneratorField>>,
+    pub variant_fields: IndexVec<VariantIdx, IndexVec<Field, GeneratorSavedLocal>>,
 
     /// Names and scopes of all the stored generator locals.
     /// NOTE(tmandry) This is *strictly* a temporary hack for codegen
     /// debuginfo generation, and will be removed at some point.
     /// Do **NOT** use it for anything else, local information should not be
     /// in the MIR, please rely on local crate HIR or other side-channels.
-    pub __local_debuginfo_codegen_only_do_not_use: IndexVec<GeneratorField, LocalDecl<'tcx>>,
+    pub __local_debuginfo_codegen_only_do_not_use: IndexVec<GeneratorSavedLocal, LocalDecl<'tcx>>,
 }
 
 #[derive(Clone, Debug, RustcEncodable, RustcDecodable, HashStable)]
@@ -3582,7 +3582,7 @@ impl<'tcx> TypeFoldable<'tcx> for Field {
     }
 }
 
-impl<'tcx> TypeFoldable<'tcx> for GeneratorField {
+impl<'tcx> TypeFoldable<'tcx> for GeneratorSavedLocal {
     fn super_fold_with<'gcx: 'tcx, F: TypeFolder<'gcx, 'tcx>>(&self, _: &mut F) -> Self {
         *self
     }
diff --git a/src/librustc/ty/layout.rs b/src/librustc/ty/layout.rs
@@ -605,6 +605,12 @@ impl<'a, 'tcx> LayoutCx<'tcx, TyCtxt<'a, 'tcx, 'tcx>> {
             }
 
             ty::Generator(def_id, ref substs, _) => {
+                // FIXME(tmandry): For fields that are repeated in multiple
+                // variants in the GeneratorLayout, we need code to ensure that
+                // the offset of these fields never change. Right now this is
+                // not an issue since every variant has every field, but once we
+                // optimize this we have to be more careful.
+
                 let discr_index = substs.prefix_tys(def_id, tcx).count();
                 let prefix_tys = substs.prefix_tys(def_id, tcx)
                     .chain(iter::once(substs.discr_ty(tcx)));
@@ -1691,7 +1697,7 @@ impl<'a, 'tcx, C> TyLayoutMethods<'tcx, C> for Ty<'tcx>
 
     fn field(this: TyLayout<'tcx>, cx: &C, i: usize) -> C::TyLayout {
         let tcx = cx.tcx();
-        let handle_discriminant = |discr: &Scalar| -> C::TyLayout {
+        let discr_layout = |discr: &Scalar| -> C::TyLayout {
             let layout = LayoutDetails::scalar(cx, discr.clone());
             MaybeResult::from_ok(TyLayout {
                 details: tcx.intern_layout(layout),
@@ -1781,7 +1787,7 @@ impl<'a, 'tcx, C> TyLayoutMethods<'tcx, C> for Ty<'tcx>
                     }
                     Variants::Multiple { ref discr, discr_index, .. } => {
                         if i == discr_index {
-                            return handle_discriminant(discr);
+                            return discr_layout(discr);
                         }
                         substs.prefix_tys(def_id, tcx).nth(i).unwrap()
                     }
@@ -1805,7 +1811,7 @@ impl<'a, 'tcx, C> TyLayoutMethods<'tcx, C> for Ty<'tcx>
                     // Discriminant field for enums (where applicable).
                     Variants::Multiple { ref discr, .. } => {
                         assert_eq!(i, 0);
-                        return handle_discriminant(discr);
+                        return discr_layout(discr);
                     }
                 }
             }
diff --git a/src/librustc/ty/sty.rs b/src/librustc/ty/sty.rs
@@ -16,6 +16,7 @@ use crate::util::captures::Captures;
 use crate::mir::interpret::{Scalar, Pointer};
 
 use smallvec::SmallVec;
+use std::borrow::Cow;
 use std::cmp::Ordering;
 use std::marker::PhantomData;
 use std::ops::Range;
@@ -513,16 +514,13 @@ impl<'a, 'gcx, 'tcx> GeneratorSubsts<'tcx> {
     /// Calls `f` with a reference to the name of the enumerator for the given
     /// variant `v`.
     #[inline]
-    pub fn map_variant_name<R>(&self, v: VariantIdx, f: impl FnOnce(&str) -> R) -> R {
-        let name = match v.as_usize() {
-            Self::UNRESUMED => Self::UNRESUMED_NAME,
-            Self::RETURNED => Self::RETURNED_NAME,
-            Self::POISONED => Self::POISONED_NAME,
-            _ => {
-                return f(&format!("variant#{}", v.as_usize()));
-            }
-        };
-        f(name)
+    pub fn variant_name(&self, v: VariantIdx) -> Cow<'static, str> {
+        match v.as_usize() {
+            Self::UNRESUMED => Cow::from(Self::UNRESUMED_NAME),
+            Self::RETURNED => Cow::from(Self::RETURNED_NAME),
+            Self::POISONED => Cow::from(Self::POISONED_NAME),
+            _ => Cow::from(format!("Suspend{}", v.as_usize() - 3))
+        }
     }
 
     /// The type of the state discriminant used in the generator type.
diff --git a/src/librustc_codegen_llvm/debuginfo/metadata.rs b/src/librustc_codegen_llvm/debuginfo/metadata.rs
@@ -1575,7 +1575,7 @@ impl<'tcx> VariantInfo<'tcx> {
         match self {
             VariantInfo::Adt(variant) => f(&variant.ident.as_str()),
             VariantInfo::Generator(substs, _, variant_index) =>
-                substs.map_variant_name(*variant_index, f),
+                f(&substs.variant_name(*variant_index)),
         }
     }
 
@@ -1720,16 +1720,16 @@ fn prepare_enum_metadata(
                 .collect(),
             ty::Generator(_, substs, _) => substs
                 .variant_range(enum_def_id, cx.tcx)
-                .map(|v| substs.map_variant_name(v, |name| {
-                    let name = SmallCStr::new(name);
+                .map(|variant_index| {
+                    let name = SmallCStr::new(&substs.variant_name(variant_index));
                     unsafe {
                         Some(llvm::LLVMRustDIBuilderCreateEnumerator(
                             DIB(cx),
                             name.as_ptr(),
                             // FIXME: what if enumeration has i128 discriminant?
-                            v.as_usize() as u64))
+                            variant_index.as_usize() as u64))
                     }
-                }))
+                })
                 .collect(),
             _ => bug!(),
         };
diff --git a/src/librustc_codegen_llvm/type_of.rs b/src/librustc_codegen_llvm/type_of.rs
@@ -63,10 +63,10 @@ fn uncached_llvm_type<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>,
                     write!(&mut name, "::{}", def.variants[index].ident).unwrap();
                 }
             }
-            if let (&ty::Generator(..), &layout::Variants::Single { index })
+            if let (&ty::Generator(_, substs, _), &layout::Variants::Single { index })
                  = (&layout.ty.sty, &layout.variants)
             {
-                write!(&mut name, "::variant#{:?}", index).unwrap();
+                write!(&mut name, "::{}", substs.variant_name(index)).unwrap();
             }
             Some(name)
         }
diff --git a/src/librustc_mir/borrow_check/nll/type_check/mod.rs b/src/librustc_mir/borrow_check/nll/type_check/mod.rs
@@ -684,25 +684,8 @@ impl<'a, 'b, 'gcx, 'tcx> TypeVerifier<'a, 'b, 'gcx, 'tcx> {
                         }
                     }
                 }
-                ty::Generator(def_id, substs, _) => {
-                    let variants = substs.state_tys(def_id, tcx).count();
-                    if index.as_usize() >= variants {
-                        PlaceTy::from_ty(
-                            span_mirbug_and_err!(
-                                self,
-                                place,
-                                "cast to variant #{:?} but generator only has {:?}",
-                                index,
-                                variants
-                            ),
-                        )
-                    } else {
-                        PlaceTy {
-                            ty: base_ty,
-                            variant_index: Some(index),
-                        }
-                    }
-                }
+                // We do not need to handle generators here, because this runs
+                // before the generator transform stage.
                 _ => {
                     let ty = if let Some(name) = maybe_name {
                         span_mirbug_and_err!(
diff --git a/src/librustc_mir/transform/deaggregator.rs b/src/librustc_mir/transform/deaggregator.rs
@@ -58,7 +58,7 @@ impl MirPass for Deaggregator {
                     }
                     AggregateKind::Generator(..) => {
                         // Right now we only support initializing generators to
-                        // variant#0.
+                        // variant 0 (Unresumed).
                         let variant_index = VariantIdx::new(0);
                         set_discriminant = Some(Statement {
                             kind: StatementKind::SetDiscriminant {
diff --git a/src/librustc_mir/transform/generator.rs b/src/librustc_mir/transform/generator.rs
@@ -561,12 +561,13 @@ fn compute_layout<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
         remap.insert(local, (var.ty, variant_index, idx));
         decls.push(var);
     }
-    let field_tys = decls.iter().map(|field| field.ty).collect::<IndexVec<GeneratorField, _>>();
+    let field_tys = decls.iter().map(|field| field.ty).collect::<IndexVec<_, _>>();
 
     // Put every var in each variant, for now.
-    let all_vars = (0..field_tys.len()).map(GeneratorField::from).collect();
+    let all_vars = (0..field_tys.len()).map(GeneratorSavedLocal::from).collect();
     let empty_variants = iter::repeat(IndexVec::new()).take(3);
     let state_variants = iter::repeat(all_vars).take(suspending_blocks.count());
+
     let layout = GeneratorLayout {
         field_tys,
         variant_fields: empty_variants.chain(state_variants).collect(),
diff --git a/src/test/debuginfo/generator-objects.rs b/src/test/debuginfo/generator-objects.rs
@@ -10,16 +10,16 @@
 
 // gdb-command:run
 // gdb-command:print b
-// gdb-check:$1 = generator_objects::main::generator {__0: 0x[...], <<variant>>: {__state: 0, 0: generator_objects::main::generator::Unresumed, 1: generator_objects::main::generator::Returned, 2: generator_objects::main::generator::Panicked, 3: generator_objects::main::generator::variant#3 {[...]}, 4: generator_objects::main::generator::variant#4 {[...]}}}
+// gdb-check:$1 = generator_objects::main::generator {__0: 0x[...], <<variant>>: {__state: 0, 0: generator_objects::main::generator::Unresumed, 1: generator_objects::main::generator::Returned, 2: generator_objects::main::generator::Panicked, 3: generator_objects::main::generator::Suspend0 {[...]}, 4: generator_objects::main::generator::Suspend1 {[...]}}}
 // gdb-command:continue
 // gdb-command:print b
-// gdb-check:$2 = generator_objects::main::generator {__0: 0x[...], <<variant>>: {__state: 3, 0: generator_objects::main::generator::Unresumed, 1: generator_objects::main::generator::Returned, 2: generator_objects::main::generator::Panicked, 3: generator_objects::main::generator::variant#3 {c: 6, d: 7}, 4: generator_objects::main::generator::variant#4 {[...]}}}
+// gdb-check:$2 = generator_objects::main::generator {__0: 0x[...], <<variant>>: {__state: 3, 0: generator_objects::main::generator::Unresumed, 1: generator_objects::main::generator::Returned, 2: generator_objects::main::generator::Panicked, 3: generator_objects::main::generator::Suspend0 {c: 6, d: 7}, 4: generator_objects::main::generator::Suspend1 {[...]}}}
 // gdb-command:continue
 // gdb-command:print b
-// gdb-check:$3 = generator_objects::main::generator {__0: 0x[...], <<variant>>: {__state: 4, 0: generator_objects::main::generator::Unresumed, 1: generator_objects::main::generator::Returned, 2: generator_objects::main::generator::Panicked, 3: generator_objects::main::generator::variant#3 {[...]}, 4: generator_objects::main::generator::variant#4 {c: 7, d: 8}}}
+// gdb-check:$3 = generator_objects::main::generator {__0: 0x[...], <<variant>>: {__state: 4, 0: generator_objects::main::generator::Unresumed, 1: generator_objects::main::generator::Returned, 2: generator_objects::main::generator::Panicked, 3: generator_objects::main::generator::Suspend0 {[...]}, 4: generator_objects::main::generator::Suspend1 {c: 7, d: 8}}}
 // gdb-command:continue
 // gdb-command:print b
-// gdb-check:$4 = generator_objects::main::generator {__0: 0x[...], <<variant>>: {__state: 1, 0: generator_objects::main::generator::Unresumed, 1: generator_objects::main::generator::Returned, 2: generator_objects::main::generator::Panicked, 3: generator_objects::main::generator::variant#3 {[...]}, 4: generator_objects::main::generator::variant#4 {[...]}}}
+// gdb-check:$4 = generator_objects::main::generator {__0: 0x[...], <<variant>>: {__state: 1, 0: generator_objects::main::generator::Unresumed, 1: generator_objects::main::generator::Returned, 2: generator_objects::main::generator::Panicked, 3: generator_objects::main::generator::Suspend0 {[...]}, 4: generator_objects::main::generator::Suspend1 {[...]}}}
 
 // === LLDB TESTS ==================================================================================
 

Original file line number	Diff line number	Diff line change
`@@ -2995,7 +2995,7 @@ pub struct UnsafetyCheckResult {`
`2995`	`2995`	`}`
`2996`	`2996`
`2997`	`2997`	`newtype_index! {`
`2998`		`- pub struct GeneratorField {`
	`2998`	`+ pub struct GeneratorSavedLocal {`
`2999`	`2999`	`derive [HashStable]`
`3000`	`3000`	`DEBUG_FORMAT = "_{}",`
`3001`	`3001`	`}`
`@@ -3005,18 +3005,18 @@ newtype_index! {`
`3005`	`3005`	`#[derive(Clone, Debug, RustcEncodable, RustcDecodable, HashStable)]`
`3006`	`3006`	`pub struct GeneratorLayout<'tcx> {`
`3007`	`3007`	`/// The type of every local stored inside the generator.`
`3008`		`- pub field_tys: IndexVec<GeneratorField, Ty<'tcx>>,`
	`3008`	`+ pub field_tys: IndexVec<GeneratorSavedLocal, Ty<'tcx>>,`
`3009`	`3009`
`3010`	`3010`	`/// Which of the above fields are in each variant. Note that one field may`
`3011`	`3011`	`/// be stored in multiple variants.`
`3012`		`- pub variant_fields: IndexVec<VariantIdx, IndexVec<Field, GeneratorField>>,`
	`3012`	`+ pub variant_fields: IndexVec<VariantIdx, IndexVec<Field, GeneratorSavedLocal>>,`
`3013`	`3013`
`3014`	`3014`	`/// Names and scopes of all the stored generator locals.`
`3015`	`3015`	`/// NOTE(tmandry) This is strictly a temporary hack for codegen`
`3016`	`3016`	`/// debuginfo generation, and will be removed at some point.`
`3017`	`3017`	`/// Do NOT use it for anything else, local information should not be`
`3018`	`3018`	`/// in the MIR, please rely on local crate HIR or other side-channels.`
`3019`		`- pub __local_debuginfo_codegen_only_do_not_use: IndexVec<GeneratorField, LocalDecl<'tcx>>,`
	`3019`	`+ pub __local_debuginfo_codegen_only_do_not_use: IndexVec<GeneratorSavedLocal, LocalDecl<'tcx>>,`
`3020`	`3020`	`}`
`3021`	`3021`
`3022`	`3022`	`#[derive(Clone, Debug, RustcEncodable, RustcDecodable, HashStable)]`
`@@ -3582,7 +3582,7 @@ impl<'tcx> TypeFoldable<'tcx> for Field {`
`3582`	`3582`	`}`
`3583`	`3583`	`}`
`3584`	`3584`
`3585`		`-impl<'tcx> TypeFoldable<'tcx> for GeneratorField {`
	`3585`	`+impl<'tcx> TypeFoldable<'tcx> for GeneratorSavedLocal {`
`3586`	`3586`	`fn super_fold_with<'gcx: 'tcx, F: TypeFolder<'gcx, 'tcx>>(&self, _: &mut F) -> Self {`
`3587`	`3587`	`*self`
`3588`	`3588`	`}`
Original file line number	Diff line number	Diff line change
`@@ -605,6 +605,12 @@ impl<'a, 'tcx> LayoutCx<'tcx, TyCtxt<'a, 'tcx, 'tcx>> {`
`605`	`605`	`}`
`606`	`606`
`607`	`607`	`ty::Generator(def_id, ref substs, _) => {`
	`608`	`+ // FIXME(tmandry): For fields that are repeated in multiple`
	`609`	`+ // variants in the GeneratorLayout, we need code to ensure that`
	`610`	`+ // the offset of these fields never change. Right now this is`
	`611`	`+ // not an issue since every variant has every field, but once we`
	`612`	`+ // optimize this we have to be more careful.`
	`613`	`+`
`608`	`614`	`let discr_index = substs.prefix_tys(def_id, tcx).count();`
`609`	`615`	`let prefix_tys = substs.prefix_tys(def_id, tcx)`
`610`	`616`	`.chain(iter::once(substs.discr_ty(tcx)));`
`@@ -1691,7 +1697,7 @@ impl<'a, 'tcx, C> TyLayoutMethods<'tcx, C> for Ty<'tcx>`
`1691`	`1697`
`1692`	`1698`	`fn field(this: TyLayout<'tcx>, cx: &C, i: usize) -> C::TyLayout {`
`1693`	`1699`	`let tcx = cx.tcx();`
`1694`		`- let handle_discriminant = \|discr: &Scalar\| -> C::TyLayout {`
	`1700`	`+ let discr_layout = \|discr: &Scalar\| -> C::TyLayout {`
`1695`	`1701`	`let layout = LayoutDetails::scalar(cx, discr.clone());`
`1696`	`1702`	`MaybeResult::from_ok(TyLayout {`
`1697`	`1703`	`details: tcx.intern_layout(layout),`
`@@ -1781,7 +1787,7 @@ impl<'a, 'tcx, C> TyLayoutMethods<'tcx, C> for Ty<'tcx>`
`1781`	`1787`	`}`
`1782`	`1788`	`Variants::Multiple { ref discr, discr_index, .. } => {`
`1783`	`1789`	`if i == discr_index {`
`1784`		`- return handle_discriminant(discr);`
	`1790`	`+ return discr_layout(discr);`
`1785`	`1791`	`}`
`1786`	`1792`	`substs.prefix_tys(def_id, tcx).nth(i).unwrap()`
`1787`	`1793`	`}`
`@@ -1805,7 +1811,7 @@ impl<'a, 'tcx, C> TyLayoutMethods<'tcx, C> for Ty<'tcx>`
`1805`	`1811`	`// Discriminant field for enums (where applicable).`
`1806`	`1812`	`Variants::Multiple { ref discr, .. } => {`
`1807`	`1813`	`assert_eq!(i, 0);`
`1808`		`- return handle_discriminant(discr);`
	`1814`	`+ return discr_layout(discr);`
`1809`	`1815`	`}`
`1810`	`1816`	`}`
`1811`	`1817`	`}`
Original file line number	Diff line number	Diff line change
`@@ -63,10 +63,10 @@ fn uncached_llvm_type<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>,`
`63`	`63`	`write!(&mut name, "::{}", def.variants[index].ident).unwrap();`
`64`	`64`	`}`
`65`	`65`	`}`
`66`		`- if let (&ty::Generator(..), &layout::Variants::Single { index })`
	`66`	`+ if let (&ty::Generator(_, substs, _), &layout::Variants::Single { index })`
`67`	`67`	`= (&layout.ty.sty, &layout.variants)`
`68`	`68`	`{`
`69`		`- write!(&mut name, "::variant#{:?}", index).unwrap();`
	`69`	`+ write!(&mut name, "::{}", substs.variant_name(index)).unwrap();`
`70`	`70`	`}`
`71`	`71`	`Some(name)`
`72`	`72`	`}`
Original file line number	Diff line number	Diff line change
`@@ -58,7 +58,7 @@ impl MirPass for Deaggregator {`
`58`	`58`	`}`
`59`	`59`	`AggregateKind::Generator(..) => {`
`60`	`60`	`// Right now we only support initializing generators to`
`61`		`- // variant#0.`
	`61`	`+ // variant 0 (Unresumed).`
`62`	`62`	`let variant_index = VariantIdx::new(0);`
`63`	`63`	`set_discriminant = Some(Statement {`
`64`	`64`	`kind: StatementKind::SetDiscriminant {`