[MS-ABI]V-base dtor called more than needed when throw happens in v-base ctor in window. Need add "complete object flag" check in eh cleanup code.

[jyu2]

Summary

The problem only happen on window ( A MS-ABI issuer )

The nature of the problem is virtual base dtor called more than it is needed after exception throw in inheriting base class(with virtual bases) ctor.

The root problem is when throw happen, not all virtual base classes have been contructed, so not all virtual base dtors are need to call for ehcleanup.

clang has code to handle vbase initialization: basically add check for "complete object flag" before call to v-base ctor.
But that part is missing for cleanup code.

To fix this add similar code as v-base init to cleanup code, same algorithm.

1> Add new routine:
EmitDtorCompleteObjectHandler

With corresponding to EmitCtorCompleteObjectHandler

2> In the EmitDestructorCal
Call EmitDtorCompleteObjectHandler when generate ehcleanup inside ctor.

Just add check for "complete object flag" before call to v-base dtor.

Without my change:
ehcleanup: ; preds = %ctor.skip_vbases

%13 = cleanuppad within none [], !dbg !66
%14 = bitcast %struct.class_0* %this1 to i8*, !dbg !66
%15 = getelementptr inbounds i8, i8* %14, i64 8, !dbg !66
%16 = bitcast i8* %15 to %struct.class_2*, !dbg !66
call void @"\01??1class_2@@UEAA@XZ"(%struct.class_2* %16) #6 [ "funclet"(token

%13) ], !dbg !66

cleanupret from %13 unwind to caller, !dbg !66

with my change:
ehcleanup: ; preds = %ctor.skip_vbases

%13 = cleanuppad within none [], !dbg !66
%14 = bitcast %struct.class_0* %this1 to i8*, !dbg !66
%15 = getelementptr inbounds i8, i8* %14, i64 8, !dbg !66
%16 = bitcast i8* %15 to %struct.class_2*, !dbg !66
%is_complete_object4 = icmp ne i32 %is_most_derived2, 0, !dbg !66
br i1 %is_complete_object4, label %Dtor.dtor_vbase, label %Dtor.skip_vbase, !d

bg !66

Dtor.dtor_vbase: ; preds = %ehcleanup

call void @"\01??1class_2@@UEAA@XZ"(%struct.class_2* %16) #6 [ "funclet"(token

%13) ], !dbg !66

br label %Dtor.skip_vbase, !dbg !66

Dtor.skip_vbase: ; preds = %Dtor.dtor_vbase, %ehcleanup

cleanupret from %13 unwind to caller, !dbg !66

Please let me know you need more info.

[rnk]

This highlights an interesting MS/Itanium C++ ABI difference. GCC and Clang emit cleanup landing pads in destructors to ensure that all subobjects are destroyed even if one throws an exception. In Mingw, both GCC and Clang print ~A and ~B for this program, but MSVC does not:

extern "C" int puts(const char *);
struct A { virtual ~A() noexcept(false) { puts("~A"); } };
struct B { virtual ~B() noexcept(false) { puts("~B"); } };
struct C : virtual A, virtual B { virtual ~C() noexcept(false) { throw 1; } };
int main() {
  try {
    C c;
  } catch (...) {
    puts("caught");
  }
}

I encountered this while tracing out call sites in clang where ForVirtualBase is set to true in EmitCXXDestructorCall.

lib/CodeGen/MicrosoftCXXABI.cpp

1530–1532

These checks seem unnecessary. ForVirtualBase should never be true if there are no vbases, and the IsMostDerivedClass assert will catch it if not.

test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp

285

Most of these can be declarations to reduce the output size.

316

Check for a call to the class_2 destructor here

[jyu2]

Update patch to address Reid's suggestion

[jyu2]

Hi Reid,

I know some problems(ms compatibility) when throw inside destructor. I have not yet look those problems. I am new for clang. I need sometime to catch up. -:)

Thank you so much for your code review. I had add new patch to address your suggestion.

Please take look. Let me know if you need more info.

Thanks.
Jennifer

lib/CodeGen/MicrosoftCXXABI.cpp

1530–1532

Yes, you are right. I can either check here, or check if IsMostDerivedClass is nullptr return instead assertion inside EmitDtorCompleteObjectHandler.

As you know ForVirutalBase is set also for destructor. But we only need this for ctor.

