-
Notifications
You must be signed in to change notification settings - Fork 15k
[win][clang] Align scalar deleting destructors with MSABI #139566
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 24 commits
1b0b624
d945d0e
2e07469
2c51545
9493c05
ef899c2
94f43ae
83a8f06
c9efdf8
f5e7a45
9d3c96c
d71d3d4
c670d74
7147603
057ac9f
05c386a
d07da86
92bbda6
83665e2
c214b69
bc55453
7b238c9
6150d04
c0a863f
d74fc8a
a852a3e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1580,29 +1580,85 @@ namespace { | |||||||||||||
| } | ||||||||||||||
| }; | ||||||||||||||
|
|
||||||||||||||
| // This function implements generation of scalar deleting destructor body for | ||||||||||||||
| // the case when the destructor also accepts an implicit flag. Right now only | ||||||||||||||
| // Microsoft ABI requires deleting destructors to accept implicit flags. | ||||||||||||||
| // The flag indicates whether an operator delete should be called and whether | ||||||||||||||
| // it should be a class-specific operator delete or a global one. | ||||||||||||||
| void EmitConditionalDtorDeleteCall(CodeGenFunction &CGF, | ||||||||||||||
tahonermann marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
| 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()); | ||||||||||||||
| // MSVC calls global operator delete inside of dtor body, but clang aligned | ||||||||||||||
| // with this behavior only after a particular version and started emitting | ||||||||||||||
| // code that is not ABI-compatible with previous versions. | ||||||||||||||
|
||||||||||||||
| // MSVC calls global operator delete inside of dtor body, but clang aligned | |
| // with this behavior only after a particular version and started emitting | |
| // code that is not ABI-compatible with previous versions. | |
| // MSVC calls global operator delete inside of the dtor body, but clang aligned | |
| // with this behavior only after a particular version. This is not | |
| // ABI-compatible with previous versions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks!
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -901,12 +901,19 @@ void MicrosoftCXXABI::emitVirtualObjectDelete(CodeGenFunction &CGF, | |
| const CXXDestructorDecl *Dtor) { | ||
| // FIXME: Provide a source location here even though there's no | ||
| // CXXMemberCallExpr for dtor call. | ||
Fznamznon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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); | ||
| if (!getContext().getTargetInfo().callGlobalDeleteInDeletingDtor( | ||
| getContext().getLangOpts())) { | ||
| 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); | ||
| } else { | ||
| EmitVirtualDestructorCall(CGF, Dtor, Dtor_Deleting, Ptr, DE, | ||
| /*CallOrInvoke=*/nullptr); | ||
| } | ||
| } | ||
|
|
||
| void MicrosoftCXXABI::emitRethrow(CodeGenFunction &CGF, bool isNoReturn) { | ||
|
|
@@ -2021,7 +2028,10 @@ 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() && | ||
|
||
| Context.getTargetInfo().callGlobalDeleteInDeletingDtor( | ||
| Context.getLangOpts()))); | ||
|
|
||
| QualType ThisTy; | ||
| if (CE) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having multiple defaulted bool arguments is really confusing/easy to misuse; do these really need to be defaulted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems they don't, there are only 2 callsites of the function so I changed these to be not defailted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did something happen to the latest changes here? It looks like the version that got merged doesn't address the review comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm, I would say it has been addressed, see
llvm-project/clang/include/clang/Sema/Sema.h
Line 8562 in ff4c499
Did you also mean to not add defaulted arguments to
FindUsualDeallocationFunction? It has only one. Should I push a change to remove it too?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind; I was looking at the wrong version of the patch somehow.