From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 118791 invoked by alias); 1 May 2017 21:10:44 -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 118780 invoked by uid 89); 1 May 2017 21:10:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,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; Mon, 01 May 2017 21:10:42 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0E6BAEF48E; Mon, 1 May 2017 21:10:43 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 0E6BAEF48E Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=law@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 0E6BAEF48E Received: from localhost.localdomain (ovpn-117-12.phx2.redhat.com [10.3.117.12]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8527C5DD71; Mon, 1 May 2017 21:10:42 +0000 (UTC) Subject: Re: PR79715: Special case strcpy/strncpy for dse To: Richard Biener , Prathamesh Kulkarni Cc: Jakub Jelinek , gcc Patches , Martin Sebor References: <20170228101012.GG1849@tucnak> <6844ba90-61e4-356b-0fb2-fa30693771d6@redhat.com> <696b15b2-c14c-a1af-c682-a7490eae804b@redhat.com> From: Jeff Law Message-ID: <9b7b1dd5-cd8b-c932-8be1-acf415312f99@redhat.com> Date: Mon, 01 May 2017 21:10:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2017-05/txt/msg00041.txt.bz2 On 05/01/2017 12:17 PM, Richard Biener wrote: > On April 28, 2017 8:14:56 PM GMT+02:00, Jeff Law wrote: >> On 04/24/2017 03:05 AM, Prathamesh Kulkarni wrote: >>> On 1 March 2017 at 13:24, Richard Biener wrote: >>>> On Tue, 28 Feb 2017, Jeff Law wrote: >>>> >>>>> On 02/28/2017 05:59 AM, Prathamesh Kulkarni wrote: >>>>>> On 28 February 2017 at 15:40, Jakub Jelinek >> wrote: >>>>>>> On Tue, Feb 28, 2017 at 03:33:11PM +0530, Prathamesh Kulkarni >> wrote: >>>>>>>> Hi, >>>>>>>> The attached patch adds special-casing for strcpy/strncpy to dse >> pass. >>>>>>>> Bootstrapped+tested on x86_64-unknown-linux-gnu. >>>>>>>> OK for GCC 8 ? >>>>>>> What is special on strcpy/strncpy? Unlike memcpy/memmove/memset, >> you >>>>>>> don't >>>>>>> know the length they store (at least not in general), you don't >> know the >>>>>>> value, all you know is that they are for the first argument plain >> store >>>>>>> without remembering the pointer or anything based on it anywhere >> except in >>>>>>> the return value. >>>>>>> I believe stpcpy, stpncpy, strcat, strncat at least have the same >>>>>>> behavior. >>>>>>> On the other side, without knowing the length of the store, you >> can't >>>>>>> treat >>>>>>> it as killing something (ok, some calls like strcpy or stpcpy >> store at >>>>>>> least >>>>>>> the first byte). >>>>>> Well, I assumed a store to dest by strcpy (and friends), which >> gets >>>>>> immediately freed would count >>>>>> as a dead store since free would kill the whole memory block >> pointed >>>>>> to by dest ? >>>>> Yes. But does it happen often in practice? I wouldn't mind >> exploring this >>>>> for gcc-8, but I'd like to see real-world code where this happens. >>>> Actually I don't mind for "real-world code" - the important part is >>>> that I believe it's reasonable to assume it can happen from some C++ >>>> abstraction and optimization. >>> Hi, >>> I have updated the patch to include stp[n]cpy and str[n]cat. >>> In initialize_ao_ref_for_dse for strncat, I suppose for strncat we >>> need to treat size as NULL_TREE >>> instead of setting it 2nd arg since we cannot know size (in general) >>> after strncat ? >> The problem isn't the size, strncat will write the appropriate number >> of >> characters, just like strncpy, stpncpy. The problem is that we don't >> know where the write will start. I'll touch further on this. >> >> >>> Patch passes bootstrap+test on x86_64-unknown-linux-gnu. >>> Cross-tested on arm*-*-*, aarch64*-*-*. >>> >>> Thanks, >>> Prathamesh >>>> Richard. >>> >>> pr79715-2.txt >>> >>> >>> 2017-04-24 Prathamesh Kulkarni >>> >>> * tree-ssa-dse.c (initialize_ao_ref_for_dse): Add cases for >>> BUILT_IN_STRNCPY, BUILT_IN_STRCPY, BUILT_IN_STPNCPY, >> BUILT_IN_STPCPY, >>> BUILT_IN_STRNCAT, BUILT_IN_STRCAT. >>> (maybe_trim_memstar_call): Likewise. >>> (dse_dom_walker::dse_optimize_stmt): Likewise. >>> >>> testsuite/ >>> * gcc.dg/tree-ssa/pr79715.c: New test. >> I'd still be surprised if this kind of think happens in the real world, >> >> even with layers of abstraction & templates. >> >> >> >>> diff --git a/gcc/tree-ssa-dse.c b/gcc/tree-ssa-dse.c >>> index 90230ab..752b2fa 100644 >>> --- a/gcc/tree-ssa-dse.c >>> +++ b/gcc/tree-ssa-dse.c >>> @@ -92,15 +92,24 @@ initialize_ao_ref_for_dse (gimple *stmt, ao_ref >> *write) >>> /* It's advantageous to handle certain mem* functions. */ >>> if (gimple_call_builtin_p (stmt, BUILT_IN_NORMAL)) >>> { >>> + tree size = NULL_TREE; >>> switch (DECL_FUNCTION_CODE (gimple_call_fndecl (stmt))) >>> { >>> case BUILT_IN_MEMCPY: >>> case BUILT_IN_MEMMOVE: >>> case BUILT_IN_MEMSET: >>> + case BUILT_IN_STRNCPY: >>> + case BUILT_IN_STRCPY: >>> + case BUILT_IN_STPNCPY: >>> + case BUILT_IN_STPCPY: >>> { >>> - tree size = NULL_TREE; >>> if (gimple_call_num_args (stmt) == 3) >>> size = gimple_call_arg (stmt, 2)This part seems reasonable. We >> know the size for strncpy, stpncpy which >> we extract from argument #3. For strcpy and stpcpy we'd have NULL for >> the size which is perfect. In all 4 cases the write starts at offset 0 >> >> in the destination string. >> . >> >> >> >>> + } >>> + /* fallthrough. */ >>> + case BUILT_IN_STRCAT: >>> + case BUILT_IN_STRNCAT: >>> + { >>> tree ptr = gimple_call_arg (stmt, 0); >>> ao_ref_init_from_ptr_and_size (write, ptr, size); >> For str[n]cat, I think this is going to result in WRITE not accurately >> reflecting what bytes are going to be written -- write.offset would >> have >> to account for the length of the destination string. >> >> I don't see a way in the ao_ref structure to indicate that the offset >> of >> a write is unknown. > > You can't make offset entirely unknown but you can use, for strcat, offset zero, size and max_size -1. It matters that the access range is correct. This is similar to how we treat array accesses with variable index. I suspect with a size/maxsize of -1 other code in DSE would probably reject the ao_ref. Though that's probably easier to solve in a way that would allow removal of the dead strcat/strncat calls, but not muck up other things. jeff