This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] add support for strnlen (PR 81384)


   { dg-do run }
   { do-options "-O2 -fno-tree-strlen" }  */

^^^^ I don't think this is doing anything.

If you look at the test run you can see that -fno-tree-strlen is never
passed (I think you actually mean -fno-optimize-strlen for that
matter).  Also, the builtins.exp harness runs your test for an
assortment of other flags, not just -O2.

This test is failing on my range branch for -Og, because
expand_builtin_strnlen() needs range info:

+  wide_int min, max;
+  enum value_range_type rng = get_range_info (bound, &min, &max);
+  if (rng != VR_RANGE)
+    return NULL_RTX;

but interestingly enough, it seems to be calculated in the sprintf
pass as part of the DOM walk:

      /* First record ranges generated by this statement.  */
      evrp_range_analyzer.record_ranges_from_stmt (stmt, false);

It feels wrong that the sprintf warning pass is generating range info
that you may later depend on at rtl expansion time (and for a totally
unrelated thing-- strlen expansion).

I don't know if this is just a quirk of builtins.exp calling your test
with flags you didn't intend, but the inconsistency could cause
problems in the future.  Errr, or my present ;-).

Would it be too much to ask for you to either fix the flags being
passed down to the test, or better yet, find some non-sprintf
dependent way of calculating range info earlier?

Aldy
On Mon, Jun 18, 2018 at 6:35 PM Martin Sebor <msebor@gmail.com> wrote:
>
> On 06/12/2018 03:11 PM, Jeff Law wrote:
> > On 06/05/2018 03:43 PM, Martin Sebor wrote:
> >> The attached patch adds basic support for handling strnlen
> >> as a built-in function.  It touches the strlen pass where
> >> it folds constant results of the function, and builtins.c
> >> to add simple support for expanding strnlen calls with known
> >> results.  It also changes calls.c to detect excessive bounds
> >> to the function and unsafe calls with arguments declared
> >> attribute nonstring.
> >>
> >> A side-effect of the strlen change I should call out is that
> >> strlen() calls to all zero-length arrays that aren't considered
> >> flexible array members (i.e., internal members or non-members)
> >> are folded into zero.  No warning is issued for such invalid
> >> uses of zero-length arrays but based on the responses to my
> >> question Re: aliasing between internal zero-length-arrays and
> >> other members(*) it sounds like one would be appropriate.
> >> I will see about adding one in a separate patch.
> >>
> >> Martin
> >>
> >> [*] https://gcc.gnu.org/ml/gcc/2018-06/msg00046.html
> >>
> >> gcc-81384.diff
> >>
> >>
> >> PR tree-optimization/81384 - built-in form of strnlen missing
> >>
> >> gcc/ChangeLog:
> >>
> >>      PR tree-optimization/81384
> >>      * builtin-types.def (BT_FN_SIZE_CONST_STRING_SIZE): New.
> >>      * builtins.c (expand_builtin_strnlen): New function.
> >>      (expand_builtin): Call it.
> >>      (fold_builtin_n): Avoid setting TREE_NO_WARNING.
> >>      * builtins.def (BUILT_IN_STRNLEN): New.
> >>      * calls.c (maybe_warn_nonstring_arg): Handle BUILT_IN_STRNLEN.
> >>      Warn for bounds in excess of maximum object size.
> >>      * tree-ssa-strlen.c (maybe_set_strlen_range): Return tree representing
> >>      single-value ranges.  Handle strnlen.
> >>      (handle_builtin_strlen): Handle strnlen.
> >>      (strlen_check_and_optimize_stmt): Same.
> >>
> >> gcc/testsuite/ChangeLog:
> >>
> >>      PR tree-optimization/81384
> >>      * gcc.c-torture/execute/builtins/lib/strnlen.c: New test.
> >>      * gcc.c-torture/execute/builtins/strnlen-lib.c: New test.
> >>      * gcc.c-torture/execute/builtins/strnlen.c: New test.
> >>      * gcc.dg/attr-nonstring-2.c: New test.
> >>      * gcc.dg/attr-nonstring-3.c: New test.
> >>      * gcc.dg/attr-nonstring-4.c: New test.
> >>      * gcc.dg/strlenopt-44.c: New test.
> >>      * gcc.dg/strlenopt.h (strnlen):  Declare.
> >>
> >> diff --git a/gcc/builtin-types.def b/gcc/builtin-types.def
> >> index 5365bef..1f15350 100644
> >> --- a/gcc/builtin-types.def
> >> +++ b/gcc/builtin-types.def
> >> @@ -322,6 +322,8 @@ DEF_FUNCTION_TYPE_2 (BT_FN_STRING_CONST_STRING_INT,
> >>                   BT_STRING, BT_CONST_STRING, BT_INT)
> >>  DEF_FUNCTION_TYPE_2 (BT_FN_STRING_CONST_STRING_SIZE,
> >>                   BT_STRING, BT_CONST_STRING, BT_SIZE)
> >> +DEF_FUNCTION_TYPE_2 (BT_FN_SIZE_CONST_STRING_SIZE,
> >> +                 BT_SIZE, BT_CONST_STRING, BT_SIZE)
> >>  DEF_FUNCTION_TYPE_2 (BT_FN_INT_CONST_STRING_FILEPTR,
> >>                   BT_INT, BT_CONST_STRING, BT_FILEPTR)
> >>  DEF_FUNCTION_TYPE_2 (BT_FN_INT_INT_FILEPTR,
> > I believe Jakub already suggested these change and you ack'd that.
> >
> > You have some hunks which will need updating now that the CHKP/MPX bits
> > are gone.
> >
> > So OK after the cleanups noted above and a fresh bootstrap & regression
> > test cycle.
> >
>
> Done.  I also added documentation for the built-in, reran GCC
> and Glibc tests with no regressions, and committed 261705.
>
> Martin


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]