[PATCH] [-cxx-abi microsoft] Fix "this" argument/parameter offsets for virtual destructors in the presence of virtual bases

[Timur Iskhodzhanov]

================
Comment at: lib/AST/VTableBuilder.cpp:2723
@@ -2720,1 +2722,3 @@
 
+    // If a "Base" class has at least one non-virtual base with a virtual
+    // destructor, the "Base" virtual destructor will take the address of the
----------------
Reid Kleckner wrote:
> maybe say "at least one path to a non-virtual base with..."
Skipping this as we've discussed on chat.

================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:585
@@ -574,5 +584,3 @@
   if (ML.VBase) {
-    This = CGF.Builder.CreateBitCast(This, charPtrTy);
-    llvm::Value *VBaseOffset = CGM.getCXXABI()
-        .GetVirtualBaseClassOffset(CGF, This, MD->getParent(), ML.VBase);
-    This = CGF.Builder.CreateInBoundsGEP(This, VBaseOffset);
+    bool AvoidVirtualOffset = false;
+    if (isa<CXXDestructorDecl>(MD) && GD.getDtorType() == Dtor_Base) {
----------------
Reid Kleckner wrote:
> Timur Iskhodzhanov wrote:
> > Reid Kleckner wrote:
> > > I'm guessing this optimization just makes it easier to test and read and test the LLVM IR output.
> > Err, no. We replace a dynamic vbtable lookup with a static adjustment, which is not only faster but can also lead to different results!
> > 
> > Interestingly, the result of the static adjustment will be correct, not the result of dynamic adjustment - see the code with "This one is interesting" comment in VTableBuilder.cpp
> Oh, that's surprising.
Added a comment below

================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:586
@@ +585,3 @@
+    bool AvoidVirtualOffset = false;
+    if (isa<CXXDestructorDecl>(MD) && GD.getDtorType() == Dtor_Base) {
+      // A base destructor can only be called from a complete destructor of the
----------------
Reid Kleckner wrote:
> At this point more than half of the method is specific to destructors.  Maybe it would be cleaner to duplicate the ML lookup and split this into an adjustThisArgumentForVirtualDtorCall, and explain why we *have* to use the static offset in some cases?
Per chat discussion, I've added a FIXME to consider this later when working on codegen for vtordisp thunks.


http://llvm-reviews.chandlerc.com/D1939