https://github.com/AaronBallman updated
https://github.com/llvm/llvm-project/pull/118800
>From 34d3d3000bc6096bbc9eb35ce85b6ceca50b91ca Mon Sep 17 00:00:00 2001
From: Aaron Ballman <aa...@aaronballman.com>
Date: Thu, 5 Dec 2024 08:31:24 -0500
Subject: [PATCH 1/5] [C++20] Destroying delete and deleted destructors
When a destroying delete overload is selected, the destructor is not
automatically called. Therefore, the destructor can be deleted without
causing the program to be ill-formed.
---
clang/docs/ReleaseNotes.rst | 2 ++
clang/lib/Sema/SemaExprCXX.cpp | 6 +++--
.../CXX/expr/expr.unary/expr.delete/p10.cpp | 22 +++++++++++++++++--
3 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index e484eb7a76e63a..4a72e4046e2d03 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -663,6 +663,8 @@ Bug Fixes in This Version
- Fixed a crash when GNU statement expression contains invalid statement
(#GH113468).
- Fixed a failed assertion when using ``__attribute__((noderef))`` on an
``_Atomic``-qualified type (#GH116124).
+- No longer incorrectly diagnosing use of a deleted destructor when the
+ selected overload of ``operator delete`` for that type is a destroying
delete.
Bug Fixes to Compiler Builtins
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index db9ea7fb66e05a..45840dfa31ac92 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -3792,13 +3792,15 @@ Sema::ActOnCXXDelete(SourceLocation StartLoc, bool
UseGlobal,
.HasSizeT;
}
- if (!PointeeRD->hasIrrelevantDestructor())
+ if (!PointeeRD->hasIrrelevantDestructor() &&
+ (!OperatorDelete || !OperatorDelete->isDestroyingOperatorDelete())) {
if (CXXDestructorDecl *Dtor = LookupDestructor(PointeeRD)) {
MarkFunctionReferenced(StartLoc,
- const_cast<CXXDestructorDecl*>(Dtor));
+ const_cast<CXXDestructorDecl *>(Dtor));
if (DiagnoseUseOfDecl(Dtor, StartLoc))
return ExprError();
}
+ }
CheckVirtualDtorCall(PointeeRD->getDestructor(), StartLoc,
/*IsDelete=*/true, /*CallCanBeVirtual=*/true,
diff --git a/clang/test/CXX/expr/expr.unary/expr.delete/p10.cpp
b/clang/test/CXX/expr/expr.unary/expr.delete/p10.cpp
index aad2747dd32f24..b2c0a2c79695fc 100644
--- a/clang/test/CXX/expr/expr.unary/expr.delete/p10.cpp
+++ b/clang/test/CXX/expr/expr.unary/expr.delete/p10.cpp
@@ -1,7 +1,14 @@
-// RUN: %clang_cc1 -std=c++1z -verify %s
+// RUN: %clang_cc1 -std=c++20 -verify %s
using size_t = decltype(sizeof(0));
-namespace std { enum class align_val_t : size_t {}; }
+namespace std {
+ enum class align_val_t : size_t {};
+ struct destroying_delete_t {
+ explicit destroying_delete_t() = default;
+ };
+
+ inline constexpr destroying_delete_t destroying_delete{};
+}
// Aligned version is preferred over unaligned version,
// unsized version is preferred over sized version.
@@ -23,3 +30,14 @@ struct alignas(Align) B {
};
void f(B<__STDCPP_DEFAULT_NEW_ALIGNMENT__> *p) { delete p; }
void f(B<__STDCPP_DEFAULT_NEW_ALIGNMENT__ * 2> *p) { delete p; } //
expected-error {{deleted}}
+
+// Ensure that a deleted destructor is acceptable when the selected overload
+// for operator delete is a destroying delete. See the comments in GH118660.
+struct S {
+ ~S() = delete;
+ void operator delete(S *, std::destroying_delete_t) noexcept {}
+};
+
+void foo(S *s) {
+ delete s; // Was rejected, is intended to be accepted.
+}
>From 133a8aa2934f9d6ed733e74af65180131d59cc91 Mon Sep 17 00:00:00 2001
From: Aaron Ballman <aa...@aaronballman.com>
Date: Thu, 5 Dec 2024 10:14:46 -0500
Subject: [PATCH 2/5] Update based on review feedback
* Added a standards reference
* Added a test case
* Fixed an issue the new test case identified
---
clang/lib/Sema/SemaExprCXX.cpp | 13 +++++++++++--
clang/test/CXX/expr/expr.unary/expr.delete/p10.cpp | 10 ++++++++++
2 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index 45840dfa31ac92..6ac44ae7af28c3 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -3792,6 +3792,14 @@ Sema::ActOnCXXDelete(SourceLocation StartLoc, bool
UseGlobal,
.HasSizeT;
}
+ // C++20 [expr.delete]p6: If the value of the operand of the delete-
+ // expression is not a null pointer value and the selected deallocation
+ // function (see below) is not a destroying operator delete, the delete-
+ // expression will invoke the destructor (if any) for the object or the
+ // elements of the array being deleted.
+ //
+ // This means we should not look at the destructor for a destroying
+ // delete operator, as that destructor is never called.
if (!PointeeRD->hasIrrelevantDestructor() &&
(!OperatorDelete || !OperatorDelete->isDestroyingOperatorDelete())) {
if (CXXDestructorDecl *Dtor = LookupDestructor(PointeeRD)) {
@@ -3831,9 +3839,10 @@ Sema::ActOnCXXDelete(SourceLocation StartLoc, bool
UseGlobal,
MarkFunctionReferenced(StartLoc, OperatorDelete);
// Check access and ambiguity of destructor if we're going to call it.
- // Note that this is required even for a virtual delete.
+ // Note that this is required even for a virtual delete, but not for a
+ // destroying delete operator.
bool IsVirtualDelete = false;
- if (PointeeRD) {
+ if (PointeeRD && !OperatorDelete->isDestroyingOperatorDelete()) {
if (CXXDestructorDecl *Dtor = LookupDestructor(PointeeRD)) {
CheckDestructorAccess(Ex.get()->getExprLoc(), Dtor,
PDiag(diag::err_access_dtor) << PointeeElem);
diff --git a/clang/test/CXX/expr/expr.unary/expr.delete/p10.cpp
b/clang/test/CXX/expr/expr.unary/expr.delete/p10.cpp
index b2c0a2c79695fc..aff3e2b449da37 100644
--- a/clang/test/CXX/expr/expr.unary/expr.delete/p10.cpp
+++ b/clang/test/CXX/expr/expr.unary/expr.delete/p10.cpp
@@ -38,6 +38,16 @@ struct S {
void operator delete(S *, std::destroying_delete_t) noexcept {}
};
+struct T {
+ void operator delete(T *, std::destroying_delete_t) noexcept {}
+private:
+ ~T();
+};
+
void foo(S *s) {
delete s; // Was rejected, is intended to be accepted.
}
+
+void bar(T *t) {
+ delete t; // Was rejected, is intended to be accepted.
+}
>From b42fb1350f95924fae0b5dd7a46d7bc4d46d3d29 Mon Sep 17 00:00:00 2001
From: Aaron Ballman <aa...@aaronballman.com>
Date: Thu, 5 Dec 2024 11:58:33 -0500
Subject: [PATCH 3/5] Fix one of the two failing tests, update release notes
It turns out someone filed an issue for this.
---
clang/docs/ReleaseNotes.rst | 3 ++-
clang/test/SemaCXX/cxx2a-destroying-delete.cpp | 12 ++++++------
2 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 4a72e4046e2d03..e6373b165b63df 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -664,7 +664,8 @@ Bug Fixes in This Version
- Fixed a failed assertion when using ``__attribute__((noderef))`` on an
``_Atomic``-qualified type (#GH116124).
- No longer incorrectly diagnosing use of a deleted destructor when the
- selected overload of ``operator delete`` for that type is a destroying
delete.
+ selected overload of ``operator delete`` for that type is a destroying delete
+ (#GH46818).
Bug Fixes to Compiler Builtins
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/test/SemaCXX/cxx2a-destroying-delete.cpp
b/clang/test/SemaCXX/cxx2a-destroying-delete.cpp
index 25b985ef11d157..e986c6abfe8cec 100644
--- a/clang/test/SemaCXX/cxx2a-destroying-delete.cpp
+++ b/clang/test/SemaCXX/cxx2a-destroying-delete.cpp
@@ -146,17 +146,17 @@ namespace dtor_access {
struct S {
void operator delete(S *p, std::destroying_delete_t);
private:
- ~S(); // expected-note {{here}}
+ ~S();
};
- // FIXME: PR47474: GCC accepts this, and it seems somewhat reasonable to
- // allow, even though [expr.delete]p12 says this is ill-formed.
- void f() { delete new S; } // expected-error {{calling a private destructor}}
+ // C++20 [expr.delete]p12 says this is ill-formed, but GCC accepts and we
+ // filed CWG2889 to resolve in the same way.
+ void f() { delete new S; }
struct T {
void operator delete(T *, std::destroying_delete_t);
protected:
- virtual ~T(); // expected-note {{here}}
+ virtual ~T();
};
struct U : T {
@@ -165,7 +165,7 @@ namespace dtor_access {
~U() override;
};
- void g() { delete (T *)new U; } // expected-error {{calling a protected
destructor}}
+ void g() { delete (T *)new U; }
}
namespace delete_from_new {
>From 7e690aed3104c3b60e2f9c006c892ef56bc2e8cc Mon Sep 17 00:00:00 2001
From: Aaron Ballman <aa...@aaronballman.com>
Date: Thu, 5 Dec 2024 13:11:11 -0500
Subject: [PATCH 4/5] Fix the other failing test case
A virtual destructor still needs to be accessible to a destroying
delete because the actual delete operator called depends on the dynamic
type of the pointer, not the static type.
---
clang/lib/Sema/SemaExprCXX.cpp | 53 ++++++++++++-------
.../CXX/expr/expr.unary/expr.delete/p10.cpp | 23 ++++++--
.../test/SemaCXX/cxx2a-destroying-delete.cpp | 4 +-
3 files changed, 56 insertions(+), 24 deletions(-)
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index 6ac44ae7af28c3..d1268acd0ec0ab 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -3768,6 +3768,28 @@ Sema::ActOnCXXDelete(SourceLocation StartLoc, bool
UseGlobal,
DeclarationName DeleteName = Context.DeclarationNames.getCXXOperatorName(
ArrayForm ? OO_Array_Delete : OO_Delete);
+ // C++20 [expr.delete]p6: If the value of the operand of the delete-
+ // expression is not a null pointer value and the selected deallocation
+ // function (see below) is not a destroying operator delete, the delete-
+ // expression will invoke the destructor (if any) for the object or the
+ // elements of the array being deleted.
+ //
+ // This means we should not look at the destructor for a destroying
+ // delete operator, as that destructor is never called, unless the
+ // destructor is virtual (see [expr.delete]p8.1) because then the
+ // selected operator depends on the dynamic type of the pointer.
+ auto IsDtorCalled = [](const CXXMethodDecl *Dtor,
+ const FunctionDecl *OperatorDelete) {
+ if (!OperatorDelete)
+ return true;
+
+ if (!OperatorDelete->isDestroyingOperatorDelete())
+ return true;
+
+ // We have a destroying operator delete, so it depends on the dtor.
+ return Dtor->isVirtual();
+ };
+
if (PointeeRD) {
if (!UseGlobal &&
FindDeallocationFunction(StartLoc, PointeeRD, DeleteName,
@@ -3792,21 +3814,14 @@ Sema::ActOnCXXDelete(SourceLocation StartLoc, bool
UseGlobal,
.HasSizeT;
}
- // C++20 [expr.delete]p6: If the value of the operand of the delete-
- // expression is not a null pointer value and the selected deallocation
- // function (see below) is not a destroying operator delete, the delete-
- // expression will invoke the destructor (if any) for the object or the
- // elements of the array being deleted.
- //
- // This means we should not look at the destructor for a destroying
- // delete operator, as that destructor is never called.
- if (!PointeeRD->hasIrrelevantDestructor() &&
- (!OperatorDelete || !OperatorDelete->isDestroyingOperatorDelete())) {
+ if (!PointeeRD->hasIrrelevantDestructor()) {
if (CXXDestructorDecl *Dtor = LookupDestructor(PointeeRD)) {
- MarkFunctionReferenced(StartLoc,
- const_cast<CXXDestructorDecl *>(Dtor));
- if (DiagnoseUseOfDecl(Dtor, StartLoc))
- return ExprError();
+ if (IsDtorCalled(Dtor, OperatorDelete)) {
+ MarkFunctionReferenced(StartLoc,
+ const_cast<CXXDestructorDecl *>(Dtor));
+ if (DiagnoseUseOfDecl(Dtor, StartLoc))
+ return ExprError();
+ }
}
}
@@ -3839,13 +3854,13 @@ Sema::ActOnCXXDelete(SourceLocation StartLoc, bool
UseGlobal,
MarkFunctionReferenced(StartLoc, OperatorDelete);
// Check access and ambiguity of destructor if we're going to call it.
- // Note that this is required even for a virtual delete, but not for a
- // destroying delete operator.
+ // Note that this is required even for a virtual delete.
bool IsVirtualDelete = false;
- if (PointeeRD && !OperatorDelete->isDestroyingOperatorDelete()) {
+ if (PointeeRD) {
if (CXXDestructorDecl *Dtor = LookupDestructor(PointeeRD)) {
- CheckDestructorAccess(Ex.get()->getExprLoc(), Dtor,
- PDiag(diag::err_access_dtor) << PointeeElem);
+ if (IsDtorCalled(Dtor, OperatorDelete))
+ CheckDestructorAccess(Ex.get()->getExprLoc(), Dtor,
+ PDiag(diag::err_access_dtor) << PointeeElem);
IsVirtualDelete = Dtor->isVirtual();
}
}
diff --git a/clang/test/CXX/expr/expr.unary/expr.delete/p10.cpp
b/clang/test/CXX/expr/expr.unary/expr.delete/p10.cpp
index aff3e2b449da37..b8d64fd7eeabb6 100644
--- a/clang/test/CXX/expr/expr.unary/expr.delete/p10.cpp
+++ b/clang/test/CXX/expr/expr.unary/expr.delete/p10.cpp
@@ -44,10 +44,27 @@ struct T {
~T();
};
-void foo(S *s) {
+void foo(S *s, T *t) {
delete s; // Was rejected, is intended to be accepted.
+ delete t; // Was rejected, is intended to be accepted.
}
-void bar(T *t) {
- delete t; // Was rejected, is intended to be accepted.
+// However, if the destructor is virtual, then it has to be accessible because
+// the behavior depends on which operator delete is selected and that is based
+// on the dynamic type of the pointer.
+struct U {
+ virtual ~U() = delete; // expected-note {{here}}
+ void operator delete(U *, std::destroying_delete_t) noexcept {}
+};
+
+struct V {
+ void operator delete(V *, std::destroying_delete_t) noexcept {}
+private:
+ virtual ~V(); // expected-note {{here}}
+};
+
+void bar(U *u, V *v) {
+ // Both should be rejected because they have virtual destructors.
+ delete u; // expected-error {{attempt to use a deleted function}}
+ delete v; // expected-error {{calling a private destructor of class 'V'}}
}
diff --git a/clang/test/SemaCXX/cxx2a-destroying-delete.cpp
b/clang/test/SemaCXX/cxx2a-destroying-delete.cpp
index e986c6abfe8cec..bf0a64a77385c6 100644
--- a/clang/test/SemaCXX/cxx2a-destroying-delete.cpp
+++ b/clang/test/SemaCXX/cxx2a-destroying-delete.cpp
@@ -156,7 +156,7 @@ namespace dtor_access {
struct T {
void operator delete(T *, std::destroying_delete_t);
protected:
- virtual ~T();
+ virtual ~T(); // expected-note {{here}}
};
struct U : T {
@@ -165,7 +165,7 @@ namespace dtor_access {
~U() override;
};
- void g() { delete (T *)new U; }
+ void g() { delete (T *)new U; } // expected-error {{calling a protected
destructor of class 'T'}}
}
namespace delete_from_new {
>From f351d4edbf55fd14e1496fe48992e8c50f23204c Mon Sep 17 00:00:00 2001
From: Aaron Ballman <aa...@aaronballman.com>
Date: Fri, 6 Dec 2024 09:36:23 -0500
Subject: [PATCH 5/5] Update noexcept behavior based on review feedback
---
clang/lib/Sema/SemaExceptionSpec.cpp | 30 ++++++++++++++-----
.../SemaCXX/noexcept-destroying-delete.cpp | 17 ++++++++++-
2 files changed, 38 insertions(+), 9 deletions(-)
diff --git a/clang/lib/Sema/SemaExceptionSpec.cpp
b/clang/lib/Sema/SemaExceptionSpec.cpp
index 6a9f43d6f5215e..e38aa48408d8a4 100644
--- a/clang/lib/Sema/SemaExceptionSpec.cpp
+++ b/clang/lib/Sema/SemaExceptionSpec.cpp
@@ -1200,21 +1200,35 @@ CanThrowResult Sema::canThrow(const Stmt *S) {
case Expr::CXXDeleteExprClass: {
auto *DE = cast<CXXDeleteExpr>(S);
- CanThrowResult CT;
+ CanThrowResult CT = CT_Cannot;
QualType DTy = DE->getDestroyedType();
if (DTy.isNull() || DTy->isDependentType()) {
CT = CT_Dependent;
} else {
+ // C++20 [expr.delete]p6: If the value of the operand of the delete-
+ // expression is not a null pointer value and the selected deallocation
+ // function (see below) is not a destroying operator delete, the delete-
+ // expression will invoke the destructor (if any) for the object or the
+ // elements of the array being deleted.
+ //
+ // So if the destructor is virtual, we only look at the exception
+ // specification of the destructor regardless of what operator delete is
+ // selected. Otherwise, we look at the selected operator delete, and if
+ // it is not a destroying delete, then we look at the destructor.
const FunctionDecl *OperatorDelete = DE->getOperatorDelete();
- CT = canCalleeThrow(*this, DE, OperatorDelete);
- if (!OperatorDelete->isDestroyingOperatorDelete()) {
- if (const auto *RD = DTy->getAsCXXRecordDecl()) {
- if (const CXXDestructorDecl *DD = RD->getDestructor())
- CT = mergeCanThrow(CT, canCalleeThrow(*this, DE, DD));
+ if (const auto *RD = DTy->getAsCXXRecordDecl()) {
+ if (const CXXDestructorDecl *DD = RD->getDestructor()) {
+ if (DD->isVirtual() || !OperatorDelete->isDestroyingOperatorDelete())
+ CT = canCalleeThrow(*this, DE, DD);
}
- if (CT == CT_Can)
- return CT;
}
+
+ // We always look at the exception specification of the operator delete.
+ CT = mergeCanThrow(CT, canCalleeThrow(*this, DE, OperatorDelete));
+
+ // If we know we can throw, we're done.
+ if (CT == CT_Can)
+ return CT;
}
return mergeCanThrow(CT, canSubStmtsThrow(*this, DE));
}
diff --git a/clang/test/SemaCXX/noexcept-destroying-delete.cpp
b/clang/test/SemaCXX/noexcept-destroying-delete.cpp
index 92ccbc1fb3f96b..6a745314b62045 100644
--- a/clang/test/SemaCXX/noexcept-destroying-delete.cpp
+++ b/clang/test/SemaCXX/noexcept-destroying-delete.cpp
@@ -1,5 +1,4 @@
// RUN: %clang_cc1 -fsyntax-only -verify -fcxx-exceptions
-Wno-unevaluated-expression -std=c++20 %s
-// expected-no-diagnostics
namespace std {
struct destroying_delete_t {
@@ -31,3 +30,19 @@ ThrowingDestroyingDelete *pn = nullptr;
// noexcept should return false here because the destroying delete itself is a
// potentially throwing function.
static_assert(!noexcept(delete(pn)));
+
+
+struct A {
+ virtual ~A(); // implicitly noexcept
+};
+struct B : A {
+ void operator delete(B *p, std::destroying_delete_t) { throw "oh no"; } //
expected-warning {{'operator delete' has a non-throwing exception specification
but can still throw}} \
+
expected-note {{deallocator has a implicit non-throwing exception
specification}}
+};
+A *p = new B;
+
+// Because the destructor for A is virtual, it is the only thing we consider
+// when determining whether the delete expression can throw or not, despite the
+// fact that the selected operator delete is a destroying delete. For virtual
+// destructors, it's the dynamic type that matters.
+static_assert(noexcept(delete p));
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
