Uninitialized variable warnings

Uninitialized variable warnings

[Zack Weinberg]

I have gone through all the uninitialized variable warnings in a
bootstrap of current CVS and classified them.  The majority are
the documented case where gcc gets it wrong:

if(flag) var = value;
...
if(flag) use(value);

although there are a number of cases where I can't prove this due to
extreme code obfuscation.  (Who was it wrote all those 6000 line
functions, anyway?)

There are a number of other places where gcc has gotten it wrong, and
a few places where the warning indicates a real bug.  There's also one
worrisome case, where we get it right or not unpredictably:

if(x == THING_1) var = val1;
else if(x == THING_2) var = val2;
else if(x == THING_3) var = val3;
...
else abort();
...
use(var);

or the equivalent

switch(x)
{
  case THING_1: var = val1; break;
  case THING_2: var = val2; break;
  case THING_3: var = val3; break;
  ...
  default: abort();
}
...
use(var);

This is documented to work correctly.

I've added some more test cases to the testsuite.  I will be sending
patches for the real bugs and the documented failures shortly.  If
people could look at the questionable cases, I'd appreciate it.

The list:

-- possibly real bugs --
file		function		variable

combine.c	find_split_point	inner
combine.c	find_split_point	pos
combine.c	find_split_point	unsignedp
integrate.c	subst_constants		op0_mode
regmove.c	fixup_match_1		dst_note
regmove.c	fixup_match_1		insn_const
reload.c	find_reloads		goal_alternative_number
reload.c	find_reloads		goal_earlyclobber

I think these may be real bugs, but the logic is so convoluted that I
can't be sure one way or the other. I'd appreciate people more
familiar with these parts of the compiler looking at them.

-- definitely real bugs --
cp/call.c	add_function_candidate	    convs
graph.c		node_data		    result
java/verify.c	verify_jvm_instructions	    oldpc
objc/objc-act.c	add_objc_string		    chain
objc/objc-act.c	build_keyword_selector	    key_name
objc/objc-act.c	build_message_expr	    sel_name
objc/objc-act.c	build_selector_expr	    selname
objc/objc-act.c	gen_declarator		    str
objc/objc-act.c	generate_protocol_list	    expr_decl
objc/objc-act.c	get_objc_string_decl	    chain
objc/objc-act.c	synth_id_with_class_suffix  string
reload1.c	reload			    is_scalar
varasm.c	const_hash		    hi

The objc ones are all assumptions that a parameter can only have some
limited range of values - which may be true if the data structure is
correct, but gcc can't know that and it isn't especially robust.  The
rest are odd corner cases or oversights.

-- spurious warnings indicating a bug in the compiler --
objc/objc-act.c	generate_protocol_list	plist
optabs.c	emit_cmp_insn		wider_mode
optabs.c	expand_float		libfcn
integrate.c	expand_inline_function	copy
integrate.c	save_for_inline_copying	copy

We get near-identical cases right.  We ought to get these right too.

-- spurious warnings, not the documented case --
java/class.c	build_utf8_ref		field
java/decl.c	init_decl_processing	field
java/verify.c	verify_jvm_instructions	last
java/verify.c	verify_jvm_instructions	prevpc
optabs.c	expand_binop		carry_in
optabs.c	expand_binop		carry_out
reg-stack.c	convert_regs		insn
reload1.c	emit_reload_insns	where

These warnings are spurious but (in most cases) gcc would have to be a
lot cleverer than it is to get them right.  In some cases it would
need to know things about functions other than the one currently being
compiled.

-- probably the documented spurious case --
cse.c		cse_insn		src_eqv_hash
cse.c		cse_insn		src_eqv_in_memory
cse.c		cse_insn		src_eqv_in_struct
cse.c		cse_insn		src_eqv_volatile
loop.c		move_movables		first
reload1.c	choose_reload_regs	mode
reload1.c	emit_reload_insns	store_insn
reload1.c	reload_combine		const_reg
stmt.c		expand_end_case		maxval
stmt.c		expand_end_case		minval
stmt.c		expand_end_case		range
varasm.c	output_constructor	byte
unroll.c	unroll_loop		local_label

Logic in these functions is too convoluted for me to be sure,
but I think these are just the documented spurious warning.  Again,
I'd like someone who understands these sections of the compiler to
review.

-- spurious warnings matching the documented case --
combine.c	simplify_if_then_else	c1
combine.c	simplify_if_then_else	op
combine.c	try_combine		i2_code_number
combine.c	try_combine		other_code_number
cse.c		cse_insn		sets
dbxout.c	dbxout_block		blocknum
fold-const.c	fold (MULT_EXPR)	alt0
fold-const.c	fold (MULT_EXPR)	alt1
fold-const.c	make_range		arg0
fold-const.c	make_range		arg1
function.c	instantiate_virtual_regs_1	offset
genopinit.c	gen_insn		m1
genopinit.c	gen_insn		m2
genopinit.c	gen_insn		op
integrate.c	mark_stores		mode
libgcc2.c	throw_helper		handler_p
libgcc2.c	throw_helper		pc_p
local-alloc.c	block_alloc		r1
loop.c		check_dbra_loop		comparison_val
loop.c		load_mems		end_label
loop.c		recombine_givs		life_end
loop.c		recombine_givs		life_start
loop.c		strength_reduce		increment
objc/objc-act.c	build_selector_translation_table    decl
optabs.c	expand_binop		op0_xhigh
optabs.c	expand_binop		op1_xhigh
reg-stack.c	record_asm_reg_life	clobber_reg
reg-stack.c	subst_asm_stack_regs	clobber_loc
reg-stack.c	subst_asm_stack_regs	clobber_reg
regmove.c	fixup_match_1		set2
reload.c	decompose		base
reload.c	find_valid_class	best_class
sched.c		schedule_block		regs_sometimes_live
sched.c		schedule_block		sometimes_max
splay-tree.c	splay_tree_insert	comparison
stupid.c	stupid_life_analysis	chain
unroll.c	copy_loop_body		copy
unroll.c	unroll_loop		unroll_type
varasm.c	assemble_variable	size_tree

