Verify that context of local DECLs is the current function

Hi,

On Tue, Apr 26, 2016 at 10:58:22AM +0200, Richard Biener wrote:
> On Mon, Apr 25, 2016 at 3:22 PM, Martin Jambor <mjambor@suse.cz> wrote:
> > Hi,
> >
> > the patch below moves an assert from expand_expr_real_1 to gimple
> > verification.  It triggers when we do a sloppy job outlining stuff
> > from one function to another (or perhaps inlining too) and leave in
> > the IL of a function a local declaration that belongs to a different
> > function.
> >
> > Like I wrote above, such cases usually ICE in expand anyway, but I
> > think it is worth bailing out sooner, if nothing because bugs like PR
> > 70348 would not be assigned to me ;-) ...well, actually, I found this
> > helpful when working on OpenMP gridification.
> >
> > In the process, I think that the verifier would not catch a
> > SSA_NAME_IN_FREE_LIST when such an SSA_NAME is a base of a MEM_REF so
> > I added that check too.
> >
> > Bootstrapped and tested on x86_64-linux, OK for trunk?
> >
> > Thanks,
> >
> > Martin
> >
> >
> >
> > 2016-04-21  Martin Jambor  <mjambor@suse.cz>
> >
> >         * tree-cfg.c (verify_var_parm_result_decl): New function.
> >         (verify_address): Call it on PARM_DECL bases.
> >         (verify_expr): Likewise, also verify SSA_NAME bases of MEM_REFs.
> > ---
> >  gcc/tree-cfg.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 47 insertions(+)
> >
> > diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
> > index 3385164..c917967 100644
> > --- a/gcc/tree-cfg.c
> > +++ b/gcc/tree-cfg.c
> > @@ -2764,6 +2764,23 @@ gimple_split_edge (edge edge_in)
> >    return new_bb;
> >  }
> >
> > +/* Verify that a VAR, PARM_DECL or RESULT_DECL T is from the current function,
> > +   and if not, return true.  If it is, return false.  */
> > +
> > +static bool
> > +verify_var_parm_result_decl (tree t)
> > +{
> > +  tree context = decl_function_context (t);
> > +  if (context != cfun->decl
> > +      && !SCOPE_FILE_SCOPE_P (context)
> > +      && !TREE_STATIC (t)
> > +      && !DECL_EXTERNAL (t))
> > +    {
> > +      error ("Local declaration from a different function");
> > +      return true;
> > +    }
> > +  return NULL;
> > +}
> >
> >  /* Verify properties of the address expression T with base object BASE.  */
> >
> > @@ -2798,6 +2815,8 @@ verify_address (tree t, tree base)
> >         || TREE_CODE (base) == RESULT_DECL))
> >      return NULL_TREE;
> >
> > +  if (verify_var_parm_result_decl (base))
> > +    return base;
> 
> Is that necessary?  We recurse after all, so ...
> 
> >    if (DECL_GIMPLE_REG_P (base))
> >      {
> >        error ("DECL_GIMPLE_REG_P set on a variable with address taken");
> > @@ -2834,6 +2853,13 @@ verify_expr (tree *tp, int *walk_subtrees, void *data ATTRIBUTE_UNUSED)
> >         }
> >        break;
> >
> > +    case PARM_DECL:
> > +    case VAR_DECL:
> > +    case RESULT_DECL:
> > +      if (verify_var_parm_result_decl (t))
> > +       return t;
> > +      break;
> > +
> 
> ... should apply.

I made that happen (see below)...

> 
> >      case INDIRECT_REF:
> >        error ("INDIRECT_REF in gimple IL");
> >        return t;
> > @@ -2852,9 +2878,25 @@ verify_expr (tree *tp, int *walk_subtrees, void *data ATTRIBUTE_UNUSED)
> >           error ("invalid offset operand of MEM_REF");
> >           return TREE_OPERAND (t, 1);
> >         }
> > +      if (TREE_CODE (x) == SSA_NAME)
> > +       {
> > +         if (SSA_NAME_IN_FREE_LIST (x))
> > +           {
> > +             error ("SSA name in freelist but still referenced");
> > +             return x;
> > +           }
> > +         if (SSA_NAME_VAR (x))
> > +           x = SSA_NAME_VAR (x);;
> > +       }
> > +      if ((TREE_CODE (x) == PARM_DECL
> > +          || TREE_CODE (x) == VAR_DECL
> > +          || TREE_CODE (x) == RESULT_DECL)
> > +         && verify_var_parm_result_decl (x))
> > +       return x;
> 
> please instead try removing *walk_subtrees = 0 ...

That unfortunately leads to the verifier complaining that DECLs which
are in ADDR_EXPRs are not marked as addressable.  So I changed the
code below

> 
> >        if (TREE_CODE (x) == ADDR_EXPR
> >           && (x = verify_address (x, TREE_OPERAND (x, 0))))
> >         return x;

to

      if (TREE_CODE (x) == ADDR_EXPR)
	{
	  tree va = verify_address (x, TREE_OPERAND (x, 0));
	  if (va)
	    return va;
	  x = TREE_OPERAND (x, 0);
	}
      walk_tree (&x, verify_expr, data, NULL);
      *walk_subtrees = 0;
      break;

> 
> ... we only get some slight duplicate address verification here
> (this copy is stronger than the ADDR_EXPR case).
> 
> > +
> >        *walk_subtrees = 0;
> >        break;
> >
> > @@ -3010,6 +3052,11 @@ verify_expr (tree *tp, int *walk_subtrees, void *data ATTRIBUTE_UNUSED)
> >
> >           t = TREE_OPERAND (t, 0);
> >         }
> > +      if ((TREE_CODE (t) == PARM_DECL
> > +          || TREE_CODE (t) == VAR_DECL
> > +          || TREE_CODE (t) == RESULT_DECL)
> > +         && verify_var_parm_result_decl (t))
> > +       return t;
> 
> Hmm.  I wonder if rather than replicating stuff everywhere we do not walk
> subtrees we instead should walk the subtree we care about explicitely
> via a walk_tree invocation.  Like in this case
> 
>            walk_tree (&t, verify_expr, data);

I was not sure whether you meant to do it for all or only some t
tree-codes.  In the end I decided to call it for all of them, the
bases of handled components are unlikely to be deep trees in any
case.  But I can change that.

In any event, below is the changed patch which also passes bootstrap
and testing on x86_64-linux.  OK for trunk?

Thanks,

Martin

2016-04-26  Martin Jambor  <mjambor@suse.cz>

	* tree-cfg.c (verify_expr): Verify that local declarations belong to
	this function.  Call verify_expr on MEM_REFs and bases of other
	handled_components.

Verify that context of local DECLs is the current function

Commit Message

Comments

Patch