Fix gimple-ssa-sprintf.c ICE (PR tree-optimization/83198)

Message ID 20171213221640.GM2353@tucnak
State New
Headers show
Series
  • Fix gimple-ssa-sprintf.c ICE (PR tree-optimization/83198)
Related show

Commit Message

Jakub Jelinek Dec. 13, 2017, 10:16 p.m.
Hi!

This patch fixes 2 issues in format_floating.  One is that when determining
precision, we should consider solely the type *printf* will read the
argument as (i.e. double unless L or ll modifier is used, in which case
long double), not the type of the argument, because the corresponding
argument could have any type, even not floating, or say __float128 etc.

This is fixed in the first 2 hunks.

The last hunk is to treat REAL_CSTs arguments of incompatible types
as unknown argument, we really don't know what say __float128 passed to
%f or double passed to %La will do; that is something diagnosed by -Wformat,
so the patch just treats it as arbitrary value of the type.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2017-12-13  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/83198
	* gimple-ssa-sprintf.c (format_floating): Set type solely based on
	dir.modifier, regardless of TREE_TYPE (arg).  Assume non-REAL_CST
	value if arg is a REAL_CST with incompatible type.

	* gcc.dg/pr83198.c: New test.


	Jakub

Comments

Martin Sebor Dec. 13, 2017, 10:55 p.m. | #1
On 12/13/2017 03:16 PM, Jakub Jelinek wrote:
> Hi!

>

> This patch fixes 2 issues in format_floating.  One is that when determining

> precision, we should consider solely the type *printf* will read the

> argument as (i.e. double unless L or ll modifier is used, in which case

> long double), not the type of the argument, because the corresponding

> argument could have any type, even not floating, or say __float128 etc.

>

> This is fixed in the first 2 hunks.

>

> The last hunk is to treat REAL_CSTs arguments of incompatible types

> as unknown argument, we really don't know what say __float128 passed to

> %f or double passed to %La will do; that is something diagnosed by -Wformat,

> so the patch just treats it as arbitrary value of the type.

>

> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?


Thanks for the fix.

For the second part, can you please also add a compile-time test
to verify that the result isn't constrained to the same range as
with a real argument?  Checking that the abort below isn't
eliminated would do it for %f:

   void f (char *d)
   {
     int n = __builtin_sprintf (d, "%f", 1.0Q);
     if (n < 8 || 13 < n)
       __builtin_abort ();
   }

A test that has a convenient setup for this is tree-ssa/builtin-
sprintf-2.c in case you want to add to it.

Martin

>

> 2017-12-13  Jakub Jelinek  <jakub@redhat.com>

>

> 	PR tree-optimization/83198

> 	* gimple-ssa-sprintf.c (format_floating): Set type solely based on

> 	dir.modifier, regardless of TREE_TYPE (arg).  Assume non-REAL_CST

> 	value if arg is a REAL_CST with incompatible type.

>

> 	* gcc.dg/pr83198.c: New test.

>

> --- gcc/gimple-ssa-sprintf.c.jj	2017-11-03 15:37:04.000000000 +0100

> +++ gcc/gimple-ssa-sprintf.c	2017-12-13 13:37:59.289435623 +0100

> @@ -1885,6 +1885,8 @@ static fmtresult

>  format_floating (const directive &dir, tree arg)

