correct an ILP32/LP64 bug in sprintf warning (PR 91567)

Message ID f452761d-0de2-3c90-4fdc-3947ba175e8d@gmail.com
State New
Headers show
Series
  • correct an ILP32/LP64 bug in sprintf warning (PR 91567)
Related show

Commit Message

Martin Sebor Aug. 27, 2019, 10:34 p.m.
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

Comments

Jeff Law Aug. 27, 2019, 11:03 p.m. | #1
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
Rainer Orth Aug. 29, 2019, 2:17 p.m. | #2
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

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Patch

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);
+}