[PING #4] [PATCH] look harder for MEM_REF operand equality to avoid -Wstringop-truncation (PR 84561)

[Martin Sebor <msebor@gmail.com>]

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 <msebor@gmail.com> 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.
>>>>
>>>
>>
>