[win][clang] Align scalar deleting destructors with MSABI #139566
Conversation
While working on vector deleting destructors support (GH19772), I noticed that MSVC produces different code in scalar deleting destructor body depending on whether class defined its own operator delete. In MSABI deleting destructors accept an additional implicit flag parameter allowing some sort of flexibility. The mismatch I noticed is that whenever a global operator delete is called, i.e. ::delete, in the code produced by MSVC the implicit flag argument has a value that makes the 3rd bit set, i.e. "5" for scalar deleting destructors "7" for vector deleting destructors. Prior to this patch, clang handled ::delete via calling global operator delete direct after the destructor call and not calling class operator delete in scalar deleting destructor body by passing "0" as implicit flag argument value. This is fine until there is an attempt to link binaries compiled with clang with binaries compiled with MSVC. The problem is that in binaries produced by MSVC the callsite of the destructor won't call global operator delete because it is assumed that the destructor will do that and a destructor body generated by clang will never do. This patch removes call to global operator delete from the callsite and add additional check of the 3rd bit of the implicit parameter inside of scalar deleting destructor body.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang Author: Mariya Podchishchaeva (Fznamznon) ChangesWhile working on vector deleting destructors support (GH19772), I noticed that MSVC produces different code in scalar deleting destructor body depending on whether class defined its own operator delete. In MSABI deleting destructors accept an additional implicit flag parameter allowing some sort of flexibility. The mismatch I noticed is that whenever a global operator delete is called, i.e. ::delete, in the code produced by MSVC the implicit flag argument has a value that makes the 3rd bit set, i.e. "5" for scalar deleting destructors "7" for vector deleting destructors. Full diff: https://github.com/llvm/llvm-project/pull/139566.diff 8 Files Affected:
diff --git a/clang/include/clang/AST/DeclCXX.h b/clang/include/clang/AST/DeclCXX.h
index b7980137002aa..cc2832ea99d4a 100644
--- a/clang/include/clang/AST/DeclCXX.h
+++ b/clang/include/clang/AST/DeclCXX.h
@@ -2855,6 +2855,7 @@ class CXXDestructorDecl : public CXXMethodDecl {
// FIXME: Don't allocate storage for these except in the first declaration
// of a virtual destructor.
FunctionDecl *OperatorDelete = nullptr;
+ FunctionDecl *OperatorGlobalDelete = nullptr;
Expr *OperatorDeleteThisArg = nullptr;
CXXDestructorDecl(ASTContext &C, CXXRecordDecl *RD, SourceLocation StartLoc,
@@ -2885,6 +2886,16 @@ class CXXDestructorDecl : public CXXMethodDecl {
return getCanonicalDecl()->OperatorDelete;
}
+ const FunctionDecl *getOperatorGlobalDelete() const {
+ return getCanonicalDecl()->OperatorGlobalDelete;
+ }
+
+ void setOperatorGlobalDelete(FunctionDecl *OD) {
+ auto *First = cast<CXXDestructorDecl>(getFirstDecl());
+ if (OD && !First->OperatorGlobalDelete)
+ First->OperatorGlobalDelete = OD;
+ }
+
Expr *getOperatorDeleteThisArg() const {
return getCanonicalDecl()->OperatorDeleteThisArg;
}
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 6ea7ee281e14d..5f2ceebcd42e3 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -8495,10 +8495,12 @@ class Sema final : public SemaBase {
bool Diagnose = true);
FunctionDecl *FindUsualDeallocationFunction(SourceLocation StartLoc,
ImplicitDeallocationParameters,
- DeclarationName Name);
- FunctionDecl *FindDeallocationFunctionForDestructor(SourceLocation StartLoc,
- CXXRecordDecl *RD,
- bool Diagnose = true);
+ DeclarationName Name,
+ bool Diagnose = true);
+ FunctionDecl *
+ FindDeallocationFunctionForDestructor(SourceLocation StartLoc,
+ CXXRecordDecl *RD, bool Diagnose = true,
+ bool LookForGlobal = false);
/// ActOnCXXDelete - Parsed a C++ 'delete' expression (C++ 5.3.5), as in:
/// @code ::delete ptr; @endcode
diff --git a/clang/lib/CodeGen/CGClass.cpp b/clang/lib/CodeGen/CGClass.cpp
index befbfc64a680c..360876244cad9 100644
--- a/clang/lib/CodeGen/CGClass.cpp
+++ b/clang/lib/CodeGen/CGClass.cpp
@@ -1589,25 +1589,74 @@ namespace {
void EmitConditionalDtorDeleteCall(CodeGenFunction &CGF,
llvm::Value *ShouldDeleteCondition,
bool ReturnAfterDelete) {
+ const CXXDestructorDecl *Dtor = cast<CXXDestructorDecl>(CGF.CurCodeDecl);
+ const CXXRecordDecl *ClassDecl = Dtor->getParent();
+ const FunctionDecl *OD = Dtor->getOperatorDelete();
+ assert(OD->isDestroyingOperatorDelete() == ReturnAfterDelete &&
+ "unexpected value for ReturnAfterDelete");
+ auto *CondTy = cast<llvm::IntegerType>(ShouldDeleteCondition->getType());
+ if (OD->isDestroyingOperatorDelete()) {
+ llvm::BasicBlock *CallDtor = CGF.createBasicBlock("dtor.call_dtor");
+ llvm::BasicBlock *DontCallDtor = CGF.createBasicBlock("dtor.entry_cont");
+ // Third bit set signals that global operator delete is called. That means
+ // despite class having destroying operator delete which is responsible
+ // for calling dtor, we need to call dtor because global operator delete
+ // won't do that.
+ llvm::Value *Check3rdBit = CGF.Builder.CreateAnd(
+ ShouldDeleteCondition, llvm::ConstantInt::get(CondTy, 4));
+ llvm::Value *ShouldCallDtor = CGF.Builder.CreateIsNull(Check3rdBit);
+ CGF.Builder.CreateCondBr(ShouldCallDtor, DontCallDtor, CallDtor);
+ CGF.EmitBlock(CallDtor);
+ QualType ThisTy = Dtor->getFunctionObjectParameterType();
+ CGF.EmitCXXDestructorCall(Dtor, Dtor_Complete, /*ForVirtualBase=*/false,
+ /*Delegating=*/false, CGF.LoadCXXThisAddress(),
+ ThisTy);
+ CGF.Builder.CreateBr(DontCallDtor);
+ CGF.EmitBlock(DontCallDtor);
+ }
llvm::BasicBlock *callDeleteBB = CGF.createBasicBlock("dtor.call_delete");
llvm::BasicBlock *continueBB = CGF.createBasicBlock("dtor.continue");
- llvm::Value *ShouldCallDelete
- = CGF.Builder.CreateIsNull(ShouldDeleteCondition);
+ // First bit set signals that operator delete must be called.
+ llvm::Value *Check1stBit = CGF.Builder.CreateAnd(
+ ShouldDeleteCondition, llvm::ConstantInt::get(CondTy, 1));
+ llvm::Value *ShouldCallDelete = CGF.Builder.CreateIsNull(Check1stBit);
CGF.Builder.CreateCondBr(ShouldCallDelete, continueBB, callDeleteBB);
CGF.EmitBlock(callDeleteBB);
- const CXXDestructorDecl *Dtor = cast<CXXDestructorDecl>(CGF.CurCodeDecl);
- const CXXRecordDecl *ClassDecl = Dtor->getParent();
- CGF.EmitDeleteCall(Dtor->getOperatorDelete(),
- LoadThisForDtorDelete(CGF, Dtor),
- CGF.getContext().getTagDeclType(ClassDecl));
- assert(Dtor->getOperatorDelete()->isDestroyingOperatorDelete() ==
- ReturnAfterDelete &&
- "unexpected value for ReturnAfterDelete");
- if (ReturnAfterDelete)
- CGF.EmitBranchThroughCleanup(CGF.ReturnBlock);
- else
- CGF.Builder.CreateBr(continueBB);
+ auto EmitDeleteAndGoToEnd = [&](const FunctionDecl *DeleteOp) {
+ CGF.EmitDeleteCall(DeleteOp, LoadThisForDtorDelete(CGF, Dtor),
+ CGF.getContext().getTagDeclType(ClassDecl));
+ if (ReturnAfterDelete)
+ CGF.EmitBranchThroughCleanup(CGF.ReturnBlock);
+ else
+ CGF.Builder.CreateBr(continueBB);
+ };
+ // If Sema only found a global operator delete previously, the dtor can
+ // always call it. Otherwise we need to check the third bit and call the
+ // appropriate operator delete, i.e. global or class-specific.
+ if (isa<CXXMethodDecl>(OD)) {
+ // Third bit set signals that global operator delete is called, i.e.
+ // ::delete appears on the callsite.
+ llvm::Value *CheckTheBitForGlobDeleteCall = CGF.Builder.CreateAnd(
+ ShouldDeleteCondition, llvm::ConstantInt::get(CondTy, 4));
+ llvm::Value *ShouldCallGlobDelete =
+ CGF.Builder.CreateIsNull(CheckTheBitForGlobDeleteCall);
+ llvm::BasicBlock *GlobDelete =
+ CGF.createBasicBlock("dtor.call_glob_delete");
+ llvm::BasicBlock *ClassDelete =
+ CGF.createBasicBlock("dtor.call_class_delete");
+ CGF.Builder.CreateCondBr(ShouldCallGlobDelete, ClassDelete, GlobDelete);
+ CGF.EmitBlock(GlobDelete);
+
+ const FunctionDecl *GlobOD = Dtor->getOperatorGlobalDelete();
+ if (GlobOD)
+ EmitDeleteAndGoToEnd(GlobOD);
+ else
+ CGF.Builder.CreateUnreachable();
+
+ CGF.EmitBlock(ClassDelete);
+ }
+ EmitDeleteAndGoToEnd(OD);
CGF.EmitBlock(continueBB);
}
diff --git a/clang/lib/CodeGen/MicrosoftCXXABI.cpp b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
index aa9a55ae05927..4128b7306687a 100644
--- a/clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -893,13 +893,8 @@ void MicrosoftCXXABI::emitVirtualObjectDelete(CodeGenFunction &CGF,
QualType ElementType,
const CXXDestructorDecl *Dtor) {
// FIXME: Provide a source location here even though there's no
- // CXXMemberCallExpr for dtor call.
- bool UseGlobalDelete = DE->isGlobalDelete();
- CXXDtorType DtorType = UseGlobalDelete ? Dtor_Complete : Dtor_Deleting;
- llvm::Value *MDThis = EmitVirtualDestructorCall(CGF, Dtor, DtorType, Ptr, DE,
- /*CallOrInvoke=*/nullptr);
- if (UseGlobalDelete)
- CGF.EmitDeleteCall(DE->getOperatorDelete(), MDThis, ElementType);
+ EmitVirtualDestructorCall(CGF, Dtor, Dtor_Deleting, Ptr, DE,
+ /*CallOrInvoke=*/nullptr);
}
void MicrosoftCXXABI::emitRethrow(CodeGenFunction &CGF, bool isNoReturn) {
@@ -2016,7 +2011,7 @@ llvm::Value *MicrosoftCXXABI::EmitVirtualDestructorCall(
ASTContext &Context = getContext();
llvm::Value *ImplicitParam = llvm::ConstantInt::get(
llvm::IntegerType::getInt32Ty(CGF.getLLVMContext()),
- DtorType == Dtor_Deleting);
+ (DtorType == Dtor_Deleting) | 4 * (D && D->isGlobalDelete()));
QualType ThisTy;
if (CE) {
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index cbccb567e2adf..258d78c2c9425 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -11400,6 +11400,21 @@ bool Sema::CheckDestructor(CXXDestructorDecl *Destructor) {
DiagnoseUseOfDecl(OperatorDelete, Loc);
MarkFunctionReferenced(Loc, OperatorDelete);
Destructor->setOperatorDelete(OperatorDelete, ThisArg);
+
+ if (isa<CXXMethodDecl>(OperatorDelete) &&
+ Context.getTargetInfo().getCXXABI().isMicrosoft()) {
+ // In Microsoft ABI whenever a class has a defined operator delete,
+ // scalar deleting destructors check the 3rd bit of the implicit
+ // parameter and if it is set, then, global operator delete must be
+ // called instead of class-specific one. Find and save global operator
+ // for that case. Do not diagnose because even if we fail to find the
+ // operator, it won't be possible to compile and execute the code that
+ // requires it.
+ FunctionDecl *GlobalOperatorDelete =
+ FindDeallocationFunctionForDestructor(Loc, RD, /*Diagnose*/ false,
+ /*LookForGlobal*/ true);
+ Destructor->setOperatorGlobalDelete(GlobalOperatorDelete);
+ }
}
}
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index b2a982e953012..e6b4a09db9d62 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -3565,7 +3565,7 @@ void Sema::DeclareGlobalAllocationFunction(DeclarationName Name,
FunctionDecl *
Sema::FindUsualDeallocationFunction(SourceLocation StartLoc,
ImplicitDeallocationParameters IDP,
- DeclarationName Name) {
+ DeclarationName Name, bool Diagnose) {
DeclareGlobalNewDelete();
LookupResult FoundDelete(*this, Name, StartLoc, LookupOrdinaryName);
@@ -3580,7 +3580,7 @@ Sema::FindUsualDeallocationFunction(SourceLocation StartLoc,
if (!Result)
return nullptr;
- if (CheckDeleteOperator(*this, StartLoc, StartLoc, /*Diagnose=*/true,
+ if (CheckDeleteOperator(*this, StartLoc, StartLoc, Diagnose,
FoundDelete.getNamingClass(), Result.Found,
Result.FD))
return nullptr;
@@ -3591,7 +3591,8 @@ Sema::FindUsualDeallocationFunction(SourceLocation StartLoc,
FunctionDecl *Sema::FindDeallocationFunctionForDestructor(SourceLocation Loc,
CXXRecordDecl *RD,
- bool Diagnose) {
+ bool Diagnose,
+ bool LookForGlobal) {
DeclarationName Name = Context.DeclarationNames.getCXXOperatorName(OO_Delete);
FunctionDecl *OperatorDelete = nullptr;
@@ -3600,18 +3601,20 @@ FunctionDecl *Sema::FindDeallocationFunctionForDestructor(SourceLocation Loc,
DeallocType, ShouldUseTypeAwareOperatorNewOrDelete(),
AlignedAllocationMode::No, SizedDeallocationMode::No};
- if (FindDeallocationFunction(Loc, RD, Name, OperatorDelete, IDP, Diagnose))
- return nullptr;
+ if (!LookForGlobal) {
+ if (FindDeallocationFunction(Loc, RD, Name, OperatorDelete, IDP, Diagnose))
+ return nullptr;
- if (OperatorDelete)
- return OperatorDelete;
+ if (OperatorDelete)
+ return OperatorDelete;
+ }
// If there's no class-specific operator delete, look up the global
// non-array delete.
IDP.PassAlignment = alignedAllocationModeFromBool(
hasNewExtendedAlignment(*this, DeallocType));
IDP.PassSize = SizedDeallocationMode::Yes;
- return FindUsualDeallocationFunction(Loc, IDP, Name);
+ return FindUsualDeallocationFunction(Loc, IDP, Name, Diagnose);
}
bool Sema::FindDeallocationFunction(SourceLocation StartLoc, CXXRecordDecl *RD,
diff --git a/clang/test/CodeGenCXX/cxx2a-destroying-delete.cpp b/clang/test/CodeGenCXX/cxx2a-destroying-delete.cpp
index f6f4a2ff735cc..8af2b42e98e7b 100644
--- a/clang/test/CodeGenCXX/cxx2a-destroying-delete.cpp
+++ b/clang/test/CodeGenCXX/cxx2a-destroying-delete.cpp
@@ -159,20 +159,43 @@ H::~H() { call_in_dtor(); }
// CHECK-ITANIUM-NOT: call
// CHECK-ITANIUM: }
-// CHECK-MSABI64-LABEL: define {{.*}} @"??_GH@@UEAAPEAXI@Z"(
+// CHECK-MSABI64-LABEL: define {{.*}} @"??_GH@@UEAAPEAXI@Z"({{.*}},
// CHECK-MSABI32-LABEL: define {{.*}} @"??_GH@@UAEPAXI@Z"(
+// CHECK-MSABI-SAME: i32 noundef %[[IP:.*]])
// CHECK-MSABI-NOT: call{{ }}
-// CHECK-MSABI: load i32
-// CHECK-MSABI: icmp eq i32 {{.*}}, 0
-// CHECK-MSABI: br i1
+// CHECK-MSABI: store i32 %[[IP]], ptr %[[IP_ALLOCA:.*]]
+// CHECK-MSABI: %[[IMP_PARAM:.*]] = load i32, ptr %[[IP_ALLOCA]]
+// CHECK-MSABI-NEXT: %[[THIRDBIT:.*]] = and i32 %[[IMP_PARAM]], 4
+// CHECK-MSABI-NEXT: %[[CHCK:.*]] = icmp eq i32 %[[THIRDBIT]], 0
+// CHECK-MSABI-NEXT: br i1 %[[CHCK]], label %dtor.entry_cont, label %dtor.call_dtor
+//
+// CHECK_MSABI-LABEL: dtor.call_dtor:
+// CHECK_MSABI-NEXT: call void @"??1H@@UEAA@XZ"({{.*}})
+// CHECK_MSABI-NEXT: br label %dtor.entry_cont
+//
+// CHECK_MSABI-LABEL: dtor.entry_cont:
+// CHECK_MSABI-NEXT: %[[FIRSTBIT:.*]] = and i32 %[[IMP_PARAM]], 1
+// CHECK_MSABI-NEXT: %[[CHCK1:.*]] = icmp eq i32 %[[FIRSTBIT]], 0
+// CHECK_MSABI-NEXT: br i1 %[[CHCK1]], label %dtor.continue, label %dtor.call_delete
+//
+// CHECK_MSABI-LABEL: dtor.call_delete:
+// CHECK-MSABI: %[[THIRDBIT1:.*]] = and i32 %[[IMP_PARAM]], 4
+// CHECK-MSABI-NEXT: %[[CHCK2:.*]] = icmp eq i32 %[[THIRDBIT1]], 0
+// CHECK-MSABI-NEXT: br i1 %[[CHCK2]], label %dtor.call_class_delete, label %dtor.call_glob_delete
+//
+// CHECK-MSABI-LABEL: dtor.call_glob_delete:
+// CHECK-MSABI64: call void @"??3@YAXPEAX_K@Z"(ptr noundef %{{.*}}, i64 noundef 48)
+// CHECK-MSABI32: call void @"??3@YAXPAXIW4align_val_t@std@@@Z"(ptr noundef %{{.*}}, i32 noundef 32, i32 noundef 16)
+// CHECK-MSABI-NEXT: br label %[[RETURN:.*]]
//
+// CHECK-MSABI: dtor.call_class_delete:
// CHECK-MSABI-NOT: call{{ }}
// CHECK-MSABI64: getelementptr {{.*}}, i64 24
// CHECK-MSABI32: getelementptr {{.*}}, i32 20
// CHECK-MSABI-NOT: call{{ }}
// CHECK-MSABI64: call void @"??3F@@SAXPEAU0@Udestroying_delete_t@std@@_KW4align_val_t@2@@Z"({{.*}}, i64 noundef 48, i64 noundef 16)
// CHECK-MSABI32: call void @"??3F@@SAXPAU0@Udestroying_delete_t@std@@IW4align_val_t@2@@Z"({{.*}}, i32 noundef 32, i32 noundef 16)
-// CHECK-MSABI: br label %[[RETURN:.*]]
+// CHECK-MSABI: br label %[[RETURN:]]
//
// CHECK-MSABI64: call void @"??1H@@UEAA@XZ"(
// CHECK-MSABI32: call x86_thiscallcc void @"??1H@@UAE@XZ"(
@@ -194,9 +217,31 @@ I::~I() { call_in_dtor(); }
// CHECK-MSABI32-LABEL: define {{.*}} @"??_GI@@UAEPAXI@Z"(
// CHECK-MSABI-NOT: call{{ }}
// CHECK-MSABI: load i32
-// CHECK-MSABI: icmp eq i32 {{.*}}, 0
-// CHECK-MSABI: br i1
+// CHECK-MSABI-NEXT: and i32 %[[IMP_PARAM:.*]], 4
+// CHECK-MSABI-NEXT: icmp eq i32 {{.*}}, 0
+// CHECK-MSABI-NEXT: br i1 %[[CHCK]], label %dtor.entry_cont, label %dtor.call_dtor
+//
+// CHECK-MSABI: dtor.call_dtor:
+// CHECK-MSABI64-NEXT: call void @"??1I@@UEAA@XZ"({{.*}})
+// CHECK-MSABI32-NEXT: call x86_thiscallcc void @"??1I@@UAE@XZ"({{.*}})
+// CHECK-MSABI-NEXT: br label %dtor.entry_cont
+//
+// CHECK-MSABI: dtor.entry_cont:
+// CHECK-MSABI-NEXT: and i32 %[[IMP_PARAM]], 1
+// CHECK-MSABI-NEXT: icmp eq i32 %{{.*}}, 0
+// CHECK-MSABI-NEXT: br i1 %{{.*}}, label %dtor.continue, label %dtor.call_delete
+//
+// CHECK-MSABI: dtor.call_delete:
+// CHECK-MSABI-NEXT: %[[THIRDBIT1:.*]] = and i32 %[[IMP_PARAM]], 4
+// CHECK-MSABI-NEXT: %[[CHCK2:.*]] = icmp eq i32 %[[THIRDBIT1]], 0
+// CHECK-MSABI-NEXT: br i1 %[[CHCK2]], label %dtor.call_class_delete, label %dtor.call_glob_delete
+//
+// CHECK-MSABI-LABEL: dtor.call_glob_delete:
+// CHECK-MSABI64: call void @"??3@YAXPEAX_KW4align_val_t@std@@@Z"(ptr noundef %{{.*}}, i64 noundef 96, i64 noundef 32)
+// CHECK-MSABI32: call void @"??3@YAXPAXIW4align_val_t@std@@@Z"(ptr noundef %{{.*}}, i32 noundef 64, i32 noundef 32)
+// CHECK-MSABI-NEXT: br label %[[RETURN:.*]]
//
+// CHECK_MSABI: dtor.call_class_delete:
// CHECK-MSABI-NOT: call{{ }}
// CHECK-MSABI64: getelementptr {{.*}}, i64 24
// CHECK-MSABI32: getelementptr {{.*}}, i32 20
diff --git a/clang/test/CodeGenCXX/microsoft-abi-structors.cpp b/clang/test/CodeGenCXX/microsoft-abi-structors.cpp
index 07abc3d065e5e..9b43483439cf8 100644
--- a/clang/test/CodeGenCXX/microsoft-abi-structors.cpp
+++ b/clang/test/CodeGenCXX/microsoft-abi-structors.cpp
@@ -52,7 +52,8 @@ struct C {
// DTORS: store ptr %{{.*}}, ptr %[[RETVAL:retval]]
// DTORS: %[[SHOULD_DELETE_VALUE:[0-9a-z._]+]] = load i32, ptr %[[SHOULD_DELETE_VAR]]
// DTORS: call x86_thiscallcc void @"??1C@basic@@UAE@XZ"(ptr {{[^,]*}} %[[THIS:[0-9a-z]+]])
-// DTORS-NEXT: %[[CONDITION:[0-9]+]] = icmp eq i32 %[[SHOULD_DELETE_VALUE]], 0
+// DTORS-NEXT: %[[AND:[0-9]+]] = and i32 %[[SHOULD_DELETE_VALUE]], 1
+// DTORS-NEXT: %[[CONDITION:[0-9]+]] = icmp eq i32 %[[AND]], 0
// DTORS-NEXT: br i1 %[[CONDITION]], label %[[CONTINUE_LABEL:[0-9a-z._]+]], label %[[CALL_DELETE_LABEL:[0-9a-z._]+]]
//
// DTORS: [[CALL_DELETE_LABEL]]
@@ -113,8 +114,7 @@ void call_deleting_dtor_and_global_delete(C *obj_ptr) {
// CHECK-NEXT: %[[VTABLE:.*]] = load ptr, ptr %[[OBJ_PTR_VALUE]]
// CHECK-NEXT: %[[PVDTOR:.*]] = getelementptr inbounds ptr, ptr %[[VTABLE]], i64 0
// CHECK-NEXT: %[[VDTOR:.*]] = load ptr, ptr %[[PVDTOR]]
-// CHECK-NEXT: %[[CALL:.*]] = call x86_thiscallcc ptr %[[VDTOR]](ptr {{[^,]*}} %[[OBJ_PTR_VALUE]], i32 0)
-// CHECK-NEXT: call void @"??3@YAXPAX@Z"(ptr %[[CALL]])
+// CHECK-NEXT: %[[CALL:.*]] = call x86_thiscallcc ptr %[[VDTOR]](ptr {{[^,]*}} %[[OBJ_PTR_VALUE]], i32 5)
// CHECK: ret void
}
@@ -458,3 +458,41 @@ class G {
extern void testG() {
G g;
}
+
+namespace operator_delete {
+
+class H { virtual ~H();
+ void operator delete(void *);
+};
+H::~H() { }
+
+void checkH() {
+ new H();
+}
+// DTORS: define linkonce_odr dso_local x86_thiscallcc ptr @"??_GH@operator_delete@@EAEPAXI@Z"(ptr {{[^,]*}} %this, i32 %should_call_delete) {{.*}} comdat {{.*}} {
+// DTORS: store i32 %should_call_delete, ptr %[[SHOULD_DELETE_VAR:[0-9a-z._]+]], align 4
+// DTORS: store ptr %{{.*}}, ptr %[[RETVAL:retval]]
+// DTORS: %[[SHOULD_DELETE_VALUE:[0-9a-z._]+]] = load i32, ptr %[[SHOULD_DELETE_VAR]]
+// DTORS: call x86_thiscallcc void @"??1H@operator_delete@@EAE@XZ"(ptr {{[^,]*}} %[[THIS:[0-9a-z]+]])
+// DTORS-NEXT: %[[AND:[0-9]+]] = and i32 %[[SHOULD_DELETE_VALUE]], 1
+// DTORS-NEXT: %[[CONDITION:[0-9]+]] = icmp eq i32 %[[AND]], 0
+// DTORS-NEXT: br i1 %[[CONDITION]], label %[[CONTINUE_LABEL:[0-9a-z._]+]], label %[[CALL_DELETE_LABEL:[0-9a-z._]+]]
+//
+// DTORS: [[CALL_DELETE_LABEL]]
+// DTORS-NEXT: %[[AND:[0-9]+]] = and i32 %[[SHOULD_DELETE_VALUE]], 4
+// DTORS-NEXT: %[[CONDITION1:[0-9]+]] = icmp eq i32 %[[AND]], 0
+// DTORS-NEXT: br i1 %[[CONDITION1]], label %[[CALL_CLASS_DELETE:[0-9a-z._]+]], label %[[CALL_GLOB_DELETE:[0-9a-z._]+]]
+//
+// DTORS: [[CALL_GLOB_DELETE]]
+// DTORS-NEXT: call void @"??3@YAXPAX@Z"(ptr %[[THIS]])
+// DTORS-NEXT: br label %[[CONTINUE_LABEL]]
+//
+// DTORS: [[CALL_CLASS_DELETE]]
+// DTORS-NEXT: call void @"??3H@operator_delete@@CAXPAX@Z"(ptr %[[THIS]])
+// DTORS-NEXT: br label %[[CONTINUE_LABEL]]
+//
+// DTORS: [[CONTINUE_LABEL]]
+// DTORS-NEXT: %[[RET:.*]] = load ptr, ptr %[[RETVAL]]
+// DTORS-NEXT: ret ptr %[[RET]]
+
+}
|
|
Ping. |
|
ping. |
|
Thanks for review @efriedma-quic . |
I think this is a case where we'd call out the breaking change in the release notes and if anyone needed the older ABI, we'd use ABI version tags for it at that point. CC @llvm/clang-vendors for awareness for the potentially breaking changes. |
|
Ok, I added the release note.
@AaronBallman Could you please elaborate a bit more on that? |
We have a command line option, llvm-project/clang/lib/AST/DeclCXX.cpp Line 1002 in edf636a |
|
Thanks @AaronBallman , It seems -fclang-abi-compat=20 doesn't work, so first we need #144109 . |
There was a problem hiding this comment.
Thanks for working on this @Fznamznon! I added some suggestions and comments, most of which are minor. There are two concerns that must be addressed.
- The ABI impact suggests this change should be tied to
-fclang-abi-compat=as has already been discussed. - AST serialization support is needed for the new
OperatorGlobalDeletemember ofCXXDestructorDecl.
clang/include/clang/AST/DeclCXX.h
Outdated
| if (OD && !First->OperatorGlobalDelete) | ||
| First->OperatorGlobalDelete = OD; |
There was a problem hiding this comment.
I think a call to setOperatorGlobalDelete(nullptr) should clear OperatorGlobalDelete.
Hmm, setOperatorDelete() silently ignores a call that passes nullptr. That seems wrong. Maybe there should be a non-null assertion?
setOperatorDelete() also registers the delete operator with ASTMutationListener::ResolvedOperatorDelete() and ASTWriter overrides that function. That seems important for PCH and module support. I think updates are needed to clang/lib/Serialization/ASTReaderDecl.cpp as well to restore OperatorGlobalDelete during AST deserialization; see ASTDeclReader::VisitCXXDestructorDecl() and ASTDeclReader::UpdateDecl() for assignments to OperatorDelete.
There was a problem hiding this comment.
Hmm, what would be the better approach to implement this? Should I invent ResolvedOperatorDelete but for global operator delete or can we tie global operator delete to a regular one like we do with ThisArg in setOperatorDelete() and other serialization parts?
cc @ChuanqiXu9
There was a problem hiding this comment.
Good question. Adding a parallel set of functions seems reasonable to me. It might also be reasonable to tie the updates together, but I think that might unnecessarily increase the serialized AST size for Itanium targets. Perhaps that can be avoided in either case; it isn't obvious to me.
While looking at this some more, I noticed that it looks like updates will be needed to clang/lib/AST/ASTImporter.cpp as well; see the call to setOperatorDelete() in ASTNodeImporter::VisitFunctionDecl().
There was a problem hiding this comment.
@tahonermann , added AST serialization support along with tests.
| // | ||
| // CHECK_MSABI: dtor.call_class_delete: |
There was a problem hiding this comment.
I think the additions here look good, but we're missing a test for a call to ::delete for a class that declares a destroying operator delete. Such a test is interesting for both the Itanium and MSVC ABIs for different reasons (the complete object destructor is called followed by global operator delete for Itanium; the deleting destructor is called and it calls the complete destructor and then the global operator delete for MSVC).
There was a problem hiding this comment.
Added the test. I noticed though that MSVC always calls scalar deleting dtor even when class declares non-virtual destructor. clang doesn't even generate deleting destructors when class destructor is not virtual. I'm not yet sure which implications it has, need more experimenting. At least unlike the virtual case the wrong operator delete is not called.
There was a problem hiding this comment.
Hmm, I'm not seeing that with this simple example (https://godbolt.org/z/xdThaEb78). g() calls the complete destructor followed by operator delete.
struct S {
~S();
};
S* f() {
return new S;
}
void g(S *p) {
::delete p;
}
There was a problem hiding this comment.
Nice, thanks. I probably hallucinated. I wasn't able to break this case while testing object files built with clang and msvc either.
Co-authored-by: Tom Honermann <[email protected]>
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
A few more comments. I'm still digesting the changes to EmitConditionalDtorDeleteCall(). If I'm reading the code correctly, CodeGenFunction::EmitDestructorBody() will already emit a call to the complete destructor unconditionally (assuming HaveInsertPoint() returns true) after the call to EnterDtorCleanups() (which calls EmitConditionalDtorDeleteCall()). I could easily be misreading things though; I might have to hop into a debugger. At a minimum, I'll have some additional suggestions for comments intended to make things more clear.
clang/lib/AST/DeclCXX.cpp
Outdated
| assert(!OD || (OD->getDeclName().getCXXOverloadedOperator() == OO_Delete && | ||
| OD->isUsableAsGlobalAllocationFunctionInConstantEvaluation())); |
There was a problem hiding this comment.
Per previous discussion, I don't think isUsableAsGlobalAllocationFunctionInConstantEvaluation() performs quite the right set of checks. It is really close though. That function will return true for a delete operator that has a trailing std::nothrow_t parameter; such declarations are not included in the usual deallocation function requirements (and ::delete will not call such a function; it won't manifest a std::nothrow_t argument).
I suggest we assert that the function is a delete operator declared at global scope and add a FIXME comment regarding the usual deallocation function requirement.
| assert(!OD || (OD->getDeclName().getCXXOverloadedOperator() == OO_Delete && | |
| OD->isUsableAsGlobalAllocationFunctionInConstantEvaluation())); | |
| // FIXME: C++23 [expr.delete] specifies that the delete operator will be | |
| // a usual deallocation function declared at global scope. A convenient | |
| // function to assert that is lacking; Sema::isUsualDeallocationFunction() | |
| // only works for CXXMethodDecl. | |
| assert(!OD || (OD->getDeclName().getCXXOverloadedOperator() == OO_Delete && | |
| OD->getDeclContext()->getRedeclContext()->isTranslationUnit())); |
clang/lib/CodeGen/CGClass.cpp
Outdated
| // Clang 20 calls global operator delete after dtor call. Clang 21 and newer | ||
| // call global operator delete inside of dtor body, as MSVC does. | ||
| bool Clang21AndNewer = CGF.getContext().getLangOpts().getClangABICompat() > | ||
| LangOptions::ClangABI::Ver20; |
There was a problem hiding this comment.
The ABI check occurs in multiple places. Can this check be encapsulated in a common function somewhere? Perhaps a TargetInfo member function named callGlobalDeleteInDestroyingDtor() or similar? There is precedent in terms of TargetInfo::getCallingConvKind() and TargetInfo::areDefaultedSMFStillPOD(). Or perhaps an ABIInfo member function? There is precedent in terms of ARMABIInfo::shouldIgnoreEmptyArg(), X86_64ABIInfo::classifyIntegerMMXAsSSE(), X86_64ABIInfo::passInt128VectorsInMem(), and X86_64ABIInfo::returnCXXRecordGreaterThan128InMem().
There was a problem hiding this comment.
Ok, I added a member to TargetInfo which seems to be easily available in both Sema and CodeGen. Thanks.
| // | ||
| // CHECK_MSABI: dtor.call_class_delete: |
There was a problem hiding this comment.
Hmm, I'm not seeing that with this simple example (https://godbolt.org/z/xdThaEb78). g() calls the complete destructor followed by operator delete.
struct S {
~S();
};
S* f() {
return new S;
}
void g(S *p) {
::delete p;
}
@tahonermann , yes, at some basic block there will be always a call to complete destructor inside of scalar deleting dtor body. The difference is whether the class has a destroying operator delete or not. If it does, then the delete operator is responsible for calling complete dtor. So we first call llvm-project/clang/lib/CodeGen/CGClass.cpp Line 1861 in 90f3147 Which actually causes it to be called after |
While working on vector deleting destructors support (GH19772), I noticed that MSVC produces different code in scalar deleting destructor body depending on whether class defined its own operator delete. In MSABI deleting destructors accept an additional implicit flag parameter allowing some sort of flexibility. The mismatch I noticed is that whenever a global operator delete is called, i.e.
::delete, in the code produced by MSVC the implicit flag argument has a value that makes the 3rd bit set, i.e. "5" for scalar deleting destructors "7" for vector deleting destructors.Prior to this patch, clang handled
::deletevia calling global operator delete direct after the destructor call and not calling class operator delete in scalar deleting destructor body by passing "0" as implicit flag argument value. This is fine until there is an attempt to link binaries compiled with clang with binaries compiled with MSVC. The problem is that in binaries produced by MSVC the callsite of the destructor won't call global operator delete because it is assumed that the destructor will do that and a destructor body generated by clang will never do.This patch removes call to global operator delete from the callsite and add additional check of the 3rd bit of the implicit parameter inside of scalar deleting destructor body.