[patch 10/10] debug-early merge: compiler proper

[Richard Biener]

On Wed, May 20, 2015 at 5:50 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
> On 05/18/2015 06:56 AM, Richard Biener wrote:
>
> BTW, thanks for the review.
>
>> On Fri, May 8, 2015 at 2:40 AM, Aldy Hernandez <aldyh@redhat.com> wrote:
>>>
>>> As seen on TV.
>>
>>
>> +/* FIRST_TIME is set to TRUE for the first time we are called for a
>> +   translation unit from finalize_compilation_unit() or false
>> +   otherwise.  */
>> +
>>   static void
>> -analyze_functions (void)
>> +analyze_functions (bool first_time)
>>   {
>> ...
>> +  if (first_time)
>> +    {
>> +      symtab_node *snode;
>> +      FOR_EACH_SYMBOL (snode)
>> +       check_global_declaration (snode->decl);
>> +    }
>> +
>>
>> I think it is better to split analyze_functions (why does it have it's own
>> copy
>> of unreachable node removal?) into analysis and unused-symbol removal
>> and have the
>> check_global_declaration call be in finalize_compilation_unit directly.
>> Honza?
>
>
> Leaving things as is, as per Honza's comment ??
>
>>
>> @@ -1113,6 +1131,19 @@ analyze_functions (void)
>>          {
>>            if (symtab->dump_file)
>>              fprintf (symtab->dump_file, " %s", node->name ());
>> +
>> +         /* See if the debugger can use anything before the DECL
>> +            passes away.  Perhaps it can notice a DECL that is now a
>> +            constant and can tag the early DIE with an appropriate
>> +            attribute.
>> +
>> +            Otherwise, this is the last chance the debug_hooks have
>> +            at looking at optimized away DECLs, since
>> +            late_global_decl will subsequently be called from the
>> +            contents of the now pruned symbol table.  */
>> +         if (!decl_function_context (node->decl))
>> +           (*debug_hooks->late_global_decl) (node->decl);
>> +
>>            node->remove ();
>>
>> so this applies to VAR_DECLs only - shouldn't this be in the
>> varpool_node::remove function then?  You can even register/deregister
>> a hook for this in finalize_compilation_unit.  That would IMHO be better.
>
>
> The problem is that varpool_node::remove() may be called before we
> have finished parsing the DECL, thus before we call
> early_global_decl() on it.  So we would essentially be calling
> late_global_decl() on a DECL for which we haven't called
> early_global_decl().
>
> To complicate matters, we may call ::remove() before we finish parsing
> a decl.  In the C front-end, for instance, we call ::remove() from
> duplicate_decls(), before we even call rest_of_decl_compilation (where
> we call early_global_decl).
>
> Calling late_global_decl so early, before we have even finished
> parsing, seems wrong and obviously causes problems.  One example is
> dwarf2out can put the DECL into the deferred_asm_names list, only to
> have duplicate_decls() gcc_free it from under us.
>
>>
>> All debug stuff apart from dwarf2out.c changes (I assume Jason reviews
>> them) are ok.
>>
>> diff --git a/gcc/gengtype.c b/gcc/gengtype.c
>> index 02012d5..15b6dd2 100644
>> --- a/gcc/gengtype.c
>> +++ b/gcc/gengtype.c
>> @@ -4718,7 +4718,8 @@ write_roots (pair_p variables, bool emit_pch)
>>      this funcion will have to be adjusted to be more like
>>      output_mangled_typename.  */
>>
>> -static void
>> +/* ?? Why are we keeping this?  Is this actually used anywhere?  */
>> +static void ATTRIBUTE_UNUSED
>>   output_typename (outf_p of, const_type_p t)
>>   {
>>     switch (t->kind)
>>
>> Just remove the function.
>
>
> Done.
>
>>
>> The langhooks changes are ok.
>>
>> diff --git a/gcc/passes.c b/gcc/passes.c
>> index 04ff042..4dee8ad 100644
>> --- a/gcc/passes.c
>> +++ b/gcc/passes.c
>> @@ -293,6 +293,28 @@ rest_of_decl_compilation (tree decl,
>>     else if (TREE_CODE (decl) == VAR_DECL && !DECL_EXTERNAL (decl)
>>             && TREE_STATIC (decl))
>>       varpool_node::get_create (decl);
>> +
>> +  /* Generate early debug for global variables.  Any local variables will
>> +     be handled by either handling reachable functions from
>> +     finalize_compilation_unit (and by consequence, locally scoped
>> +     symbols), or by rest_of_type_compilation below.
>> +
>> +     Also, pick up function prototypes, which will be mostly ignored
>> +     by the different early_global_decl() hooks, but will at least be
>> +     used by Go's hijack of the debug_hooks to implement
>> +     -fdump-go-spec.  */
>> +  if (!flag_wpa
>> +      && !in_lto_p
>>
>> Just check !in_lto_p, !flag_wpa is redundant.
>
>
> Done.
>
>>
>> +      && !decl_function_context (decl)
>> +      && !current_function_decl
>>
>> Why that?  !decl_function_context should catch relevant cases?
>
>
> You'd think, huh?  The issue here are extern declarations appearing
> inside of a function.  For this case, decl_function_context is NULL,
> because the actual context is toplevel, but current_function_decl is
> set to the function where the extern declaration appears.
>
> For example:
>
> namespace S
> {
>   int
>   f()
>   {
>     {
>       int i = 42;
>       {
>         extern int i; // Local extern declaration.
>         return i;
>       }
>     }
>   }
> }
>
> I have added a big fat comment in the code now, since it's clearly not
> obvious why we need current_function_decl.
>
>>
>> +      && !decl_type_context (decl))
>> +    (*debug_hooks->early_global_decl) (decl);
>>
>> I'll note that nested functions and class methods are not getting
>> early_global_decl()ed here.  I suppose their containing function/type
>> is supposed to generate early dwarf by means of dwarf2out walking
>> over children.
>
>
> Indeed, and I had properly commented this:
>
>   /* Emit early debug for reachable functions, and by consequence,
>      locally scoped symbols.  */
>   struct cgraph_node *cnode;
>   FOR_EACH_FUNCTION_WITH_GIMPLE_BODY (cnode)
>     (*debug_hooks->early_global_decl) (cnode->decl);
>
> I did, however, remove the !decl_function_context() restriction in this
> snippet from the previous iteration of this patch because of the nested
> function problem you mention.
>
> Doubly nested functions were fine because dwarf2out will walk the inner
> function, by virtue of it being a symbol local to the top level function,
> however it was missing triply nested functions because the 2nd nested
> functions was considered a declaration so its children (> 2 nested
> functions) were not walked.  I ran into this with Ada, and removed the
> !decl_function_context() while generating early dwarf. IIRC, the restriction
> was originally there to limit extra DIEs that may be generated, but since
> I'm already on the hook for cleaning up DIEs shortly after the merge, we're
> going to have deal with this anyhow.
>
> I also added a test for triply nested functions for C+debug-early, since Ada
> seemed to be the only front-end stressing >= 3 nested functions.  I will
> repost the test when I repost the testsuite changes.
>
>>
>> timevar changes are ok.
>>
>> the toplev changes are ok.
>>
>> diff --git a/gcc/tree-core.h b/gcc/tree-core.h
>> index ad1bb23..2a9f417 100644
>> --- a/gcc/tree-core.h
>> +++ b/gcc/tree-core.h
>> @@ -1334,6 +1334,9 @@ struct GTY(()) tree_block {
>>     tree abstract_origin;
>>     tree fragment_origin;
>>     tree fragment_chain;
>> +
>> +  /* Pointer to the DWARF lexical block.  */
>> +  struct die_struct *die;
>>   };
>>
>>   struct GTY(()) tree_type_common {
>>
>> Ick - do we need this?  dwarf2out.c has a hashtable to map blocks to
>> DIEs (which you don't remove in turn).
>
>
> We need a way to reference the early created DIE from late debugging, and we
> can't use block_map because it gets cloberred across functions. It's
> currently being released in late debug (dwarf2out_function_decl),
> that's why you see it not set to NULL in dwarf2out_c_finalize.
>
> Also, it uses BLOCK_NUMBERs, which according to the documentation in
> tree.h, are not guaranteed to be unique across functions.
>
> As Honza mentioned, we're already using a DIE map in types through
> TYPE_SYMTAB_DIE.  See lookup_type_die() in dwarf2out.c.
>
> Could we leave this as is?

But why then not eliminate block_map in favor of using the new ->die member?
Having both looks very odd to me.

Can you cook up a patch for trunk adding that field to tree_block and removing
the block_map map in favor of sth like what we do for
lookup_type_die/equate_type_number_to_die
and TYPE_SYMTAB_DIE?

> How does this version, which has been committed to the debug-early branch,
> look?

No further look right now, but I assume it's fine apart from the
block_map issue (and dwarf2out.c of course).

Richard.

> Aldy