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

[Jeff Law <law at redhat dot com>]

On 12/16/2016 11:40 AM, Jakub Jelinek wrote:
> On Fri, Dec 16, 2016 at 07:29:13PM +0100, Markus Trippelsdorf wrote:
> > 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); }
>
> In this case I think the warning is right, if you ever call sm_get_zone,
> it will always invoke UB.
Agreed.

> >  % 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.
>
> This is where you rely on jump threading to duplicate closedir,
> otherwise you don't warn.  If you don't jump thread it (say there are
> a few more statements after closedir that make it not worth it), then you
> don't warn even late.
Right.  This is an inherent issue with path isolation (which is 
performed by jump threading and other passes).  Path isolation can 
easily expose constants that were previously hidden with two paths joined.

Propagation of those constants can then trigger this kind of warning. 
And I personally think that is a good thing.  It's precisely these kind 
of path specific failures that I want GCC to be catching -- because it's 
often hard for humans to see the error, particularly if there's a 
significant amount of visual space between the key points in the code.

And yes, it's lame that additional code before the closedir may hide the 
warning.  That's an artifact of the cost/benefit of path isolation.  But 
that already happens with a slew of our other warnings.  In an ideal 
world we'd have a stronger static analyzer that followed all the paths, 
but that's not where we are today.

I don't see that this kind of scenario inherently affecting the decision 
one way or the other -- it's more about frequency of false positives and 
mitigation in the false positive case in my mind.


jeff