[clang] Do not diagnose unused deleted operator delete[]#134357
Merged
[clang] Do not diagnose unused deleted operator delete[]#134357
Conversation
For vector deleting dtors support we now also search and save operator delete[]. Avoid diagnosing deleted operator delete[] when doing that because vector deleting dtors are only called when delete[] is present and whenever delete[] is present in the TU it will be diagnosed correctly. Fixes llvm#134265
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang Author: Mariya Podchishchaeva (Fznamznon) ChangesFor vector deleting dtors support we now also search and save operator delete[]. Avoid diagnosing deleted operator delete[] when doing that because vector deleting dtors are only called when delete[] is present and whenever delete[] is present in the TU it will be diagnosed correctly. Fixes #134265 Full diff: https://github.com/llvm/llvm-project/pull/134357.diff 6 Files Affected:
diff --git a/clang/include/clang/AST/DeclCXX.h b/clang/include/clang/AST/DeclCXX.h
index 764f85b04e6a0..56cec07ec0293 100644
--- a/clang/include/clang/AST/DeclCXX.h
+++ b/clang/include/clang/AST/DeclCXX.h
@@ -2878,7 +2878,7 @@ class CXXDestructorDecl : public CXXMethodDecl {
static CXXDestructorDecl *CreateDeserialized(ASTContext &C, GlobalDeclID ID);
void setOperatorDelete(FunctionDecl *OD, Expr *ThisArg);
- void setOperatorArrayDelete(FunctionDecl *OD, Expr *ThisArg);
+ void setOperatorArrayDelete(FunctionDecl *OD);
const FunctionDecl *getOperatorDelete() const {
return getCanonicalDecl()->OperatorDelete;
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index b835697f99670..6bf1caf6bdd18 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -8336,7 +8336,8 @@ class Sema final : public SemaBase {
DeclarationName Name);
FunctionDecl *FindDeallocationFunctionForDestructor(SourceLocation StartLoc,
CXXRecordDecl *RD,
- DeclarationName Name);
+ DeclarationName Name,
+ bool Diagnose = true);
/// ActOnCXXDelete - Parsed a C++ 'delete' expression (C++ 5.3.5), as in:
/// @code ::delete ptr; @endcode
diff --git a/clang/lib/AST/DeclCXX.cpp b/clang/lib/AST/DeclCXX.cpp
index 7aa710ad7309b..fffc50eb0b078 100644
--- a/clang/lib/AST/DeclCXX.cpp
+++ b/clang/lib/AST/DeclCXX.cpp
@@ -3031,8 +3031,7 @@ void CXXDestructorDecl::setOperatorDelete(FunctionDecl *OD, Expr *ThisArg) {
}
}
-void CXXDestructorDecl::setOperatorArrayDelete(FunctionDecl *OD,
- Expr *ThisArg) {
+void CXXDestructorDecl::setOperatorArrayDelete(FunctionDecl *OD) {
auto *First = cast<CXXDestructorDecl>(getFirstDecl());
if (OD && !First->OperatorArrayDelete)
First->OperatorArrayDelete = OD;
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 07379c6876731..b86f7118e0b34 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -11048,12 +11048,12 @@ bool Sema::CheckDestructor(CXXDestructorDecl *Destructor) {
// Lookup delete[] too in case we have to emit a vector deleting dtor;
DeclarationName VDeleteName =
Context.DeclarationNames.getCXXOperatorName(OO_Array_Delete);
- FunctionDecl *ArrOperatorDelete =
- FindDeallocationFunctionForDestructor(Loc, RD, VDeleteName);
+ FunctionDecl *ArrOperatorDelete = FindDeallocationFunctionForDestructor(
+ Loc, RD, VDeleteName, /*Diagnose=*/false);
// delete[] in the TU will make sure the operator is referenced and its
// uses diagnosed, otherwise vector deleting dtor won't be called anyway,
// so just record it in the destructor.
- Destructor->setOperatorArrayDelete(ArrOperatorDelete, ThisArg);
+ Destructor->setOperatorArrayDelete(ArrOperatorDelete);
}
}
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index e43f5e3f75bfe..d5f52cd5853f0 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -3265,11 +3265,13 @@ FunctionDecl *Sema::FindUsualDeallocationFunction(SourceLocation StartLoc,
return Result.FD;
}
-FunctionDecl *Sema::FindDeallocationFunctionForDestructor(
- SourceLocation Loc, CXXRecordDecl *RD, DeclarationName Name) {
+FunctionDecl *Sema::FindDeallocationFunctionForDestructor(SourceLocation Loc,
+ CXXRecordDecl *RD,
+ DeclarationName Name,
+ bool Diagnose) {
FunctionDecl *OperatorDelete = nullptr;
- if (FindDeallocationFunction(Loc, RD, Name, OperatorDelete))
+ if (FindDeallocationFunction(Loc, RD, Name, OperatorDelete, Diagnose))
return nullptr;
if (OperatorDelete)
return OperatorDelete;
diff --git a/clang/test/SemaCXX/gh134265.cpp b/clang/test/SemaCXX/gh134265.cpp
new file mode 100644
index 0000000000000..c7bdeb2add0cc
--- /dev/null
+++ b/clang/test/SemaCXX/gh134265.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only
+
+struct Foo {
+ virtual ~Foo() {} // expected-error {{attempt to use a deleted function}}
+ static void operator delete(void* ptr) = delete; // expected-note {{explicitly marked deleted here}}
+};
+
+
+struct Bar {
+ virtual ~Bar() {}
+ static void operator delete[](void* ptr) = delete;
+};
+
+struct Baz {
+ virtual ~Baz() {}
+ static void operator delete[](void* ptr) = delete; // expected-note {{explicitly marked deleted here}}
+};
+
+void foobar() {
+ Baz *B = new Baz[10]();
+ delete [] B; // expected-error {{attempt to use a deleted function}}
+}
|
AaronBallman
approved these changes
Apr 4, 2025
Collaborator
AaronBallman
left a comment
AaronBallman
left a comment
There was a problem hiding this comment.
LGTM but do we need a release note?
|
|
||
| void setOperatorDelete(FunctionDecl *OD, Expr *ThisArg); | ||
| void setOperatorArrayDelete(FunctionDecl *OD, Expr *ThisArg); | ||
| void setOperatorArrayDelete(FunctionDecl *OD); |
Collaborator
There was a problem hiding this comment.
This looks like unrelated changes, but the changes themselves are correct.
Contributor
Author
There was a problem hiding this comment.
Yeah, this is a drive-by removal of unused argument added by a patch that caused the bug this patch is fixing.
Contributor
Author
There was a problem hiding this comment.
Should I do that in a separate commit?
Collaborator
There was a problem hiding this comment.
No, I think it's related enough as a drive-by it's fine.
Contributor
Author
No, it is a fix for a regression caused by a patch committed a couple of days ago. |
Collaborator
Excellent, thank you for confirming! LG as-is |
Collaborator
|
We're seeing crashes that bisect to this change. Here is a reproducer: https://crbug.com/410001969#comment3 I'll see if I can get something more reduced as well. |
Collaborator
|
Smaller repro: |
zmodem
pushed a commit
that referenced
this pull request
Apr 14, 2025
Finding operator delete[] is still problematic, without it the extension is a security hazard, so reverting until the problem with operator delete[] is figured out. This reverts the following PRs: Reland [MS][clang] Add support for vector deleting destructors (#133451) [MS][clang] Make sure vector deleting dtor calls correct operator delete (#133950) [MS][clang] Fix crash on deletion of array of pointers (#134088) [clang] Do not diagnose unused deleted operator delete[] (#134357) [MS][clang] Error about ambiguous operator delete[] only when required (#135041)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
For vector deleting dtors support we now also search and save operator delete[]. Avoid diagnosing deleted operator delete[] when doing that because vector deleting dtors are only called when delete[] is present and whenever delete[] is present in the TU it will be diagnosed correctly.
Fixes #134265