Bug 34793

Summary: warning: 'areg' may be used uninitialized in this function
Product: gcc Reporter: Joerg Wunsch <j>
Component: middle-endAssignee: Richard Biener <rguenth>
Status: RESOLVED FIXED    
Severity: enhancement CC: eric.weddington, fang, gcc-bugs, manu, rguenth
Priority: P3 Keywords: diagnostic
Version: 4.3.0   
Target Milestone: 4.4.0   
Host: Target:
Build: Known to work:
Known to fail: Last reconfirmed: 2008-03-28 13:02:26
Bug Depends on: 14495, 30317    
Bug Blocks: 24639    
Attachments: Testcase showing the problem

Description Joerg Wunsch 2008-01-15 10:10:32 UTC
After reading all other PRs about uninitialized variable warnings, I
believe this one is a different situtation.

PR 20968 -- is about inlining, which is not involved here
PR 21733 -- is about two concatenated blocks rather than nested ones
PR 31707 -- is about setjmp
PR 20644 -- is also about unreachable code

In this testcase, it is really obvious to the observer that the switch
statement explicitly handles all the cases where it could be reached
from the outer if statement, and thus always assigns a value to
variable areg.  GCC 3.x did not warn about it.

I verified the warning is still generated with SVN revision 131533.

(The code is taken from a project called simulavr, and has been cut
down to the minimal test case reproducing the warning.)
Comment 1 Joerg Wunsch 2008-01-15 10:10:58 UTC
Created attachment 14942 [details]
Testcase showing the problem
Comment 2 Richard Biener 2008-01-15 10:49:47 UTC
This problem is introduced by the default case that gets added to the switch
stmt in the IL.  Coming from this case, areg is uninitialized.

The fix for PR14495 will likely fix this (by removing the default case again).
Comment 3 Joerg Wunsch 2008-01-15 12:54:43 UTC
(In reply to comment #2)

> The fix for PR14495 will likely fix this (by removing the default case again).

Alas, no, it doesn't.  I applied that patch (and the one it depends one
mentioned in the article), rebuilt, but I'm still getting the warning for
this test case.

If you want me to append some trace data, just tell me how to do it.
Comment 4 rguenther@suse.de 2008-01-15 13:27:44 UTC
Subject: Re:  warning: 'areg' may be used uninitialized
 in this function

On Tue, 15 Jan 2008, j at uriah dot heep dot sax dot de wrote:

> ------- Comment #3 from j at uriah dot heep dot sax dot de  2008-01-15 12:54 -------
> (In reply to comment #2)
> 
> > The fix for PR14495 will likely fix this (by removing the default case again).
> 
> Alas, no, it doesn't.  I applied that patch (and the one it depends one
> mentioned in the article), rebuilt, but I'm still getting the warning for
> this test case.
> 
> If you want me to append some trace data, just tell me how to do it.

Oh, indeed - it also needs PR30317 fixed.  (the attached patches therein
probably no longer apply)

The problem is that for the range check we create a fancy unsigned
compare, which VRP does not handle yet.

