[10/10] debug-early merge: compiler proper
diff mbox

Message ID 555F3954.2010203@redhat.com
State New
Headers show

Commit Message

Aldy Hernandez May 22, 2015, 2:12 p.m. UTC
On 05/22/2015 07:23 AM, Richard Biener wrote:
> 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:

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

Oh, I would love to.  I just didn't want to rip things apart elsewhere 
until I was sure you guys were on board with the approach.

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

Absolutely!  The attached patch removes block_map in favor of BLOCK_DIE. 
  I did not add lookup_block_die/equate_block_number_to_die abstractions 
because I think BLOCK_DIE is pretty straightforward.

The attached patch is against mainline.  I also ported it to the branch 
for testing, and neither the branch nor mainline exhibit any regressions.

Tested on x86-64 Linux with --enable-languages=all,go,ada.

OK for trunk?

Aldy
commit 9a82ff7b044a8d17eaaaec5eaec3e73f836224df
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Fri May 22 10:07:17 2015 -0400

    	* dwarf2out.c: Remove block_map.
    	(gen_call_site_die): Replace block_map use with BLOCK_DIE.
    	(gen_lexical_block_die): Same.
    	(dwarf2out_function_decl): Remove block_map use.
    	(dwarf2out_c_finalize): Same.
    	* tree-core.h (struct tree_block): Add die field.
    	* tree.h (BLOCK_DIE): New.

Comments

Richard Biener May 27, 2015, 12:47 p.m. UTC | #1
On Fri, May 22, 2015 at 4:12 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
> On 05/22/2015 07:23 AM, Richard Biener wrote:
>>
>> 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:
>
>
>>>> 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.
>
>
> Oh, I would love to.  I just didn't want to rip things apart elsewhere until
> I was sure you guys were on board with the approach.
>
>> 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?
>
>
> Absolutely!  The attached patch removes block_map in favor of BLOCK_DIE.  I
> did not add lookup_block_die/equate_block_number_to_die abstractions because
> I think BLOCK_DIE is pretty straightforward.
>
> The attached patch is against mainline.  I also ported it to the branch for
> testing, and neither the branch nor mainline exhibit any regressions.
>
> Tested on x86-64 Linux with --enable-languages=all,go,ada.
>
> OK for trunk?

Ok.

Thanks,
Richard.

> Aldy
>

Patch
diff mbox

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index cc7ac84..15c545e 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -2908,10 +2908,6 @@  static int call_site_count = -1;
 /* Number of tail call sites in the current function.  */
 static int tail_call_site_count = -1;
 
-/* Vector mapping block numbers to DW_TAG_{lexical_block,inlined_subroutine}
-   DIEs.  */
-static vec<dw_die_ref> block_map;
-
 /* A cached location list.  */
 struct GTY ((for_user)) cached_dw_loc_list_def {
   /* The DECL_UID of the decl that this entry describes.  */
@@ -18368,8 +18364,7 @@  gen_call_site_die (tree decl, dw_die_ref subr_die,
 	 && block != DECL_INITIAL (decl)
 	 && TREE_CODE (block) == BLOCK)
     {
-      if (block_map.length () > BLOCK_NUMBER (block))
-	stmt_die = block_map[BLOCK_NUMBER (block)];
+      stmt_die = BLOCK_DIE (block);
       if (stmt_die)
 	break;
       block = BLOCK_SUPERCONTEXT (block);
@@ -19469,11 +19464,7 @@  gen_lexical_block_die (tree stmt, dw_die_ref context_die)
   dw_die_ref stmt_die = new_die (DW_TAG_lexical_block, context_die, stmt);
 
   if (call_arg_locations)
-    {
-      if (block_map.length () <= BLOCK_NUMBER (stmt))
-	block_map.safe_grow_cleared (BLOCK_NUMBER (stmt) + 1);
-      block_map[BLOCK_NUMBER (stmt)] = stmt_die;
-    }
+    BLOCK_DIE (stmt) = stmt_die;
 
   if (! BLOCK_ABSTRACT (stmt) && TREE_ASM_WRITTEN (stmt))
     add_high_low_attributes (stmt, stmt_die);
@@ -19506,11 +19497,7 @@  gen_inlined_subroutine_die (tree stmt, dw_die_ref context_die)
 	= new_die (DW_TAG_inlined_subroutine, context_die, stmt);
 
       if (call_arg_locations)
-	{
-	  if (block_map.length () <= BLOCK_NUMBER (stmt))
-	    block_map.safe_grow_cleared (BLOCK_NUMBER (stmt) + 1);
-	  block_map[BLOCK_NUMBER (stmt)] = subr_die;
-	}
+	BLOCK_DIE (stmt) = subr_die;
       add_abstract_origin_attribute (subr_die, decl);
       if (TREE_ASM_WRITTEN (stmt))
         add_high_low_attributes (stmt, subr_die);
@@ -21407,7 +21394,6 @@  dwarf2out_function_decl (tree decl)
   call_arg_loc_last = NULL;
   call_site_count = -1;
   tail_call_site_count = -1;
-  block_map.release ();
   decl_loc_table->empty ();
   cached_dw_loc_list_table->empty ();
 }
@@ -25008,7 +24994,6 @@  dwarf2out_c_finalize (void)
   call_arg_loc_last = NULL;
   call_site_count = -1;
   tail_call_site_count = -1;
-  //block_map = NULL;
   cached_dw_loc_list_table = NULL;
   abbrev_die_table = NULL;
   abbrev_die_table_allocated = 0;
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 {
diff --git a/gcc/tree.h b/gcc/tree.h
index 2bac698..ad32af0 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -1589,6 +1589,7 @@  extern void protected_set_expr_location (tree, location_t);
 #define BLOCK_CHAIN(NODE) (BLOCK_CHECK (NODE)->block.chain)
 #define BLOCK_ABSTRACT_ORIGIN(NODE) (BLOCK_CHECK (NODE)->block.abstract_origin)
 #define BLOCK_ABSTRACT(NODE) (BLOCK_CHECK (NODE)->block.abstract_flag)
+#define BLOCK_DIE(NODE) (BLOCK_CHECK (NODE)->block.die)
 
 /* True if BLOCK has the same ranges as its BLOCK_SUPERCONTEXT.  */
 #define BLOCK_SAME_RANGE(NODE) (BLOCK_CHECK (NODE)->base.u.bits.nameless_flag)