[PATCH] handle enumerated types in -Wformat-overflow (PR 80397)

[PATCH] handle enumerated types in -Wformat-overflow (PR 80397)

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 604 bytes --]

In a review of my fix for bug 80364 Jakub pointed out that to
determine whether an argument to an integer directive is of
an integer type the gimple-ssa-sprintf pass tests the type code
for equality to INTEGER_TYPE when it should instead be using
INTEGRAL_TYPE_P().  This has the effect of the pass being unable
to use the available range of arguments of enumerated types,
resulting in both false positives and false negatives, and in
some cases, in emitting suboptimal code.

The attached patch replaces those tests with INTEGRAL_TYPE_P().

Since this is not a regression I submit it for GCC 8.

Martin

[-- Attachment #2: gcc-80397.diff --]
[-- Type: text/x-patch, Size: 4207 bytes --]

PR tree-optimization/80397 - missing -Wformat-overflow with arguments of enum types

gcc/ChangeLog:

	PR tree-optimization/80397
	* gimple-ssa-sprintf.c (format_integer): Use INTEGRAL_TYPE_P()
	instead of testing for equality to INTEGER_TYPE.

gcc/testsuite/ChangeLog:

	PR tree-optimization/80397
	* gcc.dg/tree-ssa/builtin-sprintf-warn-17.c: New test.
diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
index 2e62086..d63d5be 100644
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -1249,7 +1249,7 @@ format_integer (const directive &dir, tree arg)
 
       return res;
     }
