Auto merge of #36904 - camlorn:field_offsets_refactor, r=eddyb

Refactor layout to store offsets of fields, not offsets after fields

This is the next PR moving us towards being able to reorder struct fields.

The old code implicitly stored the offset of the first field.  This is inadequate because the first field may no longer be offset 0 in future.  This PR refactors `layout` to use a `offsets` vector instead of a `offset_after_field` vector.

Author: bors

Diff for: src/librustc/ty/layout.rs

@@ -511,11 +511,11 @@ pub struct Struct {
     /// If true, the size is exact, otherwise it's only a lower bound.
     pub sized: bool,
 
-    /// Offsets for the first byte after each field.
-    /// That is, field_offset(i) = offset_after_field[i - 1] and the
-    /// whole structure's size is the last offset, excluding padding.
-    // FIXME(eddyb) use small vector optimization for the common case.
-    pub offset_after_field: Vec<Size>
+    /// Offsets for the first byte of each field.
+    /// FIXME(eddyb) use small vector optimization for the common case.
+    pub offsets: Vec<Size>,
+
+    pub min_size: Size,
 }
 
 impl<'a, 'gcx, 'tcx> Struct {
@@ -524,7 +524,8 @@ impl<'a, 'gcx, 'tcx> Struct {
             align: if packed { dl.i8_align } else { dl.aggregate_align },
             packed: packed,
             sized: true,
-            offset_after_field: vec![]
+            offsets: vec![],
+            min_size: Size::from_bytes(0),
         }
     }
 
@@ -534,12 +535,14 @@ impl<'a, 'gcx, 'tcx> Struct {
                      scapegoat: Ty<'gcx>)
                      -> Result<(), LayoutError<'gcx>>
     where I: Iterator<Item=Result<&'a Layout, LayoutError<'gcx>>> {
-        self.offset_after_field.reserve(fields.size_hint().0);
+        self.offsets.reserve(fields.size_hint().0);
+
+        let mut offset = self.min_size;
 
         for field in fields {
             if !self.sized {
                 bug!("Struct::extend: field #{} of `{}` comes after unsized field",
-                     self.offset_after_field.len(), scapegoat);
+                     self.offsets.len(), scapegoat);
             }
 
             let field = field?;
@@ -548,34 +551,29 @@ impl<'a, 'gcx, 'tcx> Struct {
             }
 
             // Invariant: offset < dl.obj_size_bound() <= 1<<61
-            let mut offset = if !self.packed {
+            if !self.packed {
                 let align = field.align(dl);
                 self.align = self.align.max(align);
-                self.offset_after_field.last_mut().map_or(Size::from_bytes(0), |last| {
-                    *last = last.abi_align(align);
-                    *last
-                })
-            } else {
-                self.offset_after_field.last().map_or(Size::from_bytes(0), |&last| last)
-            };
+                offset = offset.abi_align(align);
+            }
+
+            self.offsets.push(offset);
+
 
             offset = offset.checked_add(field.size(dl), dl)
                            .map_or(Err(LayoutError::SizeOverflow(scapegoat)), Ok)?;
-
-            self.offset_after_field.push(offset);
         }
 
+        self.min_size = offset;
+
         Ok(())
     }
 
     /// Get the size without trailing alignment padding.
-    pub fn min_size(&self) -> Size {
-        self.offset_after_field.last().map_or(Size::from_bytes(0), |&last| last)
-    }
 
     /// Get the size with trailing aligment padding.
     pub fn stride(&self) -> Size {
-        self.min_size().abi_align(self.align)
+        self.min_size.abi_align(self.align)
     }
 
     /// Determine whether a structure would be zero-sized, given its fields.
@@ -671,15 +669,6 @@ impl<'a, 'gcx, 'tcx> Struct {
         }
         Ok(None)
     }
-
-    pub fn offset_of_field(&self, index: usize) -> Size {
-        assert!(index < self.offset_after_field.len());
-        if index == 0 {
-            Size::from_bytes(0)
-        } else {
-            self.offset_after_field[index-1]
-        }
-    }
 }
 
 /// An untagged union.