These are uninteresting.

zw

Re: Uninitialized variable warnings

[craig]

>There are a number of other places where gcc has gotten it wrong, and
>a few places where the warning indicates a real bug.  There's also one
>worrisome case, where we get it right or not unpredictably:
>
>if(x == THING_1) var = val1;
>else if(x == THING_2) var = val2;
>else if(x == THING_3) var = val3;
>...
>else abort();
>...
>use(var);

Could you be more precise about "unpredictably"?  Specifically, do you
mean it's unpredictable all the way down to identical chunks of code
(in separate functions, say) compiled a *single* time?  Or across
multiple runs of the exact same compiler?  Or do you just mean it's
unpredictable across compiler targets (which might be explained by
differences in whether abort() is known to have its noreturn attribute)?

        tq vm, (burley)

Re: Uninitialized variable warnings

[Zack Weinberg]

On 15 Mar 1999 10:39:33 -0000, craig@jcb-sc.com wrote:
>>There are a number of other places where gcc has gotten it wrong, and
>>a few places where the warning indicates a real bug.  There's also one
>>worrisome case, where we get it right or not unpredictably:
>>
>>if(x == THING_1) var = val1;
>>else if(x == THING_2) var = val2;
>>else if(x == THING_3) var = val3;
>>...
>>else abort();
>>...
>>use(var);
>
>Could you be more precise about "unpredictably"?  Specifically, do you
>mean it's unpredictable all the way down to identical chunks of code
>(in separate functions, say) compiled a *single* time?  Or across
>multiple runs of the exact same compiler?  Or do you just mean it's
>unpredictable across compiler targets (which might be explained by
>differences in whether abort() is known to have its noreturn attribute)?

I mean that given two similar chunks of code one will get a warning
and the other won't, and the significant difference may seem to be
absolutely irrelevant as far as a human is concerned.  A good example
is the `plist' variable in objc-act.c:generate_protocol_list.  We warn
on that.  Prune everything else out of the function, and the compiler
doesn't warn anymore.

The compiler is consistent across multiple runs on the same code.
Header file issues are a red herring - abort() is hardwired as
noreturn in c-decl.c.

zw

Re: Uninitialized variable warnings

[craig]

>On 15 Mar 1999 10:39:33 -0000, craig@jcb-sc.com wrote:
>>>There are a number of other places where gcc has gotten it wrong, and
>>>a few places where the warning indicates a real bug.  There's also one
>>>worrisome case, where we get it right or not unpredictably:
>>>
>>>if(x == THING_1) var = val1;
>>>else if(x == THING_2) var = val2;
>>>else if(x == THING_3) var = val3;
>>>...
>>>else abort();
>>>...
>>>use(var);
>>
>>Could you be more precise about "unpredictably"?  Specifically, do you
>>mean it's unpredictable all the way down to identical chunks of code
>>(in separate functions, say) compiled a *single* time?  Or across
>>multiple runs of the exact same compiler?  Or do you just mean it's
>>unpredictable across compiler targets (which might be explained by
>>differences in whether abort() is known to have its noreturn attribute)?
>
>I mean that given two similar chunks of code one will get a warning
>and the other won't, and the significant difference may seem to be
>absolutely irrelevant as far as a human is concerned.  A good example
>is the `plist' variable in objc-act.c:generate_protocol_list.  We warn
>on that.  Prune everything else out of the function, and the compiler
>doesn't warn anymore.

Hmm, so it's possible there's an uninitialized-variable bug in the
compiler, but it might just be that the uninitialized-variable analysis
(including all RTL transforms that precede it) is overly sensitive
to things that would normally be thought of as irrelevant to the
issue of whether a particular variable is initialized.

>The compiler is consistent across multiple runs on the same code.

If that hadn't been the case, I'd recommend finding the (very likely)
uninitialized-variable bug ASAP, which is why I asked.

>Header file issues are a red herring - abort() is hardwired as
>noreturn in c-decl.c.

Ah, okay.

