Re: [PATCH] tree-optimization/101573 - improve uninit warning at -O0

[Martin Sebor <msebor@gmail.com>]

On 7/22/21 6:34 AM, Richard Biener wrote:
> We can improve uninit warnings from the early pass by looking
> at PHI arguments on fallthru edges that are uninitialized and
> have uses that are before a possible loop exit.  This catches
> some cases earlier that we'd only warn in a more confusing
> way after early inlining as seen by testcase adjustments.
> 
> It introduces
> 
> FAIL: gcc.dg/uninit-23.c (test for excess errors)
> 
> where we additionally warn
> 
> gcc.dg/uninit-23.c:21:13: warning: 't4' is used uninitialized [-Wuninitialized]
> 
> which I think is OK even if it's not obvious that the new
> warning is an improvement when you look at the obvious source.
> 
> Somehow for all cases I never get the `'foo' was declared here`
> notes, I didn't dig why that happens but it's odd.

I happen to be working in that area right now so let me look into
that.

> 
> Bootstrapped and tested on x86_64-unknown-linux-gnu.
> 
> Any comments?

I'm (of course) favor of improving the warning -- thank you!  I've
been meaning to do change the early pass to look at PHIs as well.

I would just suggest to put the new code in a new function for
better readability.

Martin

> 
> Thanks,
> Richard.
> 
> 2021-07-22  Richard Biener  <rguenther@suse.de>
> 
> 	PR tree-optimization/101573
> 	* tree-ssa-uninit.c (warn_uninitialized_vars): Look at
> 	uninitialized PHI arg defs in some constrained cases.
> 	(execute_early_warn_uninitialized): Calculate dominators.
> 
> 	* gcc.dg/uninit-pr101573.c: New testcase.
> 	* gcc.dg/uninit-15-O0.c: Adjust.
> 	* gcc.dg/uninit-15.c: Likewise.
> 	* gcc.dg/uninit-23.c: Likewise.
> 	* c-c++-common/uninit-17.c: Likewise.
> ---
>   gcc/testsuite/c-c++-common/uninit-17.c |  6 +--
>   gcc/testsuite/gcc.dg/uninit-15-O0.c    |  2 +-
>   gcc/testsuite/gcc.dg/uninit-15.c       | 10 ++--
>   gcc/testsuite/gcc.dg/uninit-23.c       |  2 +-
>   gcc/testsuite/gcc.dg/uninit-pr101573.c | 10 ++++
>   gcc/tree-ssa-uninit.c                  | 66 ++++++++++++++++++++++++++
>   6 files changed, 86 insertions(+), 10 deletions(-)
>   create mode 100644 gcc/testsuite/gcc.dg/uninit-pr101573.c
> 
> diff --git a/gcc/testsuite/c-c++-common/uninit-17.c b/gcc/testsuite/c-c++-common/uninit-17.c
> index fd773da78ad..b5495366c5b 100644
> --- a/gcc/testsuite/c-c++-common/uninit-17.c
> +++ b/gcc/testsuite/c-c++-common/uninit-17.c
> @@ -9,11 +9,11 @@ static void bar(int a, int *ptr)
>   {
>     do
>     {
> -    int b;   /* { dg-message "declared" } */
> +    int b;
>       if (b < 40) {
> -      ptr[0] = b; /* { dg-warning "may be used uninitialized" } */
> +      ptr[0] = b;
>       }
> -    b += 1;
> +    b += 1; /* { dg-warning "is used uninitialized" } */
>       ptr++;
>     }
>     while (--a != 0);
> diff --git a/gcc/testsuite/gcc.dg/uninit-15-O0.c b/gcc/testsuite/gcc.dg/uninit-15-O0.c
> index a3fd2b63ba7..36d96348617 100644
> --- a/gcc/testsuite/gcc.dg/uninit-15-O0.c
> +++ b/gcc/testsuite/gcc.dg/uninit-15-O0.c
> @@ -15,6 +15,6 @@ void baz();
>   void bar()
>   {
>       int j;           /* { dg-message "was declared here" {} { xfail *-*-* } } */
> -    for (; foo(j); ++j)
> +    for (; foo(j); ++j) /* { dg-warning "is used uninitialized" } */
>           baz();
>   }
> diff --git a/gcc/testsuite/gcc.dg/uninit-15.c b/gcc/testsuite/gcc.dg/uninit-15.c
> index 8ee10c27aba..85cb116f349 100644
> --- a/gcc/testsuite/gcc.dg/uninit-15.c
> +++ b/gcc/testsuite/gcc.dg/uninit-15.c
> @@ -1,7 +1,7 @@
>   /* PR tree-optimization/17506
> -   We issue an uninitialized variable warning at a wrong location at
> +   We used to issue an uninitialized variable warning at a wrong location at
>      line 11, which is very confusing.  Make sure we print out a note to
> -   make it less confusing.  (not xfailed alternative)
> +   make it less confusing.  (xfailed alternative)
>      But it is of course ok if we warn in bar about uninitialized use
>      of j.  (not xfailed alternative)  */
>   /* { dg-do compile } */
> @@ -10,7 +10,7 @@
>   inline int
>   foo (int i)
>   {
> -  if (i) /* { dg-warning "used uninitialized" } */
> +  if (i) /* { dg-warning "used uninitialized" "" { xfail *-*-* } } */
>       return 1;
>     return 0;
>   }
> @@ -20,7 +20,7 @@ void baz (void);
>   void
>   bar (void)
>   {
> -  int j; /* { dg-message "note: 'j' was declared here" "" } */
> -  for (; foo (j); ++j)  /* { dg-warning "'j' is used uninitialized" "" { xfail *-*-* } } */
> +  int j; /* { dg-message "note: 'j' was declared here" "" { xfail *-*-* } } */
> +  for (; foo (j); ++j)  /* { dg-warning "'j' is used uninitialized" } */
>       baz ();
>   }
> diff --git a/gcc/testsuite/gcc.dg/uninit-23.c b/gcc/testsuite/gcc.dg/uninit-23.c
> index d64eb7d2ee9..87b4e989b53 100644
> --- a/gcc/testsuite/gcc.dg/uninit-23.c
> +++ b/gcc/testsuite/gcc.dg/uninit-23.c
> @@ -18,7 +18,7 @@ ql (void)
>           int *t4 = go; /* { dg-warning "is used uninitialized" } */
>   
>    l1:
> -        *t4 = (*t4 != 0) ? 0 : 2;
> +        *t4 = (*t4 != 0) ? 0 : 2; /* { dg-warning "is used uninitialized" } */
>         }
>   
>       if (ij != 0)
> diff --git a/gcc/testsuite/gcc.dg/uninit-pr101573.c b/gcc/testsuite/gcc.dg/uninit-pr101573.c
> new file mode 100644
> index 00000000000..a574844b791
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/uninit-pr101573.c
> @@ -0,0 +1,10 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O0 -Wuninitialized" } */
> +
> +int main(int argc, char **argv)
> +{
> +  int a;
> +  for(; a < 5; ++a) /* { dg-warning "is used uninitialized" } */
> +    ;
> +  return  0;
> +}
> diff --git a/gcc/tree-ssa-uninit.c b/gcc/tree-ssa-uninit.c
> index 148f3c2b31d..2ee4edf353f 100644
> --- a/gcc/tree-ssa-uninit.c
> +++ b/gcc/tree-ssa-uninit.c
> @@ -652,6 +652,71 @@ warn_uninitialized_vars (bool wmaybe_uninit)
>       {
>         basic_block succ = single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun));
>         wlims.always_executed = dominated_by_p (CDI_POST_DOMINATORS, succ, bb);
> +
> +      if (wlims.always_executed)
> +	{
> +	  edge_iterator ei;
> +	  edge e, found = NULL, found_back = NULL;
> +	  /* Look for a fallthru and possibly a single backedge.  */
> +	  FOR_EACH_EDGE (e, ei, bb->preds)
> +	    {
> +	      /* Ignore backedges.  */
> +	      if (dominated_by_p (CDI_DOMINATORS, e->src, bb))
> +		{
> +		  if (found_back)
> +		    {
> +		      found = NULL;
> +		      break;
> +		    }
> +		  found_back = e;
> +		  continue;
> +		}
> +	      if (found)
> +		{
> +		  found = NULL;
> +		  break;
> +		}
> +	      found = e;
> +	    }
> +	  if (found)
> +	    for (gphi_iterator si = gsi_start_phis (bb); !gsi_end_p (si);
> +		 gsi_next (&si))
> +	      {
> +		gphi *phi = si.phi ();
> +		tree def = PHI_ARG_DEF_FROM_EDGE (phi, found);
> +		if (TREE_CODE (def) != SSA_NAME
> +		    || !SSA_NAME_IS_DEFAULT_DEF (def)
> +		    || virtual_operand_p (def))
> +		  continue;
> +		/* If there's a default def on the fallthru edge PHI
> +		   value and there's a use that post-dominates entry
> +		   then that use is uninitialized and we can warn.  */
> +		imm_use_iterator iter;
> +		use_operand_p use_p;
> +		gimple *use_stmt = NULL;
> +		FOR_EACH_IMM_USE_FAST (use_p, iter, gimple_phi_result (phi))
> +		  {
> +		    use_stmt = USE_STMT (use_p);
> +		    if (gimple_location (use_stmt) != UNKNOWN_LOCATION
> +			&& dominated_by_p (CDI_POST_DOMINATORS, succ,
> +					   gimple_bb (use_stmt))
> +			/* If we found a non-fallthru edge make sure the
> +			   use is inside the loop, otherwise the backedge
> +			   can serve as initialization.  */
> +			&& (!found_back
> +			    || dominated_by_p (CDI_DOMINATORS, found_back->src,
> +					       gimple_bb (use_stmt))))
> +		      break;
> +		    use_stmt = NULL;
> +		  }
> +		if (use_stmt)
> +		  warn_uninit (OPT_Wuninitialized, def, SSA_NAME_VAR (def),
> +			       SSA_NAME_VAR (def),
> +			       "%qD is used uninitialized", use_stmt,
> +			       UNKNOWN_LOCATION);
> +	      }
> +	}
> +
>         for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
>   	{
>   	  gimple *stmt = gsi_stmt (gsi);
> @@ -3135,6 +3200,7 @@ execute_early_warn_uninitialized (void)
>        optimization we want to warn about possible uninitialized as late
>        as possible, thus don't do it here.  However, without
>        optimization we need to warn here about "may be uninitialized".  */
> +  calculate_dominance_info (CDI_DOMINATORS);
>     calculate_dominance_info (CDI_POST_DOMINATORS);
>   
>     warn_uninitialized_vars (/*warn_maybe_uninitialized=*/!optimize);
>