diff mbox series

avoid using strnlen result for late calls to strlen (PR 82604)

Message ID 0997dcae-8ae2-27c0-45e9-fb110ecca2e6@gmail.com
State New
Headers show
Series avoid using strnlen result for late calls to strlen (PR 82604) | expand

Commit Message

Martin Sebor June 18, 2018, 7:15 p.m. UTC
While looking into opportunities to detect strnlen/strlen coding
mistakes (pr86199) I noticed a bug in the strnlen implementation
I committed earlier today that lets a strnlen() result be saved
and used in subsequent calls to strlen() with the same argument.
The attached patch changes the handle_builtin_strlen() function
to discard the strnlen() result unless its bound is greater than
the length of the string.

Martin

Comments

Jeff Law June 22, 2018, 10 p.m. UTC | #1
On 06/18/2018 01:15 PM, Martin Sebor wrote:
> While looking into opportunities to detect strnlen/strlen coding
> mistakes (pr86199) I noticed a bug in the strnlen implementation
> I committed earlier today that lets a strnlen() result be saved
> and used in subsequent calls to strlen() with the same argument.
> The attached patch changes the handle_builtin_strlen() function
> to discard the strnlen() result unless its bound is greater than
> the length of the string.
> 
> Martin
> 
> gcc-86204.diff
> 
> 
> PR tree-optimization/86204 -  wrong strlen result after prior strnlen
> 
> gcc/ChangeLog:
> 
> 	PR tree-optimization/86204
> 	* tree-ssa-strlen.c (handle_builtin_strlen): Avoid storing
> 	a strnlen result if it's less than the length of the string.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR tree-optimization/86204
> 	* gcc.dg/strlenopt-46.c: New test.
OK.  Though I must admit I don't like having variables "bounded" and
"bound" in the same function.  So consider renaming one to avoid future
confusion.

jeff
Martin Sebor June 25, 2018, 8:48 p.m. UTC | #2
On 06/22/2018 04:00 PM, Jeff Law wrote:
> On 06/18/2018 01:15 PM, Martin Sebor wrote:
>> While looking into opportunities to detect strnlen/strlen coding
>> mistakes (pr86199) I noticed a bug in the strnlen implementation
>> I committed earlier today that lets a strnlen() result be saved
>> and used in subsequent calls to strlen() with the same argument.
>> The attached patch changes the handle_builtin_strlen() function
>> to discard the strnlen() result unless its bound is greater than
>> the length of the string.
>>
>> Martin
>>
>> gcc-86204.diff
>>
>>
>> PR tree-optimization/86204 -  wrong strlen result after prior strnlen
>>
>> gcc/ChangeLog:
>>
>> 	PR tree-optimization/86204
>> 	* tree-ssa-strlen.c (handle_builtin_strlen): Avoid storing
>> 	a strnlen result if it's less than the length of the string.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	PR tree-optimization/86204
>> 	* gcc.dg/strlenopt-46.c: New test.
> OK.  Though I must admit I don't like having variables "bounded" and
> "bound" in the same function.  So consider renaming one to avoid future
> confusion.

Done in r262114.

Martin
Rainer Orth June 29, 2018, 12:56 p.m. UTC | #3
Hi Martin,

> While looking into opportunities to detect strnlen/strlen coding
> mistakes (pr86199) I noticed a bug in the strnlen implementation
> I committed earlier today that lets a strnlen() result be saved
> and used in subsequent calls to strlen() with the same argument.
> The attached patch changes the handle_builtin_strlen() function
> to discard the strnlen() result unless its bound is greater than
> the length of the string.

the new test FAILs to link on Solaris 10:

+FAIL: gcc.dg/strlenopt-46.c (test for excess errors)
+UNRESOLVED: gcc.dg/strlenopt-46.c compilation failed to produce executable

Excess errors:
Undefined                       first referenced
 symbol                             in file
strnlen                             /var/tmp//ccyrOY3M.o
ld: fatal: symbol referencing errors. No output written to ./strlenopt-46.exe

	Rainer
