| Message ID | 20240802211503.3992610-2-arsen@aarsen.me |
|---|---|
| State | New |
| Headers | show |
| Series | warn-access: ignore template parameters when matching operator new/delete [PR109224] | expand |
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.
Arsen Arsenović <arsen@aarsen.me> writes: > Gentle ping again. Full patch: > https://inbox.sourceware.org/gcc-patches/86y14ptvdi.fsf@aarsen.me/ And again. To clarify, the above is a v2 of sorts (it has the comment fixed, I just didn't update the subject). TIA, have a lovely day.
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)); 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); + + 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} } +} 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); +}