Richard.
Comment 5 Joerg Wunsch 2008-01-15 14:38:35 UTC
(In reply to comment #4)

> Oh, indeed - it also needs PR30317 fixed.  (the attached patches therein
> probably no longer apply)

I applied the second of the attached patches there.  It applies cleanly,
but still does not solve the issue.  OK, I patched in somewhat the
"reverse order" (PR 14495 first, then noticed I needed your previous
patchset for the find_case_label_index() function, then finally applied
the patch for PR 30317), but as the patches could all be applied, I think
that doesn't matter much.

If you've got a patched tree around, could you give the testcase of this
PR here a try?
Comment 6 rguenther@suse.de 2008-01-15 14:54:17 UTC
Subject: Re:  warning: 'areg' may be used uninitialized
 in this function

On Tue, 15 Jan 2008, j at uriah dot heep dot sax dot de wrote:

> ------- Comment #5 from j at uriah dot heep dot sax dot de  2008-01-15 14:38 -------
> (In reply to comment #4)
> 
> > Oh, indeed - it also needs PR30317 fixed.  (the attached patches therein
> > probably no longer apply)
> 
> I applied the second of the attached patches there.  It applies cleanly,
> but still does not solve the issue.  OK, I patched in somewhat the
> "reverse order" (PR 14495 first, then noticed I needed your previous
> patchset for the find_case_label_index() function, then finally applied
> the patch for PR 30317), but as the patches could all be applied, I think
> that doesn't matter much.
> 
> If you've got a patched tree around, could you give the testcase of this
> PR here a try?

Sorry, I don't have any of those trees left.  But if I ever come to
revisit those two bugs I'll make sure it fixes this bogus warning.

Richard.
Comment 7 Joerg Wunsch 2008-01-20 15:21:11 UTC
(In reply to comment #6)

> Sorry, I don't have any of those trees left.  But if I ever come to
> revisit those two bugs I'll make sure it fixes this bogus warning.

If you can give me some hints about where to start, I do have the
patched tree around on one of my machines, so I might make a stab at
fixing this.  I'm not a seasoned GCC hacker but willing to learn. ;-)
Comment 8 Manuel López-Ibáñez 2008-01-20 16:06:48 UTC
(In reply to comment #7)
> (In reply to comment #6)
> 
> > Sorry, I don't have any of those trees left.  But if I ever come to
> > revisit those two bugs I'll make sure it fixes this bogus warning.
> 
> If you can give me some hints about where to start, I do have the
> patched tree around on one of my machines, so I might make a stab at
> fixing this.  I'm not a seasoned GCC hacker but willing to learn. ;-)

If you have questions about GCC development they will be more visible on gcc@gcc.gnu.org than in this bug report.

A "may be" uninitialized warning is given when execute_late_warn_uninitialized finds an input to a PHI node that is SSA_NAME with an empty definition. This is marked as in the output of -fdump-tree-* as, for example, 

  # BLOCK 14 freq:596
  # PRED: 7 [14.3%]  (exec) 8 [100.0%]  (fallthru,exec) 9 [100.0%]  (fallthru,exec) 10 [100.0%]  (fallthru,exec) 11 [100.0%]  (fallthru,exec) 12 [100.0%]  (fallthru,exe\
c) 13 [100.0%]  (fallthru,exec)
  # aregD.1596_1 = PHI <aregD.1596_26(D)(7), aregD.1596_31(8), aregD.1596_38(9), aregD.1596_44(10), aregD.1596_51(11), aregD.1596_57(12), aregD.1596_64(13)>
<L11>:;

The aregD.1596_26(D)(7) means that if we entered this BB from BB 7, then the value of the variable is uninitialized.

In BB 7 we have:

  # BLOCK 7 freq:596
  # PRED: 6 [50.0%]  (true,exec)
  switch (val.0D.1608_22)
    {
      case 26: goto <L5>;
      case 27: goto <L6>;
      case 28: goto <L7>;
      case 29: goto <L8>;
      case 30: goto <L9>;
      case 31: goto <L10>;
      default : goto <L11>;
    }
  # SUCC: 8 [14.3%]  (exec) 9 [14.3%]  (exec) 10 [14.3%]  (exec) 11 [14.3%]  (exec) 12 [14.3%]  (exec) 13 [14.3%]  (exec) 14 [14.3%]  (exec)

The 'default' case jumps directly into BB 14 above and thus creates the extra input to the PHI node which is detected by the execute_late_warn_uninitialized pass.

If you can manage to convince GCC to not create that default case, then I think the PHI node won't contain any input with (D) and the warning won't be emitted. Now, if I understood Richard correctly, PR14495 is about not creating the default case in general by using VRP information. However, even if you fix that, there is another bug in VRP (PR30317) that doesn't handle the kind of comparisons that appear in this testcase.

I don't think those are trivial bugs, but if you still want to give it a try, to test that you fixed PR14495 you can modify your testcase to use "if" instead of a switch:

      if ((val >= 26)) && (val <= 31))
            {
              unsigned int areg;
              if (val == 26)
                  areg = xreg = (xreg & 0xff00) | (bval & 0xff);
              else if (val == 27)
                            areg = xreg =
                              (xreg & 0xff) | ((bval << 8) & 0xff00);
              else if (val == 28)
                  areg = yreg = (yreg & 0xff00) | (bval & 0xff);
              else if (val == 29)
                            areg = yreg =
                              (yreg & 0xff) | ((bval << 8) & 0xff00);
              else if (val == 30)
                  areg = zreg = (zreg & 0xff00) | (bval & 0xff);
              else if (val == 31)
                            areg = zreg =
                              (zreg & 0xff) | ((bval << 8) & 0xff00);
              disp_print_addr_reg (val, areg);
            }

But this still gets a warning because you need to fix PR30317 first. If you modify the above slightly, VRP works and you don't get the warning:

          if ((val >= 26))
            if ((val <= 31))
            {
              unsigned int areg;
              if (val == 26)
                  areg = xreg = (xreg & 0xff00) | (bval & 0xff);
              else if (val == 27)
                            areg = xreg =
                              (xreg & 0xff) | ((bval << 8) & 0xff00);
              else if (val == 28)
                  areg = yreg = (yreg & 0xff00) | (bval & 0xff);
              else if (val == 29)
                            areg = yreg =
                              (yreg & 0xff) | ((bval << 8) & 0xff00);
              else if (val == 30)
                  areg = zreg = (zreg & 0xff00) | (bval & 0xff);
              else if (val == 31)
                            areg = zreg =
                              (zreg & 0xff) | ((bval << 8) & 0xff00);
              disp_print_addr_reg (val, areg);
            }

Good luck!
Comment 9 Richard Biener 2008-03-28 12:21:01 UTC
Subject: Bug 34793

Author: rguenth
Date: Fri Mar 28 12:20:09 2008
New Revision: 133680

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=133680
Log:
2008-03-28  Richard Guenther  <rguenther@suse.de>

	PR tree-optimization/30317
	PR tree-optimization/30911
	PR tree-optimization/34793
	* tree-vrp.c (set_and_canonicalize_value_range): New function.
	(struct assert_locus_d): New member EXPR.
	(register_new_assert_for): Add EXPR parameter to support
	ASSERT_EXPR <name, expr OP limit>.
	(register_edge_assert_for_1): Adjust callers.
	(find_assert_locations): Likewise.
	(process_assert_insertions_for): Build condition from
	expression.
	(extract_range_from_assert): Handle ASSERT_EXPRs
	of the form ASSERT_EXPR <name, expr OP limit>.
	(register_edge_assert_for_2): New helper registering
	asserts for comparisons.  Recognize range tests of the form
	(unsigned)i - CST1 OP CST2.
	(register_edge_assert_for_1): Use it.
	(register_edge_assert_for): Likewise.
	* tree.def (ASSERT_EXPR): Document extra allowed conditional
	expressions.
	(needs_overflow_infinity): Integer sub-types
	do not need overflow infinities.
	(vrp_val_is_max): The extreme values of integer sub-types
	are those of the base type.
	(vrp_val_is_min): Likewise.

	* gcc.dg/tree-ssa/vrp35.c: New testcase.
	* gcc.dg/tree-ssa/vrp36.c: Likewise.
	* gcc.dg/tree-ssa/vrp37.c: Likewise.

Added:
    trunk/gcc/testsuite/gcc.dg/tree-ssa/vrp35.c
    trunk/gcc/testsuite/gcc.dg/tree-ssa/vrp36.c
    trunk/gcc/testsuite/gcc.dg/tree-ssa/vrp37.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-vrp.c
    trunk/gcc/tree.def

Comment 10 Richard Biener 2008-03-28 13:02:25 UTC
I have two patches for the other missing parts to fix this PR.
Comment 12 Richard Biener 2008-04-02 12:54:54 UTC
Subject: Bug 34793

Author: rguenth
Date: Wed Apr  2 12:54:08 2008
New Revision: 133835

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=133835
Log:
2008-04-02  Richard Guenther  <rguenther@suse.de>

	PR tree-optimization/14495
	PR tree-optimization/34793
	* tree-vrp.c (struct switch_update): New structure.
	(to_remove_edges, to_update_switch_stmts): New VECs.
	(simplify_switch_using_ranges): New function.  Remove not taken
	case labels and edges.
	(simplify_stmt_using_ranges): Call it.
	(identify_jump_threads): Mark edges we have queued for removal
	so we don't thread them.
	(execute_vrp): Remove edges queued for removal, update SWITCH_STMT
	case label vector.
	* tree-cfg.c (group_case_labels): Deal with missing default label.
	(tree_verify_flow_info): Allow missing default label.
	* stmt.c (emit_case_bit_tests): Deal with NULL default_label.
	(emit_case_nodes): Likewise.
	(expand_case): Do not rely on the default label to be present.
	* expr.c (try_casesi): Deal with NULL default_label.
	(do_tablejump): Likewise.

	* gcc.dg/tree-ssa/vrp41.c: New testcase.
	* gcc.dg/tree-ssa/vrp42.c: Likewise.

Added:
    trunk/gcc/testsuite/gcc.dg/tree-ssa/vrp41.c
    trunk/gcc/testsuite/gcc.dg/tree-ssa/vrp42.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/expr.c
    trunk/gcc/stmt.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-cfg.c
    trunk/gcc/tree-vrp.c

Comment 13 Richard Biener 2008-04-02 12:56:00 UTC
Fixed.