[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.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@206077 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/AST/RecordLayoutBuilder.cpp b/lib/AST/RecordLayoutBuilder.cpp
index c88423371a85ed753be026af6443352996d4b860..73267f70a6a5ea1c61d25172f852da9e9bed50b9 100644 (file)
--- a/lib/AST/RecordLayoutBuilder.cpp
+++ b/lib/AST/RecordLayoutBuilder.cpp
@@ -2655,16 +2655,20 @@ void MicrosoftRecordLayoutBuilder::finalizeLayout(const RecordDecl *RD) {
   }
 }
 
-static bool
-RequiresVtordisp(const llvm::SmallPtrSet<const CXXRecordDecl *, 2> &HasVtordisp,
-                 const CXXRecordDecl *RD) {
-  if (HasVtordisp.count(RD))
+// Recursively walks the non-virtual bases of a class and determines if any of
+// them are in the bases with overridden methods set.
+static bool RequiresVtordisp(
+    const llvm::SmallPtrSet<const CXXRecordDecl *, 2> &
+        BasesWithOverriddenMethods,
+    const CXXRecordDecl *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,38 +2707,38 @@ 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
-  // defined in a virtual base's vtable, that virtual bases need a vtordisp.
-  // Here we collect a list of classes with vtables for which our virtual bases
-  // actually live.  The virtual bases with this property will require
-  // vtordisps.  In addition, virtual bases that contain non-virtual bases that
-  // define functions we override also require vtordisps, this case is checked
-  // explicitly below.
-  if (RD->hasUserDeclaredConstructor() || RD->hasUserDeclaredDestructor()) {
-    llvm::SmallPtrSet<const CXXMethodDecl *, 8> Work;
-    // Seed the working set with our non-destructor virtual methods.
-    for (const auto *I : RD->methods())
-      if (I->isVirtual() && !isa<CXXDestructorDecl>(I))
-        Work.insert(I);
-    while (!Work.empty()) {
-      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());
-      else
-        Work.insert(i, e);
-      // We've finished processing this element, remove it from the working set.
-      Work.erase(MD);
-    }
+  // If we do not have a user declared constructor or destructor then we don't
+  // introduce any additional vtordisps.
+  if (!RD->hasUserDeclaredConstructor() && !RD->hasUserDeclaredDestructor())
+    return HasVtordispSet;
+  // Compute a set of base classes which define methods we override.  A virtual
+  // base in this set will require a vtordisp.  A virtual base that transitively
+  // contains one of these bases as a non-virtual base will also require a
+  // 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))
+      Work.insert(I);
+  while (!Work.empty()) {
+    const CXXMethodDecl *MD = *Work.begin();
+    CXXMethodDecl::method_iterator i = MD->begin_overridden_methods(),
+                                   e = MD->end_overridden_methods();
+    // If a virtual method has no-overrides it lives in its parent's vtable.
+    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);
   }
-  // Re-check all of our vbases for vtordisp requirements (in case their
-  // non-virtual bases have vtordisp requirements).
+  // For each of our virtual bases, check if it is in the set of overridden
+  // bases or if it transitively contains a non-virtual base that is.
   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;

diff --git a/test/Layout/ms-x86-vtordisp.cpp b/test/Layout/ms-x86-vtordisp.cpp
index 1f03b4ad44d431a95abff841e53785ecf2d45e6f..1619d1cb3bbb50bc0b038af9fd381a0bd46a4624 100644 (file)
--- a/test/Layout/ms-x86-vtordisp.cpp
+++ b/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];