Re: [PATCH] Fix minor nits in gimple-ssa-sprintf.c (PR tree-optimization/78586)

[Jakub Jelinek]

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