Re: [PATCH] Clean up (parts of) the gimple "type" verifier

[Richard Guenther <rguenther at suse dot de>]

On Wed, 3 Sep 2008, Diego Novillo wrote:

> Richard Guenther wrote:
> 
> > !   fntype = TREE_TYPE (TREE_TYPE (fn));
> > !   if (gimple_call_lhs (stmt)
> > !       && !useless_type_conversion_p (TREE_TYPE (gimple_call_lhs (stmt)),
> > ! 				     TREE_TYPE (fntype))
> > !       /* ???  At least C++ misses conversions at assignments from
> > ! 	 void * call results.  */
> > !       && !(POINTER_TYPE_P (TREE_TYPE (gimple_call_lhs (stmt)))
> > ! 	   && POINTER_TYPE_P (TREE_TYPE (fntype))
> > ! 	   && VOID_TYPE_P (TREE_TYPE (TREE_TYPE (fntype))))
> > !       /* ???  Java is completely off.  Especially with functions
> > ! 	 returning java.lang.Object.  */
> > !       && !(strcmp (lang_hooks.name, "GNU Java") == 0
> > ! 	   && POINTER_TYPE_P (TREE_TYPE (gimple_call_lhs (stmt)))
> > ! 	   && POINTER_TYPE_P (TREE_TYPE (fntype))))
> > !     {
> > !       error ("invalid non-noop conversion in gimple call");
> 
> "non-noop" is very hard to parse.    How about using 'useful' or 'active' or
> 'significant' conversion?  Perhaps 'useful' is the more appropriate term.

Ok, I'll change it to simply omit "non-noop" - it's a conversion after 
all.

> We definitely need to remove the special casing for Java, that's too hacky.

Yeah, I can allow any pointer mismatches for now if you like.

> > +     {
> > +       error ("return statement without value in function returning
> > non-void");
> > +       debug_generic_stmt (restype);
> > +       return true;
> > +     }
> 
> The gimplifier changes to fix this shouldn't be that hard.  I'd rather not add
> disabled code in the meantime.  This can go in together with the gimplifier
> patch.

Ok.

> 
> > !   if (!useless_type_conversion_p (restype, TREE_TYPE (op))
> > !       /* ???  With C++ we can have the situation that the result
> > ! 	 decl is a reference type while the return type is an aggregate.  */
> > !       && !(TREE_CODE (op) == RESULT_DECL
> > ! 	   && TREE_CODE (TREE_TYPE (op)) == REFERENCE_TYPE
> > ! 	   && useless_type_conversion_p (restype, TREE_TYPE (TREE_TYPE
> > (op)))))
> > !     {
> > !       error ("Invalid non-noop conversion in return statement");
> 
> 'non-noop' again.

Ok.

> > + +   /* ???  We have two canonical forms of direct goto destinations, a
> > +      bare LABEL_DECL and an ADDR_EXPR of a LABEL_DECL.  */
> > +   if (TREE_CODE (dest) != LABEL_DECL
> > +       && (!is_gimple_val (dest)
> > + 	  || !POINTER_TYPE_P (TREE_TYPE (dest))))
> 
> goto &label?  Where is that one coming from?  The only non-label goto I'm
> aware of is goto *ptr with ptr taking the address of a label.

The simples testcase is:

void foo(void)
{
  b:
   goto *&&b;
}

this produces a goto with &b as destination.  If you write goto b; instead
we get the LABEL_DECL b directly.  If you use a temporary like

void foo(void)
{
  void *lab = &&b;
  b:
    goto *lab;
}

you get lab (the variable of pointer type) as destination.  Later
we get &label as the result of constant propagating this case, which
cfg-cleanup then fixes up (removing the goto and doing a pure CFG jump).

> The rest of the middle-end changes look fine (with the changes I suggested
> above).  The FE changes look fine to me too, but I think it's best if an FE
> maintainer reviews those.

Ok, I'll ping one with the updated patch.

> Thanks for doing this.  As an aside, maybe we could call these verifiers from
> the gimple_*_set_* helpers (only when checking is enabled)? Perhaps it's too
> much overhead for little gain, though.

The problem right now would be that many places get it wrong ;)  Which
is one reason why this verification is only done right after the
gimplifier.  We should, of course, work towards changing this in the
future.

Thanks,
Richard.