[cfe-commits] r97303 - in /cfe/trunk: lib/CodeGen/CGVtable.cpp test/CodeGenCXX/vtable-layout.cpp

[Anders Carlsson]

Author: andersca
Date: Fri Feb 26 22:05:52 2010
New Revision: 97303

URL: http://llvm.org/viewvc/llvm-project?rev=97303&view=rev
Log:
Fix a bug where we were generating an unnecessary vtable for a virtual base that's already a primary virtual base.

Modified:
    cfe/trunk/lib/CodeGen/CGVtable.cpp
    cfe/trunk/test/CodeGenCXX/vtable-layout.cpp

Modified: cfe/trunk/lib/CodeGen/CGVtable.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGVtable.cpp?rev=97303&r1=97302&r2=97303&view=diff
==============================================================================
--- cfe/trunk/lib/CodeGen/CGVtable.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGVtable.cpp Fri Feb 26 22:05:52 2010
@@ -1220,7 +1220,12 @@
   /// LayoutSecondaryVtables - Layout the secondary vtables for the given base
   /// subobject.
   void LayoutSecondaryVtables(BaseSubobject Base);
-  
+
+  /// DeterminePrimaryVirtualBases - Determine the primary virtual bases in this
+  /// class hierarchy.
+  void DeterminePrimaryVirtualBases(const CXXRecordDecl *RD, 
+                                    VisitedVirtualBasesSetTy &VBases);
+
   /// LayoutVtablesForVirtualBases - Layout vtables for all virtual bases of the
   /// given base (excluding any primary bases).
   void LayoutVtablesForVirtualBases(const CXXRecordDecl *RD, 
@@ -1418,9 +1423,6 @@
         Context.getASTRecordLayout(MostDerivedClass);
       
       BaseOffset = MostDerivedClassLayout.getVBaseClassOffset(PrimaryBase);
-      
-      // Keep track of this primary virtual base.
-      PrimaryVirtualBases.insert(PrimaryBase);
     } else {
       assert(Layout.getBaseClassOffset(PrimaryBase) == 0 &&
              "Primary base should have a zero offset!");
@@ -1498,6 +1500,11 @@
                                       /*BaseIsVirtual=*/false);
   
   VisitedVirtualBasesSetTy VBases;
+  
+  // Determine the primary virtual bases.
+  DeterminePrimaryVirtualBases(MostDerivedClass, VBases);
+  VBases.clear();
+  
   LayoutVtablesForVirtualBases(MostDerivedClass, VBases);
 }
   
@@ -1592,15 +1599,37 @@
 }
 
 void
+VtableBuilder::DeterminePrimaryVirtualBases(const CXXRecordDecl *RD, 
+                                            VisitedVirtualBasesSetTy &VBases) {
+  const ASTRecordLayout &Layout = Context.getASTRecordLayout(RD);
+  
+  // Check if this base has a primary base.
+  if (const CXXRecordDecl *PrimaryBase = Layout.getPrimaryBase()) {
+    // Check if it's virtual
+    if (Layout.getPrimaryBaseWasVirtual())
+      PrimaryVirtualBases.insert(PrimaryBase);
+  }
+
+  // Traverse bases, looking for more primary virtual bases.
+  for (CXXRecordDecl::base_class_const_iterator I = RD->bases_begin(),
+       E = RD->bases_end(); I != E; ++I) {
+    const CXXRecordDecl *BaseDecl = 
+      cast<CXXRecordDecl>(I->getType()->getAs<RecordType>()->getDecl());
+
+    if (I->isVirtual() && !VBases.insert(BaseDecl))
+      continue;
+
+    DeterminePrimaryVirtualBases(BaseDecl, VBases);
+  }
+}
+
+void
 VtableBuilder::LayoutVtablesForVirtualBases(const CXXRecordDecl *RD, 
                                             VisitedVirtualBasesSetTy &VBases) {
   // Itanium C++ ABI 2.5.2:
   //   Then come the virtual base virtual tables, also in inheritance graph
   //   order, and again excluding primary bases (which share virtual tables with
   //   the classes for which they are primary).
-  const ASTRecordLayout &Layout = Context.getASTRecordLayout(RD);
-  const CXXRecordDecl *PrimaryBase = Layout.getPrimaryBase();
-
   for (CXXRecordDecl::base_class_const_iterator I = RD->bases_begin(),
        E = RD->bases_end(); I != E; ++I) {
     const CXXRecordDecl *BaseDecl = 
@@ -1609,8 +1638,7 @@
     // Check if this base needs a vtable. (If it's virtual, not a primary base
     // of some other class, and we haven't visited it before).
     if (I->isVirtual() && BaseDecl->isDynamicClass() && 
-        BaseDecl != PrimaryBase && !PrimaryVirtualBases.count(BaseDecl) && 
-        VBases.insert(BaseDecl)) {
+        !PrimaryVirtualBases.count(BaseDecl) && VBases.insert(BaseDecl)) {
       const ASTRecordLayout &MostDerivedClassLayout =
         Context.getASTRecordLayout(MostDerivedClass);
       uint64_t BaseOffset = 

Modified: cfe/trunk/test/CodeGenCXX/vtable-layout.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/vtable-layout.cpp?rev=97303&r1=97302&r2=97303&view=diff
==============================================================================
--- cfe/trunk/test/CodeGenCXX/vtable-layout.cpp (original)
+++ cfe/trunk/test/CodeGenCXX/vtable-layout.cpp Fri Feb 26 22:05:52 2010
@@ -539,3 +539,33 @@
 
 }
 
+namespace Test15 {
+
+// Test that we don't emit an extra vtable for B since it's a primary base of C.
+struct A { virtual void a(); };
+struct B { virtual void b(); };
+
+struct C : virtual B { };
+
+// CHECK:      Vtable for 'Test15::D' (11 entries).
+// CHECK-NEXT:    0 | vbase_offset (8)
+// CHECK-NEXT:    1 | vbase_offset (8)
+// CHECK-NEXT:    2 | offset_to_top (0)
+// CHECK-NEXT:    3 | Test15::D RTTI
+// CHECK-NEXT:        -- (Test15::A, 0) vtable address --
+// CHECK-NEXT:        -- (Test15::D, 0) vtable address --
+// CHECK-NEXT:    4 | void Test15::A::a()
+// CHECK-NEXT:    5 | void Test15::D::f()
+// CHECK-NEXT:    6 | vbase_offset (0)
+// CHECK-NEXT:    7 | vcall_offset (0)
+// CHECK-NEXT:    8 | offset_to_top (-8)
+// CHECK-NEXT:    9 | Test15::D RTTI
+// CHECK-NEXT:        -- (Test15::B, 8) vtable address --
+// CHECK-NEXT:        -- (Test15::C, 8) vtable address --
+// CHECK-NEXT:   10 | void Test15::B::b()
+struct D : A, virtual B, virtual C { 
+  virtual void f();
+};
+void D::f() { } 
+
+}