This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Make strlen range computations more conservative
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.
>> gcc.dg/strlenopt-45.c
>
> Even more here.
>
> extern char c;
> if (strnlen (&c, 0) > 0) // fold to false
> abort ();
> if (strnlen (&c, 9) > 0) // likewise
> abort ();
>
>> gcc.dg/strlenopt-48.c
>> gcc.dg/strlenopt-51.c
>
> All the test cases here include constant character arrays of
> known length. I see nothing controversial about any of them.
>
Ah, sorry, a mistake in my changelog entry. The strlenopt-51.c
test case does not need the flag.
I just changed this:
diff -Npur gcc/testsuite/gcc.dg/strlenopt-51.c gcc/testsuite/gcc.dg/strlenopt-51.c
--- gcc/testsuite/gcc.dg/strlenopt-51.c 2018-08-19 17:11:34.000000000 +0200
+++ gcc/testsuite/gcc.dg/strlenopt-51.c 2018-08-22 09:04:53.768302320 +0200
@@ -101,7 +101,7 @@ void test_keep_a9_9 (int i)
{
#undef T
#define T(I) \
- KEEP (strlen (&a9_9[i][I][0]) > (1 + I) % 9); \
+ KEEP (strlen (&a9_9[i][I][0]) > (0 + I) % 9); \
KEEP (strlen (&a9_9[i][I][1]) > (1 + I) % 9); \
KEEP (strlen (&a9_9[i][I][2]) > (2 + I) % 9); \
KEEP (strlen (&a9_9[i][I][3]) > (3 + I) % 9); \
@@ -115,7 +115,7 @@ void test_keep_a9_9 (int i)
}
/* { dg-final { scan-tree-dump-times "strlen" 72 "gimple" } }
- { dg-final { scan-tree-dump-times "strlen" 63 "optimized" } }
+ { dg-final { scan-tree-dump-times "strlen" 72 "optimized" } }
- { dg-final { scan-tree-dump-times "call_made_in_true_branch_on_line_1\[0-9\]\[0-9\]\[0-9\]" 72 "optimized" } }
+ { dg-final { scan-tree-dump-times "call_made_in_true_branch_on_line_1\[0-9\]\[0-9\]\[0-9\]" 81 "optimized" } }
{ dg-final { scan-tree-dump-times "call_made_in_false_branch_on_line_1\[0-9\]\[0-9\]\[0-9\]" 81 "optimized" } } */
... which looks like the patch fixed something here,
although I cannot tell what.
>>
>> I see no way how to fix those, as they test the information flow
>> from the get_range_string to VRP info, which has to go away.
>>
>> For the developing this patch it was fine to tweak the test cases
>> with the compiler flag, but I'd prefer to get rid of them.
>>
>> There is one test that tests a warning, gcc.dg/pr83373.c.
>> I used the flag there, but could as well have simply xfailed that:
>
> "Simply xfailing" tests for warnings would be inappropriate:
> it would cause regressions and make the reporters unhappy.
>
Okay, but that is just one single test case.
Bernd.