Re: [PATCH] Make strlen range computations more conservative

[Jeff Law <law at redhat dot com>]

On 08/09/2018 12:27 AM, Richard Biener wrote:
> On August 9, 2018 7:26:19 AM GMT+02:00, Jeff Law <law@redhat.com> wrote:
>> On 07/24/2018 05:18 PM, Bernd Edlinger wrote:
>>> On 07/24/18 23:46, Jeff Law wrote:
>>>> On 07/24/2018 01:59 AM, Bernd Edlinger wrote:
>>>>> Hi!
>>>>>
>>>>> This patch makes strlen range computations more conservative.
>>>>>
>>>>> Firstly if there is a visible type cast from type A to B before
>> passing
>>>>> then value to strlen, don't expect the type layout of B to restrict
>> the
>>>>> possible return value range of strlen.
>>>> Why do you think this is the right thing to do?  ie, is there
>> language
>>>> in the standards that makes you think the code as it stands today is
>>>> incorrect from a conformance standpoint?  Is there a significant
>> body of
>>>> code that is affected in an adverse way by the current code?  If so,
>>>> what code?
>>>>
>>>>
>>>
>>> I think if you have an object, of an effective type A say char[100],
>> then
>>> you can cast the address of A to B, say typedef char (*B)[2] for
>> instance
>>> and then to const char *, say for use in strlen.  I may be wrong, but
>> I think
>>> that we should at least try not to pick up char[2] from B, but
>> instead
>>> use A for strlen ranges, or leave this range open.  Currently the
>> range
>>> info for strlen is [0..1] in this case, even if we see the type cast
>>> in the generic tree.
>> Coming back to this... I'd like to hope we can depend on the type of
>> the
>> strlen argument.  Obviously if it's a char *, we know nothing.  But if
>> it's an ARRAY_TYPE it'd be advantageous if we could use the array
>> bounds
>> to bound the length.
> 
>  But the FE made it char * and only because pointer type conversions are useless we may see sth else. So we cannot use the type of the argument. 
I must have missed something along the line -- I thought I saw array
types passed directly without converting to a char *.  But as I noted
later, we still have things like forwprop that may convert pointer
arithmetic/access to arrays.  So I think we have to give up on using the
types for anything that affects code generation.  I think that's
consistent with your and Jakub's position.

I think we do want to use types to drive warnings though.  They help us
eliminate meaningful numbers of false positives.  There may be oddball
missed warnings or false positives because of the actions of forwprop,
but we should see far fewer issues if we rely on types for warnings.


> 
>>
>> *But* we have code which will turn pointer access into array indexing.
>> tree-ssa-forwprop.c can do that, there may be others.  So if we
>> originally had a char * argument to strlen, but forwprop changed it
>> into
>> a char[N] type, then have we just broken things?  I'd totally forgotten
>> about this behavior from forwprop.  PR 46393 contains sample code where
>> this happens.
> 
> If we really still have this it must be very constrained. Because we used to have sorts of wrong code issues with this and data dependence analysis. 
Yea, it's still in there.  I verified it during the gcc-8 cycle since
the BZ is a regression.   The general forms of the transformation is
dicussed in a comment at the top of the file.   Given all that we have
discussed in this thread this code could well be a ticking time bomb for
wrong code issues.

What's scary to me is git blame says I wrote that code.  I certainly
remember writing bits of tree-ssa-forwprop, but don't recall writing
this specific transformation.


> 
>> I also thought we had code to recover array indexing from pointer
>> arithmetic in the C/C++ front-end, but I can't seem to find it tonight.
>> But it would raise similar concerns.
> 
> I've removed that. What I did at some point is avoid decaying too much array to pointer conversions in the FEs to preserve array refs instead of trying to reconstruct them late. 
THat explains why I couldn't find it :-)


Jeff