warn-access: ignore template parameters when matching operator new/delete [PR109224]
Checks
Context |
Check |
Description |
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 |
success
|
Build passed
|
linaro-tcwg-bot/tcwg_gcc_build--master-arm |
success
|
Build passed
|
linaro-tcwg-bot/tcwg_gcc_check--master-arm |
success
|
Test passed
|
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 |
success
|
Test passed
|
Commit Message
I'm not 100% clear on what the semantics of the matching here are meant
to be - AFAICT, an operator new/delete pair matches (after falling
through the other cases) if all their components (besides the actual
operator name, of course) match, and the pair of actual operator names
matches if one is a singleton new and the other a singleton delete, or,
similarly, if one is an array new and the other an array delete. We
also appear to ignore their argument types (or so it seems by
experimentation - I was not able to quite discern what path those take).
Stripping operator template arguments from either side of the pair
should have no impact on this logic, I think.
Tested on x86_64-pc-linux-gnu, no regressions.
OK for trunk?
TIA, have a lovely evening.
---------- >8 ----------
Template parameters on a member operator new cannot affect its member
status nor whether it is a singleton or array operator new, hence, we
can ignore it for purposes of matching. Similar logic applies to the
placement operator delete.
In the PR (and a lot of idiomatic coroutine code generally), operator
new is templated in order to be able to inspect (some of) the arguments
passed to the coroutine, to make allocation-related decisions. However,
the coroutine implementation will not call a placement delete form, so
it cannot get templated. As a result, when demangling, we have an extra
template DEMANGLE_COMPONENT_TEMPLATE around the actual operator new, but
not operator delete. This terminates new_delete_mismatch_p early.
PR middle-end/109224 - Wmismatched-new-delete false positive with a templated operator new (common with coroutines)
gcc/ChangeLog:
PR middle-end/109224
* gimple-ssa-warn-access.cc (new_delete_mismatch_p): Strip
DEMANGLE_COMPONENT_TEMPLATE from the operator new and operator
after demangling.
gcc/testsuite/ChangeLog:
PR middle-end/109224
* g++.dg/warn/Wmismatched-new-delete-9.C: New test.
---
gcc/gimple-ssa-warn-access.cc | 18 ++++++-
.../g++.dg/warn/Wmismatched-new-delete-9.C | 47 +++++++++++++++++++
gcc/testsuite/g++.dg/warn/pr109224.C | 25 ++++++++++
3 files changed, 89 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-9.C
create mode 100644 gcc/testsuite/g++.dg/warn/pr109224.C
Comments
On 8/2/24 4:36 PM, Arsen Arsenović wrote:
> I'm not 100% clear on what the semantics of the matching here are meant
> to be - AFAICT, an operator new/delete pair matches (after falling
> through the other cases) if all their components (besides the actual
> operator name, of course) match, and the pair of actual operator names
> matches if one is a singleton new and the other a singleton delete, or,
> similarly, if one is an array new and the other an array delete. We
> also appear to ignore their argument types (or so it seems by
> experimentation - I was not able to quite discern what path those take).
Ignoring argument types is correct, placement delete is only called for
exception handling.
> Stripping operator template arguments from either side of the pair
> should have no impact on this logic, I think.
Agreed.
> Tested on x86_64-pc-linux-gnu, no regressions.
>
> OK for trunk?
>
> TIA, have a lovely evening.
> ---------- >8 ----------
> Template parameters on a member operator new cannot affect its member
> status nor whether it is a singleton or array operator new, hence, we
> can ignore it for purposes of matching. Similar logic applies to the
> placement operator delete.
>
> In the PR (and a lot of idiomatic coroutine code generally), operator
> new is templated in order to be able to inspect (some of) the arguments
> passed to the coroutine, to make allocation-related decisions. However,
> the coroutine implementation will not call a placement delete form, so
> it cannot get templated. As a result, when demangling, we have an extra
> template DEMANGLE_COMPONENT_TEMPLATE around the actual operator new, but
> not operator delete. This terminates new_delete_mismatch_p early.
>
> PR middle-end/109224 - Wmismatched-new-delete false positive with a templated operator new (common with coroutines)
>
> gcc/ChangeLog:
>
> PR middle-end/109224
> * gimple-ssa-warn-access.cc (new_delete_mismatch_p): Strip
> DEMANGLE_COMPONENT_TEMPLATE from the operator new and operator
> after demangling.
>
> gcc/testsuite/ChangeLog:
>
> PR middle-end/109224
> * g++.dg/warn/Wmismatched-new-delete-9.C: New test.
> ---
> gcc/gimple-ssa-warn-access.cc | 18 ++++++-
> .../g++.dg/warn/Wmismatched-new-delete-9.C | 47 +++++++++++++++++++
> gcc/testsuite/g++.dg/warn/pr109224.C | 25 ++++++++++
> 3 files changed, 89 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-9.C
> create mode 100644 gcc/testsuite/g++.dg/warn/pr109224.C
>
> diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
> index 61f9f0f3d310..e3fec5fb8e77 100644
> --- a/gcc/gimple-ssa-warn-access.cc
> +++ b/gcc/gimple-ssa-warn-access.cc
> @@ -1762,7 +1762,23 @@ new_delete_mismatch_p (tree new_decl, tree delete_decl)
> void *np = NULL, *dp = NULL;
> demangle_component *ndc = cplus_demangle_v3_components (new_str, 0, &np);
> demangle_component *ddc = cplus_demangle_v3_components (del_str, 0, &dp);
> - bool mismatch = ndc && ddc && new_delete_mismatch_p (*ndc, *ddc);
> +
> + /* Sometimes, notably quite often with coroutines, 'operator new' is
> + templated. However, template arguments can't change whether a given
> + new/delete is a singleton or array one, nor what it is a member of, so
> + the template arguments can be safely ignored for the purposes of checking
> + for mismatches. */
> +
> + auto strip_dc_template = [] (demangle_component* dc)
> + {
> + if (dc->type == DEMANGLE_COMPONENT_TEMPLATE)
> + dc = dc->u.s_binary.left;
> + return dc;
> + };
> +
> + bool mismatch = ndc && ddc
> + && new_delete_mismatch_p (*strip_dc_template (ndc),
> + *strip_dc_template (ddc));
The && should not be left of the =; if the initializer needs to span
multiple lines, it's usually best to wrap it in ().
> free (np);
> free (dp);
> return mismatch;
> diff --git a/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-9.C b/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-9.C
> new file mode 100644
> index 000000000000..fa511bbfdb4b
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-9.C
> @@ -0,0 +1,47 @@
> +/* { dg-do compile { target c++11 } } */
> +/* { dg-additional-options "-Wmismatched-new-delete" } */
> +/* PR middle-end/109224 */
> +/* Verify that we consider templated operator new matching with its operator
> + delete. */
> +
> +#include <new>
> +
> +struct foo
> +{
> + template<typename... Args>
> + void* operator new (std::size_t sz, Args&&...);
> + template<typename... Args>
> + void* operator new[] (std::size_t sz, Args&&...);
> +
> + void operator delete (void* x);
> + void operator delete[] (void* x);
> +
> + template<typename... Args>
> + void operator delete (void* x, Args&&...);
> + template<typename... Args>
> + void operator delete[] (void* x, Args&&...);
> +};
> +
> +void
> +f ()
> +{
> + delete (new (123, true) foo);
> + delete[] (new (123, true) foo[123]);
> +
> + delete (new (123, true) foo[123]);
> + // { dg-warning "Wmismatched-new-delete" "" { target *-*-* } {.-1} }
> + // { dg-note "returned from" "" { target *-*-* } {.-2} }
> + delete[] (new (123, true) foo);
> + // { dg-warning "Wmismatched-new-delete" "" { target *-*-* } {.-1} }
> + // { dg-note "returned from" "" { target *-*-* } {.-2} }
> +
> + foo::operator delete (new (123, true) foo, 123, true);
> + foo::operator delete[] (new (123, true) foo[123], 123, true);
Instead of passing the result of a new-expression directly to operator
delete, you should also call operator new directly.
> + foo::operator delete (new (123, true) foo[123], 123, true);
> + // { dg-warning "Wmismatched-new-delete" "" { target *-*-* } {.-1} }
> + // { dg-note "returned from" "" { target *-*-* } {.-2} }
> + foo::operator delete[] (new (123, true) foo, 123, true);
Likewise.
Jason
Jason Merrill <jason@redhat.com> writes:
> On 8/2/24 4:36 PM, Arsen Arsenović wrote:
>> I'm not 100% clear on what the semantics of the matching here are meant
>> to be - AFAICT, an operator new/delete pair matches (after falling
>> through the other cases) if all their components (besides the actual
>> operator name, of course) match, and the pair of actual operator names
>> matches if one is a singleton new and the other a singleton delete, or,
>> similarly, if one is an array new and the other an array delete. We
>> also appear to ignore their argument types (or so it seems by
>> experimentation - I was not able to quite discern what path those take).
>
> Ignoring argument types is correct, placement delete is only called for
> exception handling.
ACK - good to know.
>> Stripping operator template arguments from either side of the pair
>> should have no impact on this logic, I think.
>
> Agreed.
>
>> Tested on x86_64-pc-linux-gnu, no regressions.
>> OK for trunk?
>> TIA, have a lovely evening.
>> ---------- >8 ----------
>> Template parameters on a member operator new cannot affect its member
>> status nor whether it is a singleton or array operator new, hence, we
>> can ignore it for purposes of matching. Similar logic applies to the
>> placement operator delete.
>> In the PR (and a lot of idiomatic coroutine code generally), operator
>> new is templated in order to be able to inspect (some of) the arguments
>> passed to the coroutine, to make allocation-related decisions. However,
>> the coroutine implementation will not call a placement delete form, so
>> it cannot get templated. As a result, when demangling, we have an extra
>> template DEMANGLE_COMPONENT_TEMPLATE around the actual operator new, but
>> not operator delete. This terminates new_delete_mismatch_p early.
>> PR middle-end/109224 - Wmismatched-new-delete false positive with a templated
>> operator new (common with coroutines)
>> gcc/ChangeLog:
>> PR middle-end/109224
>> * gimple-ssa-warn-access.cc (new_delete_mismatch_p): Strip
>> DEMANGLE_COMPONENT_TEMPLATE from the operator new and operator
>> after demangling.
>> gcc/testsuite/ChangeLog:
>> PR middle-end/109224
>> * g++.dg/warn/Wmismatched-new-delete-9.C: New test.
>> ---
>> gcc/gimple-ssa-warn-access.cc | 18 ++++++-
>> .../g++.dg/warn/Wmismatched-new-delete-9.C | 47 +++++++++++++++++++
>> gcc/testsuite/g++.dg/warn/pr109224.C | 25 ++++++++++
>> 3 files changed, 89 insertions(+), 1 deletion(-)
>> create mode 100644 gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-9.C
>> create mode 100644 gcc/testsuite/g++.dg/warn/pr109224.C
>> diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
>> index 61f9f0f3d310..e3fec5fb8e77 100644
>> --- a/gcc/gimple-ssa-warn-access.cc
>> +++ b/gcc/gimple-ssa-warn-access.cc
>> @@ -1762,7 +1762,23 @@ new_delete_mismatch_p (tree new_decl, tree delete_decl)
>> void *np = NULL, *dp = NULL;
>> demangle_component *ndc = cplus_demangle_v3_components (new_str, 0, &np);
>> demangle_component *ddc = cplus_demangle_v3_components (del_str, 0, &dp);
>> - bool mismatch = ndc && ddc && new_delete_mismatch_p (*ndc, *ddc);
>> +
>> + /* Sometimes, notably quite often with coroutines, 'operator new' is
>> + templated. However, template arguments can't change whether a given
>> + new/delete is a singleton or array one, nor what it is a member of, so
>> + the template arguments can be safely ignored for the purposes of checking
>> + for mismatches. */
>> +
>> + auto strip_dc_template = [] (demangle_component* dc)
>> + {
>> + if (dc->type == DEMANGLE_COMPONENT_TEMPLATE)
>> + dc = dc->u.s_binary.left;
>> + return dc;
>> + };
>> +
>> + bool mismatch = ndc && ddc
>> + && new_delete_mismatch_p (*strip_dc_template (ndc),
>> + *strip_dc_template (ddc));
>
> The && should not be left of the =; if the initializer needs to span multiple
> lines, it's usually best to wrap it in ().
Okay - done.
>> free (np);
>> free (dp);
>> return mismatch;
>> diff --git a/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-9.C b/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-9.C
>> new file mode 100644
>> index 000000000000..fa511bbfdb4b
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-9.C
>> @@ -0,0 +1,47 @@
>> +/* { dg-do compile { target c++11 } } */
>> +/* { dg-additional-options "-Wmismatched-new-delete" } */
>> +/* PR middle-end/109224 */
>> +/* Verify that we consider templated operator new matching with its operator
>> + delete. */
>> +
>> +#include <new>
>> +
>> +struct foo
>> +{
>> + template<typename... Args>
>> + void* operator new (std::size_t sz, Args&&...);
>> + template<typename... Args>
>> + void* operator new[] (std::size_t sz, Args&&...);
>> +
>> + void operator delete (void* x);
>> + void operator delete[] (void* x);
>> +
>> + template<typename... Args>
>> + void operator delete (void* x, Args&&...);
>> + template<typename... Args>
>> + void operator delete[] (void* x, Args&&...);
>> +};
>> +
>> +void
>> +f ()
>> +{
>> + delete (new (123, true) foo);
>> + delete[] (new (123, true) foo[123]);
>> +
>> + delete (new (123, true) foo[123]);
>> + // { dg-warning "Wmismatched-new-delete" "" { target *-*-* } {.-1} }
>> + // { dg-note "returned from" "" { target *-*-* } {.-2} }
>> + delete[] (new (123, true) foo);
>> + // { dg-warning "Wmismatched-new-delete" "" { target *-*-* } {.-1} }
>> + // { dg-note "returned from" "" { target *-*-* } {.-2} }
>> +
>> + foo::operator delete (new (123, true) foo, 123, true);
>> + foo::operator delete[] (new (123, true) foo[123], 123, true);
>
> Instead of passing the result of a new-expression directly to operator delete,
> you should also call operator new directly.
>
>> + foo::operator delete (new (123, true) foo[123], 123, true);
>> + // { dg-warning "Wmismatched-new-delete" "" { target *-*-* } {.-1} }
>> + // { dg-note "returned from" "" { target *-*-* } {.-2} }
>> + foo::operator delete[] (new (123, true) foo, 123, true);
>
> Likewise.
Ah, good point - adjusted.
Here are the changes to v1 I've made as well as the full patch below
them. Does this look OK?
--8<---------------cut here---------------start------------->8---
modified gcc/gimple-ssa-warn-access.cc
@@ -1776,9 +1776,9 @@ new_delete_mismatch_p (tree new_decl, tree delete_decl)
return dc;
};
- bool mismatch = ndc && ddc
- && new_delete_mismatch_p (*strip_dc_template (ndc),
- *strip_dc_template (ddc));
+ bool mismatch = (ndc && ddc
+ && new_delete_mismatch_p (*strip_dc_template (ndc),
+ *strip_dc_template (ddc)));
free (np);
free (dp);
return mismatch;
modified gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-9.C
@@ -35,13 +35,13 @@ f ()
// { dg-warning "Wmismatched-new-delete" "" { target *-*-* } {.-1} }
// { dg-note "returned from" "" { target *-*-* } {.-2} }
- foo::operator delete (new (123, true) foo, 123, true);
- foo::operator delete[] (new (123, true) foo[123], 123, true);
+ foo::operator delete (foo::operator new (1, 123, true), 123, true);
+ foo::operator delete[] (foo::operator new[] (123, 123, true), 123, true);
- foo::operator delete (new (123, true) foo[123], 123, true);
+ foo::operator delete (foo::operator new[] (123, 123, true), 123, true);
// { dg-warning "Wmismatched-new-delete" "" { target *-*-* } {.-1} }
// { dg-note "returned from" "" { target *-*-* } {.-2} }
- foo::operator delete[] (new (123, true) foo, 123, true);
+ foo::operator delete[] (foo::operator new (1, 123, true), 123, true);
// { dg-warning "Wmismatched-new-delete" "" { target *-*-* } {.-1} }
// { dg-note "returned from" "" { target *-*-* } {.-2} }
}
--8<---------------cut here---------------end--------------->8---
---------- >8 ----------
Template parameters on a member operator new cannot affect its member
status nor whether it is a singleton or array operator new, hence, we
can ignore it for purposes of matching. Similar logic applies to the
placement operator delete.
In the PR (and a lot of idiomatic coroutine code generally), operator
new is templated in order to be able to inspect (some of) the arguments
passed to the coroutine, to make allocation-related decisions. However,
the coroutine implementation will not call a placement delete form, so
it cannot get templated. As a result, when demangling, we have an extra
template DEMANGLE_COMPONENT_TEMPLATE around the actual operator new, but
not operator delete. This terminates new_delete_mismatch_p early.
PR middle-end/109224 - Wmismatched-new-delete false positive with a templated operator new (common with coroutines)
gcc/ChangeLog:
* gimple-ssa-warn-access.cc (new_delete_mismatch_p): Strip
DEMANGLE_COMPONENT_TEMPLATE from the operator new and operator
after demangling.
gcc/testsuite/ChangeLog:
* g++.dg/warn/Wmismatched-new-delete-9.C: New test.
---
gcc/gimple-ssa-warn-access.cc | 18 ++++++-
.../g++.dg/warn/Wmismatched-new-delete-9.C | 47 +++++++++++++++++++
gcc/testsuite/g++.dg/warn/pr109224.C | 25 ++++++++++
3 files changed, 89 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-9.C
create mode 100644 gcc/testsuite/g++.dg/warn/pr109224.C
diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
index 61f9f0f3d310..fcce63ee332d 100644
--- a/gcc/gimple-ssa-warn-access.cc
+++ b/gcc/gimple-ssa-warn-access.cc
@@ -1762,7 +1762,23 @@ new_delete_mismatch_p (tree new_decl, tree delete_decl)
void *np = NULL, *dp = NULL;
demangle_component *ndc = cplus_demangle_v3_components (new_str, 0, &np);
demangle_component *ddc = cplus_demangle_v3_components (del_str, 0, &dp);
- bool mismatch = ndc && ddc && new_delete_mismatch_p (*ndc, *ddc);
+
+ /* Sometimes, notably quite often with coroutines, 'operator new' is
+ templated. However, template arguments can't change whether a given
+ new/delete is a singleton or array one, nor what it is a member of, so
+ the template arguments can be safely ignored for the purposes of checking
+ for mismatches. */
+
+ auto strip_dc_template = [] (demangle_component* dc)
+ {
+ if (dc->type == DEMANGLE_COMPONENT_TEMPLATE)
+ dc = dc->u.s_binary.left;
+ return dc;
+ };
+
+ bool mismatch = (ndc && ddc
+ && new_delete_mismatch_p (*strip_dc_template (ndc),
+ *strip_dc_template (ddc)));
free (np);
free (dp);
return mismatch;
diff --git a/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-9.C b/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-9.C
new file mode 100644
index 000000000000..d431f4049e87
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-9.C
@@ -0,0 +1,47 @@
+/* { dg-do compile { target c++11 } } */
+/* { dg-additional-options "-Wmismatched-new-delete" } */
+/* PR middle-end/109224 */
+/* Verify that we consider templated operator new matching with its operator
+ delete. */
+
+#include <new>
+
+struct foo
+{
+ template<typename... Args>
+ void* operator new (std::size_t sz, Args&&...);
+ template<typename... Args>
+ void* operator new[] (std::size_t sz, Args&&...);
+
+ void operator delete (void* x);
+ void operator delete[] (void* x);
+
+ template<typename... Args>
+ void operator delete (void* x, Args&&...);
+ template<typename... Args>
+ void operator delete[] (void* x, Args&&...);
+};
+
+void
+f ()
+{
+ delete (new (123, true) foo);
+ delete[] (new (123, true) foo[123]);
+
+ delete (new (123, true) foo[123]);
+ // { dg-warning "Wmismatched-new-delete" "" { target *-*-* } {.-1} }
+ // { dg-note "returned from" "" { target *-*-* } {.-2} }
+ delete[] (new (123, true) foo);
+ // { dg-warning "Wmismatched-new-delete" "" { target *-*-* } {.-1} }
+ // { dg-note "returned from" "" { target *-*-* } {.-2} }
+
+ foo::operator delete (foo::operator new (1, 123, true), 123, true);
+ foo::operator delete[] (foo::operator new[] (123, 123, true), 123, true);
+
+ foo::operator delete (foo::operator new[] (123, 123, true), 123, true);
+ // { dg-warning "Wmismatched-new-delete" "" { target *-*-* } {.-1} }
+ // { dg-note "returned from" "" { target *-*-* } {.-2} }
+ foo::operator delete[] (foo::operator new (1, 123, true), 123, true);
+ // { dg-warning "Wmismatched-new-delete" "" { target *-*-* } {.-1} }
+ // { dg-note "returned from" "" { target *-*-* } {.-2} }
+}
diff --git a/gcc/testsuite/g++.dg/warn/pr109224.C b/gcc/testsuite/g++.dg/warn/pr109224.C
new file mode 100644
index 000000000000..4b6102226ffc
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/pr109224.C
@@ -0,0 +1,25 @@
+// { dg-do compile { target c++20 } }
+#include <coroutine>
+
+struct Task {
+ struct promise_type {
+ std::suspend_never initial_suspend() { return {}; }
+ std::suspend_never final_suspend() noexcept { return {}; }
+ void unhandled_exception() { throw; }
+ Task get_return_object() { return {}; }
+ void return_void() {}
+
+ template<class I>
+ void* operator new(std::size_t sz, I);
+
+ void operator delete(void* ptr, std::size_t);
+ };
+};
+
+Task f(int) {
+ co_return;
+}
+
+int main() {
+ f(42);
+}
Hi,
Arsen Arsenović <arsen@aarsen.me> writes:
>> The && should not be left of the =; if the initializer needs to span multiple
>> lines, it's usually best to wrap it in ().
>
> Okay - done.
[...]
>>> + foo::operator delete (new (123, true) foo, 123, true);
>>> + foo::operator delete[] (new (123, true) foo[123], 123, true);
>>
>> Instead of passing the result of a new-expression directly to operator delete,
>> you should also call operator new directly.
>>
>>> + foo::operator delete (new (123, true) foo[123], 123, true);
>>> + // { dg-warning "Wmismatched-new-delete" "" { target *-*-* } {.-1} }
>>> + // { dg-note "returned from" "" { target *-*-* } {.-2} }
>>> + foo::operator delete[] (new (123, true) foo, 123, true);
>>
>> Likewise.
>
> Ah, good point - adjusted.
>
> Here are the changes to v1 I've made as well as the full patch below
> them. Does this look OK?
>
> --8<---------------cut here---------------start------------->8---
> modified gcc/gimple-ssa-warn-access.cc
> @@ -1776,9 +1776,9 @@ new_delete_mismatch_p (tree new_decl, tree delete_decl)
> return dc;
> };
>
> - bool mismatch = ndc && ddc
> - && new_delete_mismatch_p (*strip_dc_template (ndc),
> - *strip_dc_template (ddc));
> + bool mismatch = (ndc && ddc
> + && new_delete_mismatch_p (*strip_dc_template (ndc),
> + *strip_dc_template (ddc)));
> free (np);
> free (dp);
> return mismatch;
> modified gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-9.C
> @@ -35,13 +35,13 @@ f ()
> // { dg-warning "Wmismatched-new-delete" "" { target *-*-* } {.-1} }
> // { dg-note "returned from" "" { target *-*-* } {.-2} }
>
> - foo::operator delete (new (123, true) foo, 123, true);
> - foo::operator delete[] (new (123, true) foo[123], 123, true);
> + foo::operator delete (foo::operator new (1, 123, true), 123, true);
> + foo::operator delete[] (foo::operator new[] (123, 123, true), 123, true);
>
> - foo::operator delete (new (123, true) foo[123], 123, true);
> + foo::operator delete (foo::operator new[] (123, 123, true), 123, true);
> // { dg-warning "Wmismatched-new-delete" "" { target *-*-* } {.-1} }
> // { dg-note "returned from" "" { target *-*-* } {.-2} }
> - foo::operator delete[] (new (123, true) foo, 123, true);
> + foo::operator delete[] (foo::operator new (1, 123, true), 123, true);
> // { dg-warning "Wmismatched-new-delete" "" { target *-*-* } {.-1} }
> // { dg-note "returned from" "" { target *-*-* } {.-2} }
> }
> --8<---------------cut here---------------end--------------->8---
Gentle ping on this adjustment to the patch (full reproduction of the
adjusted patch is below).
TIA, have a lovely evening.
> ---------- >8 ----------
> Template parameters on a member operator new cannot affect its member
> status nor whether it is a singleton or array operator new, hence, we
> can ignore it for purposes of matching. Similar logic applies to the
> placement operator delete.
>
> In the PR (and a lot of idiomatic coroutine code generally), operator
> new is templated in order to be able to inspect (some of) the arguments
> passed to the coroutine, to make allocation-related decisions. However,
> the coroutine implementation will not call a placement delete form, so
> it cannot get templated. As a result, when demangling, we have an extra
> template DEMANGLE_COMPONENT_TEMPLATE around the actual operator new, but
> not operator delete. This terminates new_delete_mismatch_p early.
>
> PR middle-end/109224 - Wmismatched-new-delete false positive with a templated operator new (common with coroutines)
>
> gcc/ChangeLog:
>
> * gimple-ssa-warn-access.cc (new_delete_mismatch_p): Strip
> DEMANGLE_COMPONENT_TEMPLATE from the operator new and operator
> after demangling.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/warn/Wmismatched-new-delete-9.C: New test.
> ---
> gcc/gimple-ssa-warn-access.cc | 18 ++++++-
> .../g++.dg/warn/Wmismatched-new-delete-9.C | 47 +++++++++++++++++++
> gcc/testsuite/g++.dg/warn/pr109224.C | 25 ++++++++++
> 3 files changed, 89 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-9.C
> create mode 100644 gcc/testsuite/g++.dg/warn/pr109224.C
>
> diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
> index 61f9f0f3d310..fcce63ee332d 100644
> --- a/gcc/gimple-ssa-warn-access.cc
> +++ b/gcc/gimple-ssa-warn-access.cc
> @@ -1762,7 +1762,23 @@ new_delete_mismatch_p (tree new_decl, tree delete_decl)
> void *np = NULL, *dp = NULL;
> demangle_component *ndc = cplus_demangle_v3_components (new_str, 0, &np);
> demangle_component *ddc = cplus_demangle_v3_components (del_str, 0, &dp);
> - bool mismatch = ndc && ddc && new_delete_mismatch_p (*ndc, *ddc);
> +
> + /* Sometimes, notably quite often with coroutines, 'operator new' is
> + templated. However, template arguments can't change whether a given
> + new/delete is a singleton or array one, nor what it is a member of, so
> + the template arguments can be safely ignored for the purposes of checking
> + for mismatches. */
> +
> + auto strip_dc_template = [] (demangle_component* dc)
> + {
> + if (dc->type == DEMANGLE_COMPONENT_TEMPLATE)
> + dc = dc->u.s_binary.left;
> + return dc;
> + };
> +
> + bool mismatch = (ndc && ddc
> + && new_delete_mismatch_p (*strip_dc_template (ndc),
> + *strip_dc_template (ddc)));
> free (np);
> free (dp);
> return mismatch;
> diff --git a/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-9.C b/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-9.C
> new file mode 100644
> index 000000000000..d431f4049e87
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-9.C
> @@ -0,0 +1,47 @@
> +/* { dg-do compile { target c++11 } } */
> +/* { dg-additional-options "-Wmismatched-new-delete" } */
> +/* PR middle-end/109224 */
> +/* Verify that we consider templated operator new matching with its operator
> + delete. */
> +
> +#include <new>
> +
> +struct foo
> +{
> + template<typename... Args>
> + void* operator new (std::size_t sz, Args&&...);
> + template<typename... Args>
> + void* operator new[] (std::size_t sz, Args&&...);
> +
> + void operator delete (void* x);
> + void operator delete[] (void* x);
> +
> + template<typename... Args>
> + void operator delete (void* x, Args&&...);
> + template<typename... Args>
> + void operator delete[] (void* x, Args&&...);
> +};
> +
> +void
> +f ()
> +{
> + delete (new (123, true) foo);
> + delete[] (new (123, true) foo[123]);
> +
> + delete (new (123, true) foo[123]);
> + // { dg-warning "Wmismatched-new-delete" "" { target *-*-* } {.-1} }
> + // { dg-note "returned from" "" { target *-*-* } {.-2} }
> + delete[] (new (123, true) foo);
> + // { dg-warning "Wmismatched-new-delete" "" { target *-*-* } {.-1} }
> + // { dg-note "returned from" "" { target *-*-* } {.-2} }
> +
> + foo::operator delete (foo::operator new (1, 123, true), 123, true);
> + foo::operator delete[] (foo::operator new[] (123, 123, true), 123, true);
> +
> + foo::operator delete (foo::operator new[] (123, 123, true), 123, true);
> + // { dg-warning "Wmismatched-new-delete" "" { target *-*-* } {.-1} }
> + // { dg-note "returned from" "" { target *-*-* } {.-2} }
> + foo::operator delete[] (foo::operator new (1, 123, true), 123, true);
> + // { dg-warning "Wmismatched-new-delete" "" { target *-*-* } {.-1} }
> + // { dg-note "returned from" "" { target *-*-* } {.-2} }
> +}
> diff --git a/gcc/testsuite/g++.dg/warn/pr109224.C b/gcc/testsuite/g++.dg/warn/pr109224.C
> new file mode 100644
> index 000000000000..4b6102226ffc
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/warn/pr109224.C
> @@ -0,0 +1,25 @@
> +// { dg-do compile { target c++20 } }
> +#include <coroutine>
> +
> +struct Task {
> + struct promise_type {
> + std::suspend_never initial_suspend() { return {}; }
> + std::suspend_never final_suspend() noexcept { return {}; }
> + void unhandled_exception() { throw; }
> + Task get_return_object() { return {}; }
> + void return_void() {}
> +
> + template<class I>
> + void* operator new(std::size_t sz, I);
> +
> + void operator delete(void* ptr, std::size_t);
> + };
> +};
> +
> +Task f(int) {
> + co_return;
> +}
> +
> +int main() {
> + f(42);
> +}
Evening,
Arsen Arsenović <arsen@aarsen.me> writes:
> [[PGP Signed Part:Good signature from 52C294301EA2C493 Arsen Arsenović <arsen@gcc.gnu.org> (trust ultimate) created at 2024-08-28T23:00:44+0200 using EDDSA]]
> Hi,
>
> Arsen Arsenović <arsen@aarsen.me> writes:
>
>>> The && should not be left of the =; if the initializer needs to span multiple
>>> lines, it's usually best to wrap it in ().
>>
>> Okay - done.
>
> [...]
>
>>>> + foo::operator delete (new (123, true) foo, 123, true);
>>>> + foo::operator delete[] (new (123, true) foo[123], 123, true);
>>>
>>> Instead of passing the result of a new-expression directly to operator delete,
>>> you should also call operator new directly.
>>>
>>>> + foo::operator delete (new (123, true) foo[123], 123, true);
>>>> + // { dg-warning "Wmismatched-new-delete" "" { target *-*-* } {.-1} }
>>>> + // { dg-note "returned from" "" { target *-*-* } {.-2} }
>>>> + foo::operator delete[] (new (123, true) foo, 123, true);
>>>
>>> Likewise.
>>
>> Ah, good point - adjusted.
>>
>> Here are the changes to v1 I've made as well as the full patch below
>> them. Does this look OK?
>>
>> --8<---------------cut here---------------start------------->8---
>> modified gcc/gimple-ssa-warn-access.cc
>> @@ -1776,9 +1776,9 @@ new_delete_mismatch_p (tree new_decl, tree delete_decl)
>> return dc;
>> };
>>
>> - bool mismatch = ndc && ddc
>> - && new_delete_mismatch_p (*strip_dc_template (ndc),
>> - *strip_dc_template (ddc));
>> + bool mismatch = (ndc && ddc
>> + && new_delete_mismatch_p (*strip_dc_template (ndc),
>> + *strip_dc_template (ddc)));
>> free (np);
>> free (dp);
>> return mismatch;
>> modified gcc/testsuite/g++.dg/warn/Wmismatched-new-delete-9.C
>> @@ -35,13 +35,13 @@ f ()
>> // { dg-warning "Wmismatched-new-delete" "" { target *-*-* } {.-1} }
>> // { dg-note "returned from" "" { target *-*-* } {.-2} }
>>
>> - foo::operator delete (new (123, true) foo, 123, true);
>> - foo::operator delete[] (new (123, true) foo[123], 123, true);
>> + foo::operator delete (foo::operator new (1, 123, true), 123, true);
>> + foo::operator delete[] (foo::operator new[] (123, 123, true), 123, true);
>>
>> - foo::operator delete (new (123, true) foo[123], 123, true);
>> + foo::operator delete (foo::operator new[] (123, 123, true), 123, true);
>> // { dg-warning "Wmismatched-new-delete" "" { target *-*-* } {.-1} }
>> // { dg-note "returned from" "" { target *-*-* } {.-2} }
>> - foo::operator delete[] (new (123, true) foo, 123, true);
>> + foo::operator delete[] (foo::operator new (1, 123, true), 123, true);
>> // { dg-warning "Wmismatched-new-delete" "" { target *-*-* } {.-1} }
>> // { dg-note "returned from" "" { target *-*-* } {.-2} }
>> }
>> --8<---------------cut here---------------end--------------->8---
Gentle ping again. Full patch:
https://inbox.sourceware.org/gcc-patches/86y14ptvdi.fsf@aarsen.me/
TIA, have a lovely evening.
@@ -1762,7 +1762,23 @@ new_delete_mismatch_p (tree new_decl, tree delete_decl)
void *np = NULL, *dp = NULL;
demangle_component *ndc = cplus_demangle_v3_components (new_str, 0, &np);
demangle_component *ddc = cplus_demangle_v3_components (del_str, 0, &dp);
- bool mismatch = ndc && ddc && new_delete_mismatch_p (*ndc, *ddc);
+
+ /* Sometimes, notably quite often with coroutines, 'operator new' is
+ templated. However, template arguments can't change whether a given
+ new/delete is a singleton or array one, nor what it is a member of, so
+ the template arguments can be safely ignored for the purposes of checking
+ for mismatches. */
+
+ auto strip_dc_template = [] (demangle_component* dc)
+ {
+ if (dc->type == DEMANGLE_COMPONENT_TEMPLATE)
+ dc = dc->u.s_binary.left;
+ return dc;
+ };
+
+ bool mismatch = ndc && ddc
+ && new_delete_mismatch_p (*strip_dc_template (ndc),
+ *strip_dc_template (ddc));
free (np);
free (dp);
return mismatch;
new file mode 100644
@@ -0,0 +1,47 @@
+/* { dg-do compile { target c++11 } } */
+/* { dg-additional-options "-Wmismatched-new-delete" } */
+/* PR middle-end/109224 */
+/* Verify that we consider templated operator new matching with its operator
+ delete. */
+
+#include <new>
+
+struct foo
+{
+ template<typename... Args>
+ void* operator new (std::size_t sz, Args&&...);
+ template<typename... Args>
+ void* operator new[] (std::size_t sz, Args&&...);
+
+ void operator delete (void* x);
+ void operator delete[] (void* x);
+
+ template<typename... Args>
+ void operator delete (void* x, Args&&...);
+ template<typename... Args>
+ void operator delete[] (void* x, Args&&...);
+};
+
+void
+f ()
+{
+ delete (new (123, true) foo);
+ delete[] (new (123, true) foo[123]);
+
+ delete (new (123, true) foo[123]);
+ // { dg-warning "Wmismatched-new-delete" "" { target *-*-* } {.-1} }
+ // { dg-note "returned from" "" { target *-*-* } {.-2} }
+ delete[] (new (123, true) foo);
+ // { dg-warning "Wmismatched-new-delete" "" { target *-*-* } {.-1} }
+ // { dg-note "returned from" "" { target *-*-* } {.-2} }
+
+ foo::operator delete (new (123, true) foo, 123, true);
+ foo::operator delete[] (new (123, true) foo[123], 123, true);
+
+ foo::operator delete (new (123, true) foo[123], 123, true);
+ // { dg-warning "Wmismatched-new-delete" "" { target *-*-* } {.-1} }
+ // { dg-note "returned from" "" { target *-*-* } {.-2} }
+ foo::operator delete[] (new (123, true) foo, 123, true);
+ // { dg-warning "Wmismatched-new-delete" "" { target *-*-* } {.-1} }
+ // { dg-note "returned from" "" { target *-*-* } {.-2} }
+}
new file mode 100644
@@ -0,0 +1,25 @@
+// { dg-do compile { target c++20 } }
+#include <coroutine>
+
+struct Task {
+ struct promise_type {
+ std::suspend_never initial_suspend() { return {}; }
+ std::suspend_never final_suspend() noexcept { return {}; }
+ void unhandled_exception() { throw; }
+ Task get_return_object() { return {}; }
+ void return_void() {}
+
+ template<class I>
+ void* operator new(std::size_t sz, I);
+
+ void operator delete(void* ptr, std::size_t);
+ };
+};
+
+Task f(int) {
+ co_return;
+}
+
+int main() {
+ f(42);
+}