Re: [PATCH] c++: fix ICE with -Wduplicated-cond [PR107593]
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, > > > > ); > > > > > > 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 bool g() { n = 42; } > > template > void f() { > if (n) > ; > else if (g()) > ; > else if (n) > ; > } >
Re: [PATCH] c++: fix ICE with -Wduplicated-cond [PR107593]
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, > > > ); > > > > 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 bool g() { n = 42; } template void f() { if (n) ; else if (g()) ; else if (n) ; }
Re: [PATCH] c++: fix ICE with -Wduplicated-cond [PR107593]
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, > > ); > > 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
Re: [PATCH] c++: fix ICE with -Wduplicated-cond [PR107593]
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, ); 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 000..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 +void +foo () +{ + if (T() && T() && int()) +; + else if (T() && T() && int()) +; +} + +template +void bar(T a) +{ + if (a) +; + else if (a) +; +} + +template +void baz(int a) +{ + if (a) +; + else if (a) // { dg-warning "duplicated" } +; +} +void +f () +{ + foo(); + bar(1); + baz(1); +} base-commit: 94673a121cfc7f9d51c9d05e31795477f4dc8dc7 -- 2.39.1
Re: [PATCH] c++: fix ICE with -Wduplicated-cond [PR107593]
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, > ); 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 000..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 > +void > +foo () > +{ > + if (T() && T() && int()) > +; > + else if (T() && T() && int()) > +; > +} > + > +template > +void bar(T a) > +{ > + if (a) > +; > + else if (a) > +; > +} > + > +template > +void baz(int a) > +{ > + if (a) > +; > + else if (a) // { dg-warning "duplicated" } > +; > +} > +void > +f () > +{ > + foo(); > + bar(1); > + baz(1); > +} > > base-commit: 94673a121cfc7f9d51c9d05e31795477f4dc8dc7 > -- > 2.39.1 > >
[PATCH] c++: fix ICE with -Wduplicated-cond [PR107593]
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, ); 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 000..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 +void +foo () +{ + if (T() && T() && int()) +; + else if (T() && T() && int()) +; +} + +template +void bar(T a) +{ + if (a) +; + else if (a) +; +} + +template +void baz(int a) +{ + if (a) +; + else if (a) // { dg-warning "duplicated" } +; +} +void +f () +{ + foo(); + bar(1); + baz(1); +} base-commit: 94673a121cfc7f9d51c9d05e31795477f4dc8dc7 -- 2.39.1