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

[Markus Trippelsdorf <markus at trippelsdorf dot de>]

On 2016.12.16 at 11:07 -0700, Martin Sebor wrote:
> On 12/16/2016 10:27 AM, Jakub Jelinek wrote:
> > On Fri, Dec 16, 2016 at 10:10:00AM -0700, Martin Sebor wrote:
> > > > No.  The first call to sm_read_sector just doesn't exit.  So it is warning
> > > > about dead code.
> > >
> > > If the code is dead then GCC should eliminate it.  With it eliminated
> >
> > There is (especially with jump threading, but not limited to that, other
> > optimizations may result in that too), code that even very smart optimizing
> > compiler isn't able to prove that is dead.
> >
> > > the warning would be gone.  The warning isn't smart and it doesn't
> > > try to be.  It only works with what GCC gives it.  In this case the
> > > dump shows that GCC thinks the code is reachable.  If it isn't that
> > > would seem to highlight a missed optimization opportunity, not a need
> > > to make the warning smarter than the optimizer.
> >
> > No, it highlights that the warning is done in a wrong place where it suffers
> > from too many false positives.
>
> Asserting a claim without providing any data or evidence doesn't make
> it true.  We have seen fewer than 10 instances of it in just one out
> of four sizable projects.  At least three of these instances are
> debatable (as evidenced by the ongoing debate).  That can hardly be
> interpreted as "too many false positives."

So, to be fair a gave Jakub's patch a try and it has exactly the same
issues for the Linux kernel: sometimes the warning only triggers with
-O3, e.g.:

 % cat sm_ftl.i
int a;
void mtd_read_oob(int);
void sm_read_sector(int *buffer) {
  __builtin_memset(buffer, 0, 1);
  mtd_read_oob(a);
}
void sm_get_zone() { sm_read_sector(0); }

 % gcc -c -Wnonnull -O2 sm_ftl.i
 % gcc -c -Wnonnull -O3 sm_ftl.i
sm_ftl.i: In function ‘sm_get_zone’:
sm_ftl.i:4:3: warning: argument 1 null where non-null expected [-Wnonnull]
   __builtin_memset(buffer, 0, 1);
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
sm_ftl.i:4:3: note: in a call to built-in function ‘__builtin_memset’

Also, Jakub's patch doesn't catch the following case:

(tools/perf/util/trace-event-info.c from Linux)
//...
        dir = opendir(path);
        if (!dir) {
                err = -errno;
                pr_debug("can't read directory '%s'", path);
                goto out;
        }
//... other stuff
out:
        closedir(dir);

Whereas Martin's does.

--
Markus