Re: [PATCH] fix and improve strlen conditional handling of merged stores (PR 91183, 91294, 91315)

[Jeff Law <law at redhat dot com>]

On 8/9/19 5:42 PM, Martin Sebor wrote:
>>> @@ -3408,7 +3457,13 @@ static bool
>>>       }
>>>           gimple *stmt = SSA_NAME_DEF_STMT (exp);
>>> -      if (gimple_code (stmt) != GIMPLE_PHI)
>>> +      if (gimple_assign_single_p (stmt))
>>> +    {
>>> +      tree rhs = gimple_assign_rhs1 (stmt);
>>> +      return count_nonzero_bytes (rhs, offset, nbytes, lenrange,
>>> nulterm,
>>> +                      allnul, allnonnul, snlim);
>>> +    }
>>> +      else if (gimple_code (stmt) != GIMPLE_PHI)
>>>       return false;
>> What cases are you handling here?  Are there any cases where a single
>> operand expression on the RHS affects the result.  For example, if we've
>> got a NOP_EXPR which zero extends RHS?  Does that change the nonzero
>> bytes in a way that is significant?
>>
>> I'm not opposed to handling single operand objects here, I'm just
>> concerned that we're being very lenient in just stripping away the
>> operator and looking at the underlying object.
> 
> I remember adding the code because of a test failure but not
> the specifics anymore.  No tests fail with it removed so it
> may not be needed.  As you know, I've been juggling a few
> enhancements in this area and copying code between them as
> I need it so it's possible that I copied too much, or that
> some other change has obviated it, or also that the test
> failed somewhere else and I forgot to copy the test along
> with the code  I'll remove it until it's needed.
Let's pull it for now.  If we come across the need again, we can
obviously revisit with a testcase.


> 
>>> @@ -3795,7 +3824,14 @@ handle_store (gimple_stmt_iterator *gsi)
>>>           }
>>>         else
>>>           si->nonzero_chars = build_int_cst (size_type_node, offset);
>>> -      si->full_string_p = full_string_p;
>>> +
>>> +      /* Set FULL_STRING_P only if the length of the strings being
>>> +         written is the same, and clear it if the strings have
>>> +         different lengths.  In the latter case the length stored
>>> +         in si->NONZERO_CHARS becomes the lower bound.
>>> +         FIXME: Handle the upper bound of the length if possible.  */
>>> +      si->full_string_p = full_string_p && lenrange[0] == lenrange[1];
>> So there seems to be a disconnect between the comment and the code.
>>
>> The comment indicates you care about the lengths of the two strings
>> being the same.  But what you're really comparing when the lenrange[0]
>> == lenrange[1] test is that the min and max of RHS are the same.
> 
> The comment tries to make clear that all the arrays on the RHS
> of the assignment must have the same length in order to set
> FULL_STRING_P.  Like here where LENRANGE = { 4, 4, 4 }:
> 
>   void f (char *s)
>   {
>     if (__builtin_strlen (s) != 2)
>       return;
> 
>     *(int*)a = i ? 0x11111111 : 0x22222222;
>   }
> 
> but not here where LENRANGE = { 1, 4, 4 }:
> 
>     *(int*)a = i < 0 ? 0x11111111 : i ? 0x22220022 : 0x33003333;
> 
> If the bounds of the range of lengths of all the strings on
> the RHS are the same they're all the same length.
> 
> I'm open to phrasing it better.
Oh, I think I see what I was missing.  In the case where RHS is a
conditional (or perhaps a SSA_NAME which was set from a PHI) LENRANGE
will have the min/max/# bytes for the RHS was a whole, not just a single
component of the RHS.


>> It generally looks reasonable, so I think we just need to reach a
>> conclusion on the gimple_assign_single_p cases we're trying to handle
>> and the possible mismatch between the comment and the code.
> 
> Do you want me to post another revision with
> the gimple_assign_single_p test removed?
I think remove that hunk, bootstrap, test, commit and post for archival
purposes.  I do not think another round of review is necessary.

jeff