[clang] 5ff7f47 - [C++20] Destroying delete and deleted destructors (#118800)
via cfe-commits
cfe-commits at lists.llvm.org
Thu Jan 9 05:29:23 PST 2025
Author: Aaron Ballman
Date: 2025-01-09T08:29:19-05:00
New Revision: 5ff7f479a1f30d9c5393e4f94df6178a63cc2236
URL: https://github.com/llvm/llvm-project/commit/5ff7f479a1f30d9c5393e4f94df6178a63cc2236
DIFF: https://github.com/llvm/llvm-project/commit/5ff7f479a1f30d9c5393e4f94df6178a63cc2236.diff
LOG: [C++20] Destroying delete and deleted destructors (#118800)
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.
Fixes #46818
Added:
Modified:
clang/docs/ReleaseNotes.rst
clang/include/clang/AST/DeclCXX.h
clang/lib/AST/DeclCXX.cpp
clang/lib/Sema/SemaExceptionSpec.cpp
clang/lib/Sema/SemaExprCXX.cpp
clang/test/CXX/expr/expr.unary/expr.delete/p10.cpp
clang/test/SemaCXX/cxx2a-destroying-delete.cpp
clang/test/SemaCXX/noexcept-destroying-delete.cpp
Removed:
################################################################################
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 0ea39ccfc62670..e7e4e37282e527 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -767,6 +767,9 @@ Bug Fixes in This Version
- Fixed a crash when passing the variable length array type to ``va_arg`` (#GH119360).
- 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
+ (#GH46818).
- No longer return ``false`` for ``noexcept`` expressions involving a
``delete`` which resolves to a destroying delete but the type of the object
being deleted has a potentially throwing destructor (#GH118660).
diff --git a/clang/include/clang/AST/DeclCXX.h b/clang/include/clang/AST/DeclCXX.h
index c232556edeff70..fa3f4ec98eb369 100644
--- a/clang/include/clang/AST/DeclCXX.h
+++ b/clang/include/clang/AST/DeclCXX.h
@@ -2855,6 +2855,11 @@ class CXXDestructorDecl : public CXXMethodDecl {
return getCanonicalDecl()->OperatorDeleteThisArg;
}
+ /// Will this destructor ever be called when considering which deallocation
+ /// function is associated with the destructor? Can optionally be passed an
+ /// 'operator delete' function declaration to test against specifically.
+ bool isCalledByDelete(const FunctionDecl *OpDel = nullptr) const;
+
CXXDestructorDecl *getCanonicalDecl() override {
return cast<CXXDestructorDecl>(FunctionDecl::getCanonicalDecl());
}
diff --git a/clang/lib/AST/DeclCXX.cpp b/clang/lib/AST/DeclCXX.cpp
index af73c658d6a0c5..8989e46e4847cb 100644
--- a/clang/lib/AST/DeclCXX.cpp
+++ b/clang/lib/AST/DeclCXX.cpp
@@ -2969,6 +2969,28 @@ void CXXDestructorDecl::setOperatorDelete(FunctionDecl *OD, Expr *ThisArg) {
}
}
+bool CXXDestructorDecl::isCalledByDelete(const FunctionDecl *OpDel) const {
+ // 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.
+ const FunctionDecl *SelectedOperatorDelete = OpDel ? OpDel : OperatorDelete;
+ if (!SelectedOperatorDelete)
+ return true;
+
+ if (!SelectedOperatorDelete->isDestroyingOperatorDelete())
+ return true;
+
+ // We have a destroying operator delete, so it depends on the dtor.
+ return isVirtual();
+}
+
void CXXConversionDecl::anchor() {}
CXXConversionDecl *CXXConversionDecl::CreateDeserialized(ASTContext &C,
diff --git a/clang/lib/Sema/SemaExceptionSpec.cpp b/clang/lib/Sema/SemaExceptionSpec.cpp
index ac5d51a1d2ff6e..254ad05c5ba74a 100644
--- a/clang/lib/Sema/SemaExceptionSpec.cpp
+++ b/clang/lib/Sema/SemaExceptionSpec.cpp
@@ -1200,21 +1200,29 @@ 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.
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 (CT == CT_Can)
- return CT;
+ if (const auto *RD = DTy->getAsCXXRecordDecl()) {
+ if (const CXXDestructorDecl *DD = RD->getDestructor();
+ DD && DD->isCalledByDelete(OperatorDelete))
+ CT = canCalleeThrow(*this, DE, DD);
}
+
+ // 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/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index e04ece0bda82eb..1e39d69e8b230f 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -3792,13 +3792,16 @@ Sema::ActOnCXXDelete(SourceLocation StartLoc, bool UseGlobal,
.HasSizeT;
}
- if (!PointeeRD->hasIrrelevantDestructor())
+ if (!PointeeRD->hasIrrelevantDestructor()) {
if (CXXDestructorDecl *Dtor = LookupDestructor(PointeeRD)) {
- MarkFunctionReferenced(StartLoc,
- const_cast<CXXDestructorDecl*>(Dtor));
- if (DiagnoseUseOfDecl(Dtor, StartLoc))
- return ExprError();
+ if (Dtor->isCalledByDelete(OperatorDelete)) {
+ MarkFunctionReferenced(StartLoc,
+ const_cast<CXXDestructorDecl *>(Dtor));
+ if (DiagnoseUseOfDecl(Dtor, StartLoc))
+ return ExprError();
+ }
}
+ }
CheckVirtualDtorCall(PointeeRD->getDestructor(), StartLoc,
/*IsDelete=*/true, /*CallCanBeVirtual=*/true,
@@ -3833,8 +3836,9 @@ Sema::ActOnCXXDelete(SourceLocation StartLoc, bool UseGlobal,
bool IsVirtualDelete = false;
if (PointeeRD) {
if (CXXDestructorDecl *Dtor = LookupDestructor(PointeeRD)) {
- CheckDestructorAccess(Ex.get()->getExprLoc(), Dtor,
- PDiag(diag::err_access_dtor) << PointeeElem);
+ if (Dtor->isCalledByDelete(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 aad2747dd32f24..b8d64fd7eeabb6 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,41 @@ 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 {}
+};
+
+struct T {
+ void operator delete(T *, std::destroying_delete_t) noexcept {}
+private:
+ ~T();
+};
+
+void foo(S *s, T *t) {
+ delete s; // Was rejected, is intended to be accepted.
+ 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 25b985ef11d157..bf0a64a77385c6 100644
--- a/clang/test/SemaCXX/cxx2a-destroying-delete.cpp
+++ b/clang/test/SemaCXX/cxx2a-destroying-delete.cpp
@@ -146,12 +146,12 @@ 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);
@@ -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; } // expected-error {{calling a protected destructor of class 'T'}}
}
namespace delete_from_new {
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));
More information about the cfe-commits
mailing list