Skip to content

Commit efdbc0e

Browse files
committedAug 4, 2015
Auto merge of #27351 - pnkfelix:dst-size-and-align-issue-27023, r=nikomatsakis
Change the behavior of the glue code emitted for `size_and_align_of_dst`. This thus changes the behavior of `std::mem::size_of_val` and `std::mem::align_of_val`. It tries to move us towards a world where the following property holds: Given type `T` implements `Trait` and a value `b: Box<T>`, where `std::mem::size_of::<T>()` returns `k`, then: * `std::mem::size_of_val(b)` returns `k` * `std::mem::size_of_val(b as Box<Trait>)` returns `k` Note that one might legitimately question whether the above property *should* hold. The property certainly does not hold today, as illustrated by #27023. (A follow-up task is to make various tests that check that the above property holds for a wide variety of types ... I chose not to invest effort in writing such a test before we actually determine that the above property is desirable.) nmatsakis and pnkfelix agree that this PR does not require an RFC. cc @rust-lang/lang (since others may disagree). (It also *might* break code, though it is hard for me to imagine that it could break code that wasn't already going to assert-fail when run in e.g. debug builds...) Fix issue #27023 Also, this (or something like it) is a prerequisite for *fixing`make check` on `--enable-optimize --enable-debug` builds*
2 parents 6afb8f5 + 21be094 commit efdbc0e

File tree

3 files changed

+154
-28
lines changed

3 files changed

+154
-28
lines changed
 

‎src/librustc_trans/trans/adt.rs

+94-17
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,31 @@ use trans::type_of;
6767

6868
type Hint = attr::ReprAttr;
6969

70+
// Representation of the context surrounding an unsized type. I want
71+
// to be able to track the drop flags that are injected by trans.
72+
#[derive(Clone, Copy, PartialEq, Debug)]
73+
pub struct TypeContext {
74+
prefix: Type,
75+
needs_drop_flag: bool,
76+
}
77+
78+
impl TypeContext {
79+
pub fn prefix(&self) -> Type { self.prefix }
80+
pub fn needs_drop_flag(&self) -> bool { self.needs_drop_flag }
81+
82+
fn direct(t: Type) -> TypeContext {
83+
TypeContext { prefix: t, needs_drop_flag: false }
84+
}
85+
fn may_need_drop_flag(t: Type, needs_drop_flag: bool) -> TypeContext {
86+
TypeContext { prefix: t, needs_drop_flag: needs_drop_flag }
87+
}
88+
pub fn to_string(self) -> String {
89+
let TypeContext { prefix, needs_drop_flag } = self;
90+
format!("TypeContext {{ prefix: {}, needs_drop_flag: {} }}",
91+
prefix.to_string(), needs_drop_flag)
92+
}
93+
}
94+
7095
/// Representations.
7196
#[derive(Eq, PartialEq, Debug)]
7297
pub enum Repr<'tcx> {
@@ -125,7 +150,7 @@ pub struct Struct<'tcx> {
125150
pub align: u32,
126151
pub sized: bool,
127152
pub packed: bool,
128-
pub fields: Vec<Ty<'tcx>>
153+
pub fields: Vec<Ty<'tcx>>,
129154
}
130155

