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
+33 −15
Open

One more fix for P3144R2 implementation #149406

Show file tree
Hide file tree
Changes from all 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
4 changes: 3 additions & 1 deletion clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,9 @@ Bug Fixes to C++ Support
^^^^^^^^^^^^^^^^^^^^^^^^
- Diagnose binding a reference to ``*nullptr`` during constant evaluation. (#GH48665)
- Suppress ``-Wdeprecated-declarations`` in implicitly generated functions. (#GH147293)
- Fix a crash when deleting a pointer to an incomplete array (#GH150359).
- Clang no longer rejects deleting a pointer to an incomplete array, regardless
of C++ standard version. (#GH149406)
- Fix a crash when deleting a pointer to an incomplete array. (#GH150359)

Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
Expand Down
16 changes: 8 additions & 8 deletions clang/lib/Sema/SemaExprCXX.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4039,18 +4039,18 @@ Sema::ActOnCXXDelete(SourceLocation StartLoc, bool UseGlobal,
} 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>())
if (const RecordType *RT = PointeeElem->getAs<RecordType>()) {
if (!RequireCompleteType(StartLoc, PointeeElem,
LangOpts.CPlusPlus26
? diag::err_delete_incomplete
: diag::warn_delete_incomplete,
Ex.get())) {
PointeeRD = cast<CXXRecordDecl>(RT->getDecl());
}
}
}

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 type 'Foo'}}
}

namespace TypeTraitExpr {
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 type '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 type '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 type 'T4'}}
union T4 { ~T4(); };
17 changes: 14 additions & 3 deletions clang/test/SemaCXX/new-delete.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ struct S // expected-note {{candidate}}
S(double, int); // expected-note 2 {{candidate}}
S(float, int); // expected-note 2 {{candidate}}
};
struct T; // expected-note{{forward declaration of 'T'}}
struct T; // expected-note 3{{forward declaration of 'T'}}
struct U
{
// A special new, to verify that the global version isn't used.
Expand Down Expand Up @@ -216,6 +216,8 @@ void good_deletes()
delete (int*)0;
delete [](int*)0;
delete (S*)0;
delete [](S(*)[2])0;
delete [](S(*)[])0;
::delete (int*)0;
}

Expand All @@ -227,7 +229,13 @@ void bad_deletes()
// cxx98-23-warning@-1 {{cannot delete expression with pointer-to-'void' type 'void *'}}
// since-cxx26-error@-2 {{cannot delete pointer to incomplete type 'void'}}
delete (T*)0;
// cxx98-23-warning@-1 {{deleting pointer to incomplete type}}
// cxx98-23-warning@-1 {{deleting pointer to incomplete type 'T'}}
// since-cxx26-error@-2 {{cannot delete pointer to incomplete type 'T'}}
delete [](T(*)[2])0;
// cxx98-23-warning@-1 {{deleting pointer to incomplete type 'T'}}
// since-cxx26-error@-2 {{cannot delete pointer to incomplete type 'T'}}
delete [](T(*)[])0;
// cxx98-23-warning@-1 {{deleting pointer to incomplete type 'T'}}
// since-cxx26-error@-2 {{cannot delete pointer to incomplete type 'T'}}
::S::delete (int*)0; // expected-error {{expected unqualified-id}}
}
Expand Down Expand Up @@ -570,7 +578,7 @@ 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}}
// cxx98-23-warning@-2 {{deleting pointer to incomplete type 'A'}}
// since-cxx26-error@-3 {{cannot delete pointer to incomplete type 'A'}}
}

Expand All @@ -595,6 +603,9 @@ struct GH99278_2 {
} f;
};
GH99278_2<void> e;
void GH99278_3(int(*p)[]) {
delete[] p;
};
Copy link
Collaborator

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

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

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

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

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

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

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

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