On 12/16/2016 01:13 AM, Markus Trippelsdorf wrote:
On 2016.12.15 at 19:27 -0700, Martin Sebor wrote:
On 12/14/2016 09:19 PM, Jeff Law wrote:
On 12/14/2016 03:56 PM, Martin Sebor wrote:
The -Wnonnull warning improvement (PR c/17308 - nonnull attribute
not as useful as it could be) causes a couple of false positives
in a powerpc64le bootstrap. The attached fix suppresses them.
It passes bootstrap on powerpc64le but tests are still running.
I couldn't reproduce the bootstrap comparison failure that Segher
mentioned in his comment on the bug. I've gone over the patch
again to see if I could spot what could possibly be behind it
but couldn't really see anything. Jeff, you mentioned attribute
nonnull is exploited by the null pointer optimization. Do you
think it could possibly have this effect in GCC?
It's certainly possible. It would be an indicator that something in GCC
is passing a NULL pointer into a routine where it shouldn't at runtime
and that GCC is using its new knowledge to optimize the code assuming
that can't happen.
ie, it's an indicator of a latent bug in GCC. Depending on the
difficulty in tracking down, you may need to revert the change. This is
exactly the kind of scenario I was concerned about in that approval
message.
I couldn't reproduce the comparison error either on powerpc64-linux
or on powerpc64le-linux.
The change to the vec<T, va_heap, vl_ptr> is OK. Can you please verify
that if you install just that change that ppc bootstraps are restored
and if so, please install.
Done.
A profiledbootstrap exposed a few more instances of the enhanced
-Wnonnull warning. I spent some time analyzing them and decided
that three of them are justified (those in gengtype-parse.c and
gengtype.c) and the other three false positives.
The attached patch silences them.
The gengtype code alternates between checking for null pointers
and assuming that the same pointers are not null (and passing nulls
to nonnull functions like libiberty's lbasename). It could probably
benefit from some more cleanup but I'm out of cycles for that.
There are two outstanding instances of -Wnonnull in the profiled-
bootstrap log that both point to the same function that I haven't
figured out yet:
In function ‘void check_function_format(tree, int, tree_node**)’:
cc1plus: warning: argument 1 null where non-null expected [-Wnonnull]
/usr/include/string.h:398:15: note: in a call to function ‘size_t
strlen(const char*)’ declared here
I'll deal with them next but I want to get this patch reviewed
and approved so bootstrap can be restored on the targets affected
by the vec.h warning.
While waiting for builds to finish I also built Binutils, Busybox,
and the Linux kernel to see if the warning shows up there. It does
not.
It does for me with an allmodconf. At -O2 I get three warnings, and at
-O3 I get two additional warnings. Now these additional ones happen way
too deep into the pipeline to be reliable. (For a reduced testcase see:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78817#c8)
The warning looks quite justified in this case. The first call
to sm_read_sector exits with the pointer being null and so the
second call is diagnosed. That seems like a great example of
when the warning is useful.
When you as the author of the warning have difficulties in figuring out
what causes these warnings, what about the average user?
I haven't encountered such difficulties. The instance I pointed
to above in function check_function_format suggests there is a bug
that prevents GCC from pointing to the line that triggers warning
(it reads "cc1plus: warning: argument 1 null"). I just haven't
had time to understand what causes it.
Therefore -fsanitize=undefined should be preferred. It gives you better
info and a backtrace that makes diagnosis easy.
So in my opinion -Wnonnull should warn only for trivial cases.
The sanitizer is a useful and more reliable tool but it has
a number of limitations that put it off limits for many projects
(runtime instrumentation and cost, may not be available for
embedded systems, relies on running the code and exercising all
paths to find bugs).
Warnings have their own limitations (false positives and negatives)
but they are useful as the first line of defense that doesn't suffer
from the sanitizer limitations. They help catch mistakes before
they get committed into the code base and affect other programmers.
That's by far the cheapest way to find bugs. (Sorry if I'm stating
the obvious.)
The -Wnonnull implementation in GCC 6 and prior only warns on null
pointer constants, like in memset(NULL, 0, n). It's exceedingly
unlikely that someone will write such code by mistake, and so
the warning is substantially ineffective. That's why several
users over the last decade have requested it be enhanced.
To be honest, I'm surprised that this is proving controversial.
This is not something new. It's an improvement to an existing
warning that people have asked for. It's bound to have some
false positives but so far the rate is far lower than that of
almost all the other warnings on the list I posted across the
four projects I built.
I don't claim it can't be improved but it seems pretty good as
it is already. Among the 6 instances it's found in GCC three
look like real bugs.
Martin
