From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 66636 invoked by alias); 9 Jul 2018 14:36:41 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 66580 invoked by uid 89); 9 Jul 2018 14:36:40 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy=dom, responses, 3228, msebor@gmail.com X-HELO: mail-oi0-f66.google.com Received: from mail-oi0-f66.google.com (HELO mail-oi0-f66.google.com) (209.85.218.66) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 09 Jul 2018 14:36:37 +0000 Received: by mail-oi0-f66.google.com with SMTP id y207-v6so36268948oie.13 for ; Mon, 09 Jul 2018 07:36:37 -0700 (PDT) MIME-Version: 1.0 References: <5bbc3729-6c5e-3aa4-556a-f56ee4636e63@redhat.com> <31232f12-6903-3a3a-2a10-58f6b4f68586@gmail.com> In-Reply-To: <31232f12-6903-3a3a-2a10-58f6b4f68586@gmail.com> From: Aldy Hernandez Date: Mon, 09 Jul 2018 14:36:00 -0000 Message-ID: Subject: Re: [PATCH] add support for strnlen (PR 81384) To: Martin Sebor Cc: Jeff Law , gcc-patches Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2018-07/txt/msg00420.txt.bz2 { 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 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