Re: [PATCH] Make strlen range computations more conservative

[Jeff Law <law at redhat dot com>]

On 08/22/2018 11:22 AM, Bernd Edlinger wrote:
> On 08/22/18 18:05, Martin Sebor wrote:
>> On 08/21/2018 10:05 PM, Bernd Edlinger wrote:
>>> On 08/22/18 00:25, Jeff Law wrote:
>>>> On 08/21/2018 02:43 AM, Richard Biener wrote:
>>>>> On Mon, 20 Aug 2018, Bernd Edlinger wrote:
>>>> [ snip. ]
>>>>
>>>>>> Yes, I found some peanuts on my way.
>>>>>>
>>>>>> For instance this fix for PR middle-end/86121 survives bootstrap on
>>>>>> it's own, and fixes one xfail.
>>>>>>
>>>>>> Is it OK for trunk?
>>>>>
>>>>> Yes, that's OK for trunk.
>>>> Agreed.  Seems like a nice independent bugfix and I don't think it
>>>> adversely affects anything else under current discussion.  In fact, not
>>>> folding here makes it easier to warn about incorrect code elsewhere.
>>>>
>>>>>
>>>>>>> Btw, I don't think we want sth like
>>>>>>> flag_assume_zero_terminated_char_arrays or even make it default at
>>>>>>> -Ofast.
>>>>>>>
>>>>>>
>>>>>> Yes, I agree.  Is there a consensus about this?
>>>>>
>>>>> Well, it's my own opinion of course.  Show me a benchmark that
>>>>> improves with -fassume-zero-terminated-char-arrays.  Certainly
>>>>> for security reasons it sounds a dangerous thing (and the documentation
>>>>> needs a more thorough description of what it really means).
>>>> I certainly don't want to see a flag.  We've already got way too many;
>>>> adding another for marginal behavior just seems wrong.
>>>>
>>>>>
>>>>>> If yes, I go ahead and remove that option again.
>>>>>>
>>>>>> BTW: I needed this option in a few test cases, that insist in checking the
>>>>>> optimizer to eliminate stuff, based on the VRP info. (6 +/-1 or so).
>>>>>
>>>>> Any example?
>>>>>
>>>>>> But we can as well remove those test cases.
>>>> Bernd, if there are specific tests that you want to see removed, we
>>>> should discuss them.
>>>>
>>>
>>> The test cases are:
>>> gcc.dg/strlenopt-36.c
>>
>> There are plenty of valid test cases in this test.  For example:
>>
>>    extern char a7[7];
>>    if (strlen (a7) >= 7)   // fold to false
>>      abort ();
>>
>> Even if we wanted to accommodate common definitions the array
>> declarations could be changed to static and the tests would
>> be useful:
>>
>>    static char a7[7];
>>
>> There is no valid program where the if condition could be true.
>>
>>> gcc.dg/strlenopt-40.c
>>
>> There are even more completely uncontroversial test cases here,
>> such as:
>>
>>    if (strlen (i < 0 ? "123" : "4321") > 4)   // fold to false
>>      abort ();
>>
> 
> I see, the trouble is that the test case mixes valid cases with
> cases that depend on type info in GIMPLE.
I believe Martin's point is that there are tests within those files that
are still valid.  We don't want to zap the entire test  unless all the
subtests are invalid.  We need to look at each sub-test and determine if
it's valid or not.

Jeff