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.