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

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:

   _8 = &pb_3(D)->a;
   _9 = _8;
   _1 = _9;
   strncpy (MEM_REF (&pb_3(D)->a), ...);
   MEM[(struct S *)_1].a[n_7] = 0;

so the loops follow the simple assignments until we get at
the ADDR_EXPR assigned to _8 which is the same as the strncpy
destination.

Tested on x86_64-linux.

Martin

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.

So I'm confused.

>
>    _8 = &pb_3(D)->a;
>    _9 = _8;
>    _1 = _9;
>    strncpy (MEM_REF (&pb_3(D)->a), ...);
>    MEM[(struct S *)_1].a[n_7] = 0;
>
> so the loops follow the simple assignments until we get at
> the ADDR_EXPR assigned to _8 which is the same as the strncpy
> destination.
>
> Tested on x86_64-linux.
>
> Martin

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().

Martin

>
> So I'm confused.
>
>>
>>    _8 = &pb_3(D)->a;
>>    _9 = _8;
>>    _1 = _9;
>>    strncpy (MEM_REF (&pb_3(D)->a), ...);
>>    MEM[(struct S *)_1].a[n_7] = 0;
>>
>> so the loops follow the simple assignments until we get at
>> the ADDR_EXPR assigned to _8 which is the same as the strncpy
>> destination.
>>
>> Tested on x86_64-linux.
>>
>> Martin

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. 

Richard. 

>Martin
>
>>
>> So I'm confused.
>>
>>>
>>>    _8 = &pb_3(D)->a;
>>>    _9 = _8;
>>>    _1 = _9;
>>>    strncpy (MEM_REF (&pb_3(D)->a), ...);
>>>    MEM[(struct S *)_1].a[n_7] = 0;
>>>
>>> so the loops follow the simple assignments until we get at
>>> the ADDR_EXPR assigned to _8 which is the same as the strncpy
>>> destination.
>>>
>>> Tested on x86_64-linux.
>>>
>>> Martin

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.

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?

Martin

>
> Richard.
>
>> Martin
>>
>>>
>>> So I'm confused.
>>>
>>>>
>>>>    _8 = &pb_3(D)->a;
>>>>    _9 = _8;
>>>>    _1 = _9;
>>>>    strncpy (MEM_REF (&pb_3(D)->a), ...);
>>>>    MEM[(struct S *)_1].a[n_7] = 0;
>>>>
>>>> so the loops follow the simple assignments until we get at
>>>> the ADDR_EXPR assigned to _8 which is the same as the strncpy
>>>> destination.
>>>>
>>>> Tested on x86_64-linux.
>>>>
>>>> Martin
>

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.

> Martin
>
> >
> > Richard.
> >
> >> Martin
> >>
> >>>
> >>> So I'm confused.
> >>>
> >>>>
> >>>>    _8 = &pb_3(D)->a;
> >>>>    _9 = _8;
> >>>>    _1 = _9;
> >>>>    strncpy (MEM_REF (&pb_3(D)->a), ...);
> >>>>    MEM[(struct S *)_1].a[n_7] = 0;
> >>>>
> >>>> so the loops follow the simple assignments until we get at
> >>>> the ADDR_EXPR assigned to _8 which is the same as the strncpy
> >>>> destination.
> >>>>
> >>>> Tested on x86_64-linux.
> >>>>
> >>>> Martin
> >
>

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?

Martin

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.

If they're only discovered as copies within the pass where you're trying
to issue the diagnostic, then you'd want to see if the pass has any
internal structures that tell you about equivalences.

Jeff

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:

   substitute_and_fold_dom_walker walker (CDI_DOMINATORS, this);
   walker.walk (ENTRY_BLOCK_PTR_FOR_FN (cfun));

The warning is issued during the walker.walk() call as
strncpy is being folded into memcpy.  The prior assignments are
only propagated later, when the next statement after the strncpy
call is reached.  It happens in
substitute_and_fold_dom_walker::before_dom_children(). So during
the strncpy folding we see the next statement as:

   MEM[(struct S *)_1].a[n_7] = 0;

After the strncpy call is transformed to memcpy, the assignment
above is transformed to

   MEM[(struct S *)_8].a[3] = 0;

