[PATCH] c-family: ICE with -Wconversion and A ?: B [PR101030]

[PATCH] c-family: ICE with -Wconversion and A ?: B [PR101030]

[Marek Polacek]

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.

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
-- 
2.35.1

Re: [PATCH] c-family: ICE with -Wconversion and A ?: B [PR101030]

[Marek Polacek]

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

Re: [PATCH] c-family: ICE with -Wconversion and A ?: B [PR101030]

[Jason Merrill]

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

Re: [PATCH] c-family: ICE with -Wconversion and A ?: B [PR101030]

[Jason Merrill]

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.

Re: [PATCH] c-family: ICE with -Wconversion and A ?: B [PR101030]

[Marek Polacek]

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