Re: [PATCH] Make strlen range computations more conservative

[Martin Sebor <msebor at gmail dot com>]

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 ();

> 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.

> 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.

Martin