diff mbox

suppress unhelpful -Wformat-truncation=2 INT_MAX warning (PR 79448)

Message ID 324eb850-fb33-b0ef-84fd-eeb42761acf6@gmail.com
State New
Headers show

Commit Message

Martin Sebor Feb. 10, 2017, 5:55 p.m. UTC 
The recent Fedora mass rebuild revealed that the Wformat-truncation=2
checker is still a bit too aggressive and complains about potentially
unbounded strings causing subsequent directives t exceed the INT_MAX
limit.  (It's unclear how the build ended up enabling level 2 of
the warning.)

This is because for the purposes of the return value optimization
the pass must assume that such strings really are potentially unbounded
and result in as many as INT_MAX bytes (or more).  That doesn't mean
that it should warn on such cases.

The attached patch relaxes the checker to avoid the warning in this
case.  Since there's no easy way for a user to suppress the warning,
is this change okay for trunk at this stage?

Martin

Comments

Jeff Law Feb. 13, 2017, 11:33 p.m. UTC | #1 
On 02/10/2017 10:55 AM, Martin Sebor wrote:
> The recent Fedora mass rebuild revealed that the Wformat-truncation=2
> checker is still a bit too aggressive and complains about potentially
> unbounded strings causing subsequent directives t exceed the INT_MAX
> limit.  (It's unclear how the build ended up enabling level 2 of
> the warning.)
>
> This is because for the purposes of the return value optimization
> the pass must assume that such strings really are potentially unbounded
> and result in as many as INT_MAX bytes (or more).  That doesn't mean
> that it should warn on such cases.
>
> The attached patch relaxes the checker to avoid the warning in this
> case.  Since there's no easy way for a user to suppress the warning,
> is this change okay for trunk at this stage?
>
> Martin
>
> gcc-79448.diff
>
>
> PR middle-end/79448 - unhelpful -Wformat-truncation=2 warning
>
> gcc/testsuite/ChangeLog:
>
> 	PR middle-end/79448
> 	* gcc.dg/tree-ssa/builtin-snprintf-warn-3.c: New test.
> 	* gcc.dg/tree-ssa/pr79448-2.c: New test.
> 	* gcc.dg/tree-ssa/pr79448.c: New test.
>
> gcc/ChangeLog:
>
> 	PR middle-end/79448
> 	* gimple-ssa-sprintf.c (format_directive): Avoid issuing INT_MAX
> 	  warning for strings of unknown length.
>
> diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
> index e6cc31d..bf76162 100644
> --- a/gcc/gimple-ssa-sprintf.c
> +++ b/gcc/gimple-ssa-sprintf.c
> @@ -2561,11 +2561,15 @@ format_directive (const pass_sprintf_length::call_info &info,
>    /* Raise the total unlikely maximum by the larger of the maximum
>       and the unlikely maximum.  It doesn't matter if the unlikely
>       maximum overflows.  */
> +  unsigned HOST_WIDE_INT save = res->range.unlikely;
>    if (fmtres.range.max < fmtres.range.unlikely)
>      res->range.unlikely += fmtres.range.unlikely;
>    else
>      res->range.unlikely += fmtres.range.max;
>
> +  if (res->range.unlikely < save)
> +    res->range.unlikely = HOST_WIDE_INT_M1U;
> +
So this looks like you're doing an overflow check -- yet earlier your 
comment says "It doesnt' matter if the unlikely maximum overflows". 
ISTM that comment needs updating -- if it doesn't matter, then why check 
for it and clamp the value?


>    res->range.min += fmtres.range.min;
>    res->range.likely += fmtres.range.likely;
>
> @@ -2616,7 +2620,12 @@ format_directive (const pass_sprintf_length::call_info &info,
>
>    /* Has the likely and maximum directive output exceeded INT_MAX?  */
>    bool likelyximax = *dir.beg && res->range.likely > target_int_max ();
> -  bool maxximax = *dir.beg && res->range.max > target_int_max ();
> +  /* Don't consider the maximum to be in excess when it's the result
> +     of a string of unknown length (i.e., whose maximum has been set
> +     to HOST_WIDE_INT_M1U.  */
> +  bool maxximax = (*dir.beg
> +		   && res->range.max > target_int_max ()
> +		   && res->range.max < HOST_WIDE_INT_MAX);
So your comment mentions HOST_WIDE_INT_M1U as the key for a string of 
unknown length.  But that doesn't obviously correspond to what the code 
checks.

Can you please fix up the two comments.  With the comments fixed, this 
is OK.

jeff
Martin Sebor Feb. 14, 2017, 8:06 p.m. UTC | #2 
On 02/13/2017 04:33 PM, Jeff Law wrote:
> On 02/10/2017 10:55 AM, Martin Sebor wrote:
>> The recent Fedora mass rebuild revealed that the Wformat-truncation=2
>> checker is still a bit too aggressive and complains about potentially
>> unbounded strings causing subsequent directives t exceed the INT_MAX
>> limit.  (It's unclear how the build ended up enabling level 2 of
>> the warning.)
>>
>> This is because for the purposes of the return value optimization
>> the pass must assume that such strings really are potentially unbounded
>> and result in as many as INT_MAX bytes (or more).  That doesn't mean
>> that it should warn on such cases.
>>
>> The attached patch relaxes the checker to avoid the warning in this
>> case.  Since there's no easy way for a user to suppress the warning,
>> is this change okay for trunk at this stage?
>>
>> Martin
>>
>> gcc-79448.diff
>>
>>
>> PR middle-end/79448 - unhelpful -Wformat-truncation=2 warning
>>
>> gcc/testsuite/ChangeLog:
>>
>>     PR middle-end/79448
>>     * gcc.dg/tree-ssa/builtin-snprintf-warn-3.c: New test.
>>     * gcc.dg/tree-ssa/pr79448-2.c: New test.
>>     * gcc.dg/tree-ssa/pr79448.c: New test.
>>
>> gcc/ChangeLog:
>>
>>     PR middle-end/79448
>>     * gimple-ssa-sprintf.c (format_directive): Avoid issuing INT_MAX
>>       warning for strings of unknown length.
>>
>> diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
>> index e6cc31d..bf76162 100644
>> --- a/gcc/gimple-ssa-sprintf.c
>> +++ b/gcc/gimple-ssa-sprintf.c
>> @@ -2561,11 +2561,15 @@ format_directive (const
>> pass_sprintf_length::call_info &info,
>>    /* Raise the total unlikely maximum by the larger of the maximum
>>       and the unlikely maximum.  It doesn't matter if the unlikely
>>       maximum overflows.  */
>> +  unsigned HOST_WIDE_INT save = res->range.unlikely;
>>    if (fmtres.range.max < fmtres.range.unlikely)
>>      res->range.unlikely += fmtres.range.unlikely;
>>    else
>>      res->range.unlikely += fmtres.range.max;
>>
>> +  if (res->range.unlikely < save)
>> +    res->range.unlikely = HOST_WIDE_INT_M1U;
>> +
> So this looks like you're doing an overflow check -- yet earlier your
> comment says "It doesnt' matter if the unlikely maximum overflows". ISTM
> that comment needs updating -- if it doesn't matter, then why check for
> it and clamp the value?
>
>
>>    res->range.min += fmtres.range.min;
>>    res->range.likely += fmtres.range.likely;
>>
>> @@ -2616,7 +2620,12 @@ format_directive (const
>> pass_sprintf_length::call_info &info,
>>
>>    /* Has the likely and maximum directive output exceeded INT_MAX?  */
>>    bool likelyximax = *dir.beg && res->range.likely > target_int_max ();
>> -  bool maxximax = *dir.beg && res->range.max > target_int_max ();
>> +  /* Don't consider the maximum to be in excess when it's the result
>> +     of a string of unknown length (i.e., whose maximum has been set
>> +     to HOST_WIDE_INT_M1U.  */
>> +  bool maxximax = (*dir.beg
>> +           && res->range.max > target_int_max ()
>> +           && res->range.max < HOST_WIDE_INT_MAX);
> So your comment mentions HOST_WIDE_INT_M1U as the key for a string of
> unknown length.  But that doesn't obviously correspond to what the code
> checks.
>
> Can you please fix up the two comments.  With the comments fixed, this
> is OK.

Sure, I updated the comments.

The code alternately uses HOST_WIDE_INT_M1U and HOST_WIDE_INT_MAX as
a stand-in for either a "can't happen" or "unbounded/unknown" size.
It's not fully consistent and should be cleaned up and the uses of
HOST_WIDE_INT should be replaced by a class like wide_int as someone
suggested in the past.  If I get to some of the enhancements I'd like
to make in stage 1 (e.g., integrating the pass with tree-ssa-strlen)
I'll see about cleaning this up.

Martin
diff mbox

Patch

PR middle-end/79448 - unhelpful -Wformat-truncation=2 warning

gcc/testsuite/ChangeLog:

	PR middle-end/79448
	* gcc.dg/tree-ssa/builtin-snprintf-warn-3.c: New test.
	* gcc.dg/tree-ssa/pr79448-2.c: New test.
	* gcc.dg/tree-ssa/pr79448.c: New test.

gcc/ChangeLog:

	PR middle-end/79448
	* gimple-ssa-sprintf.c (format_directive): Avoid issuing INT_MAX
	  warning for strings of unknown length.

diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
index e6cc31d..bf76162 100644
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -2561,11 +2561,15 @@  format_directive (const pass_sprintf_length::call_info &info,
   /* Raise the total unlikely maximum by the larger of the maximum
      and the unlikely maximum.  It doesn't matter if the unlikely
      maximum overflows.  */
+  unsigned HOST_WIDE_INT save = res->range.unlikely;
   if (fmtres.range.max < fmtres.range.unlikely)
     res->range.unlikely += fmtres.range.unlikely;
   else
     res->range.unlikely += fmtres.range.max;
 
+  if (res->range.unlikely < save)
+    res->range.unlikely = HOST_WIDE_INT_M1U;
+
   res->range.min += fmtres.range.min;
   res->range.likely += fmtres.range.likely;
 
@@ -2616,7 +2620,12 @@  format_directive (const pass_sprintf_length::call_info &info,
 
   /* Has the likely and maximum directive output exceeded INT_MAX?  */
   bool likelyximax = *dir.beg && res->range.likely > target_int_max ();
-  bool maxximax = *dir.beg && res->range.max > target_int_max ();
+  /* Don't consider the maximum to be in excess when it's the result
+     of a string of unknown length (i.e., whose maximum has been set
+     to HOST_WIDE_INT_M1U.  */
+  bool maxximax = (*dir.beg
+		   && res->range.max > target_int_max ()
+		   && res->range.max < HOST_WIDE_INT_MAX);
 
   if (!warned
       /* Warn for the likely output size at level 1.  */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-snprintf-warn-3.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-snprintf-warn-3.c
new file mode 100644
index 0000000..81c1d89
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-snprintf-warn-3.c
@@ -0,0 +1,193 @@ 
+/* PR middle-end/79448 - unhelpful -Wformat-truncation=2 warning
+   { dg-do compile }
+   { dg-options "-O2 -Wformat -Wformat-truncation=2 -ftrack-macro-expansion=0" } */
+
+typedef __SIZE_TYPE__  size_t;
+typedef __WCHAR_TYPE__ wchar_t;
+
+#define INT_MAX __INT_MAX__
+#define INT_MIN (-INT_MAX - 1)
+
+/* When debugging, define LINE to the line number of the test case to exercise
+   and avoid exercising any of the others.  The buffer and objsize macros
+   below make use of LINE to avoid warnings for other lines.  */
+#ifndef LINE
+# define LINE 0
+#endif
+
+extern int int_value (void);
+extern size_t size_value (void);
+
+int int_range (int min, int max)
+{
+  int n = int_value ();
+  return n < min || max < n ? min : n;
+}
+
+void sink (int, char*, char*);
+
+int dummy_snprintf (char*, size_t, const char*, ...);
+
+char fixed_buffer [256];
+extern char *unknown_buffer;
+extern size_t unknown_size;
+
+/* Helper to expand function to either __builtin_f or dummy_f to
+   make debugging GCC easy.  */
+#define FUNC(f)							\
+  ((!LINE || LINE == __LINE__) ? __builtin_ ## f : dummy_ ## f)
+
+/* Helper test macro.  */
+#define T(size, ...)					\
+  do {							\
+    size_t n = size < 0 ? unknown_size : size;		\
+    char *buf = size < 0 ? unknown_buffer		\
+      : n < sizeof fixed_buffer				\
+      ? fixed_buffer + sizeof fixed_buffer - size	\
+      : unknown_buffer;					\
+    FUNC (snprintf) (buf, n, __VA_ARGS__);		\
+    sink (0, fixed_buffer, unknown_buffer);		\
+  } while (0)
+
+/* Return a value in the range [MIN, MAX].  */
+#define IR(min, max)  int_range (min, max)
+
+struct Arrays
+{
+  char a1[1];
+  char a4k[4096];
+  char a4kp1[4097];
+#if INT_MAX < LONG_MAX
+  char amax[INT_MAX];
+#else
+  char amax[32767];
+#endif
+  char ax[];
+};
+
+void test_string_unchecked (const char *s, const struct Arrays *ar)
+{
+  /* Verify there is no warning with strings of unknown length.  */
+  T (-1, "%-s", s);
+  T (-1, "%-s", ar->ax);
+
+  T (-1, "%s%s", s, s);
+  T (-1, "%s%s", "", s);
+  T (-1, "%s%s", s, "1");
+  T (-1, "%s%s", "1", s);
+
+  /* Verify there is no warning with strings of length that cannot
+     exceed 4k (because of the array size).  */
+  T (-1, "%-s", ar->a1);
+  T (-1, "%-s", ar->a4k);
+
+  /* Verify there's no "exceeds minimum required size of 4095" warning
+     with multiple %s directives and a combination of strings of unknown
+     (and potentially unbounded) length and strings whose length is
+     bounded by the size of the arrays they are stored in.  */
+  T (-1, "%s%s", s, ar->a4k);
+  T (-1, "%s%s", ar->a4k, s);
+  T (-1, "%s%s", ar->a4k, ar->a4k);
+  T (-1, "%s%s", ar->a4k, "123");
+  T (-1, "%s%s", "123", ar->a4k);
+  T (-1, "%s%s", ar->ax, ar->a4k);
+  T (-1, "%s%s", ar->a4k, ar->ax);
+
+  /* Verify that an array that fits a string longer than 4095 bytes
+     does trigger a warning.  */
+  T (-1, "%-s", ar->a4kp1);   /* { dg-warning "directive output between 0 and 4096 bytes may exceed minimum required size of 4095" } */
+
+  /* Also verify that a %s directive with width greater than 4095
+     triggers a warning even if the argument is not longer than 4k.  */
+  T (-1, "%*s", 4096, ar->a4k);   /* { dg-warning "directive output of 4096 bytes exceeds minimum required size of 4095" } */
+
+  /* Verify that precision constrains the putput and suppresses the 4k
+     warning.  */
+  T (-1, "%.*s", 4095, ar->a4kp1);
+
+  T (-1, "%s %s", s, "");
+  T (-1, "%s %s", "", s);
+  T (-1, "%s %s", s, "1");
+  T (-1, "%s %s", "1", s);
+
+  T (-1, "%s%s%s", s, "1", s);
+  T (-1, "%s%s%s", "1", s, "1");
+  T (-1, "%s%s%s", s, s, s);
+  T (-1, "%*s%*s%*s", 4093, s, 4094, s, 4095, s);
+  T (-1, "%s %s %s", s, s, s);
+  T (-1, "%s %s %s", ar->a4k, ar->a4k, ar->a4k);
+  T (-1, "%s %s %s", ar->ax, ar->ax, ar->ax);
+
+  /* Verify that an array of INT_MAX elements doesn't trigger the INT_MAX
+     warning (LP64 only).  */
+  T (-1, "%-s", ar->amax);   /* { dg-warning "directive output between 0 and \[0-9\]+ bytes may exceed minimum required size of 4095" } */
+}
+
+#undef T
+/* Helper test macro.  */
+#define T(size, ...)					\
+  do {							\
+    size_t n = size < 0 ? unknown_size : size;		\
+    char *buf = size < 0 ? unknown_buffer		\
+      : n < sizeof fixed_buffer				\
+      ? fixed_buffer + sizeof fixed_buffer - size	\
+      : unknown_buffer;					\
+    int r = FUNC (snprintf) (buf, n, __VA_ARGS__);	\
+    sink (r, fixed_buffer, unknown_buffer);		\
+  } while (0)
+
+void test_string_checked (const char *s, const struct Arrays *ar)
+{
+  /* Verify there is no warning with strings of unknown length.  */
+  T (-1, "%-s", s);
+  T (-1, "%-s", ar->ax);
+
+  T (-1, "%s%s", s, s);
+  T (-1, "%s%s", "", s);
+  T (-1, "%s%s", s, "1");
+  T (-1, "%s%s", "1", s);
+
+  /* Verify there is no warning with strings of length that cannot
+     exceed 4k (because of the array size).  */
+  T (-1, "%-s", ar->a1);
+  T (-1, "%-s", ar->a4k);
+
+  /* Verify there's no "exceeds minimum required size of 4095" warning
+     with multiple %s directives and a combination of strings of unknown
+     (and potentially unbounded) length and strings whose length is
+     bounded by the size of the arrays they are stored in.  */
+  T (-1, "%s%s", s, ar->a4k);
+  T (-1, "%s%s", ar->a4k, s);
+  T (-1, "%s%s", ar->a4k, ar->a4k);
+  T (-1, "%s%s", ar->a4k, "123");
+  T (-1, "%s%s", "123", ar->a4k);
+  T (-1, "%s%s", ar->ax, ar->a4k);
+  T (-1, "%s%s", ar->a4k, ar->ax);
+
+  /* Verify that an array that fits a string longer than 4095 bytes
+     does trigger a warning.  */
+  T (-1, "%-s", ar->a4kp1);   /* { dg-warning "directive output between 0 and 4096 bytes may exceed minimum required size of 4095" } */
+
+  /* Also verify that a %s directive with width greater than 4095
+     triggers a warning even if the argument is not longer than 4k.  */
+  T (-1, "%*s", 4096, ar->a4k);   /* { dg-warning "directive output of 4096 bytes exceeds minimum required size of 4095" } */
+
+  /* Verify that precision constrains the putput and suppresses the 4k
+     warning.  */
+  T (-1, "%.*s", 4095, ar->a4kp1);
+
+  T (-1, "%s %s", s, "");
+  T (-1, "%s %s", "", s);
+  T (-1, "%s %s", s, "1");
+  T (-1, "%s %s", "1", s);
+
+  T (-1, "%s%s%s", s, "1", s);
+  T (-1, "%s%s%s", "1", s, "1");
+  T (-1, "%s%s%s", s, s, s);
+  T (-1, "%*s%*s%*s", 4093, s, 4094, s, 4095, s);
+  T (-1, "%s %s %s", s, s, s);
+  T (-1, "%s %s %s", ar->a4k, ar->a4k, ar->a4k);
+  T (-1, "%s %s %s", ar->ax, ar->ax, ar->ax);
+
+  T (-1, "%-s", ar->amax);   /* { dg-warning "directive output between 0 and \[0-9\]+ bytes may exceed minimum required size of 4095" } */
+}
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr79448-2.c b/gcc/testsuite/gcc.dg/tree-ssa/pr79448-2.c
new file mode 100644
index 0000000..f75f523
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr79448-2.c
@@ -0,0 +1,21 @@ 
+/* PR middle-end/79448 - unhelpful -Wformat-truncation=2 warning
+   Verify that there's no warning with optimization.
+   { dg-do compile }
+   { dg-options "-O2 -Wall -Wformat -Wformat-truncation=2" } */
+
+typedef __SIZE_TYPE__ size_t;
+
+extern int
+snprintf (char*, size_t, const char*, ...);
+
+char*
+gettext (char*);
+
+char*
+fill (char *buf, size_t len, int count)
+{
+  if (snprintf (buf, len, "%s: %d", gettext ("count"), count) >= len)  /* { dg-bogus "directive output of 2 bytes causes result to exceed .INT_MAX." */
+    return 0;
+
+  return buf;
+}
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr79448.c b/gcc/testsuite/gcc.dg/tree-ssa/pr79448.c
new file mode 100644
index 0000000..c346c9e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr79448.c
@@ -0,0 +1,21 @@ 
+/* PR middle-end/79448 - unhelpful -Wformat-truncation=2 warning
+   Verify that there's no warning without optimization.
+   { dg-do compile }
+   { dg-options "-Wall -Wformat -Wformat-truncation=2" } */
+
+typedef __SIZE_TYPE__ size_t;
+
+extern int
+snprintf (char*, size_t, const char*, ...);
+
+char*
+gettext (char*);
+
+char*
+fill (char *buf, size_t len, int count)
+{
+  if (snprintf (buf, len, "%s: %d", gettext ("count"), count) >= len)  /* { dg-bogus "directive output of 2 bytes causes result to exceed .INT_MAX." */
+    return 0;
+
+  return buf;
+}