>
> If they're only discovered as copies within the pass where you're trying
> to issue the diagnostic, then you'd want to see if the pass has any
> internal structures that tell you about equivalences.

I don't know if this is possible.  I don't see any APIs in
tree-ssa-propagate.h that would let me query the internal data
somehow to find out during folding (when the warning is issued).

Martin

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;

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?


jeff

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

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?)


Jeff

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).

Martin

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.

Jeff
jeff

On Tue, Sep 18, 2018 at 8:58 PM Jeff Law <law@redhat.com> 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?)

Note CCP also propagates copies.

Richard.

>
> Jeff

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?

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.

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.

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.

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.

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.
>

On 9/19/18 8:19 AM, Martin Sebor wrote:
> 
> 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?
Well, given that Wrestrict is already a DOM walk integrating it with
sprintf and strlen would be fairly natural.   In that model we'd come up
with some clever name for the pass.  It'd have an embedded strlen (and
perhaps objsize) analysis.  It's walker would dispatch each statement to
the restrict or sprintf & strlen checker.

> 
> 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.
Given the time lost to the STRING_CST and range issues, agreed.  It's
not going to happen this stage1.

Jeff

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.
>>
>

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.
>>>
>>
>

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.
>>>>
>>>
>>
>

On Fri, Nov 16, 2018 at 4:09 AM Martin Sebor <msebor@gmail.com> wrote:
>
> 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?

So the issue is the next_stmt handling because for the _next_ stmt
we did not yet replace uses with lattice values.  This information
was missing all the time along (and absent from patch context).

I notice the next_stmt handling is incredibly fragile and it doesn't
even check the store you identify as thouching the same object
writes a '\0', nor does it verify the store writes to a position after
the strncpy accessed area (but eventually anywhere is OK...).

So I really wonder why there's the restriction on 1:1 equality of the
base.  That relies on proper CSE (as you saw and tried to work-around
in your patch) and more.

So what I'd do is the attached.  Apply more leeway (and less at the
same time) and restrict it to stores from zero but allow any aliasing
one.  One could build up more precision by, instead of using
ptr_may_alias_ref_p use refs_may_alias_p and build up a ao_ref
for the strncpy destination using ao_ref_from_ptr_and_size, but
I didn't bother to figure out what constraint on len the function
computed up to this point.

The patch fixes the testcase.

Richard.

> 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.
> >>>>
> >>>
> >>
> >
>

On 11/16/18 1:46 AM, Richard Biener wrote:
> On Fri, Nov 16, 2018 at 4:09 AM Martin Sebor <msebor@gmail.com> wrote:
>>
>> 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?
> 
> So the issue is the next_stmt handling because for the _next_ stmt
> we did not yet replace uses with lattice values.  This information
> was missing all the time along (and absent from patch context).
> 
> I notice the next_stmt handling is incredibly fragile and it doesn't
> even check the store you identify as thouching the same object
> writes a '\0', nor does it verify the store writes to a position after
> the strncpy accessed area (but eventually anywhere is OK...).
> 
> So I really wonder why there's the restriction on 1:1 equality of the
> base.  That relies on proper CSE (as you saw and tried to work-around
> in your patch) and more.
> 
> So what I'd do is the attached.  Apply more leeway (and less at the
> same time) and restrict it to stores from zero but allow any aliasing
> one.  One could build up more precision by, instead of using
> ptr_may_alias_ref_p use refs_may_alias_p and build up a ao_ref
> for the strncpy destination using ao_ref_from_ptr_and_size, but
> I didn't bother to figure out what constraint on len the function
> computed up to this point.
So FWIW I threw this into the tester and it had a consistent regression
on one of the stringop truncation tests. I think it was
stringop-truncation-2 (logs lost due to stupidity on my part).  It was
visible on x86_64 native as well as other configurations.

It may will be a viable option once that regression is investigated.

jeff

On 11/16/2018 01:46 AM, Richard Biener wrote:
> On Fri, Nov 16, 2018 at 4:09 AM Martin Sebor <msebor@gmail.com> wrote:
>>
>> 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?
>
> So the issue is the next_stmt handling because for the _next_ stmt
> we did not yet replace uses with lattice values.  This information
> was missing all the time along (and absent from patch context).
>
> I notice the next_stmt handling is incredibly fragile and it doesn't
> even check the store you identify as thouching the same object
> writes a '\0', nor does it verify the store writes to a position after
> the strncpy accessed area (but eventually anywhere is OK...).

