[PATCH] Make strlen range computations more conservative

[Martin Sebor]

On 08/22/2018 04:34 PM, Jeff Law wrote:
> 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.

Right.  And if these changes extend to sprintf as I expect will
be the case there will be many more adjustments to make to those
tests.  Those tests are quite delicate.

As I think Jeff already implied, I would really prefer to tackle
this work myself, both to make sure that it's done without
compromising existing warnings, and that the future enhancements
we have planned in these areas are made possible without too much
churn.

I expect to be able to get to it after I get back from Cauldron.

Martin