Message ID | f452761d-0de2-3c90-4fdc-3947ba175e8d@gmail.com |
---|---|
State | New |
Headers | show |
Series | correct an ILP32/LP64 bug in sprintf warning (PR 91567) | expand |
On 8/27/19 4:34 PM, Martin Sebor wrote: > The recent sprintf+strlen integration doesn't handle unbounded > string lengths entirely correctly for ILP32 targets and causes > -Wformat-overflow false positives in some common cases, including > during GCC bootstrap targeting such systems The attached patch > fixes that mistake. (I think this code could be cleaned up and > simplified some more but in the interest of unblocking the ILP32 > bootstrap and Glibc builds I haven't taken the time to do that.) > The patch also adjusts down the maximum strlen result set by EVRP > to PTRDIFF_MAX - 2, to match what the strlen pass does. > > The strlen maximum would ideally be computed in terms of > max_object_size() (for which there would ideally be a --param > setting), and checked the same way to avoid off-by-one mistakes > between subsystems and their clients. I have not made this change > here but added a FIXME comment mentioning it. I plan to add such > a parameter and use it in max_object_size() in a future change. > > Testing with an ILP32 compiler also ran into the known limitation > of the strlen pass being unable to determine the length of array > members of local aggregates (PR 83543) initialized using > the braced-list syntax. gcc.dg/tree-ssa/builtin-snprintf-6.c > fails a few cases as a result. I've xfailed the assertions > for targets other than x86_64 where it isn't an issue. > > Martin > > gcc-91567.diff > > PR tree-optimization/91567 - Spurious -Wformat-overflow warnings building glibc (32-bit only) > > gcc/ChangeLog: > > PR tree-optimization/91567 > * gimple-ssa-sprintf.c (get_string_length): Handle more forms of lengths > of unknown strings. > * vr-values.c (vr_values::extract_range_basic): Set strlen upper bound > to PTRDIFF_MAX - 2. > > gcc/testsuite/ChangeLog: > > PR tree-optimization/91567 > * gcc.dg/tree-ssa/builtin-snprintf-6.c: Xfail a subset of assertions > on targets other than x86_64 to work around PR 83543. > * gcc.dg/tree-ssa/builtin-sprintf-warn-22.c: New test. OK. I'm not sure this will fix the glibc issue (it looked like it was unrelated to ILP32 to me). jeff
Hi Martin, > The recent sprintf+strlen integration doesn't handle unbounded > string lengths entirely correctly for ILP32 targets and causes > -Wformat-overflow false positives in some common cases, including > during GCC bootstrap targeting such systems The attached patch > fixes that mistake. (I think this code could be cleaned up and > simplified some more but in the interest of unblocking the ILP32 > bootstrap and Glibc builds I haven't taken the time to do that.) > The patch also adjusts down the maximum strlen result set by EVRP > to PTRDIFF_MAX - 2, to match what the strlen pass does. > > The strlen maximum would ideally be computed in terms of > max_object_size() (for which there would ideally be a --param > setting), and checked the same way to avoid off-by-one mistakes > between subsystems and their clients. I have not made this change > here but added a FIXME comment mentioning it. I plan to add such > a parameter and use it in max_object_size() in a future change. > > Testing with an ILP32 compiler also ran into the known limitation > of the strlen pass being unable to determine the length of array > members of local aggregates (PR 83543) initialized using > the braced-list syntax. gcc.dg/tree-ssa/builtin-snprintf-6.c > fails a few cases as a result. I've xfailed the assertions > for targets other than x86_64 where it isn't an issue. there's still something wrong with that testcase: * It XPASSes on i386-pc-solaris2.11 with -m64: XPASS: gcc.dg/tree-ssa/builtin-snprintf-6.c scan-tree-dump-times optimized "Function test_assign_aggregate" 1 It's almost always wrong to only handle x86_64; you need to consider i?86 && lp64, too. * Even after your patch, there are several gcc-testresults postings showing the test to XPASS on x86_64-pc-linux-gnu. I haven't looked closer yet. Rainer
PR tree-optimization/91567 - Spurious -Wformat-overflow warnings building glibc (32-bit only) gcc/ChangeLog: PR tree-optimization/91567 * gimple-ssa-sprintf.c (get_string_length): Handle more forms of lengths of unknown strings. * vr-values.c (vr_values::extract_range_basic): Set strlen upper bound to PTRDIFF_MAX - 2. gcc/testsuite/ChangeLog: PR tree-optimization/91567 * gcc.dg/tree-ssa/builtin-snprintf-6.c: Xfail a subset of assertions on targets other than x86_64 to work around PR 83543. * gcc.dg/tree-ssa/builtin-sprintf-warn-22.c: New test. Index: gcc/gimple-ssa-sprintf.c =================================================================== --- gcc/gimple-ssa-sprintf.c (revision 274960) +++ gcc/gimple-ssa-sprintf.c (working copy) @@ -1994,11 +1994,22 @@ get_string_length (tree str, unsigned eltsize, con or it's SIZE_MAX otherwise. */ /* Return the default result when nothing is known about the string. */ - if (lendata.maxbound - && integer_all_onesp (lendata.maxbound) - && integer_all_onesp (lendata.maxlen)) - return fmtresult (); + if (lendata.maxbound) + { + if (integer_all_onesp (lendata.maxbound) + && integer_all_onesp (lendata.maxlen)) + return fmtresult (); + if (!tree_fits_uhwi_p (lendata.maxbound) + || !tree_fits_uhwi_p (lendata.maxlen)) + return fmtresult (); + + unsigned HOST_WIDE_INT lenmax = tree_to_uhwi (max_object_size ()) - 2; + if (lenmax <= tree_to_uhwi (lendata.maxbound) + && lenmax <= tree_to_uhwi (lendata.maxlen)) + return fmtresult (); + } + HOST_WIDE_INT min = (tree_fits_uhwi_p (lendata.minlen) ? tree_to_uhwi (lendata.minlen) Index: gcc/vr-values.c =================================================================== --- gcc/vr-values.c (revision 274960) +++ gcc/vr-values.c (working copy) @@ -1319,7 +1319,12 @@ vr_values::extract_range_basic (value_range *vr, g tree max = vrp_val_max (ptrdiff_type_node); wide_int wmax = wi::to_wide (max, TYPE_PRECISION (TREE_TYPE (max))); tree range_min = build_zero_cst (type); - tree range_max = wide_int_to_tree (type, wmax - 1); + /* To account for the terminating NUL, the maximum length + is one less than the maximum array size, which in turn + is one less than PTRDIFF_MAX (or SIZE_MAX where it's + smaller than the former type). + FIXME: Use max_object_size() - 1 here. */ + tree range_max = wide_int_to_tree (type, wmax - 2); vr->set (VR_RANGE, range_min, range_max); return; } Index: gcc/testsuite/gcc.dg/tree-ssa/builtin-snprintf-6.c =================================================================== --- gcc/testsuite/gcc.dg/tree-ssa/builtin-snprintf-6.c (revision 274960) +++ gcc/testsuite/gcc.dg/tree-ssa/builtin-snprintf-6.c (working copy) @@ -65,6 +65,10 @@ void test_assign_init_list (void) T (5, ARGS ({ 1, 2, 3, 4, 5, 6, 0 }), "s=%.*s", 3, &a[2]); } +#if __x86_64__ + +/* Enabled only on x86_64 to work around PR 83543. */ + #undef T #define T(expect, init, fmt, ...) \ do { \ @@ -87,7 +91,10 @@ void test_assign_aggregate (void) T (5, "123456", "s=%.*s", 3, &s.a[2]); } +/* { dg-final { scan-tree-dump-times "Function test_assign_aggregate" 1 "optimized" { xfail { ! x86_64-*-* } } } } */ +#endif /* x86_64 */ + #undef T #define T(expect, init, fmt, ...) \ do { \ Index: gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-22.c =================================================================== --- gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-22.c (nonexistent) +++ gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-22.c (working copy) @@ -0,0 +1,58 @@ +/* PR tree-optimization/91567 - Spurious -Wformat-overflow warnings building + glibc (32-bit only) + { dg-do compile } + { dg-options "-O2 -Wall -ftrack-macro-expansion=0" } */ + +typedef __SIZE_TYPE__ size_t; + +extern int sprintf (char*, const char*, ...); +extern size_t strlen (const char*); + +void f (char *); + +void g (char *s1, char *s2) +{ + char b[1025]; + size_t n = __builtin_strlen (s1), d = __builtin_strlen (s2); + if (n + d + 1 >= 1025) + return; + + sprintf (b, "%s.%s", s1, s2); // { dg-bogus "\\\[-Wformat-overflow" } + + f (b); +} + +/* Extracted from gcc/c-cppbuiltin.c. */ + +void cpp_define (char*); + +static void +builtin_define_type_minmax (const char *min_macro, const char *max_macro, + void *type) +{ + extern const char *suffix; + char *buf; + + if (type) + { + buf = (char *) __builtin_alloca (__builtin_strlen (min_macro) + 2 + + __builtin_strlen (suffix) + 1); + sprintf (buf, "%s=0%s", min_macro, suffix); // { dg-bogus "\\\[-Wformat-overflow" } + } + else + { + buf = (char *) __builtin_alloca (__builtin_strlen (min_macro) + 3 + + __builtin_strlen (max_macro) + 6); + sprintf (buf, "%s=(-%s - 1)", min_macro, max_macro); // { dg-bogus "\\\[-Wformat-overflow" } + } + + cpp_define (buf); +} + +void +c_cpp_builtins (void *type) +{ + + builtin_define_type_minmax ("__WCHAR_MIN__", "__WCHAR_MAX__", type); + builtin_define_type_minmax ("__WINT_MIN__", "__WINT_MAX__", type); +}