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 <mjam...@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 <mjam...@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 <mjam...@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.
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index 04e46fd..943bb39 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -2834,6 +2834,22 @@ verify_expr (tree *tp, int *walk_subtrees, void *data
ATTRIBUTE_UNUSED)
}
break;
+ case PARM_DECL:
+ case VAR_DECL:
+ case RESULT_DECL:
+ {
+ 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 t;
+ }
+ }
+ break;
+
case INDIRECT_REF:
error ("INDIRECT_REF in gimple IL");
return t;
@@ -2852,9 +2868,14 @@ 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) == ADDR_EXPR
- && (x = verify_address (x, TREE_OPERAND (x, 0))))
- return x;
+ 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;
@@ -3016,6 +3037,7 @@ verify_expr (tree *tp, int *walk_subtrees, void *data
ATTRIBUTE_UNUSED)
error ("invalid reference prefix");
return t;
}
+ walk_tree (&t, verify_expr, data, NULL);
*walk_subtrees = 0;
break;
case PLUS_EXPR:
