Re: [PATCH] fix powerpc64le bootstrap failure caused by r243661 (PR 78817)

[Jeff Law <law at redhat dot com>]

On 12/16/2016 01:17 PM, Jakub Jelinek wrote:
> On Fri, Dec 16, 2016 at 01:01:13PM -0700, Jeff Law wrote:
> > > Thanks.  Reduced to something like:
> > > int
> > > foo (const char *name)
> > > {
> > >   if (name)
> > >     return 6;
> > >   return __builtin_strlen (name);
> > > }
> > > This is warned about both with Martin's late warning and my after ccp2
> > > warning version.  We should include it in gcc testsuite.
> > I'll note this is an example of a case where Andrew's work would likely help
> > because it allows us to ask for a range of name_XX at the return statement
> > and it'll give us back a range that is constrained by the path(s) which
> > reach the return statement.
> >
> > Contrast to the current VRP work where each SSA_NAME has a range, but that
> > range must be valid for every context in which that SSA_NAME appears.
>
> Well, inside the current VRP it has separate ranges for the different paths
> and actually replaces the name in the strlen argument with NULL during evrp,
> so doesn't suffer from the current VRP limitations.
It'll do that sometimes, but it's not consistent and its a conscious 
design decision (which I may not necessarily agree with, but I'm not 
going to open that can-o-worms).

> Anyway, let's consider the warning from the linux kernel with the closedir,
> I guess it can be simplified to something along the lines of:
> void baz (char *) __attribute__((nonnull));
> char *bar (int);
>
> int
> foo (void)
> {
>   char *p = bar (1);
>   int ret = 0;
>   if (p == 0)
>     {
>       bar (2);
>       bar (3);
>       bar (4);
>       ret = 1;
>       goto out;
>     }
>   bar (5);
>   bar (6);
>   bar (7);
>   bar (8);
>  out:
>   baz (p);
>   if (ret)
>     bar (10);
>   return ret;
> }
>
> Here we jump thread it and with Martin's warning position warn, with
> my patch don't warn.  But if the if (ret) bar (10); is removed, the
> code has the same problem that on the error path p will be NULL, but it is
> not going to be diagnosed.  For -Wmaybe-nonnull we could e.g. look at if
> the argument is a PHI that has NULL at any of the edges; but that doesn't
> cover the above case, because p has just one def and so there will be no
> PHIs.
Yea, and so what if the warning changes if that statement is removed. 
That kind of change is inherent in any late running warning.  But late 
warnings, in general, generate fewer false positives than early warnings.

I don't see this as a reason to summarily reject Martin's work.

This whole discussion highlights the primary reason why I stopped 
working on uninitialized warnings many years ago and focused strictly on 
the optimization side of the question.

Everyone has a different view on what is acceptable and what is not for 
a false positive.  Everyone has a different view on whether or not it is 
acceptable that a warning disappear when seemingly innocent changes are 
made to the source.  Should we warn early and generate more false 
positives, or late, reducing false positives, but missing warnings in 
unreachable code.   There is no single right answer and the debates are 
endless and frustrating as hell.


Jeff