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

[Reid Kleckner]

================
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
----------------
maybe say "at least one path to a non-virtual base with..."

================
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) {
----------------
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.

================
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
----------------
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?


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