From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 125229 invoked by alias); 25 Sep 2018 02:16:17 -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 119359 invoked by uid 89); 25 Sep 2018 02:15:59 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 25 Sep 2018 02:15:58 +0000 Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.25]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id A28F9308421A; Tue, 25 Sep 2018 02:15:50 +0000 (UTC) Received: from localhost.localdomain (ovpn-112-5.rdu2.redhat.com [10.10.112.5]) by smtp.corp.redhat.com (Postfix) with ESMTP id F0CE72010D97; Tue, 25 Sep 2018 02:15:48 +0000 (UTC) Subject: Re: [PATCH] Cleanup strcpy/stpcpy no nul warning code To: Bernd Edlinger , "gcc-patches@gcc.gnu.org" , Richard Biener , Martin Sebor References: From: Jeff Law Openpgp: preference=signencrypt Message-ID: Date: Tue, 25 Sep 2018 03:19:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2018-09/txt/msg01391.txt.bz2 On 9/24/18 12:18 PM, Bernd Edlinger wrote: > On 09/24/18 19:48, Jeff Law wrote: >> On 9/16/18 1:58 PM, Bernd Edlinger wrote: >>> Hi, >>> >>> this is a cleanup of the recently added strlen/strcpy/stpcpy >>> no nul warning code. >>> >>> Most importantly it moves the SSA_NAME handling from >>> unterminated_array to string_constant, thereby fixing >>> another round of xfails in the strlen and stpcpy test cases. >>> >>> I need to say that the fix for bug 86622 is relying in >>> type info on the pointer which is probably not safe in >>> GIMPLE in the light of the recent discussion. >>> >>> I had to add two further exceptions, which should >>> be safe in general: that is a pointer arithmentic on a string >>> literal is okay, and arithmetic on a string constant >>> that is exactly the size of the whole DECL, cannot >>> access an adjacent member. >>> >>> >>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu. >>> Is it OK for trunk? >>> >>> >>> Thanks >>> Bernd. >>> >>> >>> patch-cleanup-no-nul.diff >>> >>> gcc: >>> 2018-09-16 Bernd Edlinger >>> >>> * builtins.h (unterminated_array): Remove prototype. >>> * builtins.c (unterminated_array): Simplify. Make static. >>> (expand_builtin_strcpy_args): Don't use TREE_NO_WARNING here. >>> (expand_builtin_stpcpy_1): Remove warn_string_no_nul without effect. >>> * expr.c (string_constant): Handle SSA_NAME. Add more exceptions >>> where pointer arithmetic is safe. >>> * gimple-fold.c (get_range_strlen): Handle nonstr like in c_strlen. >>> (get_max_strlen): Remove the unnecessary mynonstr handling. >>> (gimple_fold_builtin_strcpy): Simplify. >>> (gimple_fold_builtin_stpcpy): Simplify. >>> (gimple_fold_builtin_sprintf): Remove NO_WARNING propagation >>> without effect. >>> (gimple_fold_builtin_strlen): Simplify. >> So my thinking right now is to go forward with the API change to allow >> c_strlen to fill in a structure with relevant tidbits about the string. >> >> That in turn allows us to use simplify unterminated_array in a manner >> similar to what you've done in your patch -- while carrying forward the >> capabilities we need for Martin's nul terminator warnings. This would >> be combined with the expr.c chunks from your patch. >> > > Do you want me to elaborate that idea? No need. I've already got those bits here. > >> However, most of the changes to drop NO_WARNING stuff should be handled >> separately. I don't think they're safe as-is. I'm also pretty sure the >> stpcpy changes in builtins.c aren't correct as-is. >> >> > > Well, I think you must be referring to this: > > @@ -3984,14 +3964,10 @@ expand_builtin_stpcpy_1 (tree exp, rtx target, mac > compile-time, not an expression containing a string. This is > because the latter will potentially produce pessimized code > when used to produce the return value. */ > - tree nonstr = NULL_TREE; > if (!c_getstr (src, NULL) > - || !(len = c_strlen (src, 0, &nonstr, 1))) > + || !(len = c_strlen (src, 0))) > return expand_movstr (dst, src, target, /*endp=*/2); > > - if (nonstr && !TREE_NO_WARNING (exp)) > - warn_string_no_nul (EXPR_LOCATION (exp), "stpcpy", src, nonstr); > - > lenp1 = size_binop_loc (loc, PLUS_EXPR, len, ssize_int (1)); > ret = expand_builtin_mempcpy_args (dst, src, lenp1, > target, exp, /*endp=*/2); > > > My observation is: If that one is necessary and does not only emit some > duplicated warnings, then the test case must be incomplete, at least it did not > regress when this code is removed. I'm pretty sure the test is incomplete. > > Maybe there could a better way than TREE_NO_WARNING to get rid > of the duplicated warnings. > > Maybe it will be best to concentrate the warnings on a single pass, > which means expand will it not be, right? COnceptually getting all the warnings out of the folder is a good start, but insufficient. You'd need to look at the thread for the duplicated warnings due to how the expanders call into each other. jeff