This is the mail archive of the
gdb-patches@sources.redhat.com
mailing list for the GDB project.
Re: macrotab.c -Werror
- From: Daniel Berlin <dberlin at dberlin dot org>
- To: Jim Blandy <jimb at redhat dot com>
- Cc: Andrew Cagney <ac131313 at cygnus dot com>,<gdb-patches at sources dot redhat dot com>
- Date: Tue, 14 May 2002 16:50:09 -0400 (EDT)
- Subject: Re: macrotab.c -Werror
On 14 May 2002, Jim Blandy wrote:
>
> Daniel Berlin <dberlin@dberlin.org> writes:
> > It's an obvious false positive (!best will be true the first time through,
> > meaning the only time we check best_depth, it's already been set at
> > least once).
> >
> > Here, you can't just initialize best_depth to 0, you have to initialize it
> > to either INT_MAX, or inclusion_depth (result).
> >
> > Sucks.
>
> You're going too fast. Here's the whole loop, for the sake of
> discussion:
>
> /* It's not us. Try all our children, and return the lowest. */
> {
> struct macro_source_file *child;
> struct macro_source_file *best = 0;
> int best_depth;
>
> for (child = source->includes; child; child = child->next_included)
> {
> struct macro_source_file *result
> = macro_lookup_inclusion (child, name);
>
> if (result)
> {
> int result_depth = inclusion_depth (result);
>
> if (! best || result_depth < best_depth)
> {
> best = result;
> best_depth = result_depth;
> }
> }
> }
>
As the third person to point this out, you can be sure i'm aware that the
!best short circuits it.
In fact, i realized it 5 seconds after the email.
> The only reference to `best_depth''s value is in the right operand of
> `||'. That operand will never be evaluated unless `best' is non-zero.
> But `best' is initially zero, and is only assigned along with
> `best_depth'. So `best_depth''s initial value is never used. This
> means:
> - the original code is correct (although the compiler doesn't figure
> that out), and
> - you can initialize it to anything you want, since its initial value
> is never used.
However, it's a bad idea, since with the current unnecessary
initialization, someone could think they could eliminate !best, since
doing so
A. won't cause a warning
B. probably wouldn't cause any very easily noticeable problems (unless we
have tests that fail if it picks the wrong macro depth).
You could also eliminate the !best altogether, by initializing best_depth
to INT_MAX.
>
> I don't actually know how many unnecessary initializations there are
> in GDB to silence the compiler, but it's my impression that the
> compiler's false positive rate for `var might be used uninitialized'
> warnings is low enough that it's still a useful sanity check. So I'm
> happy to add a few unnecessary initializations.
>
> What sucks (a bit) is that every one of those unnecessary
> initializations does end up generating code --- if the compiler could
> tell it was unnecessary, it wouldn't have printed the warning!
Actually, this isn't true necessarily for gcc, but it's offtopic.
Suffice to say:
/buildspace/cfg-branch/gcc/cc1 -O2 test.c -da -fverbose-asm -Wall -fnew-unroll-loops
test.c: In function `main':
test.c:16: warning: control reaches end of non-void function
[dberlin@dberlin gcc]$ /buildspace/cfg-branch/gcc/cc1 -O2 test.c -da -fverbose-asm -Wall
main
test.c: In function `main':
test.c:16: warning: control reaches end of non-void function
test.c:4: warning: `best_depth' might be used uninitialized in this
function
We don't actually eliminate it by unrolling the loop, but we won't get
the warning.
I can show cases that do the other case as well (we do eliminate it, but
get the warning).
But, the fact that we can't currently eliminate it on the mainline
is yet another reason to initialize best_depth to INT_MAX and remove the !best;
It removes a branch.
In fact, it generates code without *any* branches on a p2/p4
(-march/-mcpu=pentiumpro+).