macrotab.c -Werror

macrotab.c -Werror

[Andrew Cagney]

[second attempt at posting]

Jim,

FYI, I'm seeing:

gdb/macrotab.c:504: warning: `best_depth' might be used uninitialized in
this function
gmake: *** [macrotab.o] Error 1

for the code:

    /* 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) <-- HERE
                {
                  best = result;
                  best_depth = result_depth;
                }
            }
        }

      return best;
    }

enjoy,
Andrew

Re: macrotab.c -Werror

[Daniel Berlin]

On Mon, 13 May 2002, Andrew Cagney wrote:

> [second attempt at posting]
> 
> Jim,
> 
> FYI, I'm seeing:
> 
> gdb/macrotab.c:504: warning: `best_depth' might be used uninitialized in
> this function
> gmake: *** [macrotab.o] Error 1
> 
> for the code:
> 
>     /* 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) <-- HERE

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.
--Dan

Re: macrotab.c -Werror

[Jim Blandy]

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;
              }
          }
      }

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.

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!

Re: macrotab.c -Werror

[Andrew Cagney]

> 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.

See http://sources.redhat.com/gdb/onlinedocs/gdbint_13.html#SEC104

When fixing the original warnings (few were present) a significant 
number of those really were bugs.  Look for Wuninitialized in the 
ChangeLogs.

enjoy,
Andrew

Re: macrotab.c -Werror

[Daniel Berlin]

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+).