Auto merge of #1084 - fitzgen:explicit-vtable-pointer-refactor, r=fitzgen

Explicit vtable pointer refactor

r? @emilio

This should make it easier to move padding into its own pass, rather than inside codegen, which should in turn let us start handling (or at least intelligently determining when we *can't* handle) `#pragma pack(..)` and other things that affect layout in exotic ways that we can only indirectly observe.

See each commit for details.

The reason for the first commit is this: when we compare, we rustfmt both expected and actual, so the expectations don't get updated to be formatted nicely until some patch that changes what gets generated. This is annoying, however, when debugging some minor difference, and not being able to see what it is easily. Best to just bite the bullet and format all the expectations the once and make the problem go away.

Author: bors-servo

src/codegen/mod.rs

@@ -1424,7 +1424,7 @@ impl CodeGenerator for CompInfo {
         //
         // FIXME: Once we generate proper vtables, we need to codegen the
         // vtable, but *not* generate a field for it in the case that
-        // needs_explicit_vtable is false but has_vtable is true.
+        // HasVtable::has_vtable_ptr is false but HasVtable::has_vtable is true.
         //
         // Also, we need to generate the vtable in such a way it "inherits" from
         // the parent too.
@@ -1434,7 +1434,7 @@ impl CodeGenerator for CompInfo {
             StructLayoutTracker::new(ctx, self, &canonical_name);
 
         if !is_opaque {
-            if self.needs_explicit_vtable(ctx, item) {
+            if item.has_vtable_ptr(ctx) {
                 let vtable =
                     Vtable::new(item.id(), self.methods(), self.base_members());
                 vtable.codegen(ctx, result, item);

src/ir/analysis/has_vtable.rs

@@ -4,8 +4,66 @@ use super::{ConstrainResult, MonotoneFramework, generate_dependencies};
 use ir::context::{BindgenContext, ItemId};
 use ir::traversal::EdgeKind;
 use ir::ty::TypeKind;
+use std::cmp;
 use std::collections::HashMap;
-use std::collections::HashSet;
+use std::collections::hash_map::Entry;
+use std::ops;
+
+/// The result of the `HasVtableAnalysis` for an individual item.
+#[derive(Copy, Clone, Debug, PartialEq, Eq, Ord)]
+pub enum HasVtableResult {
+    /// The item has a vtable, but the actual vtable pointer is in a base
+    /// member.
+    BaseHasVtable,
+
+    /// The item has a vtable and the actual vtable pointer is within this item.
+    SelfHasVtable,
+
+    /// The item does not have a vtable pointer.
+    No
+}
+
+impl Default for HasVtableResult {
+    fn default() -> Self {
+        HasVtableResult::No
+    }
+}
+
+impl cmp::PartialOrd for HasVtableResult {
+    fn partial_cmp(&self, rhs: &Self) -> Option<cmp::Ordering> {
+        use self::HasVtableResult::*;
+
+        match (*self, *rhs) {
+            (x, y) if x == y => Some(cmp::Ordering::Equal),
+            (BaseHasVtable, _) => Some(cmp::Ordering::Greater),
+            (_, BaseHasVtable) => Some(cmp::Ordering::Less),
+            (SelfHasVtable, _) => Some(cmp::Ordering::Greater),
+            (_, SelfHasVtable) => Some(cmp::Ordering::Less),
+            _ => unreachable!(),
+        }
+    }
+}
+
+impl HasVtableResult {
+    /// Take the least upper bound of `self` and `rhs`.
+    pub fn join(self, rhs: Self) -> Self {
+        cmp::max(self, rhs)
+    }
+}
+
+impl ops::BitOr for HasVtableResult {
+    type Output = Self;
+
+    fn bitor(self, rhs: HasVtableResult) -> Self::Output {
+        self.join(rhs)
+    }
+}
+
+impl ops::BitOrAssign for HasVtableResult {
+    fn bitor_assign(&mut self, rhs: HasVtableResult) {
+        *self = self.join(rhs)
+    }
+}
 
 /// An analysis that finds for each IR item whether it has vtable or not
 ///
@@ -23,7 +81,7 @@ pub struct HasVtableAnalysis<'ctx> {
 
     // The incremental result of this analysis's computation. Everything in this
     // set definitely has a vtable.
-    have_vtable: HashSet<ItemId>,
+    have_vtable: HashMap<ItemId, HasVtableResult>,
 
     // Dependencies saying that if a key ItemId has been inserted into the
     // `have_vtable` set, then each of the ids in Vec<ItemId> need to be
@@ -47,26 +105,50 @@ impl<'ctx> HasVtableAnalysis<'ctx> {
         }
     }
 
-    fn insert<Id: Into<ItemId>>(&mut self, id: Id) -> ConstrainResult {
+    fn insert<Id: Into<ItemId>>(&mut self, id: Id, result: HasVtableResult) -> ConstrainResult {
+        if let HasVtableResult::No = result {
+            return ConstrainResult::Same;
+        }
+
         let id = id.into();
-        let was_not_already_in_set = self.have_vtable.insert(id);
-        assert!(
-            was_not_already_in_set,
-            "We shouldn't try and insert {:?} twice because if it was \
-             already in the set, `constrain` should have exited early.",
-            id
-        );
-        ConstrainResult::Changed
+        match self.have_vtable.entry(id) {
+            Entry::Occupied(mut entry) => {
+                if *entry.get() < result {
+                    entry.insert(result);
+                    ConstrainResult::Changed
+                } else {
+                    ConstrainResult::Same
+                }
+            }
+            Entry::Vacant(entry) => {
+                entry.insert(result);
+                ConstrainResult::Changed
+            }
+        }
+    }
+
+    fn forward<Id1, Id2>(&mut self, from: Id1, to: Id2) -> ConstrainResult
+    where
+        Id1: Into<ItemId>,
+        Id2: Into<ItemId>,
+    {
+        let from = from.into();
+        let to = to.into();
+
+        match self.have_vtable.get(&from).cloned() {
+            None => ConstrainResult::Same,
+            Some(r) => self.insert(to, r),
+        }
     }
 }
 
 impl<'ctx> MonotoneFramework for HasVtableAnalysis<'ctx> {
     type Node = ItemId;
     type Extra = &'ctx BindgenContext;
-    type Output = HashSet<ItemId>;
+    type Output = HashMap<ItemId, HasVtableResult>;
 
     fn new(ctx: &'ctx BindgenContext) -> HasVtableAnalysis<'ctx> {
-        let have_vtable = HashSet::new();
+        let have_vtable = HashMap::new();
         let dependencies = generate_dependencies(ctx, Self::consider_edge);
 
         HasVtableAnalysis {
@@ -81,11 +163,7 @@ impl<'ctx> MonotoneFramework for HasVtableAnalysis<'ctx> {
     }
 
     fn constrain(&mut self, id: ItemId) -> ConstrainResult {
-        if self.have_vtable.contains(&id) {
-            // We've already computed that this type has a vtable and that can't
-            // change.
-            return ConstrainResult::Same;
-        }
+        trace!("constrain {:?}", id);
 
         let item = self.ctx.resolve_item(id);
         let ty = match item.as_type() {
@@ -99,33 +177,32 @@ impl<'ctx> MonotoneFramework for HasVtableAnalysis<'ctx> {
             TypeKind::Alias(t) |
             TypeKind::ResolvedTypeRef(t) |
             TypeKind::Reference(t) => {
-                if self.have_vtable.contains(&t.into()) {
-                    self.insert(id)
-                } else {
-                    ConstrainResult::Same
-                }
+                trace!("    aliases and references forward to their inner type");
+                self.forward(t, id)
             }
 
             TypeKind::Comp(ref info) => {
+                trace!("    comp considers its own methods and bases");
+                let mut result = HasVtableResult::No;
+
                 if info.has_own_virtual_method() {
-                    return self.insert(id);
+                    trace!("    comp has its own virtual method");
+                    result |= HasVtableResult::SelfHasVtable;
                 }
+
                 let bases_has_vtable = info.base_members().iter().any(|base| {
-                    self.have_vtable.contains(&base.ty.into())
+                    trace!("    comp has a base with a vtable: {:?}", base);
+                    self.have_vtable.contains_key(&base.ty.into())
                 });
                 if bases_has_vtable {
-                    self.insert(id)
-                } else {
-                    ConstrainResult::Same
+                    result |= HasVtableResult::BaseHasVtable;
                 }
+
+                self.insert(id, result)
             }
 
             TypeKind::TemplateInstantiation(ref inst) => {
-                if self.have_vtable.contains(&inst.template_definition().into()) {
-                    self.insert(id)
-                } else {
-                    ConstrainResult::Same
-                }
+                self.forward(inst.template_definition(), id)
             }
 
             _ => ConstrainResult::Same,
@@ -145,8 +222,13 @@ impl<'ctx> MonotoneFramework for HasVtableAnalysis<'ctx> {
     }
 }
 
-impl<'ctx> From<HasVtableAnalysis<'ctx>> for HashSet<ItemId> {
+impl<'ctx> From<HasVtableAnalysis<'ctx>> for HashMap<ItemId, HasVtableResult> {
     fn from(analysis: HasVtableAnalysis<'ctx>) -> Self {
+        // We let the lack of an entry mean "No" to save space.
+        extra_assert!(analysis.have_vtable.values().all(|v| {
+            *v != HasVtableResult::No
+        }));
+
         analysis.have_vtable
     }
 }
@@ -160,4 +242,8 @@ impl<'ctx> From<HasVtableAnalysis<'ctx>> for HashSet<ItemId> {
 pub trait HasVtable {
     /// Return `true` if this thing has vtable, `false` otherwise.
     fn has_vtable(&self, ctx: &BindgenContext) -> bool;
+
+    /// Return `true` if this thing has an actual vtable pointer in itself, as
+    /// opposed to transitively in a base member.
+    fn has_vtable_ptr(&self, ctx: &BindgenContext) -> bool;
 }

src/ir/analysis/mod.rs

@@ -43,8 +43,7 @@ pub use self::template_params::UsedTemplateParameters;
 mod derive_debug;
 pub use self::derive_debug::CannotDeriveDebug;
 mod has_vtable;
-pub use self::has_vtable::HasVtable;
-pub use self::has_vtable::HasVtableAnalysis;
+pub use self::has_vtable::{HasVtable, HasVtableAnalysis, HasVtableResult};
 mod has_destructor;
 pub use self::has_destructor::HasDestructorAnalysis;
 mod derive_default;
@@ -64,6 +63,7 @@ use ir::context::{BindgenContext, ItemId};
 use ir::traversal::{EdgeKind, Trace};
 use std::collections::HashMap;
 use std::fmt;
+use std::ops;
 
 /// An analysis in the monotone framework.
 ///
@@ -125,6 +125,7 @@ pub trait MonotoneFramework: Sized + fmt::Debug {
 
 /// Whether an analysis's `constrain` function modified the incremental results
 /// or not.
+#[derive(Debug, Copy, Clone, PartialEq, Eq)]
 pub enum ConstrainResult {
     /// The incremental results were updated, and the fix-point computation
     /// should continue.
@@ -134,6 +135,30 @@ pub enum ConstrainResult {
     Same,
 }
 
+impl Default for ConstrainResult {
+    fn default() -> Self {
+        ConstrainResult::Same
+    }
+}
+
+impl ops::BitOr for ConstrainResult {
+    type Output = Self;
+
+    fn bitor(self, rhs: ConstrainResult) -> Self::Output {
+        if self == ConstrainResult::Changed || rhs == ConstrainResult::Changed {
+            ConstrainResult::Changed
+        } else {
+            ConstrainResult::Same
+        }
+    }
+}
+
+impl ops::BitOrAssign for ConstrainResult {
+    fn bitor_assign(&mut self, rhs: ConstrainResult) {
+        *self = *self | rhs;
+    }
+}
+
 /// Run an analysis in the monotone framework.
 pub fn analyze<Analysis>(extra: Analysis::Extra) -> Analysis::Output
 where

src/ir/comp.rs

@@ -1012,7 +1012,7 @@ impl CompInfo {
 
     /// Is this compound type unsized?
     pub fn is_unsized(&self, ctx: &BindgenContext, id: TypeId) -> bool {
-        !ctx.lookup_has_vtable(id) && self.fields().is_empty() &&
+        !id.has_vtable(ctx) && self.fields().is_empty() &&
             self.base_members.iter().all(|base| {
                 ctx.resolve_type(base.ty).canonical_type(ctx).is_unsized(
                     ctx,
@@ -1451,27 +1451,6 @@ impl CompInfo {
         self.packed
     }
 
-    /// Returns whether this type needs an explicit vtable because it has
-    /// virtual methods and none of its base classes has already a vtable.
-    pub fn needs_explicit_vtable(
-        &self,
-        ctx: &BindgenContext,
-        item: &Item,
-    ) -> bool {
-        item.has_vtable(ctx) && !self.base_members.iter().any(|base| {
-            // NB: Ideally, we could rely in all these types being `comp`, and
-            // life would be beautiful.
-            //
-            // Unfortunately, given the way we implement --match-pat, and also
-            // that you can inherit from templated types, we need to handle
-            // other cases here too.
-            ctx.resolve_type(base.ty)
-                .canonical_type(ctx)
-                .as_comp()
-                .map_or(false, |_| base.ty.has_vtable(ctx))
-        })
-    }
-
     /// Returns true if compound type has been forward declared
     pub fn is_forward_declaration(&self) -> bool {
         self.is_forward_declaration

src/ir/context.rs

@@ -3,8 +3,8 @@
 use super::analysis::{CannotDeriveCopy, CannotDeriveDebug,
                       CannotDeriveDefault, CannotDeriveHash,
                       CannotDerivePartialEqOrPartialOrd, HasTypeParameterInArray,
-                      HasVtableAnalysis, HasDestructorAnalysis, UsedTemplateParameters,
-                      HasFloat, analyze};
+                      HasVtableAnalysis, HasVtableResult, HasDestructorAnalysis,
+                      UsedTemplateParameters, HasFloat, analyze};
 use super::derive::{CanDeriveCopy, CanDeriveDebug, CanDeriveDefault,
                     CanDeriveHash, CanDerivePartialOrd, CanDeriveOrd,
                     CanDerivePartialEq, CanDeriveEq, CannotDeriveReason};
@@ -432,7 +432,7 @@ pub struct BindgenContext {
     ///
     /// Populated when we enter codegen by `compute_has_vtable`; always `None`
     /// before that and `Some` after.
-    have_vtable: Option<HashSet<ItemId>>,
+    have_vtable: Option<HashMap<ItemId, HasVtableResult>>,
 
     /// The set of (`ItemId's of`) types that has destructor.
     ///
@@ -1267,15 +1267,20 @@ impl BindgenContext {
     }
 
     /// Look up whether the item with `id` has vtable or not.
-    pub fn lookup_has_vtable(&self, id: TypeId) -> bool {
+    pub fn lookup_has_vtable(&self, id: TypeId) -> HasVtableResult {
         assert!(
             self.in_codegen_phase(),
             "We only compute vtables when we enter codegen"
         );
 
         // Look up the computed value for whether the item with `id` has a
         // vtable or not.
-        self.have_vtable.as_ref().unwrap().contains(&id.into())
+        self.have_vtable
+            .as_ref()
+            .unwrap()
+            .get(&id.into())
+            .cloned()
+            .unwrap_or(HasVtableResult::No)
     }
 
     /// Compute whether the type has a destructor.