Skip to content

One more fix for P3144R2 implementation #149406

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.

Sign up for GitHub

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

One more fix for P3144R2 implementation #149406

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
94e652b
More fixes for P3144R2 implementation
hvdijk Jul 17, 2025
a2123f9
Update PR.
hvdijk Jul 18, 2025
69a65b2
Also tweak wording of deleting pointers to void.
hvdijk Jul 18, 2025
2f751bc
Revert diagnostics changes.
hvdijk Jul 18, 2025
6c21226
Add delete pointer to array of class test
hvdijk Jul 22, 2025
eff9ccb
Change array deletion tests to use delete [].
hvdijk Jul 23, 2025
268265f
One more.
hvdijk Jul 23, 2025
d9449b2
Merge remote-tracking branch 'origin/main' into pr3144r2-fixes
hvdijk Jul 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -977,6 +977,7 @@ Bug Fixes to C++ Support
non-empty initializer list. (#GH147949)
- Fixed constant evaluation of equality comparisons of constexpr-unknown references. (#GH147663)
- Diagnose binding a reference to ``*nullptr`` during constant evaluation. (#GH48665)
- Clang no longer rejects deleting a pointer of incomplete array type. (#GH149406)

Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
Expand Down
8 changes: 5 additions & 3 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8392,17 +8392,19 @@ def ext_default_init_const : ExtWarn<
"is a Microsoft extension">,
InGroup<MicrosoftConstInit>;
def err_delete_operand : Error<"cannot delete expression of type %0">;
def err_delete_void_ptr_operand : Error<
"cannot delete pointer to %0">;
def ext_delete_void_ptr_operand : ExtWarn<
"cannot delete expression with pointer-to-'void' type %0">,
"cannot delete pointer to %0">,
InGroup<DeleteIncomplete>;
def err_ambiguous_delete_operand : Error<
"ambiguous conversion of delete expression of type %0 to a pointer">;
def warn_delete_incomplete : Warning<
"deleting pointer to incomplete type %0 is incompatible with C++2c"
"deleting pointer to incomplete %select{struct|interface|union|class|enum}0 %1 is incompatible with C++2c"
" and may cause undefined behavior">,
InGroup<DeleteIncomplete>;
def err_delete_incomplete : Error<
"cannot delete pointer to incomplete type %0">;
"cannot delete pointer to incomplete %select{struct|interface|union|class|enum}0 %1">;
def err_delete_incomplete_class_type : Error<
"deleting incomplete class type %0; no conversions to pointer type">;
def err_delete_explicit_conversion : Error<
Expand Down
1 change: 1 addition & 0 deletions clang/include/clang/Sema/Sema.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14855,6 +14855,7 @@ class Sema final : public SemaBase {
}
static DeclarationName getPrintable(DeclarationName N) { return N; }
static QualType getPrintable(QualType T) { return T; }
static TagTypeKind getPrintable(TagTypeKind T) { return T; }
static SourceRange getPrintable(SourceRange R) { return R; }
static SourceRange getPrintable(SourceLocation L) { return L; }
static SourceRange getPrintable(const Expr *E) { return E->getSourceRange(); }
Expand Down
24 changes: 12 additions & 12 deletions clang/lib/Sema/SemaExprCXX.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4033,25 +4033,25 @@ Sema::ActOnCXXDelete(SourceLocation StartLoc, bool UseGlobal,
// effectively bans deletion of "void*". However, most compilers support
// this, so we treat it as a warning unless we're in a SFINAE context.
// But we still prohibit this since C++26.
Diag(StartLoc, LangOpts.CPlusPlus26 ? diag::err_delete_incomplete
Diag(StartLoc, LangOpts.CPlusPlus26 ? diag::err_delete_void_ptr_operand
: diag::ext_delete_void_ptr_operand)
<< (LangOpts.CPlusPlus26 ? Pointee : Type)
<< Ex.get()->getSourceRange();
<< Pointee << Ex.get()->getSourceRange();
} else if (Pointee->isFunctionType() || Pointee->isVoidType() ||
Pointee->isSizelessType()) {
return ExprError(Diag(StartLoc, diag::err_delete_operand)
<< Type << Ex.get()->getSourceRange());
<< Type << Ex.get()->getSourceRange());
} else if (!Pointee->isDependentType()) {
// FIXME: This can result in errors if the definition was imported from a
// module but is hidden.
if (Pointee->isEnumeralType() ||
!RequireCompleteType(StartLoc, Pointee,
LangOpts.CPlusPlus26
? diag::err_delete_incomplete
: diag::warn_delete_incomplete,
Ex.get())) {
if (const RecordType *RT = PointeeElem->getAs<RecordType>())
PointeeRD = cast<CXXRecordDecl>(RT->getDecl());
if (const RecordType *RT = PointeeElem->getAs<RecordType>()) {
PointeeRD = cast<CXXRecordDecl>(RT->getDecl());
if (RequireCompleteType(StartLoc, Pointee,
LangOpts.CPlusPlus26
? diag::err_delete_incomplete
: diag::warn_delete_incomplete,
PointeeRD->getTagKind(), Ex.get())) {
PointeeRD = nullptr;
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion clang/test/Analysis/dtor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -499,7 +499,7 @@ namespace PseudoDtor {

namespace Incomplete {
class Foo; // expected-note{{forward declaration}}
void f(Foo *foo) { delete foo; } // expected-warning{{deleting pointer to incomplete type}}
void f(Foo *foo) { delete foo; } // expected-warning{{deleting pointer to incomplete class 'Foo'}}
}

namespace TypeTraitExpr {
Expand Down
12 changes: 6 additions & 6 deletions clang/test/CXX/drs/cwg5xx.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -906,8 +906,8 @@ namespace cwg573 { // cwg573: no
void *d = reinterpret_cast<void*>(c);
// cxx98-error@-1 {{cast between pointer-to-function and pointer-to-object is an extension}}
void f() { delete a; }
// cxx98-23-error@-1 {{cannot delete expression with pointer-to-'void' type 'void *'}}
// since-cxx26-error@-2 {{cannot delete pointer to incomplete type 'void'}}
// cxx98-23-error@-1 {{cannot delete pointer to 'void'}}
// since-cxx26-error@-2 {{cannot delete pointer to 'void'}}
int n = d - a;
// expected-error@-1 {{arithmetic on pointers to void}}
// FIXME: This is ill-formed.
Expand Down Expand Up @@ -1297,13 +1297,13 @@ namespace cwg599 { // cwg599: partial
struct V { operator int*(); operator Fn*(); };
void f(void *p, void (*q)(), S s, T t, U u, V v) {
delete p;
// cxx98-23-error@-1 {{cannot delete expression with pointer-to-'void' type 'void *'}}
// since-cxx26-error@-2 {{cannot delete pointer to incomplete type 'void'}}
// cxx98-23-error@-1 {{cannot delete pointer to 'void'}}
// since-cxx26-error@-2 {{cannot delete pointer to 'void'}}
delete q;
// expected-error@-1 {{cannot delete expression of type 'void (*)()'}}
delete s;
// cxx98-23-error@-1 {{cannot delete expression with pointer-to-'void' type 'void *'}}
// since-cxx26-error@-2 {{cannot delete pointer to incomplete type 'void'}}
// cxx98-23-error@-1 {{cannot delete pointer to 'void'}}
// since-cxx26-error@-2 {{cannot delete pointer to 'void'}}
delete t;
// expected-error@-1 {{cannot delete expression of type 'T'}}
// FIXME: This is valid, but is rejected due to a non-conforming GNU
Expand Down
9 changes: 7 additions & 2 deletions clang/test/CXX/expr/expr.unary/expr.delete/p5.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,12 @@

// The trivial case.
class T0; // expected-note {{forward declaration}}
void f0(T0 *a) { delete a; } // expected-warning {{deleting pointer to incomplete type}}
void f0(T0 *a) { delete a; } // expected-warning {{deleting pointer to incomplete class 'T0'}}
class T0 { ~T0(); };

// The trivial case, inside a template instantiation.
template<typename T>
struct T1_A { T *x; ~T1_A() { delete x; } }; // expected-warning {{deleting pointer to incomplete type}}
struct T1_A { T *x; ~T1_A() { delete x; } }; // expected-warning {{deleting pointer to incomplete class 'T1_B'}}
class T1_B; // expected-note {{forward declaration}}
void f0() { T1_A<T1_B> x; } // expected-note {{in instantiation of member function}}

Expand Down Expand Up @@ -44,3 +44,8 @@ class T3_A {
private:
~T3_A(); // expected-note{{declared private here}}
};

// The trivial case but with a union.
union T4; // expected-note {{forward declaration}}
void f4(T4 *a) { delete a; } // expected-warning {{deleting pointer to incomplete union 'T4'}}
union T4 { ~T4(); };
2 changes: 1 addition & 1 deletion clang/test/OpenMP/deferred-diags.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ namespace TestDeleteIncompleteClassDefinition {
struct a;
struct b {
b() {
delete c; // expected-warning {{deleting pointer to incomplete type 'a' is incompatible with C++2c and may cause undefined behavior}}
delete c; // expected-warning {{deleting pointer to incomplete struct 'a' is incompatible with C++2c and may cause undefined behavior}}
}
a *c;
};
Expand Down
16 changes: 10 additions & 6 deletions clang/test/SemaCXX/new-delete.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,11 +224,11 @@ void bad_deletes()
delete 0; // expected-error {{cannot delete expression of type 'int'}}
delete [0] (int*)0; // expected-error {{expected variable name or 'this' in lambda capture list}}
delete (void*)0;
// cxx98-23-warning@-1 {{cannot delete expression with pointer-to-'void' type 'void *'}}
// since-cxx26-error@-2 {{cannot delete pointer to incomplete type 'void'}}
// cxx98-23-warning@-1 {{cannot delete pointer to 'void'}}
// since-cxx26-error@-2 {{cannot delete pointer to 'void'}}
delete (T*)0;
// cxx98-23-warning@-1 {{deleting pointer to incomplete type}}
// since-cxx26-error@-2 {{cannot delete pointer to incomplete type 'T'}}
// cxx98-23-warning@-1 {{deleting pointer to incomplete struct 'T'}}
// since-cxx26-error@-2 {{cannot delete pointer to incomplete struct 'T'}}
::S::delete (int*)0; // expected-error {{expected unqualified-id}}
}

Expand Down Expand Up @@ -570,8 +570,8 @@ namespace DeleteIncompleteClassPointerError {
struct A; // expected-note {{forward declaration}}
void f(A *x) { 1+delete x; }
// expected-error@-1 {{invalid operands to binary expression}}
// cxx98-23-warning@-2 {{deleting pointer to incomplete type}}
// since-cxx26-error@-3 {{cannot delete pointer to incomplete type 'A'}}
// cxx98-23-warning@-2 {{deleting pointer to incomplete struct 'A'}}
// since-cxx26-error@-3 {{cannot delete pointer to incomplete struct 'A'}}
}

namespace PR10504 {
Expand All @@ -595,6 +595,10 @@ struct GH99278_2 {
} f;
};
GH99278_2<void> e;
void GH99278_3(int(*p)[]) {
delete p;
// expected-warning@-1 {{'delete' applied to a pointer-to-array type 'int (*)[]' treated as 'delete[]'}}
};
Copy link
Collaborator

@efriedma-quic efriedma-quic Jul 21, 2025

Choose a reason for hiding this comment

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

Maybe also test deleting an incomplete array of a complete class type?

Copy link
Contributor Author

@hvdijk hvdijk Jul 21, 2025

Choose a reason for hiding this comment

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

Hmm, that made me think, what about a pointer to an array of incomplete class type? If that's done through delete[], it would be ill-formed. If that's done through delete, the compiler tells the user that it's treated as delete[], yet it would not be ill-formed. That does not make sense and I am not sure what's the right thing to do there is.

Comparing to what GCC does, GCC doesn't accept this example (even with int(*)[]).

The example does not appear to violate any rule (at least when the pointer is a null pointer) and clearly was not intended to be banned by P3144R2, yet at the same time I am getting the feeling it never should have been allowed.

Copy link
Contributor Author

@hvdijk hvdijk Jul 22, 2025

Choose a reason for hiding this comment

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

What EDG does (thanks Compiler Explorer for having that available) is allow it for arrays of complete class type, and disallow it for arrays of incomplete class type. That seems like a sensible way of going about it, it effectively interprets "If the object being deleted has incomplete class type at the point of deletion, the program is ill-formed" as applying also to subobjects of the object being deleted. Which is not what the standard says, but necessary for what P3144R2 aimed to accomplish, so I'll see if I can implement that.

Copy link
Contributor Author

@hvdijk hvdijk Jul 22, 2025

Choose a reason for hiding this comment

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

That turned out to be a trivial one-line change, so I've done it and added tests for it.

Copy link
Collaborator

@efriedma-quic efriedma-quic Jul 22, 2025

Choose a reason for hiding this comment

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

The general issue here is that there's a hole in the standard: a non-array new-expression can't return a pointer to an array, so [expr.delete]p2 effectively says it's always undefined behavior to delete a pointer to an array (unless it's null). Existing compilers treat this as if you wrote delete[]. (clang and EDG warn.) This is a problem whether or not the array is incomplete.

Probably we should ask the committee to address this.

That said, given the current state of things, this patch seems fine.

On a sort of related note, the following currently crashes in codegen:

struct A { ~A(); };
void f(A (*a)[]){ delete[] a; }

Copy link
Contributor Author

@hvdijk hvdijk Jul 22, 2025

Choose a reason for hiding this comment

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

The general issue here is that there's a hole in the standard: a non-array new-expression can't return a pointer to an array, so [expr.delete]p2 effectively says it's always undefined behavior to delete a pointer to an array (unless it's null).

True for the tests that I added with delete. For your example with delete[], consider:

struct S { ~S(); };
void del(S(*p)[]) { delete[] p; }
void test() { del(new S[4][4]); }

The checks that I implemented in this PR handle this, they allow it in that test, but reject it if S is incomplete which is necessary to avoid UB.

The codegen crash is worrying, the test I'm showing here shows that this can occur in legitimate code. I'll have a look at that when I have some extra time.

Copy link
Contributor Author

@hvdijk hvdijk Jul 23, 2025

Choose a reason for hiding this comment

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

I've changed the array tests to use delete [] so that they test something useful, so that they test something that could be valid even for non-null pointers.

Copy link
Contributor Author

@hvdijk hvdijk Jul 24, 2025

Choose a reason for hiding this comment

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

For the codegen crash, I have opened #150359

#endif

struct PlacementArg {};
Expand Down
Loading