From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 119759 invoked by alias); 16 Nov 2018 03:09:39 -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 119747 invoked by uid 89); 16 Nov 2018 03:09:39 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-2.8 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=lying, ike X-HELO: mail-qk1-f193.google.com Received: from mail-qk1-f193.google.com (HELO mail-qk1-f193.google.com) (209.85.222.193) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 16 Nov 2018 03:09:37 +0000 Received: by mail-qk1-f193.google.com with SMTP id o89so35349860qko.0 for ; Thu, 15 Nov 2018 19:09:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:references:cc:from:message-id:date:user-agent :mime-version:in-reply-to:content-transfer-encoding; bh=l0kdvYg7XDqpjr1AbEXJTLonKZ5IW74TFRxZeRKyA4M=; b=pSwyDoWJq/J/hpkFwXy1SukgqTfCks+mcLArY39ck15d+WqkUTUTRmL6sw5dpIK/Hz GWxds9Q+7CersGqx4YE3EIwQ+7pVvxPYH6qWtEdsPoP0Wp351NkpNVwGZI8YWQq6Tf4J aAJ0j9Zb6i2gB4hgh6iHPkSBobK96UODZQQ9zKx3RSgkXlrz5SOWvmcMRfFsUrdAKLnc Fta1NlX4+VFWBOw3HcGR+kb/IRY13AdC1bLt+Y5cezHPrpspva7HUShgd7gcpa+O6v+o baB18VbXF7wwXuHMlU7fsGAcDHsTm1vPvcu1uvX4W59JeUUKC7A/0b/G//MGUOQjpDFD wBQw== Return-Path: Received: from localhost.localdomain (184-96-239-209.hlrn.qwest.net. [184.96.239.209]) by smtp.gmail.com with ESMTPSA id 24sm28428703qkp.65.2018.11.15.19.09.32 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 15 Nov 2018 19:09:33 -0800 (PST) Subject: [PING #4] [PATCH] look harder for MEM_REF operand equality to avoid -Wstringop-truncation (PR 84561) To: Richard Biener , Jeff Law References: <6c2126fa-32b3-693b-e3da-cf70391710bf@gmail.com> <346c0950-ac9e-b7b4-799f-a8b661f7d55b@gmail.com> <9ca1c7dc-041a-3e15-a191-34d16a68aa2f@gmail.com> <1555c8f2-019a-1164-d1cc-650b78230c63@redhat.com> <26c29db3-6e0c-7df4-c779-cf01e958c654@gmail.com> <51ebb626-7207-edf8-9a70-c1ec525d24f3@gmail.com> <16290c2f-6203-7709-e7ec-1d56de6c20a2@redhat.com> <3911c594-d8f8-d2d5-0d6f-70c62275ce12@redhat.com> <4aa12e16-f1ab-4311-ac4e-0050ce3c9218@gmail.com> <23ca6dca-8887-77ba-03fe-fdbb5639f2dd@gmail.com> <6ea88a1b-849f-a781-6963-72cc205e433f@gmail.com> <6034b402-e7f6-530e-6f0f-da1060ea165f@gmail.com> <7b12ea89-9b24-bf38-a8be-05cb51dbbadd@gmail.com> Cc: GCC Patches From: Martin Sebor Message-ID: Date: Fri, 16 Nov 2018 03:09:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <7b12ea89-9b24-bf38-a8be-05cb51dbbadd@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2018-11/txt/msg01415.txt.bz2 Ping: https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01934.html Do I need to change something in this patch to make it acceptable or should I assume we will leave this bug in GCC unfixed? On 10/31/2018 10:35 AM, Martin Sebor wrote: > Ping: https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01934.html > > On 10/08/2018 03:40 PM, Martin Sebor wrote: >> Ping: https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01934.html >> >> On 10/01/2018 03:30 PM, Martin Sebor wrote: >>> Ping: https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01934.html >>> >>> We have discussed a number of different approaches to moving >>> the warning somewhere else but none is feasible in the limited >>> amount of time remaining in stage 1 of GCC 9. I'd like to >>> avoid the false positive in GCC 9 by using the originally >>> submitted, simple approach and look into the suggested design >>> changes for GCC 10. >>> >>> On 09/21/2018 08:36 AM, Martin Sebor wrote: >>>> On 09/20/2018 03:06 AM, Richard Biener wrote: >>>>> On Wed, Sep 19, 2018 at 4:19 PM Martin Sebor wrote: >>>>>> >>>>>> On 09/18/2018 10:23 PM, Jeff Law wrote: >>>>>>> On 9/18/18 1:46 PM, Martin Sebor wrote: >>>>>>>> On 09/18/2018 12:58 PM, Jeff Law wrote: >>>>>>>>> On 9/18/18 11:12 AM, Martin Sebor wrote: >>>>>>>>> >>>>>>>>>>> My bad. Sigh. CCP doesn't track copies, just constants, so >>>>>>>>>>> there's not >>>>>>>>>>> going to be any data structure you can exploit. And I don't >>>>>>>>>>> think >>>>>>>>>>> there's a value number you can use to determine the two objects >>>>>>>>>>> are the >>>>>>>>>>> same. >>>>>>>>>>> >>>>>>>>>>> Hmm, let's back up a bit, what is does the relevant part of the >>>>>>>>>>> IL look >>>>>>>>>>> like before CCP? Is the real problem here that we have >>>>>>>>>>> unpropagated >>>>>>>>>>> copies lying around in the IL? Hmm, more likely the IL looksl >>>>>>>>>>> ike: >>>>>>>>>>> >>>>>>>>>>> _8 = &pb_3(D)->a; >>>>>>>>>>> _9 = _8; >>>>>>>>>>> _1 = _9; >>>>>>>>>>> strncpy (MEM_REF (&pb_3(D)->a), ...); >>>>>>>>>>> MEM[(struct S *)_1].a[n_7] = 0; >>>>>>>>>> >>>>>>>>>> Yes, that is what the folder sees while the strncpy call is >>>>>>>>>> being transformed/folded by ccp. The MEM_REF is folded just >>>>>>>>>> after the strncpy call and that's when it's transformed into >>>>>>>>>> >>>>>>>>>> MEM[(struct S *)_8].a[n_7] = 0; >>>>>>>>>> >>>>>>>>>> (The assignments to _1 and _9 don't get removed until after >>>>>>>>>> the dom walk finishes). >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> If we were to propagate the copies out we'd at best have: >>>>>>>>>>> >>>>>>>>>>> _8 = &pb_3(D)->a; >>>>>>>>>>> strncpy (MEM_REF (&pb_3(D)->a), ...); >>>>>>>>>>> MEM[(struct S *)_8].a[n_7] = 0; >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Is that in a form you can handle? Or would we also need to >>>>>>>>>>> forward >>>>>>>>>>> propagate the address computation into the use of _8? >>>>>>>>>> >>>>>>>>>> The above works as long as we look at the def_stmt of _8 in >>>>>>>>>> the MEM_REF (we currently don't). That's also what the last >>>>>>>>>> iteration of the loop does. In this case (with _8) it would >>>>>>>>>> be discovered in the first iteration, so the loop could be >>>>>>>>>> replaced by a simple if statement. >>>>>>>>>> >>>>>>>>>> But I'm not sure I understand the concern with the loop. Is >>>>>>>>>> it that we are looping at all, i.e., the cost? Or that ccp >>>>>>>>>> is doing something wrong or suboptimal? (Should have >>>>>>>>>> propagated the value of _8 earlier?) >>>>>>>>> I suspect it's more a concern that things like copies are >>>>>>>>> typically >>>>>>>>> propagated away. So their existence in the IL (and consequently >>>>>>>>> your >>>>>>>>> need to handle them) raises the question "has something else >>>>>>>>> failed to >>>>>>>>> do its job earlier". >>>>>>>>> >>>>>>>>> During which of the CCP passes is this happening? Can we pull the >>>>>>>>> warning out of the folder (even if that means having a distinct >>>>>>>>> warning >>>>>>>>> pass over the IL?) >>>>>>>> >>>>>>>> It happens during the third run of the pass. >>>>>>>> >>>>>>>> The only way to do what you suggest that I could think of is >>>>>>>> to defer the strncpy to memcpy transformation until after >>>>>>>> the warning pass. That was also my earlier suggestion: defer >>>>>>>> both it and the warning until the tree-ssa-strlen pass (where >>>>>>>> the warning is implemented to begin with -- the folder calls >>>>>>>> into it). >>>>>>> If it's happening that late (CCP3) in general, then ISTM we ought >>>>>>> to be >>>>>>> able to get the warning out of the folder. We just have to pick the >>>>>>> right spot. >>>>>>> >>>>>>> warn_restrict runs before fold_all_builtins, but after dom/vrp so we >>>>>>> should have the IL in pretty good shape. That seems like about the >>>>>>> right time. >>>>>>> >>>>>>> I wonder if we could generalize warn_restrict to be a more generic >>>>>>> warning pass over the IL and place it right before fold_builtins. >>>>>> >>>>>> The restrict pass doesn't know about string lengths so it can't >>>>>> handle all the warnings about string built-ins (the strlen pass >>>>>> now calls into it to issue some). The strlen pass does so it >>>>>> could handle most if not all of them (the folder also calls >>>>>> into it to issue some warnings). It would work even better if >>>>>> it were also integrated with the object size pass. >>>>>> >>>>>> We're already working on merging strlen with sprintf. It seems >>>>>> to me that the strlen pass would benefit not only from that but >>>>>> also from integrating with object size and warn-restrict. With >>>>>> that, -Wstringop-overflow could be moved from builtins.c into >>>>>> it as well (and also benefit not only from accurate string >>>>>> lengths but also from the more accurate object size info). >>>>>> >>>>>> What do you think about that? >>>>> >>>>> I think integrating the various "passes" (objectsize is also >>>>> as much a facility as a pass) generally makes sense given >>>>> it might end up improving all of them and reduce code duplication. >>>> >>>> Okay. If Jeff agrees I'll see if I can make it happen for GCC >>>> 10. Until then, does either of you have any suggestions for >>>> a different approach in this patch or is it acceptable with >>>> the loop as is? >>>> >>>> Martin >>>> >>>>> >>>>> Richard. >>>>> >>>>>> >>>>>> Martin >>>>>> >>>>>> PS I don't think I could do more than merger strlen and sprintf >>>>>> before stage 1 ends (if even that much) so this would be a longer >>>>>> term goal. >>>> >>> >> >