| Message ID | 20200415194427.1815190-1-ppalka@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++: Error recovery with errenous DECL_INITIAL [PR94475] | expand |
Oops, consider the typo in the subject line fixed. Also ... On Wed, 15 Apr 2020, Patrick Palka wrote: > Here we're ICE'ing in do_narrow during error-recovery, because ocp_convert > returns error_mark_node after it attempts to reduce a const decl to its > erroneous DECL_INITIAL via scalar_constant_value, and we later pass this > error_mark_node to fold_build2 which isn't prepared to handle error_mark_nodes. > > We could fix this ICE in do_narrow by checking if ocp_convert returns > error_mark_node, but for the sake of consistency and for better error recovery > it seems it'd be better if ocp_convert didn't care that a const decl's > initializer is erroneous and would instead proceed as if the decl was not const, > which is the approach that this patch takes. > > Passes 'make check-c++', does this look OK to commit after full bootstrap and > regtest? > > gcc/cp/ChangeLog: > > PR c++/94475 > * cvt.c (ocp_convert): If the result of scalar_constant_value is > erroneous, discard it and carry on with the original expression. > > gcc/testsuite/ChangeLog: > > PR c++/94475 > * g++.dg/conversion/err-recover2.C: New test. > * g++.dg/diagnostic/pr84138.C: Remove now-bogus warning. > * g++.dg/warn/Wsign-compare-8.C: Remove now-bogus warning. > --- > gcc/cp/cvt.c | 6 +++--- > gcc/testsuite/g++.dg/conversion/err-recover2.C | 10 ++++++++++ > gcc/testsuite/g++.dg/diagnostic/pr84138.C | 2 +- > gcc/testsuite/g++.dg/warn/Wsign-compare-8.C | 2 +- > 4 files changed, 15 insertions(+), 5 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/conversion/err-recover2.C > > diff --git a/gcc/cp/cvt.c b/gcc/cp/cvt.c > index a3b80968b33..b94231a6d08 100644 > --- a/gcc/cp/cvt.c > +++ b/gcc/cp/cvt.c > @@ -723,10 +723,10 @@ ocp_convert (tree type, tree expr, int convtype, int flags, > if (!CLASS_TYPE_P (type)) > { > e = mark_rvalue_use (e); > - e = scalar_constant_value (e); > + tree v = scalar_constant_value (e); > + if (!error_operand_p (v)) > + e = v; > } > - if (error_operand_p (e)) > - return error_mark_node; Removing this error_operand_p check might make an error_mark_node slip through and get processed by the rest of ocp_convert, if the call to mark_rvalue_use above returns error_mark_node. In light of that, please consider this patch instead which restores that error_operand_p check: -- >8 -- Subject: [PATCH] c++: Error recovery with erroneous DECL_INITIAL [PR94475] gcc/cp/ChangeLog: PR c++/94475 * cvt.c (ocp_convert): If the result of scalar_constant_value is erroneous, ignore it and use the original expression. gcc/testsuite/ChangeLog: PR c++/94475 * g++.dg/conversion/err-recover2.C: New test. * g++.dg/diagnostic/pr84138.C: Remove now-bogus warning. * g++.dg/warn/Wsign-compare-8.C: Remove now-bogus warning. --- gcc/cp/cvt.c | 4 +++- gcc/testsuite/g++.dg/conversion/err-recover2.C | 10 ++++++++++ gcc/testsuite/g++.dg/diagnostic/pr84138.C | 2 +- gcc/testsuite/g++.dg/warn/Wsign-compare-8.C | 2 +- 4 files changed, 15 insertions(+), 3 deletions(-) create mode 100644 gcc/testsuite/g++.dg/conversion/err-recover2.C diff --git a/gcc/cp/cvt.c b/gcc/cp/cvt.c index a3b80968b33..656e7fd3ec0 100644 --- a/gcc/cp/cvt.c +++ b/gcc/cp/cvt.c @@ -723,7 +723,9 @@ ocp_convert (tree type, tree expr, int convtype, int flags, if (!CLASS_TYPE_P (type)) { e = mark_rvalue_use (e); - e = scalar_constant_value (e); + tree v = scalar_constant_value (e); + if (!error_operand_p (v)) + e = v; } if (error_operand_p (e)) return error_mark_node; diff --git a/gcc/testsuite/g++.dg/conversion/err-recover2.C b/gcc/testsuite/g++.dg/conversion/err-recover2.C new file mode 100644 index 00000000000..437e1a919ea --- /dev/null +++ b/gcc/testsuite/g++.dg/conversion/err-recover2.C @@ -0,0 +1,10 @@ +// PR c++/94475 +// { dg-do compile } + +unsigned char +sr () +{ + const unsigned char xz = EI; // { dg-error "not declared" } + + return xz - (xz >> 1); +} diff --git a/gcc/testsuite/g++.dg/diagnostic/pr84138.C b/gcc/testsuite/g++.dg/diagnostic/pr84138.C index 5c48b9b164a..00352306a56 100644 --- a/gcc/testsuite/g++.dg/diagnostic/pr84138.C +++ b/gcc/testsuite/g++.dg/diagnostic/pr84138.C @@ -5,4 +5,4 @@ foo() { const int i = 0 = 0; // { dg-error "lvalue required as left operand" } return 1 ? 0 : (char)i; -} // { dg-warning "control reaches" } +} diff --git a/gcc/testsuite/g++.dg/warn/Wsign-compare-8.C b/gcc/testsuite/g++.dg/warn/Wsign-compare-8.C index 237ba84d526..4d2688157fc 100644 --- a/gcc/testsuite/g++.dg/warn/Wsign-compare-8.C +++ b/gcc/testsuite/g++.dg/warn/Wsign-compare-8.C @@ -5,4 +5,4 @@ bool foo (char c) { const int i = 0 = 0; // { dg-error "lvalue" } return c = i; -} // { dg-warning "control reaches" } +}
On 4/15/20 4:43 PM, Patrick Palka wrote: > Oops, consider the typo in the subject line fixed. Also ... > > On Wed, 15 Apr 2020, Patrick Palka wrote: > >> Here we're ICE'ing in do_narrow during error-recovery, because ocp_convert >> returns error_mark_node after it attempts to reduce a const decl to its >> erroneous DECL_INITIAL via scalar_constant_value, and we later pass this >> error_mark_node to fold_build2 which isn't prepared to handle error_mark_nodes. >> >> We could fix this ICE in do_narrow by checking if ocp_convert returns >> error_mark_node, but for the sake of consistency and for better error recovery >> it seems it'd be better if ocp_convert didn't care that a const decl's >> initializer is erroneous and would instead proceed as if the decl was not const, >> which is the approach that this patch takes. >> >> Passes 'make check-c++', does this look OK to commit after full bootstrap and >> regtest? >> >> gcc/cp/ChangeLog: >> >> PR c++/94475 >> * cvt.c (ocp_convert): If the result of scalar_constant_value is >> erroneous, discard it and carry on with the original expression. >> >> gcc/testsuite/ChangeLog: >> >> PR c++/94475 >> * g++.dg/conversion/err-recover2.C: New test. >> * g++.dg/diagnostic/pr84138.C: Remove now-bogus warning. >> * g++.dg/warn/Wsign-compare-8.C: Remove now-bogus warning. >> --- >> gcc/cp/cvt.c | 6 +++--- >> gcc/testsuite/g++.dg/conversion/err-recover2.C | 10 ++++++++++ >> gcc/testsuite/g++.dg/diagnostic/pr84138.C | 2 +- >> gcc/testsuite/g++.dg/warn/Wsign-compare-8.C | 2 +- >> 4 files changed, 15 insertions(+), 5 deletions(-) >> create mode 100644 gcc/testsuite/g++.dg/conversion/err-recover2.C >> >> diff --git a/gcc/cp/cvt.c b/gcc/cp/cvt.c >> index a3b80968b33..b94231a6d08 100644 >> --- a/gcc/cp/cvt.c >> +++ b/gcc/cp/cvt.c >> @@ -723,10 +723,10 @@ ocp_convert (tree type, tree expr, int convtype, int flags, >> if (!CLASS_TYPE_P (type)) >> { >> e = mark_rvalue_use (e); >> - e = scalar_constant_value (e); >> + tree v = scalar_constant_value (e); >> + if (!error_operand_p (v)) >> + e = v; >> } >> - if (error_operand_p (e)) >> - return error_mark_node; > > Removing this error_operand_p check might make an error_mark_node slip > through and get processed by the rest of ocp_convert, if the call to > mark_rvalue_use above returns error_mark_node. > > In light of that, please consider this patch instead which restores that > error_operand_p check: OK. I wonder if we want to drop the call to scalar_constant_value entirely in GCC 11, and expect that the expression will be folded properly later. > -- >8 -- > > Subject: [PATCH] c++: Error recovery with erroneous DECL_INITIAL [PR94475] > > gcc/cp/ChangeLog: > > PR c++/94475 > * cvt.c (ocp_convert): If the result of scalar_constant_value is > erroneous, ignore it and use the original expression. > > gcc/testsuite/ChangeLog: > > PR c++/94475 > * g++.dg/conversion/err-recover2.C: New test. > * g++.dg/diagnostic/pr84138.C: Remove now-bogus warning. > * g++.dg/warn/Wsign-compare-8.C: Remove now-bogus warning. > --- > gcc/cp/cvt.c | 4 +++- > gcc/testsuite/g++.dg/conversion/err-recover2.C | 10 ++++++++++ > gcc/testsuite/g++.dg/diagnostic/pr84138.C | 2 +- > gcc/testsuite/g++.dg/warn/Wsign-compare-8.C | 2 +- > 4 files changed, 15 insertions(+), 3 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/conversion/err-recover2.C > > diff --git a/gcc/cp/cvt.c b/gcc/cp/cvt.c > index a3b80968b33..656e7fd3ec0 100644 > --- a/gcc/cp/cvt.c > +++ b/gcc/cp/cvt.c > @@ -723,7 +723,9 @@ ocp_convert (tree type, tree expr, int convtype, int flags, > if (!CLASS_TYPE_P (type)) > { > e = mark_rvalue_use (e); > - e = scalar_constant_value (e); > + tree v = scalar_constant_value (e); > + if (!error_operand_p (v)) > + e = v; > } > if (error_operand_p (e)) > return error_mark_node; > diff --git a/gcc/testsuite/g++.dg/conversion/err-recover2.C b/gcc/testsuite/g++.dg/conversion/err-recover2.C > new file mode 100644 > index 00000000000..437e1a919ea > --- /dev/null > +++ b/gcc/testsuite/g++.dg/conversion/err-recover2.C > @@ -0,0 +1,10 @@ > +// PR c++/94475 > +// { dg-do compile } > + > +unsigned char > +sr () > +{ > + const unsigned char xz = EI; // { dg-error "not declared" } > + > + return xz - (xz >> 1); > +} > diff --git a/gcc/testsuite/g++.dg/diagnostic/pr84138.C b/gcc/testsuite/g++.dg/diagnostic/pr84138.C > index 5c48b9b164a..00352306a56 100644 > --- a/gcc/testsuite/g++.dg/diagnostic/pr84138.C > +++ b/gcc/testsuite/g++.dg/diagnostic/pr84138.C > @@ -5,4 +5,4 @@ foo() > { > const int i = 0 = 0; // { dg-error "lvalue required as left operand" } > return 1 ? 0 : (char)i; > -} // { dg-warning "control reaches" } > +} > diff --git a/gcc/testsuite/g++.dg/warn/Wsign-compare-8.C b/gcc/testsuite/g++.dg/warn/Wsign-compare-8.C > index 237ba84d526..4d2688157fc 100644 > --- a/gcc/testsuite/g++.dg/warn/Wsign-compare-8.C > +++ b/gcc/testsuite/g++.dg/warn/Wsign-compare-8.C > @@ -5,4 +5,4 @@ bool foo (char c) > { > const int i = 0 = 0; // { dg-error "lvalue" } > return c = i; > -} // { dg-warning "control reaches" } > +} >
On Wed, 15 Apr 2020, Jason Merrill wrote: > On 4/15/20 4:43 PM, Patrick Palka wrote: > > Oops, consider the typo in the subject line fixed. Also ... > > > > On Wed, 15 Apr 2020, Patrick Palka wrote: > > > > > Here we're ICE'ing in do_narrow during error-recovery, because ocp_convert > > > returns error_mark_node after it attempts to reduce a const decl to its > > > erroneous DECL_INITIAL via scalar_constant_value, and we later pass this > > > error_mark_node to fold_build2 which isn't prepared to handle > > > error_mark_nodes. > > > > > > We could fix this ICE in do_narrow by checking if ocp_convert returns > > > error_mark_node, but for the sake of consistency and for better error > > > recovery > > > it seems it'd be better if ocp_convert didn't care that a const decl's > > > initializer is erroneous and would instead proceed as if the decl was not > > > const, > > > which is the approach that this patch takes. > > > > > > Passes 'make check-c++', does this look OK to commit after full bootstrap > > > and > > > regtest? > > > > > > gcc/cp/ChangeLog: > > > > > > PR c++/94475 > > > * cvt.c (ocp_convert): If the result of scalar_constant_value is > > > erroneous, discard it and carry on with the original expression. > > > > > > gcc/testsuite/ChangeLog: > > > > > > PR c++/94475 > > > * g++.dg/conversion/err-recover2.C: New test. > > > * g++.dg/diagnostic/pr84138.C: Remove now-bogus warning. > > > * g++.dg/warn/Wsign-compare-8.C: Remove now-bogus warning. > > > --- > > > gcc/cp/cvt.c | 6 +++--- > > > gcc/testsuite/g++.dg/conversion/err-recover2.C | 10 ++++++++++ > > > gcc/testsuite/g++.dg/diagnostic/pr84138.C | 2 +- > > > gcc/testsuite/g++.dg/warn/Wsign-compare-8.C | 2 +- > > > 4 files changed, 15 insertions(+), 5 deletions(-) > > > create mode 100644 gcc/testsuite/g++.dg/conversion/err-recover2.C > > > > > > diff --git a/gcc/cp/cvt.c b/gcc/cp/cvt.c > > > index a3b80968b33..b94231a6d08 100644 > > > --- a/gcc/cp/cvt.c > > > +++ b/gcc/cp/cvt.c > > > @@ -723,10 +723,10 @@ ocp_convert (tree type, tree expr, int convtype, int > > > flags, > > > if (!CLASS_TYPE_P (type)) > > > { > > > e = mark_rvalue_use (e); > > > - e = scalar_constant_value (e); > > > + tree v = scalar_constant_value (e); > > > + if (!error_operand_p (v)) > > > + e = v; > > > } > > > - if (error_operand_p (e)) > > > - return error_mark_node; > > > > Removing this error_operand_p check might make an error_mark_node slip > > through and get processed by the rest of ocp_convert, if the call to > > mark_rvalue_use above returns error_mark_node. > > > > In light of that, please consider this patch instead which restores that > > error_operand_p check: > > OK. I wonder if we want to drop the call to scalar_constant_value entirely in > GCC 11, and expect that the expression will be folded properly later. That makes sense to me. On its own, removing the call entirely causes a few testsuite regressions, but I haven't looked into any of them yet. > > > -- >8 -- > > > > Subject: [PATCH] c++: Error recovery with erroneous DECL_INITIAL [PR94475] > > > > gcc/cp/ChangeLog: > > > > PR c++/94475 > > * cvt.c (ocp_convert): If the result of scalar_constant_value is > > erroneous, ignore it and use the original expression. > > > > gcc/testsuite/ChangeLog: > > > > PR c++/94475 > > * g++.dg/conversion/err-recover2.C: New test. > > * g++.dg/diagnostic/pr84138.C: Remove now-bogus warning. > > * g++.dg/warn/Wsign-compare-8.C: Remove now-bogus warning. > > --- > > gcc/cp/cvt.c | 4 +++- > > gcc/testsuite/g++.dg/conversion/err-recover2.C | 10 ++++++++++ > > gcc/testsuite/g++.dg/diagnostic/pr84138.C | 2 +- > > gcc/testsuite/g++.dg/warn/Wsign-compare-8.C | 2 +- > > 4 files changed, 15 insertions(+), 3 deletions(-) > > create mode 100644 gcc/testsuite/g++.dg/conversion/err-recover2.C > > > > diff --git a/gcc/cp/cvt.c b/gcc/cp/cvt.c > > index a3b80968b33..656e7fd3ec0 100644 > > --- a/gcc/cp/cvt.c > > +++ b/gcc/cp/cvt.c > > @@ -723,7 +723,9 @@ ocp_convert (tree type, tree expr, int convtype, int > > flags, > > if (!CLASS_TYPE_P (type)) > > { > > e = mark_rvalue_use (e); > > - e = scalar_constant_value (e); > > + tree v = scalar_constant_value (e); > > + if (!error_operand_p (v)) > > + e = v; > > } > > if (error_operand_p (e)) > > return error_mark_node; > > diff --git a/gcc/testsuite/g++.dg/conversion/err-recover2.C > > b/gcc/testsuite/g++.dg/conversion/err-recover2.C > > new file mode 100644 > > index 00000000000..437e1a919ea > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/conversion/err-recover2.C > > @@ -0,0 +1,10 @@ > > +// PR c++/94475 > > +// { dg-do compile } > > + > > +unsigned char > > +sr () > > +{ > > + const unsigned char xz = EI; // { dg-error "not declared" } > > + > > + return xz - (xz >> 1); > > +} > > diff --git a/gcc/testsuite/g++.dg/diagnostic/pr84138.C > > b/gcc/testsuite/g++.dg/diagnostic/pr84138.C > > index 5c48b9b164a..00352306a56 100644 > > --- a/gcc/testsuite/g++.dg/diagnostic/pr84138.C > > +++ b/gcc/testsuite/g++.dg/diagnostic/pr84138.C > > @@ -5,4 +5,4 @@ foo() > > { > > const int i = 0 = 0; // { dg-error "lvalue required as left operand" } > > return 1 ? 0 : (char)i; > > -} // { dg-warning "control reaches" } > > +} > > diff --git a/gcc/testsuite/g++.dg/warn/Wsign-compare-8.C > > b/gcc/testsuite/g++.dg/warn/Wsign-compare-8.C > > index 237ba84d526..4d2688157fc 100644 > > --- a/gcc/testsuite/g++.dg/warn/Wsign-compare-8.C > > +++ b/gcc/testsuite/g++.dg/warn/Wsign-compare-8.C > > @@ -5,4 +5,4 @@ bool foo (char c) > > { > > const int i = 0 = 0; // { dg-error "lvalue" } > > return c = i; > > -} // { dg-warning "control reaches" } > > +} > > > >
diff --git a/gcc/cp/cvt.c b/gcc/cp/cvt.c index a3b80968b33..b94231a6d08 100644 --- a/gcc/cp/cvt.c +++ b/gcc/cp/cvt.c @@ -723,10 +723,10 @@ ocp_convert (tree type, tree expr, int convtype, int flags, if (!CLASS_TYPE_P (type)) { e = mark_rvalue_use (e); - e = scalar_constant_value (e); + tree v = scalar_constant_value (e); + if (!error_operand_p (v)) + e = v; } - if (error_operand_p (e)) - return error_mark_node; if (NULLPTR_TYPE_P (type) && null_ptr_cst_p (e)) { diff --git a/gcc/testsuite/g++.dg/conversion/err-recover2.C b/gcc/testsuite/g++.dg/conversion/err-recover2.C new file mode 100644 index 00000000000..437e1a919ea --- /dev/null +++ b/gcc/testsuite/g++.dg/conversion/err-recover2.C @@ -0,0 +1,10 @@ +// PR c++/94475 +// { dg-do compile } + +unsigned char +sr () +{ + const unsigned char xz = EI; // { dg-error "not declared" } + + return xz - (xz >> 1); +} diff --git a/gcc/testsuite/g++.dg/diagnostic/pr84138.C b/gcc/testsuite/g++.dg/diagnostic/pr84138.C index 5c48b9b164a..00352306a56 100644 --- a/gcc/testsuite/g++.dg/diagnostic/pr84138.C +++ b/gcc/testsuite/g++.dg/diagnostic/pr84138.C @@ -5,4 +5,4 @@ foo() { const int i = 0 = 0; // { dg-error "lvalue required as left operand" } return 1 ? 0 : (char)i; -} // { dg-warning "control reaches" } +} diff --git a/gcc/testsuite/g++.dg/warn/Wsign-compare-8.C b/gcc/testsuite/g++.dg/warn/Wsign-compare-8.C index 237ba84d526..4d2688157fc 100644 --- a/gcc/testsuite/g++.dg/warn/Wsign-compare-8.C +++ b/gcc/testsuite/g++.dg/warn/Wsign-compare-8.C @@ -5,4 +5,4 @@ bool foo (char c) { const int i = 0 = 0; // { dg-error "lvalue" } return c = i; -} // { dg-warning "control reaches" } +}