Martin Sebor June 29, 2018, 4:40 p.m. UTC | #4
On 06/29/2018 06:56 AM, Rainer Orth wrote:
> Hi Martin,
>
>> While looking into opportunities to detect strnlen/strlen coding
>> mistakes (pr86199) I noticed a bug in the strnlen implementation
>> I committed earlier today that lets a strnlen() result be saved
>> and used in subsequent calls to strlen() with the same argument.
>> The attached patch changes the handle_builtin_strlen() function
>> to discard the strnlen() result unless its bound is greater than
>> the length of the string.
>
> the new test FAILs to link on Solaris 10:
>
> +FAIL: gcc.dg/strlenopt-46.c (test for excess errors)
> +UNRESOLVED: gcc.dg/strlenopt-46.c compilation failed to produce executable
>
> Excess errors:
> Undefined                       first referenced
>  symbol                             in file
> strnlen                             /var/tmp//ccyrOY3M.o
> ld: fatal: symbol referencing errors. No output written to ./strlenopt-46.exe

I forgot that the strlenopt tests don't define their own library
versions of the tested functions like the ones in
the gcc.c-torture/execute/builtins directory do.  I've added one
in r262255 so the test should link on targets without the function
too.  Please let me know if the problem doesn't clear up.

Martin
diff mbox series

Patch

PR tree-optimization/86204 -  wrong strlen result after prior strnlen

gcc/ChangeLog:

	PR tree-optimization/86204
	* tree-ssa-strlen.c (handle_builtin_strlen): Avoid storing
	a strnlen result if it's less than the length of the string.

gcc/testsuite/ChangeLog:

	PR tree-optimization/86204
	* gcc.dg/strlenopt-46.c: New test.

Index: gcc/testsuite/gcc.dg/strlenopt-46.c
===================================================================
--- gcc/testsuite/gcc.dg/strlenopt-46.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/strlenopt-46.c	(working copy)
@@ -0,0 +1,131 @@ 
+/* PR tree-optimization/86204 - wrong strlen result after prior strnlen
+   { dg-do compile }
+   { dg-options "-O2 -Wall" } */
+
+#include "strlenopt.h"
+
+#define NOIPA   __attribute__ ((noipa))
+
+char a[] = "12345";
+
+NOIPA void f0 (void)
+{
+  unsigned n0 = strnlen (a, 0);
+  unsigned n1 = strlen (a);
+
+  if (n0 != 0 || n1 != 5)
+    abort ();
+}
+
+NOIPA void f1 (void)
+{
+  unsigned n0 = strnlen (a, 1);
+  unsigned n1 = strlen (a);
+
+  if (n0 != 1 || n1 != 5)
+    abort ();
+}
+
+NOIPA void f2 (void)
+{
+  unsigned n0 = strnlen (a, 2);
+  unsigned n1 = strlen (a);
+
+  if (n0 != 2 || n1 != 5)
+    abort ();
+}
+
+NOIPA void f3 (void)
+{
+  unsigned n0 = strnlen (a, 3);
+  unsigned n1 = strlen (a);
+
+  if (n0 != 3 || n1 != 5)
+    abort ();
+}
+
+NOIPA void f4 (void)
+{
+  unsigned n0 = strnlen (a, 4);
+  unsigned n1 = strlen (a);
+
+  if (n0 != 4 || n1 != 5)
+    abort ();
+}
+
+NOIPA void f5 (void)
+{
+  unsigned n0 = strnlen (a, 5);
+  unsigned n1 = strlen (a);
+
+  if (n0 != 5 || n1 != 5)
+    abort ();
+}
+
+NOIPA void f6 (void)
+{
+  unsigned n0 = strnlen (a, 6);
+  unsigned n1 = strlen (a);
+
+  if (n0 != 5 || n1 != 5)
+    abort ();
+}
+
+NOIPA void fx (unsigned n)
+{
+  unsigned n0 = strnlen (a, n);
+  unsigned n1 = strlen (a);
+
+  unsigned min = n < 5 ? n : 5;
+  if (n0 != min || n1 != 5)
+    abort ();
+}
+
+NOIPA void g2 (void)
+{
+  strcpy (a, "123");
+  unsigned n0 = strnlen (a, 2);
+  unsigned n1 = strlen (a);
+
+  if (n0 != 2 || n1 != 3)
+    abort ();
+}
+
+NOIPA void g7 (void)
+{
+  strcpy (a, "123");
+  unsigned n0 = strnlen (a, 7);
+  unsigned n1 = strlen (a);
+
+  if (n0 != 3 || n1 != 3)
+    abort ();
+}
+
+NOIPA void gx (unsigned n)
+{
+  strcpy (a, "123");
+  unsigned n0 = strnlen (a, n);
+  unsigned n1 = strlen (a);
+
+  unsigned min = n < 3 ? n : 3;
+  if (n0 != min || n1 != 3)
+    abort ();
+}
+
+int main (void)
+{
+  f0 ();
+  f1 ();
+  f2 ();
+  f3 ();
+  f4 ();
+  f5 ();
+  f6 ();
+  fx (2);
+  fx (7);
+
+  g2 ();
+  g7 ();
+  gx (2);
+  gx (7);
+}
Index: gcc/tree-ssa-strlen.c
===================================================================
--- gcc/tree-ssa-strlen.c	(revision 261705)
+++ gcc/tree-ssa-strlen.c	(working copy)
@@ -1270,8 +1270,19 @@  handle_builtin_strlen (gimple_stmt_iterator *gsi)
 	  rhs = unshare_expr (rhs);
 	  if (!useless_type_conversion_p (TREE_TYPE (lhs), TREE_TYPE (rhs)))
 	    rhs = fold_convert_loc (loc, TREE_TYPE (lhs), rhs);