-  else if (TREE_CODE (TREE_TYPE (arg)) == INTEGER_TYPE
+  else if (INTEGRAL_TYPE_P (TREE_TYPE (arg))
 	   || TREE_CODE (TREE_TYPE (arg)) == POINTER_TYPE)
     /* Determine the type of the provided non-constant argument.  */
     argtype = TREE_TYPE (arg);
@@ -1269,7 +1269,7 @@ format_integer (const directive &dir, tree arg)
 
   if (arg
       && TREE_CODE (arg) == SSA_NAME
-      && TREE_CODE (argtype) == INTEGER_TYPE)
+      && INTEGRAL_TYPE_P (argtype))
     {
       /* Try to determine the range of values of the integer argument
 	 (range information is not available for pointers).  */
@@ -1314,7 +1314,7 @@ format_integer (const directive &dir, tree arg)
 	      if (code == NOP_EXPR)
 		{
 		  tree type = TREE_TYPE (gimple_assign_rhs1 (def));
-		  if (TREE_CODE (type) == INTEGER_TYPE
+		  if (INTEGRAL_TYPE_P (type)
 		      || TREE_CODE (type) == POINTER_TYPE)
 		    argtype = type;
 		}
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-17.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-17.c
new file mode 100644
index 0000000..e043938
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-17.c
@@ -0,0 +1,62 @@
+/* PR tree-optimization/80397 - missing -Wformat-overflow with arguments
+   of enum types
+   { dg-do compile }
+   { dg-options "-O2 -Wall -Wformat-overflow=1 -ftrack-macro-expansion=0" }
+   { dg-require-effective-target int32plus } */
+
+void sink (char*);
+
+long long integer_range (long long min, long long max)
+{
+  extern long long integer_value (void);
+  long long n = integer_value ();
+  return n < min || max < n ? min : n;
+}
+
+typedef enum { i0, imax = __INT_MAX__ } Int;
+typedef enum { ll0, llmax = __LONG_LONG_MAX__ } LLong;
+
+#define R(T, min, max) (T)integer_range (min, max)
+
+char buffer[1];
+#define T(fmt, ...)						\
+  __builtin_sprintf (buffer + 1, fmt, __VA_ARGS__), sink (buffer)
+
+void test_bool (_Bool b)
+{
+  T ("%hhi", b);   // { dg-warning "writing 1 byte" }
+  T ( "%hi", b);   // { dg-warning "writing 1 byte" }
+  T (  "%i", b);   // { dg-warning "writing 1 byte" }
+}
+
+void test_enum (void)
+{
+  T ("%hhi", R (Int,    1,     1));   // { dg-warning "writing 1 byte" }
+  T ("%hhi", R (Int,    1,    22));   // { dg-warning "between 1 and 2 bytes" }
+
+  T ( "%hi", R (Int,    1,     2));   // { dg-warning "writing 1 " }
+  T ( "%hi", R (Int,    1,    22));   // { dg-warning "between 1 and 2 " }
+  T ( "%hi", R (Int,   22,   333));   // { dg-warning "between 2 and 3 " }
+  T ( "%hi", R (Int,  333,  4444));   // { dg-warning "between 3 and 4 " }
+
+  T (  "%i", R (Int,    1,     1));   // { dg-warning "writing 1 " }
+  T (  "%i", R (Int,    1,    22));   // { dg-warning "between 1 and 2 " }
+  T (  "%i", R (Int,   22,   333));   // { dg-warning "between 2 and 3 " }
+  T (  "%i", R (Int,  333,  4444));   // { dg-warning "between 3 and 4 " }
+  T (  "%i", R (Int, 4444, 55555));   // { dg-warning "between 4 and 5 " }
+
+#if __LONG_MAX__ == __LONG_LONG_MAX__
+#  define LLI "%li"
+#else
+#  define LLI "%lli"
+#endif
+
+  T (LLI, R (LLong,    1,     1));       // { dg-warning "writing 1 " }
+  T (LLI, R (LLong,    1,    22));       // { dg-warning "between 1 and 2 " }
+  T (LLI, R (LLong,   22,   333));       // { dg-warning "between 2 and 3 " }
+  T (LLI, R (LLong,  333,  4444));       // { dg-warning "between 3 and 4 " }
+  T (LLI, R (LLong, 4444, 55555));       // { dg-warning "between 4 and 5 " }
+
+  T (LLI, R (LLong, 4444, 1234567890));  // { dg-warning "between 4 and 10 " }
+  T (LLI, R (LLong, 4444, 12345678901)); // { dg-warning "between 4 and 11 " }
+}

Re: [PATCH] handle enumerated types in -Wformat-overflow (PR 80397)

[Jeff Law]

On 04/11/2017 12:57 PM, Martin Sebor wrote:
> In a review of my fix for bug 80364 Jakub pointed out that to
> determine whether an argument to an integer directive is of
> an integer type the gimple-ssa-sprintf pass tests the type code
> for equality to INTEGER_TYPE when it should instead be using
> INTEGRAL_TYPE_P().  This has the effect of the pass being unable
> to use the available range of arguments of enumerated types,
> resulting in both false positives and false negatives, and in
> some cases, in emitting suboptimal code.
> 
> The attached patch replaces those tests with INTEGRAL_TYPE_P().
> 
> Since this is not a regression I submit it for GCC 8.
You might consider using POINTER_TYPE_P as well.

It's also worth noting that an enum object can have values that aren't 
part of the enum.  It's a long standing language wart that I don't 
expect to ever be fixed.  As a result enums often don't have as good of 
range data as you might expect.

jeff

Re: [PATCH] handle enumerated types in -Wformat-overflow (PR 80397)

[Martin Sebor]

On 04/24/2017 03:35 PM, Jeff Law wrote:
> On 04/11/2017 12:57 PM, Martin Sebor wrote:
>> In a review of my fix for bug 80364 Jakub pointed out that to
>> determine whether an argument to an integer directive is of
>> an integer type the gimple-ssa-sprintf pass tests the type code
>> for equality to INTEGER_TYPE when it should instead be using
>> INTEGRAL_TYPE_P().  This has the effect of the pass being unable
>> to use the available range of arguments of enumerated types,
>> resulting in both false positives and false negatives, and in
>> some cases, in emitting suboptimal code.
>>
>> The attached patch replaces those tests with INTEGRAL_TYPE_P().
>>
>> Since this is not a regression I submit it for GCC 8.
> You might consider using POINTER_TYPE_P as well.

You mean rather than (TREE_CODE (type) == POINTER_TYPE)?  Those
I believe are vestiges of the %p handling that was removed sometime
last year, and (unless you are recommending I remove them as part
of this patch) should probably be removed during the next cleanup.

>
> It's also worth noting that an enum object can have values that aren't
> part of the enum.  It's a long standing language wart that I don't
> expect to ever be fixed.  As a result enums often don't have as good of
> range data as you might expect.

Yes, it's a common source of bugs.  Unfortunately, unlike Clang,
GCC doesn't have a warning for assigning a constant to an emum
variable that doesn't match one of its enumerators.  I'm testing
a patch to add the same warning.

Martin

Re: [PATCH] handle enumerated types in -Wformat-overflow (PR 80397)

[Jeff Law]

On 04/26/2017 11:05 AM, Martin Sebor wrote:
> On 04/24/2017 03:35 PM, Jeff Law wrote:
>> On 04/11/2017 12:57 PM, Martin Sebor wrote:
>>> In a review of my fix for bug 80364 Jakub pointed out that to
>>> determine whether an argument to an integer directive is of
>>> an integer type the gimple-ssa-sprintf pass tests the type code
>>> for equality to INTEGER_TYPE when it should instead be using
>>> INTEGRAL_TYPE_P().  This has the effect of the pass being unable
>>> to use the available range of arguments of enumerated types,
>>> resulting in both false positives and false negatives, and in
>>> some cases, in emitting suboptimal code.
>>>
>>> The attached patch replaces those tests with INTEGRAL_TYPE_P().
>>>
>>> Since this is not a regression I submit it for GCC 8.
>> You might consider using POINTER_TYPE_P as well.
> 
> You mean rather than (TREE_CODE (type) == POINTER_TYPE)?  Those
> I believe are vestiges of the %p handling that was removed sometime
> last year, and (unless you are recommending I remove them as part
> of this patch) should probably be removed during the next cleanup.Yes, that can be a follow-up cleanup.

For the future, if you find yourself writing something like
TREE_CODE (TREE_TYPE (x)) == POINTER_TYPE, you're usually going to be 
better off using POINTER_TYPE_P (TREE_TYPE (x)).  That allows the code 
to work with C++ references as well as C pointers.


Jeff

Re: [PATCH] handle enumerated types in -Wformat-overflow (PR 80397)

[Martin Sebor]

On 04/28/2017 12:35 PM, Jeff Law wrote:
> On 04/26/2017 11:05 AM, Martin Sebor wrote:
>> On 04/24/2017 03:35 PM, Jeff Law wrote:
>>> On 04/11/2017 12:57 PM, Martin Sebor wrote:
>>>> In a review of my fix for bug 80364 Jakub pointed out that to
>>>> determine whether an argument to an integer directive is of
>>>> an integer type the gimple-ssa-sprintf pass tests the type code
>>>> for equality to INTEGER_TYPE when it should instead be using
>>>> INTEGRAL_TYPE_P().  This has the effect of the pass being unable
>>>> to use the available range of arguments of enumerated types,
>>>> resulting in both false positives and false negatives, and in
>>>> some cases, in emitting suboptimal code.
>>>>
>>>> The attached patch replaces those tests with INTEGRAL_TYPE_P().
>>>>
>>>> Since this is not a regression I submit it for GCC 8.
>>> You might consider using POINTER_TYPE_P as well.
>>
>> You mean rather than (TREE_CODE (type) == POINTER_TYPE)?  Those
>> I believe are vestiges of the %p handling that was removed sometime
>> last year, and (unless you are recommending I remove them as part
>> of this patch) should probably be removed during the next cleanup.Yes,
>> that can be a follow-up cleanup.
>
> For the future, if you find yourself writing something like
> TREE_CODE (TREE_TYPE (x)) == POINTER_TYPE, you're usually going to be
> better off using POINTER_TYPE_P (TREE_TYPE (x)).  That allows the code
> to work with C++ references as well as C pointers.

I'll keep it in mind, thanks.  Should I take this as approval
of the patch as is or are there some changes you'd like me to
make?

Martin

Re: [PATCH] handle enumerated types in -Wformat-overflow (PR 80397)

[Martin Sebor]

Ping: Jeff, is this patch approved?
   https://gcc.gnu.org/ml/gcc-patches/2017-04/msg00547.html

On 05/08/2017 08:38 PM, Martin Sebor wrote:
> On 04/28/2017 12:35 PM, Jeff Law wrote:
>> On 04/26/2017 11:05 AM, Martin Sebor wrote:
>>> On 04/24/2017 03:35 PM, Jeff Law wrote:
>>>> On 04/11/2017 12:57 PM, Martin Sebor wrote:
>>>>> In a review of my fix for bug 80364 Jakub pointed out that to
>>>>> determine whether an argument to an integer directive is of
>>>>> an integer type the gimple-ssa-sprintf pass tests the type code
>>>>> for equality to INTEGER_TYPE when it should instead be using
>>>>> INTEGRAL_TYPE_P().  This has the effect of the pass being unable
>>>>> to use the available range of arguments of enumerated types,
>>>>> resulting in both false positives and false negatives, and in
>>>>> some cases, in emitting suboptimal code.
>>>>>
>>>>> The attached patch replaces those tests with INTEGRAL_TYPE_P().
>>>>>
>>>>> Since this is not a regression I submit it for GCC 8.
>>>> You might consider using POINTER_TYPE_P as well.
>>>
>>> You mean rather than (TREE_CODE (type) == POINTER_TYPE)?  Those
>>> I believe are vestiges of the %p handling that was removed sometime
>>> last year, and (unless you are recommending I remove them as part
>>> of this patch) should probably be removed during the next cleanup.Yes,
>>> that can be a follow-up cleanup.
>>
>> For the future, if you find yourself writing something like
>> TREE_CODE (TREE_TYPE (x)) == POINTER_TYPE, you're usually going to be
>> better off using POINTER_TYPE_P (TREE_TYPE (x)).  That allows the code
>> to work with C++ references as well as C pointers.
>
> I'll keep it in mind, thanks.  Should I take this as approval
> of the patch as is or are there some changes you'd like me to
> make?
>
> Martin

[PING #2] [PATCH] handle enumerated types in -Wformat-overflow (PR 80397)

[Martin Sebor]

Ping:

   https://gcc.gnu.org/ml/gcc-patches/2017-04/msg00547.html

On 05/15/2017 09:01 PM, Martin Sebor wrote:
> Ping: Jeff, is this patch approved?
>   https://gcc.gnu.org/ml/gcc-patches/2017-04/msg00547.html
>
> On 05/08/2017 08:38 PM, Martin Sebor wrote:
>> On 04/28/2017 12:35 PM, Jeff Law wrote:
>>> On 04/26/2017 11:05 AM, Martin Sebor wrote:
>>>> On 04/24/2017 03:35 PM, Jeff Law wrote:
>>>>> On 04/11/2017 12:57 PM, Martin Sebor wrote:
>>>>>> In a review of my fix for bug 80364 Jakub pointed out that to
>>>>>> determine whether an argument to an integer directive is of
>>>>>> an integer type the gimple-ssa-sprintf pass tests the type code
>>>>>> for equality to INTEGER_TYPE when it should instead be using
>>>>>> INTEGRAL_TYPE_P().  This has the effect of the pass being unable
>>>>>> to use the available range of arguments of enumerated types,
>>>>>> resulting in both false positives and false negatives, and in
>>>>>> some cases, in emitting suboptimal code.
>>>>>>
>>>>>> The attached patch replaces those tests with INTEGRAL_TYPE_P().
>>>>>>
>>>>>> Since this is not a regression I submit it for GCC 8.
>>>>> You might consider using POINTER_TYPE_P as well.
>>>>
>>>> You mean rather than (TREE_CODE (type) == POINTER_TYPE)?  Those
>>>> I believe are vestiges of the %p handling that was removed sometime
>>>> last year, and (unless you are recommending I remove them as part
>>>> of this patch) should probably be removed during the next cleanup.Yes,
>>>> that can be a follow-up cleanup.
>>>
>>> For the future, if you find yourself writing something like
>>> TREE_CODE (TREE_TYPE (x)) == POINTER_TYPE, you're usually going to be
>>> better off using POINTER_TYPE_P (TREE_TYPE (x)).  That allows the code
>>> to work with C++ references as well as C pointers.
>>
>> I'll keep it in mind, thanks.  Should I take this as approval
>> of the patch as is or are there some changes you'd like me to
>> make?
>>
>> Martin
>

[PING #3] [PATCH] handle enumerated types in -Wformat-overflow (PR 80397)

[Martin Sebor]

Jeff, I suspect you're still busy but I'm Looking for approval
to commit the nearly trivial patch below:

   https://gcc.gnu.org/ml/gcc-patches/2017-04/msg00547.html

Martin

On 05/22/2017 08:19 PM, Martin Sebor wrote:
> Ping:
>
>   https://gcc.gnu.org/ml/gcc-patches/2017-04/msg00547.html
>
> On 05/15/2017 09:01 PM, Martin Sebor wrote:
>> Ping: Jeff, is this patch approved?
>>   https://gcc.gnu.org/ml/gcc-patches/2017-04/msg00547.html
>>
>> On 05/08/2017 08:38 PM, Martin Sebor wrote:
>>> On 04/28/2017 12:35 PM, Jeff Law wrote:
>>>> On 04/26/2017 11:05 AM, Martin Sebor wrote:
>>>>> On 04/24/2017 03:35 PM, Jeff Law wrote:
>>>>>> On 04/11/2017 12:57 PM, Martin Sebor wrote:
>>>>>>> In a review of my fix for bug 80364 Jakub pointed out that to
>>>>>>> determine whether an argument to an integer directive is of
>>>>>>> an integer type the gimple-ssa-sprintf pass tests the type code
>>>>>>> for equality to INTEGER_TYPE when it should instead be using
>>>>>>> INTEGRAL_TYPE_P().  This has the effect of the pass being unable
>>>>>>> to use the available range of arguments of enumerated types,
>>>>>>> resulting in both false positives and false negatives, and in
>>>>>>> some cases, in emitting suboptimal code.
>>>>>>>
>>>>>>> The attached patch replaces those tests with INTEGRAL_TYPE_P().
>>>>>>>
>>>>>>> Since this is not a regression I submit it for GCC 8.
>>>>>> You might consider using POINTER_TYPE_P as well.
>>>>>
>>>>> You mean rather than (TREE_CODE (type) == POINTER_TYPE)?  Those
>>>>> I believe are vestiges of the %p handling that was removed sometime
>>>>> last year, and (unless you are recommending I remove them as part
>>>>> of this patch) should probably be removed during the next cleanup.Yes,
>>>>> that can be a follow-up cleanup.
>>>>
>>>> For the future, if you find yourself writing something like
>>>> TREE_CODE (TREE_TYPE (x)) == POINTER_TYPE, you're usually going to be
>>>> better off using POINTER_TYPE_P (TREE_TYPE (x)).  That allows the code
>>>> to work with C++ references as well as C pointers.
>>>
>>> I'll keep it in mind, thanks.  Should I take this as approval
>>> of the patch as is or are there some changes you'd like me to
>>> make?
>>>
>>> Martin
>>
>

Re: [PATCH] handle enumerated types in -Wformat-overflow (PR 80397)

[Jeff Law]

On 05/08/2017 08:38 PM, Martin Sebor wrote:
> On 04/28/2017 12:35 PM, Jeff Law wrote:
>> On 04/26/2017 11:05 AM, Martin Sebor wrote:
>>> On 04/24/2017 03:35 PM, Jeff Law wrote:
>>>> On 04/11/2017 12:57 PM, Martin Sebor wrote:
>>>>> In a review of my fix for bug 80364 Jakub pointed out that to
>>>>> determine whether an argument to an integer directive is of
>>>>> an integer type the gimple-ssa-sprintf pass tests the type code
>>>>> for equality to INTEGER_TYPE when it should instead be using
>>>>> INTEGRAL_TYPE_P().  This has the effect of the pass being unable
>>>>> to use the available range of arguments of enumerated types,
>>>>> resulting in both false positives and false negatives, and in
>>>>> some cases, in emitting suboptimal code.
>>>>>
>>>>> The attached patch replaces those tests with INTEGRAL_TYPE_P().
>>>>>
>>>>> Since this is not a regression I submit it for GCC 8.
>>>> You might consider using POINTER_TYPE_P as well.
>>>
>>> You mean rather than (TREE_CODE (type) == POINTER_TYPE)?  Those
>>> I believe are vestiges of the %p handling that was removed sometime
>>> last year, and (unless you are recommending I remove them as part
>>> of this patch) should probably be removed during the next cleanup.Yes,
>>> that can be a follow-up cleanup.
>>
>> For the future, if you find yourself writing something like
>> TREE_CODE (TREE_TYPE (x)) == POINTER_TYPE, you're usually going to be
>> better off using POINTER_TYPE_P (TREE_TYPE (x)).  That allows the code
>> to work with C++ references as well as C pointers.
> 
> I'll keep it in mind, thanks.  Should I take this as approval
> of the patch as is or are there some changes you'd like me to
> make?
Yes, Jakub's approval still stands.  My comment was more for future
reference.

Removal of the dead code WRT %p handling seems wise, but makes the most
sense as a follow-up.

Sorry for the long delay,

jeff