Fix PR20479 -- missing vftable slots in case of virtual inheritance vs return adjusting thunks

[rnk]

lgtm

lib/AST/VTableBuilder.cpp

3177–3178

Yeah. It seems clear to me that they reorder base specifiers into record layout order, which is approximately this. The only differences in order will arise from non-polymorphic bases, which shouldn't matter.

[timurrrr]

r215285 it is!

lib/AST/VTableBuilder.cpp

                                                 VPtrInfoVector &Paths) {
   assert(Paths.empty());
   const ASTRecordLayout &Layout = Context.getASTRecordLayout(RD);
 
   // Base case: this subobject has its own vptr.
   if (ForVBTables ? Layout.hasOwnVBPtr() : Layout.hasOwnVFPtr())
     Paths.push_back(new VPtrInfo(RD));
 
-  // Recursive case: get all the vbtables from our bases and remove anything
-  // that shares a virtual base.
+  // Recursive case: get all the vtables from our bases and remove anything
+  // that shares a virtual base.  Look at non-virtual bases first so we get
+  // longer inheritance paths from the derived class to the virtual bases.
+  llvm::SmallVector<CXXBaseSpecifier, 10> Bases;
+  std::copy_if(RD->bases_begin(), RD->bases_end(), std::back_inserter(Bases),
+               [](CXXBaseSpecifier bs) { return !bs.isVirtual(); });
+  std::copy_if(RD->bases_begin(), RD->bases_end(), std::back_inserter(Bases),
+               [](CXXBaseSpecifier bs) { return bs.isVirtual(); });
+
   llvm::SmallPtrSet<const CXXRecordDecl*, 4> VBasesSeen;
-  for (const auto &B : RD->bases()) {
+  for (const auto &B : Bases) {
     const CXXRecordDecl *Base = B.getType()->getAsCXXRecordDecl();
     if (B.isVirtual() && VBasesSeen.count(Base))
       continue;
 
     if (!Base->isDynamicClass())
       continue;
 
     const VPtrInfoVector &BasePaths =

test/CodeGenCXX/microsoft-abi-vtables-return-thunks.cpp

 // VFTABLES-NEXT:         [return adjustment (to type 'struct test2::B *'): 4 non-virtual]
 // VFTABLES-NEXT:    1 | test2::E *test2::K::foo()
 // VFTABLES-NEXT:         [return adjustment (to type 'struct test2::D *'): 0 non-virtual]
 // VFTABLES-NEXT:    2 | test2::E *test2::K::foo()
 
 // GLOBALS-LABEL: @"\01??_7K@test2@@6B@" = linkonce_odr unnamed_addr constant [3 x i8*]
 
 }
+
+namespace pr20479 {
+struct A {
+  virtual A *f();
+};
+
+struct B : virtual A {
+  virtual B *f();
+};
+
+struct C : virtual A, B {
+// VFTABLES-LABEL: VFTable for 'pr20479::A' in 'pr20479::B' in 'pr20479::C' (2 entries).
+// VFTABLES-NEXT:   0 | pr20479::B *pr20479::B::f()
+// VFTABLES-NEXT:       [return adjustment (to type 'struct pr20479::A *'): vbase #1, 0 non-virtual]
+// VFTABLES-NEXT:   1 | pr20479::B *pr20479::B::f()
+  C();
+};
+
+C::C() {}
+
+// GLOBALS-LABEL: @"\01??_7C@pr20479@@6B@" = linkonce_odr unnamed_addr constant [2 x i8*]
+// GLOBALS: @"\01?f@B@pr20479@@QAEPAUA@2@XZ"
+// GLOBALS: @"\01?f@B@pr20479@@UAEPAU12@XZ"
+}

test/CodeGenCXX/microsoft-abi-vtables-virtual-inheritance.cpp

 void use(P *obj) { delete obj; }
 
 struct Q {
   virtual ~Q();
 };
 
 // PR19172: Yet another diamond we miscompiled.
 struct R : virtual Q, X {
+  // CHECK-LABEL: VFTable for 'vdtors::X' in 'vdtors::R' (2 entries).
+  // CHECK-NEXT: 0 | vdtors::R::~R() [scalar deleting]
+  // CHECK-NEXT: 1 | void vdtors::X::zzz()
+
   // CHECK-LABEL: VFTable for 'vdtors::Q' in 'vdtors::R' (1 entry).
   // CHECK-NEXT: 0 | vdtors::R::~R() [scalar deleting]
   // CHECK-NEXT:     [this adjustment: -8 non-virtual]
 
   // CHECK-LABEL: Thunks for 'vdtors::R::~R()' (1 entry).
   // CHECK-NEXT: 0 | [this adjustment: -8 non-virtual]
 
-  // CHECK-LABEL: VFTable for 'vdtors::X' in 'vdtors::R' (2 entries).
-  // CHECK-NEXT: 0 | vdtors::R::~R() [scalar deleting]
-  // CHECK-NEXT: 1 | void vdtors::X::zzz()
-
   // CHECK-LABEL: VFTable indices for 'vdtors::R' (1 entry).
   // CHECK-NEXT: 0 | vdtors::R::~R() [scalar deleting]
   virtual ~R();
 };
 
 R r;
 void use(R *obj) { delete obj; }
 }

 // MANGLING-DAG: @"\01??_7C@pr17748@@6BB@1@@"
 }
 
 namespace pr19066 {
 struct X : virtual B {};
 
 struct Y : virtual X, B {
   Y();
-  // CHECK-LABEL: VFTable for 'B' in 'pr19066::X' in 'pr19066::Y' (1 entry).
+  // CHECK-LABEL: VFTable for 'B' in 'pr19066::Y' (1 entry).
   // CHECK-NEXT:  0 | void B::g()
 
-  // CHECK-LABEL: VFTable for 'B' in 'pr19066::Y' (1 entry).
+  // CHECK-LABEL: VFTable for 'B' in 'pr19066::X' in 'pr19066::Y' (1 entry).
   // CHECK-NEXT:  0 | void B::g()
 };
 
 Y::Y() {}
 }
 
 namespace pr19240 {
 struct A {