+
+	  bool bounded = false;
 	  if (bound)
-	    rhs = fold_build2_loc (loc, MIN_EXPR, TREE_TYPE (rhs), rhs, bound);
+	    {
+	      tree new_rhs
+		= fold_build2_loc (loc, MIN_EXPR, TREE_TYPE (rhs), rhs, bound);
+
+	      bounded = (TREE_CODE (new_rhs) != INTEGER_CST
+			 || tree_int_cst_lt (new_rhs, rhs));
+
+	      rhs = new_rhs;
+	    }
+
 	  if (!update_call_from_tree (gsi, rhs))
 	    gimplify_and_update_call_from_tree (gsi, rhs);
 	  stmt = gsi_stmt (*gsi);
@@ -1281,6 +1292,11 @@  handle_builtin_strlen (gimple_stmt_iterator *gsi)
 	      fprintf (dump_file, "into: ");
 	      print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM);
 	    }
+
+	  /* Avoid storing the length for calls to strnlen().  */
+	  if (bounded)
+	    return;
+
 	  if (si != NULL
 	      && TREE_CODE (si->nonzero_chars) != SSA_NAME
 	      && TREE_CODE (si->nonzero_chars) != INTEGER_CST
@@ -1299,6 +1315,7 @@  handle_builtin_strlen (gimple_stmt_iterator *gsi)
     }
   if (SSA_NAME_OCCURS_IN_ABNORMAL_PHI (lhs))
     return;
+
   if (idx == 0)
     idx = new_stridx (src);
   else
@@ -1333,9 +1350,14 @@  handle_builtin_strlen (gimple_stmt_iterator *gsi)
     }
   if (idx)
     {
-      strinfo *si = new_strinfo (src, idx, lhs, true);
-      set_strinfo (idx, si);
-      find_equal_ptrs (src, idx);
+      if (!bound)
+	{
+	  /* Only store the new length information for calls to strlen(),
+	     not for those to strnlen().  */
+	  strinfo *si = new_strinfo (src, idx, lhs, true);
+	  set_strinfo (idx, si);
+	  find_equal_ptrs (src, idx);
+	}
 
       /* For SRC that is an array of N elements, set LHS's range
 	 to [0, min (N, BOUND)].  A constant return value means
@@ -1362,7 +1384,7 @@  handle_builtin_strlen (gimple_stmt_iterator *gsi)
 	      }
 	  }
 
-      if (strlen_to_stridx)
+      if (strlen_to_stridx && !bound)
 	strlen_to_stridx->put (lhs, stridx_strlenloc (idx, loc));
     }
 }