Re: [lto][patch] Avoid stale references to tree nodes discarded by declaration merging

["Diego Novillo" <dnovillo at google dot com>]

On Tue, Aug 19, 2008 at 21:08, Bill Maddox <maddox@google.com> wrote:

> 2008-08-19  Bill Maddox <maddox@google.com>
>
>        * lto-tree-flags.def: Stream private_flag for
>        PARM_DECL and RESULT_DECL.
>        * lto-function-out.c (output_type): Use debug token
>        "fields" for unions as well as records.
>        * lto-function-in.c (typedef tree_ptr): New.
>        Also, declared vec.h-style vector of same.
>        (global_vector_enter): Added need_fixups parameter,
>        and code to mark vector entry for backpatching.
>        (global_vector_fixup): Implement backpatching of
>        references to the replaced vector entry.  Add comments.
>        Call ggc_free on the replaced node.
>        (input_field_decl, input_function_decl, input_var_decl,
>        input_parm_decl, input_result_decl, input_type_decl,
>        input_namespace_decl, input_translation_unit_decl,
>        input_binfo, input_type): Use new API for reading
>        backpatchable fields.
>        (input_type): Use debug token "fields" for unions
>        as well as records.
>        (input_tree_operand): Assert if reference is resolved
>        to a backpatchable node without providing a location
>        to backpatch.  Add comment.  Allow for backpatching
>        of operands of TREE_VEC, TREE_LIST, and various
>        expression operators.
>        (input_tree): Added slot parameter, providing location
>        to backpatch.  Added code to register this location
>        for backpatching if needed.
>        * lto-tree-in.h (input_tree): Added slot parameter
>        to function prototype.
>
>
>        * lto.c (lto_read_decls): Provide dummy argument to input_tree
>        to conform to its new signature.
>        * lto-symtab.c (lto_symtab_merge_decl): Do not invoke ggc_free
>        on discarded node here, now called in global_vector_fixup.

OK with

> @@ -2443,7 +2450,7 @@ lto_input_constructors_and_inits (struct
>
>  /* Read types and globals.  */
>
> -tree input_tree (struct lto_input_block *, struct data_in *);
> +void input_tree (tree *slot, struct lto_input_block *, struct data_in *);
>  tree input_type_tree (struct data_in *, struct lto_input_block *);

These declarations should go in lto-tree-in.h.  input_tree is
already there.

>  static tree input_tree_operand (struct lto_input_block *,
> @@ -2455,7 +2462,7 @@ static tree input_tree_operand (struct l
>     might be reachable.  */
>
>  static unsigned
> -global_vector_enter (struct data_in *data_in, tree node)
> +global_vector_enter (struct data_in *data_in, tree node, bool need_fixups)

Needs comment and documentation for the arguments.

> +/* After reading a declaration, we may merge it with an existing
> +   declaration, in which case all references to the declaration
> +   we just read should point to the merged result.  For future
> +   references, it suffices to replace the entry for this object
> +   in GLOBALS_INDEX with the merged declaration.  If references
> +   were resolved while the object was being read, however, they
> +   must be backpatched.  Both of these cases are handled here.  */
> +
>  static void
>  global_vector_fixup (struct data_in *data_in, unsigned index, tree node)

Arguments need documenting.

> +      /* Note that we cannot do the ggc_free when we merge the declaration,
> +	 but must wait until we have finished using it above. */
> +      if (old_node != node)
> +	{
> +	  remove_decl_from_map (old_node);
> +	  ggc_free (old_node);

Any particular reason we need to call ggc_free instead of letting
the GC machinery do it?

>    binfo->binfo.base_accesses = VEC_alloc (tree, gc, num_base_accesses);
>    LTO_DEBUG_TOKEN ("base_accesses");
>    for (i = 0; i < num_base_accesses; ++i)
> -    VEC_quick_push (tree, binfo->binfo.base_accesses, input_tree (ib, data_in));
> +    /* These should all reference well-known nodes defined in GLOBAL_TREES,
> +       (e.g., access_public_node) and should never need to be backpatched.  */

Could we assert the fact that these nodes need no backpatching?

> +    VEC_quick_push (tree, binfo->binfo.base_accesses,
> +		    input_tree_operand (ib, data_in, NULL,
> +					input_record_start (ib)));
>
>    LTO_DEBUG_TOKEN ("base_binfos");
>    for (i = 0; i < num_base_binfos; ++i)
> -    VEC_quick_push (tree, &binfo->binfo.base_binfos, input_tree (ib, data_in));
> +    /* These are all references to binfos, which should never need to be
> +       packpatched.  */

s/packpatched/backpatched/

>  /* Input a generic tree, allowing for NULL_TREE.  */
> -tree
> -input_tree (struct lto_input_block *ib, struct data_in *data_in)
> +void
> +input_tree (tree *slot, struct lto_input_block *ib, struct data_in *data_in)

Arguments needs documenting.


Diego.