[PATCH] issue -Wstring-compare in more case (PR 95673)

[PATCH] issue -Wstring-compare in more case (PR 95673)

[Martin Sebor]

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

-Wstring-compare triggers under the same strict conditions as
the strcmp/strncmp call is folded into a constant: only when
all the uses of the result are [in]equality expressions with
zero.  However, even when the call cannot be folded into
a constant because the result is in addition used in other
expressions besides equality to zero, GCC still sets the range
of the result to nonzero.  So in more complex functions where
some of the uses of the same result are in tests for equality
to zero and others in other expressions, the warning fails to
point out the very mistake it's designed to detect.

The attached change enhances the function that determines how
the strcmp/strncmp is used to also make it possible to detect
the mistakes in the multi-use situations.

Tested on x86_64-linux & by building Glibc and Binutils/GDB
and confirming it triggers no new warnings.

Martin

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

PR middle-end/95673 - missing -Wstring-compare for an impossible strncmp test

gcc/ChangeLog:

	PR middle-end/95673
	* tree-ssa-strlen.c (used_only_for_zero_equality): Rename...
	(use_in_zero_equality): ...to this.  Add a default argument.
	(handle_builtin_memcmp): Adjust to the name change above.
	(handle_builtin_string_cmp): Same.
	(maybe_warn_pointless_strcmp): Same.  Pass in an explicit argument.

gcc/testsuite/ChangeLog:

	PR middle-end/95673
	* gcc.dg/Wstring-compare-3.c: New test.

diff --git a/gcc/testsuite/gcc.dg/Wstring-compare-3.c b/gcc/testsuite/gcc.dg/Wstring-compare-3.c
new file mode 100644
index 00000000000..d4d7121dba7
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wstring-compare-3.c
@@ -0,0 +1,106 @@
+/* PR middle-end/95673 - missing -Wstring-compare for an impossible strncmp test
+   { dg-do compile }
+   { dg-options "-O2 -Wall -Wstring-compare -ftrack-macro-expansion=0" } */
+
+typedef __SIZE_TYPE__ size_t;
+
+extern int strcmp (const char*, const char*);
+extern int strncmp (const char*, const char*, size_t);
+
+void sink (int, ...);
+
+extern char a3[3];
+
+int nowarn_strcmp_one_use_ltz (int c)
+{
+  const char *s = c ? "1234" : a3;
+  int n = strcmp (s, "123");
+  return n < 0;
+}
+
+
+int nowarn_strcmp_one_use_eqnz (int c)
+{
+  const char *s = c ? "12345" : a3;
+  int n = strcmp (s, "123");
+  return n == 1;
+}
+
+
+int warn_strcmp_one_use_eqz (int c)
+{
+  const char *s = c ? "123456" : a3;
+  int n = strcmp (s, "123");    // { dg-warning "'strcmp' of a string of length 3 and an array of size 3 evaluates to nonzero" }
+  return n == 0;                // { dg-message "in this expression" }
+}
+
+
+int warn_strcmp_one_use_bang (int c)
+{
+  const char *s = c ? "1234567" : a3;
+  int n = strcmp (s, "123");    // { dg-warning "'strcmp' of a string of length 3 and an array of size 3 evaluates to nonzero" }
+  return !n;                    // { dg-message "in this expression" }
+}
+
+
+int warn_strcmp_one_use_bang_bang (int c)
+{
+  const char *s = c ? "12345678" : a3;
+  int n = strcmp (s, "123");    // { dg-warning "'strcmp' of a string of length 3 and an array of size 3 evaluates to nonzero" }
+  return !!n;                   // { dg-message "in this expression" }
+}
+
+
+_Bool warn_one_use_bool (int c)
+{
+  const char *s = c ? "123456789" : a3;
+  int n = strcmp (s, "123");    // { dg-warning "'strcmp' of a string of length 3 and an array of size 3 evaluates to nonzero" }
+  return (_Bool)n;              // { dg-message "in this expression" }
+}
+
+
+int warn_strcmp_one_use_cond (int c)
+{
+  const char *s = c ? "1234567890" : a3;
+  int n = strcmp (s, "123");    // { dg-warning "'strcmp' of a string of length 3 and an array of size 3 evaluates to nonzero" }
+  return n ? 3 : 5;             // { dg-message "in this expression" }
+}
+
+
+int nowarn_strcmp_multiple_uses (int c)
+{
+  const char *s = c ? "1234" : a3;
+  int n = strcmp (s, "123");
+  sink (n < 0);
+  sink (n > 0);
+  sink (n <= 0);
+  sink (n >= 0);
+  sink (n + 1);
+  return n;
+}
+
+
+int warn_strcmp_multiple_uses (int c)
+{
+  const char *s = c ? "12345" : a3;
+  int n = strcmp (s, "123");    // { dg-warning "'strcmp' of a string of length 3 and an array of size 3 evaluates to nonzero" }
+  sink (n < 0);
+  sink (n > 0);
+  sink (n <= 0);
+  sink (n >= 0);
+  sink (n == 0);                // { dg-message "in this expression" }
+  return n;
+}
+
+
+int warn_strncmp_multiple_uses (int c)
+{
+  const char *s = a3;
+  int n = strncmp (s, "1234", 4); // { dg-warning "'strncmp' of a string of length 4, an array of size 3 and bound of 4 evaluates to nonzero" }
+  sink (n < 0);
+  sink (n > 0);
+  sink (n <= 0);
+  sink (n >= 0);
+  sink (n == 0);                // { dg-message "in this expression" }
+  return n;
+}
diff --git a/gcc/tree-ssa-strlen.c b/gcc/tree-ssa-strlen.c
index 47f537ab210..936b39577b8 100644
--- a/gcc/tree-ssa-strlen.c
+++ b/gcc/tree-ssa-strlen.c
@@ -3982,11 +3982,13 @@ handle_builtin_memset (gimple_stmt_iterator *gsi, bool *zero_write,
   return true;
 }
 
