[MS-ABI] Update to vtordisp computation

A portion of the vtordisp computation that was previously unguarded by a 
test for the declaration of user defined constructors/destructors was 
erroniously adding vtordisps to things that shouldn't have them.  This 
patch correctly guards that codepath.  In addition, it updates the 
comments to make them more clear.  Test case is included.

llvm-svn: 206077

clang/lib/AST/RecordLayoutBuilder.cpp

@@ -2655,16 +2655,20 @@ void MicrosoftRecordLayoutBuilder::finalizeLayout(const RecordDecl *RD) {
   }
 }
 
-static bool
+// Recursively walks the non-virtual bases of a class and determines if any of
-RequiresVtordisp(const llvm::SmallPtrSet<const CXXRecordDecl *, 2> &HasVtordisp,
+// them are in the bases with overridden methods set.
+static bool RequiresVtordisp(
+    const llvm::SmallPtrSet<const CXXRecordDecl *, 2> &
+        BasesWithOverriddenMethods,
     const CXXRecordDecl *RD) {
-  if (HasVtordisp.count(RD))
+  if (BasesWithOverriddenMethods.count(RD))
     return true;
   // If any of a virtual bases non-virtual bases (recursively) requires a
   // vtordisp than so does this virtual base.
   for (const auto &I : RD->bases())
     if (!I.isVirtual() &&
-        RequiresVtordisp(HasVtordisp, I.getType()->getAsCXXRecordDecl()))
+        RequiresVtordisp(BasesWithOverriddenMethods,
+                         I.getType()->getAsCXXRecordDecl()))
       return true;
   return false;
 }
@@ -2703,15 +2707,16 @@ MicrosoftRecordLayoutBuilder::computeVtorDispSet(const CXXRecordDecl *RD) {
       if (bi.second.hasVtorDisp())
         HasVtordispSet.insert(bi.first);
   }
-  // If we define a constructor or destructor and override a function that is
+  // If we do not have a user declared constructor or destructor then we don't
-  // defined in a virtual base's vtable, that virtual bases need a vtordisp.
+  // introduce any additional vtordisps.
-  // Here we collect a list of classes with vtables for which our virtual bases
+  if (!RD->hasUserDeclaredConstructor() && !RD->hasUserDeclaredDestructor())
-  // actually live.  The virtual bases with this property will require
+    return HasVtordispSet;
-  // vtordisps.  In addition, virtual bases that contain non-virtual bases that
+  // Compute a set of base classes which define methods we override.  A virtual
-  // define functions we override also require vtordisps, this case is checked
+  // base in this set will require a vtordisp.  A virtual base that transitively
-  // explicitly below.
+  // contains one of these bases as a non-virtual base will also require a
-  if (RD->hasUserDeclaredConstructor() || RD->hasUserDeclaredDestructor()) {
+  // vtordisp.
   llvm::SmallPtrSet<const CXXMethodDecl *, 8> Work;
+  llvm::SmallPtrSet<const CXXRecordDecl *, 2> BasesWithOverriddenMethods;
   // Seed the working set with our non-destructor virtual methods.
   for (const auto *I : RD->methods())
     if (I->isVirtual() && !isa<CXXDestructorDecl>(I))
@@ -2720,21 +2725,20 @@ MicrosoftRecordLayoutBuilder::computeVtorDispSet(const CXXRecordDecl *RD) {
     const CXXMethodDecl *MD = *Work.begin();
     CXXMethodDecl::method_iterator i = MD->begin_overridden_methods(),
                                    e = MD->end_overridden_methods();
-      if (i == e)
     // If a virtual method has no-overrides it lives in its parent's vtable.
-        HasVtordispSet.insert(MD->getParent());
+    if (i == e)
+      BasesWithOverriddenMethods.insert(MD->getParent());
     else
       Work.insert(i, e);
     // We've finished processing this element, remove it from the working set.
     Work.erase(MD);
   }
-  }
+  // For each of our virtual bases, check if it is in the set of overridden
-  // Re-check all of our vbases for vtordisp requirements (in case their
+  // bases or if it transitively contains a non-virtual base that is.
-  // non-virtual bases have vtordisp requirements).
   for (const auto &I : RD->vbases()) {
     const CXXRecordDecl *BaseDecl =  I.getType()->getAsCXXRecordDecl();
     if (!HasVtordispSet.count(BaseDecl) &&
-        RequiresVtordisp(HasVtordispSet, BaseDecl))
+        RequiresVtordisp(BasesWithOverriddenMethods, BaseDecl))
       HasVtordispSet.insert(BaseDecl);
   }
   return HasVtordispSet;

clang/test/Layout/ms-x86-vtordisp.cpp

