From cae94e8ec0971d6762fb06aa05c3d733e670abe5 Mon Sep 17 00:00:00 2001 From: Austin Hicks Date: Sun, 16 Oct 2016 17:31:19 -0400 Subject: [PATCH 01/28] Optimize anything using a layout::Struct by introducing a mapping from source code field order to in-memory field order and sorting by alignment. --- src/librustc/ty/layout.rs | 215 ++++++++++++++++++++++++++----------- src/librustc_trans/adt.rs | 42 ++++---- src/librustc_trans/base.rs | 4 +- 3 files changed, 180 insertions(+), 81 deletions(-) diff --git a/src/librustc/ty/layout.rs b/src/librustc/ty/layout.rs index 8646bccf1e9ed..74b3bc609abfe 100644 --- a/src/librustc/ty/layout.rs +++ b/src/librustc/ty/layout.rs @@ -24,6 +24,7 @@ use syntax_pos::DUMMY_SP; use std::cmp; use std::fmt; use std::i64; +use std::iter; /// Parsed [Data layout](http://llvm.org/docs/LangRef.html#data-layout) /// for a target, which contains everything needed to compute layouts. @@ -511,41 +512,76 @@ pub struct Struct { /// If true, the size is exact, otherwise it's only a lower bound. pub sized: bool, - /// Offsets for the first byte of each field. + /// Offsets for the first byte of each field, ordered to match the tys. + /// This vector does not go in increasing order. /// FIXME(eddyb) use small vector optimization for the common case. pub offsets: Vec, + /// Maps field indices to GEP indices, depending how fields were permuted. + /// FIXME (camlorn) also consider small vector optimization here. + pub gep_index: Vec, + pub min_size: Size, } impl<'a, 'gcx, 'tcx> Struct { - pub fn new(dl: &TargetDataLayout, packed: bool) -> Struct { - Struct { + pub fn new(dl: &TargetDataLayout, fields: I, + repr: attr::ReprAttr, is_enum_variant: bool, + scapegoat: Ty<'gcx>) -> Result> + where I: Iterator>>{ + let packed = repr == attr::ReprPacked; + let mut ret = Struct { align: if packed { dl.i8_align } else { dl.aggregate_align }, packed: packed, sized: true, offsets: vec![], + gep_index: vec![], min_size: Size::from_bytes(0), - } + }; + ret.fill_in_fields(dl, fields, scapegoat, repr, is_enum_variant)?; + Ok(ret) } - /// Extend the Struct with more fields. - pub fn extend(&mut self, dl: &TargetDataLayout, + fn fill_in_fields(&mut self, dl: &TargetDataLayout, fields: I, - scapegoat: Ty<'gcx>) + scapegoat: Ty<'gcx>, + repr: attr::ReprAttr, + is_enum_variant: bool) -> Result<(), LayoutError<'gcx>> where I: Iterator>> { - self.offsets.reserve(fields.size_hint().0); + let fields = fields.collect::, LayoutError<'gcx>>>()?; + if fields.len() == 0 {return Ok(())}; + + self.offsets = vec![Size::from_bytes(0); fields.len()]; + let mut inverse_gep_index: Vec = Vec::with_capacity(fields.len()); + inverse_gep_index.extend(0..fields.len() as u32); + + if repr == attr::ReprAny { + let start: usize = if is_enum_variant {1} else {0}; + // FIXME(camlorn): we can't reorder the last field because it is possible for structs to be coerced to unsized. + // Example: struct Foo { x: i32, y: T } + // We can coerce &Foo to &Foo. + let end = inverse_gep_index.len()-1; + if end > start { + let optimizing = &mut inverse_gep_index[start..end]; + optimizing.sort_by_key(|&x| fields[x as usize].align(dl).abi()); + } + } + + // At this point, inverse_gep_index holds field indices by increasing offset. + // That is, if field 5 has offset 0, the first element of inverse_gep_index is 5. + // We now write field offsets to the corresponding offset slot; field 5 with offset 0 puts 0 in offsets[5]. + // At the bottom of this function, we use inverse_gep_index to produce gep_index. - let mut offset = self.min_size; + let mut offset = Size::from_bytes(0); - for field in fields { + for i in inverse_gep_index.iter() { + let field = fields[*i as usize]; if !self.sized { bug!("Struct::extend: field #{} of `{}` comes after unsized field", self.offsets.len(), scapegoat); } - let field = field?; if field.is_unsized() { self.sized = false; } @@ -557,9 +593,10 @@ impl<'a, 'gcx, 'tcx> Struct { offset = offset.abi_align(align); } - self.offsets.push(offset); debug!("Struct::extend offset: {:?} field: {:?} {:?}", offset, field, field.size(dl)); + self.offsets[*i as usize] = offset; + offset = offset.checked_add(field.size(dl), dl) .map_or(Err(LayoutError::SizeOverflow(scapegoat)), Ok)?; @@ -569,12 +606,21 @@ impl<'a, 'gcx, 'tcx> Struct { self.min_size = offset; + // As stated above, inverse_gep_index holds field indices by increasing offset. + // This makes it an already-sorted view of the offsets vec. + // To invert it, consider: + // If field 5 has offset 0, offsets[0] is 5, and gep_index[5] should be 0. + // Field 5 would be the first element, so gep_index is i: + self.gep_index = vec![0; inverse_gep_index.len()]; + + for i in 0..inverse_gep_index.len() { + self.gep_index[inverse_gep_index[i] as usize] = i as u32; + } + Ok(()) } - /// Get the size without trailing alignment padding. - - /// Get the size with trailing aligment padding. + /// Get the size with trailing alignment padding. pub fn stride(&self) -> Size { self.min_size.abi_align(self.align) } @@ -592,10 +638,35 @@ impl<'a, 'gcx, 'tcx> Struct { Ok(true) } + /// Get indices of the tys that made this struct by increasing offset. + #[inline] + pub fn field_index_by_increasing_offset<'b>(&'b self) -> impl iter::Iterator+'b { + let mut inverse_small = [0u8; 64]; + let mut inverse_big = vec![]; + let use_small = self.gep_index.len() <= inverse_small.len(); + + // We have to write this logic twice in order to keep the array small. + if use_small { + for i in 0..self.gep_index.len() { + inverse_small[self.gep_index[i] as usize] = i as u8; + } + } else { + inverse_big = vec![0; self.gep_index.len()]; + for i in 0..self.gep_index.len() { + inverse_big[self.gep_index[i] as usize] = i as u32; + } + } + + (0..self.gep_index.len()).map(move |i| { + if use_small { inverse_small[i] as usize } + else { inverse_big[i] as usize } + }) + } + /// Find the path leading to a non-zero leaf field, starting from /// the given type and recursing through aggregates. // FIXME(eddyb) track value ranges and traverse already optimized enums. - pub fn non_zero_field_in_type(infcx: &InferCtxt<'a, 'gcx, 'tcx>, + fn non_zero_field_in_type(infcx: &InferCtxt<'a, 'gcx, 'tcx>, ty: Ty<'gcx>) -> Result, LayoutError<'gcx>> { let tcx = infcx.tcx.global_tcx(); @@ -625,27 +696,30 @@ impl<'a, 'gcx, 'tcx> Struct { // Perhaps one of the fields of this struct is non-zero // let's recurse and find out - (_, &ty::TyAdt(def, substs)) if def.is_struct() => { + (&Univariant { ref variant, .. }, &ty::TyAdt(def, substs)) if def.is_struct() => { Struct::non_zero_field_path(infcx, def.struct_variant().fields .iter().map(|field| { field.ty(tcx, substs) - })) + }), + Some(&variant.gep_index[..])) } // Perhaps one of the upvars of this closure is non-zero - // Let's recurse and find out! - (_, &ty::TyClosure(def_id, ref substs)) => { - Struct::non_zero_field_path(infcx, substs.upvar_tys(def_id, tcx)) + (&Univariant { ref variant, .. }, &ty::TyClosure(def, substs)) => { + let upvar_tys = substs.upvar_tys(def, tcx); + Struct::non_zero_field_path(infcx, upvar_tys, + Some(&variant.gep_index[..])) } // Can we use one of the fields in this tuple? - (_, &ty::TyTuple(tys)) => { - Struct::non_zero_field_path(infcx, tys.iter().cloned()) + (&Univariant { ref variant, .. }, &ty::TyTuple(tys)) => { + Struct::non_zero_field_path(infcx, tys.iter().cloned(), + Some(&variant.gep_index[..])) } // Is this a fixed-size array of something non-zero // with at least one element? (_, &ty::TyArray(ety, d)) if d > 0 => { - Struct::non_zero_field_path(infcx, Some(ety).into_iter()) + Struct::non_zero_field_path(infcx, Some(ety).into_iter(), None) } (_, &ty::TyProjection(_)) | (_, &ty::TyAnon(..)) => { @@ -663,13 +737,19 @@ impl<'a, 'gcx, 'tcx> Struct { /// Find the path leading to a non-zero leaf field, starting from /// the given set of fields and recursing through aggregates. - pub fn non_zero_field_path(infcx: &InferCtxt<'a, 'gcx, 'tcx>, - fields: I) + fn non_zero_field_path(infcx: &InferCtxt<'a, 'gcx, 'tcx>, + fields: I, + permutation: Option<&[u32]>) -> Result, LayoutError<'gcx>> where I: Iterator> { for (i, ty) in fields.enumerate() { if let Some(mut path) = Struct::non_zero_field_in_type(infcx, ty)? { - path.push(i as u32); + let index = if let Some(p) = permutation { + p[i] as usize + } else { + i + }; + path.push(index as u32); return Ok(Some(path)); } } @@ -723,7 +803,7 @@ impl<'a, 'gcx, 'tcx> Union { Ok(()) } - /// Get the size with trailing aligment padding. + /// Get the size with trailing alignment padding. pub fn stride(&self) -> Size { self.min_size.abi_align(self.align) } @@ -887,6 +967,7 @@ impl<'a, 'gcx, 'tcx> Layout { let dl = &tcx.data_layout; assert!(!ty.has_infer_types()); + let layout = match ty.sty { // Basic scalars. ty::TyBool => Scalar { value: Int(I1), non_zero: false }, @@ -908,7 +989,7 @@ impl<'a, 'gcx, 'tcx> Layout { ty::TyFnPtr(_) => Scalar { value: Pointer, non_zero: true }, // The never type. - ty::TyNever => Univariant { variant: Struct::new(dl, false), non_zero: false }, + ty::TyNever => Univariant { variant: Struct::new(dl, iter::empty(), attr::ReprAny, false, ty)?, non_zero: false }, // Potentially-fat pointers. ty::TyBox(pointee) | @@ -959,27 +1040,30 @@ impl<'a, 'gcx, 'tcx> Layout { // Odd unit types. ty::TyFnDef(..) => { Univariant { - variant: Struct::new(dl, false), + variant: Struct::new(dl, iter::empty(), attr::ReprAny, false, ty)?, non_zero: false } } - ty::TyDynamic(..) => { - let mut unit = Struct::new(dl, false); + ty::TyDynamic(_) => { + let mut unit = Struct::new(dl, iter::empty(), attr::ReprAny, false, ty)?; unit.sized = false; Univariant { variant: unit, non_zero: false } } // Tuples and closures. ty::TyClosure(def_id, ref substs) => { - let mut st = Struct::new(dl, false); let tys = substs.upvar_tys(def_id, tcx); - st.extend(dl, tys.map(|ty| ty.layout(infcx)), ty)?; + let mut st = Struct::new(dl, + tys.map(|ty| ty.layout(infcx)), + attr::ReprAny, + false, ty)?; Univariant { variant: st, non_zero: false } } ty::TyTuple(tys) => { - let mut st = Struct::new(dl, false); - st.extend(dl, tys.iter().map(|ty| ty.layout(infcx)), ty)?; + let st = Struct::new(dl, + tys.iter().map(|ty| ty.layout(infcx)), + attr::ReprAny, false, ty)?; Univariant { variant: st, non_zero: false } } @@ -1012,7 +1096,7 @@ impl<'a, 'gcx, 'tcx> Layout { assert_eq!(hint, attr::ReprAny); return success(Univariant { - variant: Struct::new(dl, false), + variant: Struct::new(dl, iter::empty(), hint, false, ty)?, non_zero: false }); } @@ -1050,8 +1134,7 @@ impl<'a, 'gcx, 'tcx> Layout { un.extend(dl, fields, ty)?; UntaggedUnion { variants: un } } else { - let mut st = Struct::new(dl, packed); - st.extend(dl, fields, ty)?; + let st = Struct::new(dl, fields, hint, false, ty)?; let non_zero = Some(def.did) == tcx.lang_items.non_zero(); Univariant { variant: st, non_zero: non_zero } }; @@ -1083,7 +1166,8 @@ impl<'a, 'gcx, 'tcx> Layout { continue; } let path = Struct::non_zero_field_path(infcx, - variants[discr].iter().cloned())?; + variants[discr].iter().cloned(), + None)?; let mut path = if let Some(p) = path { p } else { continue }; // FIXME(eddyb) should take advantage of a newtype. @@ -1101,10 +1185,17 @@ impl<'a, 'gcx, 'tcx> Layout { }); } + let st = Struct::new(dl, + variants[discr].iter().map(|ty| ty.layout(infcx)), + hint, false, ty)?; + + // We have to fix the last element of path here as only we know the right value. + let mut i = *path.last().unwrap(); + i = st.gep_index[i as usize]; + *path.last_mut().unwrap() = i; path.push(0); // For GEP through a pointer. path.reverse(); - let mut st = Struct::new(dl, false); - st.extend(dl, variants[discr].iter().map(|ty| ty.layout(infcx)), ty)?; + return success(StructWrappedNullablePointer { nndiscr: discr as u64, nonnull: st, @@ -1126,24 +1217,25 @@ impl<'a, 'gcx, 'tcx> Layout { // Create the set of structs that represent each variant // Use the minimum integer type we figured out above - let discr = Some(Scalar { value: Int(min_ity), non_zero: false }); + let discr = Scalar { value: Int(min_ity), non_zero: false }; let mut variants = variants.into_iter().map(|fields| { - let mut found_start = false; - let fields = fields.into_iter().map(|field| { - let field = field.layout(infcx)?; - if !found_start { - // Find the first field we can't move later - // to make room for a larger discriminant. - let field_align = field.align(dl); - if field.size(dl).bytes() != 0 || field_align.abi() != 1 { - start_align = start_align.min(field_align); - found_start = true; - } + let mut fields = fields.into_iter().map(|field| { + field.layout(infcx) + }).collect::>(); + fields.insert(0, Ok(&discr)); + let st = Struct::new(dl, + fields.iter().cloned(), + hint, false, ty)?; + // Find the first field we can't move later + // to make room for a larger discriminant. + for i in st.field_index_by_increasing_offset() { + let field = fields[i].unwrap(); + let field_align = field.align(dl); + if field.size(dl).bytes() != 0 || field_align.abi() != 1 { + start_align = start_align.min(field_align); + break; } - Ok(field) - }); - let mut st = Struct::new(dl, false); - st.extend(dl, discr.iter().map(Ok).chain(fields), ty)?; + } size = cmp::max(size, st.min_size); align = align.max(st.align); Ok(st) @@ -1177,11 +1269,10 @@ impl<'a, 'gcx, 'tcx> Layout { let old_ity_size = Int(min_ity).size(dl); let new_ity_size = Int(ity).size(dl); for variant in &mut variants { - for offset in &mut variant.offsets[1..] { - if *offset > old_ity_size { - break; + for i in variant.offsets.iter_mut() { + if *i <= old_ity_size { + *i = new_ity_size; } - *offset = new_ity_size; } // We might be making the struct larger. if variant.min_size <= old_ity_size { diff --git a/src/librustc_trans/adt.rs b/src/librustc_trans/adt.rs index 8ee362bae3551..0690bea6c2f85 100644 --- a/src/librustc_trans/adt.rs +++ b/src/librustc_trans/adt.rs @@ -151,14 +151,14 @@ pub fn finish_type_of<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>, | layout::UntaggedUnion { .. } | layout::RawNullablePointer { .. } => { } layout::Univariant { ..} | layout::StructWrappedNullablePointer { .. } => { - let (nonnull_variant, packed) = match *l { - layout::Univariant { ref variant, .. } => (0, variant.packed), + let (nonnull_variant_index, nonnull_variant, packed) = match *l { + layout::Univariant { ref variant, .. } => (0, variant, variant.packed), layout::StructWrappedNullablePointer { nndiscr, ref nonnull, .. } => - (nndiscr, nonnull.packed), + (nndiscr, nonnull, nonnull.packed), _ => unreachable!() }; - let fields = compute_fields(cx, t, nonnull_variant as usize, true); - llty.set_struct_body(&struct_llfields(cx, &fields, false, false), + let fields = compute_fields(cx, t, nonnull_variant_index as usize, true); + llty.set_struct_body(&struct_llfields(cx, &fields, nonnull_variant, false, false), packed) }, _ => bug!("This function cannot handle {} with layout {:#?}", t, l) @@ -188,7 +188,7 @@ fn generic_type_of<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>, let fields = compute_fields(cx, t, nndiscr as usize, false); match name { None => { - Type::struct_(cx, &struct_llfields(cx, &fields, sizing, dst), + Type::struct_(cx, &struct_llfields(cx, &fields, nonnull, sizing, dst), nonnull.packed) } Some(name) => { @@ -203,7 +203,7 @@ fn generic_type_of<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>, let fields = compute_fields(cx, t, 0, true); match name { None => { - let fields = struct_llfields(cx, &fields, sizing, dst); + let fields = struct_llfields(cx, &fields, &variant, sizing, dst); Type::struct_(cx, &fields, variant.packed) } Some(name) => { @@ -291,12 +291,14 @@ fn union_fill(cx: &CrateContext, size: u64, align: u64) -> Type { fn struct_llfields<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>, fields: &Vec>, + variant: &layout::Struct, sizing: bool, dst: bool) -> Vec { + let fields = variant.field_index_by_increasing_offset().map(|i| fields[i as usize]); if sizing { - fields.iter().filter(|&ty| !dst || type_is_sized(cx.tcx(), *ty)) - .map(|&ty| type_of::sizing_type_of(cx, ty)).collect() + fields.filter(|ty| !dst || type_is_sized(cx.tcx(), *ty)) + .map(|ty| type_of::sizing_type_of(cx, ty)).collect() } else { - fields.iter().map(|&ty| type_of::in_memory_type_of(cx, ty)).collect() + fields.map(|ty| type_of::in_memory_type_of(cx, ty)).collect() } } @@ -564,16 +566,16 @@ pub fn trans_field_ptr_builder<'blk, 'tcx>(bcx: &BlockAndBuilder<'blk, 'tcx>, fn struct_field_ptr<'blk, 'tcx>(bcx: &BlockAndBuilder<'blk, 'tcx>, st: &layout::Struct, fields: &Vec>, val: MaybeSizedValue, ix: usize, needs_cast: bool) -> ValueRef { - let ccx = bcx.ccx(); let fty = fields[ix]; + let ccx = bcx.ccx(); let ll_fty = type_of::in_memory_type_of(bcx.ccx(), fty); if bcx.is_unreachable() { return C_undef(ll_fty.ptr_to()); } let ptr_val = if needs_cast { - let fields = fields.iter().map(|&ty| { - type_of::in_memory_type_of(ccx, ty) + let fields = st.field_index_by_increasing_offset().map(|i| { + type_of::in_memory_type_of(ccx, fields[i]) }).collect::>(); let real_ty = Type::struct_(ccx, &fields[..], st.packed); bcx.pointercast(val.value, real_ty.ptr_to()) @@ -585,15 +587,15 @@ fn struct_field_ptr<'blk, 'tcx>(bcx: &BlockAndBuilder<'blk, 'tcx>, // * First field - Always aligned properly // * Packed struct - There is no alignment padding // * Field is sized - pointer is properly aligned already - if ix == 0 || st.packed || type_is_sized(bcx.tcx(), fty) { - return bcx.struct_gep(ptr_val, ix); + if st.offsets[ix] == layout::Size::from_bytes(0) || st.packed || type_is_sized(bcx.tcx(), fty) { + return bcx.struct_gep(ptr_val, st.gep_index[ix] as usize); } // If the type of the last field is [T] or str, then we don't need to do // any adjusments match fty.sty { ty::TySlice(..) | ty::TyStr => { - return bcx.struct_gep(ptr_val, ix); + return bcx.struct_gep(ptr_val, st.gep_index[ix] as usize); } _ => () } @@ -755,8 +757,12 @@ fn build_const_struct<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, // offset of current value let mut offset = 0; let mut cfields = Vec::new(); - let offsets = st.offsets.iter().map(|i| i.bytes()); - for (&val, target_offset) in vals.iter().zip(offsets) { + cfields.reserve(st.offsets.len()*2); + + let parts = st.field_index_by_increasing_offset().map(|i| { + (&vals[i], st.offsets[i].bytes()) + }); + for (&val, target_offset) in parts { if offset < target_offset { cfields.push(padding(ccx, target_offset - offset)); offset = target_offset; diff --git a/src/librustc_trans/base.rs b/src/librustc_trans/base.rs index f70c24c3ccb2d..c379ec7da90e1 100644 --- a/src/librustc_trans/base.rs +++ b/src/librustc_trans/base.rs @@ -827,7 +827,9 @@ pub fn alloca(cx: Block, ty: Type, name: &str) -> ValueRef { } } DebugLoc::None.apply(cx.fcx); - Alloca(cx, ty, name) + let result = Alloca(cx, ty, name); + debug!("alloca({:?}) = {:?}", name, result); + result } impl<'blk, 'tcx> FunctionContext<'blk, 'tcx> { From 8cfbffea3bca6152ab1a829c4a1cbff009581ebb Mon Sep 17 00:00:00 2001 From: Austin Hicks Date: Wed, 16 Nov 2016 17:00:35 -0500 Subject: [PATCH 02/28] Fix bugs to optimizing enums: - The discriminant must be first in all variants. - The loop responsible for patching enum variants when the discriminant is enlarged was nonfunctional. --- src/librustc/ty/layout.rs | 11 ++++++++--- src/librustc_lint/types.rs | 2 +- src/test/run-pass/enum-size-variance.rs | 3 +++ src/test/run-pass/nonzero-enum.rs | 5 +++-- 4 files changed, 15 insertions(+), 6 deletions(-) diff --git a/src/librustc/ty/layout.rs b/src/librustc/ty/layout.rs index 74b3bc609abfe..f4960b5680f01 100644 --- a/src/librustc/ty/layout.rs +++ b/src/librustc/ty/layout.rs @@ -550,6 +550,7 @@ impl<'a, 'gcx, 'tcx> Struct { -> Result<(), LayoutError<'gcx>> where I: Iterator>> { let fields = fields.collect::, LayoutError<'gcx>>>()?; + if is_enum_variant { assert!(fields.len() >= 1, "Enum variants must have at least a discriminant field.") } if fields.len() == 0 {return Ok(())}; self.offsets = vec![Size::from_bytes(0); fields.len()]; @@ -566,6 +567,7 @@ impl<'a, 'gcx, 'tcx> Struct { let optimizing = &mut inverse_gep_index[start..end]; optimizing.sort_by_key(|&x| fields[x as usize].align(dl).abi()); } + if is_enum_variant { assert_eq!(inverse_gep_index[0], 0, "Enums must have field 0 as the field with lowest offset.") } } // At this point, inverse_gep_index holds field indices by increasing offset. @@ -1053,7 +1055,7 @@ impl<'a, 'gcx, 'tcx> Layout { // Tuples and closures. ty::TyClosure(def_id, ref substs) => { let tys = substs.upvar_tys(def_id, tcx); - let mut st = Struct::new(dl, + let st = Struct::new(dl, tys.map(|ty| ty.layout(infcx)), attr::ReprAny, false, ty)?; @@ -1228,7 +1230,8 @@ impl<'a, 'gcx, 'tcx> Layout { hint, false, ty)?; // Find the first field we can't move later // to make room for a larger discriminant. - for i in st.field_index_by_increasing_offset() { + // It is important to skip the first field. + for i in st.field_index_by_increasing_offset().skip(1) { let field = fields[i].unwrap(); let field_align = field.align(dl); if field.size(dl).bytes() != 0 || field_align.abi() != 1 { @@ -1270,7 +1273,9 @@ impl<'a, 'gcx, 'tcx> Layout { let new_ity_size = Int(ity).size(dl); for variant in &mut variants { for i in variant.offsets.iter_mut() { - if *i <= old_ity_size { + // The first field is the discrimminant, at offset 0. + // These aren't in order, and we need to skip it. + if *i <= old_ity_size && *i > Size::from_bytes(0){ *i = new_ity_size; } } diff --git a/src/librustc_lint/types.rs b/src/librustc_lint/types.rs index 5e9fdbfa07314..49bbcc18efb28 100644 --- a/src/librustc_lint/types.rs +++ b/src/librustc_lint/types.rs @@ -750,7 +750,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for VariantSizeDifferences { if let Layout::General { ref variants, ref size, discr, .. } = *layout { let discr_size = Primitive::Int(discr).size(&cx.tcx.data_layout).bytes(); - debug!("enum `{}` is {} bytes large", t, size.bytes()); + debug!("enum `{}` is {} bytes large with layout:\n{:#?}", t, size.bytes(), layout); let (largest, slargest, largest_index) = enum_definition.variants .iter() diff --git a/src/test/run-pass/enum-size-variance.rs b/src/test/run-pass/enum-size-variance.rs index 26deb0ed72ade..beccbf5eb0394 100644 --- a/src/test/run-pass/enum-size-variance.rs +++ b/src/test/run-pass/enum-size-variance.rs @@ -11,6 +11,9 @@ #![warn(variant_size_differences)] #![allow(dead_code)] +// Note that the following test only works because all fields of the enum variants are of the same size. +// If this test is modified so that the reordering logic in librustc/ty/layout.rs kicks in, it will fail. + enum Enum1 { } enum Enum2 { A, B, C } diff --git a/src/test/run-pass/nonzero-enum.rs b/src/test/run-pass/nonzero-enum.rs index 266506e04b74d..2cd3136b0ebe6 100644 --- a/src/test/run-pass/nonzero-enum.rs +++ b/src/test/run-pass/nonzero-enum.rs @@ -26,8 +26,9 @@ fn main() { assert_eq!(size_of::(), 1); assert_eq!(size_of::>(), 1); assert_eq!(size_of::>(), 1); - assert_eq!(size_of::(), 4); - assert_eq!(size_of::>(), 4); + // The next asserts are correct given the currently dumb field reordering algorithm, which actually makes this struct larger. + assert_eq!(size_of::(), 6); + assert_eq!(size_of::>(), 6); let enone = None::; let esome = Some(E::A); if let Some(..) = enone { From 1969aeb3d708237327239fdfb7da3d575601199e Mon Sep 17 00:00:00 2001 From: Austin Hicks Date: Fri, 18 Nov 2016 13:41:21 -0500 Subject: [PATCH 03/28] Make constant field access account for field reordering. --- src/librustc_trans/adt.rs | 11 ++++------- src/test/run-pass/nonzero-enum.rs | 4 +--- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/src/librustc_trans/adt.rs b/src/librustc_trans/adt.rs index 0690bea6c2f85..c7a177196ffe5 100644 --- a/src/librustc_trans/adt.rs +++ b/src/librustc_trans/adt.rs @@ -813,14 +813,11 @@ pub fn const_get_field<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, t: Ty<'tcx>, let l = ccx.layout_of(t); match *l { layout::CEnum { .. } => bug!("element access in C-like enum const"), - layout::Univariant { .. } | layout::Vector { .. } => const_struct_field(val, ix), + layout::Univariant { ref variant, .. } => { + const_struct_field(val, variant.gep_index[ix] as usize) + } + layout::Vector { .. } => const_struct_field(val, ix), layout::UntaggedUnion { .. } => const_struct_field(val, 0), - layout::General { .. } => const_struct_field(val, ix + 1), - layout::RawNullablePointer { .. } => { - assert_eq!(ix, 0); - val - }, - layout::StructWrappedNullablePointer{ .. } => const_struct_field(val, ix), _ => bug!("{} does not have fields.", t) } } diff --git a/src/test/run-pass/nonzero-enum.rs b/src/test/run-pass/nonzero-enum.rs index 2cd3136b0ebe6..fc92c9df9f7ef 100644 --- a/src/test/run-pass/nonzero-enum.rs +++ b/src/test/run-pass/nonzero-enum.rs @@ -26,9 +26,7 @@ fn main() { assert_eq!(size_of::(), 1); assert_eq!(size_of::>(), 1); assert_eq!(size_of::>(), 1); - // The next asserts are correct given the currently dumb field reordering algorithm, which actually makes this struct larger. - assert_eq!(size_of::(), 6); - assert_eq!(size_of::>(), 6); + assert_eq!(size_of::>(), size_of::()); let enone = None::; let esome = Some(E::A); if let Some(..) = enone { From 27469037d7dabe602e0f528dbb9791c44cc236b0 Mon Sep 17 00:00:00 2001 From: Austin Hicks Date: Fri, 18 Nov 2016 14:20:02 -0500 Subject: [PATCH 04/28] Fix extern-pass-empty test, which needed repr(C) --- src/test/run-pass/extern-pass-empty.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/test/run-pass/extern-pass-empty.rs b/src/test/run-pass/extern-pass-empty.rs index cce7dc5bf32ef..2606c92868004 100644 --- a/src/test/run-pass/extern-pass-empty.rs +++ b/src/test/run-pass/extern-pass-empty.rs @@ -14,11 +14,13 @@ // ignore-msvc // ignore-emscripten +#[repr(C)] struct TwoU8s { one: u8, two: u8, } +#[repr(C)] struct ManyInts { arg1: i8, arg2: i16, @@ -28,6 +30,7 @@ struct ManyInts { arg6: TwoU8s, } +#[repr(C)] struct Empty; #[link(name = "rust_test_helpers", kind = "static")] From d75477808307ca1c0e0384f300404b94565b7ac4 Mon Sep 17 00:00:00 2001 From: Austin Hicks Date: Sun, 20 Nov 2016 13:14:13 -0500 Subject: [PATCH 05/28] Fix tuple and closure literals. --- src/librustc_trans/mir/operand.rs | 2 +- src/librustc_trans/mir/rvalue.rs | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/librustc_trans/mir/operand.rs b/src/librustc_trans/mir/operand.rs index 62eda56e2e1ba..83e1d03c689ab 100644 --- a/src/librustc_trans/mir/operand.rs +++ b/src/librustc_trans/mir/operand.rs @@ -246,7 +246,7 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { lldest: ValueRef, operand: OperandRef<'tcx>) { - debug!("store_operand: operand={:?}", operand); + debug!("store_operand: operand={:?} lldest={:?}", operand, lldest); bcx.with_block(|bcx| self.store_operand_direct(bcx, lldest, operand)) } diff --git a/src/librustc_trans/mir/rvalue.rs b/src/librustc_trans/mir/rvalue.rs index 15cbbc720d6d4..a89c61cd36a2b 100644 --- a/src/librustc_trans/mir/rvalue.rs +++ b/src/librustc_trans/mir/rvalue.rs @@ -133,6 +133,13 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { } }, _ => { + // If this is a tuple or closure, we need to translate GEP indices. + let layout = bcx.ccx().layout_of(dest.ty.to_ty(bcx.tcx())); + let translation = if let Layout::Univariant { ref variant, .. } = *layout { + Some(&variant.gep_index) + } else { + None + }; for (i, operand) in operands.iter().enumerate() { let op = self.trans_operand(&bcx, operand); // Do not generate stores and GEPis for zero-sized fields. @@ -140,6 +147,11 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { // Note: perhaps this should be StructGep, but // note that in some cases the values here will // not be structs but arrays. + let i = if let Some(ref t) = translation { + t[i] as usize + } else { + i + }; let dest = bcx.gepi(dest.llval, &[0, i]); self.store_operand(&bcx, dest, op); } From c7ec0dfcb68eee8a3313e47dd00dbb1709bdbcdd Mon Sep 17 00:00:00 2001 From: Austin Hicks Date: Sun, 20 Nov 2016 13:22:54 -0500 Subject: [PATCH 06/28] Add yet more missing #[repr(C)] to tests --- src/test/run-pass/struct-return.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/test/run-pass/struct-return.rs b/src/test/run-pass/struct-return.rs index 3d4601ad0cfa1..ed618cea98ac0 100644 --- a/src/test/run-pass/struct-return.rs +++ b/src/test/run-pass/struct-return.rs @@ -9,9 +9,11 @@ // except according to those terms. // +#[repr(C)] #[derive(Copy, Clone)] pub struct Quad { a: u64, b: u64, c: u64, d: u64 } +#[repr(C)] #[derive(Copy, Clone)] pub struct Floats { a: f64, b: u8, c: f64 } From 0e61c0e231ccd1db200917c1adff35a0128dffe8 Mon Sep 17 00:00:00 2001 From: Austin Hicks Date: Sun, 20 Nov 2016 14:04:17 -0500 Subject: [PATCH 07/28] Incorporate a bunch of review comments. --- src/librustc/ty/layout.rs | 99 ++++++++++++++------------------ src/librustc_trans/adt.rs | 6 +- src/librustc_trans/mir/rvalue.rs | 2 +- 3 files changed, 47 insertions(+), 60 deletions(-) diff --git a/src/librustc/ty/layout.rs b/src/librustc/ty/layout.rs index f4960b5680f01..3d73af28dc99c 100644 --- a/src/librustc/ty/layout.rs +++ b/src/librustc/ty/layout.rs @@ -512,14 +512,14 @@ pub struct Struct { /// If true, the size is exact, otherwise it's only a lower bound. pub sized: bool, - /// Offsets for the first byte of each field, ordered to match the tys. + /// Offsets for the first byte of each field, ordered to match the source definition order. /// This vector does not go in increasing order. /// FIXME(eddyb) use small vector optimization for the common case. pub offsets: Vec, - /// Maps field indices to GEP indices, depending how fields were permuted. + /// Maps source order field indices to memory order indices, depending how fields were permuted. /// FIXME (camlorn) also consider small vector optimization here. - pub gep_index: Vec, + pub memory_index: Vec, pub min_size: Size, } @@ -535,91 +535,78 @@ impl<'a, 'gcx, 'tcx> Struct { packed: packed, sized: true, offsets: vec![], - gep_index: vec![], + memory_index: vec![], min_size: Size::from_bytes(0), }; - ret.fill_in_fields(dl, fields, scapegoat, repr, is_enum_variant)?; - Ok(ret) - } - fn fill_in_fields(&mut self, dl: &TargetDataLayout, - fields: I, - scapegoat: Ty<'gcx>, - repr: attr::ReprAttr, - is_enum_variant: bool) - -> Result<(), LayoutError<'gcx>> - where I: Iterator>> { let fields = fields.collect::, LayoutError<'gcx>>>()?; if is_enum_variant { assert!(fields.len() >= 1, "Enum variants must have at least a discriminant field.") } - if fields.len() == 0 {return Ok(())}; + if fields.len() == 0 {return Ok(ret)}; - self.offsets = vec![Size::from_bytes(0); fields.len()]; - let mut inverse_gep_index: Vec = Vec::with_capacity(fields.len()); - inverse_gep_index.extend(0..fields.len() as u32); + ret.offsets = vec![Size::from_bytes(0); fields.len()]; + let mut inverse_memory_index: Vec = (0..fields.len() as u32).collect(); if repr == attr::ReprAny { - let start: usize = if is_enum_variant {1} else {0}; + let start = if is_enum_variant {1} else {0}; // FIXME(camlorn): we can't reorder the last field because it is possible for structs to be coerced to unsized. // Example: struct Foo { x: i32, y: T } // We can coerce &Foo to &Foo. - let end = inverse_gep_index.len()-1; + let end = inverse_memory_index.len()-1; if end > start { - let optimizing = &mut inverse_gep_index[start..end]; + let optimizing = &mut inverse_memory_index[start..end]; optimizing.sort_by_key(|&x| fields[x as usize].align(dl).abi()); } - if is_enum_variant { assert_eq!(inverse_gep_index[0], 0, "Enums must have field 0 as the field with lowest offset.") } + if is_enum_variant { assert_eq!(inverse_memory_index[0], 0, "Enums must have field 0 as the field with lowest offset.") } } - // At this point, inverse_gep_index holds field indices by increasing offset. - // That is, if field 5 has offset 0, the first element of inverse_gep_index is 5. + // At this point, inverse_memory_index holds field indices by increasing offset. + // That is, if field 5 has offset 0, the first element of inverse_memory_index is 5. // We now write field offsets to the corresponding offset slot; field 5 with offset 0 puts 0 in offsets[5]. - // At the bottom of this function, we use inverse_gep_index to produce gep_index. + // At the bottom of this function, we use inverse_memory_index to produce memory_index. let mut offset = Size::from_bytes(0); - for i in inverse_gep_index.iter() { + for i in inverse_memory_index.iter() { let field = fields[*i as usize]; - if !self.sized { - bug!("Struct::extend: field #{} of `{}` comes after unsized field", - self.offsets.len(), scapegoat); + if !ret.sized { + bug!("Struct::new: field #{} of `{}` comes after unsized field", + ret.offsets.len(), scapegoat); } if field.is_unsized() { - self.sized = false; + ret.sized = false; } // Invariant: offset < dl.obj_size_bound() <= 1<<61 - if !self.packed { + if !ret.packed { let align = field.align(dl); - self.align = self.align.max(align); + ret.align = ret.align.max(align); offset = offset.abi_align(align); } - - debug!("Struct::extend offset: {:?} field: {:?} {:?}", offset, field, field.size(dl)); - self.offsets[*i as usize] = offset; - + debug!("Struct::new offset: {:?} field: {:?} {:?}", offset, field, field.size(dl)); + ret.offsets[*i as usize] = offset; offset = offset.checked_add(field.size(dl), dl) .map_or(Err(LayoutError::SizeOverflow(scapegoat)), Ok)?; } - debug!("Struct::extend min_size: {:?}", offset); - self.min_size = offset; + debug!("Struct::new min_size: {:?}", offset); + ret.min_size = offset; - // As stated above, inverse_gep_index holds field indices by increasing offset. + // As stated above, inverse_memory_index holds field indices by increasing offset. // This makes it an already-sorted view of the offsets vec. // To invert it, consider: - // If field 5 has offset 0, offsets[0] is 5, and gep_index[5] should be 0. - // Field 5 would be the first element, so gep_index is i: - self.gep_index = vec![0; inverse_gep_index.len()]; + // If field 5 has offset 0, offsets[0] is 5, and memory_index[5] should be 0. + // Field 5 would be the first element, so memory_index is i: + ret.memory_index = vec![0; inverse_memory_index.len()]; - for i in 0..inverse_gep_index.len() { - self.gep_index[inverse_gep_index[i] as usize] = i as u32; + for i in 0..inverse_memory_index.len() { + ret.memory_index[inverse_memory_index[i] as usize] = i as u32; } - Ok(()) + Ok(ret) } /// Get the size with trailing alignment padding. @@ -645,21 +632,21 @@ impl<'a, 'gcx, 'tcx> Struct { pub fn field_index_by_increasing_offset<'b>(&'b self) -> impl iter::Iterator+'b { let mut inverse_small = [0u8; 64]; let mut inverse_big = vec![]; - let use_small = self.gep_index.len() <= inverse_small.len(); + let use_small = self.memory_index.len() <= inverse_small.len(); // We have to write this logic twice in order to keep the array small. if use_small { - for i in 0..self.gep_index.len() { - inverse_small[self.gep_index[i] as usize] = i as u8; + for i in 0..self.memory_index.len() { + inverse_small[self.memory_index[i] as usize] = i as u8; } } else { - inverse_big = vec![0; self.gep_index.len()]; - for i in 0..self.gep_index.len() { - inverse_big[self.gep_index[i] as usize] = i as u32; + inverse_big = vec![0; self.memory_index.len()]; + for i in 0..self.memory_index.len() { + inverse_big[self.memory_index[i] as usize] = i as u32; } } - (0..self.gep_index.len()).map(move |i| { + (0..self.memory_index.len()).map(move |i| { if use_small { inverse_small[i] as usize } else { inverse_big[i] as usize } }) @@ -703,19 +690,19 @@ impl<'a, 'gcx, 'tcx> Struct { .iter().map(|field| { field.ty(tcx, substs) }), - Some(&variant.gep_index[..])) + Some(&variant.memory_index[..])) } // Perhaps one of the upvars of this closure is non-zero (&Univariant { ref variant, .. }, &ty::TyClosure(def, substs)) => { let upvar_tys = substs.upvar_tys(def, tcx); Struct::non_zero_field_path(infcx, upvar_tys, - Some(&variant.gep_index[..])) + Some(&variant.memory_index[..])) } // Can we use one of the fields in this tuple? (&Univariant { ref variant, .. }, &ty::TyTuple(tys)) => { Struct::non_zero_field_path(infcx, tys.iter().cloned(), - Some(&variant.gep_index[..])) + Some(&variant.memory_index[..])) } // Is this a fixed-size array of something non-zero @@ -1193,7 +1180,7 @@ impl<'a, 'gcx, 'tcx> Layout { // We have to fix the last element of path here as only we know the right value. let mut i = *path.last().unwrap(); - i = st.gep_index[i as usize]; + i = st.memory_index[i as usize]; *path.last_mut().unwrap() = i; path.push(0); // For GEP through a pointer. path.reverse(); diff --git a/src/librustc_trans/adt.rs b/src/librustc_trans/adt.rs index c7a177196ffe5..9c82e25077371 100644 --- a/src/librustc_trans/adt.rs +++ b/src/librustc_trans/adt.rs @@ -588,14 +588,14 @@ fn struct_field_ptr<'blk, 'tcx>(bcx: &BlockAndBuilder<'blk, 'tcx>, // * Packed struct - There is no alignment padding // * Field is sized - pointer is properly aligned already if st.offsets[ix] == layout::Size::from_bytes(0) || st.packed || type_is_sized(bcx.tcx(), fty) { - return bcx.struct_gep(ptr_val, st.gep_index[ix] as usize); + return bcx.struct_gep(ptr_val, st.memory_index[ix] as usize); } // If the type of the last field is [T] or str, then we don't need to do // any adjusments match fty.sty { ty::TySlice(..) | ty::TyStr => { - return bcx.struct_gep(ptr_val, st.gep_index[ix] as usize); + return bcx.struct_gep(ptr_val, st.memory_index[ix] as usize); } _ => () } @@ -814,7 +814,7 @@ pub fn const_get_field<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, t: Ty<'tcx>, match *l { layout::CEnum { .. } => bug!("element access in C-like enum const"), layout::Univariant { ref variant, .. } => { - const_struct_field(val, variant.gep_index[ix] as usize) + const_struct_field(val, variant.memory_index[ix] as usize) } layout::Vector { .. } => const_struct_field(val, ix), layout::UntaggedUnion { .. } => const_struct_field(val, 0), diff --git a/src/librustc_trans/mir/rvalue.rs b/src/librustc_trans/mir/rvalue.rs index a89c61cd36a2b..2ee49db477864 100644 --- a/src/librustc_trans/mir/rvalue.rs +++ b/src/librustc_trans/mir/rvalue.rs @@ -136,7 +136,7 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { // If this is a tuple or closure, we need to translate GEP indices. let layout = bcx.ccx().layout_of(dest.ty.to_ty(bcx.tcx())); let translation = if let Layout::Univariant { ref variant, .. } = *layout { - Some(&variant.gep_index) + Some(&variant.memory_index) } else { None }; From 8e852e9902dd006a7e77a7349edb4e88c77e5842 Mon Sep 17 00:00:00 2001 From: Austin Hicks Date: Sun, 20 Nov 2016 14:38:25 -0500 Subject: [PATCH 08/28] Struct::new takes a vec, avoiding double allocation in some cases --- src/librustc/ty/layout.rs | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/src/librustc/ty/layout.rs b/src/librustc/ty/layout.rs index 3d73af28dc99c..bcbafc8a28a5e 100644 --- a/src/librustc/ty/layout.rs +++ b/src/librustc/ty/layout.rs @@ -525,10 +525,9 @@ pub struct Struct { } impl<'a, 'gcx, 'tcx> Struct { - pub fn new(dl: &TargetDataLayout, fields: I, + pub fn new(dl: &TargetDataLayout, fields: &Vec<&'a Layout>, repr: attr::ReprAttr, is_enum_variant: bool, - scapegoat: Ty<'gcx>) -> Result> - where I: Iterator>>{ + scapegoat: Ty<'gcx>) -> Result> { let packed = repr == attr::ReprPacked; let mut ret = Struct { align: if packed { dl.i8_align } else { dl.aggregate_align }, @@ -539,7 +538,6 @@ impl<'a, 'gcx, 'tcx> Struct { min_size: Size::from_bytes(0), }; - let fields = fields.collect::, LayoutError<'gcx>>>()?; if is_enum_variant { assert!(fields.len() >= 1, "Enum variants must have at least a discriminant field.") } if fields.len() == 0 {return Ok(ret)}; @@ -978,7 +976,7 @@ impl<'a, 'gcx, 'tcx> Layout { ty::TyFnPtr(_) => Scalar { value: Pointer, non_zero: true }, // The never type. - ty::TyNever => Univariant { variant: Struct::new(dl, iter::empty(), attr::ReprAny, false, ty)?, non_zero: false }, + ty::TyNever => Univariant { variant: Struct::new(dl, &vec![], attr::ReprAny, false, ty)?, non_zero: false }, // Potentially-fat pointers. ty::TyBox(pointee) | @@ -1029,12 +1027,12 @@ impl<'a, 'gcx, 'tcx> Layout { // Odd unit types. ty::TyFnDef(..) => { Univariant { - variant: Struct::new(dl, iter::empty(), attr::ReprAny, false, ty)?, + variant: Struct::new(dl, &vec![], attr::ReprAny, false, ty)?, non_zero: false } } ty::TyDynamic(_) => { - let mut unit = Struct::new(dl, iter::empty(), attr::ReprAny, false, ty)?; + let mut unit = Struct::new(dl, &vec![], attr::ReprAny, false, ty)?; unit.sized = false; Univariant { variant: unit, non_zero: false } } @@ -1043,7 +1041,8 @@ impl<'a, 'gcx, 'tcx> Layout { ty::TyClosure(def_id, ref substs) => { let tys = substs.upvar_tys(def_id, tcx); let st = Struct::new(dl, - tys.map(|ty| ty.layout(infcx)), + &tys.map(|ty| ty.layout(infcx)) + .collect::, _>>()?, attr::ReprAny, false, ty)?; Univariant { variant: st, non_zero: false } @@ -1051,7 +1050,8 @@ impl<'a, 'gcx, 'tcx> Layout { ty::TyTuple(tys) => { let st = Struct::new(dl, - tys.iter().map(|ty| ty.layout(infcx)), + &tys.iter().map(|ty| ty.layout(infcx)) + .collect::, _>>()?, attr::ReprAny, false, ty)?; Univariant { variant: st, non_zero: false } } @@ -1085,7 +1085,7 @@ impl<'a, 'gcx, 'tcx> Layout { assert_eq!(hint, attr::ReprAny); return success(Univariant { - variant: Struct::new(dl, iter::empty(), hint, false, ty)?, + variant: Struct::new(dl, &vec![], hint, false, ty)?, non_zero: false }); } @@ -1116,14 +1116,14 @@ impl<'a, 'gcx, 'tcx> Layout { let fields = def.variants[0].fields.iter().map(|field| { field.ty(tcx, substs).layout(infcx) - }); + }).collect::, _>>()?; let packed = tcx.lookup_packed(def.did); let layout = if def.is_union() { let mut un = Union::new(dl, packed); - un.extend(dl, fields, ty)?; + un.extend(dl, fields.iter().map(|&f| Ok(f)), ty)?; UntaggedUnion { variants: un } } else { - let st = Struct::new(dl, fields, hint, false, ty)?; + let st = Struct::new(dl, &fields, hint, false, ty)?; let non_zero = Some(def.did) == tcx.lang_items.non_zero(); Univariant { variant: st, non_zero: non_zero } }; @@ -1175,7 +1175,8 @@ impl<'a, 'gcx, 'tcx> Layout { } let st = Struct::new(dl, - variants[discr].iter().map(|ty| ty.layout(infcx)), + &variants[discr].iter().map(|ty| ty.layout(infcx)) + .collect::, _>>()?, hint, false, ty)?; // We have to fix the last element of path here as only we know the right value. @@ -1210,16 +1211,16 @@ impl<'a, 'gcx, 'tcx> Layout { let mut variants = variants.into_iter().map(|fields| { let mut fields = fields.into_iter().map(|field| { field.layout(infcx) - }).collect::>(); - fields.insert(0, Ok(&discr)); + }).collect::, _>>()?; + fields.insert(0, &discr); let st = Struct::new(dl, - fields.iter().cloned(), + &fields, hint, false, ty)?; // Find the first field we can't move later // to make room for a larger discriminant. // It is important to skip the first field. for i in st.field_index_by_increasing_offset().skip(1) { - let field = fields[i].unwrap(); + let field = fields[i]; let field_align = field.align(dl); if field.size(dl).bytes() != 0 || field_align.abi() != 1 { start_align = start_align.min(field_align); From 3d23dc7956ab31a2758b723213bf6235ba57ed55 Mon Sep 17 00:00:00 2001 From: Austin Hicks Date: Sun, 20 Nov 2016 19:56:51 -0500 Subject: [PATCH 09/28] Modify debuginfo to deal with the difference between source and memory order Fix gdb enum tests to ignore garbage variants, as we no longer actually know what the garbage is. --- src/librustc_trans/debuginfo/metadata.rs | 49 ++++++++++++------- src/test/debuginfo/borrowed-enum.rs | 4 +- .../by-value-non-immediate-argument.rs | 2 +- .../debuginfo/generic-struct-style-enum.rs | 6 +-- .../debuginfo/generic-tuple-style-enum.rs | 6 +-- src/test/debuginfo/struct-style-enum.rs | 6 +-- src/test/debuginfo/tuple-style-enum.rs | 6 +-- src/test/debuginfo/unique-enum.rs | 4 +- 8 files changed, 47 insertions(+), 36 deletions(-) diff --git a/src/librustc_trans/debuginfo/metadata.rs b/src/librustc_trans/debuginfo/metadata.rs index 5022e0750e38e..ad6103965986c 100644 --- a/src/librustc_trans/debuginfo/metadata.rs +++ b/src/librustc_trans/debuginfo/metadata.rs @@ -881,25 +881,28 @@ impl<'tcx> MemberDescriptionFactory<'tcx> { // Creates MemberDescriptions for the fields of a struct struct StructMemberDescriptionFactory<'tcx> { + ty: Ty<'tcx>, variant: &'tcx ty::VariantDef, substs: &'tcx Substs<'tcx>, - is_simd: bool, span: Span, } impl<'tcx> StructMemberDescriptionFactory<'tcx> { fn create_member_descriptions<'a>(&self, cx: &CrateContext<'a, 'tcx>) -> Vec { - let field_size = if self.is_simd { - let fty = monomorphize::field_ty(cx.tcx(), - self.substs, - &self.variant.fields[0]); - Some(machine::llsize_of_alloc( - cx, - type_of::type_of(cx, fty) - ) as usize) - } else { - None + let layout = cx.layout_of(self.ty); + + // The following code is slightly convoluted as to allow us to avoid allocating in the Univariant case. + // tmp exists only so we can take a reference to it in the second match arm below. + let tmp; + let offsets = match *layout { + layout::Univariant { ref variant, .. } => &variant.offsets, + layout::Vector { element, count } => { + let element_size = element.size(&cx.tcx().data_layout).bytes(); + tmp = (0..count).map(|i| layout::Size::from_bytes(i*element_size)).collect::>(); + &tmp + } + _ => bug!("{} is not a struct", self.ty) }; self.variant.fields.iter().enumerate().map(|(i, f)| { @@ -910,11 +913,7 @@ impl<'tcx> StructMemberDescriptionFactory<'tcx> { }; let fty = monomorphize::field_ty(cx.tcx(), self.substs, f); - let offset = if self.is_simd { - FixedMemberOffset { bytes: i * field_size.unwrap() } - } else { - ComputedMemberOffset - }; + let offset = FixedMemberOffset { bytes: offsets[i].bytes() as usize}; MemberDescription { name: name, @@ -956,9 +955,9 @@ fn prepare_struct_metadata<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>, struct_metadata_stub, struct_llvm_type, StructMDF(StructMemberDescriptionFactory { + ty: struct_type, variant: variant, substs: substs, - is_simd: struct_type.is_simd(), span: span, }) ) @@ -970,6 +969,7 @@ fn prepare_struct_metadata<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>, // Creates MemberDescriptions for the fields of a tuple struct TupleMemberDescriptionFactory<'tcx> { + ty: Ty<'tcx>, component_types: Vec>, span: Span, } @@ -977,6 +977,13 @@ struct TupleMemberDescriptionFactory<'tcx> { impl<'tcx> TupleMemberDescriptionFactory<'tcx> { fn create_member_descriptions<'a>(&self, cx: &CrateContext<'a, 'tcx>) -> Vec { + let layout = cx.layout_of(self.ty); + let offsets = if let layout::Univariant { ref variant, .. } = *layout { + &variant.offsets + } else { + bug!("{} is not a tuple", self.ty); + }; + self.component_types .iter() .enumerate() @@ -985,7 +992,7 @@ impl<'tcx> TupleMemberDescriptionFactory<'tcx> { name: format!("__{}", i), llvm_type: type_of::type_of(cx, component_type), type_metadata: type_metadata(cx, component_type, self.span), - offset: ComputedMemberOffset, + offset: FixedMemberOffset { bytes: offsets[i].bytes() as usize }, flags: DIFlags::FlagZero, } }).collect() @@ -1012,6 +1019,7 @@ fn prepare_tuple_metadata<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>, NO_SCOPE_METADATA), tuple_llvm_type, TupleMDF(TupleMemberDescriptionFactory { + ty: tuple_type, component_types: component_types.to_vec(), span: span, }) @@ -1300,6 +1308,8 @@ impl<'tcx> EnumMemberDescriptionFactory<'tcx> { // Creates MemberDescriptions for the fields of a single enum variant. struct VariantMemberDescriptionFactory<'tcx> { + // Cloned from the layout::Struct describing the variant. + offsets: Vec, args: Vec<(String, Ty<'tcx>)>, discriminant_type_metadata: Option, span: Span, @@ -1316,7 +1326,7 @@ impl<'tcx> VariantMemberDescriptionFactory<'tcx> { Some(metadata) if i == 0 => metadata, _ => type_metadata(cx, ty, self.span) }, - offset: ComputedMemberOffset, + offset: FixedMemberOffset { bytes: self.offsets[i].bytes() as usize }, flags: DIFlags::FlagZero } }).collect() @@ -1420,6 +1430,7 @@ fn describe_enum_variant<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>, let member_description_factory = VariantMDF(VariantMemberDescriptionFactory { + offsets: struct_def.offsets.clone(), args: args, discriminant_type_metadata: match discriminant_info { RegularDiscriminant(discriminant_type_metadata) => { diff --git a/src/test/debuginfo/borrowed-enum.rs b/src/test/debuginfo/borrowed-enum.rs index ddc29c6430270..f34fc3b20d2be 100644 --- a/src/test/debuginfo/borrowed-enum.rs +++ b/src/test/debuginfo/borrowed-enum.rs @@ -18,11 +18,11 @@ // gdb-command:run // gdb-command:print *the_a_ref -// gdbg-check:$1 = {{RUST$ENUM$DISR = TheA, x = 0, y = 8970181431921507452}, {RUST$ENUM$DISR = TheA, __0 = 0, __1 = 2088533116, __2 = 2088533116}} +// gdbg-check:$1 = {{RUST$ENUM$DISR = TheA, x = 0, y = 8970181431921507452}, {RUST$ENUM$DISR = TheA, [...]}} // gdbr-check:$1 = borrowed_enum::ABC::TheA{x: 0, y: 8970181431921507452} // gdb-command:print *the_b_ref -// gdbg-check:$2 = {{RUST$ENUM$DISR = TheB, x = 0, y = 1229782938247303441}, {RUST$ENUM$DISR = TheB, __0 = 0, __1 = 286331153, __2 = 286331153}} +// gdbg-check:$2 = {{RUST$ENUM$DISR = TheB, [...]}, {RUST$ENUM$DISR = TheB, __0 = 0, __1 = 286331153, __2 = 286331153}} // gdbr-check:$2 = borrowed_enum::ABC::TheB(0, 286331153, 286331153) // gdb-command:print *univariant_ref diff --git a/src/test/debuginfo/by-value-non-immediate-argument.rs b/src/test/debuginfo/by-value-non-immediate-argument.rs index 6d821dbc155d0..0fe08c3a22731 100644 --- a/src/test/debuginfo/by-value-non-immediate-argument.rs +++ b/src/test/debuginfo/by-value-non-immediate-argument.rs @@ -42,7 +42,7 @@ // gdb-command:continue // gdb-command:print x -// gdbg-check:$7 = {{RUST$ENUM$DISR = Case1, x = 0, y = 8970181431921507452}, {RUST$ENUM$DISR = Case1, __0 = 0, __1 = 2088533116, __2 = 2088533116}} +// gdbg-check:$7 = {{RUST$ENUM$DISR = Case1, x = 0, y = 8970181431921507452}, {RUST$ENUM$DISR = Case1, [...]}} // gdbr-check:$7 = by_value_non_immediate_argument::Enum::Case1{x: 0, y: 8970181431921507452} // gdb-command:continue diff --git a/src/test/debuginfo/generic-struct-style-enum.rs b/src/test/debuginfo/generic-struct-style-enum.rs index dba9422721a96..a328eec689394 100644 --- a/src/test/debuginfo/generic-struct-style-enum.rs +++ b/src/test/debuginfo/generic-struct-style-enum.rs @@ -17,15 +17,15 @@ // gdb-command:run // gdb-command:print case1 -// gdbg-check:$1 = {{RUST$ENUM$DISR = Case1, a = 0, b = 31868, c = 31868, d = 31868, e = 31868}, {RUST$ENUM$DISR = Case1, a = 0, b = 2088533116, c = 2088533116}, {RUST$ENUM$DISR = Case1, a = 0, b = 8970181431921507452}} +// gdbg-check:$1 = {{RUST$ENUM$DISR = Case1, a = 0, b = 31868, c = 31868, d = 31868, e = 31868}, {RUST$ENUM$DISR = Case1, [...]}, {RUST$ENUM$DISR = Case1, [...]}} // gdbr-check:$1 = generic_struct_style_enum::Regular::Case1{a: 0, b: 31868, c: 31868, d: 31868, e: 31868} // gdb-command:print case2 -// gdbg-check:$2 = {{RUST$ENUM$DISR = Case2, a = 0, b = 4369, c = 4369, d = 4369, e = 4369}, {RUST$ENUM$DISR = Case2, a = 0, b = 286331153, c = 286331153}, {RUST$ENUM$DISR = Case2, a = 0, b = 1229782938247303441}} +// gdbg-check:$2 = {{RUST$ENUM$DISR = Case2, [...]}, {RUST$ENUM$DISR = Case2, a = 0, b = 286331153, c = 286331153}, {RUST$ENUM$DISR = Case2, [...]}} // gdbr-check:$2 = generic_struct_style_enum::Regular::Case2{a: 0, b: 286331153, c: 286331153} // gdb-command:print case3 -// gdbg-check:$3 = {{RUST$ENUM$DISR = Case3, a = 0, b = 22873, c = 22873, d = 22873, e = 22873}, {RUST$ENUM$DISR = Case3, a = 0, b = 1499027801, c = 1499027801}, {RUST$ENUM$DISR = Case3, a = 0, b = 6438275382588823897}} +// gdbg-check:$3 = {{RUST$ENUM$DISR = Case3, [...]}, {RUST$ENUM$DISR = Case3, [...]}, {RUST$ENUM$DISR = Case3, a = 0, b = 6438275382588823897}} // gdbr-check:$3 = generic_struct_style_enum::Regular::Case3{a: 0, b: 6438275382588823897} // gdb-command:print univariant diff --git a/src/test/debuginfo/generic-tuple-style-enum.rs b/src/test/debuginfo/generic-tuple-style-enum.rs index 01d2ff4e33439..9ada5fdeff788 100644 --- a/src/test/debuginfo/generic-tuple-style-enum.rs +++ b/src/test/debuginfo/generic-tuple-style-enum.rs @@ -19,15 +19,15 @@ // gdb-command:run // gdb-command:print case1 -// gdbg-check:$1 = {{RUST$ENUM$DISR = Case1, __0 = 0, __1 = 31868, __2 = 31868, __3 = 31868, __4 = 31868}, {RUST$ENUM$DISR = Case1, __0 = 0, __1 = 2088533116, __2 = 2088533116}, {RUST$ENUM$DISR = Case1, __0 = 0, __1 = 8970181431921507452}} +// gdbg-check:$1 = {{RUST$ENUM$DISR = Case1, __0 = 0, __1 = 31868, __2 = 31868, __3 = 31868, __4 = 31868}, {RUST$ENUM$DISR = Case1, [...]}, {RUST$ENUM$DISR = Case1, [...]}} // gdbr-check:$1 = generic_tuple_style_enum::Regular::Case1(0, 31868, 31868, 31868, 31868) // gdb-command:print case2 -// gdbg-check:$2 = {{RUST$ENUM$DISR = Case2, __0 = 0, __1 = 4369, __2 = 4369, __3 = 4369, __4 = 4369}, {RUST$ENUM$DISR = Case2, __0 = 0, __1 = 286331153, __2 = 286331153}, {RUST$ENUM$DISR = Case2, __0 = 0, __1 = 1229782938247303441}} +// gdbg-check:$2 = {{RUST$ENUM$DISR = Case2, [...]}, {RUST$ENUM$DISR = Case2, __0 = 0, __1 = 286331153, __2 = 286331153}, {RUST$ENUM$DISR = Case2, [...]}} // gdbr-check:$2 = generic_tuple_style_enum::Regular::Case2(0, 286331153, 286331153) // gdb-command:print case3 -// gdbg-check:$3 = {{RUST$ENUM$DISR = Case3, __0 = 0, __1 = 22873, __2 = 22873, __3 = 22873, __4 = 22873}, {RUST$ENUM$DISR = Case3, __0 = 0, __1 = 1499027801, __2 = 1499027801}, {RUST$ENUM$DISR = Case3, __0 = 0, __1 = 6438275382588823897}} +// gdbg-check:$3 = {{RUST$ENUM$DISR = Case3, [...]}, {RUST$ENUM$DISR = Case3, [...]}, {RUST$ENUM$DISR = Case3, __0 = 0, __1 = 6438275382588823897}} // gdbr-check:$3 = generic_tuple_style_enum::Regular::Case3(0, 6438275382588823897) // gdb-command:print univariant diff --git a/src/test/debuginfo/struct-style-enum.rs b/src/test/debuginfo/struct-style-enum.rs index 8abc139eb1172..b6196daaa4656 100644 --- a/src/test/debuginfo/struct-style-enum.rs +++ b/src/test/debuginfo/struct-style-enum.rs @@ -19,15 +19,15 @@ // gdb-command:run // gdb-command:print case1 -// gdbg-check:$1 = {{RUST$ENUM$DISR = Case1, a = 0, b = 31868, c = 31868, d = 31868, e = 31868}, {RUST$ENUM$DISR = Case1, a = 0, b = 2088533116, c = 2088533116}, {RUST$ENUM$DISR = Case1, a = 0, b = 8970181431921507452}} +// gdbg-check:$1 = {{RUST$ENUM$DISR = Case1, a = 0, b = 31868, c = 31868, d = 31868, e = 31868}, {RUST$ENUM$DISR = Case1, [...]}, {RUST$ENUM$DISR = Case1, [...]}} // gdbr-check:$1 = struct_style_enum::Regular::Case1{a: 0, b: 31868, c: 31868, d: 31868, e: 31868} // gdb-command:print case2 -// gdbg-check:$2 = {{RUST$ENUM$DISR = Case2, a = 0, b = 4369, c = 4369, d = 4369, e = 4369}, {RUST$ENUM$DISR = Case2, a = 0, b = 286331153, c = 286331153}, {RUST$ENUM$DISR = Case2, a = 0, b = 1229782938247303441}} +// gdbg-check:$2 = {{RUST$ENUM$DISR = Case2, [...]}, {RUST$ENUM$DISR = Case2, a = 0, b = 286331153, c = 286331153}, {RUST$ENUM$DISR = Case2, [...]}} // gdbr-check:$2 = struct_style_enum::Regular::Case2{a: 0, b: 286331153, c: 286331153} // gdb-command:print case3 -// gdbg-check:$3 = {{RUST$ENUM$DISR = Case3, a = 0, b = 22873, c = 22873, d = 22873, e = 22873}, {RUST$ENUM$DISR = Case3, a = 0, b = 1499027801, c = 1499027801}, {RUST$ENUM$DISR = Case3, a = 0, b = 6438275382588823897}} +// gdbg-check:$3 = {{RUST$ENUM$DISR = Case3, [...]}, {RUST$ENUM$DISR = Case3, [...]}, {RUST$ENUM$DISR = Case3, a = 0, b = 6438275382588823897}} // gdbr-check:$3 = struct_style_enum::Regular::Case3{a: 0, b: 6438275382588823897} // gdb-command:print univariant diff --git a/src/test/debuginfo/tuple-style-enum.rs b/src/test/debuginfo/tuple-style-enum.rs index d05edec3e737d..988f223b3bc4f 100644 --- a/src/test/debuginfo/tuple-style-enum.rs +++ b/src/test/debuginfo/tuple-style-enum.rs @@ -19,15 +19,15 @@ // gdb-command:run // gdb-command:print case1 -// gdbg-check:$1 = {{RUST$ENUM$DISR = Case1, __0 = 0, __1 = 31868, __2 = 31868, __3 = 31868, __4 = 31868}, {RUST$ENUM$DISR = Case1, __0 = 0, __1 = 2088533116, __2 = 2088533116}, {RUST$ENUM$DISR = Case1, __0 = 0, __1 = 8970181431921507452}} +// gdbg-check:$1 = {{RUST$ENUM$DISR = Case1, __0 = 0, __1 = 31868, __2 = 31868, __3 = 31868, __4 = 31868}, {RUST$ENUM$DISR = Case1, [...]}, {RUST$ENUM$DISR = Case1, [...]}} // gdbr-check:$1 = tuple_style_enum::Regular::Case1(0, 31868, 31868, 31868, 31868) // gdb-command:print case2 -// gdbg-check:$2 = {{RUST$ENUM$DISR = Case2, __0 = 0, __1 = 4369, __2 = 4369, __3 = 4369, __4 = 4369}, {RUST$ENUM$DISR = Case2, __0 = 0, __1 = 286331153, __2 = 286331153}, {RUST$ENUM$DISR = Case2, __0 = 0, __1 = 1229782938247303441}} +// gdbg-check:$2 = {{RUST$ENUM$DISR = Case2, [...]}, {RUST$ENUM$DISR = Case2, __0 = 0, __1 = 286331153, __2 = 286331153}, {RUST$ENUM$DISR = Case2, [...]}} // gdbr-check:$2 = tuple_style_enum::Regular::Case2(0, 286331153, 286331153) // gdb-command:print case3 -// gdbg-check:$3 = {{RUST$ENUM$DISR = Case3, __0 = 0, __1 = 22873, __2 = 22873, __3 = 22873, __4 = 22873}, {RUST$ENUM$DISR = Case3, __0 = 0, __1 = 1499027801, __2 = 1499027801}, {RUST$ENUM$DISR = Case3, __0 = 0, __1 = 6438275382588823897}} +// gdbg-check:$3 = {{RUST$ENUM$DISR = Case3, [...]}, {RUST$ENUM$DISR = Case3, [...]}, {RUST$ENUM$DISR = Case3, __0 = 0, __1 = 6438275382588823897}} // gdbr-check:$3 = tuple_style_enum::Regular::Case3(0, 6438275382588823897) // gdb-command:print univariant diff --git a/src/test/debuginfo/unique-enum.rs b/src/test/debuginfo/unique-enum.rs index e882544b802bd..cf8d90e30f169 100644 --- a/src/test/debuginfo/unique-enum.rs +++ b/src/test/debuginfo/unique-enum.rs @@ -18,11 +18,11 @@ // gdb-command:run // gdb-command:print *the_a -// gdbg-check:$1 = {{RUST$ENUM$DISR = TheA, x = 0, y = 8970181431921507452}, {RUST$ENUM$DISR = TheA, __0 = 0, __1 = 2088533116, __2 = 2088533116}} +// gdbg-check:$1 = {{RUST$ENUM$DISR = TheA, x = 0, y = 8970181431921507452}, {RUST$ENUM$DISR = TheA, [...]}} // gdbr-check:$1 = unique_enum::ABC::TheA{x: 0, y: 8970181431921507452} // gdb-command:print *the_b -// gdbg-check:$2 = {{RUST$ENUM$DISR = TheB, x = 0, y = 1229782938247303441}, {RUST$ENUM$DISR = TheB, __0 = 0, __1 = 286331153, __2 = 286331153}} +// gdbg-check:$2 = {{RUST$ENUM$DISR = TheB, [...]}, {RUST$ENUM$DISR = TheB, __0 = 0, __1 = 286331153, __2 = 286331153}} // gdbr-check:$2 = unique_enum::ABC::TheB(0, 286331153, 286331153) // gdb-command:print *univariant From adae9bc25ecb25b3d0ac03591438bfde72f98291 Mon Sep 17 00:00:00 2001 From: Austin Hicks Date: Mon, 21 Nov 2016 14:23:15 -0500 Subject: [PATCH 10/28] Make tidy --- src/librustc/ty/layout.rs | 25 +++++++++++++++++------- src/librustc_lint/types.rs | 3 ++- src/librustc_trans/debuginfo/metadata.rs | 8 ++++---- src/test/run-pass/enum-size-variance.rs | 4 ++-- 4 files changed, 26 insertions(+), 14 deletions(-) diff --git a/src/librustc/ty/layout.rs b/src/librustc/ty/layout.rs index bcbafc8a28a5e..ebda879e9086f 100644 --- a/src/librustc/ty/layout.rs +++ b/src/librustc/ty/layout.rs @@ -538,7 +538,10 @@ impl<'a, 'gcx, 'tcx> Struct { min_size: Size::from_bytes(0), }; - if is_enum_variant { assert!(fields.len() >= 1, "Enum variants must have at least a discriminant field.") } + if is_enum_variant { + assert!(fields.len() >= 1, "Enum variants must have at least a discriminant field.") + } + if fields.len() == 0 {return Ok(ret)}; ret.offsets = vec![Size::from_bytes(0); fields.len()]; @@ -546,7 +549,8 @@ impl<'a, 'gcx, 'tcx> Struct { if repr == attr::ReprAny { let start = if is_enum_variant {1} else {0}; - // FIXME(camlorn): we can't reorder the last field because it is possible for structs to be coerced to unsized. + // FIXME(camlorn): we can't reorder the last field because + // it is possible for structs to be coerced to unsized. // Example: struct Foo { x: i32, y: T } // We can coerce &Foo to &Foo. let end = inverse_memory_index.len()-1; @@ -554,12 +558,16 @@ impl<'a, 'gcx, 'tcx> Struct { let optimizing = &mut inverse_memory_index[start..end]; optimizing.sort_by_key(|&x| fields[x as usize].align(dl).abi()); } - if is_enum_variant { assert_eq!(inverse_memory_index[0], 0, "Enums must have field 0 as the field with lowest offset.") } + if is_enum_variant { + assert_eq!(inverse_memory_index[0], 0, + "Enums must have field 0 as the field with lowest offset.") + } } - + // At this point, inverse_memory_index holds field indices by increasing offset. // That is, if field 5 has offset 0, the first element of inverse_memory_index is 5. - // We now write field offsets to the corresponding offset slot; field 5 with offset 0 puts 0 in offsets[5]. + // We now write field offsets to the corresponding offset slot; + // field 5 with offset 0 puts 0 in offsets[5]. // At the bottom of this function, we use inverse_memory_index to produce memory_index. let mut offset = Size::from_bytes(0); @@ -976,7 +984,10 @@ impl<'a, 'gcx, 'tcx> Layout { ty::TyFnPtr(_) => Scalar { value: Pointer, non_zero: true }, // The never type. - ty::TyNever => Univariant { variant: Struct::new(dl, &vec![], attr::ReprAny, false, ty)?, non_zero: false }, + ty::TyNever => Univariant { + variant: Struct::new(dl, &vec![], attr::ReprAny, false, ty)?, + non_zero: false + }, // Potentially-fat pointers. ty::TyBox(pointee) | @@ -1179,7 +1190,7 @@ impl<'a, 'gcx, 'tcx> Layout { .collect::, _>>()?, hint, false, ty)?; - // We have to fix the last element of path here as only we know the right value. + // We have to fix the last element of path here. let mut i = *path.last().unwrap(); i = st.memory_index[i as usize]; *path.last_mut().unwrap() = i; diff --git a/src/librustc_lint/types.rs b/src/librustc_lint/types.rs index 49bbcc18efb28..751c9c3440f66 100644 --- a/src/librustc_lint/types.rs +++ b/src/librustc_lint/types.rs @@ -750,7 +750,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for VariantSizeDifferences { if let Layout::General { ref variants, ref size, discr, .. } = *layout { let discr_size = Primitive::Int(discr).size(&cx.tcx.data_layout).bytes(); - debug!("enum `{}` is {} bytes large with layout:\n{:#?}", t, size.bytes(), layout); + debug!("enum `{}` is {} bytes large with layout:\n{:#?}", + t, size.bytes(), layout); let (largest, slargest, largest_index) = enum_definition.variants .iter() diff --git a/src/librustc_trans/debuginfo/metadata.rs b/src/librustc_trans/debuginfo/metadata.rs index ad6103965986c..f6b5bde6ebb44 100644 --- a/src/librustc_trans/debuginfo/metadata.rs +++ b/src/librustc_trans/debuginfo/metadata.rs @@ -891,15 +891,15 @@ impl<'tcx> StructMemberDescriptionFactory<'tcx> { fn create_member_descriptions<'a>(&self, cx: &CrateContext<'a, 'tcx>) -> Vec { let layout = cx.layout_of(self.ty); - - // The following code is slightly convoluted as to allow us to avoid allocating in the Univariant case. - // tmp exists only so we can take a reference to it in the second match arm below. + let tmp; let offsets = match *layout { layout::Univariant { ref variant, .. } => &variant.offsets, layout::Vector { element, count } => { let element_size = element.size(&cx.tcx().data_layout).bytes(); - tmp = (0..count).map(|i| layout::Size::from_bytes(i*element_size)).collect::>(); + tmp = (0..count). + map(|i| layout::Size::from_bytes(i*element_size)) + .collect::>(); &tmp } _ => bug!("{} is not a struct", self.ty) diff --git a/src/test/run-pass/enum-size-variance.rs b/src/test/run-pass/enum-size-variance.rs index beccbf5eb0394..a3e95a153418d 100644 --- a/src/test/run-pass/enum-size-variance.rs +++ b/src/test/run-pass/enum-size-variance.rs @@ -11,8 +11,8 @@ #![warn(variant_size_differences)] #![allow(dead_code)] -// Note that the following test only works because all fields of the enum variants are of the same size. -// If this test is modified so that the reordering logic in librustc/ty/layout.rs kicks in, it will fail. +// Note that the following test works because all fields of the enum variants are of the same size. +// If this test is modified and the reordering logic in librustc/ty/layout.rs kicks in, it fails. enum Enum1 { } From e7c3540706af276f6770d8a2fabecde38c33ce98 Mon Sep 17 00:00:00 2001 From: Austin Hicks Date: Mon, 21 Nov 2016 17:46:14 -0500 Subject: [PATCH 11/28] Use an enum to differentiate between kinds of structs. --- src/librustc/ty/layout.rs | 91 ++++++++++++++++++++++++++------------- 1 file changed, 61 insertions(+), 30 deletions(-) diff --git a/src/librustc/ty/layout.rs b/src/librustc/ty/layout.rs index ebda879e9086f..bc9d32535f2d6 100644 --- a/src/librustc/ty/layout.rs +++ b/src/librustc/ty/layout.rs @@ -524,9 +524,20 @@ pub struct Struct { pub min_size: Size, } +// Info required to optimize struct layout. +#[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Debug)] +enum StructKind { + // A tuple, closure, or univariant which cannot be coerced to unsized. + AlwaysSizedUnivariant, + // A univariant, the last field of which may be coerced to unsized. + MaybeUnsizedUnivariant, + // A univariant, but part of an enum. + EnumVariant, +} + impl<'a, 'gcx, 'tcx> Struct { - pub fn new(dl: &TargetDataLayout, fields: &Vec<&'a Layout>, - repr: attr::ReprAttr, is_enum_variant: bool, + fn new(dl: &TargetDataLayout, fields: &Vec<&'a Layout>, + repr: attr::ReprAttr, kind: StructKind, scapegoat: Ty<'gcx>) -> Result> { let packed = repr == attr::ReprPacked; let mut ret = Struct { @@ -538,38 +549,48 @@ impl<'a, 'gcx, 'tcx> Struct { min_size: Size::from_bytes(0), }; - if is_enum_variant { - assert!(fields.len() >= 1, "Enum variants must have at least a discriminant field.") - } + let (optimize, sort_ascending) = match (repr, kind) { + (attr::ReprAny, StructKind::AlwaysSizedUnivariant) => (true, false), + (attr::ReprAny, StructKind::MaybeUnsizedUnivariant) => (true, true), + (attr::ReprAny, StructKind::EnumVariant) => { + assert!(fields.len() >= 1, "Enum variants must have discriminants."); + (true, fields[0].size(dl).bytes() == 1) + } + _ => (false, false) + }; if fields.len() == 0 {return Ok(ret)}; ret.offsets = vec![Size::from_bytes(0); fields.len()]; let mut inverse_memory_index: Vec = (0..fields.len() as u32).collect(); - if repr == attr::ReprAny { - let start = if is_enum_variant {1} else {0}; - // FIXME(camlorn): we can't reorder the last field because - // it is possible for structs to be coerced to unsized. - // Example: struct Foo { x: i32, y: T } - // We can coerce &Foo to &Foo. - let end = inverse_memory_index.len()-1; + if optimize { + let start = if let StructKind::EnumVariant = kind {1} else {0}; + let end = if let StructKind::MaybeUnsizedUnivariant = kind { fields.len()-1 } else { 0 }; if end > start { let optimizing = &mut inverse_memory_index[start..end]; - optimizing.sort_by_key(|&x| fields[x as usize].align(dl).abi()); - } - if is_enum_variant { - assert_eq!(inverse_memory_index[0], 0, - "Enums must have field 0 as the field with lowest offset.") + if sort_ascending { + optimizing.sort_by_key(|&x| fields[x as usize].align(dl).abi()); + } else { + optimizing.sort_by(| &a, &b | { + let a = fields[a as usize].align(dl).abi(); + let b = fields[b as usize].align(dl).abi(); + b.cmp(&a) + }); + } } } - // At this point, inverse_memory_index holds field indices by increasing offset. // That is, if field 5 has offset 0, the first element of inverse_memory_index is 5. // We now write field offsets to the corresponding offset slot; // field 5 with offset 0 puts 0 in offsets[5]. // At the bottom of this function, we use inverse_memory_index to produce memory_index. + if let StructKind::EnumVariant = kind { + assert_eq!(inverse_memory_index[0], 0, + "Enum variant discriminants must have the lowest offset."); + } + let mut offset = Size::from_bytes(0); for i in inverse_memory_index.iter() { @@ -606,10 +627,16 @@ impl<'a, 'gcx, 'tcx> Struct { // To invert it, consider: // If field 5 has offset 0, offsets[0] is 5, and memory_index[5] should be 0. // Field 5 would be the first element, so memory_index is i: - ret.memory_index = vec![0; inverse_memory_index.len()]; + // Note: if we didn't optimize, it's already right. + + if optimize { + ret.memory_index = vec![0; inverse_memory_index.len()]; - for i in 0..inverse_memory_index.len() { - ret.memory_index[inverse_memory_index[i] as usize] = i as u32; + for i in 0..inverse_memory_index.len() { + ret.memory_index[inverse_memory_index[i] as usize] = i as u32; + } + } else { + ret.memory_index = inverse_memory_index; } Ok(ret) @@ -985,7 +1012,8 @@ impl<'a, 'gcx, 'tcx> Layout { // The never type. ty::TyNever => Univariant { - variant: Struct::new(dl, &vec![], attr::ReprAny, false, ty)?, + variant: Struct::new(dl, &vec![], attr::ReprAny, + StructKind::AlwaysSizedUnivariant, ty)?, non_zero: false }, @@ -1038,12 +1066,13 @@ impl<'a, 'gcx, 'tcx> Layout { // Odd unit types. ty::TyFnDef(..) => { Univariant { - variant: Struct::new(dl, &vec![], attr::ReprAny, false, ty)?, + variant: Struct::new(dl, &vec![], attr::ReprAny, StructKind::AlwaysSizedUnivariant, ty)?, non_zero: false } } ty::TyDynamic(_) => { - let mut unit = Struct::new(dl, &vec![], attr::ReprAny, false, ty)?; + let mut unit = Struct::new(dl, &vec![], attr::ReprAny, + StructKind::AlwaysSizedUnivariant, ty)?; unit.sized = false; Univariant { variant: unit, non_zero: false } } @@ -1055,15 +1084,16 @@ impl<'a, 'gcx, 'tcx> Layout { &tys.map(|ty| ty.layout(infcx)) .collect::, _>>()?, attr::ReprAny, - false, ty)?; + StructKind::AlwaysSizedUnivariant, ty)?; Univariant { variant: st, non_zero: false } } ty::TyTuple(tys) => { + // FIXME(camlorn): if we ever allow unsized tuples, this needs to be checked in the same way it is for univariant. let st = Struct::new(dl, &tys.iter().map(|ty| ty.layout(infcx)) .collect::, _>>()?, - attr::ReprAny, false, ty)?; + attr::ReprAny, StructKind::AlwaysSizedUnivariant, ty)?; Univariant { variant: st, non_zero: false } } @@ -1096,7 +1126,7 @@ impl<'a, 'gcx, 'tcx> Layout { assert_eq!(hint, attr::ReprAny); return success(Univariant { - variant: Struct::new(dl, &vec![], hint, false, ty)?, + variant: Struct::new(dl, &vec![], hint, StructKind::AlwaysSizedUnivariant, ty)?, non_zero: false }); } @@ -1134,7 +1164,8 @@ impl<'a, 'gcx, 'tcx> Layout { un.extend(dl, fields.iter().map(|&f| Ok(f)), ty)?; UntaggedUnion { variants: un } } else { - let st = Struct::new(dl, &fields, hint, false, ty)?; + let st = Struct::new(dl, &fields, hint, + StructKind::MaybeUnsizedUnivariant, ty)?; let non_zero = Some(def.did) == tcx.lang_items.non_zero(); Univariant { variant: st, non_zero: non_zero } }; @@ -1188,7 +1219,7 @@ impl<'a, 'gcx, 'tcx> Layout { let st = Struct::new(dl, &variants[discr].iter().map(|ty| ty.layout(infcx)) .collect::, _>>()?, - hint, false, ty)?; + hint, StructKind::AlwaysSizedUnivariant, ty)?; // We have to fix the last element of path here. let mut i = *path.last().unwrap(); @@ -1226,7 +1257,7 @@ impl<'a, 'gcx, 'tcx> Layout { fields.insert(0, &discr); let st = Struct::new(dl, &fields, - hint, false, ty)?; + hint, StructKind::EnumVariant, ty)?; // Find the first field we can't move later // to make room for a larger discriminant. // It is important to skip the first field. From 1af8e146f117efe7cfc1cdf7136068747d3b552c Mon Sep 17 00:00:00 2001 From: Austin Hicks Date: Mon, 21 Nov 2016 19:26:56 -0500 Subject: [PATCH 12/28] First attempt at detecting if structs can ever be unsized --- src/librustc/ty/layout.rs | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/librustc/ty/layout.rs b/src/librustc/ty/layout.rs index bc9d32535f2d6..87f6d6fc6e1ae 100644 --- a/src/librustc/ty/layout.rs +++ b/src/librustc/ty/layout.rs @@ -566,7 +566,7 @@ impl<'a, 'gcx, 'tcx> Struct { if optimize { let start = if let StructKind::EnumVariant = kind {1} else {0}; - let end = if let StructKind::MaybeUnsizedUnivariant = kind { fields.len()-1 } else { 0 }; + let end = if let StructKind::MaybeUnsizedUnivariant = kind { fields.len()-1 } else { fields.len() }; if end > start { let optimizing = &mut inverse_memory_index[start..end]; if sort_ascending { @@ -1155,6 +1155,18 @@ impl<'a, 'gcx, 'tcx> Layout { // Struct, or union, or univariant enum equivalent to a struct. // (Typechecking will reject discriminant-sizing attrs.) + let kind = if def.is_enum() || def.variants[0].fields.len() == 0{ + StructKind::AlwaysSizedUnivariant + } else { + use middle::region::ROOT_CODE_EXTENT; + let param_env = tcx.construct_parameter_environment(DUMMY_SP, def.did, ROOT_CODE_EXTENT); + let fields = &def.variants[0].fields; + let last_field = &fields[fields.len()-1]; + let always_sized = last_field.unsubst_ty().is_sized(tcx, ¶m_env, DUMMY_SP); + if !always_sized { StructKind::MaybeUnsizedUnivariant } + else { StructKind::AlwaysSizedUnivariant } + }; + let fields = def.variants[0].fields.iter().map(|field| { field.ty(tcx, substs).layout(infcx) }).collect::, _>>()?; @@ -1165,7 +1177,7 @@ impl<'a, 'gcx, 'tcx> Layout { UntaggedUnion { variants: un } } else { let st = Struct::new(dl, &fields, hint, - StructKind::MaybeUnsizedUnivariant, ty)?; + kind, ty)?; let non_zero = Some(def.did) == tcx.lang_items.non_zero(); Univariant { variant: st, non_zero: non_zero } }; From cb21cc5607557538ffff8be2213e522feb527080 Mon Sep 17 00:00:00 2001 From: Austin Hicks Date: Mon, 21 Nov 2016 20:38:47 -0500 Subject: [PATCH 13/28] Don't optimize pairs --- src/librustc/ty/layout.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/librustc/ty/layout.rs b/src/librustc/ty/layout.rs index 87f6d6fc6e1ae..6d7625bf15d41 100644 --- a/src/librustc/ty/layout.rs +++ b/src/librustc/ty/layout.rs @@ -549,9 +549,14 @@ impl<'a, 'gcx, 'tcx> Struct { min_size: Size::from_bytes(0), }; + // 1-member and 2-member structs don't optimize. + // In addition, large bodies of code in trans assume that 2-element structs can become pairs. + // It's easier to just short-circuit here. + let can_optimize_struct = fields.len() > 2; + let (optimize, sort_ascending) = match (repr, kind) { - (attr::ReprAny, StructKind::AlwaysSizedUnivariant) => (true, false), - (attr::ReprAny, StructKind::MaybeUnsizedUnivariant) => (true, true), + (attr::ReprAny, StructKind::AlwaysSizedUnivariant) => (can_optimize_struct, false), + (attr::ReprAny, StructKind::MaybeUnsizedUnivariant) => (can_optimize_struct, true), (attr::ReprAny, StructKind::EnumVariant) => { assert!(fields.len() >= 1, "Enum variants must have discriminants."); (true, fields[0].size(dl).bytes() == 1) From b3c285fa25d0ae1c1d439fe5ff6c097b8bdbc8ad Mon Sep 17 00:00:00 2001 From: Austin Hicks Date: Tue, 22 Nov 2016 14:10:52 -0500 Subject: [PATCH 14/28] Fix checking to see if the last field of a struct can be unsized. --- src/librustc/ty/layout.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc/ty/layout.rs b/src/librustc/ty/layout.rs index 6d7625bf15d41..0210b77dba06d 100644 --- a/src/librustc/ty/layout.rs +++ b/src/librustc/ty/layout.rs @@ -1167,7 +1167,7 @@ impl<'a, 'gcx, 'tcx> Layout { let param_env = tcx.construct_parameter_environment(DUMMY_SP, def.did, ROOT_CODE_EXTENT); let fields = &def.variants[0].fields; let last_field = &fields[fields.len()-1]; - let always_sized = last_field.unsubst_ty().is_sized(tcx, ¶m_env, DUMMY_SP); + let always_sized = last_field.ty(tcx, param_env.free_substs).is_sized(tcx, ¶m_env, DUMMY_SP); if !always_sized { StructKind::MaybeUnsizedUnivariant } else { StructKind::AlwaysSizedUnivariant } }; From 487ef589d33bef46dad8ecd5e2c1cd1ef22ceffd Mon Sep 17 00:00:00 2001 From: Austin Hicks Date: Tue, 22 Nov 2016 14:53:42 -0500 Subject: [PATCH 15/28] Fix type-sizes test --- src/test/run-pass/type-sizes.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/run-pass/type-sizes.rs b/src/test/run-pass/type-sizes.rs index 86159ce340b4d..8c58ec3e526c6 100644 --- a/src/test/run-pass/type-sizes.rs +++ b/src/test/run-pass/type-sizes.rs @@ -52,5 +52,5 @@ pub fn main() { assert_eq!(size_of::(), 8 as usize); assert_eq!(size_of::(), 8 as usize); - assert_eq!(size_of::(), 4 as usize); + assert_eq!(size_of::(), 2 as usize); } From 5adf6943a7c3fadf42688d63204e40dff7326095 Mon Sep 17 00:00:00 2001 From: Austin Hicks Date: Tue, 22 Nov 2016 15:03:55 -0500 Subject: [PATCH 16/28] Make tidy --- src/librustc/ty/layout.rs | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/src/librustc/ty/layout.rs b/src/librustc/ty/layout.rs index 0210b77dba06d..93a9e1e700a5d 100644 --- a/src/librustc/ty/layout.rs +++ b/src/librustc/ty/layout.rs @@ -550,7 +550,7 @@ impl<'a, 'gcx, 'tcx> Struct { }; // 1-member and 2-member structs don't optimize. - // In addition, large bodies of code in trans assume that 2-element structs can become pairs. + // In addition, code in trans assume that 2-element structs can become pairs. // It's easier to just short-circuit here. let can_optimize_struct = fields.len() > 2; @@ -571,7 +571,11 @@ impl<'a, 'gcx, 'tcx> Struct { if optimize { let start = if let StructKind::EnumVariant = kind {1} else {0}; - let end = if let StructKind::MaybeUnsizedUnivariant = kind { fields.len()-1 } else { fields.len() }; + let end = if let StructKind::MaybeUnsizedUnivariant = kind { + fields.len()-1 + } else { + fields.len() + }; if end > start { let optimizing = &mut inverse_memory_index[start..end]; if sort_ascending { @@ -633,7 +637,7 @@ impl<'a, 'gcx, 'tcx> Struct { // If field 5 has offset 0, offsets[0] is 5, and memory_index[5] should be 0. // Field 5 would be the first element, so memory_index is i: // Note: if we didn't optimize, it's already right. - + if optimize { ret.memory_index = vec![0; inverse_memory_index.len()]; @@ -1071,7 +1075,8 @@ impl<'a, 'gcx, 'tcx> Layout { // Odd unit types. ty::TyFnDef(..) => { Univariant { - variant: Struct::new(dl, &vec![], attr::ReprAny, StructKind::AlwaysSizedUnivariant, ty)?, + variant: Struct::new(dl, &vec![], + attr::ReprAny, StructKind::AlwaysSizedUnivariant, ty)?, non_zero: false } } @@ -1094,7 +1099,8 @@ impl<'a, 'gcx, 'tcx> Layout { } ty::TyTuple(tys) => { - // FIXME(camlorn): if we ever allow unsized tuples, this needs to be checked in the same way it is for univariant. + // FIXME(camlorn): if we ever allow unsized tuples, this needs to be checked. + // See the univariant case below to learn how. let st = Struct::new(dl, &tys.iter().map(|ty| ty.layout(infcx)) .collect::, _>>()?, @@ -1131,7 +1137,8 @@ impl<'a, 'gcx, 'tcx> Layout { assert_eq!(hint, attr::ReprAny); return success(Univariant { - variant: Struct::new(dl, &vec![], hint, StructKind::AlwaysSizedUnivariant, ty)?, + variant: Struct::new(dl, &vec![], + hint, StructKind::AlwaysSizedUnivariant, ty)?, non_zero: false }); } @@ -1164,10 +1171,12 @@ impl<'a, 'gcx, 'tcx> Layout { StructKind::AlwaysSizedUnivariant } else { use middle::region::ROOT_CODE_EXTENT; - let param_env = tcx.construct_parameter_environment(DUMMY_SP, def.did, ROOT_CODE_EXTENT); + let param_env = tcx.construct_parameter_environment(DUMMY_SP, + def.did, ROOT_CODE_EXTENT); let fields = &def.variants[0].fields; let last_field = &fields[fields.len()-1]; - let always_sized = last_field.ty(tcx, param_env.free_substs).is_sized(tcx, ¶m_env, DUMMY_SP); + let always_sized = last_field.ty(tcx, param_env.free_substs) + .is_sized(tcx, ¶m_env, DUMMY_SP); if !always_sized { StructKind::MaybeUnsizedUnivariant } else { StructKind::AlwaysSizedUnivariant } }; From 74f5c61d288fdd925968aca49d1fd82f6e226ff8 Mon Sep 17 00:00:00 2001 From: Austin Hicks Date: Tue, 22 Nov 2016 15:18:28 -0500 Subject: [PATCH 17/28] Change how type-sizes works slightly: we want to ensure that [i16; 0] introduces padding --- src/test/run-pass/type-sizes.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/test/run-pass/type-sizes.rs b/src/test/run-pass/type-sizes.rs index 8c58ec3e526c6..359f825bb63e0 100644 --- a/src/test/run-pass/type-sizes.rs +++ b/src/test/run-pass/type-sizes.rs @@ -26,6 +26,7 @@ enum e2 { a(u32), b } +#[repr(u8)] enum e3 { a([u16; 0], u8), b } @@ -52,5 +53,5 @@ pub fn main() { assert_eq!(size_of::(), 8 as usize); assert_eq!(size_of::(), 8 as usize); - assert_eq!(size_of::(), 2 as usize); + assert_eq!(size_of::(), 4 as usize); } From cf5f80c68d8e924a6a578e975cd92cc018a11983 Mon Sep 17 00:00:00 2001 From: Austin Hicks Date: Wed, 23 Nov 2016 10:19:22 -0500 Subject: [PATCH 18/28] Fix having multiple reprs on the same type. This bug has applied to master for an indefinite period of time and is orthogonal to univariant layout optimization. --- src/librustc/ty/layout.rs | 132 +++++++++++++++++----------- src/test/run-pass/multiple-reprs.rs | 44 ++++++++++ src/test/run-pass/type-sizes.rs | 2 +- 3 files changed, 124 insertions(+), 54 deletions(-) create mode 100644 src/test/run-pass/multiple-reprs.rs diff --git a/src/librustc/ty/layout.rs b/src/librustc/ty/layout.rs index 93a9e1e700a5d..f580730064c36 100644 --- a/src/librustc/ty/layout.rs +++ b/src/librustc/ty/layout.rs @@ -416,7 +416,7 @@ impl Integer { /// signed discriminant range and #[repr] attribute. /// N.B.: u64 values above i64::MAX will be treated as signed, but /// that shouldn't affect anything, other than maybe debuginfo. - pub fn repr_discr(tcx: TyCtxt, ty: Ty, hint: attr::ReprAttr, min: i64, max: i64) + fn repr_discr(tcx: TyCtxt, ty: Ty, hints: &[attr::ReprAttr], min: i64, max: i64) -> (Integer, bool) { // Theoretically, negative values could be larger in unsigned representation // than the unsigned representation of the signed minimum. However, if there @@ -425,33 +425,44 @@ impl Integer { let unsigned_fit = Integer::fit_unsigned(cmp::max(min as u64, max as u64)); let signed_fit = cmp::max(Integer::fit_signed(min), Integer::fit_signed(max)); - let at_least = match hint { - attr::ReprInt(ity) => { - let discr = Integer::from_attr(&tcx.data_layout, ity); - let fit = if ity.is_signed() { signed_fit } else { unsigned_fit }; - if discr < fit { - bug!("Integer::repr_discr: `#[repr]` hint too small for \ - discriminant range of enum `{}", ty) + let mut min_from_extern = None; + let min_default = I8; + + for &r in hints.iter() { + match r { + attr::ReprInt(ity) => { + let discr = Integer::from_attr(&tcx.data_layout, ity); + let fit = if ity.is_signed() { signed_fit } else { unsigned_fit }; + if discr < fit { + bug!("Integer::repr_discr: `#[repr]` hint too small for \ + discriminant range of enum `{}", ty) + } + return (discr, ity.is_signed()); } - return (discr, ity.is_signed()); - } - attr::ReprExtern => { - match &tcx.sess.target.target.arch[..] { - // WARNING: the ARM EABI has two variants; the one corresponding - // to `at_least == I32` appears to be used on Linux and NetBSD, - // but some systems may use the variant corresponding to no - // lower bound. However, we don't run on those yet...? - "arm" => I32, - _ => I32, + attr::ReprExtern => { + match &tcx.sess.target.target.arch[..] { + // WARNING: the ARM EABI has two variants; the one corresponding + // to `at_least == I32` appears to be used on Linux and NetBSD, + // but some systems may use the variant corresponding to no + // lower bound. However, we don't run on those yet...? + "arm" => min_from_extern = Some(I32), + _ => min_from_extern = Some(I32), + } + } + attr::ReprAny => {}, + attr::ReprPacked => { + bug!("Integer::repr_discr: found #[repr(packed)] on enum `{}", ty); + } + attr::ReprSimd => { + bug!("Integer::repr_discr: found #[repr(simd)] on enum `{}", ty); } } - attr::ReprAny => I8, - attr::ReprPacked => { - bug!("Integer::repr_discr: found #[repr(packed)] on enum `{}", ty); - } - attr::ReprSimd => { - bug!("Integer::repr_discr: found #[repr(simd)] on enum `{}", ty); - } + } + + let at_least = if let Some(i) = min_from_extern { + i + } else { + min_default }; // If there are no negative values, we can use the unsigned fit. @@ -536,10 +547,11 @@ enum StructKind { } impl<'a, 'gcx, 'tcx> Struct { + // FIXME(camlorn): reprs need a better representation to deal with multiple reprs on one type. fn new(dl: &TargetDataLayout, fields: &Vec<&'a Layout>, - repr: attr::ReprAttr, kind: StructKind, + reprs: &[attr::ReprAttr], kind: StructKind, scapegoat: Ty<'gcx>) -> Result> { - let packed = repr == attr::ReprPacked; + let packed = reprs.contains(&attr::ReprPacked); let mut ret = Struct { align: if packed { dl.i8_align } else { dl.aggregate_align }, packed: packed, @@ -549,23 +561,37 @@ impl<'a, 'gcx, 'tcx> Struct { min_size: Size::from_bytes(0), }; - // 1-member and 2-member structs don't optimize. + if fields.len() == 0 {return Ok(ret)}; + + // Anything with ReprExtern or ReprPacked doesn't optimize. + // Neither do 1-member and 2-member structs. // In addition, code in trans assume that 2-element structs can become pairs. // It's easier to just short-circuit here. - let can_optimize_struct = fields.len() > 2; + let mut can_optimize = fields.len() > 2 || StructKind::EnumVariant == kind; + if can_optimize { + // This exhaustive match makes new reprs force the adder to modify this function. + // Otherwise, things can silently break. + // Note the inversion, return true to stop matching. + can_optimize = !reprs.iter().any(|r| { + match *r { + attr::ReprAny => false, + attr::ReprInt(_) => false, + attr::ReprExtern => true, + attr::ReprPacked => true, + attr::ReprSimd => bug!("Simd vectors should be represented as a layout::Vector") + } + }); + } - let (optimize, sort_ascending) = match (repr, kind) { - (attr::ReprAny, StructKind::AlwaysSizedUnivariant) => (can_optimize_struct, false), - (attr::ReprAny, StructKind::MaybeUnsizedUnivariant) => (can_optimize_struct, true), - (attr::ReprAny, StructKind::EnumVariant) => { + let (optimize, sort_ascending) = match kind { + StructKind::AlwaysSizedUnivariant => (can_optimize, false), + StructKind::MaybeUnsizedUnivariant => (can_optimize, true), + StructKind::EnumVariant => { assert!(fields.len() >= 1, "Enum variants must have discriminants."); - (true, fields[0].size(dl).bytes() == 1) + (can_optimize, fields[0].size(dl).bytes() == 1) } - _ => (false, false) }; - if fields.len() == 0 {return Ok(ret)}; - ret.offsets = vec![Size::from_bytes(0); fields.len()]; let mut inverse_memory_index: Vec = (0..fields.len() as u32).collect(); @@ -590,6 +616,7 @@ impl<'a, 'gcx, 'tcx> Struct { } } + // inverse_memory_index holds field indices by increasing memory offset. // That is, if field 5 has offset 0, the first element of inverse_memory_index is 5. // We now write field offsets to the corresponding offset slot; // field 5 with offset 0 puts 0 in offsets[5]. @@ -1021,7 +1048,7 @@ impl<'a, 'gcx, 'tcx> Layout { // The never type. ty::TyNever => Univariant { - variant: Struct::new(dl, &vec![], attr::ReprAny, + variant: Struct::new(dl, &vec![], &[], StructKind::AlwaysSizedUnivariant, ty)?, non_zero: false }, @@ -1076,12 +1103,12 @@ impl<'a, 'gcx, 'tcx> Layout { ty::TyFnDef(..) => { Univariant { variant: Struct::new(dl, &vec![], - attr::ReprAny, StructKind::AlwaysSizedUnivariant, ty)?, + &[], StructKind::AlwaysSizedUnivariant, ty)?, non_zero: false } } ty::TyDynamic(_) => { - let mut unit = Struct::new(dl, &vec![], attr::ReprAny, + let mut unit = Struct::new(dl, &vec![], &[], StructKind::AlwaysSizedUnivariant, ty)?; unit.sized = false; Univariant { variant: unit, non_zero: false } @@ -1093,7 +1120,7 @@ impl<'a, 'gcx, 'tcx> Layout { let st = Struct::new(dl, &tys.map(|ty| ty.layout(infcx)) .collect::, _>>()?, - attr::ReprAny, + &[], StructKind::AlwaysSizedUnivariant, ty)?; Univariant { variant: st, non_zero: false } } @@ -1104,7 +1131,7 @@ impl<'a, 'gcx, 'tcx> Layout { let st = Struct::new(dl, &tys.iter().map(|ty| ty.layout(infcx)) .collect::, _>>()?, - attr::ReprAny, StructKind::AlwaysSizedUnivariant, ty)?; + &[], StructKind::AlwaysSizedUnivariant, ty)?; Univariant { variant: st, non_zero: false } } @@ -1128,17 +1155,16 @@ impl<'a, 'gcx, 'tcx> Layout { // ADTs. ty::TyAdt(def, substs) => { - let hint = *tcx.lookup_repr_hints(def.did).get(0) - .unwrap_or(&attr::ReprAny); + let hints = &tcx.lookup_repr_hints(def.did)[..]; if def.variants.is_empty() { // Uninhabitable; represent as unit // (Typechecking will reject discriminant-sizing attrs.) - assert_eq!(hint, attr::ReprAny); + assert_eq!(hints.len(), 0); return success(Univariant { variant: Struct::new(dl, &vec![], - hint, StructKind::AlwaysSizedUnivariant, ty)?, + &hints[..], StructKind::AlwaysSizedUnivariant, ty)?, non_zero: false }); } @@ -1153,7 +1179,7 @@ impl<'a, 'gcx, 'tcx> Layout { if x > max { max = x; } } - let (discr, signed) = Integer::repr_discr(tcx, ty, hint, min, max); + let (discr, signed) = Integer::repr_discr(tcx, ty, &hints[..], min, max); return success(CEnum { discr: discr, signed: signed, @@ -1163,7 +1189,7 @@ impl<'a, 'gcx, 'tcx> Layout { }); } - if !def.is_enum() || def.variants.len() == 1 && hint == attr::ReprAny { + if !def.is_enum() || def.variants.len() == 1 && hints.len() == 0 { // Struct, or union, or univariant enum equivalent to a struct. // (Typechecking will reject discriminant-sizing attrs.) @@ -1190,7 +1216,7 @@ impl<'a, 'gcx, 'tcx> Layout { un.extend(dl, fields.iter().map(|&f| Ok(f)), ty)?; UntaggedUnion { variants: un } } else { - let st = Struct::new(dl, &fields, hint, + let st = Struct::new(dl, &fields, &hints[..], kind, ty)?; let non_zero = Some(def.did) == tcx.lang_items.non_zero(); Univariant { variant: st, non_zero: non_zero } @@ -1213,7 +1239,7 @@ impl<'a, 'gcx, 'tcx> Layout { v.fields.iter().map(|field| field.ty(tcx, substs)).collect::>() }).collect::>(); - if variants.len() == 2 && hint == attr::ReprAny { + if variants.len() == 2 && hints.len() == 0 { // Nullable pointer optimization for discr in 0..2 { let other_fields = variants[1 - discr].iter().map(|ty| { @@ -1245,7 +1271,7 @@ impl<'a, 'gcx, 'tcx> Layout { let st = Struct::new(dl, &variants[discr].iter().map(|ty| ty.layout(infcx)) .collect::, _>>()?, - hint, StructKind::AlwaysSizedUnivariant, ty)?; + &hints[..], StructKind::AlwaysSizedUnivariant, ty)?; // We have to fix the last element of path here. let mut i = *path.last().unwrap(); @@ -1265,7 +1291,7 @@ impl<'a, 'gcx, 'tcx> Layout { // The general case. let discr_max = (variants.len() - 1) as i64; assert!(discr_max >= 0); - let (min_ity, _) = Integer::repr_discr(tcx, ty, hint, 0, discr_max); + let (min_ity, _) = Integer::repr_discr(tcx, ty, &hints[..], 0, discr_max); let mut align = dl.aggregate_align; let mut size = Size::from_bytes(0); @@ -1283,7 +1309,7 @@ impl<'a, 'gcx, 'tcx> Layout { fields.insert(0, &discr); let st = Struct::new(dl, &fields, - hint, StructKind::EnumVariant, ty)?; + &hints[..], StructKind::EnumVariant, ty)?; // Find the first field we can't move later // to make room for a larger discriminant. // It is important to skip the first field. diff --git a/src/test/run-pass/multiple-reprs.rs b/src/test/run-pass/multiple-reprs.rs new file mode 100644 index 0000000000000..7c6740d4fde52 --- /dev/null +++ b/src/test/run-pass/multiple-reprs.rs @@ -0,0 +1,44 @@ +// Copyright 2012 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + + +use std::mem::size_of; + +// The two enums that follow are designed so that bugs trigger layout optimization. +// Specifically, if either of the following reprs used here is not detected by the compiler, +// then the sizes will be wrong. + +#[repr(C, u8)] +enum E1 { + A(u8, u16, u8), + B(u8, u16, u8) +} + +#[repr(u8, C)] +enum E2 { + A(u8, u16, u8), + B(u8, u16, u8) +} + +// From pr 37429 +pub const SIZEOF_QUERY: usize = 21; + +#[repr(C,packed)] +pub struct p0f_api_query { + pub magic: u32, + pub addr_type: u8, + pub addr: [u8; 16], + +} +pub fn main() { + assert_eq!(size_of::(), 6); + assert_eq!(size_of::(), 6); + assert_eq!(size_of::(), 21); +} diff --git a/src/test/run-pass/type-sizes.rs b/src/test/run-pass/type-sizes.rs index 359f825bb63e0..bbb01eaaf46b9 100644 --- a/src/test/run-pass/type-sizes.rs +++ b/src/test/run-pass/type-sizes.rs @@ -26,7 +26,7 @@ enum e2 { a(u32), b } -#[repr(u8)] +#[repr(C, u8)] enum e3 { a([u16; 0], u8), b } From c8c3579bffaeb29b1feae286e06d792293e10287 Mon Sep 17 00:00:00 2001 From: Austin Hicks Date: Wed, 23 Nov 2016 14:48:31 -0500 Subject: [PATCH 19/28] Fix closure arguments which are immediate because of field reordering. While building immediates goes through type_of::type_of, extracting them must account for field reorderings. --- src/librustc_trans/mir/block.rs | 10 ++++++++-- src/test/run-pass/closure-immediate.rs | 22 ++++++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) create mode 100644 src/test/run-pass/closure-immediate.rs diff --git a/src/librustc_trans/mir/block.rs b/src/librustc_trans/mir/block.rs index 76f5f32b5dcff..fe087bc495121 100644 --- a/src/librustc_trans/mir/block.rs +++ b/src/librustc_trans/mir/block.rs @@ -11,7 +11,7 @@ use llvm::{self, ValueRef}; use rustc_const_eval::{ErrKind, ConstEvalErr, note_const_eval_err}; use rustc::middle::lang_items; -use rustc::ty; +use rustc::ty::{self, layout}; use rustc::mir; use abi::{Abi, FnType, ArgType}; use adt; @@ -722,8 +722,14 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { } Immediate(llval) => { + let l = bcx.ccx().layout_of(tuple.ty); + let v = if let layout::Univariant { ref variant, .. } = *l { + variant + } else { + bug!("Not a tuple."); + }; for (n, &ty) in arg_types.iter().enumerate() { - let mut elem = bcx.extract_value(llval, n); + let mut elem = bcx.extract_value(llval, v.memory_index[n] as usize); // Truncate bools to i1, if needed if ty.is_bool() && common::val_ty(elem) != Type::i1(bcx.ccx()) { elem = bcx.trunc(elem, Type::i1(bcx.ccx())); diff --git a/src/test/run-pass/closure-immediate.rs b/src/test/run-pass/closure-immediate.rs new file mode 100644 index 0000000000000..69aa16c3fb592 --- /dev/null +++ b/src/test/run-pass/closure-immediate.rs @@ -0,0 +1,22 @@ +// Copyright 2012 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + + +// After the work to reoptimize structs, it became possible for immediate logic to fail. +// This test verifies that it actually works. + +fn main() { + let c = |a: u8, b: u16, c: u8| { + assert_eq!(a, 1); + assert_eq!(b, 2); + assert_eq!(c, 3); + }; + c(1, 2, 3); +} \ No newline at end of file From 052e59cc128993abfc5e1c8ce9148bef97df572c Mon Sep 17 00:00:00 2001 From: Austin Hicks Date: Wed, 23 Nov 2016 16:31:21 -0500 Subject: [PATCH 20/28] Make tidy --- src/librustc/ty/layout.rs | 2 +- src/test/run-pass/multiple-reprs.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/librustc/ty/layout.rs b/src/librustc/ty/layout.rs index f580730064c36..342542edcf080 100644 --- a/src/librustc/ty/layout.rs +++ b/src/librustc/ty/layout.rs @@ -578,7 +578,7 @@ impl<'a, 'gcx, 'tcx> Struct { attr::ReprInt(_) => false, attr::ReprExtern => true, attr::ReprPacked => true, - attr::ReprSimd => bug!("Simd vectors should be represented as a layout::Vector") + attr::ReprSimd => bug!("Simd vectors should be represented as layout::Vector") } }); } diff --git a/src/test/run-pass/multiple-reprs.rs b/src/test/run-pass/multiple-reprs.rs index 7c6740d4fde52..726563e5b498a 100644 --- a/src/test/run-pass/multiple-reprs.rs +++ b/src/test/run-pass/multiple-reprs.rs @@ -35,8 +35,8 @@ pub struct p0f_api_query { pub magic: u32, pub addr_type: u8, pub addr: [u8; 16], - } + pub fn main() { assert_eq!(size_of::(), 6); assert_eq!(size_of::(), 6); From e9580e262bee0f8a5487f1a549feea561396eb20 Mon Sep 17 00:00:00 2001 From: Austin Hicks Date: Wed, 23 Nov 2016 16:50:36 -0500 Subject: [PATCH 21/28] Some small fixes to how structs/enums are optimized --- src/librustc/ty/layout.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/librustc/ty/layout.rs b/src/librustc/ty/layout.rs index 342542edcf080..6f63b15abe8a7 100644 --- a/src/librustc/ty/layout.rs +++ b/src/librustc/ty/layout.rs @@ -585,10 +585,10 @@ impl<'a, 'gcx, 'tcx> Struct { let (optimize, sort_ascending) = match kind { StructKind::AlwaysSizedUnivariant => (can_optimize, false), - StructKind::MaybeUnsizedUnivariant => (can_optimize, true), + StructKind::MaybeUnsizedUnivariant => (can_optimize, false), StructKind::EnumVariant => { assert!(fields.len() >= 1, "Enum variants must have discriminants."); - (can_optimize, fields[0].size(dl).bytes() == 1) + (can_optimize || fields[0].size(dl).bytes() == 1, true) } }; From 025456e8a4e418813edf77bd9c1e69213276d447 Mon Sep 17 00:00:00 2001 From: Austin Hicks Date: Wed, 23 Nov 2016 17:43:36 -0500 Subject: [PATCH 22/28] Incorporate review comments --- src/librustc/ty/layout.rs | 20 +++++++------------- src/test/run-pass/closure-immediate.rs | 2 +- src/test/run-pass/multiple-reprs.rs | 1 - 3 files changed, 8 insertions(+), 15 deletions(-) diff --git a/src/librustc/ty/layout.rs b/src/librustc/ty/layout.rs index 6f63b15abe8a7..96d66153d777e 100644 --- a/src/librustc/ty/layout.rs +++ b/src/librustc/ty/layout.rs @@ -459,11 +459,7 @@ impl Integer { } } - let at_least = if let Some(i) = min_from_extern { - i - } else { - min_default - }; + let at_least = min_from_extern.unwrap_or(min_default); // If there are no negative values, we can use the unsigned fit. if min >= 0 { @@ -571,13 +567,11 @@ impl<'a, 'gcx, 'tcx> Struct { if can_optimize { // This exhaustive match makes new reprs force the adder to modify this function. // Otherwise, things can silently break. - // Note the inversion, return true to stop matching. + // Note the inversion, return true to stop optimizing. can_optimize = !reprs.iter().any(|r| { match *r { - attr::ReprAny => false, - attr::ReprInt(_) => false, - attr::ReprExtern => true, - attr::ReprPacked => true, + attr::ReprAny | attr::ReprInt(_) => false, + attr::ReprExtern | attr::ReprPacked => true, attr::ReprSimd => bug!("Simd vectors should be represented as layout::Vector") } }); @@ -588,7 +582,7 @@ impl<'a, 'gcx, 'tcx> Struct { StructKind::MaybeUnsizedUnivariant => (can_optimize, false), StructKind::EnumVariant => { assert!(fields.len() >= 1, "Enum variants must have discriminants."); - (can_optimize || fields[0].size(dl).bytes() == 1, true) + (can_optimize && fields[0].size(dl).bytes() == 1, true) } }; @@ -1189,7 +1183,7 @@ impl<'a, 'gcx, 'tcx> Layout { }); } - if !def.is_enum() || def.variants.len() == 1 && hints.len() == 0 { + if !def.is_enum() || def.variants.len() == 1 && hints.is_empty() { // Struct, or union, or univariant enum equivalent to a struct. // (Typechecking will reject discriminant-sizing attrs.) @@ -1239,7 +1233,7 @@ impl<'a, 'gcx, 'tcx> Layout { v.fields.iter().map(|field| field.ty(tcx, substs)).collect::>() }).collect::>(); - if variants.len() == 2 && hints.len() == 0 { + if variants.len() == 2 && hints.is_empty() { // Nullable pointer optimization for discr in 0..2 { let other_fields = variants[1 - discr].iter().map(|ty| { diff --git a/src/test/run-pass/closure-immediate.rs b/src/test/run-pass/closure-immediate.rs index 69aa16c3fb592..e566c10583597 100644 --- a/src/test/run-pass/closure-immediate.rs +++ b/src/test/run-pass/closure-immediate.rs @@ -19,4 +19,4 @@ fn main() { assert_eq!(c, 3); }; c(1, 2, 3); -} \ No newline at end of file +} diff --git a/src/test/run-pass/multiple-reprs.rs b/src/test/run-pass/multiple-reprs.rs index 726563e5b498a..c2fe943eed85a 100644 --- a/src/test/run-pass/multiple-reprs.rs +++ b/src/test/run-pass/multiple-reprs.rs @@ -28,7 +28,6 @@ enum E2 { } // From pr 37429 -pub const SIZEOF_QUERY: usize = 21; #[repr(C,packed)] pub struct p0f_api_query { From cfe1a776ee77697b5947849a71b411978eb23a28 Mon Sep 17 00:00:00 2001 From: Austin Hicks Date: Sat, 26 Nov 2016 14:37:15 -0500 Subject: [PATCH 23/28] Fix -Z print-type-sizes and tests. This was done by sorting the fields by increasing offset; as a consequence, the order of -Z print-type-sizes matches memory order not source order. --- src/librustc/session/code_stats.rs | 7 +++- src/test/ui/print_type_sizes/nullable.stdout | 35 +++++++++----------- src/test/ui/print_type_sizes/packed.stdout | 10 +++--- src/test/ui/print_type_sizes/padding.stdout | 16 +++++---- 4 files changed, 35 insertions(+), 33 deletions(-) diff --git a/src/librustc/session/code_stats.rs b/src/librustc/session/code_stats.rs index 8308c54d70bf4..a042b2abf3a29 100644 --- a/src/librustc/session/code_stats.rs +++ b/src/librustc/session/code_stats.rs @@ -142,7 +142,12 @@ impl CodeStats { max_variant_size = cmp::max(max_variant_size, size); let mut min_offset = discr_size; - for field in fields { + + // We want to print fields by increasing offset. + let mut fields = fields.clone(); + fields.sort_by_key(|f| f.offset); + + for field in fields.iter() { let FieldInfo { ref name, offset, size, align } = *field; // Include field alignment in output only if it caused padding injection diff --git a/src/test/ui/print_type_sizes/nullable.stdout b/src/test/ui/print_type_sizes/nullable.stdout index dd999c4a5e4c7..830678f174f88 100644 --- a/src/test/ui/print_type_sizes/nullable.stdout +++ b/src/test/ui/print_type_sizes/nullable.stdout @@ -1,25 +1,22 @@ -print-type-size type: `IndirectNonZero`: 20 bytes, alignment: 4 bytes -print-type-size field `.pre`: 1 bytes -print-type-size padding: 3 bytes -print-type-size field `.nested`: 12 bytes, alignment: 4 bytes +print-type-size type: `IndirectNonZero`: 12 bytes, alignment: 4 bytes +print-type-size field `.nested`: 8 bytes print-type-size field `.post`: 2 bytes -print-type-size end padding: 2 bytes -print-type-size type: `MyOption>`: 20 bytes, alignment: 4 bytes -print-type-size variant `Some`: 20 bytes -print-type-size field `.0`: 20 bytes -print-type-size type: `EmbeddedDiscr`: 12 bytes, alignment: 4 bytes -print-type-size variant `Record`: 10 bytes -print-type-size field `.pre`: 1 bytes -print-type-size padding: 3 bytes -print-type-size field `.val`: 4 bytes, alignment: 4 bytes -print-type-size field `.post`: 2 bytes -print-type-size end padding: 2 bytes -print-type-size type: `NestedNonZero`: 12 bytes, alignment: 4 bytes print-type-size field `.pre`: 1 bytes -print-type-size padding: 3 bytes -print-type-size field `.val`: 4 bytes, alignment: 4 bytes +print-type-size end padding: 1 bytes +print-type-size type: `MyOption>`: 12 bytes, alignment: 4 bytes +print-type-size variant `Some`: 12 bytes +print-type-size field `.0`: 12 bytes +print-type-size type: `EmbeddedDiscr`: 8 bytes, alignment: 4 bytes +print-type-size variant `Record`: 7 bytes +print-type-size field `.val`: 4 bytes +print-type-size field `.post`: 2 bytes +print-type-size field `.pre`: 1 bytes +print-type-size end padding: 1 bytes +print-type-size type: `NestedNonZero`: 8 bytes, alignment: 4 bytes +print-type-size field `.val`: 4 bytes print-type-size field `.post`: 2 bytes -print-type-size end padding: 2 bytes +print-type-size field `.pre`: 1 bytes +print-type-size end padding: 1 bytes print-type-size type: `MyOption>`: 4 bytes, alignment: 4 bytes print-type-size variant `Some`: 4 bytes print-type-size field `.0`: 4 bytes diff --git a/src/test/ui/print_type_sizes/packed.stdout b/src/test/ui/print_type_sizes/packed.stdout index 1278a7d7c92c6..83fd333c9c7fc 100644 --- a/src/test/ui/print_type_sizes/packed.stdout +++ b/src/test/ui/print_type_sizes/packed.stdout @@ -1,13 +1,11 @@ -print-type-size type: `Padded`: 16 bytes, alignment: 4 bytes +print-type-size type: `Padded`: 12 bytes, alignment: 4 bytes +print-type-size field `.g`: 4 bytes +print-type-size field `.h`: 2 bytes print-type-size field `.a`: 1 bytes print-type-size field `.b`: 1 bytes -print-type-size padding: 2 bytes -print-type-size field `.g`: 4 bytes, alignment: 4 bytes print-type-size field `.c`: 1 bytes -print-type-size padding: 1 bytes -print-type-size field `.h`: 2 bytes, alignment: 2 bytes print-type-size field `.d`: 1 bytes -print-type-size end padding: 3 bytes +print-type-size end padding: 2 bytes print-type-size type: `Packed`: 10 bytes, alignment: 1 bytes print-type-size field `.a`: 1 bytes print-type-size field `.b`: 1 bytes diff --git a/src/test/ui/print_type_sizes/padding.stdout b/src/test/ui/print_type_sizes/padding.stdout index bb95f790bd9e4..0eaff7118b35c 100644 --- a/src/test/ui/print_type_sizes/padding.stdout +++ b/src/test/ui/print_type_sizes/padding.stdout @@ -1,10 +1,12 @@ print-type-size type: `E1`: 12 bytes, alignment: 4 bytes -print-type-size discriminant: 4 bytes -print-type-size variant `A`: 5 bytes -print-type-size field `.0`: 4 bytes +print-type-size discriminant: 1 bytes +print-type-size variant `A`: 7 bytes print-type-size field `.1`: 1 bytes -print-type-size variant `B`: 8 bytes -print-type-size field `.0`: 8 bytes +print-type-size padding: 2 bytes +print-type-size field `.0`: 4 bytes, alignment: 4 bytes +print-type-size variant `B`: 11 bytes +print-type-size padding: 3 bytes +print-type-size field `.0`: 8 bytes, alignment: 4 bytes print-type-size type: `E2`: 12 bytes, alignment: 4 bytes print-type-size discriminant: 1 bytes print-type-size variant `A`: 7 bytes @@ -15,7 +17,7 @@ print-type-size variant `B`: 11 bytes print-type-size padding: 3 bytes print-type-size field `.0`: 8 bytes, alignment: 4 bytes print-type-size type: `S`: 8 bytes, alignment: 4 bytes +print-type-size field `.g`: 4 bytes print-type-size field `.a`: 1 bytes print-type-size field `.b`: 1 bytes -print-type-size padding: 2 bytes -print-type-size field `.g`: 4 bytes, alignment: 4 bytes +print-type-size end padding: 2 bytes From a65cc1ea546bca6cccf4e479e4acd09a8a839ce0 Mon Sep 17 00:00:00 2001 From: Austin Hicks Date: Wed, 30 Nov 2016 16:21:40 -0500 Subject: [PATCH 24/28] Fix error introduced during last rebase --- src/librustc/ty/layout.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc/ty/layout.rs b/src/librustc/ty/layout.rs index 96d66153d777e..8021fc3ba15cf 100644 --- a/src/librustc/ty/layout.rs +++ b/src/librustc/ty/layout.rs @@ -1101,7 +1101,7 @@ impl<'a, 'gcx, 'tcx> Layout { non_zero: false } } - ty::TyDynamic(_) => { + ty::TyDynamic(..) => { let mut unit = Struct::new(dl, &vec![], &[], StructKind::AlwaysSizedUnivariant, ty)?; unit.sized = false; From 9966bbd1b11fcd2866cdbd21146f5e9a0d8ea66c Mon Sep 17 00:00:00 2001 From: Austin Hicks Date: Fri, 2 Dec 2016 19:24:24 -0500 Subject: [PATCH 25/28] Fix computation of enum names based off the discrfield in the case of the null pointer optimization. This functionality is needed by pretty printers for gdb and lldb. --- src/librustc/ty/layout.rs | 46 +++++++++++++++--------- src/librustc_trans/base.rs | 3 +- src/librustc_trans/debuginfo/metadata.rs | 6 ++-- src/test/debuginfo/struct-in-enum.rs | 4 +-- 4 files changed, 36 insertions(+), 23 deletions(-) diff --git a/src/librustc/ty/layout.rs b/src/librustc/ty/layout.rs index 8021fc3ba15cf..7a8a34788d277 100644 --- a/src/librustc/ty/layout.rs +++ b/src/librustc/ty/layout.rs @@ -717,16 +717,18 @@ impl<'a, 'gcx, 'tcx> Struct { /// Find the path leading to a non-zero leaf field, starting from /// the given type and recursing through aggregates. + /// The tuple is `(path, source_path)1, + /// where `path` is in memory order and `source_path` in source order. // FIXME(eddyb) track value ranges and traverse already optimized enums. fn non_zero_field_in_type(infcx: &InferCtxt<'a, 'gcx, 'tcx>, ty: Ty<'gcx>) - -> Result, LayoutError<'gcx>> { + -> Result, LayoutError<'gcx>> { let tcx = infcx.tcx.global_tcx(); match (ty.layout(infcx)?, &ty.sty) { (&Scalar { non_zero: true, .. }, _) | - (&CEnum { non_zero: true, .. }, _) => Ok(Some(vec![])), + (&CEnum { non_zero: true, .. }, _) => Ok(Some((vec![], vec![]))), (&FatPointer { non_zero: true, .. }, _) => { - Ok(Some(vec![FAT_PTR_ADDR as u32])) + Ok(Some((vec![FAT_PTR_ADDR as u32], vec![FAT_PTR_ADDR as u32]))) } // Is this the NonZero lang item wrapping a pointer or integer type? @@ -737,10 +739,11 @@ impl<'a, 'gcx, 'tcx> Struct { // FIXME(eddyb) also allow floating-point types here. Scalar { value: Int(_), non_zero: false } | Scalar { value: Pointer, non_zero: false } => { - Ok(Some(vec![0])) + Ok(Some((vec![0], vec![0]))) } FatPointer { non_zero: false, .. } => { - Ok(Some(vec![FAT_PTR_ADDR as u32, 0])) + let tmp = vec![FAT_PTR_ADDR as u32, 0]; + Ok(Some((tmp.clone(), tmp))) } _ => Ok(None) } @@ -749,7 +752,7 @@ impl<'a, 'gcx, 'tcx> Struct { // Perhaps one of the fields of this struct is non-zero // let's recurse and find out (&Univariant { ref variant, .. }, &ty::TyAdt(def, substs)) if def.is_struct() => { - Struct::non_zero_field_path(infcx, def.struct_variant().fields + Struct::non_zero_field_paths(infcx, def.struct_variant().fields .iter().map(|field| { field.ty(tcx, substs) }), @@ -759,19 +762,19 @@ impl<'a, 'gcx, 'tcx> Struct { // Perhaps one of the upvars of this closure is non-zero (&Univariant { ref variant, .. }, &ty::TyClosure(def, substs)) => { let upvar_tys = substs.upvar_tys(def, tcx); - Struct::non_zero_field_path(infcx, upvar_tys, + Struct::non_zero_field_paths(infcx, upvar_tys, Some(&variant.memory_index[..])) } // Can we use one of the fields in this tuple? (&Univariant { ref variant, .. }, &ty::TyTuple(tys)) => { - Struct::non_zero_field_path(infcx, tys.iter().cloned(), + Struct::non_zero_field_paths(infcx, tys.iter().cloned(), Some(&variant.memory_index[..])) } // Is this a fixed-size array of something non-zero // with at least one element? (_, &ty::TyArray(ety, d)) if d > 0 => { - Struct::non_zero_field_path(infcx, Some(ety).into_iter(), None) + Struct::non_zero_field_paths(infcx, Some(ety).into_iter(), None) } (_, &ty::TyProjection(_)) | (_, &ty::TyAnon(..)) => { @@ -789,20 +792,23 @@ impl<'a, 'gcx, 'tcx> Struct { /// Find the path leading to a non-zero leaf field, starting from /// the given set of fields and recursing through aggregates. - fn non_zero_field_path(infcx: &InferCtxt<'a, 'gcx, 'tcx>, + // / Returns Some((path, source_path)) on success. + /// `path` is translated to memory order. `source_path` is not. + fn non_zero_field_paths(infcx: &InferCtxt<'a, 'gcx, 'tcx>, fields: I, permutation: Option<&[u32]>) - -> Result, LayoutError<'gcx>> + -> Result, LayoutError<'gcx>> where I: Iterator> { for (i, ty) in fields.enumerate() { - if let Some(mut path) = Struct::non_zero_field_in_type(infcx, ty)? { + if let Some((mut path, mut source_path)) = Struct::non_zero_field_in_type(infcx, ty)? { + source_path.push(i as u32); let index = if let Some(p) = permutation { p[i] as usize } else { i }; path.push(index as u32); - return Ok(Some(path)); + return Ok(Some((path, source_path))); } } Ok(None) @@ -965,7 +971,9 @@ pub enum Layout { nndiscr: u64, nonnull: Struct, // N.B. There is a 0 at the start, for LLVM GEP through a pointer. - discrfield: FieldPath + discrfield: FieldPath, + // Like discrfield, but in source order. For debuginfo. + discrfield_source: FieldPath } } @@ -1242,10 +1250,11 @@ impl<'a, 'gcx, 'tcx> Layout { if !Struct::would_be_zero_sized(dl, other_fields)? { continue; } - let path = Struct::non_zero_field_path(infcx, + let paths = Struct::non_zero_field_paths(infcx, variants[discr].iter().cloned(), None)?; - let mut path = if let Some(p) = path { p } else { continue }; + let (mut path, mut path_source) = if let Some(p) = paths { p } + else { continue }; // FIXME(eddyb) should take advantage of a newtype. if path == &[0] && variants[discr].len() == 1 { @@ -1273,11 +1282,14 @@ impl<'a, 'gcx, 'tcx> Layout { *path.last_mut().unwrap() = i; path.push(0); // For GEP through a pointer. path.reverse(); + path_source.push(0); + path_source.reverse(); return success(StructWrappedNullablePointer { nndiscr: discr as u64, nonnull: st, - discrfield: path + discrfield: path, + discrfield_source: path_source }); } } diff --git a/src/librustc_trans/base.rs b/src/librustc_trans/base.rs index c379ec7da90e1..c7f21427a0ceb 100644 --- a/src/librustc_trans/base.rs +++ b/src/librustc_trans/base.rs @@ -1870,7 +1870,8 @@ fn gather_type_sizes<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>) { match **layout { Layout::StructWrappedNullablePointer { nonnull: ref variant_layout, nndiscr, - discrfield: _ } => { + discrfield: _, + discrfield_source: _ } => { debug!("print-type-size t: `{:?}` adt struct-wrapped nullable nndiscr {} is {:?}", ty, nndiscr, variant_layout); let variant_def = &adt_def.variants[nndiscr as usize]; diff --git a/src/librustc_trans/debuginfo/metadata.rs b/src/librustc_trans/debuginfo/metadata.rs index f6b5bde6ebb44..f9600be1c6618 100644 --- a/src/librustc_trans/debuginfo/metadata.rs +++ b/src/librustc_trans/debuginfo/metadata.rs @@ -1258,7 +1258,7 @@ impl<'tcx> EnumMemberDescriptionFactory<'tcx> { }, layout::StructWrappedNullablePointer { nonnull: ref struct_def, nndiscr, - ref discrfield, ..} => { + ref discrfield_source, ..} => { // Create a description of the non-null variant let (variant_type_metadata, variant_llvm_type, member_description_factory) = describe_enum_variant(cx, @@ -1281,12 +1281,12 @@ impl<'tcx> EnumMemberDescriptionFactory<'tcx> { // member's name. let null_variant_index = (1 - nndiscr) as usize; let null_variant_name = adt.variants[null_variant_index].name; - let discrfield = discrfield.iter() + let discrfield_source = discrfield_source.iter() .skip(1) .map(|x| x.to_string()) .collect::>().join("$"); let union_member_name = format!("RUST$ENCODED$ENUM${}${}", - discrfield, + discrfield_source, null_variant_name); // Create the (singleton) list of descriptions of union members. diff --git a/src/test/debuginfo/struct-in-enum.rs b/src/test/debuginfo/struct-in-enum.rs index d0aceaa4f3f9c..ffd36ae14ad7c 100644 --- a/src/test/debuginfo/struct-in-enum.rs +++ b/src/test/debuginfo/struct-in-enum.rs @@ -19,11 +19,11 @@ // gdb-command:run // gdb-command:print case1 -// gdbg-check:$1 = {{RUST$ENUM$DISR = Case1, __0 = 0, __1 = {x = 2088533116, y = 2088533116, z = 31868}}, {RUST$ENUM$DISR = Case1, __0 = 0, __1 = 8970181431921507452, __2 = 31868}} +// gdbg-check:$1 = {{RUST$ENUM$DISR = Case1, __0 = 0, __1 = {x = 2088533116, y = 2088533116, z = 31868}}, {RUST$ENUM$DISR = Case1, [...]}} // gdbr-check:$1 = struct_in_enum::Regular::Case1(0, struct_in_enum::Struct {x: 2088533116, y: 2088533116, z: 31868}) // gdb-command:print case2 -// gdbg-check:$2 = {{RUST$ENUM$DISR = Case2, __0 = 0, __1 = {x = 286331153, y = 286331153, z = 4369}}, {RUST$ENUM$DISR = Case2, __0 = 0, __1 = 1229782938247303441, __2 = 4369}} +// gdbg-check:$2 = {{RUST$ENUM$DISR = Case2, [...]}, {RUST$ENUM$DISR = Case2, __0 = 0, __1 = 1229782938247303441, __2 = 4369}} // gdbr-check:$2 = struct_in_enum::Regular::Case2(0, 1229782938247303441, 4369) // gdb-command:print univariant From 79c35bbd8cbab76e0ba2e51970e2fc2c35cdcc45 Mon Sep 17 00:00:00 2001 From: Austin Hicks Date: Wed, 7 Dec 2016 12:42:07 -0500 Subject: [PATCH 26/28] Add a case to type-sizes to explicitly verify that field reordering triggers. --- src/test/run-pass/type-sizes.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/test/run-pass/type-sizes.rs b/src/test/run-pass/type-sizes.rs index bbb01eaaf46b9..6614a76107f85 100644 --- a/src/test/run-pass/type-sizes.rs +++ b/src/test/run-pass/type-sizes.rs @@ -31,6 +31,10 @@ enum e3 { a([u16; 0], u8), b } +// Test struct field reordering to make sure it actually reorders. +struct WillOptimize1(u8, u16, u8); +struct WillOptimize2 { a: u8, b: u16, c: u8} + pub fn main() { assert_eq!(size_of::(), 1 as usize); assert_eq!(size_of::(), 4 as usize); @@ -54,4 +58,7 @@ pub fn main() { assert_eq!(size_of::(), 8 as usize); assert_eq!(size_of::(), 8 as usize); assert_eq!(size_of::(), 4 as usize); + + assert_eq!(size_of::(), 4); + assert_eq!(size_of::(), 4); } From f22a22b28658dc55e69d1aeb978c384493fdb6ee Mon Sep 17 00:00:00 2001 From: Austin Hicks Date: Fri, 16 Dec 2016 12:06:12 -0500 Subject: [PATCH 27/28] Incorporate review comments. --- src/librustc/ty/layout.rs | 16 +++++++--------- src/librustc_trans/debuginfo/metadata.rs | 6 +++--- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/librustc/ty/layout.rs b/src/librustc/ty/layout.rs index 7a8a34788d277..53ed103ad9141 100644 --- a/src/librustc/ty/layout.rs +++ b/src/librustc/ty/layout.rs @@ -557,8 +557,6 @@ impl<'a, 'gcx, 'tcx> Struct { min_size: Size::from_bytes(0), }; - if fields.len() == 0 {return Ok(ret)}; - // Anything with ReprExtern or ReprPacked doesn't optimize. // Neither do 1-member and 2-member structs. // In addition, code in trans assume that 2-element structs can become pairs. @@ -590,9 +588,9 @@ impl<'a, 'gcx, 'tcx> Struct { let mut inverse_memory_index: Vec = (0..fields.len() as u32).collect(); if optimize { - let start = if let StructKind::EnumVariant = kind {1} else {0}; + let start = if let StructKind::EnumVariant = kind { 1 } else { 0 }; let end = if let StructKind::MaybeUnsizedUnivariant = kind { - fields.len()-1 + fields.len() - 1 } else { fields.len() }; @@ -717,12 +715,12 @@ impl<'a, 'gcx, 'tcx> Struct { /// Find the path leading to a non-zero leaf field, starting from /// the given type and recursing through aggregates. - /// The tuple is `(path, source_path)1, + /// The tuple is `(path, source_path)`, /// where `path` is in memory order and `source_path` in source order. // FIXME(eddyb) track value ranges and traverse already optimized enums. fn non_zero_field_in_type(infcx: &InferCtxt<'a, 'gcx, 'tcx>, - ty: Ty<'gcx>) - -> Result, LayoutError<'gcx>> { + ty: Ty<'gcx>) + -> Result, LayoutError<'gcx>> { let tcx = infcx.tcx.global_tcx(); match (ty.layout(infcx)?, &ty.sty) { (&Scalar { non_zero: true, .. }, _) | @@ -792,7 +790,7 @@ impl<'a, 'gcx, 'tcx> Struct { /// Find the path leading to a non-zero leaf field, starting from /// the given set of fields and recursing through aggregates. - // / Returns Some((path, source_path)) on success. + /// Returns Some((path, source_path)) on success. /// `path` is translated to memory order. `source_path` is not. fn non_zero_field_paths(infcx: &InferCtxt<'a, 'gcx, 'tcx>, fields: I, @@ -1363,7 +1361,7 @@ impl<'a, 'gcx, 'tcx> Layout { for i in variant.offsets.iter_mut() { // The first field is the discrimminant, at offset 0. // These aren't in order, and we need to skip it. - if *i <= old_ity_size && *i > Size::from_bytes(0){ + if *i <= old_ity_size && *i > Size::from_bytes(0) { *i = new_ity_size; } } diff --git a/src/librustc_trans/debuginfo/metadata.rs b/src/librustc_trans/debuginfo/metadata.rs index f9600be1c6618..dc89caf02016c 100644 --- a/src/librustc_trans/debuginfo/metadata.rs +++ b/src/librustc_trans/debuginfo/metadata.rs @@ -1309,7 +1309,7 @@ impl<'tcx> EnumMemberDescriptionFactory<'tcx> { // Creates MemberDescriptions for the fields of a single enum variant. struct VariantMemberDescriptionFactory<'tcx> { // Cloned from the layout::Struct describing the variant. - offsets: Vec, + offsets: &'tcx [layout::Size], args: Vec<(String, Ty<'tcx>)>, discriminant_type_metadata: Option, span: Span, @@ -1346,7 +1346,7 @@ enum EnumDiscriminantInfo { // full RecursiveTypeDescription. fn describe_enum_variant<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>, enum_type: Ty<'tcx>, - struct_def: &layout::Struct, + struct_def: &'tcx layout::Struct, variant: &'tcx ty::VariantDef, discriminant_info: EnumDiscriminantInfo, containing_scope: DIScope, @@ -1430,7 +1430,7 @@ fn describe_enum_variant<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>, let member_description_factory = VariantMDF(VariantMemberDescriptionFactory { - offsets: struct_def.offsets.clone(), + offsets: &struct_def.offsets[..], args: args, discriminant_type_metadata: match discriminant_info { RegularDiscriminant(discriminant_type_metadata) => { From ff59474ed356d69d75447af79278bdd28db16710 Mon Sep 17 00:00:00 2001 From: Austin Hicks Date: Fri, 16 Dec 2016 15:50:14 -0500 Subject: [PATCH 28/28] flock needs repr(C) --- src/librustc_data_structures/flock.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/librustc_data_structures/flock.rs b/src/librustc_data_structures/flock.rs index 510c9ceef0960..33d71ba862643 100644 --- a/src/librustc_data_structures/flock.rs +++ b/src/librustc_data_structures/flock.rs @@ -31,6 +31,7 @@ mod imp { mod os { use libc; + #[repr(C)] pub struct flock { pub l_type: libc::c_short, pub l_whence: libc::c_short, @@ -53,6 +54,7 @@ mod imp { mod os { use libc; + #[repr(C)] pub struct flock { pub l_start: libc::off_t, pub l_len: libc::off_t, @@ -76,6 +78,7 @@ mod imp { mod os { use libc; + #[repr(C)] pub struct flock { pub l_start: libc::off_t, pub l_len: libc::off_t, @@ -98,6 +101,7 @@ mod imp { mod os { use libc; + #[repr(C)] pub struct flock { pub l_type: libc::c_short, pub l_whence: libc::c_short, @@ -119,6 +123,7 @@ mod imp { mod os { use libc; + #[repr(C)] pub struct flock { pub l_start: libc::off_t, pub l_len: libc::off_t, @@ -141,6 +146,7 @@ mod imp { mod os { use libc; + #[repr(C)] pub struct flock { pub l_type: libc::c_short, pub l_whence: libc::c_short,