Re: [PATCH] Verify that context of local DECLs is the current function

[Richard Biener <richard dot guenther at gmail dot com>]

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.

>      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 ...

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

... 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);

Richard.

>        if (!is_gimple_min_invariant (t) && !is_gimple_lvalue (t))
>         {
> --
> 2.8.1
>