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

Martin Sebor msebor@gmail.com
Tue Sep 18 17:38:00 GMT 2018


On 09/17/2018 05:09 PM, Jeff Law wrote:
> On 9/14/18 4:11 PM, Martin Sebor wrote:
>> On 09/14/2018 03:35 PM, Jeff Law wrote:
>>> On 9/12/18 11:46 AM, Martin Sebor wrote:
>>>> On 08/31/2018 04:07 AM, Richard Biener wrote:
>>>>> On Thu, Aug 30, 2018 at 7:39 PM Martin Sebor <msebor@gmail.com> wrote:
>>>>>>
>>>>>> On 08/30/2018 11:22 AM, Richard Biener wrote:
>>>>>>> On August 30, 2018 6:54:21 PM GMT+02:00, Martin Sebor
>>>>>>> <msebor@gmail.com> wrote:
>>>>>>>> On 08/30/2018 02:35 AM, Richard Biener wrote:
>>>>>>>>> On Thu, Aug 30, 2018 at 2:12 AM Martin Sebor <msebor@gmail.com>
>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>> The attached patch adds code to work harder to determine whether
>>>>>>>>>> the destination of an assignment involving MEM_REF is the same
>>>>>>>>>> as the destination of a prior strncpy call.  The included test
>>>>>>>>>> case demonstrates when this situation comes up.  During ccp,
>>>>>>>>>> dstbase and lhsbase returned by get_addr_base_and_unit_offset()
>>>>>>>>>> end up looking like this:
>>>>>>>>>
>>>>>>>>> "During CCP" means exactly when?  The CCP lattice tracks copies
>>>>>>>>> so CCP should already know that _1 == _8.  I suppose during
>>>>>>>>> substitute_and_fold then?  But that replaces uses before folding
>>>>>>>>> the stmt.
>>>>>>>>
>>>>>>>> Yes, when ccp_finalize() performs the final substitution during
>>>>>>>> substitute_and_fold().
>>>>>>>
>>>>>>> But then you shouldn't need the loop but at most look at the pointer
>>>>>>> SSA Def to get at the non-invariant ADDR_EXPR.
>>>>>>
>>>>>> I don't follow.   Are you suggesting to compare
>>>>>> SSA_NAME_DEF_STMT (dstbase) to SSA_NAME_DEF_STMT (lhsbase) for
>>>>>> equality?  They're not equal.
>>>>>
>>>>> No.
>>>>>
>>>>>> The first loop iterates once and retrieves
>>>>>>
>>>>>>    1.  _8 = &pb_3(D)->a;
>>>>>>
>>>>>> The second loop iterates three times and retrieves:
>>>>>>
>>>>>>    1.  _1 = _9
>>>>>>    2.  _9 = _8
>>>>>>    3.  _8 = &pb_3(D)->a;
>>>>>>
>>>>>> How do I get from _1 to &pb_3(D)->a without iterating?  Or are
>>>>>> you saying to still iterate but compare the SSA_NAME_DEF_STMT?
>>>>>
>>>>> I say you should retrieve _8 = &pb_3(D)->a immediately since the
>>>>> copies should be
>>>>> propagated out at this stage.
>>>>
>>>> The warning is issued as the strncpy call is being folded (during
>>>> the dom walk in substitute_and_fold_engine::substitute_and_fold)
>>>> but before the subsequent statements have been folded (during
>>>> the subsequent loop to eliminate statements).  So at the point
>>>> of the strncpy folding the three assignments above are still
>>>> there.
>>>>
>>>> I can't think of a good way to solve this problem that's not
>>>> overly intrusive.  Unless you have some suggestions for how
>>>> to deal with it, is the patch okay as is?
>>> In what pass do you see the the naked copies?  In general those should
>>> have been propagated away.
>>
>> As I said above, this happens during the dom walk in the ccp
>> pass:
> 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?)

Martin



More information about the Gcc-patches mailing list