Yes, this is being tracked in bug 84396.  I have been planing
to tighten it up to check that it is, in fact, the NUL character
being inserted but other things have been getting in the way (like
trying to fix this bug).

> So I really wonder why there's the restriction on 1:1 equality of the
> base.  That relies on proper CSE (as you saw and tried to work-around
> in your patch) and more.
>
> So what I'd do is the attached.  Apply more leeway (and less at the
> same time) and restrict it to stores from zero but allow any aliasing
> one.  One could build up more precision by, instead of using
> ptr_may_alias_ref_p use refs_may_alias_p and build up a ao_ref
> for the strncpy destination using ao_ref_from_ptr_and_size, but
> I didn't bother to figure out what constraint on len the function
> computed up to this point.
>
> The patch fixes the testcase.

It does, but it also introduces two regressions into the test
suite (false negatives).  The code your patch removes is there
to handle these cases.  I'll look into your suggestion to use
refs_may_alias_p to avoid these regressions.

Martin

PS With the suggested patch GCC fails to detect the following:

   struct A { char str[3]; };

   struct B { struct A a[3]; int i; };
   struct C { struct B b[3]; int i; };

   void f (struct B *p, const char *s)
   {
     // write into p->a[0]:
     __builtin_strncpy (p->a[0].str, s, sizeof p->a[0].str);

     // write into p->a[1]:
     p->a[1].str[sizeof p->a[1].str - 1] = 0;
   }

On Mon, Nov 19, 2018 at 5:27 PM Martin Sebor <msebor@gmail.com> wrote:
>
> On 11/16/2018 01:46 AM, Richard Biener wrote:
> > On Fri, Nov 16, 2018 at 4:09 AM Martin Sebor <msebor@gmail.com> wrote:
> >>
> >> 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?
> >
> > So the issue is the next_stmt handling because for the _next_ stmt
> > we did not yet replace uses with lattice values.  This information
> > was missing all the time along (and absent from patch context).
> >
> > I notice the next_stmt handling is incredibly fragile and it doesn't
> > even check the store you identify as thouching the same object
> > writes a '\0', nor does it verify the store writes to a position after
> > the strncpy accessed area (but eventually anywhere is OK...).
>
> Yes, this is being tracked in bug 84396.  I have been planing
> to tighten it up to check that it is, in fact, the NUL character
> being inserted but other things have been getting in the way (like
> trying to fix this bug).
>
> > So I really wonder why there's the restriction on 1:1 equality of the
> > base.  That relies on proper CSE (as you saw and tried to work-around
> > in your patch) and more.
> >
> > So what I'd do is the attached.  Apply more leeway (and less at the
> > same time) and restrict it to stores from zero but allow any aliasing
> > one.  One could build up more precision by, instead of using
> > ptr_may_alias_ref_p use refs_may_alias_p and build up a ao_ref
> > for the strncpy destination using ao_ref_from_ptr_and_size, but
> > I didn't bother to figure out what constraint on len the function
> > computed up to this point.
> >
> > The patch fixes the testcase.
>
> It does, but it also introduces two regressions into the test
> suite (false negatives).

The patch was only for suggestion and not meant to be complete ....

>  The code your patch removes is there
> to handle these cases.  I'll look into your suggestion to use
> refs_may_alias_p to avoid these regressions.
>
> Martin
>
> PS With the suggested patch GCC fails to detect the following:

eventually fixed with my suggestion to use ao_ref_from_ptr_and_size.

>    struct A { char str[3]; };
>
>    struct B { struct A a[3]; int i; };
>    struct C { struct B b[3]; int i; };
>
>    void f (struct B *p, const char *s)
>    {
>      // write into p->a[0]:
>      __builtin_strncpy (p->a[0].str, s, sizeof p->a[0].str);
>
>      // write into p->a[1]:
>      p->a[1].str[sizeof p->a[1].str - 1] = 0;
>    }