Fix PR19487, PR19505 and PR19506 -- redundant vtordisp thunks when the final overrider is present in both a vbase and nvbase

[timurrrr]

Summary

I believe the change in fact makes the code even simpler than before :)

[rnk]

LGTM

Nice!

[timurrrr]

r206908, thanks!

lib/AST/VTableBuilder.cpp

 /// member functions in the base subobjects of a class.
 class FinalOverriders {
 public:
   /// OverriderInfo - Information about a final overrider.
   struct OverriderInfo {
     /// Method - The method decl of the overrider.
     const CXXMethodDecl *Method;
 
+    /// VirtualBase - The virtual base class subobject of this overridder.
+    /// Note that this records the closest derived virtual base class subobject.
+    const CXXRecordDecl *VirtualBase;
+
     /// Offset - the base offset of the overrider's parent in the layout class.
     CharUnits Offset;
     
-    OverriderInfo() : Method(0), Offset(CharUnits::Zero()) { }
+    OverriderInfo() : Method(0), VirtualBase(0), Offset(CharUnits::Zero()) { }
   };
 
 private:
   /// MostDerivedClass - The most derived class for which the final overriders
   /// are stored.
   const CXXRecordDecl *MostDerivedClass;
   
   /// MostDerivedClassOffset - If we're building final overriders for a 

         SubobjectLayoutClassOffsets[std::make_pair(OverriderRD, 
                                                    Method.Subobject)];
 
       OverriderInfo& Overrider = OverridersMap[std::make_pair(MD, BaseOffset)];
       assert(!Overrider.Method && "Overrider should not exist yet!");
       
       Overrider.Offset = OverriderOffset;
       Overrider.Method = Method.Method;
+      Overrider.VirtualBase = Method.InVirtualSubobject;
     }
   }
 
 #if DUMP_OVERRIDERS
   // And dump them (for now).
   dump();
 #endif
 }

     FinalOverriders::OverriderInfo Overrider, CharUnits ThisOffset,
     ThisAdjustment &TA) {
   const ASTRecordLayout::VBaseOffsetsMapTy &VBaseMap =
       MostDerivedClassLayout.getVBaseOffsetsMap();
   const ASTRecordLayout::VBaseOffsetsMapTy::const_iterator &VBaseMapEntry =
       VBaseMap.find(WhichVFPtr.getVBaseWithVPtr());
   assert(VBaseMapEntry != VBaseMap.end());
 
-  // If there's no vtordisp, we don't need any vtordisp adjustment.
-  if (!VBaseMapEntry->second.hasVtorDisp())
-    return;
-
-  const CXXRecordDecl *OverriderRD = Overrider.Method->getParent();
-  const CXXRecordDecl *OverriderVBase = 0;
-  if (OverriderRD != MostDerivedClass) {
-    OverriderVBase =
-        ComputeBaseOffset(Context, OverriderRD, MostDerivedClass).VirtualBase;
-  }
-
-  // If the final overrider is defined in the same vbase as the initial
-  // declaration, we don't need a vtordisp thunk at all.
-  if (OverriderVBase == WhichVFPtr.getVBaseWithVPtr())
+  // If there's no vtordisp or the final overrider is defined in the same vbase
+  // as the initial declaration, we don't need any vtordisp adjustment.
+  if (!VBaseMapEntry->second.hasVtorDisp() ||
+      Overrider.VirtualBase == WhichVFPtr.getVBaseWithVPtr())
     return;
 
   // OK, now we know we need to use a vtordisp thunk.
   // The implicit vtordisp field is located right before the vbase.
   CharUnits VFPtrVBaseOffset = VBaseMapEntry->second.VBaseOffset;
   TA.Virtual.Microsoft.VtordispOffset =
       (VFPtrVBaseOffset - WhichVFPtr.FullOffsetInMDC).getQuantity() - 4;
 
   // A simple vtordisp thunk will suffice if the final overrider is defined
   // in either the most derived class or its non-virtual base.
-  if (OverriderRD == MostDerivedClass || !OverriderVBase)
+  if (Overrider.Method->getParent() == MostDerivedClass ||
+      !Overrider.VirtualBase)
     return;
 
   // Otherwise, we need to do use the dynamic offset of the final overrider
   // in order to get "this" adjustment right.
   TA.Virtual.Microsoft.VBPtrOffset =
       (VFPtrVBaseOffset + WhichVFPtr.NonVirtualOffset -
        MostDerivedClassLayout.getVBPtrOffset()).getQuantity();
   TA.Virtual.Microsoft.VBOffsetOffset =
       Context.getTypeSizeInChars(Context.IntTy).getQuantity() *
-      VTables.getVBTableIndex(MostDerivedClass, OverriderVBase);
+      VTables.getVBTableIndex(MostDerivedClass, Overrider.VirtualBase);
 
   TA.NonVirtual = (ThisOffset - Overrider.Offset).getQuantity();
 }
 
 static void GroupNewVirtualOverloads(
     const CXXRecordDecl *RD,
     SmallVector<const CXXMethodDecl *, 10> &VirtualMethods) {
   // Put the virtual methods into VirtualMethods in the proper order:

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

struct C : virtual B {
   virtual ~C();
 
   // MANGLING-DAG: @"\01?prot@B@access@@$R277PPPPPPPM@7AEXXZ"
   // MANGLING-DAG: @"\01?priv@B@access@@$R077PPPPPPPM@7AEXXZ"
 };
 
 C c;
 }
+
+namespace pr19505 {
+struct A {
+  virtual void f();
+  virtual void z();
+};
+
+struct B : A {
+  virtual void f();
+};
+
+struct C : A, B {
+  virtual void g();
+};
+
+struct X : B, virtual C {
+  X() {}
+  virtual void g();
+
+  // CHECK-LABEL: VFTable for 'pr19505::A' in 'pr19505::B' in 'pr19505::C' in 'pr19505::X' (2 entries).
+  // CHECK-NEXT:   0 | void pr19505::B::f()
+  // CHECK-NEXT:   1 | void pr19505::A::z()
+
+  // MANGLING-DAG: @"\01??_7X@pr19505@@6BB@1@@" = {{.*}}@"\01?f@B@pr19505@@UAEXXZ"
+} x;
+
+void build_vftable(X *obj) { obj->g(); }
+}
+
+namespace pr19506 {
+struct A {
+  virtual void f();
+  virtual void g();
+};
+
+struct B : A {
+  virtual void f();
+};
+
+struct C : B {};
+
+struct X : C, virtual B {
+  virtual void g();
+  X() {}
+
+  // CHECK-LABEL: VFTable for 'pr19506::A' in 'pr19506::B' in 'pr19506::X' (2 entries).
+  // CHECK-NEXT:   0 | void pr19506::B::f()
+  // CHECK-NEXT:   1 | void pr19506::X::g()
+  // CHECK-NEXT:       [this adjustment: vtordisp at -4, -12 non-virtual]
+
+  // MANGLING-DAG: @"\01??_7X@pr19506@@6BB@1@@" = {{.*}}@"\01?f@B@pr19506@@UAEXXZ"
+} x;
+
+void build_vftable(X *obj) { obj->g(); }
+}