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