test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp

285

Thank you! I changed, see my new diff

316

Thank you so much. I changed see my new diff.

[jyu2]

A typo.

[rnk]

Looks pretty good, just one more thing.

lib/CodeGen/MicrosoftCXXABI.cpp

1530–1532

Yes, the first if check is necessary, but the second check for ClassDecl != nullptr && ClassDecl->getNumVBases() should never be false when ForVirtualBase is true.

[jyu2]

I changed! Thank you do much. Upload new patch.

lib/CodeGen/MicrosoftCXXABI.cpp

1530–1532

Yes, make sense!! I don't know what I was think. Changed.

[jyu2]

Remove unnecessary check.

[jyu2]

last one missing test part diff. Sorry for that

[rnk]

Looks good, please commit. If you don't have commit access, ask a coworker who does to land it, otherwise I'll try to get around to it at some point. Thanks for the fix!

[erichkeane]

I'll land it, thanks!

lib/CodeGen/MicrosoftCXXABI.cpp

public:
   llvm::Value *
   GetVirtualBaseClassOffset(CodeGenFunction &CGF, Address This,
                             const CXXRecordDecl *ClassDecl,
                             const CXXRecordDecl *BaseClassDecl) override;
 
   llvm::BasicBlock *
   EmitCtorCompleteObjectHandler(CodeGenFunction &CGF,
                                 const CXXRecordDecl *RD) override;
   
+  llvm::BasicBlock *
+  EmitDtorCompleteObjectHandler(CodeGenFunction &CGF);
+
   void initializeHiddenVirtualInheritanceMembers(CodeGenFunction &CGF,
                                               const CXXRecordDecl *RD) override;
 
   void EmitCXXConstructors(const CXXConstructorDecl *D) override;
 
   // Background on MSVC destructors
   // ==============================
   //
MicrosoftCXXABI::EmitCtorCompleteObjectHandler(CodeGenFunction &CGF,
   // Fill in the vbtable pointers here.
   EmitVBPtrStores(CGF, RD);
 
   // CGF will put the base ctor calls in this basic block for us later.
 
   return SkipVbaseCtorsBB;
 }
 