-/* Return a pointer to the first such equality expression if RES is used
-   only in expressions testing its equality to zero, and null otherwise.  */
+/* Return first such statement if RES is used in statements testing its
+   equality to zero, and null otherwise.  If EXCLUSIVE is true, return
+   nonnull if and only RES is used in such expressions exclusively and
+   in none other.  */
 
 static gimple *
-used_only_for_zero_equality (tree res)
+use_in_zero_equality (tree res, bool exclusive = true)
 {
   gimple *first_use = NULL;
 
@@ -3999,6 +4001,7 @@ used_only_for_zero_equality (tree res)
 
       if (is_gimple_debug (use_stmt))
         continue;
+
       if (gimple_code (use_stmt) == GIMPLE_ASSIGN)
 	{
 	  tree_code code = gimple_assign_rhs_code (use_stmt);
@@ -4008,25 +4011,41 @@ used_only_for_zero_equality (tree res)
 	      if ((TREE_CODE (cond_expr) != EQ_EXPR
 		   && (TREE_CODE (cond_expr) != NE_EXPR))
 		  || !integer_zerop (TREE_OPERAND (cond_expr, 1)))
-		return NULL;
+		{
+		  if (exclusive)
+		    return NULL;
+		  continue;
+		}
 	    }
 	  else if (code == EQ_EXPR || code == NE_EXPR)
 	    {
 	      if (!integer_zerop (gimple_assign_rhs2 (use_stmt)))
-		return NULL;
+		{
+		  if (exclusive)
+		    return NULL;
+		  continue;
+		}
             }
-	  else
+	  else if (exclusive)
 	    return NULL;
+	  else
+	    continue;
 	}
       else if (gimple_code (use_stmt) == GIMPLE_COND)
 	{
 	  tree_code code = gimple_cond_code (use_stmt);
 	  if ((code != EQ_EXPR && code != NE_EXPR)
 	      || !integer_zerop (gimple_cond_rhs (use_stmt)))
-	    return NULL;
+	    {
+	      if (exclusive)
+		return NULL;
+	      continue;
+	    }
 	}
+      else if (exclusive)
+	return NULL;
       else
-        return NULL;
+	continue;
 
       if (!first_use)
 	first_use = use_stmt;
@@ -4046,7 +4065,7 @@ handle_builtin_memcmp (gimple_stmt_iterator *gsi)
   gcall *stmt = as_a <gcall *> (gsi_stmt (*gsi));
   tree res = gimple_call_lhs (stmt);
 
-  if (!res || !used_only_for_zero_equality (res))
+  if (!res || !use_in_zero_equality (res))
     return false;
 
   tree arg1 = gimple_call_arg (stmt, 0);
@@ -4308,7 +4327,7 @@ maybe_warn_pointless_strcmp (gimple *stmt, HOST_WIDE_INT bound,
 			     unsigned HOST_WIDE_INT siz)
 {
   tree lhs = gimple_call_lhs (stmt);
-  gimple *use = used_only_for_zero_equality (lhs);
+  gimple *use = use_in_zero_equality (lhs, /* exclusive = */ false);
   if (!use)
     return;
 
@@ -4358,12 +4377,12 @@ maybe_warn_pointless_strcmp (gimple *stmt, HOST_WIDE_INT bound,
 			     stmt, callee, minlen, siz, bound);
     }
 
-  if (warned)
-    {
-      location_t use_loc = gimple_location (use);
-      if (LOCATION_LINE (stmt_loc) != LOCATION_LINE (use_loc))
-	inform (use_loc, "in this expression");
-    }
+  if (!warned)
+    return;
+
+  location_t use_loc = gimple_location (use);
+  if (LOCATION_LINE (stmt_loc) != LOCATION_LINE (use_loc))
+    inform (use_loc, "in this expression");
 }
 
 
@@ -4497,7 +4516,7 @@ handle_builtin_string_cmp (gimple_stmt_iterator *gsi, const vr_values *rvals)
   /* The size of the array in which the unknown string is stored.  */
   HOST_WIDE_INT varsiz = arysiz1 < 0 ? arysiz2 : arysiz1;
 
-  if ((varsiz < 0 || cmpsiz < varsiz) && used_only_for_zero_equality (lhs))
+  if ((varsiz < 0 || cmpsiz < varsiz) && use_in_zero_equality (lhs))
     {
       /* If the known length is less than the size of the other array
 	 and the strcmp result is only used to test equality to zero,

Re: [PATCH] issue -Wstring-compare in more case (PR 95673)

[Martin Sebor]

Ping: https://gcc.gnu.org/pipermail/gcc-patches/2020-October/555225.html

On 9/30/20 6:14 PM, Martin Sebor wrote:
> -Wstring-compare triggers under the same strict conditions as
> the strcmp/strncmp call is folded into a constant: only when
> all the uses of the result are [in]equality expressions with
> zero.  However, even when the call cannot be folded into
> a constant because the result is in addition used in other
> expressions besides equality to zero, GCC still sets the range
> of the result to nonzero.  So in more complex functions where
> some of the uses of the same result are in tests for equality
> to zero and others in other expressions, the warning fails to
> point out the very mistake it's designed to detect.
> 
> The attached change enhances the function that determines how
> the strcmp/strncmp is used to also make it possible to detect
> the mistakes in the multi-use situations.
> 
> Tested on x86_64-linux & by building Glibc and Binutils/GDB
> and confirming it triggers no new warnings.
> 
> Martin

[PING 2][PATCH] issue -Wstring-compare in more case (PR 95673)

[Martin Sebor]

Ping: https://gcc.gnu.org/pipermail/gcc-patches/2020-October/555225.html

On 10/8/20 8:43 AM, Martin Sebor wrote:
> Ping: https://gcc.gnu.org/pipermail/gcc-patches/2020-October/555225.html
> 
> On 9/30/20 6:14 PM, Martin Sebor wrote:
>> -Wstring-compare triggers under the same strict conditions as
>> the strcmp/strncmp call is folded into a constant: only when
>> all the uses of the result are [in]equality expressions with
>> zero.  However, even when the call cannot be folded into
>> a constant because the result is in addition used in other
>> expressions besides equality to zero, GCC still sets the range
>> of the result to nonzero.  So in more complex functions where
>> some of the uses of the same result are in tests for equality
>> to zero and others in other expressions, the warning fails to
>> point out the very mistake it's designed to detect.
>>
>> The attached change enhances the function that determines how
>> the strcmp/strncmp is used to also make it possible to detect
>> the mistakes in the multi-use situations.
>>
>> Tested on x86_64-linux & by building Glibc and Binutils/GDB
>> and confirming it triggers no new warnings.
>>
>> Martin
> 

Re: [PATCH] issue -Wstring-compare in more case (PR 95673)

[Jeff Law]

On 9/30/20 6:14 PM, Martin Sebor via Gcc-patches wrote:
> -Wstring-compare triggers under the same strict conditions as
> the strcmp/strncmp call is folded into a constant: only when
> all the uses of the result are [in]equality expressions with
> zero.  However, even when the call cannot be folded into
> a constant because the result is in addition used in other
> expressions besides equality to zero, GCC still sets the range
> of the result to nonzero.  So in more complex functions where
> some of the uses of the same result are in tests for equality
> to zero and others in other expressions, the warning fails to
> point out the very mistake it's designed to detect.
>
> The attached change enhances the function that determines how
> the strcmp/strncmp is used to also make it possible to detect
> the mistakes in the multi-use situations.
>
> Tested on x86_64-linux & by building Glibc and Binutils/GDB
> and confirming it triggers no new warnings.
>
> Martin
>
> gcc-95673.diff
>
> PR middle-end/95673 - missing -Wstring-compare for an impossible strncmp test
>
> gcc/ChangeLog:
>
> 	PR middle-end/95673
> 	* tree-ssa-strlen.c (used_only_for_zero_equality): Rename...
> 	(use_in_zero_equality): ...to this.  Add a default argument.
> 	(handle_builtin_memcmp): Adjust to the name change above.
> 	(handle_builtin_string_cmp): Same.
> 	(maybe_warn_pointless_strcmp): Same.  Pass in an explicit argument.
>
> gcc/testsuite/ChangeLog:
>
> 	PR middle-end/95673
> 	* gcc.dg/Wstring-compare-3.c: New test.

Please retest on the trunk and if testing is OK this is fine for the trunk.

jeff