I guess your analysis means that there's probably no "obvious" solution
to the problems of unpredictability you're seeing.  But it seems worth
investigating anyway.  (I haven't got a real strong opinion regarding
when the warnings should be given.  One school of thought would be that
they're just for when the compiler detects a possible problem, so the
better it can be at "proving" no possible problem, the fewer warnings
and, probably, the better it can do at optimizing.  Another school of
thought would be that they're for improving code readability, so
programmers don't have to do code-analysis contortions a la a compiler
to figure out whether a variable is actually used before being defined,
which would imply that it's okay for the compiler to warn in provably
"spurious" cases that aren't *easy* to prove.)

        tq vm, (burley)

Re: Uninitialized variable warnings

[Zack Weinberg]

>>>>There are a number of other places where gcc has gotten it wrong, and
>>>>a few places where the warning indicates a real bug.  There's also one
>>>>worrisome case, where we get it right or not unpredictably:
[...]
>>>Could you be more precise about "unpredictably"?
[...]
>>I mean that given two similar chunks of code one will get a warning
>>and the other won't, and the significant difference may seem to be
>>absolutely irrelevant as far as a human is concerned.  A good example
>>is the `plist' variable in objc-act.c:generate_protocol_list.  We warn
>>on that.  Prune everything else out of the function, and the compiler
>>doesn't warn anymore.
>
>Hmm, so it's possible there's an uninitialized-variable bug in the
>compiler, 

Unless we have false _negatives_ in -Wuninitialized, there don't seem
to be any problems with the section of the code (flow - I think) that
generates the warnings.

>but it might just be that the uninitialized-variable analysis
>(including all RTL transforms that precede it) is overly sensitive
>to things that would normally be thought of as irrelevant to the
>issue of whether a particular variable is initialized.

I believe it is CSE which is too sensitive.  Here is a real example.

/* x86, 2.93.12 19990314
   -O  -Wall: warning
   -O2 -Wall: warning
   -O  -Wall -DCASE2: no warning
   -O  -Wall -DCASE3: warning
   -O2 -Wall -DCASE3: no warning */

struct operation {
    short op;
    char rprio;
#ifndef CASE2
    char flags;
#endif
    char unsignedp;
    long value;
};

extern struct operation cpp_lex (void);

void
cpp_parse_expr (void)
{
  int rprio;
  struct operation op;

  for (;;)
    {
      op = cpp_lex ();

      switch (op.op)
	{
	case 0:
	  break;
#ifndef CASE3
	case 1:
	  return;
#endif
	case 2:
	  rprio = 1;
	  break;
	default:
	  return;
	}

      if (op.op == 0)
	return;

      if (rprio != 1)
	abort();
    }
}

This is too complicated for flow to get the uninit warnings right no
matter which case it is.  However, CSE is able to collapse CASE2 down to 

for (;;)
  {
    op = cpp_lex ();
    if (op.op != 2)
      break;
  }

which flow understands.  It can do the same with CASE3, but only if -O2.

Now, the only significant differences in -dj dumps for default and
CASE2 are that CASE2 has (subreg:HI (reg:DI N)) in a few places where
default has (reg:HI N), and the single insn that initializes reg N is
different in mode and offset.  I don't think there's a good reason for
cse to get this wrong.

zw

Re: Uninitialized variable warnings

[Zack Weinberg]

>>>>There are a number of other places where gcc has gotten it wrong, and
>>>>a few places where the warning indicates a real bug.  There's also one
>>>>worrisome case, where we get it right or not unpredictably:
[...]
>>>Could you be more precise about "unpredictably"?
[...]
>>I mean that given two similar chunks of code one will get a warning
>>and the other won't, and the significant difference may seem to be
>>absolutely irrelevant as far as a human is concerned.  A good example
>>is the `plist' variable in objc-act.c:generate_protocol_list.  We warn
>>on that.  Prune everything else out of the function, and the compiler
>>doesn't warn anymore.
>
>Hmm, so it's possible there's an uninitialized-variable bug in the
>compiler, 

Unless we have false _negatives_ in -Wuninitialized, there don't seem
to be any problems with the section of the code (flow - I think) that
generates the warnings.

>but it might just be that the uninitialized-variable analysis
>(including all RTL transforms that precede it) is overly sensitive
>to things that would normally be thought of as irrelevant to the
>issue of whether a particular variable is initialized.

I believe it is CSE which is too sensitive.  Here is a real example.

/* x86, 2.93.12 19990314
   -O  -Wall: warning
   -O2 -Wall: warning
   -O  -Wall -DCASE2: no warning
   -O  -Wall -DCASE3: warning
   -O2 -Wall -DCASE3: no warning */

struct operation {
    short op;
    char rprio;
#ifndef CASE2
    char flags;
#endif
    char unsignedp;
    long value;
};

extern struct operation cpp_lex (void);

void
cpp_parse_expr (void)
{
  int rprio;
  struct operation op;

  for (;;)
    {
      op = cpp_lex ();

      switch (op.op)
	{
	case 0:
	  break;
#ifndef CASE3
	case 1:
	  return;
#endif
	case 2:
	  rprio = 1;
	  break;
	default:
	  return;
	}

      if (op.op == 0)
	return;

      if (rprio != 1)
	abort();
    }
}

This is too complicated for flow to get the uninit warnings right no
matter which case it is.  However, CSE is able to collapse CASE2 down to 

for (;;)
  {
    op = cpp_lex ();
    if (op.op != 2)
      break;
  }

which flow understands.  It can do the same with CASE3, but only if -O2.

Now, the only significant differences in -dj dumps for default and
CASE2 are that CASE2 has (subreg:HI (reg:DI N)) in a few places where
default has (reg:HI N), and the single insn that initializes reg N is
different in mode and offset.  I don't think there's a good reason for
cse to get this wrong.

zw

Re: Uninitialized variable warnings

[craig]

>On 15 Mar 1999 10:39:33 -0000, craig@jcb-sc.com wrote:
>>>There are a number of other places where gcc has gotten it wrong, and
>>>a few places where the warning indicates a real bug.  There's also one
>>>worrisome case, where we get it right or not unpredictably:
>>>
>>>if(x == THING_1) var = val1;
>>>else if(x == THING_2) var = val2;
>>>else if(x == THING_3) var = val3;
>>>...
>>>else abort();
>>>...
>>>use(var);
>>
>>Could you be more precise about "unpredictably"?  Specifically, do you
>>mean it's unpredictable all the way down to identical chunks of code
>>(in separate functions, say) compiled a *single* time?  Or across
>>multiple runs of the exact same compiler?  Or do you just mean it's
>>unpredictable across compiler targets (which might be explained by
>>differences in whether abort() is known to have its noreturn attribute)?
>
>I mean that given two similar chunks of code one will get a warning
>and the other won't, and the significant difference may seem to be
>absolutely irrelevant as far as a human is concerned.  A good example
>is the `plist' variable in objc-act.c:generate_protocol_list.  We warn
>on that.  Prune everything else out of the function, and the compiler
>doesn't warn anymore.

Hmm, so it's possible there's an uninitialized-variable bug in the
compiler, but it might just be that the uninitialized-variable analysis
(including all RTL transforms that precede it) is overly sensitive
to things that would normally be thought of as irrelevant to the
issue of whether a particular variable is initialized.

>The compiler is consistent across multiple runs on the same code.

If that hadn't been the case, I'd recommend finding the (very likely)
uninitialized-variable bug ASAP, which is why I asked.

>Header file issues are a red herring - abort() is hardwired as
>noreturn in c-decl.c.

Ah, okay.

I guess your analysis means that there's probably no "obvious" solution
to the problems of unpredictability you're seeing.  But it seems worth
investigating anyway.  (I haven't got a real strong opinion regarding
when the warnings should be given.  One school of thought would be that
they're just for when the compiler detects a possible problem, so the
better it can be at "proving" no possible problem, the fewer warnings
and, probably, the better it can do at optimizing.  Another school of
thought would be that they're for improving code readability, so
programmers don't have to do code-analysis contortions a la a compiler
to figure out whether a variable is actually used before being defined,
which would imply that it's okay for the compiler to warn in provably
"spurious" cases that aren't *easy* to prove.)

        tq vm, (burley)

Re: Uninitialized variable warnings

[Zack Weinberg]

On 15 Mar 1999 10:39:33 -0000, craig@jcb-sc.com wrote:
>>There are a number of other places where gcc has gotten it wrong, and
>>a few places where the warning indicates a real bug.  There's also one
>>worrisome case, where we get it right or not unpredictably:
>>
>>if(x == THING_1) var = val1;
>>else if(x == THING_2) var = val2;
>>else if(x == THING_3) var = val3;
>>...
>>else abort();
>>...
>>use(var);
>
>Could you be more precise about "unpredictably"?  Specifically, do you
>mean it's unpredictable all the way down to identical chunks of code
>(in separate functions, say) compiled a *single* time?  Or across
>multiple runs of the exact same compiler?  Or do you just mean it's
>unpredictable across compiler targets (which might be explained by
>differences in whether abort() is known to have its noreturn attribute)?

I mean that given two similar chunks of code one will get a warning
and the other won't, and the significant difference may seem to be
absolutely irrelevant as far as a human is concerned.  A good example
is the `plist' variable in objc-act.c:generate_protocol_list.  We warn
on that.  Prune everything else out of the function, and the compiler
doesn't warn anymore.

The compiler is consistent across multiple runs on the same code.
Header file issues are a red herring - abort() is hardwired as
noreturn in c-decl.c.

zw

Re: Uninitialized variable warnings

[craig]

>There are a number of other places where gcc has gotten it wrong, and
>a few places where the warning indicates a real bug.  There's also one
>worrisome case, where we get it right or not unpredictably:
>
>if(x == THING_1) var = val1;
>else if(x == THING_2) var = val2;
>else if(x == THING_3) var = val3;
>...
>else abort();
>...
>use(var);

Could you be more precise about "unpredictably"?  Specifically, do you
mean it's unpredictable all the way down to identical chunks of code
(in separate functions, say) compiled a *single* time?  Or across
multiple runs of the exact same compiler?  Or do you just mean it's
unpredictable across compiler targets (which might be explained by
differences in whether abort() is known to have its noreturn attribute)?

        tq vm, (burley)

Re: Uninitialized variable warnings

[Joern Rennecke]

> I've added some more test cases to the testsuite.  I will be sending
> patches for the real bugs and the documented failures shortly.  If
> people could look at the questionable cases, I'd appreciate it.
> 
> The list:
> 
> -- possibly real bugs --
> file		function		variable
...
> regmove.c	fixup_match_1		dst_note

No bug here.  The use of dst_note depends on (success != 0) .  Success is
initialized to 0, and only set to 1 in a code path where dest_note is
initialized.

> regmove.c	fixup_match_1		insn_const

No bug here, either.  Early in the function, either code is set to NOTE,
or insn_const is set, or the function returns early.
insn_const is never used if code is NOTE.

> reload.c	find_reloads		goal_alternative_number
> reload.c	find_reloads		goal_earlyclobber
> 
> I think these may be real bugs, but the logic is so convoluted that I
> can't be sure one way or the other. I'd appreciate people more
> familiar with these parts of the compiler looking at them.
> 
> -- definitely real bugs --
...
definitely not!

> reload1.c	reload			    is_scalar

is_scalar is always set when reg_equiv_memory_loc[i] is non-zero.
addr is only set when reg_equiv_mem[i] or reg_equiv_address[i] is non-zero.

One of the latter two is always non-zero if reg_equiv_memory_loc[i] is
non-zero (of course the compiler doesn't know about that when compiling
itself, which just goes to show that -Wuninitialized can't work quite
right without solving the halting problem).

is_scalar is only used when addr is set.

> varasm.c	const_hash		    hi

This is pretty straightforward setting hi in both arms of an if-else
or even unconditionally in each of the disjoint lifetimes.

Re: Uninitialized variable warnings

[Zack Weinberg]

On Mon, 15 Mar 1999 21:27:58 +0000 (GMT), Joern Rennecke wrote:
>> -- definitely real bugs --
>...
>definitely not!
>
>> reload1.c	reload			    is_scalar
>
>is_scalar is always set when reg_equiv_memory_loc[i] is non-zero.
>addr is only set when reg_equiv_mem[i] or reg_equiv_address[i] is non-zero.
>
>One of the latter two is always non-zero if reg_equiv_memory_loc[i] is
>non-zero (of course the compiler doesn't know about that when compiling
>itself, which just goes to show that -Wuninitialized can't work quite
>right without solving the halting problem).

I didn't know that either.  Is there a difference between
(reg_equiv_mem[i] || reg_equiv_address[i]) and
reg_equiv_memory_loc[i]?  If not, why is there a separate table?

>> varasm.c	const_hash		    hi
>
>This is pretty straightforward setting hi in both arms of an if-else
>or even unconditionally in each of the disjoint lifetimes.

    case ADDR_EXPR:
      {
        struct addr_const value;

        decode_addr_const (exp, &value);
        if (GET_CODE (value.base) == SYMBOL_REF)
          {
            /* Don't hash the address of the SYMBOL_REF;
               only use the offset and the symbol name.  */
            hi = value.offset;
            p = XSTR (value.base, 0);
            for (i = 0; p[i] != 0; i++)
              hi = ((hi * 613) + (unsigned) (p[i]));
          }
        else if (GET_CODE (value.base) == LABEL_REF)
          hi = value.offset + CODE_LABEL_NUMBER (XEXP (value.base, 0)) * 13;

	hi &= (1 << HASHBITS) - 1;

If GET_CODE (value.base) is not SYMBOL_REF or LABEL_REF, hi is used
unintialized.  Maybe that never happens with correct RTL, but we ought
at least to have an `else abort()' in there.  I considered all the
cases like this to be real bugs.

zw

Re: Uninitialized variable warnings

[Joern Rennecke]

> I didn't know that either.  Is there a difference between
> (reg_equiv_mem[i] || reg_equiv_address[i]) and
> reg_equiv_memory_loc[i]?  If not, why is there a separate table?

reg_equiv_memory_loc gets set first.  later reload decides if to set
reg_equiv_mem or reg_equiv_address.

>         else if (GET_CODE (value.base) == LABEL_REF)
...
> If GET_CODE (value.base) is not SYMBOL_REF or LABEL_REF, hi is used
> unintialized.  Maybe that never happens with correct RTL, but we ought

Oops, I missed the 'if' in the 'else if',

Re: Uninitialized variable warnings

[Joern Rennecke]

> I didn't know that either.  Is there a difference between
> (reg_equiv_mem[i] || reg_equiv_address[i]) and
> reg_equiv_memory_loc[i]?  If not, why is there a separate table?

reg_equiv_memory_loc gets set first.  later reload decides if to set
reg_equiv_mem or reg_equiv_address.

>         else if (GET_CODE (value.base) == LABEL_REF)
...
> If GET_CODE (value.base) is not SYMBOL_REF or LABEL_REF, hi is used
> unintialized.  Maybe that never happens with correct RTL, but we ought

Oops, I missed the 'if' in the 'else if',

Re: Uninitialized variable warnings

[Jeffrey A Law]

  In message < 199903160417.XAA29108@blastula.phys.columbia.edu >you write:
  > >This is pretty straightforward setting hi in both arms of an if-else
  > >or even unconditionally in each of the disjoint lifetimes.
  > 
  >     case ADDR_EXPR:
  >       {
  >         struct addr_const value;
  > 
  >         decode_addr_const (exp, &value);
  >         if (GET_CODE (value.base) == SYMBOL_REF)
  >           {
  >             /* Don't hash the address of the SYMBOL_REF;
  >                only use the offset and the symbol name.  */
  >             hi = value.offset;
  >             p = XSTR (value.base, 0);
  >             for (i = 0; p[i] != 0; i++)
  >               hi = ((hi * 613) + (unsigned) (p[i]));
  >           }
  >         else if (GET_CODE (value.base) == LABEL_REF)
  >           hi = value.offset + CODE_LABEL_NUMBER (XEXP (value.base, 0)) * 13
  > ;
  > 
  > 	hi &= (1 << HASHBITS) - 1;
  > 
  > If GET_CODE (value.base) is not SYMBOL_REF or LABEL_REF, hi is used
  > unintialized.  Maybe that never happens with correct RTL, but we ought
  > at least to have an `else abort()' in there.  I considered all the
  > cases like this to be real bugs.
Adding an else abort () seems quite reasonable to me.  If it ever triggers
we've got a real bug to track down.

These are precisely the kinds of things we need to spend time looking at --
no -W flag will ever know enough about the semantics of this code to determine
whether or not the variable can be used without being set.

jeff

Re: Uninitialized variable warnings

[Jeffrey A Law]

  In message < 199903160417.XAA29108@blastula.phys.columbia.edu >you write:
  > >This is pretty straightforward setting hi in both arms of an if-else
  > >or even unconditionally in each of the disjoint lifetimes.
  > 
  >     case ADDR_EXPR:
  >       {
  >         struct addr_const value;
  > 
  >         decode_addr_const (exp, &value);
  >         if (GET_CODE (value.base) == SYMBOL_REF)
  >           {
  >             /* Don't hash the address of the SYMBOL_REF;
  >                only use the offset and the symbol name.  */
  >             hi = value.offset;
  >             p = XSTR (value.base, 0);
  >             for (i = 0; p[i] != 0; i++)
  >               hi = ((hi * 613) + (unsigned) (p[i]));
  >           }
  >         else if (GET_CODE (value.base) == LABEL_REF)
  >           hi = value.offset + CODE_LABEL_NUMBER (XEXP (value.base, 0)) * 13
  > ;
  > 
  > 	hi &= (1 << HASHBITS) - 1;
  > 
  > If GET_CODE (value.base) is not SYMBOL_REF or LABEL_REF, hi is used
  > unintialized.  Maybe that never happens with correct RTL, but we ought
  > at least to have an `else abort()' in there.  I considered all the
  > cases like this to be real bugs.
Adding an else abort () seems quite reasonable to me.  If it ever triggers
we've got a real bug to track down.

These are precisely the kinds of things we need to spend time looking at --
no -W flag will ever know enough about the semantics of this code to determine
whether or not the variable can be used without being set.

jeff

Re: Uninitialized variable warnings

[Zack Weinberg]

On Mon, 15 Mar 1999 21:27:58 +0000 (GMT), Joern Rennecke wrote:
>> -- definitely real bugs --
>...
>definitely not!
>
>> reload1.c	reload			    is_scalar
>
>is_scalar is always set when reg_equiv_memory_loc[i] is non-zero.
>addr is only set when reg_equiv_mem[i] or reg_equiv_address[i] is non-zero.
>
>One of the latter two is always non-zero if reg_equiv_memory_loc[i] is
>non-zero (of course the compiler doesn't know about that when compiling
>itself, which just goes to show that -Wuninitialized can't work quite
>right without solving the halting problem).

I didn't know that either.  Is there a difference between
(reg_equiv_mem[i] || reg_equiv_address[i]) and
reg_equiv_memory_loc[i]?  If not, why is there a separate table?

>> varasm.c	const_hash		    hi
>
>This is pretty straightforward setting hi in both arms of an if-else
>or even unconditionally in each of the disjoint lifetimes.

    case ADDR_EXPR:
      {
        struct addr_const value;

        decode_addr_const (exp, &value);
        if (GET_CODE (value.base) == SYMBOL_REF)
          {
            /* Don't hash the address of the SYMBOL_REF;
               only use the offset and the symbol name.  */
            hi = value.offset;
            p = XSTR (value.base, 0);
            for (i = 0; p[i] != 0; i++)
              hi = ((hi * 613) + (unsigned) (p[i]));
          }
        else if (GET_CODE (value.base) == LABEL_REF)
          hi = value.offset + CODE_LABEL_NUMBER (XEXP (value.base, 0)) * 13;

	hi &= (1 << HASHBITS) - 1;

If GET_CODE (value.base) is not SYMBOL_REF or LABEL_REF, hi is used
unintialized.  Maybe that never happens with correct RTL, but we ought
at least to have an `else abort()' in there.  I considered all the
cases like this to be real bugs.

zw

Re: Uninitialized variable warnings

[Jeffrey A Law]

  In message < 199903152127.VAA32106@phal.cygnus.co.uk >you write:
  > > I've added some more test cases to the testsuite.  I will be sending
  > > patches for the real bugs and the documented failures shortly.  If
  > > people could look at the questionable cases, I'd appreciate it.
  > > 
  > > The list:
  > > 
  > > -- possibly real bugs --
  > > file		function		variable
  > ...
  > > regmove.c	fixup_match_1		dst_note
  > 
  > No bug here.  The use of dst_note depends on (success != 0) .  Success is
  > initialized to 0, and only set to 1 in a code path where dest_note is
  > initialized.
Agreed.  This _may_ be an example of the cases we may be able to detect via
control dependence analysis.  I've got no problem if someone initializes
dst_note to NULL_RTX.

  > > regmove.c	fixup_match_1		insn_const
  > 
  > No bug here, either.  Early in the function, either code is set to NOTE,
  > or insn_const is set, or the function returns early.
  > insn_const is never used if code is NOTE.
Similarly.  Control dependence analysis may allow us to tackle this one
someday.  Initializing insn_const to NULL_RTX seems reasonable to me.

Can someone work with Joern to deal with the reload issues?

jeff

Re: Uninitialized variable warnings

[Zack Weinberg]

On Wed, 17 Mar 1999 21:38:12 -0700, Jeffrey A Law wrote:
>
>  In message < 199903152127.VAA32106@phal.cygnus.co.uk >you write:
>  > > I've added some more test cases to the testsuite.  I will be sending
>  > > patches for the real bugs and the documented failures shortly.  If
>  > > people could look at the questionable cases, I'd appreciate it.
[...]
>Can someone work with Joern to deal with the reload issues?

I've discussed these and a couple others with him.  I was going to
submit patches shortly, but it'll have to wait until my computer gets
fixed.  If someone else wants to tackle it, fine with me.

zw

Re: Uninitialized variable warnings

[Zack Weinberg]

On Wed, 17 Mar 1999 21:38:12 -0700, Jeffrey A Law wrote:
>
>  In message < 199903152127.VAA32106@phal.cygnus.co.uk >you write:
>  > > I've added some more test cases to the testsuite.  I will be sending
>  > > patches for the real bugs and the documented failures shortly.  If
>  > > people could look at the questionable cases, I'd appreciate it.
[...]
>Can someone work with Joern to deal with the reload issues?

I've discussed these and a couple others with him.  I was going to
submit patches shortly, but it'll have to wait until my computer gets
fixed.  If someone else wants to tackle it, fine with me.

zw

Re: Uninitialized variable warnings

[Jeffrey A Law]

  In message < 199903152127.VAA32106@phal.cygnus.co.uk >you write:
  > > I've added some more test cases to the testsuite.  I will be sending
  > > patches for the real bugs and the documented failures shortly.  If
  > > people could look at the questionable cases, I'd appreciate it.
  > > 
  > > The list:
  > > 
  > > -- possibly real bugs --
  > > file		function		variable
  > ...
  > > regmove.c	fixup_match_1		dst_note
  > 
  > No bug here.  The use of dst_note depends on (success != 0) .  Success is
  > initialized to 0, and only set to 1 in a code path where dest_note is
  > initialized.
Agreed.  This _may_ be an example of the cases we may be able to detect via
control dependence analysis.  I've got no problem if someone initializes
dst_note to NULL_RTX.

  > > regmove.c	fixup_match_1		insn_const
  > 
  > No bug here, either.  Early in the function, either code is set to NOTE,
  > or insn_const is set, or the function returns early.
  > insn_const is never used if code is NOTE.
Similarly.  Control dependence analysis may allow us to tackle this one
someday.  Initializing insn_const to NULL_RTX seems reasonable to me.

Can someone work with Joern to deal with the reload issues?

jeff

Re: Uninitialized variable warnings

[Joern Rennecke]

> I've added some more test cases to the testsuite.  I will be sending
> patches for the real bugs and the documented failures shortly.  If
> people could look at the questionable cases, I'd appreciate it.
> 
> The list:
> 
> -- possibly real bugs --
> file		function		variable
...
> regmove.c	fixup_match_1		dst_note

No bug here.  The use of dst_note depends on (success != 0) .  Success is
initialized to 0, and only set to 1 in a code path where dest_note is
initialized.

> regmove.c	fixup_match_1		insn_const

No bug here, either.  Early in the function, either code is set to NOTE,
or insn_const is set, or the function returns early.
insn_const is never used if code is NOTE.

> reload.c	find_reloads		goal_alternative_number
> reload.c	find_reloads		goal_earlyclobber
> 
> I think these may be real bugs, but the logic is so convoluted that I
> can't be sure one way or the other. I'd appreciate people more
> familiar with these parts of the compiler looking at them.
> 
> -- definitely real bugs --
...
definitely not!

> reload1.c	reload			    is_scalar

is_scalar is always set when reg_equiv_memory_loc[i] is non-zero.
addr is only set when reg_equiv_mem[i] or reg_equiv_address[i] is non-zero.

One of the latter two is always non-zero if reg_equiv_memory_loc[i] is
non-zero (of course the compiler doesn't know about that when compiling
itself, which just goes to show that -Wuninitialized can't work quite
right without solving the halting problem).

is_scalar is only used when addr is set.

> varasm.c	const_hash		    hi

This is pretty straightforward setting hi in both arms of an if-else
or even unconditionally in each of the disjoint lifetimes.

Uninitialized variable warnings

[Zack Weinberg]

I have gone through all the uninitialized variable warnings in a
bootstrap of current CVS and classified them.  The majority are
the documented case where gcc gets it wrong:

if(flag) var = value;
...
if(flag) use(value);

although there are a number of cases where I can't prove this due to
extreme code obfuscation.  (Who was it wrote all those 6000 line
functions, anyway?)

There are a number of other places where gcc has gotten it wrong, and
a few places where the warning indicates a real bug.  There's also one
worrisome case, where we get it right or not unpredictably:

if(x == THING_1) var = val1;
else if(x == THING_2) var = val2;
else if(x == THING_3) var = val3;
...
else abort();
...
use(var);

or the equivalent

switch(x)
{
  case THING_1: var = val1; break;
  case THING_2: var = val2; break;
  case THING_3: var = val3; break;
  ...
  default: abort();
}
...
use(var);

This is documented to work correctly.

I've added some more test cases to the testsuite.  I will be sending
patches for the real bugs and the documented failures shortly.  If
people could look at the questionable cases, I'd appreciate it.

The list:

-- possibly real bugs --
file		function		variable

combine.c	find_split_point	inner
combine.c	find_split_point	pos
combine.c	find_split_point	unsignedp
integrate.c	subst_constants		op0_mode
regmove.c	fixup_match_1		dst_note
regmove.c	fixup_match_1		insn_const
reload.c	find_reloads		goal_alternative_number
reload.c	find_reloads		goal_earlyclobber

I think these may be real bugs, but the logic is so convoluted that I
can't be sure one way or the other. I'd appreciate people more
familiar with these parts of the compiler looking at them.

-- definitely real bugs --
cp/call.c	add_function_candidate	    convs
graph.c		node_data		    result
java/verify.c	verify_jvm_instructions	    oldpc
objc/objc-act.c	add_objc_string		    chain
objc/objc-act.c	build_keyword_selector	    key_name
objc/objc-act.c	build_message_expr	    sel_name
objc/objc-act.c	build_selector_expr	    selname
objc/objc-act.c	gen_declarator		    str
objc/objc-act.c	generate_protocol_list	    expr_decl
objc/objc-act.c	get_objc_string_decl	    chain
objc/objc-act.c	synth_id_with_class_suffix  string
reload1.c	reload			    is_scalar
varasm.c	const_hash		    hi

The objc ones are all assumptions that a parameter can only have some
limited range of values - which may be true if the data structure is
correct, but gcc can't know that and it isn't especially robust.  The
rest are odd corner cases or oversights.

-- spurious warnings indicating a bug in the compiler --
objc/objc-act.c	generate_protocol_list	plist
optabs.c	emit_cmp_insn		wider_mode
optabs.c	expand_float		libfcn
integrate.c	expand_inline_function	copy
integrate.c	save_for_inline_copying	copy

We get near-identical cases right.  We ought to get these right too.

-- spurious warnings, not the documented case --
java/class.c	build_utf8_ref		field
java/decl.c	init_decl_processing	field
java/verify.c	verify_jvm_instructions	last
java/verify.c	verify_jvm_instructions	prevpc
optabs.c	expand_binop		carry_in
optabs.c	expand_binop		carry_out
reg-stack.c	convert_regs		insn
reload1.c	emit_reload_insns	where

These warnings are spurious but (in most cases) gcc would have to be a
lot cleverer than it is to get them right.  In some cases it would
need to know things about functions other than the one currently being
compiled.

-- probably the documented spurious case --
cse.c		cse_insn		src_eqv_hash
cse.c		cse_insn		src_eqv_in_memory
cse.c		cse_insn		src_eqv_in_struct
cse.c		cse_insn		src_eqv_volatile
loop.c		move_movables		first
reload1.c	choose_reload_regs	mode
reload1.c	emit_reload_insns	store_insn
reload1.c	reload_combine		const_reg
stmt.c		expand_end_case		maxval
stmt.c		expand_end_case		minval
stmt.c		expand_end_case		range
varasm.c	output_constructor	byte
unroll.c	unroll_loop		local_label

Logic in these functions is too convoluted for me to be sure,
but I think these are just the documented spurious warning.  Again,
I'd like someone who understands these sections of the compiler to
review.

-- spurious warnings matching the documented case --
combine.c	simplify_if_then_else	c1
combine.c	simplify_if_then_else	op
combine.c	try_combine		i2_code_number
combine.c	try_combine		other_code_number
cse.c		cse_insn		sets
dbxout.c	dbxout_block		blocknum
fold-const.c	fold (MULT_EXPR)	alt0
fold-const.c	fold (MULT_EXPR)	alt1
fold-const.c	make_range		arg0
fold-const.c	make_range		arg1
function.c	instantiate_virtual_regs_1	offset
genopinit.c	gen_insn		m1
genopinit.c	gen_insn		m2
genopinit.c	gen_insn		op
integrate.c	mark_stores		mode
libgcc2.c	throw_helper		handler_p
libgcc2.c	throw_helper		pc_p
local-alloc.c	block_alloc		r1
loop.c		check_dbra_loop		comparison_val
loop.c		load_mems		end_label
loop.c		recombine_givs		life_end
loop.c		recombine_givs		life_start
loop.c		strength_reduce		increment
objc/objc-act.c	build_selector_translation_table    decl
optabs.c	expand_binop		op0_xhigh
optabs.c	expand_binop		op1_xhigh
reg-stack.c	record_asm_reg_life	clobber_reg
reg-stack.c	subst_asm_stack_regs	clobber_loc
reg-stack.c	subst_asm_stack_regs	clobber_reg
regmove.c	fixup_match_1		set2
reload.c	decompose		base
reload.c	find_valid_class	best_class
sched.c		schedule_block		regs_sometimes_live
sched.c		schedule_block		sometimes_max
splay-tree.c	splay_tree_insert	comparison
stupid.c	stupid_life_analysis	chain
unroll.c	copy_loop_body		copy
unroll.c	unroll_loop		unroll_type
varasm.c	assemble_variable	size_tree

These are uninteresting.

zw

Re: Uninitialized variable warnings

[Joern Rennecke]

> I'm basically wondering if it would be safe to move the if(reg_equiv_mem[i])
> and if(reg_equiv_address[i]) blocks that set `addr' inside the
> if(reg_equiv_memory_loc[i]) block that sets is_scalar.  That wouldn't make
> gcc stop warning, but it would make it clearer to a human what's going on.

Yes, that would be safe.

However, I'm not so sure that it makes the code easier to read -
if you don't know what reg_equiv_memory_loc / reg_equiv_address /
reg_equiv_mem are, you're pretty much lost anyways.

Besides, their meaning is explained at the definition of reg_equiv_memory_loc.

Re: Uninitialized variable warnings

[Joern Rennecke]

> I'm basically wondering if it would be safe to move the if(reg_equiv_mem[i])
> and if(reg_equiv_address[i]) blocks that set `addr' inside the
> if(reg_equiv_memory_loc[i]) block that sets is_scalar.  That wouldn't make
> gcc stop warning, but it would make it clearer to a human what's going on.

Yes, that would be safe.

However, I'm not so sure that it makes the code easier to read -
if you don't know what reg_equiv_memory_loc / reg_equiv_address /
reg_equiv_mem are, you're pretty much lost anyways.

Besides, their meaning is explained at the definition of reg_equiv_memory_loc.