@@ -1138,7 +1127,7 @@ impl<'a, 'gcx, 'tcx> Layout {
                     });
                     let mut st = Struct::new(dl, false);
                     st.extend(dl, discr.iter().map(Ok).chain(fields), ty)?;
-                    size = cmp::max(size, st.min_size());
+                    size = cmp::max(size, st.min_size);
                     align = align.max(st.align);
                     Ok(st)
                 }).collect::<Result<Vec<_>, _>>()?;
@@ -1171,12 +1160,16 @@ 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.offset_after_field {
+                        for offset in &mut variant.offsets[1..] {
                             if *offset > old_ity_size {
                                 break;
                             }
                             *offset = new_ity_size;
                         }
+                        // We might be making the struct larger.
+                        if variant.min_size <= old_ity_size {
+                            variant.min_size = new_ity_size;
+                        }
                     }
                 }
 

Diff for: src/librustc_lint/types.rs

@@ -738,7 +738,7 @@ impl LateLintPass for VariantSizeDifferences {
                         .zip(variants)
                         .map(|(variant, variant_layout)| {
                             // Subtract the size of the enum discriminant
-                            let bytes = variant_layout.min_size().bytes()
+                            let bytes = variant_layout.min_size.bytes()
                                                                  .saturating_sub(discr_size);
 
                             debug!("- variant `{}` is {} bytes large", variant.node.name, bytes);

Diff for: src/librustc_trans/adt.rs

@@ -632,7 +632,7 @@ fn struct_field_ptr<'blk, 'tcx>(bcx: &BlockAndBuilder<'blk, 'tcx>,
     let meta = val.meta;
 
 
-    let offset = st.offset_of_field(ix).bytes();
+    let offset = st.offsets[ix].bytes();
     let unaligned_offset = C_uint(bcx.ccx(), offset);
 
     // Get the alignment of the field
@@ -695,9 +695,9 @@ pub fn trans_const<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, t: Ty<'tcx>, discr: D
             let lldiscr = C_integral(Type::from_integer(ccx, d), discr.0 as u64, true);
             let mut vals_with_discr = vec![lldiscr];
             vals_with_discr.extend_from_slice(vals);
-            let mut contents = build_const_struct(ccx, &variant.offset_after_field[..],
-                &vals_with_discr[..], variant.packed);
-            let needed_padding = l.size(dl).bytes() - variant.min_size().bytes();
+            let mut contents = build_const_struct(ccx, &variant,
+                &vals_with_discr[..]);
+            let needed_padding = l.size(dl).bytes() - variant.min_size.bytes();
             if needed_padding > 0 {
                 contents.push(padding(ccx, needed_padding));
             }
@@ -711,7 +711,7 @@ pub fn trans_const<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, t: Ty<'tcx>, discr: D
         layout::Univariant { ref variant, .. } => {
             assert_eq!(discr, Disr(0));
             let contents = build_const_struct(ccx,
-                &variant.offset_after_field[..], vals, variant.packed);
+                &variant, vals);
             C_struct(ccx, &contents[..], variant.packed)
         }
         layout::Vector { .. } => {
@@ -728,9 +728,7 @@ pub fn trans_const<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, t: Ty<'tcx>, discr: D
         }
         layout::StructWrappedNullablePointer { ref nonnull, nndiscr, .. } => {
             if discr.0 == nndiscr {
-                C_struct(ccx, &build_const_struct(ccx,
-                                                 &nonnull.offset_after_field[..],
-                                                 vals, nonnull.packed),
+                C_struct(ccx, &build_const_struct(ccx, &nonnull, vals),
                          false)
             } else {
                 let fields = compute_fields(ccx, t, nndiscr as usize, false);
@@ -739,10 +737,7 @@ pub fn trans_const<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, t: Ty<'tcx>, discr: D
                     // field; see #8506.
                     C_null(type_of::sizing_type_of(ccx, ty))
                 }).collect::<Vec<ValueRef>>();
-                C_struct(ccx, &build_const_struct(ccx,
-                                                 &nonnull.offset_after_field[..],
-                                                 &vals[..],
-                                                 false),
+                C_struct(ccx, &build_const_struct(ccx, &nonnull, &vals[..]),
                          false)
             }
         }
@@ -759,11 +754,10 @@ pub fn trans_const<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, t: Ty<'tcx>, discr: D
 /// a two-element struct will locate it at offset 4, and accesses to it
 /// will read the wrong memory.
 fn build_const_struct<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
-                                offset_after_field: &[layout::Size],
-                                vals: &[ValueRef],
-                                packed: bool)
+                                st: &layout::Struct,
+                                vals: &[ValueRef])
                                 -> Vec<ValueRef> {
-    assert_eq!(vals.len(), offset_after_field.len());
+    assert_eq!(vals.len(), st.offsets.len());
 
     if vals.len() == 0 {
         return Vec::new();
@@ -772,24 +766,19 @@ fn build_const_struct<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
     // offset of current value
     let mut offset = 0;
     let mut cfields = Vec::new();
-    let target_offsets = offset_after_field.iter().map(|i| i.bytes());
-    for (&val, target_offset) in vals.iter().zip(target_offsets) {
-        assert!(!is_undef(val));
-        cfields.push(val);
-        offset += machine::llsize_of_alloc(ccx, val_ty(val));
-        if !packed {
-            let val_align = machine::llalign_of_min(ccx, val_ty(val));
-            offset = roundup(offset, val_align);
-        }
-        if offset != target_offset {
+    let offsets = st.offsets.iter().map(|i| i.bytes());
+    for (&val, target_offset) in vals.iter().zip(offsets) {
+        if offset < target_offset {
             cfields.push(padding(ccx, target_offset - offset));
             offset = target_offset;
         }
+        assert!(!is_undef(val));
+        cfields.push(val);
+        offset += machine::llsize_of_alloc(ccx, val_ty(val));
     }
 
-    let size = offset_after_field.last().unwrap();
-    if offset < size.bytes() {
-        cfields.push(padding(ccx, size.bytes() - offset));
+    if offset < st.min_size.bytes() {
+        cfields.push(padding(ccx, st.min_size.bytes() - offset));
     }
 
     cfields

Diff for: src/librustc_trans/common.rs

@@ -127,7 +127,7 @@ pub fn type_is_imm_pair<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, ty: Ty<'tcx>)
         Layout::FatPointer { .. } => true,
         Layout::Univariant { ref variant, .. } => {
             // There must be only 2 fields.
-            if variant.offset_after_field.len() != 2 {
+            if variant.offsets.len() != 2 {
                 return false;
             }
 

Diff for: src/librustc_trans/glue.rs

@@ -335,20 +335,9 @@ pub fn size_and_align_of_dst<'blk, 'tcx>(bcx: &BlockAndBuilder<'blk, 'tcx>,
             let layout = ccx.layout_of(t);
             debug!("DST {} layout: {:?}", t, layout);
 
-            // Returns size in bytes of all fields except the last one
-            // (we will be recursing on the last one).
-            fn local_prefix_bytes(variant: &ty::layout::Struct) -> u64 {
-                let fields = variant.offset_after_field.len();
-                if fields > 1 {
-                    variant.offset_after_field[fields - 2].bytes()
-                } else {
-                    0
-                }
-            }
-
             let (sized_size, sized_align) = match *layout {
                 ty::layout::Layout::Univariant { ref variant, .. } => {
-                    (local_prefix_bytes(variant), variant.align.abi())
+                    (variant.offsets.last().map_or(0, |o| o.bytes()), variant.align.abi())
                 }
                 _ => {
                     bug!("size_and_align_of_dst: expcted Univariant for `{}`, found {:#?}",