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(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(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(Dtor)); - if (DiagnoseUseOfDecl(Dtor, StartLoc)) - return ExprError(); + if (Dtor->isCalledByDelete(OperatorDelete)) { + MarkFunctionReferenced(StartLoc, + const_cast(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));