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

[Martin Sebor <msebor@gmail.com>]

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.

>
> 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