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)


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


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