| Message ID | 20220329205321.90251-1-polacek@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | c-family: ICE with -Wconversion and A ?: B [PR101030] | expand |
On Tue, Mar 29, 2022 at 04:53:21PM -0400, Marek Polacek via Gcc-patches wrote: > This patch fixes a crash in conversion_warning on a null expression. > It is null because the testcase uses the GNU A ?: B extension. We > could also use op0 instead of op1 in this case, but it doesn't seem > to be necessary. A related issue: we print warning: overflow in conversion from 'int' to 'char' changes value from '300' to '','' in the test in the patch. That's because the value is 44, it's type is char, and the ASCII value for ',' is 44. So I think we should convert values of char type to int for the diagnostic. Marek
On 3/29/22 16:53, Marek Polacek wrote: > This patch fixes a crash in conversion_warning on a null expression. > It is null because the testcase uses the GNU A ?: B extension. We > could also use op0 instead of op1 in this case, but it doesn't seem > to be necessary. I wonder why we don't represent the extension as SAVE_EXPR(A) ? SAVE_EXPR(A) : B so we don't hit this sort of problem. But the patch is OK. > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > PR c++/101030 > > gcc/c-family/ChangeLog: > > * c-warn.cc (conversion_warning) <case COND_EXPR>: Don't call > conversion_warning when OP1 is null. > > gcc/testsuite/ChangeLog: > > * g++.dg/ext/cond5.C: New test. > --- > gcc/c-family/c-warn.cc | 2 +- > gcc/testsuite/g++.dg/ext/cond5.C | 13 +++++++++++++ > 2 files changed, 14 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/g++.dg/ext/cond5.C > > diff --git a/gcc/c-family/c-warn.cc b/gcc/c-family/c-warn.cc > index 9025fc1c20e..f24ac5d0539 100644 > --- a/gcc/c-family/c-warn.cc > +++ b/gcc/c-family/c-warn.cc > @@ -1300,7 +1300,7 @@ conversion_warning (location_t loc, tree type, tree expr, tree result) > tree op1 = TREE_OPERAND (expr, 1); > tree op2 = TREE_OPERAND (expr, 2); > > - return (conversion_warning (loc, type, op1, result) > + return ((op1 && conversion_warning (loc, type, op1, result)) > || conversion_warning (loc, type, op2, result)); > } > > diff --git a/gcc/testsuite/g++.dg/ext/cond5.C b/gcc/testsuite/g++.dg/ext/cond5.C > new file mode 100644 > index 00000000000..a92f28998f9 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/ext/cond5.C > @@ -0,0 +1,13 @@ > +// PR c++/101030 > +// { dg-do compile { target { c++11 } } } > +// { dg-options "-Wconversion" } > + > +template <int N> > +struct jj { > + int ii[N ?: 1]; > + char c = N ?: 1; // { dg-warning "conversion from .int. to .char. changes value from .300. to " } > +}; > + > +int main() { > + jj<300> kk; > +} > > base-commit: 69db6e7f4e1d07bf8f33e93a29139cc16c1e0a2f
On 3/29/22 16:59, Marek Polacek wrote: > On Tue, Mar 29, 2022 at 04:53:21PM -0400, Marek Polacek via Gcc-patches wrote: >> This patch fixes a crash in conversion_warning on a null expression. >> It is null because the testcase uses the GNU A ?: B extension. We >> could also use op0 instead of op1 in this case, but it doesn't seem >> to be necessary. > > A related issue: we print > warning: overflow in conversion from 'int' to 'char' changes value from '300' to '','' > in the test in the patch. That's because the value is 44, it's type is char, > and the ASCII value for ',' is 44. So I think we should convert values of char > type to int for the diagnostic. Sure.
On Tue, Mar 29, 2022 at 10:21:47PM -0400, Jason Merrill wrote: > On 3/29/22 16:53, Marek Polacek wrote: > > This patch fixes a crash in conversion_warning on a null expression. > > It is null because the testcase uses the GNU A ?: B extension. We > > could also use op0 instead of op1 in this case, but it doesn't seem > > to be necessary. > > I wonder why we don't represent the extension as > > SAVE_EXPR(A) ? SAVE_EXPR(A) : B > > so we don't hit this sort of problem. But the patch is OK. The reason there are no SAVE_EXPRs is that we don't create them in a template: cp_save_expr: /* There is no reason to create a SAVE_EXPR within a template; if needed, we can create the SAVE_EXPR when instantiating the template. Furthermore, the middle-end cannot handle C++-specific tree codes. */ if (processing_template_decl) return expr; and when instantiating we see the first expression is tree_invariant_p_1 aka constant, so no SAVE_EXPR gets created. Patch pushed, thanks. Marek
diff --git a/gcc/c-family/c-warn.cc b/gcc/c-family/c-warn.cc index 9025fc1c20e..f24ac5d0539 100644 --- a/gcc/c-family/c-warn.cc +++ b/gcc/c-family/c-warn.cc @@ -1300,7 +1300,7 @@ conversion_warning (location_t loc, tree type, tree expr, tree result) tree op1 = TREE_OPERAND (expr, 1); tree op2 = TREE_OPERAND (expr, 2); - return (conversion_warning (loc, type, op1, result) + return ((op1 && conversion_warning (loc, type, op1, result)) || conversion_warning (loc, type, op2, result)); } diff --git a/gcc/testsuite/g++.dg/ext/cond5.C b/gcc/testsuite/g++.dg/ext/cond5.C new file mode 100644 index 00000000000..a92f28998f9 --- /dev/null +++ b/gcc/testsuite/g++.dg/ext/cond5.C @@ -0,0 +1,13 @@ +// PR c++/101030 +// { dg-do compile { target { c++11 } } } +// { dg-options "-Wconversion" } + +template <int N> +struct jj { + int ii[N ?: 1]; + char c = N ?: 1; // { dg-warning "conversion from .int. to .char. changes value from .300. to " } +}; + +int main() { + jj<300> kk; +}