Skip to content

[C++20] Destroying delete and deleted destructors #118800

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

Merged
merged 11 commits into from
Jan 9, 2025

Conversation

AaronBallman
Copy link
Collaborator

@AaronBallman AaronBallman commented Dec 5, 2024

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

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.
@AaronBallman AaronBallman added clang Clang issues not falling into any other category c++20 clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Dec 5, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 5, 2024

@llvm/pr-subscribers-clang

Author: Aaron Ballman (AaronBallman)

Changes

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.


Full diff: https://github.com/llvm/llvm-project/pull/118800.diff

3 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+2)
  • (modified) clang/lib/Sema/SemaExprCXX.cpp (+4-2)
  • (modified) clang/test/CXX/expr/expr.unary/expr.delete/p10.cpp (+20-2)
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.
+}

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 nit, else LGTM.

* Added a standards reference
* Added a test case
* Fixed an issue the new test case identified
It turns out someone filed an issue for this.
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.
@zygoloid
Copy link
Collaborator

zygoloid commented Dec 5, 2024

As I just noted in #118687, I think we also need similar treatment for noexcept(delete p).

@AaronBallman AaronBallman requested a review from zygoloid December 9, 2024 13:45
@AaronBallman
Copy link
Collaborator Author

FYI, I'm going to be out on vacation after today, so if this is ready to land, someone else can feel free to press the button. Otherwise, I can pick this up again when I'm back.

@AaronBallman
Copy link
Collaborator Author

Ping @zygoloid

Copy link

github-actions bot commented Jan 8, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Collaborator

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG other than the function name.

@AaronBallman AaronBallman merged commit 5ff7f47 into llvm:main Jan 9, 2025
4 of 5 checks passed
@AaronBallman AaronBallman deleted the aballman-gh118660-second-bug branch January 9, 2025 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++20 clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Destroying delete requires accessible destructor
5 participants