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 |
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
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
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
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
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)); } }