diff mbox

PR debug/45682 - wrong struct DIE nesting with -fdebug-types-section

Message ID m3ty3vn0bc.fsf@redhat.com
State New
Headers show

Commit Message

Dodji Seketeli Jan. 16, 2012, 3:17 p.m. UTC 
Cary Coutant <ccoutant@google.com> writes:

> gcc/testsuite/ChangeLog:
>
> 	PR debug/45682
> 	* g++.dg/debug/dwarf2/nested-3.C: New test.

As per this comment from Jakub in another subthread:

    Just a testcase comment, I bet this will surely fail on Darwin
    or other targets that aren't capable of merging string sections.
    So, you should add -fno-merge-debug-strings to dg-options
    and adjust the example and regexp for that, that way
    it will be the same on all targets.

I have updated the test case in your patch as below.  Please note that
lines of the patch in your mail were wrapped so I had to edit the patch
by hand to have it apply.

Tested on x86_64-unknown-linux-gnu against trunk.

Thanks.

gcc/ChangeLog:

	PR debug/45682
	* dwarf2out.c (copy_declaration_context): Return ref to parent
	of declaration DIE, if necessary.  Update caller.
	(remove_child_or_replace_with_skeleton): Add new parameter; update
	caller.  Place skeleton DIE under parent DIE of original declaration.

gcc/testsuite/ChangeLog:

	PR debug/45682
	* g++.dg/debug/dwarf2/nested-3.C: New test.

Comments

Jason Merrill Jan. 17, 2012, 8:41 p.m. UTC | #1 
> +      /* If the original DIE was a specification, we need to put
> +         the skeleton under the parent DIE of the declaration.  */
> +      if (new_parent != NULL)
> +	{
> +	  remove_child_with_prev (child, prev);
> +	  add_child_die (new_parent, skeleton);
> +	}

This adds a new declaration without removing the old one, though it's 
later removed by prune_unused_types.

It also seems to me that copy_declaration_context and 
remove_child_or_replace_with_skeleton should be combined if they need to 
work together like this.

Jason
Cary Coutant Jan. 17, 2012, 11:52 p.m. UTC | #2 
On Tue, Jan 17, 2012 at 12:41 PM, Jason Merrill <jason@redhat.com> wrote:
>> +      /* If the original DIE was a specification, we need to put
>> +         the skeleton under the parent DIE of the declaration.  */
>> +      if (new_parent != NULL)
>> +       {
>> +         remove_child_with_prev (child, prev);
>> +         add_child_die (new_parent, skeleton);
>> +       }
>
>
> This adds a new declaration without removing the old one, though it's later
> removed by prune_unused_types.

Yes. At this point, I don't have the "prev" pointer needed to remove
the old one efficiently, and since it will get pruned anyway, it
seemed more efficient to let that happen. If it doesn't actually get
pruned, I think there's no harm done other than an unnecessary DIE.
Would you consider it OK with a comment?

> It also seems to me that copy_declaration_context and
> remove_child_or_replace_with_skeleton should be combined if they need to
> work together like this.

How about if I call copy_declaration_context directly from
remove_child_or_replace_with_skeleton, instead of calling them
sequentially in break_out_comdat_types? Or would you prefer combining
them into a single function?

-cary
Jason Merrill Jan. 19, 2012, 3:17 p.m. UTC | #3 
On 01/17/2012 06:52 PM, Cary Coutant wrote:
> Would you consider it OK with a comment?

Yes.

> How about if I call copy_declaration_context directly from
> remove_child_or_replace_with_skeleton, instead of calling them
> sequentially in break_out_comdat_types?

OK.

Jason
diff mbox

Patch

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index f9f4295..4f6bcad 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -3302,11 +3302,12 @@  static int should_move_die_to_comdat (dw_die_ref);
 static dw_die_ref clone_as_declaration (dw_die_ref);
 static dw_die_ref clone_die (dw_die_ref);
 static dw_die_ref clone_tree (dw_die_ref);
