On Wed, May 20, 2015 at 5:50 PM, Aldy Hernandez <al...@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 <al...@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
