Message ID | 20161130190124.GA3541@tucnak.redhat.com |
---|---|
State | New |
Headers | show |
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
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
> 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
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
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
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
--- 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; + } } } }