-static void copy_declaration_context (dw_die_ref, dw_die_ref);
+static dw_die_ref copy_declaration_context (dw_die_ref, dw_die_ref);
 static void generate_skeleton_ancestor_tree (skeleton_chain_node *);
 static void generate_skeleton_bottom_up (skeleton_chain_node *);
 static dw_die_ref generate_skeleton (dw_die_ref);
 static dw_die_ref remove_child_or_replace_with_skeleton (dw_die_ref,
+                                                         dw_die_ref,
                                                          dw_die_ref);
 static void break_out_comdat_types (dw_die_ref);
 static dw_die_ref copy_ancestor_tree (dw_die_ref, dw_die_ref, htab_t);
@@ -7075,11 +7076,12 @@  clone_as_declaration (dw_die_ref die)
    AT_specification attribute, it also includes attributes and children
    attached to the specification.  */
 
-static void
+static dw_die_ref
 copy_declaration_context (dw_die_ref unit, dw_die_ref die)
 {
   dw_die_ref decl;
   dw_die_ref new_decl;
+  dw_die_ref new_parent = NULL;
 
   decl = get_AT_ref (die, DW_AT_specification);
   if (decl == NULL)
@@ -7090,6 +7092,10 @@  copy_declaration_context (dw_die_ref unit, dw_die_ref die)
       dw_die_ref c;
       dw_attr_ref a;
 
+      /* The original DIE will be changed to a declaration, and must
+         be moved to be a child of the original declaration DIE.  */
+      new_parent = decl->die_parent;
+
       /* Copy the type node pointer from the new DIE to the original
          declaration DIE so we can forward references later.  */
       decl->die_id.die_type_node = die->die_id.die_type_node;
@@ -7118,6 +7124,8 @@  copy_declaration_context (dw_die_ref unit, dw_die_ref die)
           add_AT_specification (die, new_decl);
         }
     }
+
+  return new_parent;
 }
 
 /* Generate the skeleton ancestor tree for the given NODE, then clone
@@ -7201,7 +7209,7 @@  generate_skeleton (dw_die_ref die)
   return node.new_die;
 }
 
-/* Remove the DIE from its parent, possibly replacing it with a cloned
+/* Remove the CHILD DIE from its parent, possibly replacing it with a cloned
    declaration.  The original DIE will be moved to a new compile unit
    so that existing references to it follow it to the new location.  If
    any of the original DIE's descendants is a declaration, we need to
@@ -7209,7 +7217,8 @@  generate_skeleton (dw_die_ref die)
    declarations back into the skeleton tree.  */
 
 static dw_die_ref
