[PATCH] Verify that context of local DECLs is the current function
Richard Biener
richard.guenther@gmail.com
Wed Apr 27 12:40:00 GMT 2016
On Wed, Apr 27, 2016 at 2:23 PM, Martin Jambor <mjambor@suse.cz> wrote:
> 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?
Ok.
Thanks,
Richard.
> 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.
>
> 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:
More information about the Gcc-patches
mailing list