* [PATCH] c++: fix ICE with -Wduplicated-cond [PR107593] @ 2023-01-26 22:17 Marek Polacek 2023-01-27 22:15 ` Patrick Palka 0 siblings, 1 reply; 10+ messages in thread From: Marek Polacek @ 2023-01-26 22:17 UTC (permalink / raw) To: Jason Merrill, GCC Patches Here we crash because a CAST_EXPR, representing T(), doesn't have its operand, and operand_equal_p's STRIP_ANY_LOCATION_WRAPPER doesn't expect that. (o_e_p is called from warn_duplicated_cond_add_or_warn.) In the past we've adjusted o_e_p to better cope with template codes, but in this case I think we just want to avoid attempting to warn about inst-dependent expressions; I don't think I've ever envisioned -Wduplicated-cond to warn about them. The ICE started with r12-6022, two-stage name lookup for overloaded operators, which gave dependent operators a TREE_TYPE (in particular, DEPENDENT_OPERATOR_TYPE), so we no longer bail out here in o_e_p: /* Similar, if either does not have a type (like a template id), they aren't equal. */ if (!TREE_TYPE (arg0) || !TREE_TYPE (arg1)) return false; Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? PR c++/107593 gcc/cp/ChangeLog: * parser.cc (cp_parser_selection_statement): Don't do -Wduplicated-cond when the condition is dependent. gcc/testsuite/ChangeLog: * g++.dg/warn/Wduplicated-cond3.C: New test. --- gcc/cp/parser.cc | 3 +- gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C | 38 +++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc index 4cdc1cd472f..3df85d49e16 100644 --- a/gcc/cp/parser.cc +++ b/gcc/cp/parser.cc @@ -13209,7 +13209,8 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p, /* Add the condition. */ condition = finish_if_stmt_cond (condition, statement); - if (warn_duplicated_cond) + if (warn_duplicated_cond + && !instantiation_dependent_expression_p (condition)) warn_duplicated_cond_add_or_warn (token->location, condition, &chain); diff --git a/gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C b/gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C new file mode 100644 index 00000000000..3da054e5485 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C @@ -0,0 +1,38 @@ +// PR c++/107593 +// { dg-do compile } +// { dg-options "-Wduplicated-cond" } + +template <typename T> +void +foo () +{ + if (T() && T() && int()) + ; + else if (T() && T() && int()) + ; +} + +template <typename T> +void bar(T a) +{ + if (a) + ; + else if (a) + ; +} + +template <typename> +void baz(int a) +{ + if (a) + ; + else if (a) // { dg-warning "duplicated" } + ; +} +void +f () +{ + foo<int>(); + bar(1); + baz<int>(1); +} base-commit: 94673a121cfc7f9d51c9d05e31795477f4dc8dc7 -- 2.39.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++: fix ICE with -Wduplicated-cond [PR107593] 2023-01-26 22:17 [PATCH] c++: fix ICE with -Wduplicated-cond [PR107593] Marek Polacek @ 2023-01-27 22:15 ` Patrick Palka 2023-01-27 22:49 ` Jason Merrill 2023-01-27 22:56 ` Marek Polacek 0 siblings, 2 replies; 10+ messages in thread From: Patrick Palka @ 2023-01-27 22:15 UTC (permalink / raw) To: Marek Polacek; +Cc: Jason Merrill, GCC Patches On Thu, 26 Jan 2023, Marek Polacek via Gcc-patches wrote: > Here we crash because a CAST_EXPR, representing T(), doesn't have > its operand, and operand_equal_p's STRIP_ANY_LOCATION_WRAPPER doesn't > expect that. (o_e_p is called from warn_duplicated_cond_add_or_warn.) > > In the past we've adjusted o_e_p to better cope with template codes, > but in this case I think we just want to avoid attempting to warn > about inst-dependent expressions; I don't think I've ever envisioned > -Wduplicated-cond to warn about them. > > The ICE started with r12-6022, two-stage name lookup for overloaded > operators, which gave dependent operators a TREE_TYPE (in particular, > DEPENDENT_OPERATOR_TYPE), so we no longer bail out here in o_e_p: > > /* Similar, if either does not have a type (like a template id), > they aren't equal. */ > if (!TREE_TYPE (arg0) || !TREE_TYPE (arg1)) > return false; > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > PR c++/107593 > > gcc/cp/ChangeLog: > > * parser.cc (cp_parser_selection_statement): Don't do > -Wduplicated-cond when the condition is dependent. > > gcc/testsuite/ChangeLog: > > * g++.dg/warn/Wduplicated-cond3.C: New test. > --- > gcc/cp/parser.cc | 3 +- > gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C | 38 +++++++++++++++++++ > 2 files changed, 40 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C > > diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc > index 4cdc1cd472f..3df85d49e16 100644 > --- a/gcc/cp/parser.cc > +++ b/gcc/cp/parser.cc > @@ -13209,7 +13209,8 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p, > /* Add the condition. */ > condition = finish_if_stmt_cond (condition, statement); > > - if (warn_duplicated_cond) > + if (warn_duplicated_cond > + && !instantiation_dependent_expression_p (condition)) > warn_duplicated_cond_add_or_warn (token->location, condition, > &chain); I noticed warn_duplicated_cond_add_or_warn already has logic to handle TREE_SIDE_EFFECTS conditions by invaliding the entire chain. I wonder if we'd want to do the same for instantiation-dep conditions? > > diff --git a/gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C b/gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C > new file mode 100644 > index 00000000000..3da054e5485 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C > @@ -0,0 +1,38 @@ > +// PR c++/107593 > +// { dg-do compile } > +// { dg-options "-Wduplicated-cond" } > + > +template <typename T> > +void > +foo () > +{ > + if (T() && T() && int()) > + ; > + else if (T() && T() && int()) > + ; > +} > + > +template <typename T> > +void bar(T a) > +{ > + if (a) > + ; > + else if (a) > + ; > +} > + > +template <typename> > +void baz(int a) > +{ > + if (a) > + ; > + else if (a) // { dg-warning "duplicated" } > + ; > +} > +void > +f () > +{ > + foo<int>(); > + bar(1); > + baz<int>(1); > +} > > base-commit: 94673a121cfc7f9d51c9d05e31795477f4dc8dc7 > -- > 2.39.1 > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++: fix ICE with -Wduplicated-cond [PR107593] 2023-01-27 22:15 ` Patrick Palka @ 2023-01-27 22:49 ` Jason Merrill 2023-01-27 22:56 ` Marek Polacek 1 sibling, 0 replies; 10+ messages in thread From: Jason Merrill @ 2023-01-27 22:49 UTC (permalink / raw) To: Patrick Palka, Marek Polacek; +Cc: GCC Patches On 1/27/23 17:15, Patrick Palka wrote: > On Thu, 26 Jan 2023, Marek Polacek via Gcc-patches wrote: > >> Here we crash because a CAST_EXPR, representing T(), doesn't have >> its operand, and operand_equal_p's STRIP_ANY_LOCATION_WRAPPER doesn't >> expect that. (o_e_p is called from warn_duplicated_cond_add_or_warn.) >> >> In the past we've adjusted o_e_p to better cope with template codes, >> but in this case I think we just want to avoid attempting to warn >> about inst-dependent expressions; I don't think I've ever envisioned >> -Wduplicated-cond to warn about them. >> >> The ICE started with r12-6022, two-stage name lookup for overloaded >> operators, which gave dependent operators a TREE_TYPE (in particular, >> DEPENDENT_OPERATOR_TYPE), so we no longer bail out here in o_e_p: >> >> /* Similar, if either does not have a type (like a template id), >> they aren't equal. */ >> if (!TREE_TYPE (arg0) || !TREE_TYPE (arg1)) >> return false; >> >> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? >> >> PR c++/107593 >> >> gcc/cp/ChangeLog: >> >> * parser.cc (cp_parser_selection_statement): Don't do >> -Wduplicated-cond when the condition is dependent. >> >> gcc/testsuite/ChangeLog: >> >> * g++.dg/warn/Wduplicated-cond3.C: New test. >> --- >> gcc/cp/parser.cc | 3 +- >> gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C | 38 +++++++++++++++++++ >> 2 files changed, 40 insertions(+), 1 deletion(-) >> create mode 100644 gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C >> >> diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc >> index 4cdc1cd472f..3df85d49e16 100644 >> --- a/gcc/cp/parser.cc >> +++ b/gcc/cp/parser.cc >> @@ -13209,7 +13209,8 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p, >> /* Add the condition. */ >> condition = finish_if_stmt_cond (condition, statement); >> >> - if (warn_duplicated_cond) >> + if (warn_duplicated_cond >> + && !instantiation_dependent_expression_p (condition)) >> warn_duplicated_cond_add_or_warn (token->location, condition, >> &chain); > > I noticed warn_duplicated_cond_add_or_warn already has logic to handle > TREE_SIDE_EFFECTS conditions by invaliding the entire chain. I wonder > if we'd want to do the same for instantiation-dep conditions? Makes sense. >> >> diff --git a/gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C b/gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C >> new file mode 100644 >> index 00000000000..3da054e5485 >> --- /dev/null >> +++ b/gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C >> @@ -0,0 +1,38 @@ >> +// PR c++/107593 >> +// { dg-do compile } >> +// { dg-options "-Wduplicated-cond" } >> + >> +template <typename T> >> +void >> +foo () >> +{ >> + if (T() && T() && int()) >> + ; >> + else if (T() && T() && int()) >> + ; >> +} >> + >> +template <typename T> >> +void bar(T a) >> +{ >> + if (a) >> + ; >> + else if (a) >> + ; >> +} >> + >> +template <typename> >> +void baz(int a) >> +{ >> + if (a) >> + ; >> + else if (a) // { dg-warning "duplicated" } >> + ; >> +} >> +void >> +f () >> +{ >> + foo<int>(); >> + bar(1); >> + baz<int>(1); >> +} >> >> base-commit: 94673a121cfc7f9d51c9d05e31795477f4dc8dc7 >> -- >> 2.39.1 >> >> > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++: fix ICE with -Wduplicated-cond [PR107593] 2023-01-27 22:15 ` Patrick Palka 2023-01-27 22:49 ` Jason Merrill @ 2023-01-27 22:56 ` Marek Polacek 2023-01-27 23:17 ` Patrick Palka 1 sibling, 1 reply; 10+ messages in thread From: Marek Polacek @ 2023-01-27 22:56 UTC (permalink / raw) To: Patrick Palka; +Cc: Jason Merrill, GCC Patches On Fri, Jan 27, 2023 at 05:15:00PM -0500, Patrick Palka wrote: > On Thu, 26 Jan 2023, Marek Polacek via Gcc-patches wrote: > > > Here we crash because a CAST_EXPR, representing T(), doesn't have > > its operand, and operand_equal_p's STRIP_ANY_LOCATION_WRAPPER doesn't > > expect that. (o_e_p is called from warn_duplicated_cond_add_or_warn.) > > > > In the past we've adjusted o_e_p to better cope with template codes, > > but in this case I think we just want to avoid attempting to warn > > about inst-dependent expressions; I don't think I've ever envisioned > > -Wduplicated-cond to warn about them. > > > > The ICE started with r12-6022, two-stage name lookup for overloaded > > operators, which gave dependent operators a TREE_TYPE (in particular, > > DEPENDENT_OPERATOR_TYPE), so we no longer bail out here in o_e_p: > > > > /* Similar, if either does not have a type (like a template id), > > they aren't equal. */ > > if (!TREE_TYPE (arg0) || !TREE_TYPE (arg1)) > > return false; > > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > > > PR c++/107593 > > > > gcc/cp/ChangeLog: > > > > * parser.cc (cp_parser_selection_statement): Don't do > > -Wduplicated-cond when the condition is dependent. > > > > gcc/testsuite/ChangeLog: > > > > * g++.dg/warn/Wduplicated-cond3.C: New test. > > --- > > gcc/cp/parser.cc | 3 +- > > gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C | 38 +++++++++++++++++++ > > 2 files changed, 40 insertions(+), 1 deletion(-) > > create mode 100644 gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C > > > > diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc > > index 4cdc1cd472f..3df85d49e16 100644 > > --- a/gcc/cp/parser.cc > > +++ b/gcc/cp/parser.cc > > @@ -13209,7 +13209,8 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p, > > /* Add the condition. */ > > condition = finish_if_stmt_cond (condition, statement); > > > > - if (warn_duplicated_cond) > > + if (warn_duplicated_cond > > + && !instantiation_dependent_expression_p (condition)) > > warn_duplicated_cond_add_or_warn (token->location, condition, > > &chain); > > I noticed warn_duplicated_cond_add_or_warn already has logic to handle > TREE_SIDE_EFFECTS conditions by invaliding the entire chain. I wonder > if we'd want to do the same for instantiation-dep conditions? warn_duplicated_cond_add_or_warn lives in c-family/c-warn.cc so I can't use instantiation_dependent_expression_p there. Sure, I could write a C++ wrapper but with my patch we just won't add CONDITION to the chain which I thought would work just as well. Marek ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++: fix ICE with -Wduplicated-cond [PR107593] 2023-01-27 22:56 ` Marek Polacek @ 2023-01-27 23:17 ` Patrick Palka 2023-01-27 23:18 ` Patrick Palka 2023-01-30 16:00 ` [PATCH v2] " Marek Polacek 0 siblings, 2 replies; 10+ messages in thread From: Patrick Palka @ 2023-01-27 23:17 UTC (permalink / raw) To: Marek Polacek; +Cc: Patrick Palka, Jason Merrill, GCC Patches On Fri, 27 Jan 2023, Marek Polacek wrote: > On Fri, Jan 27, 2023 at 05:15:00PM -0500, Patrick Palka wrote: > > On Thu, 26 Jan 2023, Marek Polacek via Gcc-patches wrote: > > > > > Here we crash because a CAST_EXPR, representing T(), doesn't have > > > its operand, and operand_equal_p's STRIP_ANY_LOCATION_WRAPPER doesn't > > > expect that. (o_e_p is called from warn_duplicated_cond_add_or_warn.) > > > > > > In the past we've adjusted o_e_p to better cope with template codes, > > > but in this case I think we just want to avoid attempting to warn > > > about inst-dependent expressions; I don't think I've ever envisioned > > > -Wduplicated-cond to warn about them. > > > > > > The ICE started with r12-6022, two-stage name lookup for overloaded > > > operators, which gave dependent operators a TREE_TYPE (in particular, > > > DEPENDENT_OPERATOR_TYPE), so we no longer bail out here in o_e_p: > > > > > > /* Similar, if either does not have a type (like a template id), > > > they aren't equal. */ > > > if (!TREE_TYPE (arg0) || !TREE_TYPE (arg1)) > > > return false; > > > > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > > > > > PR c++/107593 > > > > > > gcc/cp/ChangeLog: > > > > > > * parser.cc (cp_parser_selection_statement): Don't do > > > -Wduplicated-cond when the condition is dependent. > > > > > > gcc/testsuite/ChangeLog: > > > > > > * g++.dg/warn/Wduplicated-cond3.C: New test. > > > --- > > > gcc/cp/parser.cc | 3 +- > > > gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C | 38 +++++++++++++++++++ > > > 2 files changed, 40 insertions(+), 1 deletion(-) > > > create mode 100644 gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C > > > > > > diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc > > > index 4cdc1cd472f..3df85d49e16 100644 > > > --- a/gcc/cp/parser.cc > > > +++ b/gcc/cp/parser.cc > > > @@ -13209,7 +13209,8 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p, > > > /* Add the condition. */ > > > condition = finish_if_stmt_cond (condition, statement); > > > > > > - if (warn_duplicated_cond) > > > + if (warn_duplicated_cond > > > + && !instantiation_dependent_expression_p (condition)) > > > warn_duplicated_cond_add_or_warn (token->location, condition, > > > &chain); > > > > I noticed warn_duplicated_cond_add_or_warn already has logic to handle > > TREE_SIDE_EFFECTS conditions by invaliding the entire chain. I wonder > > if we'd want to do the same for instantiation-dep conditions? > > warn_duplicated_cond_add_or_warn lives in c-family/c-warn.cc so I can't > use instantiation_dependent_expression_p there. Sure, I could write a > C++ wrapper but with my patch we just won't add CONDITION to the chain > which I thought would work just as well. Ah that's unfortunate :( ISTM desirable to conservatively assume an inst-dep cond has side effects though (possibly directly from cp_parser_selection_statement), to avoid false positives as in: int n; template<class T> bool g() { n = 42; } template<class T> void f() { if (n) ; else if (g<T>()) ; else if (n) ; } ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++: fix ICE with -Wduplicated-cond [PR107593] 2023-01-27 23:17 ` Patrick Palka @ 2023-01-27 23:18 ` Patrick Palka 2023-01-30 16:00 ` [PATCH v2] " Marek Polacek 1 sibling, 0 replies; 10+ messages in thread From: Patrick Palka @ 2023-01-27 23:18 UTC (permalink / raw) To: Patrick Palka; +Cc: Marek Polacek, Jason Merrill, GCC Patches On Fri, 27 Jan 2023, Patrick Palka wrote: > On Fri, 27 Jan 2023, Marek Polacek wrote: > > > On Fri, Jan 27, 2023 at 05:15:00PM -0500, Patrick Palka wrote: > > > On Thu, 26 Jan 2023, Marek Polacek via Gcc-patches wrote: > > > > > > > Here we crash because a CAST_EXPR, representing T(), doesn't have > > > > its operand, and operand_equal_p's STRIP_ANY_LOCATION_WRAPPER doesn't > > > > expect that. (o_e_p is called from warn_duplicated_cond_add_or_warn.) > > > > > > > > In the past we've adjusted o_e_p to better cope with template codes, > > > > but in this case I think we just want to avoid attempting to warn > > > > about inst-dependent expressions; I don't think I've ever envisioned > > > > -Wduplicated-cond to warn about them. > > > > > > > > The ICE started with r12-6022, two-stage name lookup for overloaded > > > > operators, which gave dependent operators a TREE_TYPE (in particular, > > > > DEPENDENT_OPERATOR_TYPE), so we no longer bail out here in o_e_p: > > > > > > > > /* Similar, if either does not have a type (like a template id), > > > > they aren't equal. */ > > > > if (!TREE_TYPE (arg0) || !TREE_TYPE (arg1)) > > > > return false; > > > > > > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > > > > > > > PR c++/107593 > > > > > > > > gcc/cp/ChangeLog: > > > > > > > > * parser.cc (cp_parser_selection_statement): Don't do > > > > -Wduplicated-cond when the condition is dependent. > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > * g++.dg/warn/Wduplicated-cond3.C: New test. > > > > --- > > > > gcc/cp/parser.cc | 3 +- > > > > gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C | 38 +++++++++++++++++++ > > > > 2 files changed, 40 insertions(+), 1 deletion(-) > > > > create mode 100644 gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C > > > > > > > > diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc > > > > index 4cdc1cd472f..3df85d49e16 100644 > > > > --- a/gcc/cp/parser.cc > > > > +++ b/gcc/cp/parser.cc > > > > @@ -13209,7 +13209,8 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p, > > > > /* Add the condition. */ > > > > condition = finish_if_stmt_cond (condition, statement); > > > > > > > > - if (warn_duplicated_cond) > > > > + if (warn_duplicated_cond > > > > + && !instantiation_dependent_expression_p (condition)) > > > > warn_duplicated_cond_add_or_warn (token->location, condition, > > > > &chain); > > > > > > I noticed warn_duplicated_cond_add_or_warn already has logic to handle > > > TREE_SIDE_EFFECTS conditions by invaliding the entire chain. I wonder > > > if we'd want to do the same for instantiation-dep conditions? > > > > warn_duplicated_cond_add_or_warn lives in c-family/c-warn.cc so I can't > > use instantiation_dependent_expression_p there. Sure, I could write a > > C++ wrapper but with my patch we just won't add CONDITION to the chain > > which I thought would work just as well. > > Ah that's unfortunate :( ISTM desirable to conservatively assume an > inst-dep cond has side effects though (possibly directly from oops, "has side effects and clear the chain" rather > cp_parser_selection_statement), to avoid false positives as in: > > int n; > > template<class T> bool g() { n = 42; } > > template<class T> > void f() { > if (n) > ; > else if (g<T>()) > ; > else if (n) > ; > } > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] c++: fix ICE with -Wduplicated-cond [PR107593] 2023-01-27 23:17 ` Patrick Palka 2023-01-27 23:18 ` Patrick Palka @ 2023-01-30 16:00 ` Marek Polacek 2023-01-30 18:12 ` Jason Merrill 1 sibling, 1 reply; 10+ messages in thread From: Marek Polacek @ 2023-01-30 16:00 UTC (permalink / raw) To: Patrick Palka; +Cc: Jason Merrill, GCC Patches On Fri, Jan 27, 2023 at 06:17:00PM -0500, Patrick Palka wrote: > On Fri, 27 Jan 2023, Marek Polacek wrote: > > > On Fri, Jan 27, 2023 at 05:15:00PM -0500, Patrick Palka wrote: > > > On Thu, 26 Jan 2023, Marek Polacek via Gcc-patches wrote: > > > > > > > Here we crash because a CAST_EXPR, representing T(), doesn't have > > > > its operand, and operand_equal_p's STRIP_ANY_LOCATION_WRAPPER doesn't > > > > expect that. (o_e_p is called from warn_duplicated_cond_add_or_warn.) > > > > > > > > In the past we've adjusted o_e_p to better cope with template codes, > > > > but in this case I think we just want to avoid attempting to warn > > > > about inst-dependent expressions; I don't think I've ever envisioned > > > > -Wduplicated-cond to warn about them. > > > > > > > > The ICE started with r12-6022, two-stage name lookup for overloaded > > > > operators, which gave dependent operators a TREE_TYPE (in particular, > > > > DEPENDENT_OPERATOR_TYPE), so we no longer bail out here in o_e_p: > > > > > > > > /* Similar, if either does not have a type (like a template id), > > > > they aren't equal. */ > > > > if (!TREE_TYPE (arg0) || !TREE_TYPE (arg1)) > > > > return false; > > > > > > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > > > > > > > PR c++/107593 > > > > > > > > gcc/cp/ChangeLog: > > > > > > > > * parser.cc (cp_parser_selection_statement): Don't do > > > > -Wduplicated-cond when the condition is dependent. > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > * g++.dg/warn/Wduplicated-cond3.C: New test. > > > > --- > > > > gcc/cp/parser.cc | 3 +- > > > > gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C | 38 +++++++++++++++++++ > > > > 2 files changed, 40 insertions(+), 1 deletion(-) > > > > create mode 100644 gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C > > > > > > > > diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc > > > > index 4cdc1cd472f..3df85d49e16 100644 > > > > --- a/gcc/cp/parser.cc > > > > +++ b/gcc/cp/parser.cc > > > > @@ -13209,7 +13209,8 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p, > > > > /* Add the condition. */ > > > > condition = finish_if_stmt_cond (condition, statement); > > > > > > > > - if (warn_duplicated_cond) > > > > + if (warn_duplicated_cond > > > > + && !instantiation_dependent_expression_p (condition)) > > > > warn_duplicated_cond_add_or_warn (token->location, condition, > > > > &chain); > > > > > > I noticed warn_duplicated_cond_add_or_warn already has logic to handle > > > TREE_SIDE_EFFECTS conditions by invaliding the entire chain. I wonder > > > if we'd want to do the same for instantiation-dep conditions? > > > > warn_duplicated_cond_add_or_warn lives in c-family/c-warn.cc so I can't > > use instantiation_dependent_expression_p there. Sure, I could write a > > C++ wrapper but with my patch we just won't add CONDITION to the chain > > which I thought would work just as well. > > Ah that's unfortunate :( ISTM desirable to conservatively assume an > inst-dep cond has side effects though (possibly directly from > cp_parser_selection_statement), to avoid false positives as in: > > int n; > > template<class T> bool g() { n = 42; } > > template<class T> > void f() { > if (n) > ; > else if (g<T>()) > ; > else if (n) > ; > } You're right, we shouldn't warn there. So I've just added a new param with a default argument. A new PR was just opened for the same problem so I've added another test. Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? -- >8 -- Here we crash because a CAST_EXPR, representing T(), doesn't have its operand, and operand_equal_p's STRIP_ANY_LOCATION_WRAPPER doesn't expect that. (o_e_p is called from warn_duplicated_cond_add_or_warn.) In the past we've adjusted o_e_p to better cope with template codes, but in this case I think we just want to avoid attempting to warn about inst-dependent expressions; I don't think I've ever envisioned -Wduplicated-cond to warn about them. Also destroy the chain when an inst-dependent expression is encountered to not warn in Wduplicated-cond4.C. The ICE started with r12-6022, two-stage name lookup for overloaded operators, which gave dependent operators a TREE_TYPE (in particular, DEPENDENT_OPERATOR_TYPE), so we no longer bail out here in o_e_p: /* Similar, if either does not have a type (like a template id), they aren't equal. */ if (!TREE_TYPE (arg0) || !TREE_TYPE (arg1)) return false; PR c++/107593 PR c++/108597 gcc/c-family/ChangeLog: * c-common.h (warn_duplicated_cond_add_or_warn): New parameter. * c-warn.cc (warn_duplicated_cond_add_or_warn): Add new parameter. Use it. gcc/cp/ChangeLog: * parser.cc (cp_parser_selection_statement): Pass the result of instantiation_dependent_expression_p down to warn_duplicated_cond_add_or_warn. gcc/testsuite/ChangeLog: * g++.dg/warn/Wduplicated-cond3.C: New test. * g++.dg/warn/Wduplicated-cond4.C: New test. * g++.dg/warn/Wduplicated-cond5.C: New test. --- gcc/c-family/c-common.h | 3 +- gcc/c-family/c-warn.cc | 5 ++- gcc/cp/parser.cc | 8 +++- gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C | 38 +++++++++++++++++++ gcc/testsuite/g++.dg/warn/Wduplicated-cond4.C | 17 +++++++++ gcc/testsuite/g++.dg/warn/Wduplicated-cond5.C | 16 ++++++++ 6 files changed, 82 insertions(+), 5 deletions(-) create mode 100644 gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C create mode 100644 gcc/testsuite/g++.dg/warn/Wduplicated-cond4.C create mode 100644 gcc/testsuite/g++.dg/warn/Wduplicated-cond5.C diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h index bb6271d4a83..a9b47e15894 100644 --- a/gcc/c-family/c-common.h +++ b/gcc/c-family/c-common.h @@ -1514,7 +1514,8 @@ extern void maybe_record_typedef_use (tree); extern void maybe_warn_unused_local_typedefs (void); extern void maybe_warn_bool_compare (location_t, enum tree_code, tree, tree); extern bool maybe_warn_shift_overflow (location_t, tree, tree); -extern void warn_duplicated_cond_add_or_warn (location_t, tree, vec<tree> **); +extern void warn_duplicated_cond_add_or_warn (location_t, tree, vec<tree> **, + bool = false); extern bool diagnose_mismatched_attributes (tree, tree); extern tree do_warn_duplicated_branches_r (tree *, int *, void *); extern void warn_for_multistatement_macros (location_t, location_t, diff --git a/gcc/c-family/c-warn.cc b/gcc/c-family/c-warn.cc index 5ed7bcab16a..b7621656f89 100644 --- a/gcc/c-family/c-warn.cc +++ b/gcc/c-family/c-warn.cc @@ -2529,13 +2529,14 @@ maybe_warn_unused_local_typedefs (void) of COND. */ void -warn_duplicated_cond_add_or_warn (location_t loc, tree cond, vec<tree> **chain) +warn_duplicated_cond_add_or_warn (location_t loc, tree cond, vec<tree> **chain, + bool dependent_p /*=false*/) { /* No chain has been created yet. Do nothing. */ if (*chain == NULL) return; - if (TREE_SIDE_EFFECTS (cond)) + if (dependent_p || TREE_SIDE_EFFECTS (cond)) { /* Uh-oh! This condition has a side-effect, thus invalidates the whole chain. */ diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc index 4cdc1cd472f..d62db229518 100644 --- a/gcc/cp/parser.cc +++ b/gcc/cp/parser.cc @@ -13210,8 +13210,12 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p, condition = finish_if_stmt_cond (condition, statement); if (warn_duplicated_cond) - warn_duplicated_cond_add_or_warn (token->location, condition, - &chain); + { + const bool dep + = instantiation_dependent_expression_p (condition); + warn_duplicated_cond_add_or_warn (token->location, condition, + &chain, dep); + } /* Parse the then-clause. */ in_statement = parser->in_statement; diff --git a/gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C b/gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C new file mode 100644 index 00000000000..3da054e5485 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C @@ -0,0 +1,38 @@ +// PR c++/107593 +// { dg-do compile } +// { dg-options "-Wduplicated-cond" } + +template <typename T> +void +foo () +{ + if (T() && T() && int()) + ; + else if (T() && T() && int()) + ; +} + +template <typename T> +void bar(T a) +{ + if (a) + ; + else if (a) + ; +} + +template <typename> +void baz(int a) +{ + if (a) + ; + else if (a) // { dg-warning "duplicated" } + ; +} +void +f () +{ + foo<int>(); + bar(1); + baz<int>(1); +} diff --git a/gcc/testsuite/g++.dg/warn/Wduplicated-cond4.C b/gcc/testsuite/g++.dg/warn/Wduplicated-cond4.C new file mode 100644 index 00000000000..41bb9f09b4f --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wduplicated-cond4.C @@ -0,0 +1,17 @@ +// PR c++/107593 +// { dg-do compile } +// { dg-options "-Wduplicated-cond" } + +int n; + +template<class T> bool g() { n = 42; return false; } + +template<class T> +void f() { + if (n) + ; + else if (g<T>()) + ; + else if (n) + ; +} diff --git a/gcc/testsuite/g++.dg/warn/Wduplicated-cond5.C b/gcc/testsuite/g++.dg/warn/Wduplicated-cond5.C new file mode 100644 index 00000000000..23a0bf212b5 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wduplicated-cond5.C @@ -0,0 +1,16 @@ +// PR c++/108597 +// { dg-do compile } +// { dg-options "-Wduplicated-cond" } + +template <typename T> +struct MyStruct { + + void check(int &x) { + if (&x == &_a) { + } else if (&x == &_b) { + } + } + + int _a; + int _b; +}; base-commit: 5f8950b403f6351f125d8281d2e7430a43e7d125 -- 2.39.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] c++: fix ICE with -Wduplicated-cond [PR107593] 2023-01-30 16:00 ` [PATCH v2] " Marek Polacek @ 2023-01-30 18:12 ` Jason Merrill 2023-01-31 2:34 ` [PATCH v3] " Marek Polacek 0 siblings, 1 reply; 10+ messages in thread From: Jason Merrill @ 2023-01-30 18:12 UTC (permalink / raw) To: Marek Polacek, Patrick Palka; +Cc: GCC Patches On 1/30/23 11:00, Marek Polacek wrote: > On Fri, Jan 27, 2023 at 06:17:00PM -0500, Patrick Palka wrote: >> On Fri, 27 Jan 2023, Marek Polacek wrote: >> >>> On Fri, Jan 27, 2023 at 05:15:00PM -0500, Patrick Palka wrote: >>>> On Thu, 26 Jan 2023, Marek Polacek via Gcc-patches wrote: >>>> >>>>> Here we crash because a CAST_EXPR, representing T(), doesn't have >>>>> its operand, and operand_equal_p's STRIP_ANY_LOCATION_WRAPPER doesn't >>>>> expect that. (o_e_p is called from warn_duplicated_cond_add_or_warn.) >>>>> >>>>> In the past we've adjusted o_e_p to better cope with template codes, >>>>> but in this case I think we just want to avoid attempting to warn >>>>> about inst-dependent expressions; I don't think I've ever envisioned >>>>> -Wduplicated-cond to warn about them. >>>>> >>>>> The ICE started with r12-6022, two-stage name lookup for overloaded >>>>> operators, which gave dependent operators a TREE_TYPE (in particular, >>>>> DEPENDENT_OPERATOR_TYPE), so we no longer bail out here in o_e_p: >>>>> >>>>> /* Similar, if either does not have a type (like a template id), >>>>> they aren't equal. */ >>>>> if (!TREE_TYPE (arg0) || !TREE_TYPE (arg1)) >>>>> return false; >>>>> >>>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? >>>>> >>>>> PR c++/107593 >>>>> >>>>> gcc/cp/ChangeLog: >>>>> >>>>> * parser.cc (cp_parser_selection_statement): Don't do >>>>> -Wduplicated-cond when the condition is dependent. >>>>> >>>>> gcc/testsuite/ChangeLog: >>>>> >>>>> * g++.dg/warn/Wduplicated-cond3.C: New test. >>>>> --- >>>>> gcc/cp/parser.cc | 3 +- >>>>> gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C | 38 +++++++++++++++++++ >>>>> 2 files changed, 40 insertions(+), 1 deletion(-) >>>>> create mode 100644 gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C >>>>> >>>>> diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc >>>>> index 4cdc1cd472f..3df85d49e16 100644 >>>>> --- a/gcc/cp/parser.cc >>>>> +++ b/gcc/cp/parser.cc >>>>> @@ -13209,7 +13209,8 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p, >>>>> /* Add the condition. */ >>>>> condition = finish_if_stmt_cond (condition, statement); >>>>> >>>>> - if (warn_duplicated_cond) >>>>> + if (warn_duplicated_cond >>>>> + && !instantiation_dependent_expression_p (condition)) >>>>> warn_duplicated_cond_add_or_warn (token->location, condition, >>>>> &chain); >>>> >>>> I noticed warn_duplicated_cond_add_or_warn already has logic to handle >>>> TREE_SIDE_EFFECTS conditions by invaliding the entire chain. I wonder >>>> if we'd want to do the same for instantiation-dep conditions? >>> >>> warn_duplicated_cond_add_or_warn lives in c-family/c-warn.cc so I can't >>> use instantiation_dependent_expression_p there. Sure, I could write a >>> C++ wrapper but with my patch we just won't add CONDITION to the chain >>> which I thought would work just as well. Or maybe define instantiation_dependent_expression_p in the C front-end to just return false? >> Ah that's unfortunate :( ISTM desirable to conservatively assume an >> inst-dep cond has side effects though (possibly directly from >> cp_parser_selection_statement), to avoid false positives as in: >> >> int n; >> >> template<class T> bool g() { n = 42; } >> >> template<class T> >> void f() { >> if (n) >> ; >> else if (g<T>()) >> ; >> else if (n) >> ; >> } > > You're right, we shouldn't warn there. So I've just added a new param > with a default argument. A new PR was just opened for the same problem > so I've added another test. > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > -- >8 -- > Here we crash because a CAST_EXPR, representing T(), doesn't have > its operand, and operand_equal_p's STRIP_ANY_LOCATION_WRAPPER doesn't > expect that. (o_e_p is called from warn_duplicated_cond_add_or_warn.) > > In the past we've adjusted o_e_p to better cope with template codes, > but in this case I think we just want to avoid attempting to warn > about inst-dependent expressions; I don't think I've ever envisioned > -Wduplicated-cond to warn about them. Also destroy the chain when > an inst-dependent expression is encountered to not warn in > Wduplicated-cond4.C. > > The ICE started with r12-6022, two-stage name lookup for overloaded > operators, which gave dependent operators a TREE_TYPE (in particular, > DEPENDENT_OPERATOR_TYPE), so we no longer bail out here in o_e_p: > > /* Similar, if either does not have a type (like a template id), > they aren't equal. */ > if (!TREE_TYPE (arg0) || !TREE_TYPE (arg1)) > return false; > > PR c++/107593 > PR c++/108597 > > gcc/c-family/ChangeLog: > > * c-common.h (warn_duplicated_cond_add_or_warn): New parameter. > * c-warn.cc (warn_duplicated_cond_add_or_warn): Add new parameter. > Use it. > > gcc/cp/ChangeLog: > > * parser.cc (cp_parser_selection_statement): Pass the result of > instantiation_dependent_expression_p down to > warn_duplicated_cond_add_or_warn. > > gcc/testsuite/ChangeLog: > > * g++.dg/warn/Wduplicated-cond3.C: New test. > * g++.dg/warn/Wduplicated-cond4.C: New test. > * g++.dg/warn/Wduplicated-cond5.C: New test. > --- > gcc/c-family/c-common.h | 3 +- > gcc/c-family/c-warn.cc | 5 ++- > gcc/cp/parser.cc | 8 +++- > gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C | 38 +++++++++++++++++++ > gcc/testsuite/g++.dg/warn/Wduplicated-cond4.C | 17 +++++++++ > gcc/testsuite/g++.dg/warn/Wduplicated-cond5.C | 16 ++++++++ > 6 files changed, 82 insertions(+), 5 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C > create mode 100644 gcc/testsuite/g++.dg/warn/Wduplicated-cond4.C > create mode 100644 gcc/testsuite/g++.dg/warn/Wduplicated-cond5.C > > diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h > index bb6271d4a83..a9b47e15894 100644 > --- a/gcc/c-family/c-common.h > +++ b/gcc/c-family/c-common.h > @@ -1514,7 +1514,8 @@ extern void maybe_record_typedef_use (tree); > extern void maybe_warn_unused_local_typedefs (void); > extern void maybe_warn_bool_compare (location_t, enum tree_code, tree, tree); > extern bool maybe_warn_shift_overflow (location_t, tree, tree); > -extern void warn_duplicated_cond_add_or_warn (location_t, tree, vec<tree> **); > +extern void warn_duplicated_cond_add_or_warn (location_t, tree, vec<tree> **, > + bool = false); > extern bool diagnose_mismatched_attributes (tree, tree); > extern tree do_warn_duplicated_branches_r (tree *, int *, void *); > extern void warn_for_multistatement_macros (location_t, location_t, > diff --git a/gcc/c-family/c-warn.cc b/gcc/c-family/c-warn.cc > index 5ed7bcab16a..b7621656f89 100644 > --- a/gcc/c-family/c-warn.cc > +++ b/gcc/c-family/c-warn.cc > @@ -2529,13 +2529,14 @@ maybe_warn_unused_local_typedefs (void) > of COND. */ > > void > -warn_duplicated_cond_add_or_warn (location_t loc, tree cond, vec<tree> **chain) > +warn_duplicated_cond_add_or_warn (location_t loc, tree cond, vec<tree> **chain, > + bool dependent_p /*=false*/) > { > /* No chain has been created yet. Do nothing. */ > if (*chain == NULL) > return; > > - if (TREE_SIDE_EFFECTS (cond)) > + if (dependent_p || TREE_SIDE_EFFECTS (cond)) > { > /* Uh-oh! This condition has a side-effect, thus invalidates > the whole chain. */ > diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc > index 4cdc1cd472f..d62db229518 100644 > --- a/gcc/cp/parser.cc > +++ b/gcc/cp/parser.cc > @@ -13210,8 +13210,12 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p, > condition = finish_if_stmt_cond (condition, statement); > > if (warn_duplicated_cond) > - warn_duplicated_cond_add_or_warn (token->location, condition, > - &chain); > + { > + const bool dep > + = instantiation_dependent_expression_p (condition); > + warn_duplicated_cond_add_or_warn (token->location, condition, > + &chain, dep); > + } > > /* Parse the then-clause. */ > in_statement = parser->in_statement; > diff --git a/gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C b/gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C > new file mode 100644 > index 00000000000..3da054e5485 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C > @@ -0,0 +1,38 @@ > +// PR c++/107593 > +// { dg-do compile } > +// { dg-options "-Wduplicated-cond" } > + > +template <typename T> > +void > +foo () > +{ > + if (T() && T() && int()) > + ; > + else if (T() && T() && int()) > + ; > +} > + > +template <typename T> > +void bar(T a) > +{ > + if (a) > + ; > + else if (a) > + ; > +} > + > +template <typename> > +void baz(int a) > +{ > + if (a) > + ; > + else if (a) // { dg-warning "duplicated" } > + ; > +} > +void > +f () > +{ > + foo<int>(); > + bar(1); > + baz<int>(1); > +} > diff --git a/gcc/testsuite/g++.dg/warn/Wduplicated-cond4.C b/gcc/testsuite/g++.dg/warn/Wduplicated-cond4.C > new file mode 100644 > index 00000000000..41bb9f09b4f > --- /dev/null > +++ b/gcc/testsuite/g++.dg/warn/Wduplicated-cond4.C > @@ -0,0 +1,17 @@ > +// PR c++/107593 > +// { dg-do compile } > +// { dg-options "-Wduplicated-cond" } > + > +int n; > + > +template<class T> bool g() { n = 42; return false; } > + > +template<class T> > +void f() { > + if (n) > + ; > + else if (g<T>()) > + ; > + else if (n) > + ; > +} > diff --git a/gcc/testsuite/g++.dg/warn/Wduplicated-cond5.C b/gcc/testsuite/g++.dg/warn/Wduplicated-cond5.C > new file mode 100644 > index 00000000000..23a0bf212b5 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/warn/Wduplicated-cond5.C > @@ -0,0 +1,16 @@ > +// PR c++/108597 > +// { dg-do compile } > +// { dg-options "-Wduplicated-cond" } > + > +template <typename T> > +struct MyStruct { > + > + void check(int &x) { > + if (&x == &_a) { > + } else if (&x == &_b) { > + } > + } > + > + int _a; > + int _b; > +}; > > base-commit: 5f8950b403f6351f125d8281d2e7430a43e7d125 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3] c++: fix ICE with -Wduplicated-cond [PR107593] 2023-01-30 18:12 ` Jason Merrill @ 2023-01-31 2:34 ` Marek Polacek 2023-01-31 16:30 ` Jason Merrill 0 siblings, 1 reply; 10+ messages in thread From: Marek Polacek @ 2023-01-31 2:34 UTC (permalink / raw) To: Jason Merrill; +Cc: Patrick Palka, GCC Patches On Mon, Jan 30, 2023 at 01:12:00PM -0500, Jason Merrill wrote: > On 1/30/23 11:00, Marek Polacek wrote: > > On Fri, Jan 27, 2023 at 06:17:00PM -0500, Patrick Palka wrote: > > > On Fri, 27 Jan 2023, Marek Polacek wrote: > > > > > > > On Fri, Jan 27, 2023 at 05:15:00PM -0500, Patrick Palka wrote: > > > > > On Thu, 26 Jan 2023, Marek Polacek via Gcc-patches wrote: > > > > > > > > > > > Here we crash because a CAST_EXPR, representing T(), doesn't have > > > > > > its operand, and operand_equal_p's STRIP_ANY_LOCATION_WRAPPER doesn't > > > > > > expect that. (o_e_p is called from warn_duplicated_cond_add_or_warn.) > > > > > > > > > > > > In the past we've adjusted o_e_p to better cope with template codes, > > > > > > but in this case I think we just want to avoid attempting to warn > > > > > > about inst-dependent expressions; I don't think I've ever envisioned > > > > > > -Wduplicated-cond to warn about them. > > > > > > > > > > > > The ICE started with r12-6022, two-stage name lookup for overloaded > > > > > > operators, which gave dependent operators a TREE_TYPE (in particular, > > > > > > DEPENDENT_OPERATOR_TYPE), so we no longer bail out here in o_e_p: > > > > > > > > > > > > /* Similar, if either does not have a type (like a template id), > > > > > > they aren't equal. */ > > > > > > if (!TREE_TYPE (arg0) || !TREE_TYPE (arg1)) > > > > > > return false; > > > > > > > > > > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > > > > > > > > > > > PR c++/107593 > > > > > > > > > > > > gcc/cp/ChangeLog: > > > > > > > > > > > > * parser.cc (cp_parser_selection_statement): Don't do > > > > > > -Wduplicated-cond when the condition is dependent. > > > > > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > > > > > * g++.dg/warn/Wduplicated-cond3.C: New test. > > > > > > --- > > > > > > gcc/cp/parser.cc | 3 +- > > > > > > gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C | 38 +++++++++++++++++++ > > > > > > 2 files changed, 40 insertions(+), 1 deletion(-) > > > > > > create mode 100644 gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C > > > > > > > > > > > > diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc > > > > > > index 4cdc1cd472f..3df85d49e16 100644 > > > > > > --- a/gcc/cp/parser.cc > > > > > > +++ b/gcc/cp/parser.cc > > > > > > @@ -13209,7 +13209,8 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p, > > > > > > /* Add the condition. */ > > > > > > condition = finish_if_stmt_cond (condition, statement); > > > > > > - if (warn_duplicated_cond) > > > > > > + if (warn_duplicated_cond > > > > > > + && !instantiation_dependent_expression_p (condition)) > > > > > > warn_duplicated_cond_add_or_warn (token->location, condition, > > > > > > &chain); > > > > > > > > > > I noticed warn_duplicated_cond_add_or_warn already has logic to handle > > > > > TREE_SIDE_EFFECTS conditions by invaliding the entire chain. I wonder > > > > > if we'd want to do the same for instantiation-dep conditions? > > > > > > > > warn_duplicated_cond_add_or_warn lives in c-family/c-warn.cc so I can't > > > > use instantiation_dependent_expression_p there. Sure, I could write a > > > > C++ wrapper but with my patch we just won't add CONDITION to the chain > > > > which I thought would work just as well. > > Or maybe define instantiation_dependent_expression_p in the C front-end to > just return false? Like this? Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? -- >8 -- Here we crash because a CAST_EXPR, representing T(), doesn't have its operand, and operand_equal_p's STRIP_ANY_LOCATION_WRAPPER doesn't expect that. (o_e_p is called from warn_duplicated_cond_add_or_warn.) In the past we've adjusted o_e_p to better cope with template codes, but in this case I think we just want to avoid attempting to warn about inst-dependent expressions; I don't think I've ever envisioned -Wduplicated-cond to warn about them. Also destroy the chain when an inst-dependent expression is encountered to not warn in Wduplicated-cond4.C. The ICE started with r12-6022, two-stage name lookup for overloaded operators, which gave dependent operators a TREE_TYPE (in particular, DEPENDENT_OPERATOR_TYPE), so we no longer bail out here in o_e_p: /* Similar, if either does not have a type (like a template id), they aren't equal. */ if (!TREE_TYPE (arg0) || !TREE_TYPE (arg1)) return false; PR c++/107593 PR c++/108597 gcc/c-family/ChangeLog: * c-common.h (instantiation_dependent_expression_p): Declare. * c-warn.cc (warn_duplicated_cond_add_or_warn): If the condition is dependent, invalidate the chain. gcc/c/ChangeLog: * c-objc-common.cc (instantiation_dependent_expression_p): New. gcc/cp/ChangeLog: * cp-tree.h (instantiation_dependent_expression_p): Don't declare here. gcc/testsuite/ChangeLog: * g++.dg/warn/Wduplicated-cond3.C: New test. * g++.dg/warn/Wduplicated-cond4.C: New test. * g++.dg/warn/Wduplicated-cond5.C: New test. --- gcc/c-family/c-common.h | 1 + gcc/c-family/c-warn.cc | 2 +- gcc/c/c-objc-common.cc | 8 ++++ gcc/cp/cp-tree.h | 1 - gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C | 38 +++++++++++++++++++ gcc/testsuite/g++.dg/warn/Wduplicated-cond4.C | 17 +++++++++ gcc/testsuite/g++.dg/warn/Wduplicated-cond5.C | 16 ++++++++ 7 files changed, 81 insertions(+), 2 deletions(-) create mode 100644 gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C create mode 100644 gcc/testsuite/g++.dg/warn/Wduplicated-cond4.C create mode 100644 gcc/testsuite/g++.dg/warn/Wduplicated-cond5.C diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h index bb6271d4a83..2dadb33476c 100644 --- a/gcc/c-family/c-common.h +++ b/gcc/c-family/c-common.h @@ -1101,6 +1101,7 @@ extern tree lookup_label (tree); extern tree lookup_name (tree); extern bool lvalue_p (const_tree); extern int maybe_adjust_arg_pos_for_attribute (const_tree); +extern bool instantiation_dependent_expression_p (tree); extern bool vector_targets_convertible_p (const_tree t1, const_tree t2); extern bool vector_types_convertible_p (const_tree t1, const_tree t2, bool emit_lax_note); diff --git a/gcc/c-family/c-warn.cc b/gcc/c-family/c-warn.cc index 5ed7bcab16a..29efce3f2c0 100644 --- a/gcc/c-family/c-warn.cc +++ b/gcc/c-family/c-warn.cc @@ -2535,7 +2535,7 @@ warn_duplicated_cond_add_or_warn (location_t loc, tree cond, vec<tree> **chain) if (*chain == NULL) return; - if (TREE_SIDE_EFFECTS (cond)) + if (TREE_SIDE_EFFECTS (cond) || instantiation_dependent_expression_p (cond)) { /* Uh-oh! This condition has a side-effect, thus invalidates the whole chain. */ diff --git a/gcc/c/c-objc-common.cc b/gcc/c/c-objc-common.cc index d4f0b262ebb..0350733250b 100644 --- a/gcc/c/c-objc-common.cc +++ b/gcc/c/c-objc-common.cc @@ -400,3 +400,11 @@ maybe_adjust_arg_pos_for_attribute (const_tree) { return 0; } + +/* In C, no expression is dependent. */ + +bool +instantiation_dependent_expression_p (tree) +{ + return false; +} diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 9f188724cf2..31fd8af4f21 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -7428,7 +7428,6 @@ extern bool any_type_dependent_arguments_p (const vec<tree, va_gc> *); extern bool any_type_dependent_elements_p (const_tree); extern bool type_dependent_expression_p_push (tree); extern bool value_dependent_expression_p (tree); -extern bool instantiation_dependent_expression_p (tree); extern bool instantiation_dependent_uneval_expression_p (tree); extern bool any_value_dependent_elements_p (const_tree); extern bool dependent_omp_for_p (tree, tree, tree, tree); diff --git a/gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C b/gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C new file mode 100644 index 00000000000..3da054e5485 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C @@ -0,0 +1,38 @@ +// PR c++/107593 +// { dg-do compile } +// { dg-options "-Wduplicated-cond" } + +template <typename T> +void +foo () +{ + if (T() && T() && int()) + ; + else if (T() && T() && int()) + ; +} + +template <typename T> +void bar(T a) +{ + if (a) + ; + else if (a) + ; +} + +template <typename> +void baz(int a) +{ + if (a) + ; + else if (a) // { dg-warning "duplicated" } + ; +} +void +f () +{ + foo<int>(); + bar(1); + baz<int>(1); +} diff --git a/gcc/testsuite/g++.dg/warn/Wduplicated-cond4.C b/gcc/testsuite/g++.dg/warn/Wduplicated-cond4.C new file mode 100644 index 00000000000..41bb9f09b4f --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wduplicated-cond4.C @@ -0,0 +1,17 @@ +// PR c++/107593 +// { dg-do compile } +// { dg-options "-Wduplicated-cond" } + +int n; + +template<class T> bool g() { n = 42; return false; } + +template<class T> +void f() { + if (n) + ; + else if (g<T>()) + ; + else if (n) + ; +} diff --git a/gcc/testsuite/g++.dg/warn/Wduplicated-cond5.C b/gcc/testsuite/g++.dg/warn/Wduplicated-cond5.C new file mode 100644 index 00000000000..23a0bf212b5 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wduplicated-cond5.C @@ -0,0 +1,16 @@ +// PR c++/108597 +// { dg-do compile } +// { dg-options "-Wduplicated-cond" } + +template <typename T> +struct MyStruct { + + void check(int &x) { + if (&x == &_a) { + } else if (&x == &_b) { + } + } + + int _a; + int _b; +}; base-commit: 897a0502056e6cc6613f26e0b22d1c1e06b1490f -- 2.39.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] c++: fix ICE with -Wduplicated-cond [PR107593] 2023-01-31 2:34 ` [PATCH v3] " Marek Polacek @ 2023-01-31 16:30 ` Jason Merrill 0 siblings, 0 replies; 10+ messages in thread From: Jason Merrill @ 2023-01-31 16:30 UTC (permalink / raw) To: Marek Polacek; +Cc: Patrick Palka, GCC Patches On 1/30/23 21:34, Marek Polacek wrote: > On Mon, Jan 30, 2023 at 01:12:00PM -0500, Jason Merrill wrote: >> On 1/30/23 11:00, Marek Polacek wrote: >>> On Fri, Jan 27, 2023 at 06:17:00PM -0500, Patrick Palka wrote: >>>> On Fri, 27 Jan 2023, Marek Polacek wrote: >>>> >>>>> On Fri, Jan 27, 2023 at 05:15:00PM -0500, Patrick Palka wrote: >>>>>> On Thu, 26 Jan 2023, Marek Polacek via Gcc-patches wrote: >>>>>> >>>>>>> Here we crash because a CAST_EXPR, representing T(), doesn't have >>>>>>> its operand, and operand_equal_p's STRIP_ANY_LOCATION_WRAPPER doesn't >>>>>>> expect that. (o_e_p is called from warn_duplicated_cond_add_or_warn.) >>>>>>> >>>>>>> In the past we've adjusted o_e_p to better cope with template codes, >>>>>>> but in this case I think we just want to avoid attempting to warn >>>>>>> about inst-dependent expressions; I don't think I've ever envisioned >>>>>>> -Wduplicated-cond to warn about them. >>>>>>> >>>>>>> The ICE started with r12-6022, two-stage name lookup for overloaded >>>>>>> operators, which gave dependent operators a TREE_TYPE (in particular, >>>>>>> DEPENDENT_OPERATOR_TYPE), so we no longer bail out here in o_e_p: >>>>>>> >>>>>>> /* Similar, if either does not have a type (like a template id), >>>>>>> they aren't equal. */ >>>>>>> if (!TREE_TYPE (arg0) || !TREE_TYPE (arg1)) >>>>>>> return false; >>>>>>> >>>>>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? >>>>>>> >>>>>>> PR c++/107593 >>>>>>> >>>>>>> gcc/cp/ChangeLog: >>>>>>> >>>>>>> * parser.cc (cp_parser_selection_statement): Don't do >>>>>>> -Wduplicated-cond when the condition is dependent. >>>>>>> >>>>>>> gcc/testsuite/ChangeLog: >>>>>>> >>>>>>> * g++.dg/warn/Wduplicated-cond3.C: New test. >>>>>>> --- >>>>>>> gcc/cp/parser.cc | 3 +- >>>>>>> gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C | 38 +++++++++++++++++++ >>>>>>> 2 files changed, 40 insertions(+), 1 deletion(-) >>>>>>> create mode 100644 gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C >>>>>>> >>>>>>> diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc >>>>>>> index 4cdc1cd472f..3df85d49e16 100644 >>>>>>> --- a/gcc/cp/parser.cc >>>>>>> +++ b/gcc/cp/parser.cc >>>>>>> @@ -13209,7 +13209,8 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p, >>>>>>> /* Add the condition. */ >>>>>>> condition = finish_if_stmt_cond (condition, statement); >>>>>>> - if (warn_duplicated_cond) >>>>>>> + if (warn_duplicated_cond >>>>>>> + && !instantiation_dependent_expression_p (condition)) >>>>>>> warn_duplicated_cond_add_or_warn (token->location, condition, >>>>>>> &chain); >>>>>> >>>>>> I noticed warn_duplicated_cond_add_or_warn already has logic to handle >>>>>> TREE_SIDE_EFFECTS conditions by invaliding the entire chain. I wonder >>>>>> if we'd want to do the same for instantiation-dep conditions? >>>>> >>>>> warn_duplicated_cond_add_or_warn lives in c-family/c-warn.cc so I can't >>>>> use instantiation_dependent_expression_p there. Sure, I could write a >>>>> C++ wrapper but with my patch we just won't add CONDITION to the chain >>>>> which I thought would work just as well. >> >> Or maybe define instantiation_dependent_expression_p in the C front-end to >> just return false? > > Like this? > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? OK, thanks. > -- >8 -- > Here we crash because a CAST_EXPR, representing T(), doesn't have > its operand, and operand_equal_p's STRIP_ANY_LOCATION_WRAPPER doesn't > expect that. (o_e_p is called from warn_duplicated_cond_add_or_warn.) > > In the past we've adjusted o_e_p to better cope with template codes, > but in this case I think we just want to avoid attempting to warn > about inst-dependent expressions; I don't think I've ever envisioned > -Wduplicated-cond to warn about them. Also destroy the chain when > an inst-dependent expression is encountered to not warn in > Wduplicated-cond4.C. > > The ICE started with r12-6022, two-stage name lookup for overloaded > operators, which gave dependent operators a TREE_TYPE (in particular, > DEPENDENT_OPERATOR_TYPE), so we no longer bail out here in o_e_p: > > /* Similar, if either does not have a type (like a template id), > they aren't equal. */ > if (!TREE_TYPE (arg0) || !TREE_TYPE (arg1)) > return false; > > PR c++/107593 > PR c++/108597 > > gcc/c-family/ChangeLog: > > * c-common.h (instantiation_dependent_expression_p): Declare. > * c-warn.cc (warn_duplicated_cond_add_or_warn): If the condition > is dependent, invalidate the chain. > > gcc/c/ChangeLog: > > * c-objc-common.cc (instantiation_dependent_expression_p): New. > > gcc/cp/ChangeLog: > > * cp-tree.h (instantiation_dependent_expression_p): Don't > declare here. > > gcc/testsuite/ChangeLog: > > * g++.dg/warn/Wduplicated-cond3.C: New test. > * g++.dg/warn/Wduplicated-cond4.C: New test. > * g++.dg/warn/Wduplicated-cond5.C: New test. > --- > gcc/c-family/c-common.h | 1 + > gcc/c-family/c-warn.cc | 2 +- > gcc/c/c-objc-common.cc | 8 ++++ > gcc/cp/cp-tree.h | 1 - > gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C | 38 +++++++++++++++++++ > gcc/testsuite/g++.dg/warn/Wduplicated-cond4.C | 17 +++++++++ > gcc/testsuite/g++.dg/warn/Wduplicated-cond5.C | 16 ++++++++ > 7 files changed, 81 insertions(+), 2 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C > create mode 100644 gcc/testsuite/g++.dg/warn/Wduplicated-cond4.C > create mode 100644 gcc/testsuite/g++.dg/warn/Wduplicated-cond5.C > > diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h > index bb6271d4a83..2dadb33476c 100644 > --- a/gcc/c-family/c-common.h > +++ b/gcc/c-family/c-common.h > @@ -1101,6 +1101,7 @@ extern tree lookup_label (tree); > extern tree lookup_name (tree); > extern bool lvalue_p (const_tree); > extern int maybe_adjust_arg_pos_for_attribute (const_tree); > +extern bool instantiation_dependent_expression_p (tree); > > extern bool vector_targets_convertible_p (const_tree t1, const_tree t2); > extern bool vector_types_convertible_p (const_tree t1, const_tree t2, bool emit_lax_note); > diff --git a/gcc/c-family/c-warn.cc b/gcc/c-family/c-warn.cc > index 5ed7bcab16a..29efce3f2c0 100644 > --- a/gcc/c-family/c-warn.cc > +++ b/gcc/c-family/c-warn.cc > @@ -2535,7 +2535,7 @@ warn_duplicated_cond_add_or_warn (location_t loc, tree cond, vec<tree> **chain) > if (*chain == NULL) > return; > > - if (TREE_SIDE_EFFECTS (cond)) > + if (TREE_SIDE_EFFECTS (cond) || instantiation_dependent_expression_p (cond)) > { > /* Uh-oh! This condition has a side-effect, thus invalidates > the whole chain. */ > diff --git a/gcc/c/c-objc-common.cc b/gcc/c/c-objc-common.cc > index d4f0b262ebb..0350733250b 100644 > --- a/gcc/c/c-objc-common.cc > +++ b/gcc/c/c-objc-common.cc > @@ -400,3 +400,11 @@ maybe_adjust_arg_pos_for_attribute (const_tree) > { > return 0; > } > + > +/* In C, no expression is dependent. */ > + > +bool > +instantiation_dependent_expression_p (tree) > +{ > + return false; > +} > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > index 9f188724cf2..31fd8af4f21 100644 > --- a/gcc/cp/cp-tree.h > +++ b/gcc/cp/cp-tree.h > @@ -7428,7 +7428,6 @@ extern bool any_type_dependent_arguments_p (const vec<tree, va_gc> *); > extern bool any_type_dependent_elements_p (const_tree); > extern bool type_dependent_expression_p_push (tree); > extern bool value_dependent_expression_p (tree); > -extern bool instantiation_dependent_expression_p (tree); > extern bool instantiation_dependent_uneval_expression_p (tree); > extern bool any_value_dependent_elements_p (const_tree); > extern bool dependent_omp_for_p (tree, tree, tree, tree); > diff --git a/gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C b/gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C > new file mode 100644 > index 00000000000..3da054e5485 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/warn/Wduplicated-cond3.C > @@ -0,0 +1,38 @@ > +// PR c++/107593 > +// { dg-do compile } > +// { dg-options "-Wduplicated-cond" } > + > +template <typename T> > +void > +foo () > +{ > + if (T() && T() && int()) > + ; > + else if (T() && T() && int()) > + ; > +} > + > +template <typename T> > +void bar(T a) > +{ > + if (a) > + ; > + else if (a) > + ; > +} > + > +template <typename> > +void baz(int a) > +{ > + if (a) > + ; > + else if (a) // { dg-warning "duplicated" } > + ; > +} > +void > +f () > +{ > + foo<int>(); > + bar(1); > + baz<int>(1); > +} > diff --git a/gcc/testsuite/g++.dg/warn/Wduplicated-cond4.C b/gcc/testsuite/g++.dg/warn/Wduplicated-cond4.C > new file mode 100644 > index 00000000000..41bb9f09b4f > --- /dev/null > +++ b/gcc/testsuite/g++.dg/warn/Wduplicated-cond4.C > @@ -0,0 +1,17 @@ > +// PR c++/107593 > +// { dg-do compile } > +// { dg-options "-Wduplicated-cond" } > + > +int n; > + > +template<class T> bool g() { n = 42; return false; } > + > +template<class T> > +void f() { > + if (n) > + ; > + else if (g<T>()) > + ; > + else if (n) > + ; > +} > diff --git a/gcc/testsuite/g++.dg/warn/Wduplicated-cond5.C b/gcc/testsuite/g++.dg/warn/Wduplicated-cond5.C > new file mode 100644 > index 00000000000..23a0bf212b5 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/warn/Wduplicated-cond5.C > @@ -0,0 +1,16 @@ > +// PR c++/108597 > +// { dg-do compile } > +// { dg-options "-Wduplicated-cond" } > + > +template <typename T> > +struct MyStruct { > + > + void check(int &x) { > + if (&x == &_a) { > + } else if (&x == &_b) { > + } > + } > + > + int _a; > + int _b; > +}; > > base-commit: 897a0502056e6cc6613f26e0b22d1c1e06b1490f ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-01-31 16:30 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-01-26 22:17 [PATCH] c++: fix ICE with -Wduplicated-cond [PR107593] Marek Polacek 2023-01-27 22:15 ` Patrick Palka 2023-01-27 22:49 ` Jason Merrill 2023-01-27 22:56 ` Marek Polacek 2023-01-27 23:17 ` Patrick Palka 2023-01-27 23:18 ` Patrick Palka 2023-01-30 16:00 ` [PATCH v2] " Marek Polacek 2023-01-30 18:12 ` Jason Merrill 2023-01-31 2:34 ` [PATCH v3] " Marek Polacek 2023-01-31 16:30 ` Jason Merrill
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).