-remove_child_or_replace_with_skeleton (dw_die_ref child, dw_die_ref prev)
+remove_child_or_replace_with_skeleton (dw_die_ref child, dw_die_ref prev,
+				       dw_die_ref new_parent)
 {
   dw_die_ref skeleton;
 
@@ -7219,7 +7228,16 @@  remove_child_or_replace_with_skeleton (dw_die_ref child, dw_die_ref prev)
   else
     {
       skeleton->die_id.die_type_node = child->die_id.die_type_node;
-      replace_child (child, skeleton, prev);
+
+      /* If the original DIE was a specification, we need to put
+         the skeleton under the parent DIE of the declaration.  */
+      if (new_parent != NULL)
+	{
+	  remove_child_with_prev (child, prev);
+	  add_child_die (new_parent, skeleton);
+	}
+      else
+	replace_child (child, skeleton, prev);
     }
 
   return skeleton;
@@ -7247,7 +7265,7 @@  break_out_comdat_types (dw_die_ref die)
     next = (c == first ? NULL : c->die_sib);
     if (should_move_die_to_comdat (c))
       {
-        dw_die_ref replacement;
+        dw_die_ref replacement, new_parent;
 	comdat_type_node_ref type_node;
 
         /* Create a new type unit DIE as the root for the new tree, and
@@ -7265,10 +7283,11 @@  break_out_comdat_types (dw_die_ref die)
 
         /* Copy the declaration context, attributes, and children of the
            declaration into the new compile unit DIE.  */
-	copy_declaration_context (unit, c);
+	new_parent = copy_declaration_context (unit, c);
 
         /* Remove this DIE from the main CU.  */
-	replacement = remove_child_or_replace_with_skeleton (c, prev);
+	replacement = remove_child_or_replace_with_skeleton (c, prev,
+							     new_parent);
 
         /* Break out nested types into their own type units.  */
         break_out_comdat_types (c);
diff --git a/gcc/testsuite/g++.dg/debug/dwarf2/nested-3.C b/gcc/testsuite/g++.dg/debug/dwarf2/nested-3.C
new file mode 100644
index 0000000..3b48c34
--- /dev/null
+++ b/gcc/testsuite/g++.dg/debug/dwarf2/nested-3.C
@@ -0,0 +1,54 @@ 
+// Origin: PR debug/45682
+// { dg-options "-g -fno-merge-debug-strings -gdwarf-4 -dA -fdebug-types-section" }
+
+namespace thread {
+
+class Executor {
+ public:
+  static Executor* CurrentExecutor();
+};
+
+}
+
+namespace thread {
+
+Executor* Executor::CurrentExecutor() {
+  return 0;
+}
+
+}
+
+thread::Executor *te;
+
+int
+main ()
+{
+    return 0;
+}
+
+// We want to express the fact that the DIE for the definition of
+// 'Executor::CurrentExecutor' is a grand-child of the DIE for the
+// namespace 'thread'.  We must have something like this output:
+//	.uleb128 0x8    # (DIE (0x29) DW_TAG_namespace)
+//	.long   .LASF0  # DW_AT_name: "thread"
+//	.byte   0x1     # DW_AT_decl_file (.../testsuite/g++.dg/debug/dwarf2/nested-3.C)
+//	.byte   0x4     # DW_AT_decl_line
+//	.long   0x4b    # DW_AT_sibling
+//	.uleb128 0x9    # (DIE (0x34) DW_TAG_class_type)
+//	.long   .LASF1  # DW_AT_name: "Executor"
+//			# DW_AT_declaration
+//	.uleb128 0x5    # (DIE (0x39) DW_TAG_subprogram)
+//			# DW_AT_external
+//	.long   .LASF2  # DW_AT_name: "CurrentExecutor"
+//	.byte   0x1     # DW_AT_decl_file (.../testsuite/g++.dg/debug/dwarf2/nested-3.C)
+//	.byte   0x8     # DW_AT_decl_line
+//	.long   .LASF3  # DW_AT_linkage_name: "_ZN6thread8Executor15CurrentExecutorEv"
+//	.long   0x4b    # DW_AT_type
+//	.byte   0x1     # DW_AT_accessibility
+//			# DW_AT_declaration
+//	.byte   0       # end of children of DIE 0x34
+//	.byte   0       # end of children of DIE 0x29
+//
+//     Hence the scary regexp:
+//
+//     { dg-final { scan-assembler "\[^\n\r\]*\\(DIE \\(0x(\[0-9a-f\]+)\\) DW_TAG_namespace\\)\[\n\r\]+\[^\n\r\]*\"thread\[^\n\r\]*\"\[^\n\r\]*DW_AT_name\[\n\r\]+(\[^\n\r\]*\[\n\r\]+)+\[^\n\r\]*DW_AT_sibling\[\n\r\]+\[^\n\r\]*\\(DIE \\(0x(\[0-9a-f\]+)\\) DW_TAG_class_type\\)\[\n\r\]+\[^\n\r\]*DW_AT_name: \"Executor\"\[\n\r\]+\[^\n\r\]*DW_AT_declaration\[\n\r\]+\[^\n\r\]*\\(DIE\[^\n\r\]*DW_TAG_subprogram\\)\[\n\r\]+(\[^\n\r\]*\[\n\r\]+)+\[^\n\r\]*DW_AT_name: \"CurrentExecutor\"\[\n\r\]+(\[^\n\r\]*\[\n\r\]+)+(\[^\n\r\]*\[\n\r\]+)+\[^\n\r\]*end of children of DIE 0x\\3\[\n\r]+\[^\n\r\]*end of children of DIE 0x\\1\[\n\r]+" } }