@@ -234,6 +234,9 @@ struct C : virtual B { int c; };
 // CHECK-NEXT:   24 |     int b
 // CHECK-NEXT:      | [sizeof=28, align=4
 // CHECK-NEXT:      |  nvsize=8, nvalign=4]
+// CHECK-X64: *** Dumping AST Record Layout
+// CHECK-X64: *** Dumping AST Record Layout
+// CHECK-X64: *** Dumping AST Record Layout
 }
 
 namespace pragma_test2 {
@@ -260,6 +263,9 @@ struct C : virtual B { int c; };
 // CHECK-NEXT:   32 |     int b
 // CHECK-NEXT:      | [sizeof=36, align=4
 // CHECK-NEXT:      |  nvsize=8, nvalign=4]
+// CHECK-X64: *** Dumping AST Record Layout
+// CHECK-X64: *** Dumping AST Record Layout
+// CHECK-X64: *** Dumping AST Record Layout
 }
 
 namespace pragma_test3 {
@@ -284,6 +290,9 @@ struct C : virtual B { int c; };
 // CHECK-NEXT:   24 |     int b
 // CHECK-NEXT:      | [sizeof=28, align=4
 // CHECK-NEXT:      |  nvsize=8, nvalign=4]
+// CHECK-X64: *** Dumping AST Record Layout
+// CHECK-X64: *** Dumping AST Record Layout
+// CHECK-X64: *** Dumping AST Record Layout
 }
 
 namespace pragma_test4 {
@@ -324,8 +333,54 @@ struct C : virtual B<int> { int c; };
 // CHECK-NEXT:   28 |     int b
 // CHECK-NEXT:      | [sizeof=32, align=4
 // CHECK-NEXT:      |  nvsize=8, nvalign=4]
+// CHECK-X64: *** Dumping AST Record Layout
+// CHECK-X64: *** Dumping AST Record Layout
+// CHECK-X64: *** Dumping AST Record Layout
 }
 
+struct GA {
+	virtual void fun() {}
+};
+struct GB: public GA {};
+struct GC: public virtual GA {
+	virtual void fun() {}
+	GC() {}
+};
+struct GD: public virtual GC, public virtual GB {};
+
+// CHECK: *** Dumping AST Record Layout
+// CHECK: *** Dumping AST Record Layout
+// CHECK: *** Dumping AST Record Layout
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT:    0 | struct GD
+// CHECK-NEXT:    0 |   (GD vbtable pointer)
+// CHECK-NEXT:    4 |   (vtordisp for vbase GA)
+// CHECK-NEXT:    8 |   struct GA (virtual base)
+// CHECK-NEXT:    8 |     (GA vftable pointer)
+// CHECK-NEXT:   12 |   struct GC (virtual base)
+// CHECK-NEXT:   12 |     (GC vbtable pointer)
+// CHECK-NEXT:   16 |   struct GB (virtual base)
+// CHECK-NEXT:   16 |     struct GA (primary base)
+// CHECK-NEXT:   16 |       (GA vftable pointer)
+// CHECK-NEXT:      | [sizeof=20, align=4
+// CHECK-NEXT:      |  nvsize=4, nvalign=4]
+// CHECK-X64: *** Dumping AST Record Layout
+// CHECK-X64: *** Dumping AST Record Layout
+// CHECK-X64: *** Dumping AST Record Layout
+// CHECK-X64: *** Dumping AST Record Layout
+// CHECK-X64-NEXT:    0 | struct GD
+// CHECK-X64-NEXT:    0 |   (GD vbtable pointer)
+// CHECK-X64-NEXT:   12 |   (vtordisp for vbase GA)
+// CHECK-X64-NEXT:   16 |   struct GA (virtual base)
+// CHECK-X64-NEXT:   16 |     (GA vftable pointer)
+// CHECK-X64-NEXT:   24 |   struct GC (virtual base)
+// CHECK-X64-NEXT:   24 |     (GC vbtable pointer)
+// CHECK-X64-NEXT:   32 |   struct GB (virtual base)
+// CHECK-X64-NEXT:   32 |     struct GA (primary base)
+// CHECK-X64-NEXT:   32 |       (GA vftable pointer)
+// CHECK-X64-NEXT:      | [sizeof=40, align=8
+// CHECK-X64-NEXT:      |  nvsize=8, nvalign=8]
+
 int a[
 sizeof(A)+
 sizeof(C)+
@@ -335,4 +390,6 @@ sizeof(XC)+
 sizeof(pragma_test1::C)+
 sizeof(pragma_test2::C)+
 sizeof(pragma_test3::C)+
-sizeof(pragma_test4::C)];
+sizeof(pragma_test4::C)+
+sizeof(GD)+
+0];