From dafc7ee6c974e39dba64ff77aefc5b82db65f625 Mon Sep 17 00:00:00 2001 From: Tyler Mandry Date: Thu, 20 Jun 2019 16:12:23 -0700 Subject: [PATCH 1/2] Keep ZSTs at beginning of generator layout fields Fixes #61793. --- src/librustc/ty/layout.rs | 63 ++++++++++++++++++-------- src/test/ui/async-await/issue-61793.rs | 17 +++++++ 2 files changed, 60 insertions(+), 20 deletions(-) create mode 100644 src/test/ui/async-await/issue-61793.rs diff --git a/src/librustc/ty/layout.rs b/src/librustc/ty/layout.rs index 9ce1d2eec5d27..765850adc8aa1 100644 --- a/src/librustc/ty/layout.rs +++ b/src/librustc/ty/layout.rs @@ -1311,10 +1311,19 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { ) -> Result<&'tcx LayoutDetails, LayoutError<'tcx>> { use SavedLocalEligibility::*; let tcx = self.tcx; - let recompute_memory_index = |offsets: &[Size]| -> Vec { + let recompute_memory_index = |offsets: &[Size], fields: &[TyLayout<'_>]| -> Vec { debug!("recompute_memory_index({:?})", offsets); + debug!("fields = {:#?}", fields); let mut inverse_index = (0..offsets.len() as u32).collect::>(); - inverse_index.sort_unstable_by_key(|i| offsets[*i as usize]); + inverse_index.sort_unstable_by_key(|i| { + // Place ZSTs before other fields at the same offset so all fields are + // in order by offset. Codegen expects this. + // + // In generators we can have ZST fields with nonzero offsets (these are + // fields specific to one variant that come after the prefix). + let zst = fields[*i as usize].is_zst(); + (offsets[*i as usize], !zst) + }); let mut index = vec![0; offsets.len()]; for i in 0..index.len() { @@ -1337,9 +1346,12 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { let prefix_tys = substs.prefix_tys(def_id, tcx) .chain(iter::once(substs.discr_ty(tcx))) .chain(promoted_tys); + let prefix_layouts = prefix_tys + .map(|ty| self.layout_of(ty)) + .collect::, _>>()?; let prefix = self.univariant_uninterned( ty, - &prefix_tys.map(|ty| self.layout_of(ty)).collect::, _>>()?, + &prefix_layouts, &ReprOptions::default(), StructKind::AlwaysSized)?; let (prefix_size, prefix_align) = (prefix.size, prefix.align); @@ -1354,7 +1366,11 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { let offsets_b = offsets.split_off(discr_index + 1); let offsets_a = offsets; - let memory_index = recompute_memory_index(&offsets_a); + // Okay to use `prefix_layouts` here since we're accessing + // fields (0..discr_index + 1); the field indices will be the + // same. + let memory_index = recompute_memory_index(&offsets_a, &prefix_layouts); + let outer_fields = FieldPlacement::Arbitrary { offsets: offsets_a, memory_index }; (outer_fields, offsets_b) } @@ -1364,24 +1380,29 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { let mut size = prefix.size; let mut align = prefix.align; let variants = info.variant_fields.iter_enumerated().map(|(index, variant_fields)| { - // Only include overlap-eligible fields when we compute our variant layout. - let variant_only_tys = variant_fields - .iter() - .filter(|local| { - match assignments[**local] { - Unassigned => bug!(), - Assigned(v) if v == index => true, - Assigned(_) => bug!("assignment does not match variant"), - Ineligible(_) => false, - } - }) - .map(|local| subst_field(info.field_tys[*local])); + let mut variant_layouts = Vec::with_capacity(variant_fields.len()); + let mut variant_only_layouts = Vec::with_capacity(variant_fields.len()); + for local in variant_fields { + let ty = subst_field(info.field_tys[*local]); + let layout = self.layout_of(ty)?; + + variant_layouts.push(layout); + + // Only include overlap-eligible fields when we compute our variant layout. + let variant_only = match assignments[*local] { + Unassigned => bug!(), + Assigned(v) if v == index => true, + Assigned(_) => bug!("assignment does not match variant"), + Ineligible(_) => false, + }; + if variant_only { + variant_only_layouts.push(layout); + } + } let mut variant = self.univariant_uninterned( ty, - &variant_only_tys - .map(|ty| self.layout_of(ty)) - .collect::, _>>()?, + &variant_only_layouts, &ReprOptions::default(), StructKind::Prefixed(prefix_size, prefix_align.abi))?; variant.variants = Variants::Single { index }; @@ -1408,7 +1429,7 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { } } } - let memory_index = recompute_memory_index(&combined_offsets); + let memory_index = recompute_memory_index(&combined_offsets, &variant_layouts); variant.fields = FieldPlacement::Arbitrary { offsets: combined_offsets, memory_index }; size = size.max(variant.size); @@ -1442,7 +1463,9 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { debug!("generator layout ({:?}): {:#?}", ty, layout); Ok(layout) } +} +impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { /// This is invoked by the `layout_raw` query to record the final /// layout of each type. #[inline(always)] diff --git a/src/test/ui/async-await/issue-61793.rs b/src/test/ui/async-await/issue-61793.rs new file mode 100644 index 0000000000000..2ee5803b4655c --- /dev/null +++ b/src/test/ui/async-await/issue-61793.rs @@ -0,0 +1,17 @@ +// This regression test exposed an issue where ZSTs were not being placed at the +// beinning of generator field layouts, causing an assertion error downstream. + +// compile-pass +// edition:2018 + +#![feature(async_await)] +#![allow(unused)] + +async fn foo(_: &(), _: F) {} + +fn main() { + foo(&(), || {}); + async { + foo(&(), || {}).await; + }; +} From 1930bfb1feb1039a6b48925ef72dade02ecdec05 Mon Sep 17 00:00:00 2001 From: Tyler Mandry Date: Thu, 20 Jun 2019 16:13:11 -0700 Subject: [PATCH 2/2] Rename GeneratorSubsts::prefix_tys to outer_tys for consistency --- src/librustc/ty/layout.rs | 8 ++++---- src/librustc/ty/sty.rs | 2 +- src/librustc_codegen_llvm/debuginfo/metadata.rs | 2 +- src/librustc_mir/borrow_check/nll/type_check/mod.rs | 8 ++++---- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/librustc/ty/layout.rs b/src/librustc/ty/layout.rs index 765850adc8aa1..4ba81293e9d18 100644 --- a/src/librustc/ty/layout.rs +++ b/src/librustc/ty/layout.rs @@ -1340,13 +1340,13 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { // Build a prefix layout, including "promoting" all ineligible // locals as part of the prefix. We compute the layout of all of // these fields at once to get optimal packing. - let discr_index = substs.prefix_tys(def_id, tcx).count(); + let discr_index = substs.outer_tys(def_id, tcx).count(); let promoted_tys = ineligible_locals.iter().map(|local| subst_field(info.field_tys[local])); - let prefix_tys = substs.prefix_tys(def_id, tcx) + let outer_tys = substs.outer_tys(def_id, tcx) .chain(iter::once(substs.discr_ty(tcx))) .chain(promoted_tys); - let prefix_layouts = prefix_tys + let prefix_layouts = outer_tys .map(|ty| self.layout_of(ty)) .collect::, _>>()?; let prefix = self.univariant_uninterned( @@ -2044,7 +2044,7 @@ where if i == discr_index { return discr_layout(discr); } - substs.prefix_tys(def_id, tcx).nth(i).unwrap() + substs.outer_tys(def_id, tcx).nth(i).unwrap() } } } diff --git a/src/librustc/ty/sty.rs b/src/librustc/ty/sty.rs index 8bfbd8b854b03..8f2beb5965400 100644 --- a/src/librustc/ty/sty.rs +++ b/src/librustc/ty/sty.rs @@ -558,7 +558,7 @@ impl<'tcx> GeneratorSubsts<'tcx> { /// This is the types of the fields of a generator which are not stored in a /// variant. #[inline] - pub fn prefix_tys(self, def_id: DefId, tcx: TyCtxt<'tcx>) -> impl Iterator> { + pub fn outer_tys(self, def_id: DefId, tcx: TyCtxt<'tcx>) -> impl Iterator> { self.upvar_tys(def_id, tcx) } } diff --git a/src/librustc_codegen_llvm/debuginfo/metadata.rs b/src/librustc_codegen_llvm/debuginfo/metadata.rs index fbeda43af42b0..d3b94bedc8ba9 100644 --- a/src/librustc_codegen_llvm/debuginfo/metadata.rs +++ b/src/librustc_codegen_llvm/debuginfo/metadata.rs @@ -690,7 +690,7 @@ pub fn type_metadata( usage_site_span).finalize(cx) } ty::Generator(def_id, substs, _) => { - let upvar_tys : Vec<_> = substs.prefix_tys(def_id, cx.tcx).map(|t| { + let upvar_tys : Vec<_> = substs.outer_tys(def_id, cx.tcx).map(|t| { cx.tcx.normalize_erasing_regions(ParamEnv::reveal_all(), t) }).collect(); prepare_enum_metadata(cx, diff --git a/src/librustc_mir/borrow_check/nll/type_check/mod.rs b/src/librustc_mir/borrow_check/nll/type_check/mod.rs index e1f5964ff9340..d56f186de5c3e 100644 --- a/src/librustc_mir/borrow_check/nll/type_check/mod.rs +++ b/src/librustc_mir/borrow_check/nll/type_check/mod.rs @@ -790,10 +790,10 @@ impl<'a, 'b, 'tcx> TypeVerifier<'a, 'b, 'tcx> { ty::Generator(def_id, substs, _) => { // Only prefix fields (upvars and current state) are // accessible without a variant index. - return match substs.prefix_tys(def_id, tcx).nth(field.index()) { + return match substs.outer_tys(def_id, tcx).nth(field.index()) { Some(ty) => Ok(ty), None => Err(FieldAccessError::OutOfRange { - field_count: substs.prefix_tys(def_id, tcx).count(), + field_count: substs.outer_tys(def_id, tcx).count(), }), } } @@ -1922,10 +1922,10 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { // It doesn't make sense to look at a field beyond the prefix; // these require a variant index, and are not initialized in // aggregate rvalues. - match substs.prefix_tys(def_id, tcx).nth(field_index) { + match substs.outer_tys(def_id, tcx).nth(field_index) { Some(ty) => Ok(ty), None => Err(FieldAccessError::OutOfRange { - field_count: substs.prefix_tys(def_id, tcx).count(), + field_count: substs.outer_tys(def_id, tcx).count(), }), } }