r235899 - [MS ABI] Use 'continue' instead of 'return false' where intended

[Reid Kleckner]

Author: rnk
Date: Mon Apr 27 12:19:49 2015
New Revision: 235899

URL: http://llvm.org/viewvc/llvm-project?rev=235899&view=rev
Log:
[MS ABI] Use 'continue' instead of 'return false' where intended

This was a bug in r218285 that prevented us from seeing subsequent
virtual bases in the class hierarchy, leading to crashes later.

Also add some comments to this function, now that we better understand
what it's trying to do.

Fixes PR21062 and PR21064.

Modified:
    cfe/trunk/lib/AST/VTableBuilder.cpp
    cfe/trunk/test/CodeGenCXX/microsoft-abi-vtables-virtual-inheritance.cpp

Modified: cfe/trunk/lib/AST/VTableBuilder.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/VTableBuilder.cpp?rev=235899&r1=235898&r2=235899&view=diff
==============================================================================
--- cfe/trunk/lib/AST/VTableBuilder.cpp (original)
+++ cfe/trunk/lib/AST/VTableBuilder.cpp Mon Apr 27 12:19:49 2015
@@ -3442,6 +3442,15 @@ MicrosoftVTableContext::~MicrosoftVTable
   llvm::DeleteContainerSeconds(VBaseInfo);
 }
 
+/// Find the full path of bases from the most derived class to the base class
+/// containing the vptr described by Info. Use depth-first search for this, but
+/// search non-virtual bases before virtual bases. This is important in cases
+/// like this where we need to find the path to a vbase that goes through an
+/// nvbase:
+///   struct A { virtual void f(); }
+///   struct B : virtual A { virtual void f(); };
+///   struct C : virtual A, B { virtual void f(); };
+/// The path to A's vftable in C should be 'C, B, A', not 'C, A'.
 static bool
 findPathForVPtr(ASTContext &Context, const ASTRecordLayout &MostDerivedLayout,
                 const CXXRecordDecl *RD, CharUnits Offset,
@@ -3454,9 +3463,9 @@ findPathForVPtr(ASTContext &Context, con
 
   const ASTRecordLayout &Layout = Context.getASTRecordLayout(RD);
 
-  // Recurse with non-virtual bases first.
-  // FIXME: Does this need to be in layout order? Virtual bases will be in base
-  // specifier order, which isn't necessarily layout order.
+  // Sort direct bases into non-virtual bases followed by virtual bases. This
+  // approximates layout order with the exception of classes that do not contain
+  // vptrs, and those don't affect our results.
   SmallVector<CXXBaseSpecifier, 4> Bases(RD->bases_begin(), RD->bases_end());
   std::stable_partition(Bases.begin(), Bases.end(),
                         [](CXXBaseSpecifier bs) { return !bs.isVirtual(); });
@@ -3468,7 +3477,7 @@ findPathForVPtr(ASTContext &Context, con
       NewOffset = Offset + Layout.getBaseClassOffset(Base);
     else {
       if (!VBasesSeen.insert(Base).second)
-        return false;
+        continue;
       NewOffset = MostDerivedLayout.getVBaseClassOffset(Base);
     }
     FullPath.push_back(Base);

Modified: cfe/trunk/test/CodeGenCXX/microsoft-abi-vtables-virtual-inheritance.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/microsoft-abi-vtables-virtual-inheritance.cpp?rev=235899&r1=235898&r2=235899&view=diff
==============================================================================
--- cfe/trunk/test/CodeGenCXX/microsoft-abi-vtables-virtual-inheritance.cpp (original)
+++ cfe/trunk/test/CodeGenCXX/microsoft-abi-vtables-virtual-inheritance.cpp Mon Apr 27 12:19:49 2015
@@ -807,3 +807,41 @@ C::C() {}
 // MANGLING-DAG: @"\01??_7C at pr21031_2@@6BA at 1@@" = {{.*}} constant [1 x i8*]
 // MANGLING-DAG: @"\01??_7C at pr21031_2@@6BB at 1@@" = {{.*}} constant [1 x i8*]
 }
+
+namespace pr21062_1 {
+struct A { virtual void f(); };
+struct B {};
+struct C : virtual B {};
+struct D : virtual C, virtual B, virtual A { D();};
+D::D() {}
+
+// CHECK-LABEL: VFTable for 'pr21062_1::A' in 'pr21062_1::D' (1 entry)
+// CHECK-NEXT:   0 | void pr21062_1::A::f()
+
+// MANGLING-DAG: @"\01??_7D at pr21062_1@@6B@" = {{.*}} constant [1 x i8*]
+}
+
+namespace pr21062_2 {
+struct A { virtual void f(); };
+struct B {};
+struct C : virtual B {};
+struct D : C, virtual B, virtual A { D(); };
+D::D() {}
+
+// CHECK-LABEL: VFTable for 'pr21062_2::A' in 'pr21062_2::D' (1 entry)
+// CHECK-NEXT:   0 | void pr21062_2::A::f()
+
+// MANGLING-DAG: @"\01??_7D at pr21062_2@@6B@" = {{.*}} constant [1 x i8*]
+}
+
+namespace pr21064 {
+struct A {};
+struct B { virtual void f(); };
+struct C : virtual A, virtual B {};
+struct D : virtual A, virtual C { D(); };
+D::D() {}
+// CHECK-LABEL: VFTable for 'pr21064::B' in 'pr21064::C' in 'pr21064::D' (1 entry)
+// CHECK-NEXT:   0 | void pr21064::B::f()
+
+// MANGLING-DAG: @"\01??_7D at pr21064@@6B@" = {{.*}} constant [1 x i8*]
+}