diff mbox

Fix minor nits in gimple-ssa-sprintf.c (PR tree-optimization/78586)

Message ID 20161130190124.GA3541@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek Nov. 30, 2016, 7:01 p.m. UTC 
Hi!

This patch fixes some minor nits I've raised in the PR, more severe issues
left unresolved there.

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

2016-11-30  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/78586
	* gimple-ssa-sprintf.c (format_integer): Don't handle NOP_EXPR,
	CONVERT_EXPR or COMPONENT_REF here.  Formatting fix.  For
	SSA_NAME_DEF_STMT with NOP_EXPR only change argtype if the rhs1's
	type is INTEGER_TYPE or POINTER_TYPE.


	Jakub

Comments

Martin Sebor Dec. 1, 2016, 1:14 a.m. UTC | #1 
On 11/30/2016 12:01 PM, Jakub Jelinek wrote:
> Hi!
>
> This patch fixes some minor nits I've raised in the PR, more severe issues
> left unresolved there.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Thank you.  One comment below.

> @@ -1059,7 +1048,12 @@ format_integer (const conversion_spec &s
>  		}
>
>  	      if (code == NOP_EXPR)
> -		argtype = TREE_TYPE (gimple_assign_rhs1 (def));
> +		{
> +		  tree type = TREE_TYPE (gimple_assign_rhs1 (def));
> +		  if (TREE_CODE (type) == INTEGER_TYPE
> +		      || TREE_CODE (type) == POINTER_TYPE)
> +		    argtype = type;

As I replied in my comment #6 on the bug, I'm not sure I see what
is wrong with the original code, and I haven't been able to come
up with a test case that demonstrates a problem because of it with
any of the types you mentioned (bool, enum, or floating).

I trust you when you say the change is necessary but I would like
to see a test case.  I can add it myself if you can sketch it out.

Martin
Jakub Jelinek Dec. 1, 2016, 7:51 a.m. UTC | #2 
On Wed, Nov 30, 2016 at 06:14:14PM -0700, Martin Sebor wrote:
> On 11/30/2016 12:01 PM, Jakub Jelinek wrote:
> >Hi!
> >
> >This patch fixes some minor nits I've raised in the PR, more severe issues
> >left unresolved there.
> >
> >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> Thank you.  One comment below.
> 
> >@@ -1059,7 +1048,12 @@ format_integer (const conversion_spec &s
> > 		}
> >
> > 	      if (code == NOP_EXPR)
> >-		argtype = TREE_TYPE (gimple_assign_rhs1 (def));
> >+		{
> >+		  tree type = TREE_TYPE (gimple_assign_rhs1 (def));
> >+		  if (TREE_CODE (type) == INTEGER_TYPE
> >+		      || TREE_CODE (type) == POINTER_TYPE)
> >+		    argtype = type;
> 
> As I replied in my comment #6 on the bug, I'm not sure I see what
> is wrong with the original code, and I haven't been able to come
> up with a test case that demonstrates a problem because of it with
> any of the types you mentioned (bool, enum, or floating).

I think for floating we don't emit NOP_EXPR, but FIX_TRUNC_EXPR;
perhaps bool/enum is fine, but in the UB case where one loads arbitrary
values into bool or enum the precision might be too small for those
(for enums only with -fstrict-enums).  I've been worried about stuff
like FIXED_TYPE etc. as well.  So, if you want a testcase, say with
-O2 -W -Wall -fstrict-enums
enum E { A = 0, B = 3 };
volatile enum E e;
volatile int x;

int
main ()
{
  x = 12345678;
  *(int *)&e = x;
  enum E f = e;
  x = __builtin_snprintf (0, 0, "%d", f);
}

To be precise, this doesn't fail, TREE_TYPE (gimple_assign_rhs1 (def))
in this case is:
 <enumeral_type 0x7ffff170bd20 E
    type <integer_type 0x7ffff170bdc8 unsigned int public unsigned SI
        size <integer_cst 0x7ffff15bf0d8 constant 32>
        unit size <integer_cst 0x7ffff15bf0f0 constant 4>
        align 32 symtab 0 alias set -1 canonical type 0x7ffff170bdc8
        precision 2 min <integer_cst 0x7ffff171bb88 0> max <integer_cst 0x7ffff171bba0 3>>
    sizes-gimplified unsigned SI size <integer_cst 0x7ffff15bf0d8 32> unit size <integer_cst 0x7ffff15bf0f0 4>
    align 32 symtab 0 alias set 1 canonical type 0x7ffff170bd20 precision 32 min <integer_cst 0x7ffff171bb28 0> max <integer_cst 0x7ffff171bbb8 3>
...
so TYPE_PRECISION (argtype) is still 32, but as you can see, TYPE_MIN_VALUE
and TYPE_MAX_VALUE are much more limited, so if we ever wanted to use
those...

So, let's use another testcase, -O2 -W -Wall -fno-tree-vrp -fno-tree-ccp
and again UB in it:
volatile bool e;
volatile int x;   

int
main ()
{
  x = 123;
  *(char *)&e = x;
  bool f = e;
  x = __builtin_snprintf (0, 0, "%d", f);
}

This will store 1 into x, while without -fprintf-return-value it would store
3.

	Jakub
Martin Sebor Dec. 1, 2016, 4:24 p.m. UTC | #3 
> So, let's use another testcase, -O2 -W -Wall -fno-tree-vrp -fno-tree-ccp
> and again UB in it:
> volatile bool e;
> volatile int x;
>
> int
> main ()
> {
>   x = 123;
>   *(char *)&e = x;
>   bool f = e;
>   x = __builtin_snprintf (0, 0, "%d", f);
> }
>
> This will store 1 into x, while without -fprintf-return-value it would store
> 3.

Great, that's what I was looking for.  I turned it into the following
test case.  Let me try to massage it into a compile-only test suitable
for the test suite and commit it later today.

volatile bool e;
volatile int x;

#define FMT "%d"
const char *fmt = FMT;

int
main ()
{
   x = 123;
   *(char *)&e = x;
   bool f = e;

   int n1 = __builtin_snprintf (0, 0, FMT, f);
   int n2 = __builtin_snprintf (0, 0, fmt, f);

   __builtin_printf ("%i == %i\n", n1, n2);
   if (n1 != n2)
     __builtin_abort ();
}

Martin
Jeff Law Dec. 1, 2016, 5:46 p.m. UTC | #4 
On 12/01/2016 12:51 AM, Jakub Jelinek wrote:
> On Wed, Nov 30, 2016 at 06:14:14PM -0700, Martin Sebor wrote:
>> On 11/30/2016 12:01 PM, Jakub Jelinek wrote:
>>> Hi!
>>>
>>> This patch fixes some minor nits I've raised in the PR, more severe issues
>>> left unresolved there.
>>>
>>> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>>
>> Thank you.  One comment below.
>>
>>> @@ -1059,7 +1048,12 @@ format_integer (const conversion_spec &s
>>> 		}
>>>
>>> 	      if (code == NOP_EXPR)
>>> -		argtype = TREE_TYPE (gimple_assign_rhs1 (def));
>>> +		{
>>> +		  tree type = TREE_TYPE (gimple_assign_rhs1 (def));
>>> +		  if (TREE_CODE (type) == INTEGER_TYPE
>>> +		      || TREE_CODE (type) == POINTER_TYPE)
>>> +		    argtype = type;
>>
>> As I replied in my comment #6 on the bug, I'm not sure I see what
>> is wrong with the original code, and I haven't been able to come
>> up with a test case that demonstrates a problem because of it with
>> any of the types you mentioned (bool, enum, or floating).
>
> I think for floating we don't emit NOP_EXPR, but FIX_TRUNC_EXPR;
Correct.  The way to think about this stuff is whether or not the type 
change is *likely* to lead to code generated.  If the type change is not 
likely to generate code, then NOP_EXPR is appropriate.  Else a suitable 
conversion is necessary.  This distinction has long been something we'd 
like to fix, but haven't gotten around it it.

It is almost always the case that a floating point conversion will 
generate code.  So NOP_EXPR is not appropriate for a floating point 
conversion.

A NOP_EXPR will be used for a large number of integer conversions though.

> perhaps bool/enum is fine, but in the UB case where one loads arbitrary
> values into bool or enum the precision might be too small for those
> (for enums only with -fstrict-enums).
Note that loading an out-of-range value into an enum object is, sadly, 
not UB as long as there's enough storage space in the object to hold the 
value.  That's why enums often don't constrain ranges.


Jeff
Jeff Law Dec. 1, 2016, 11:09 p.m. UTC | #5 
On 11/30/2016 12:01 PM, Jakub Jelinek wrote:
> Hi!
>
> This patch fixes some minor nits I've raised in the PR, more severe issues
> left unresolved there.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2016-11-30  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR tree-optimization/78586
> 	* gimple-ssa-sprintf.c (format_integer): Don't handle NOP_EXPR,
> 	CONVERT_EXPR or COMPONENT_REF here.  Formatting fix.  For
> 	SSA_NAME_DEF_STMT with NOP_EXPR only change argtype if the rhs1's
> 	type is INTEGER_TYPE or POINTER_TYPE.
OK.
jeff
Martin Sebor Dec. 2, 2016, 12:20 a.m. UTC | #6 
On 12/01/2016 09:24 AM, Martin Sebor wrote:
>> So, let's use another testcase, -O2 -W -Wall -fno-tree-vrp -fno-tree-ccp
>> and again UB in it:
>> volatile bool e;
>> volatile int x;
>>
>> int
>> main ()
>> {
>>   x = 123;
>>   *(char *)&e = x;
>>   bool f = e;
>>   x = __builtin_snprintf (0, 0, "%d", f);
>> }
>>
>> This will store 1 into x, while without -fprintf-return-value it would
>> store
>> 3.
>
> Great, that's what I was looking for.  I turned it into the following
> test case.  Let me try to massage it into a compile-only test suitable
> for the test suite and commit it later today.

I hadn't looked at the test case carefully enough when I said
the above.  Now that I have I see it fails even with your patch
with VRP enabled) and I don't think that's a problem in GCC but
rather in the test (the undefined behavior you mentioned).  So
I won't be adding it after all.  I don't think it's appropriate
to exercise behavior that we cannot or will not (or should not)
guarantee.  If you have a different test case does show
the problem under these constraints I'm still interested.

Martin

> volatile bool e;
> volatile int x;
>
> #define FMT "%d"
> const char *fmt = FMT;
>
> int
> main ()
> {
>   x = 123;
>   *(char *)&e = x;
>   bool f = e;
>
>   int n1 = __builtin_snprintf (0, 0, FMT, f);
>   int n2 = __builtin_snprintf (0, 0, fmt, f);
>
>   __builtin_printf ("%i == %i\n", n1, n2);
>   if (n1 != n2)
>     __builtin_abort ();
> }
>
> Martin
diff mbox

Patch

--- gcc/gimple-ssa-sprintf.c.jj	2016-11-30 09:00:42.000000000 +0100
+++ gcc/gimple-ssa-sprintf.c	2016-11-30 12:57:05.996480633 +0100
@@ -968,24 +968,13 @@  format_integer (const conversion_spec &s
     }
   else if (TREE_CODE (TREE_TYPE (arg)) == INTEGER_TYPE
 	   || TREE_CODE (TREE_TYPE (arg)) == POINTER_TYPE)
-    {
-      /* Determine the type of the provided non-constant argument.  */
-      if (TREE_CODE (arg) == NOP_EXPR)
-	arg = TREE_OPERAND (arg, 0);
-      else if (TREE_CODE (arg) == CONVERT_EXPR)
-	arg = TREE_OPERAND (arg, 0);
-      if (TREE_CODE (arg) == COMPONENT_REF)
-	arg = TREE_OPERAND (arg, 1);
-
-      argtype = TREE_TYPE (arg);
-    }
+    /* Determine the type of the provided non-constant argument.  */
+    argtype = TREE_TYPE (arg);
   else
-    {
-      /* Don't bother with invalid arguments since they likely would
-	 have already been diagnosed, and disable any further checking
-	 of the format string by returning [-1, -1].  */
-      return fmtresult ();
-    }
+    /* Don't bother with invalid arguments since they likely would
+       have already been diagnosed, and disable any further checking
+       of the format string by returning [-1, -1].  */
+    return fmtresult ();
 
   fmtresult res;
 
@@ -1059,7 +1048,12 @@  format_integer (const conversion_spec &s
 		}
 
 	      if (code == NOP_EXPR)
-		argtype = TREE_TYPE (gimple_assign_rhs1 (def));
+		{
+		  tree type = TREE_TYPE (gimple_assign_rhs1 (def));
+		  if (TREE_CODE (type) == INTEGER_TYPE
+		      || TREE_CODE (type) == POINTER_TYPE)
+		    argtype = type;
+		}
 	    }
 	}
     }