>  {

>    HOST_WIDE_INT prec[] = { dir.prec[0], dir.prec[1] };

> +  tree type = (dir.modifier == FMT_LEN_L || dir.modifier == FMT_LEN_ll

> +	       ? long_double_type_node : double_type_node);

>

>    /* For an indeterminate precision the lower bound must be assumed

>       to be zero.  */

> @@ -1892,10 +1894,6 @@ format_floating (const directive &dir, t

>      {

>        /* Get the number of fractional decimal digits needed to represent

>  	 the argument without a loss of accuracy.  */

> -      tree type = arg ? TREE_TYPE (arg) :

> -	(dir.modifier == FMT_LEN_L || dir.modifier == FMT_LEN_ll

> -	 ? long_double_type_node : double_type_node);

> -

>        unsigned fmtprec

>  	= REAL_MODE_FORMAT (TYPE_MODE (type))->p;

>

> @@ -1946,7 +1944,9 @@ format_floating (const directive &dir, t

>  	}

>      }

>

> -  if (!arg || TREE_CODE (arg) != REAL_CST)

> +  if (!arg

> +      || TREE_CODE (arg) != REAL_CST

> +      || !useless_type_conversion_p (type, TREE_TYPE (arg)))

>      return format_floating (dir, prec);

>

>    /* The minimum and maximum number of bytes produced by the directive.  */

> --- gcc/testsuite/gcc.dg/pr83198.c.jj	2017-12-13 13:43:36.056192309 +0100

> +++ gcc/testsuite/gcc.dg/pr83198.c	2017-12-13 13:47:11.716474956 +0100

> @@ -0,0 +1,18 @@

> +/* PR tree-optimization/83198 */

> +/* { dg-do compile } */

> +/* { dg-options "-Wall -Wno-format" } */

> +

> +int

> +foo (char *d[6], int x)

> +{

> +  int r = 0;

> +  r += __builtin_sprintf (d[0], "%f", x);

> +  r += __builtin_sprintf (d[1], "%a", x);

> +  r += __builtin_sprintf (d[2], "%f", "foo");

> +  r += __builtin_sprintf (d[3], "%a", "bar");

> +#ifdef __SIZEOF_FLOAT128__

> +  r += __builtin_sprintf (d[4], "%a", 1.0Q);

> +  r += __builtin_sprintf (d[5], "%Lf", 1.0Q);

> +#endif

> +  return r;

> +}

>

> 	Jakub

>
Richard Biener Dec. 14, 2017, 10:30 a.m. | #2
On Wed, 13 Dec 2017, Jakub Jelinek wrote:

> Hi!

> 

> This patch fixes 2 issues in format_floating.  One is that when determining

> precision, we should consider solely the type *printf* will read the

> argument as (i.e. double unless L or ll modifier is used, in which case

> long double), not the type of the argument, because the corresponding

> argument could have any type, even not floating, or say __float128 etc.

> 

> This is fixed in the first 2 hunks.

> 

> The last hunk is to treat REAL_CSTs arguments of incompatible types

> as unknown argument, we really don't know what say __float128 passed to

> %f or double passed to %La will do; that is something diagnosed by -Wformat,

> so the patch just treats it as arbitrary value of the type.

> 

> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?


Ok.

Richard.

> 2017-12-13  Jakub Jelinek  <jakub@redhat.com>

> 

> 	PR tree-optimization/83198

> 	* gimple-ssa-sprintf.c (format_floating): Set type solely based on

> 	dir.modifier, regardless of TREE_TYPE (arg).  Assume non-REAL_CST

> 	value if arg is a REAL_CST with incompatible type.

> 

> 	* gcc.dg/pr83198.c: New test.

> 

> --- gcc/gimple-ssa-sprintf.c.jj	2017-11-03 15:37:04.000000000 +0100

> +++ gcc/gimple-ssa-sprintf.c	2017-12-13 13:37:59.289435623 +0100

> @@ -1885,6 +1885,8 @@ static fmtresult

>  format_floating (const directive &dir, tree arg)

>  {

>    HOST_WIDE_INT prec[] = { dir.prec[0], dir.prec[1] };

> +  tree type = (dir.modifier == FMT_LEN_L || dir.modifier == FMT_LEN_ll

> +	       ? long_double_type_node : double_type_node);

>  

>    /* For an indeterminate precision the lower bound must be assumed

>       to be zero.  */

> @@ -1892,10 +1894,6 @@ format_floating (const directive &dir, t

>      {

>        /* Get the number of fractional decimal digits needed to represent

>  	 the argument without a loss of accuracy.  */

> -      tree type = arg ? TREE_TYPE (arg) :

> -	(dir.modifier == FMT_LEN_L || dir.modifier == FMT_LEN_ll

> -	 ? long_double_type_node : double_type_node);

> -

>        unsigned fmtprec

>  	= REAL_MODE_FORMAT (TYPE_MODE (type))->p;

>  

> @@ -1946,7 +1944,9 @@ format_floating (const directive &dir, t

>  	}

>      }

>  

> -  if (!arg || TREE_CODE (arg) != REAL_CST)

> +  if (!arg

> +      || TREE_CODE (arg) != REAL_CST

> +      || !useless_type_conversion_p (type, TREE_TYPE (arg)))

>      return format_floating (dir, prec);

>  

>    /* The minimum and maximum number of bytes produced by the directive.  */

> --- gcc/testsuite/gcc.dg/pr83198.c.jj	2017-12-13 13:43:36.056192309 +0100

> +++ gcc/testsuite/gcc.dg/pr83198.c	2017-12-13 13:47:11.716474956 +0100

> @@ -0,0 +1,18 @@

> +/* PR tree-optimization/83198 */

> +/* { dg-do compile } */

> +/* { dg-options "-Wall -Wno-format" } */

> +

> +int

> +foo (char *d[6], int x)

> +{

> +  int r = 0;

> +  r += __builtin_sprintf (d[0], "%f", x);

> +  r += __builtin_sprintf (d[1], "%a", x);

> +  r += __builtin_sprintf (d[2], "%f", "foo");

> +  r += __builtin_sprintf (d[3], "%a", "bar");

> +#ifdef __SIZEOF_FLOAT128__

> +  r += __builtin_sprintf (d[4], "%a", 1.0Q);

> +  r += __builtin_sprintf (d[5], "%Lf", 1.0Q);

> +#endif

> +  return r;

> +}

> 

> 	Jakub

> 

> 


-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Patch

--- gcc/gimple-ssa-sprintf.c.jj	2017-11-03 15:37:04.000000000 +0100
+++ gcc/gimple-ssa-sprintf.c	2017-12-13 13:37:59.289435623 +0100
@@ -1885,6 +1885,8 @@  static fmtresult
 format_floating (const directive &dir, tree arg)
 {
   HOST_WIDE_INT prec[] = { dir.prec[0], dir.prec[1] };
+  tree type = (dir.modifier == FMT_LEN_L || dir.modifier == FMT_LEN_ll
+	       ? long_double_type_node : double_type_node);
 
   /* For an indeterminate precision the lower bound must be assumed
      to be zero.  */
@@ -1892,10 +1894,6 @@  format_floating (const directive &dir, t
     {
       /* Get the number of fractional decimal digits needed to represent
 	 the argument without a loss of accuracy.  */
-      tree type = arg ? TREE_TYPE (arg) :
-	(dir.modifier == FMT_LEN_L || dir.modifier == FMT_LEN_ll
-	 ? long_double_type_node : double_type_node);
-
       unsigned fmtprec
 	= REAL_MODE_FORMAT (TYPE_MODE (type))->p;
 
@@ -1946,7 +1944,9 @@  format_floating (const directive &dir, t
 	}
     }
 
-  if (!arg || TREE_CODE (arg) != REAL_CST)
+  if (!arg
+      || TREE_CODE (arg) != REAL_CST
+      || !useless_type_conversion_p (type, TREE_TYPE (arg)))
     return format_floating (dir, prec);
 
   /* The minimum and maximum number of bytes produced by the directive.  */
--- gcc/testsuite/gcc.dg/pr83198.c.jj	2017-12-13 13:43:36.056192309 +0100
+++ gcc/testsuite/gcc.dg/pr83198.c	2017-12-13 13:47:11.716474956 +0100
@@ -0,0 +1,18 @@ 
+/* PR tree-optimization/83198 */
+/* { dg-do compile } */
+/* { dg-options "-Wall -Wno-format" } */
+
+int
+foo (char *d[6], int x)
+{
+  int r = 0;
+  r += __builtin_sprintf (d[0], "%f", x);
+  r += __builtin_sprintf (d[1], "%a", x);
+  r += __builtin_sprintf (d[2], "%f", "foo");
+  r += __builtin_sprintf (d[3], "%a", "bar");
+#ifdef __SIZEOF_FLOAT128__
+  r += __builtin_sprintf (d[4], "%a", 1.0Q);
+  r += __builtin_sprintf (d[5], "%Lf", 1.0Q);
+#endif
+  return r;
+}