diff mbox

[debug-early] emit locals early patchset

Message ID 54823AEF.7090104@redhat.com
State New
Headers show

Commit Message

Aldy Hernandez Dec. 5, 2014, 11:08 p.m. UTC 
On 10/28/14 10:28, Jason Merrill wrote:

My apologies for the long delay.  I was on PTO.

 > On 10/27/2014 08:00 PM, Aldy Hernandez wrote:
>> 2. Changes to gen_variable_die() to handle multiple passes (early/late
>> dwarf generation).
>>
>> A lot of this is complicated by the fact that old_die's are cached and
>> keyed by `tree', but an abstract instance and an inline instance share
>> trees, while dwarf2out_abstract_function() sets DECL_ABSTRACT_P behind
>> the scenes.
>>
>> The current support (and my changes) maintain this shared and delicate
>> design.  I wonder whether we could simplify a lot of this code by
>> unsharing these trees, but this may be beyond the scope of this work.
>
> Copying all the trees in a function just for debug generation?  No, that
> sounds undesirable.
>
>> 3. I've removed deferred_locations.  With multiple dwarf passes we can
>> do without them.
>
> Yay!
>
>> Kind words greatly appreciated.  Basically I'm looking for feedback
>> and positive reinforcement that this is all eventually useful
>
> This all looks very good, just a few nitpicks:

Yay!

>
>> -     instance tree [that has DW_AT_inline] should not contain any
>> +     instance tree [has DW_AT_inline] should not contain any
>
> This doesn't seem like an improvement.

Reverted.

>
>> +      /* Find and reuse a previously generated DW_TAG_subrange_type if
>> +     available.  */
>
> Let's expand this comment a bit to clarify how this works for
> multi-dimensional arrays.

Done.

>
>> -     abstract instance (origin != NULL), in which case we need a new
>> +     inline instance (origin != NULL), in which case we need a new DIE
>
> I think "concrete instance" is what you want here.

Done.

>
>> +          /* Even if we have locations, we need to recurse through
>> +         the locals to make sure they also have locations.  */
>
> Why?  What is adding a location to the function without doing the same
> for the locals?

Apparently, nothing.  I put a gcc_unreachable() there, and in all my 
tests, it never got triggered, so I've removed it.  Thanks.

>
>> +      current_function_has_inlines = 0;
>> +
>> +      /* The first time through decls_for_scope we will generate the
>> +     DIEs for the locals.  The second time, we fill in the
>> +     location info.  */
>> +      decls_for_scope (outer_scope, subr_die, 0);
>> +
>>        /* Emit a DW_TAG_variable DIE for a named return value.  */
>>        if (DECL_NAME (DECL_RESULT (decl)))
>>      gen_decl_die (DECL_RESULT (decl), NULL, subr_die);
>>
>> -      current_function_has_inlines = 0;
>> -      decls_for_scope (outer_scope, subr_die, 0);
>
> Why does this need to be reordered?

This may have been fall back from a previous version.  I have reverted 
the change.

>
>> +  /* If the compiler emitted a definition for the DECL declaration
>> +     and we already emitted a DIE for it, don't emit a second
>> +     DIE for it again. Allow re-declarations of DECLs that are
>> +     inside functions, though.  */
>> +  else if (old_die && !declaration && !local_scope_p (context_die))
>> +    return;
>
> What DECLs in functions need re-declaration?

This was already there.  It is pre-existing code that got moved down 
after the new caching code.

>
>> -  if (decl && (DECL_ABSTRACT_P (decl) || declaration || old_die ==
>> NULL))
>> +  if (decl && (DECL_ABSTRACT_P (decl) || declaration || old_die == NULL
>> +           /* If we make it to a specialization, we have already
>> +          handled the declaration by virtue of early dwarf.
>> +          If so, make a new assocation if available, so late
>> +          dwarf can find it.  */
>> +           || (specialization_p && old_die && old_die->dumped_early)))
>>      equate_decl_number_to_die (decl, var_die);
>
> Instead of old_die->dumped_early, I think it would make more sense to
> check early_dwarf_dumping; the reason we need to call
> equate_decl_number_to_die is because we're early-dumping the definition
> and we will need to find it again later.

I've rewritten the above as:

	       || (specialization_p && early_dwarf_dumping)))

>
>> +  else if (BLOCK_ABSTRACT_ORIGIN (stmt))
>>      {
>> +      /* If this is an inlined instance, create a new lexical die for
>> +     anything below to attach DW_AT_abstract_origin to.  */
>> +      stmt_die = new_die (DW_TAG_lexical_block, context_die, stmt);
>> +    }
>
> What if we early dumped this block?

What do you mean?  Would you like me to calls decls_for_scope earlier 
for abstract instances, or generate the DW_TAG_lexical_block die earlier 
for abstract instances, or what?

>
>> +      /* Variabled-lengthed types may be incomplete even if
>> +     TREE_ASM_WRITTEN.
>
> "variable-length", I think.

Fixed in the changelog and otherwise in the patch.

I have committed the attached patch.  We can iterate on the 
DW_TAG_lexical_block and DECL re-declaration issues in subsequent followups.

As usual, feel free to scream 
(https://www.youtube.com/watch?v=HLI4EuDckgM) if in violent disagreement.

Aldy
commit 7d0ab897d086ac8648928b1236dc88697c96d037
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Fri Dec 5 14:54:41 2014 -0800

    	* dwarf2out.c (check_die_inline): Revert previous comment change.
    	(add_subscript_info): Comment better.
    	(gen_subprogram_die): Do not try to generate local locations if we
    	already have locations as a whole.
    	Remove recurse_through_locals label.
    	Revert reordering of decls_for_scope call.
    	(gen_variable_die): Check early_dwarf_dumping when testing for
    	specialization_p instead of checking old_die->dumped_early.
diff mbox

Patch

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 9cb4ace..3528083 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -5558,7 +5558,7 @@  static void
 check_die_inline (dw_die_ref die)
 {
   /* A debugging information entry that is a member of an abstract
-     instance tree [has DW_AT_inline] should not contain any
+     instance tree [that has DW_AT_inline] should not contain any
      attributes which describe aspects of the subroutine which vary
      between distinct inlined expansions or distinct out-of-line
      expansions.  */
@@ -16637,7 +16637,16 @@  add_subscript_info (dw_die_ref type_die, tree type, bool collapse_p)
 	 here.  */
 
       /* Find and reuse a previously generated DW_TAG_subrange_type if
-	 available.  */
+	 available.
+
+         For multi-dimensional arrays, as we iterate through the
+         various dimensions in the enclosing for loop above, we also
+         iterate through the DIE children and pick at each
+         DW_TAG_subrange_type previously generated (if available).
+         Each child DW_TAG_subrange_type DIE describes the range of
+         the current dimension.  At this point we should have as many
+         DW_TAG_subrange_type's as we have dimensions in the
+         array.  */
       dw_die_ref subrange_die = NULL;
       if (child)
 	while (1)
@@ -17327,7 +17336,7 @@  gen_array_type_die (tree type, dw_die_ref context_die)
 
   bool collapse_nested_arrays = !is_ada ();
 
-  /* For variable-lengthed arrays that have been previously generated,
+  /* For variable-length arrays that have been previously generated,
      just fill in the possibly missing subscript info.  */
   if (TREE_ASM_WRITTEN (type)
       && TREE_CODE (type) == ARRAY_TYPE
@@ -17818,8 +17827,8 @@  gen_formal_parameter_die (tree node, tree origin, bool emit_name_p,
     }
 
   /* If we have a previously generated DIE, use it, unless this is an
-     inline instance (origin != NULL), in which case we need a new DIE
-     with a corresponding DW_AT_abstract_origin.  */
+     concrete instance (origin != NULL), in which case we need a new
+     DIE with a corresponding DW_AT_abstract_origin.  */
   bool reusing_die;
   if (parm_die && origin == NULL)
     reusing_die = true;
@@ -18418,14 +18427,7 @@  gen_subprogram_die (tree decl, dw_die_ref context_die)
 	     something we have already output.  */
 	  if (get_AT (old_die, DW_AT_low_pc)
 	      || get_AT (old_die, DW_AT_ranges))
-	    {
-	      /* Even if we have locations, we need to recurse through
-		 the locals to make sure they also have locations.  */
-	      if (dumped_early)
-		goto recurse_through_locals;
-
-	      return;
-	    }
+	    return;
 
 	  /* If we have no location information, this must be a
 	     partially generated DIE from early dwarf generation.
@@ -18835,7 +18837,6 @@  gen_subprogram_die (tree decl, dw_die_ref context_die)
     /* Add the calling convention attribute if requested.  */
     add_calling_convention_attribute (subr_die, decl);
 
- recurse_through_locals:
   /* Output Dwarf info for all of the stuff within the body of the function
      (if it has one - it may be just a declaration).
 
@@ -18859,6 +18860,10 @@  gen_subprogram_die (tree decl, dw_die_ref context_die)
       int call_site_note_count = 0;
       int tail_call_site_note_count = 0;
 
+      /* Emit a DW_TAG_variable DIE for a named return value.  */
+      if (DECL_NAME (DECL_RESULT (decl)))
+	gen_decl_die (DECL_RESULT (decl), NULL, subr_die);
+
       current_function_has_inlines = 0;
 
       /* The first time through decls_for_scope we will generate the
@@ -18866,10 +18871,6 @@  gen_subprogram_die (tree decl, dw_die_ref context_die)
 	 location info.  */
       decls_for_scope (outer_scope, subr_die, 0);
 
-      /* Emit a DW_TAG_variable DIE for a named return value.  */
-      if (DECL_NAME (DECL_RESULT (decl)))
-	gen_decl_die (DECL_RESULT (decl), NULL, subr_die);
-
       if (call_arg_locations && !dwarf_strict)
 	{
 	  struct call_arg_loc_node *ca_loc;
@@ -19312,7 +19313,7 @@  gen_variable_die (tree decl, tree origin, dw_die_ref context_die)
 		  handled the declaration by virtue of early dwarf.
 		  If so, make a new assocation if available, so late
 		  dwarf can find it.  */
-	       || (specialization_p && old_die && old_die->dumped_early)))
+	       || (specialization_p && early_dwarf_dumping)))
     equate_decl_number_to_die (decl, var_die);
 
  gen_variable_die_location: