[PATCH] D27358: [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.

[Erich Keane via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL288869: [MS-ABI]V-base dtor called more than needed when 
throw happens in v-base ctor… (authored by erichkeane).

Changed prior to commit:
  https://reviews.llvm.org/D27358?vs=80491=80505#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D27358

Files:
  cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp
  cfe/trunk/test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp

Index: cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp
===
--- cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp
+++ cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -165,6 +165,9 @@
   llvm::BasicBlock *
   EmitCtorCompleteObjectHandler(CodeGenFunction ,
 const CXXRecordDecl *RD) override;
+  
+  llvm::BasicBlock *
+  EmitDtorCompleteObjectHandler(CodeGenFunction );
 
   void initializeHiddenVirtualInheritanceMembers(CodeGenFunction ,
   const CXXRecordDecl *RD) override;
@@ -1135,6 +1138,25 @@
   return SkipVbaseCtorsBB;
 }
 
+llvm::BasicBlock *
+MicrosoftCXXABI::EmitDtorCompleteObjectHandler(CodeGenFunction ) {
+  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 , const CXXRecordDecl *RD) {
   // In most cases, an override for a vbase virtual method can adjust
@@ -1512,11 +1534,21 @@
 This = adjustThisArgumentForVirtualFunctionCall(CGF, GlobalDecl(DD, Type),
 This, false);
   }
+  
+  llvm::BasicBlock *BaseDtorEndBB = nullptr;
+  if (ForVirtualBase && isa(CGF.CurCodeDecl)) {
+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 ,
Index: cfe/trunk/test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp
===
--- cfe/trunk/test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp
+++ cfe/trunk/test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp
@@ -278,3 +278,38 @@
 // 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 {
+  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-NEXT: call x86_thiscallcc void @"\01??1class_2@@UAE@XZ"
+  // WIN32: br label %[[SKIP_VBASE]]
+  // WIN32: [[SKIP_VBASE]]
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27358: [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.

[Reid Kleckner via Phabricator via cfe-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

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!


Repository:
  rL LLVM

https://reviews.llvm.org/D27358



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27358: [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.

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I'll land it, thanks!


Repository:
  rL LLVM

https://reviews.llvm.org/D27358



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27358: [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.

[Jennifer Yu via Phabricator via cfe-commits]

jyu2 updated this revision to Diff 80491.
jyu2 added a comment.

last one missing test part diff.  Sorry for that


Repository:
  rL LLVM

https://reviews.llvm.org/D27358

Files:
  lib/CodeGen/MicrosoftCXXABI.cpp
  test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp

Index: lib/CodeGen/MicrosoftCXXABI.cpp
===
--- lib/CodeGen/MicrosoftCXXABI.cpp
+++ lib/CodeGen/MicrosoftCXXABI.cpp
@@ -164,6 +164,9 @@
   llvm::BasicBlock *
   EmitCtorCompleteObjectHandler(CodeGenFunction ,
 const CXXRecordDecl *RD) override;
+  
+  llvm::BasicBlock *
+  EmitDtorCompleteObjectHandler(CodeGenFunction );
 
   void initializeHiddenVirtualInheritanceMembers(CodeGenFunction ,
   const CXXRecordDecl *RD) override;
@@ -1127,6 +1130,25 @@
   return SkipVbaseCtorsBB;
 }
 
+llvm::BasicBlock *
+MicrosoftCXXABI::EmitDtorCompleteObjectHandler(CodeGenFunction ) {
+  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 , const CXXRecordDecl *RD) {
   // In most cases, an override for a vbase virtual method can adjust
@@ -1502,11 +1524,21 @@
 This = adjustThisArgumentForVirtualFunctionCall(CGF, GlobalDecl(DD, Type),
 This, false);
   }
+  
+  llvm::BasicBlock *BaseDtorEndBB = nullptr;
+  if (ForVirtualBase && isa(CGF.CurCodeDecl)) {
+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 ,
Index: test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp
===
--- test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp
+++ test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp
@@ -278,3 +278,38 @@
 // 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 {
+  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-NEXT: call x86_thiscallcc void @"\01??1class_2@@UAE@XZ"
+  // WIN32: br label %[[SKIP_VBASE]]
+  // WIN32: [[SKIP_VBASE]]
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27358: [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.

[Jennifer Yu via Phabricator via cfe-commits]

jyu2 updated this revision to Diff 80487.
jyu2 added a comment.

Remove unnecessary check.


Repository:
  rL LLVM

https://reviews.llvm.org/D27358

Files:
  CodeGen/MicrosoftCXXABI.cpp


Index: CodeGen/MicrosoftCXXABI.cpp
===
--- CodeGen/MicrosoftCXXABI.cpp
+++ CodeGen/MicrosoftCXXABI.cpp
@@ -164,6 +164,9 @@
   llvm::BasicBlock *
   EmitCtorCompleteObjectHandler(CodeGenFunction ,
 const CXXRecordDecl *RD) override;
+  
+  llvm::BasicBlock *
+  EmitDtorCompleteObjectHandler(CodeGenFunction );
 
   void initializeHiddenVirtualInheritanceMembers(CodeGenFunction ,
   const CXXRecordDecl *RD) 
override;
@@ -1127,6 +1130,25 @@
   return SkipVbaseCtorsBB;
 }
 
+llvm::BasicBlock *
+MicrosoftCXXABI::EmitDtorCompleteObjectHandler(CodeGenFunction ) {
+  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 , const CXXRecordDecl *RD) {
   // In most cases, an override for a vbase virtual method can adjust
@@ -1502,11 +1524,21 @@
 This = adjustThisArgumentForVirtualFunctionCall(CGF, GlobalDecl(DD, Type),
 This, false);
   }
+  
+  llvm::BasicBlock *BaseDtorEndBB = nullptr;
+  if (ForVirtualBase && isa(CGF.CurCodeDecl)) {
+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 ,


Index: CodeGen/MicrosoftCXXABI.cpp
===
--- CodeGen/MicrosoftCXXABI.cpp
+++ CodeGen/MicrosoftCXXABI.cpp
@@ -164,6 +164,9 @@
   llvm::BasicBlock *
   EmitCtorCompleteObjectHandler(CodeGenFunction ,
 const CXXRecordDecl *RD) override;
+  
+  llvm::BasicBlock *
+  EmitDtorCompleteObjectHandler(CodeGenFunction );
 
   void initializeHiddenVirtualInheritanceMembers(CodeGenFunction ,
   const CXXRecordDecl *RD) override;
@@ -1127,6 +1130,25 @@
   return SkipVbaseCtorsBB;
 }
 
+llvm::BasicBlock *
+MicrosoftCXXABI::EmitDtorCompleteObjectHandler(CodeGenFunction ) {
+  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 , const CXXRecordDecl *RD) {
   // In most cases, an override for a vbase virtual method can adjust
@@ -1502,11 +1524,21 @@
 This = adjustThisArgumentForVirtualFunctionCall(CGF, GlobalDecl(DD, Type),
 This, false);
   }
+  
+  llvm::BasicBlock *BaseDtorEndBB = nullptr;
+  if (ForVirtualBase && isa(CGF.CurCodeDecl)) {
+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);
+

[PATCH] D27358: [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.

[Jennifer Yu via Phabricator via cfe-commits]

jyu2 added a comment.

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




Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1530-1532
+const CXXRecordDecl *ClassDecl =
+ cast(CGF.CurCodeDecl)->getParent();
+if (ClassDecl != nullptr && ClassDecl->getNumVBases())

rnk wrote:
> jyu2 wrote:
> > rnk wrote:
> > > These checks seem unnecessary. ForVirtualBase should never be true if 
> > > there are no vbases, and the IsMostDerivedClass assert will catch it if 
> > > not.
> > 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.  
> > 
> > 
> Yes, the first if check is necessary, but the second check for `ClassDecl != 
> nullptr && ClassDecl->getNumVBases()` should never be false when 
> ForVirtualBase is true.
Yes, make sense!! I don't know what I was think.  Changed.   


Repository:
  rL LLVM

https://reviews.llvm.org/D27358



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27358: [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.

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

Looks pretty good, just one more thing.




Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1530-1532
+const CXXRecordDecl *ClassDecl =
+ cast(CGF.CurCodeDecl)->getParent();
+if (ClassDecl != nullptr && ClassDecl->getNumVBases())

jyu2 wrote:
> rnk wrote:
> > These checks seem unnecessary. ForVirtualBase should never be true if there 
> > are no vbases, and the IsMostDerivedClass assert will catch it if not.
> 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.  
> 
> 
Yes, the first if check is necessary, but the second check for `ClassDecl != 
nullptr && ClassDecl->getNumVBases()` should never be false when ForVirtualBase 
is true.


Repository:
  rL LLVM

https://reviews.llvm.org/D27358



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27358: [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.

[Jennifer Yu via Phabricator via cfe-commits]

jyu2 set the repository for this revision to rL LLVM.
jyu2 updated this revision to Diff 80462.
jyu2 added a comment.

A typo.


Repository:
  rL LLVM

https://reviews.llvm.org/D27358

Files:
  lib/CodeGen/MicrosoftCXXABI.cpp
  test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp

Index: lib/CodeGen/MicrosoftCXXABI.cpp
===
--- lib/CodeGen/MicrosoftCXXABI.cpp
+++ lib/CodeGen/MicrosoftCXXABI.cpp
@@ -164,6 +164,9 @@
   llvm::BasicBlock *
   EmitCtorCompleteObjectHandler(CodeGenFunction ,
 const CXXRecordDecl *RD) override;
+  
+  llvm::BasicBlock *
+  EmitDtorCompleteObjectHandler(CodeGenFunction );
 
   void initializeHiddenVirtualInheritanceMembers(CodeGenFunction ,
   const CXXRecordDecl *RD) override;
@@ -1127,6 +1130,25 @@
   return SkipVbaseCtorsBB;
 }
 
+llvm::BasicBlock *
+MicrosoftCXXABI::EmitDtorCompleteObjectHandler(CodeGenFunction ) {
+  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 , const CXXRecordDecl *RD) {
   // In most cases, an override for a vbase virtual method can adjust
@@ -1502,11 +1524,24 @@
 This = adjustThisArgumentForVirtualFunctionCall(CGF, GlobalDecl(DD, Type),
 This, false);
   }
+  
+  llvm::BasicBlock *BaseDtorEndBB = nullptr;
+  if (ForVirtualBase && isa(CGF.CurCodeDecl)) {
+const CXXRecordDecl *ClassDecl =
+ cast(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 ,
Index: test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp
===
--- test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp
+++ test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp
@@ -278,3 +278,38 @@
 // 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 {
+  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-NEXT: call x86_thiscallcc void @"\01??1class_2@@UAE@XZ"
+  // WIN32: br label %[[SKIP_VBASE]]
+  // WIN32: [[SKIP_VBASE]]
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27358: [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.

[Jennifer Yu via Phabricator via cfe-commits]

jyu2 added a comment.

Hi Reid,

I know some problems(ms compatibility) when throw inside destructor.  I have 
not yet look like 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




Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1530-1532
+const CXXRecordDecl *ClassDecl =
+ cast(CGF.CurCodeDecl)->getParent();
+if (ClassDecl != nullptr && ClassDecl->getNumVBases())

rnk wrote:
> These checks seem unnecessary. ForVirtualBase should never be true if there 
> are no vbases, and the IsMostDerivedClass assert will catch it if not.
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.  





Comment at: test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp:285
+  int var_0;
+  class_2() {};
+  virtual ~class_2() {

rnk wrote:
> Most of these can be declarations to reduce the output size.
Thank you!  I changed, see my new diff



Comment at: test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp:316
+  // WIN32: [[DTOR_VBASE]]
+  // WIN32: br label %[[SKIP_VBASE]]
+  // WIN32: [[SKIP_VBASE]]

rnk wrote:
> Check for a call to the class_2 destructor here
Thank you so much.  I changed see my new diff.


https://reviews.llvm.org/D27358



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27358: [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.

[Jennifer Yu via Phabricator via cfe-commits]

jyu2 removed rL LLVM as the repository for this revision.
jyu2 updated this revision to Diff 80459.
jyu2 added a comment.

Update patch to address Reid's suggestion


https://reviews.llvm.org/D27358

Files:
  lib/CodeGen/MicrosoftCXXABI.cpp
  test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp

Index: lib/CodeGen/MicrosoftCXXABI.cpp
===
--- lib/CodeGen/MicrosoftCXXABI.cpp
+++ lib/CodeGen/MicrosoftCXXABI.cpp
@@ -164,6 +164,9 @@
   llvm::BasicBlock *
   EmitCtorCompleteObjectHandler(CodeGenFunction ,
 const CXXRecordDecl *RD) override;
+  
+  llvm::BasicBlock *
+  EmitDtorCompleteObjectHandler(CodeGenFunction );
 
   void initializeHiddenVirtualInheritanceMembers(CodeGenFunction ,
   const CXXRecordDecl *RD) override;
@@ -1127,6 +1130,25 @@
   return SkipVbaseCtorsBB;
 }
 
+llvm::BasicBlock *
+MicrosoftCXXABI::EmitDtorCompleteObjectHandler(CodeGenFunction ) {
+  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 , const CXXRecordDecl *RD) {
   // In most cases, an override for a vbase virtual method can adjust
@@ -1502,11 +1524,24 @@
 This = adjustThisArgumentForVirtualFunctionCall(CGF, GlobalDecl(DD, Type),
 This, false);
   }
+  
+  llvm::BasicBlock *BaseDtorEndBB = nullptr;
+  if (ForVirtualBase && isa(CGF.CurCodeDecl)) {
+const CXXRecordDecl *ClassDecl =
+ cast(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 ,
Index: test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp
===
--- test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp
+++ test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp
@@ -278,3 +278,39 @@
 // 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: call x86_thiscallcc void @"\01??1class_2@@UAE@XZ"
+  // WIN32: [[DTOR_VBASE]]
+  // WIN32: br label %[[SKIP_VBASE]]
+  // WIN32: [[SKIP_VBASE]]
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27358: [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.

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

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.




Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1530-1532
+const CXXRecordDecl *ClassDecl =
+ cast(CGF.CurCodeDecl)->getParent();
+if (ClassDecl != nullptr && ClassDecl->getNumVBases())

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



Comment at: test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp:285
+  int var_0;
+  class_2() {};
+  virtual ~class_2() {

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



Comment at: test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp:316
+  // WIN32: [[DTOR_VBASE]]
+  // WIN32: br label %[[SKIP_VBASE]]
+  // WIN32: [[SKIP_VBASE]]

Check for a call to the class_2 destructor here


Repository:
  rL LLVM

https://reviews.llvm.org/D27358



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits