diff mbox series

[committed] correct strcmp() == 0 result for unknown strings (PR 92157)

Message ID 1a588e89-1857-c694-114f-65b2ea7b82eb@gmail.com
State New
Headers show
Series [committed] correct strcmp() == 0 result for unknown strings (PR 92157) | expand

Commit Message

Martin Sebor Oct. 18, 2019, 10:27 p.m. UTC 
The optimization to fold (strcmp() == 0) results involving
arrays/strings of unequal size/length has a bug where it is
unprepared for the compute_string_length() function to return
an invalid length as an indication that the length is unknown.
This leads to some strings that are unequal being considered
equal.

The attached patch corrects this handling.  I have committed
it in r277194 with Jeff's okay.

Martin

Comments

Jeff Law Oct. 18, 2019, 10:48 p.m. UTC | #1 
On 10/18/19 4:27 PM, Martin Sebor wrote:
> The optimization to fold (strcmp() == 0) results involving
> arrays/strings of unequal size/length has a bug where it is
> unprepared for the compute_string_length() function to return
> an invalid length as an indication that the length is unknown.
> This leads to some strings that are unequal being considered
> equal.
> 
> The attached patch corrects this handling.  I have committed
> it in r277194 with Jeff's okay.
And just for the record, this was caught by the Fedora tester I spoke
about at Cauldron.  I'm spinning the entire distro against our weekly
snapshots again.

Most of the issues have been package level problems (missing #includes,
narrowing conversions, fortran argument passing, assumptions about the
inliner, etc).   Those are largely under control, leading to...

My focus now is on chasing down the codegen issues and I've contacted a
few folks already with problems in their code.  There'll certainly be
more over time.  I'm starting with the easier to understand problems in
the hopes the tough ones (python & perl interpreters) get fixed along
the way.

The nastiest problem so far is the improved tail call opts from Jakub in
May.  AFAICT they're doing the right thing in the caller which is a good
indication that something is going wrong in the indirect sibling callee
(ugh).

jeff
diff mbox series

Patch

PR tree-optimization/92157 - incorrect strcmp() == 0 result for unknown strings

gcc/testsuite/ChangeLog:

	PR tree-optimization/92157
	* gcc.dg/strlenopt-69.c: Disable test failing due to PR 92155.
	* gcc.dg/strlenopt-87.c: New test.

gcc/ChangeLog:

	PR tree-optimization/92157
	* tree-ssa-strlen.c (handle_builtin_string_cmp): Be prepared for
	compute_string_length to return a negative result.


Index: gcc/testsuite/gcc.dg/strlenopt-69.c
===================================================================
--- gcc/testsuite/gcc.dg/strlenopt-69.c	(revision 277156)
+++ gcc/testsuite/gcc.dg/strlenopt-69.c	(working copy)
@@ -66,11 +66,14 @@  void test_empty_string (void)
   b4[2] = '\0';
   A (0 == strcmp (&a4[2], &b4[2]));
 
+#if 0
+  /* The following isn't handled yet due to PR 92155.  */
   clobber (a4, b4);
 
   memset (a4, 0, sizeof a4);
   memset (b4, 0, sizeof b4);
   A (0 == strcmp (a4, b4));
+#endif
 }
 
 /* Verify that comparison of dynamically created strings with unknown
Index: gcc/testsuite/gcc.dg/strlenopt-87.c
===================================================================
--- gcc/testsuite/gcc.dg/strlenopt-87.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/strlenopt-87.c	(working copy)
@@ -0,0 +1,105 @@ 
+/* PR tree-optimization/92157 - incorrect strcmp() == 0 result for unknown
+   strings​
+   { dg-do run }
+   { dg-options "-O2 -Wall" } */
+
+#include "strlenopt.h"
+
+
+char a2[2], a3[3];
+
+
+static inline __attribute__ ((always_inline)) int
+verify_not_equal (const char *s, const char *t, int x)
+{
+  int n = x < 0 ? strlen (s) : 0 < x ? strlen (t) : strlen (s) + strlen (t);
+
+  if (strcmp (t, s) == 0)
+    abort ();
+
+  return n;
+}
+
+__attribute__ ((noipa)) int test_a2_s (const char *s)
+{
+  return verify_not_equal (a2, s, 0);
+}
+
+__attribute__ ((noipa)) int test_a2_a3 (void)
+{
+  return verify_not_equal (a2, a3, 0);
+}
+
+__attribute__ ((noipa)) int test_a3_a2 (void)
+{
+  return verify_not_equal (a3, a2, 0);
+}
+
+__attribute__ ((noipa)) int test_s_a2 (const char *s)
+{
+  return verify_not_equal (s, a2, 0);
+}
+
+
+__attribute__ ((noipa)) int test_a2_s_1 (const char *s)
+{
+  return verify_not_equal (a2, s, -1);
+}
+
+__attribute__ ((noipa)) int test_a2_a3_1 (void)
+{
+  return verify_not_equal (a2, a3, -1);
+}
+
+__attribute__ ((noipa)) int test_a3_a2_1 (void)
+{
+  return verify_not_equal (a3, a2, -1);
+}
+
+__attribute__ ((noipa)) int test_s_a2_1 (const char *s)
+{
+  return verify_not_equal (s, a2, -1);
+}
+
+
+__attribute__ ((noipa)) int test_a2_s_2 (const char *s)
+{
+  return verify_not_equal (a2, s, +1);
+}
+
+__attribute__ ((noipa)) int test_a2_a3_2 (void)
+{
+  return verify_not_equal (a2, a3, +1);
+}
+
+__attribute__ ((noipa)) int test_a3_a2_2 (void)
+{
+  return verify_not_equal (a3, a2, +1);
+}
+
+__attribute__ ((noipa)) int test_s_a2_2 (const char *s)
+{
+  return verify_not_equal (s, a2, +1);
+}
+
+int main (void)
+{
+  a2[0] = '1';
+  a3[0] = '1';
+  a3[0] = '2';
+
+  test_a2_s ("");
+  test_a2_a3 ();
+  test_a3_a2 ();
+  test_s_a2 ("");
+
+  test_a2_s_1 ("");
+  test_a2_a3_1 ();
+  test_a3_a2_1 ();
+  test_s_a2_1 ("");
+
+  test_a2_s_2 ("");
+  test_a2_a3_2 ();
+  test_a3_a2_2 ();
+  test_s_a2_2 ("");
+}
Index: gcc/tree-ssa-strlen.c
===================================================================
--- gcc/tree-ssa-strlen.c	(revision 277156)
+++ gcc/tree-ssa-strlen.c	(working copy)
@@ -3842,7 +3842,7 @@  handle_builtin_string_cmp (gimple_stmt_iterator *g
   HOST_WIDE_INT arysiz1 = -1, arysiz2 = -1;
 
   if (idx1)
-    cstlen1 = compute_string_length (idx1) + 1;
+    cstlen1 = compute_string_length (idx1);
   else
     arysiz1 = determine_min_objsize (arg1);
 
@@ -3853,7 +3853,7 @@  handle_builtin_string_cmp (gimple_stmt_iterator *g
 
   /* Repeat for the second argument.  */
   if (idx2)
-    cstlen2 = compute_string_length (idx2) + 1;
+    cstlen2 = compute_string_length (idx2);
   else
     arysiz2 = determine_min_objsize (arg2);
 
@@ -3860,6 +3860,14 @@  handle_builtin_string_cmp (gimple_stmt_iterator *g
   if (cstlen2 < 0 && arysiz2 < 0)
     return false;
 
+  if (cstlen1 < 0 && cstlen2 < 0)
+    return false;
+
+  if (cstlen1 >= 0)
+    ++cstlen1;
+  if (cstlen2 >= 0)
+    ++cstlen2;
+
   /* The exact number of characters to compare.  */
   HOST_WIDE_INT cmpsiz = bound < 0 ? cstlen1 < 0 ? cstlen2 : cstlen1 : bound;
   /* The size of the array in which the unknown string is stored.  */