Re: [PATCH] Fix PR79345, better uninit warnings for memory

[Richard Biener <rguenther@suse.de>]

On Thu, 2 Mar 2017, Jakub Jelinek wrote:

> On Wed, Mar 01, 2017 at 03:03:29PM +0100, Richard Biener wrote:
> > One issue with the patch is duplicate warnings as TREE_NO_WARNING
> > doesn't work very well on tcc_reference trees which are not
> > shared.  A followup could use some sort of hash table to mitigate
> 
> Can't we use TREE_NO_WARNING on get_base_address instead?  That is
> shared...

At least for VAR_DECL based refs, yes (the only ones we currently
analyze).  For MEM_REF bases it won't work.  It would of 
course only warn once per aggregate and
for a maybe-uninit warning it might be the false positive
(the patch only sets TREE_NO_WARNING on the always-executed case).

For the user maybe we should change the order we walk BBs to RPO
and thus warn at the first maybe-uninit point so we can say
"further uninit warnings suppressed for X"?

> > @@ -2925,14 +2927,22 @@ walk_aliased_vdefs_1 (ao_ref *ref, tree
> >  	  if (!*visited)
> >  	    *visited = BITMAP_ALLOC (NULL);
> >  	  for (i = 0; i < gimple_phi_num_args (def_stmt); ++i)
> > -	    cnt += walk_aliased_vdefs_1 (ref, gimple_phi_arg_def (def_stmt, i),
> > -					 walker, data, visited, 0,
> > -					 function_entry_reached);
> > +	    {
> > +	      int res = walk_aliased_vdefs_1 (ref,
> > +					      gimple_phi_arg_def (def_stmt, i),
> > +					      walker, data, visited, 0,
> > +					      function_entry_reached, limit);
> > +	      if (res == -1)
> > +		return -1;
> > +	      cnt += res;
> > +	    }
> 
> This doesn't look right.  The recursive call starts with cnt of 0 again,
> so if say you have limit 20 and cnt 10 when you are about to recurse and
> that walk does 15 oracle comparisons, it won't return -1, but 15 and
> you cnt += 15 to get 25 and never reach the limit, so then you walk perhaps
> another 100000 times.
> Can't you just pass cnt instead of 0 and then use
> 	      if (res == -1)
> 		return -1;
> 	      cnt = res;
> ?  Or you'd need to play gaves with decrementing the limit for the recursive
> call.

Ah, indeed.  Will fix (passing cnt).

> > +	  /* For limiting the alias walk below we count all
> > +	     vdefs in the function.  We then give the alias walk an
> > +	     overall budget (and simply not warn for further stmts).
> > +	     ???  The interface doesn't give us the chance to assign
> > +	     a maximum cost to each walk (and abort the walk if hit).  */
> 
> I don't understand the last two lines, I thought you've added the ability
> to the interface above and you actually use it.

Will adjust the comment.

> Otherwise it LGTM, yes, we will end up with new warnings, but unless the
> alias oracle is wrong, there shouldn't be too many false positives, right?

Yes, we are not warning for the case where there is at least one path
with an initialization, like

void foo(int b)
{
  S s;
  if (b)
    s.a = 1;
  return s.a;
}

will not warn (trivial to add but not necessary for the regression fix).

We also do not warn for pointer-based accesses (so unfortunately no
fix for PR2972).  Also trivial to add, but see PR79814 for more
bootstrap fallout.

Thanks,
Richard.