On 08/23/18 00:57, Martin Sebor wrote: > 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. >
None of these test cases are affected. > 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. > Sure, but it is funny, how this patch does not change a single sprintf warning test case. (a previous version did, but that was fixed). > I expect to be able to get to it after I get back from Cauldron. > > Martin