+llvm::BasicBlock *
+MicrosoftCXXABI::EmitDtorCompleteObjectHandler(CodeGenFunction &CGF) {
+  llvm::Value *IsMostDerivedClass = getStructorImplicitParamValue(CGF);
+  assert(IsMostDerivedClass &&
+         "ctor for a class with virtual bases must have an implicit parameter");
+  llvm::Value *IsCompleteObject =
+      CGF.Builder.CreateIsNotNull(IsMostDerivedClass, "is_complete_object");
+
+  llvm::BasicBlock *CallVbaseDtorsBB = CGF.createBasicBlock("Dtor.dtor_vbases");
+  llvm::BasicBlock *SkipVbaseDtorsBB = CGF.createBasicBlock("Dtor.skip_vbases");
+  CGF.Builder.CreateCondBr(IsCompleteObject,
+                           CallVbaseDtorsBB, SkipVbaseDtorsBB);
+
+  CGF.EmitBlock(CallVbaseDtorsBB);
+  // CGF will put the base dtor calls in this basic block for us later.
+    
+  return SkipVbaseDtorsBB;
+}
+
 void MicrosoftCXXABI::initializeHiddenVirtualInheritanceMembers(
     CodeGenFunction &CGF, const CXXRecordDecl *RD) {
   // In most cases, an override for a vbase virtual method can adjust
   // the "this" parameter by applying a constant offset.
   // However, this is not enough while a constructor or a destructor of some
   // class X is being executed if all the following conditions are met:
   //  - X has virtual bases, (1)
   //  - X overrides a virtual method M of a vbase Y, (2)
void MicrosoftCXXABI::EmitDestructorCall(CodeGenFunction &CGF,
   llvm::Value *Callee = CGM.getAddrOfCXXStructor(DD, getFromDtorType(Type));
 
   if (DD->isVirtual()) {
     assert(Type != CXXDtorType::Dtor_Deleting &&
            "The deleting destructor should only be called via a virtual call");
     This = adjustThisArgumentForVirtualFunctionCall(CGF, GlobalDecl(DD, Type),
                                                     This, false);
   }
   
+  llvm::BasicBlock *BaseDtorEndBB = nullptr;
+  if (ForVirtualBase && isa<CXXConstructorDecl>(CGF.CurCodeDecl)) {
+    const CXXRecordDecl *ClassDecl =
+             cast<CXXConstructorDecl>(CGF.CurCodeDecl)->getParent();
+    if (ClassDecl != nullptr && ClassDecl->getNumVBases())
+      BaseDtorEndBB = EmitDtorCompleteObjectHandler(CGF);
+  }  
+
   CGF.EmitCXXDestructorCall(DD, Callee, This.getPointer(),
                             /*ImplicitParam=*/nullptr,
                             /*ImplicitParamTy=*/QualType(), nullptr,
                             getFromDtorType(Type));
+  if (BaseDtorEndBB) {
+    // Complete object handler should continue to be the remaining 
+    CGF.Builder.CreateBr(BaseDtorEndBB);
+    CGF.EmitBlock(BaseDtorEndBB);
+  } 
 }
 
 void MicrosoftCXXABI::emitVTableTypeMetadata(const VPtrInfo &Info,
                                              const CXXRecordDecl *RD,
                                              llvm::GlobalVariable *VTable) {
   if (!CGM.getCodeGenOpts().PrepareForLTO)
     return;
 

test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp

 // WIN32-LIFETIME: call x86_thiscallcc void @"\01??1C@lifetime_marker@@QAE@XZ"({{.*}})
 // WIN32-LIFETIME: cleanupret {{.*}} unwind label %[[lpad1:[^ ]*]]
 //
 // WIN32-LIFETIME: [[lpad1]]
 // WIN32-LIFETIME-NEXT: cleanuppad
 // WIN32-LIFETIME: %[[bc2:.*]] = bitcast %"struct.lifetime_marker::C"* %[[c]] to i8*
 // WIN32-LIFETIME: call void @llvm.lifetime.end(i64 1, i8* %[[bc2]])
 }
+
+
+struct class_2 {
+  int var_0;
+  class_2() {};
+  virtual ~class_2() {
+  }
+};
+
+struct class_1 : virtual class_2 {
+  class_1() {throw "Unhandled exception";}
+  virtual ~class_1() {}
+};
+struct class_0 : class_1 {
+  class_0() ;
+  virtual ~class_0() {}
+};
+
+class_0::class_0() {
+  // WIN32: define x86_thiscallcc %struct.class_0* @"\01??0class_0@@QAE@XZ"(%struct.class_0* returned %this, i32 %is_most_derived) 
+  // WIN32: store i32 %is_most_derived, i32* %[[IS_MOST_DERIVED_VAR:.*]], align 4
+  // WIN32: %[[IS_MOST_DERIVED_VAL:.*]] = load i32, i32* %[[IS_MOST_DERIVED_VAR]]
+  // WIN32: %[[SHOULD_CALL_VBASE_CTORS:.*]] = icmp ne i32 %[[IS_MOST_DERIVED_VAL]], 0
+  // WIN32: br i1 %[[SHOULD_CALL_VBASE_CTORS]], label %[[INIT_VBASES:.*]], label %[[SKIP_VBASES:.*]]
+  // WIN32: [[INIT_VBASES]]
+  // WIN32: br label %[[SKIP_VBASES]]
+  // WIN32: [[SKIP_VBASES]]
+// ehcleanup:
+  // WIN32: %[[CLEANUPPAD:.*]] = cleanuppad within none []
+  // WIN32-NEXT: bitcast %{{.*}}* %{{.*}} to i8*
+  // WIN32-NEXT: getelementptr inbounds i8, i8* %{{.*}}, i{{.*}} {{.}} 
+  // WIN32-NEXT: bitcast i8* %{{.*}} to %{{.*}}*
+  // WIN32-NEXT: %[[SHOULD_CALL_VBASE_DTOR:.*]] = icmp ne i32 %[[IS_MOST_DERIVED_VAL]], 0
+  // WIN32-NEXT: br i1 %[[SHOULD_CALL_VBASE_DTOR]], label %[[DTOR_VBASE:.*]], label %[[SKIP_VBASE:.*]]
+  // WIN32: [[DTOR_VBASE]]
+  // WIN32: br label %[[SKIP_VBASE]]
+  // WIN32: [[SKIP_VBASE]]
+}