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

["Bill Maddox" <maddox at google dot com>]

On Wed, Aug 20, 2008 at 6:26 AM, Diego Novillo <dnovillo@google.com> wrote:
> On Tue, Aug 19, 2008 at 21:08, Bill Maddox <maddox@google.com> wrote:

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

input_type_tree isn't meant to be called from outside lto_function_in.c.
Shouldn't I just declare it static?

>> +      /* 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?

No, other than the fact that we know at this point that old_node
should be the last remaining reference, and that we were doing this
aggressive freeing before.  Doing so will help detect violations of
this expectation, but the result is corruption, perhaps difficult to
diagnose, instead of a clean assertion failure.   I don't know much
about the GCC GC scheme, but expect that at explicit freeing might
reduce memory occupancy, though possibly at an increase in execution
time.  I could "#if 0" the code out with a comment that it might be
enabled for diagnostic purposes.  It's your call.

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

By using input_tree_operand, I've already done so.  The comment is
just explaining why I don't expect  the assertion to fail here.
Calls to input_tree_operand will assert if they resolve a global
reference to a node that is subject to backpatching, as indicated by
calls to global_vector_enter and global_vector_fixup.

--Bill