From 39ec9f850b05bafd6af7e1d3c143bc56b21f7742 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Mon, 27 Jul 2015 14:40:55 +0200 Subject: [PATCH 1/5] trans: Add Type::to_string method to improve options for debug output. --- src/librustc_trans/trans/type_.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/librustc_trans/trans/type_.rs b/src/librustc_trans/trans/type_.rs index c88509dac4caa..699115a070986 100644 --- a/src/librustc_trans/trans/type_.rs +++ b/src/librustc_trans/trans/type_.rs @@ -50,6 +50,12 @@ impl Type { self.rf } + pub fn to_string(self: Type) -> String { + llvm::build_string(|s| unsafe { + llvm::LLVMWriteTypeToString(self.to_ref(), s); + }).expect("non-UTF8 type description from LLVM") + } + pub fn void(ccx: &CrateContext) -> Type { ty!(llvm::LLVMVoidTypeInContext(ccx.llcx())) } @@ -315,9 +321,7 @@ impl TypeNames { } pub fn type_to_string(&self, ty: Type) -> String { - llvm::build_string(|s| unsafe { - llvm::LLVMWriteTypeToString(ty.to_ref(), s); - }).expect("non-UTF8 type description from LLVM") + ty.to_string() } pub fn types_to_str(&self, tys: &[Type]) -> String { From cea0dc4f6d3d9648f7f2a1d894a884563de2984b Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Mon, 27 Jul 2015 14:45:20 +0200 Subject: [PATCH 2/5] Part of #27023: Put drop flag at end of a type's representation. This is trickier than it sounds (because the DST code was written assuming that one could divide the sized and unsized portions of a type strictly into a sized prefix and unsized suffix, when it reality it is more like a sized prefix and sized suffix that surround the single unsized field). I chose to put in a pretty hack-ish approach to this because drop-flags are scheduled to go away anyway, so its not really worth the effort to to make an infrastructure that sounds as general as the previous paragraph indicates. Also, I have written notes of other fixes that need to be made to really finish fixing #27023, namely more work needs to be done to account for alignment when computing the size of a value. --- src/librustc_trans/trans/adt.rs | 111 ++++++++++++++++++++++++++----- src/librustc_trans/trans/glue.rs | 40 +++++++++-- 2 files changed, 128 insertions(+), 23 deletions(-) diff --git a/src/librustc_trans/trans/adt.rs b/src/librustc_trans/trans/adt.rs index b47d2dd4112ca..1a5eebfa77ae4 100644 --- a/src/librustc_trans/trans/adt.rs +++ b/src/librustc_trans/trans/adt.rs @@ -67,6 +67,31 @@ use trans::type_of; type Hint = attr::ReprAttr; +// Representation of the context surrounding an unsized type. I want +// to be able to track the drop flags that are injected by trans. +#[derive(Clone, Copy, PartialEq, Debug)] +pub struct TypeContext { + prefix: Type, + needs_drop_flag: bool, +} + +impl TypeContext { + pub fn prefix(&self) -> Type { self.prefix } + pub fn needs_drop_flag(&self) -> bool { self.needs_drop_flag } + + fn direct(t: Type) -> TypeContext { + TypeContext { prefix: t, needs_drop_flag: false } + } + fn may_need_drop_flag(t: Type, needs_drop_flag: bool) -> TypeContext { + TypeContext { prefix: t, needs_drop_flag: needs_drop_flag } + } + pub fn to_string(self) -> String { + let TypeContext { prefix, needs_drop_flag } = self; + format!("TypeContext {{ prefix: {}, needs_drop_flag: {} }}", + prefix.to_string(), needs_drop_flag) + } +} + /// Representations. #[derive(Eq, PartialEq, Debug)] pub enum Repr<'tcx> { @@ -125,7 +150,7 @@ pub struct Struct<'tcx> { pub align: u32, pub sized: bool, pub packed: bool, - pub fields: Vec> + pub fields: Vec>, } /// Convenience for `represent_type`. There should probably be more or @@ -681,18 +706,30 @@ fn ensure_enum_fits_in_address_space<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, /// and fill in the actual contents in a second pass to prevent /// unbounded recursion; see also the comments in `trans::type_of`. pub fn type_of<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>, r: &Repr<'tcx>) -> Type { - generic_type_of(cx, r, None, false, false) + let c = generic_type_of(cx, r, None, false, false, false); + assert!(!c.needs_drop_flag); + c.prefix } + + // Pass dst=true if the type you are passing is a DST. Yes, we could figure // this out, but if you call this on an unsized type without realising it, you // are going to get the wrong type (it will not include the unsized parts of it). pub fn sizing_type_of<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>, r: &Repr<'tcx>, dst: bool) -> Type { - generic_type_of(cx, r, None, true, dst) + let c = generic_type_of(cx, r, None, true, dst, false); + assert!(!c.needs_drop_flag); + c.prefix +} +pub fn sizing_type_context_of<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>, + r: &Repr<'tcx>, dst: bool) -> TypeContext { + generic_type_of(cx, r, None, true, dst, true) } pub fn incomplete_type_of<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>, r: &Repr<'tcx>, name: &str) -> Type { - generic_type_of(cx, r, Some(name), false, false) + let c = generic_type_of(cx, r, Some(name), false, false, false); + assert!(!c.needs_drop_flag); + c.prefix } pub fn finish_type_of<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>, r: &Repr<'tcx>, llty: &mut Type) { @@ -708,20 +745,50 @@ fn generic_type_of<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>, r: &Repr<'tcx>, name: Option<&str>, sizing: bool, - dst: bool) -> Type { + dst: bool, + delay_drop_flag: bool) -> TypeContext { + debug!("adt::generic_type_of r: {:?} name: {:?} sizing: {} dst: {} delay_drop_flag: {}", + r, name, sizing, dst, delay_drop_flag); match *r { - CEnum(ity, _, _) => ll_inttype(cx, ity), - RawNullablePointer { nnty, .. } => type_of::sizing_type_of(cx, nnty), - Univariant(ref st, _) | StructWrappedNullablePointer { nonnull: ref st, .. } => { + CEnum(ity, _, _) => TypeContext::direct(ll_inttype(cx, ity)), + RawNullablePointer { nnty, .. } => + TypeContext::direct(type_of::sizing_type_of(cx, nnty)), + StructWrappedNullablePointer { nonnull: ref st, .. } => { + match name { + None => { + TypeContext::direct( + Type::struct_(cx, &struct_llfields(cx, st, sizing, dst), + st.packed)) + } + Some(name) => { + assert_eq!(sizing, false); + TypeContext::direct(Type::named_struct(cx, name)) + } + } + } + Univariant(ref st, dtor_needed) => { + let dtor_needed = dtor_needed != 0; match name { None => { - Type::struct_(cx, &struct_llfields(cx, st, sizing, dst), - st.packed) + let mut fields = struct_llfields(cx, st, sizing, dst); + if delay_drop_flag && dtor_needed { + fields.pop(); + } + TypeContext::may_need_drop_flag( + Type::struct_(cx, &fields, + st.packed), + delay_drop_flag && dtor_needed) + } + Some(name) => { + // Hypothesis: named_struct's can never need a + // drop flag. (... needs validation.) + assert_eq!(sizing, false); + TypeContext::direct(Type::named_struct(cx, name)) } - Some(name) => { assert_eq!(sizing, false); Type::named_struct(cx, name) } } } - General(ity, ref sts, _) => { + General(ity, ref sts, dtor_needed) => { + let dtor_needed = dtor_needed != 0; // We need a representation that has: // * The alignment of the most-aligned field // * The size of the largest variant (rounded up to that alignment) @@ -753,15 +820,25 @@ fn generic_type_of<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>, }; assert_eq!(machine::llalign_of_min(cx, fill_ty), align); assert_eq!(align_s % discr_size, 0); - let fields = [discr_ty, - Type::array(&discr_ty, align_s / discr_size - 1), - fill_ty]; + let mut fields: Vec = + [discr_ty, + Type::array(&discr_ty, align_s / discr_size - 1), + fill_ty].iter().cloned().collect(); + if delay_drop_flag && dtor_needed { + fields.pop(); + } match name { - None => Type::struct_(cx, &fields[..], false), + None => { + TypeContext::may_need_drop_flag( + Type::struct_(cx, &fields[..], false), + delay_drop_flag && dtor_needed) + } Some(name) => { let mut llty = Type::named_struct(cx, name); llty.set_struct_body(&fields[..], false); - llty + TypeContext::may_need_drop_flag( + llty, + delay_drop_flag && dtor_needed) } } } diff --git a/src/librustc_trans/trans/glue.rs b/src/librustc_trans/trans/glue.rs index 6019099c05825..526c7277a665e 100644 --- a/src/librustc_trans/trans/glue.rs +++ b/src/librustc_trans/trans/glue.rs @@ -406,8 +406,12 @@ pub fn size_and_align_of_dst<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, t: Ty<'tcx>, in t, bcx.val_to_string(info)); if type_is_sized(bcx.tcx(), t) { let sizing_type = sizing_type_of(bcx.ccx(), t); - let size = C_uint(bcx.ccx(), llsize_of_alloc(bcx.ccx(), sizing_type)); - let align = C_uint(bcx.ccx(), align_of(bcx.ccx(), t)); + let size = llsize_of_alloc(bcx.ccx(), sizing_type); + let align = align_of(bcx.ccx(), t); + debug!("size_and_align_of_dst t={} info={} size: {} align: {}", + t, bcx.val_to_string(info), size, align); + let size = C_uint(bcx.ccx(), size); + let align = C_uint(bcx.ccx(), align); return (size, align); } match t.sty { @@ -417,9 +421,14 @@ pub fn size_and_align_of_dst<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, t: Ty<'tcx>, in // Don't use type_of::sizing_type_of because that expects t to be sized. assert!(!t.is_simd(bcx.tcx())); let repr = adt::represent_type(ccx, t); - let sizing_type = adt::sizing_type_of(ccx, &*repr, true); - let sized_size = C_uint(ccx, llsize_of_alloc(ccx, sizing_type)); - let sized_align = C_uint(ccx, llalign_of_min(ccx, sizing_type)); + let sizing_type = adt::sizing_type_context_of(ccx, &*repr, true); + debug!("DST {} sizing_type: {}", t, sizing_type.to_string()); + let sized_size = llsize_of_alloc(ccx, sizing_type.prefix()); + let sized_align = llalign_of_min(ccx, sizing_type.prefix()); + debug!("DST {} statically sized prefix size: {} align: {}", + t, sized_size, sized_align); + let sized_size = C_uint(ccx, sized_size); + let sized_align = C_uint(ccx, sized_align); // Recurse to get the size of the dynamically sized field (must be // the last field). @@ -428,8 +437,22 @@ pub fn size_and_align_of_dst<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, t: Ty<'tcx>, in let field_ty = last_field.mt.ty; let (unsized_size, unsized_align) = size_and_align_of_dst(bcx, field_ty, info); + // #27023 FIXME: We should be adding any necessary padding + // to `sized_size` (to accommodate the `unsized_align` + // required of the unsized field that follows) before + // summing it with `sized_size`. + // Return the sum of sizes and max of aligns. - let size = Add(bcx, sized_size, unsized_size, DebugLoc::None); + let mut size = Add(bcx, sized_size, unsized_size, DebugLoc::None); + + // Issue #27023: If there is a drop flag, *now* we add 1 + // to the size. (We can do this without adding any + // padding because drop flags do not have any alignment + // constraints.) + if sizing_type.needs_drop_flag() { + size = Add(bcx, size, C_uint(bcx.ccx(), 1_u64), DebugLoc::None); + } + let align = Select(bcx, ICmp(bcx, llvm::IntULT, @@ -438,6 +461,11 @@ pub fn size_and_align_of_dst<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, t: Ty<'tcx>, in DebugLoc::None), sized_align, unsized_align); + + // #27023 FIXME: We should be adding any necessary padding + // to `size` (to make it a multiple of `align`) before + // returning it. + (size, align) } ty::TyTrait(..) => { From 5a156190cb74ac92432e6ec8c91048ffa91a8a98 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Mon, 27 Jul 2015 16:53:57 +0200 Subject: [PATCH 3/5] Fix more bugs in the alignment calculation refs to DSTs. --- src/librustc_trans/trans/glue.rs | 37 ++++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 7 deletions(-) diff --git a/src/librustc_trans/trans/glue.rs b/src/librustc_trans/trans/glue.rs index 526c7277a665e..67e2e4cba4c34 100644 --- a/src/librustc_trans/trans/glue.rs +++ b/src/librustc_trans/trans/glue.rs @@ -437,34 +437,57 @@ pub fn size_and_align_of_dst<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, t: Ty<'tcx>, in let field_ty = last_field.mt.ty; let (unsized_size, unsized_align) = size_and_align_of_dst(bcx, field_ty, info); + let dbloc = DebugLoc::None; + // #27023 FIXME: We should be adding any necessary padding // to `sized_size` (to accommodate the `unsized_align` // required of the unsized field that follows) before // summing it with `sized_size`. // Return the sum of sizes and max of aligns. - let mut size = Add(bcx, sized_size, unsized_size, DebugLoc::None); + let mut size = Add(bcx, sized_size, unsized_size, dbloc); // Issue #27023: If there is a drop flag, *now* we add 1 // to the size. (We can do this without adding any // padding because drop flags do not have any alignment // constraints.) if sizing_type.needs_drop_flag() { - size = Add(bcx, size, C_uint(bcx.ccx(), 1_u64), DebugLoc::None); + size = Add(bcx, size, C_uint(bcx.ccx(), 1_u64), dbloc); } + // Choose max of two known alignments (combined value must + // be aligned according to more restrictive of the two). let align = Select(bcx, ICmp(bcx, - llvm::IntULT, + llvm::IntUGT, sized_align, unsized_align, - DebugLoc::None), + dbloc), sized_align, unsized_align); - // #27023 FIXME: We should be adding any necessary padding - // to `size` (to make it a multiple of `align`) before - // returning it. + // Issue #27023: must add any necessary padding to `size` + // (to make it a multiple of `align`) before returning it. + // + // Namely, the returned size should be, in C notation: + // + // `size + ((size & (align-1)) ? align : 0)` + // + // Currently I am emulating the above via: + // + // `size + ((size & (align-1)) * align-(size & (align-1)))` + // + // because I am not sure which is cheaper between a branch + // or a multiply. + + let mask = Sub(bcx, align, C_uint(bcx.ccx(), 1_u64), dbloc); + let lowbits = And(bcx, size, mask, DebugLoc::None); + let nonzero = ICmp(bcx, llvm::IntNE, lowbits, C_uint(bcx.ccx(), 0_u64), dbloc); + let add_size = Mul(bcx, + ZExt(bcx, nonzero, Type::i64(bcx.ccx())), + Sub(bcx, align, lowbits, dbloc), + dbloc); + let size = Add(bcx, size, add_size, dbloc); (size, align) } From 26f4ebe7a0b166cbfb04f6e8df16cb799f81058b Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Wed, 29 Jul 2015 19:43:26 +0200 Subject: [PATCH 4/5] Revise a comment to include reference to issue #26403. --- src/librustc_trans/trans/glue.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/librustc_trans/trans/glue.rs b/src/librustc_trans/trans/glue.rs index 67e2e4cba4c34..91b20f0b9ded0 100644 --- a/src/librustc_trans/trans/glue.rs +++ b/src/librustc_trans/trans/glue.rs @@ -439,10 +439,12 @@ pub fn size_and_align_of_dst<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, t: Ty<'tcx>, in let dbloc = DebugLoc::None; - // #27023 FIXME: We should be adding any necessary padding + // FIXME (#26403, #27023): We should be adding padding // to `sized_size` (to accommodate the `unsized_align` // required of the unsized field that follows) before - // summing it with `sized_size`. + // summing it with `sized_size`. (Note that since #26403 + // is unfixed, we do not yet add the necessary padding + // here. But this is where the add would go.) // Return the sum of sizes and max of aligns. let mut size = Add(bcx, sized_size, unsized_size, dbloc); From 21be09448b21c25570f28ddd597f3d30ccc6fabf Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Wed, 29 Jul 2015 22:18:39 +0200 Subject: [PATCH 5/5] Improve code emitted for inserting padding before unsized field. Hat-tip to eddyb for the appropriate bit-trickery here. --- src/librustc_trans/trans/glue.rs | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/src/librustc_trans/trans/glue.rs b/src/librustc_trans/trans/glue.rs index 91b20f0b9ded0..18fedda49193c 100644 --- a/src/librustc_trans/trans/glue.rs +++ b/src/librustc_trans/trans/glue.rs @@ -475,21 +475,13 @@ pub fn size_and_align_of_dst<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, t: Ty<'tcx>, in // // `size + ((size & (align-1)) ? align : 0)` // - // Currently I am emulating the above via: + // emulated via the semi-standard fast bit trick: // - // `size + ((size & (align-1)) * align-(size & (align-1)))` - // - // because I am not sure which is cheaper between a branch - // or a multiply. - - let mask = Sub(bcx, align, C_uint(bcx.ccx(), 1_u64), dbloc); - let lowbits = And(bcx, size, mask, DebugLoc::None); - let nonzero = ICmp(bcx, llvm::IntNE, lowbits, C_uint(bcx.ccx(), 0_u64), dbloc); - let add_size = Mul(bcx, - ZExt(bcx, nonzero, Type::i64(bcx.ccx())), - Sub(bcx, align, lowbits, dbloc), - dbloc); - let size = Add(bcx, size, add_size, dbloc); + // `(size + (align-1)) & !align` + + let addend = Sub(bcx, align, C_uint(bcx.ccx(), 1_u64), dbloc); + let size = And( + bcx, Add(bcx, size, addend, dbloc), Neg(bcx, align, dbloc), dbloc); (size, align) }