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

[Richard Biener <richard dot guenther at gmail dot com>]

On Mon, Jul 9, 2018 at 11:26 PM Martin Sebor <msebor@gmail.com> wrote:
>
> On 07/09/2018 08:36 AM, Aldy Hernandez wrote:
> >    { 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.
>
> I didn't know the harness ignores dg-options specified in these
> tests.  That's surprising and feels like a bug in the harness
> not to complain about it.  The purpose of the test is to verify
> that the strnlen expansion in builtins.c does the right thing
> and it deliberately tries to disable the earlier strlen
> optimizations to make sure the expansion in builtins.c is fully
> exercised.  By not pointing out my mistake the harness effectively
> let me commit a change without making sure it's thoroughly tested
> (I tested it manually before committing the patch but things could
> regress without us noticing).  I'll look into fixing this somehow.
>
> >
> > 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).
>
> Any pass that records ranges for statements will have this
> effect.  The sprintf pass seems to be the first one to make
> use of this utility (and it's not just a warning pass but also
> an optimization pass) but it would be a shame to put it off
> limits to warning-only passes only because it happens to set
> ranges.

As you noted elsewhere warning options shouldn't change code-generation.
This means that ranges may not be set to the IL in case they are only
computed when a warning option is enabled.

Richard.

> >
> > 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?
>
> At the time I wrote the test I didn't realize the statement
> range info was being computed only in the sprintf pass --
> I thought it was done as "a basic service for the greater
> good" by VRP.  It seems that it should be such a service.
>
> Let me look into tweaking the test.
>
> Martin
>
> >
> > 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
>