[Clang] Deleting an incomplete enum type is not an error

[cor3ntin]

The changes introduced in #97733 accidentally prevented to delete an incomplete enum (the validity of which has been confirmed by CWG2925

Fixes #99278

[llvmbot]

@llvm/pr-subscribers-clang

Author: cor3ntin (cor3ntin)

Changes

The changes introduced in #97733 accidentally prevented to delete an incomplete enum (the validity of which has been confirmed by CWG2925

Fixes #99278

---

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

3 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+1)
• (modified) clang/lib/Sema/SemaExprCXX.cpp (+2-1)
• (modified) clang/test/SemaCXX/new-delete.cpp (+8)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 01c7899e36c932..e442516c225f74 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -766,6 +766,7 @@ Bug Fixes to C++ Support
 - Fixed an assertion failure caused by mangled names with invalid identifiers. (#GH112205)
 - Fixed an incorrect lambda scope of generic lambdas that caused Clang to crash when computing potential lambda
   captures at the end of a full expression. (#GH115931)
+- Clang no longer rejects deleting a pointer of incomplete enumeration type. (#GH99278)
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index d85819b21c8265..20947ca4e35b17 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -3748,7 +3748,8 @@ Sema::ActOnCXXDelete(SourceLocation StartLoc, bool UseGlobal,
       // FIXME: This can result in errors if the definition was imported from a
       // module but is hidden.
       if (!RequireCompleteType(StartLoc, Pointee,
-                               LangOpts.CPlusPlus26
+                               Pointee->isStructureOrClassType() &&
+                                       LangOpts.CPlusPlus26
                                    ? diag::err_delete_incomplete
                                    : diag::warn_delete_incomplete,
                                Ex.get())) {
diff --git a/clang/test/SemaCXX/new-delete.cpp b/clang/test/SemaCXX/new-delete.cpp
index 595bdc689d694b..18b26e7f0f08a1 100644
--- a/clang/test/SemaCXX/new-delete.cpp
+++ b/clang/test/SemaCXX/new-delete.cpp
@@ -540,6 +540,14 @@ namespace PR10504 {
   void f(A *x) { delete x; } // expected-warning {{delete called on 'PR10504::A' that is abstract but has non-virtual destructor}}
 }
 
+#if __cplusplus >= 201103L
+enum GH99278_1 { // expected-note {{definition of 'GH99278_1' is not complete until the closing '}'}}
+    zero = decltype(delete static_cast<GH99278_1*>(nullptr), 0){}
+    // expected-warning@-1 {{deleting pointer to incomplete type}}
+    // expected-warning@-2 {{expression with side effects has no effect in an unevaluated context}}
+};
+#endif
+
 struct PlacementArg {};
 inline void *operator new[](size_t, const PlacementArg &) throw () {
   return 0;

[erichkeane]

Would prefer this mentions the CWG issue perhaps? Or at least a touch of backstory?

[cor3ntin]

The CWG issue is NAD - the back story is just "regression"

[AaronBallman]

LGTM!

[alexfh]

Heads up: we're seeing clang crashes after this revision. Assertions-enabled clang fails this assertion (probably related?):

assert.h assertion failed at clang/lib/Sema/SemaLookup.cpp:2408 in bool clang::Sema::LookupQualifiedName(LookupResult &, DeclContext *, bool): (!isa<TagDecl>(LookupCtx) || LookupCtx->isDependentContext() || cast<TagDecl>(LookupCtx)->isCompleteDefinition() || cast<TagDecl>(LookupCtx)->isBeingDefined()) && "Declaration context must already be complete!"
    @     0x564ff412fec4  __assert_fail
    @     0x564fefcf58aa  clang::Sema::LookupQualifiedName()
    @     0x564fefbf848c  clang::Sema::FindDeallocationFunction()
    @     0x564fefbf9a28  clang::Sema::ActOnCXXDelete()
    @     0x564feffded5a  clang::TreeTransform<>::TransformStmt()
    @     0x564ff00299f4  clang::TreeTransform<>::TransformCompoundStmt()
    @     0x564ff0044578  clang::TreeTransform<>::TransformIfStmt()
    @     0x564ff00299f4  clang::TreeTransform<>::TransformCompoundStmt()
    @     0x564feffdece8  clang::Sema::SubstStmt()
    @     0x564ff006bef1  clang::Sema::InstantiateFunctionDefinition()
    @     0x564ff006ef7f  clang::Sema::PerformPendingInstantiations()
    @     0x564ff006bfdc  clang::Sema::InstantiateFunctionDefinition()
    @     0x564ff006ef7f  clang::Sema::PerformPendingInstantiations()
    @     0x564ff006bfdc  clang::Sema::InstantiateFunctionDefinition()
    @     0x564ff006ef7f  clang::Sema::PerformPendingInstantiations()
    @     0x564fef86faa7  clang::Sema::ActOnEndOfTranslationUnitFragment()
    @     0x564fef8701e9  clang::Sema::ActOnEndOfTranslationUnit()
    @     0x564fef5a619a  clang::Parser::ParseTopLevelDecl()
    @     0x564fef5a25ee  clang::ParseAST()
    @     0x564fef316d43  clang::FrontendAction::Execute()
    @     0x564fef29b50d  clang::CompilerInstance::ExecuteAction()                                                                                                                                                                                       
    @     0x564fee4490a8  clang::ExecuteCompilerInvocation()
    @     0x564fee43dbbf  cc1_main()
    @     0x564fee43b1d3  ExecuteCC1Tool()
    @     0x564fef42913e  llvm::function_ref<>::callback_fn<>()
    @     0x564ff3db145c  llvm::CrashRecoveryContext::RunSafely()
    @     0x564fef4287a4  clang::driver::CC1Command::Execute()
    @     0x564fef3eee26  clang::driver::Compilation::ExecuteCommand()
    @     0x564fef3ef08f  clang::driver::Compilation::ExecuteJobs()
    @     0x564fef407e00  clang::driver::Driver::ExecuteCompilation()
    @     0x564fee43a89e  clang_main()
    @     0x564fee439074  main
    @     0x7fc63a9bc3d4  __libc_start_main
    @     0x564fee438faa  _start

[alexfh]

The original file is huge and template heavy, so reduction will take some time. I wonder if you can spot something obviously wrong in the commit given the information above? Failing that, can we revert in the meantime?

[glandium]

We're getting a similar crash bisected to this change as well, also with a huge original file. Funnily enough, a debug build hits an assertion that happens even without this change, so I'm going to dig into that before even trying to reduce the crasher.

[alexfh]

In my case I don't see an assertion before this change (using Clang built from c0192a0).

[cor3ntin]

PR here #118980 - But I'd love to understand the actual issue before merging

[alexfh]

PR here #118980 - But I'd love to understand the actual issue before merging

It'll take some time. cvise has reduced the preprocessed input from 13MB to 7.5MB so far. The input is not shareable so far.

[glandium]

cvise has reduced my testcase to

union StyleColorFunctionStyleColorFunction *mRaw {
  delete mRaw

... which is nowhere close to the real thing and nowhere close to legal C++ at all.

Well, that definitely shouldn't trigger a crash, though.

[alexfh]

To avoid this I usually incorporate a validity check in my interestingness test, e.g. by running a known-good clang on the candidate input, e.g. currently I'm running cvise with this:

$ cat bad-clang-crashes.sh
#!/bin/bash -eux
ARGS="-fdiagnostics-show-option -Wall -Werror -Wno-inconsistent-missing-override -Wno-unused -Wno-unused-lambda-capture -Wno-deprecated -Wno-mismatched-tags -Wno-gnu-alignof-expression -Wno-deprecated-this-capture -std=gnu++20 -Wno-defaulted-function-deleted -fsyntax-only -Wno-user-defined-literals -Wno-nontrivial-memaccess -Wno-invalid-constexpr -Wno-misleading-indentation -Wno-dynamic-class-memaccess -Wno-ignored-attributes"

$(dirname $0)/clang-good $ARGS q.cc &
pid=$!
$(dirname $0)/clang-bad-checked $ARGS q.cc 2>&1 | grep "Declaration context must already be complete"
wait $pid

[alexfh]

The test case was reduced to this:

template <typename = void> struct a {
  union b {};
  struct c {
    c() { delete d; }
    b *d;
  } f;
};
a e;

https://gcc.godbolt.org/z/d5cP4McG3