131156
/// 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>,
681706
/// and fill in the actual contents in a second pass to prevent
682707
/// unbounded recursion; see also the comments in `trans::type_of`.
683708
pub fn type_of<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>, r: &Repr<'tcx>) -> Type {
684-
generic_type_of(cx, r, None, false, false)
709+
let c = generic_type_of(cx, r, None, false, false, false);
710+
assert!(!c.needs_drop_flag);
711+
c.prefix
685712
}
713+
714+
686715
// Pass dst=true if the type you are passing is a DST. Yes, we could figure
687716
// this out, but if you call this on an unsized type without realising it, you
688717
// are going to get the wrong type (it will not include the unsized parts of it).
689718
pub fn sizing_type_of<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>,
690719
r: &Repr<'tcx>, dst: bool) -> Type {
691-
generic_type_of(cx, r, None, true, dst)
720+
let c = generic_type_of(cx, r, None, true, dst, false);
721+
assert!(!c.needs_drop_flag);
722+
c.prefix
723+
}
724+
pub fn sizing_type_context_of<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>,
725+
r: &Repr<'tcx>, dst: bool) -> TypeContext {
726+
generic_type_of(cx, r, None, true, dst, true)
692727
}
693728
pub fn incomplete_type_of<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>,
694729
r: &Repr<'tcx>, name: &str) -> Type {
695-
generic_type_of(cx, r, Some(name), false, false)
730+
let c = generic_type_of(cx, r, Some(name), false, false, false);
731+
assert!(!c.needs_drop_flag);
732+
c.prefix
696733
}
697734
pub fn finish_type_of<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>,
698735
r: &Repr<'tcx>, llty: &mut Type) {
@@ -708,20 +745,50 @@ fn generic_type_of<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>,
708745
r: &Repr<'tcx>,
709746
name: Option<&str>,
710747
sizing: bool,
711-
dst: bool) -> Type {
748+
dst: bool,
749+
delay_drop_flag: bool) -> TypeContext {
750+
debug!("adt::generic_type_of r: {:?} name: {:?} sizing: {} dst: {} delay_drop_flag: {}",
751+
r, name, sizing, dst, delay_drop_flag);
712752
match *r {
713-
CEnum(ity, _, _) => ll_inttype(cx, ity),
714-
RawNullablePointer { nnty, .. } => type_of::sizing_type_of(cx, nnty),
715-
Univariant(ref st, _) | StructWrappedNullablePointer { nonnull: ref st, .. } => {
753+
CEnum(ity, _, _) => TypeContext::direct(ll_inttype(cx, ity)),
754+
RawNullablePointer { nnty, .. } =>
755+
TypeContext::direct(type_of::sizing_type_of(cx, nnty)),
756+
StructWrappedNullablePointer { nonnull: ref st, .. } => {
757+
match name {
758+
None => {
759+
TypeContext::direct(
760+
Type::struct_(cx, &struct_llfields(cx, st, sizing, dst),
761+
st.packed))
762+
}
763+
Some(name) => {
764+
assert_eq!(sizing, false);
765+
TypeContext::direct(Type::named_struct(cx, name))
766+
}
767+
}
768+
}
769+
Univariant(ref st, dtor_needed) => {
770+
let dtor_needed = dtor_needed != 0;
716771
match name {
717772
None => {
718-
Type::struct_(cx, &struct_llfields(cx, st, sizing, dst),
719-
st.packed)
773+
let mut fields = struct_llfields(cx, st, sizing, dst);
774+
if delay_drop_flag && dtor_needed {
775+
fields.pop();
776+
}
777+
TypeContext::may_need_drop_flag(
778+
Type::struct_(cx, &fields,
779+
st.packed),
780+
delay_drop_flag && dtor_needed)
781+
}
782+
Some(name) => {
783+
// Hypothesis: named_struct's can never need a
784+
// drop flag. (... needs validation.)
785+
assert_eq!(sizing, false);
786+
TypeContext::direct(Type::named_struct(cx, name))
720787
}
721-
Some(name) => { assert_eq!(sizing, false); Type::named_struct(cx, name) }
722788
}
723789
}
724-
General(ity, ref sts, _) => {
790+
General(ity, ref sts, dtor_needed) => {
791+
let dtor_needed = dtor_needed != 0;
725792
// We need a representation that has:
726793
// * The alignment of the most-aligned field
727794
// * 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>,
753820
};
754821
assert_eq!(machine::llalign_of_min(cx, fill_ty), align);
755822
assert_eq!(align_s % discr_size, 0);
756-
let fields = [discr_ty,
757-
Type::array(&discr_ty, align_s / discr_size - 1),
758-
fill_ty];
823+
let mut fields: Vec<Type> =
824+
[discr_ty,
825+
Type::array(&discr_ty, align_s / discr_size - 1),
826+
fill_ty].iter().cloned().collect();
827+
if delay_drop_flag && dtor_needed {
828+
fields.pop();
829+
}
759830
match name {
760-
None => Type::struct_(cx, &fields[..], false),
831+
None => {
832+
TypeContext::may_need_drop_flag(
833+
Type::struct_(cx, &fields[..], false),
834+
delay_drop_flag && dtor_needed)
835+
}
761836
Some(name) => {
762837
let mut llty = Type::named_struct(cx, name);
763838
llty.set_struct_body(&fields[..], false);
764-
llty
839+
TypeContext::may_need_drop_flag(
840+
llty,
841+
delay_drop_flag && dtor_needed)
765842
}
766843
}
767844
}

‎src/librustc_trans/trans/glue.rs

+53-8
Original file line numberDiff line numberDiff line change
@@ -406,8 +406,12 @@ pub fn size_and_align_of_dst<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, t: Ty<'tcx>, in
406406
t, bcx.val_to_string(info));
407407
if type_is_sized(bcx.tcx(), t) {
408408
let sizing_type = sizing_type_of(bcx.ccx(), t);
409-
let size = C_uint(bcx.ccx(), llsize_of_alloc(bcx.ccx(), sizing_type));
410-
let align = C_uint(bcx.ccx(), align_of(bcx.ccx(), t));
409+
let size = llsize_of_alloc(bcx.ccx(), sizing_type);
410+
let align = align_of(bcx.ccx(), t);
411+
debug!("size_and_align_of_dst t={} info={} size: {} align: {}",
412+
t, bcx.val_to_string(info), size, align);
413+
let size = C_uint(bcx.ccx(), size);
414+
let align = C_uint(bcx.ccx(), align);
411415
return (size, align);
412416
}
413417
match t.sty {
@@ -417,9 +421,14 @@ pub fn size_and_align_of_dst<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, t: Ty<'tcx>, in
417421
// Don't use type_of::sizing_type_of because that expects t to be sized.
418422
assert!(!t.is_simd(bcx.tcx()));
419423
let repr = adt::represent_type(ccx, t);
420-
let sizing_type = adt::sizing_type_of(ccx, &*repr, true);
421-
let sized_size = C_uint(ccx, llsize_of_alloc(ccx, sizing_type));
422-
let sized_align = C_uint(ccx, llalign_of_min(ccx, sizing_type));
424+
let sizing_type = adt::sizing_type_context_of(ccx, &*repr, true);
425+
debug!("DST {} sizing_type: {}", t, sizing_type.to_string());
426+
let sized_size = llsize_of_alloc(ccx, sizing_type.prefix());
427+
let sized_align = llalign_of_min(ccx, sizing_type.prefix());
428+
debug!("DST {} statically sized prefix size: {} align: {}",
429+
t, sized_size, sized_align);
430+
let sized_size = C_uint(ccx, sized_size);
431+
let sized_align = C_uint(ccx, sized_align);
423432

424433
// Recurse to get the size of the dynamically sized field (must be
425434
// the last field).
@@ -428,16 +437,52 @@ pub fn size_and_align_of_dst<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, t: Ty<'tcx>, in
428437
let field_ty = last_field.mt.ty;
429438
let (unsized_size, unsized_align) = size_and_align_of_dst(bcx, field_ty, info);
430439

440+
let dbloc = DebugLoc::None;
441+
442+
// FIXME (#26403, #27023): We should be adding padding
443+
// to `sized_size` (to accommodate the `unsized_align`
444+
// required of the unsized field that follows) before
445+
// summing it with `sized_size`. (Note that since #26403
446+
// is unfixed, we do not yet add the necessary padding
447+
// here. But this is where the add would go.)
448+
431449
// Return the sum of sizes and max of aligns.
432-
let size = Add(bcx, sized_size, unsized_size, DebugLoc::None);
450+
let mut size = Add(bcx, sized_size, unsized_size, dbloc);
451+
452+
// Issue #27023: If there is a drop flag, *now* we add 1
453+
// to the size. (We can do this without adding any
454+
// padding because drop flags do not have any alignment
455+
// constraints.)
456+
if sizing_type.needs_drop_flag() {
457+
size = Add(bcx, size, C_uint(bcx.ccx(), 1_u64), dbloc);
458+
}
459+
460+
// Choose max of two known alignments (combined value must
461+
// be aligned according to more restrictive of the two).
433462
let align = Select(bcx,
434463
ICmp(bcx,
435-
llvm::IntULT,
464+
llvm::IntUGT,
436465
sized_align,
437466
unsized_align,
438-
DebugLoc::None),
467+
dbloc),
439468
sized_align,
440469
unsized_align);
470+
471+
// Issue #27023: must add any necessary padding to `size`
472+
// (to make it a multiple of `align`) before returning it.
473+
//
474+
// Namely, the returned size should be, in C notation:
475+
//
476+
// `size + ((size & (align-1)) ? align : 0)`
477+
//
478+
// emulated via the semi-standard fast bit trick:
479+
//
480+
// `(size + (align-1)) & !align`
481+
482+
let addend = Sub(bcx, align, C_uint(bcx.ccx(), 1_u64), dbloc);
483+
let size = And(
484+
bcx, Add(bcx, size, addend, dbloc), Neg(bcx, align, dbloc), dbloc);
485+
441486
(size, align)
442487
}
443488
ty::TyTrait(..) => {

‎src/librustc_trans/trans/type_.rs

+7-3
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,12 @@ impl Type {
5050
self.rf
5151
}
5252

53+
pub fn to_string(self: Type) -> String {
54+
llvm::build_string(|s| unsafe {
55+
llvm::LLVMWriteTypeToString(self.to_ref(), s);
56+
}).expect("non-UTF8 type description from LLVM")
57+
}
58+
5359
pub fn void(ccx: &CrateContext) -> Type {
5460
ty!(llvm::LLVMVoidTypeInContext(ccx.llcx()))
5561
}
@@ -315,9 +321,7 @@ impl TypeNames {
315321
}
316322

317323
pub fn type_to_string(&self, ty: Type) -> String {
318-
llvm::build_string(|s| unsafe {
319-
llvm::LLVMWriteTypeToString(ty.to_ref(), s);
320-
}).expect("non-UTF8 type description from LLVM")
324+
ty.to_string()
321325
}
322326

323327
pub fn types_to_str(&self, tys: &[Type]) -> String {

0 commit comments

Comments
 (0)
Please sign in to comment.