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

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

[Dodji Seketeli]

Hello,

In the example of the patch below, the DIE for the definition of the
class 'Executor', when put in the type unit section is not put under
the DIE for the 'thread' namespace.  Rather, it's put under the DIE
for the global namespace.

One thing to note is that the DIE for the declaration of 'Executor'
(the DIE that has a DW_AT_declaration attribute, and which is referred
to by the DW_AT_specification attribute of the DIE for the definition
of 'Executor') is correctly positioned as a children of the DIE for
the 'thread' namespace.

My understanding is that during the moving of a given type DIE into
its type unit section, we correctly move the declaration DIE of that
type along with its ancestor tree, but we fail to do the same for the
definition DIE.  For the later, we just omit its ancestor DIE in the
process.

Fixed thus, bootstrapped and tested on x86_64-unknown-linux-gnu
against trunk.

gcc/

	PR debug/45682
	* dwarf2out.c (copy_declaration_context): Return the copied DIE.
	(break_out_comdat_types): Move the definition DIE to under the
	same DIE as the declaration DIE.

gcc/testsuite/

	PR debug/45682
	* g++.dg/debug/dwarf2/nested-3.C: New test.
---
 gcc/dwarf2out.c                              |   13 ++++++----
 gcc/testsuite/g++.dg/debug/dwarf2/nested-3.C |   34 ++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/debug/dwarf2/nested-3.C

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 850eb55..96c247c 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -3302,7 +3302,7 @@ 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);
@@ -7075,11 +7075,11 @@ 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_decl = NULL;
 
   decl = get_AT_ref (die, DW_AT_specification);
   if (decl == NULL)
@@ -7118,6 +7118,7 @@ copy_declaration_context (dw_die_ref unit, dw_die_ref die)
           add_AT_specification (die, new_decl);
         }
     }
+  return new_decl ;
 }
 
 /* Generate the skeleton ancestor tree for the given NODE, then clone
@@ -7247,7 +7248,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, copied = NULL;
 	comdat_type_node_ref type_node;
 
         /* Create a new type unit DIE as the root for the new tree, and
@@ -7265,7 +7266,7 @@ 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);
+	copied = copy_declaration_context (unit, c);
 
         /* Remove this DIE from the main CU.  */
 	replacement = remove_child_or_replace_with_skeleton (c, prev);
@@ -7274,6 +7275,8 @@ break_out_comdat_types (dw_die_ref die)
         break_out_comdat_types (c);
 
         /* Add the DIE to the new compunit.  */
+	if (copied != NULL && copied->die_parent)
+	  unit = copied->die_parent;
 	add_child_die (unit, c);
 
         if (replacement != NULL)
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..847afd7
--- /dev/null
+++ b/gcc/testsuite/g++.dg/debug/dwarf2/nested-3.C
@@ -0,0 +1,34 @@
+// Origin: PR debug/45682
+// { dg-options "-g -gdwarf-4 -dA -fdebug-types-section" }
+
+namespace thread {
+    class Executor {};
+}
+
+thread::Executor *te;
+
+int
+main ()
+{
+    return 0;
+}
+
+// We want to express the fact that the DIE for the definition of
+// 'Executor' is a child of the DIE for the namespace 'thread'. E.g,
+// we must have this outout:
+// 	.uleb128 0x2	# (DIE (0x25) DW_TAG_namespace)
+//	.long	.LASF0	# DW_AT_name: "thread"
+//			# DW_AT_declaration
+//	.uleb128 0x3	# (DIE (0x2a) DW_TAG_class_type)
+//	.long	.LASF1	# DW_AT_name: "Executor"
+//			# DW_AT_declaration
+//	.uleb128 0x4	# (DIE (0x2f) DW_TAG_class_type)
+//	.byte	0x1	# DW_AT_byte_size
+//	.byte	0x1	# DW_AT_decl_file (../../prtests/test-PR45682-2.cc)
+//	.byte	0x2	# DW_AT_decl_line
+//	.long	0x2a	# DW_AT_specification
+//	.byte	0	# end of children of DIE 0x25
+//
+//	Hence the scary regexp:
+//
+//	{ dg-final { scan-assembler "\[^\n\r\]*\\(DIE \\(0x(\[0-9a-f\]+)\\) DW_TAG_namespace\\)\[\n\r\]+\[^\n\r\]*DW_AT_name: \"thread\"\[\n\r\]+\[^\n\r\]*DW_AT_declaration\[\n\r\]+\[^\n\r\]*\\(DIE \\(0x(\[0-9a-f\]+)\\)\[^\n\r\]*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_class_type\\)\[\n\r\]+(\[^\n\r\]*\[\n\r\]+)+\[^\n\r\]*0x\\2\[^\n\r\]*\[\n\r\]+\[^\n\r\]*end of children of DIE 0x\\1" } }
-- 
1.7.6.4


-- 
		Dodji

Re: [PATCH] PR debug/45682 - wrong struct DIE nesting with -fdebug-types-section

[Jakub Jelinek]

On Thu, Jan 05, 2012 at 11:47:49PM +0100, Dodji Seketeli wrote:
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/debug/dwarf2/nested-3.C
> @@ -0,0 +1,34 @@
> +// Origin: PR debug/45682
> +// { dg-options "-g -gdwarf-4 -dA -fdebug-types-section" }
> +
> +namespace thread {
> +    class Executor {};
> +}
> +
> +thread::Executor *te;
> +
> +int
> +main ()
> +{
> +    return 0;
> +}
> +
> +// We want to express the fact that the DIE for the definition of
> +// 'Executor' is a child of the DIE for the namespace 'thread'. E.g,
> +// we must have this outout:
> +// 	.uleb128 0x2	# (DIE (0x25) DW_TAG_namespace)
> +//	.long	.LASF0	# DW_AT_name: "thread"
> +//			# DW_AT_declaration
> +//	.uleb128 0x3	# (DIE (0x2a) DW_TAG_class_type)
> +//	.long	.LASF1	# DW_AT_name: "Executor"
> +//			# DW_AT_declaration
> +//	.uleb128 0x4	# (DIE (0x2f) DW_TAG_class_type)
> +//	.byte	0x1	# DW_AT_byte_size
> +//	.byte	0x1	# DW_AT_decl_file (../../prtests/test-PR45682-2.cc)
> +//	.byte	0x2	# DW_AT_decl_line
> +//	.long	0x2a	# DW_AT_specification
> +//	.byte	0	# end of children of DIE 0x25
> +//
> +//	Hence the scary regexp:
> +//
> +//	{ dg-final { scan-assembler "\[^\n\r\]*\\(DIE \\(0x(\[0-9a-f\]+)\\) DW_TAG_namespace\\)\[\n\r\]+\[^\n\r\]*DW_AT_name: \"thread\"\[\n\r\]+\[^\n\r\]*DW_AT_declaration\[\n\r\]+\[^\n\r\]*\\(DIE \\(0x(\[0-9a-f\]+)\\)\[^\n\r\]*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_class_type\\)\[\n\r\]+(\[^\n\r\]*\[\n\r\]+)+\[^\n\r\]*0x\\2\[^\n\r\]*\[\n\r\]+\[^\n\r\]*end of children of DIE 0x\\1" } }

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.

	Jakub

Re: [PATCH] PR debug/45682 - wrong struct DIE nesting with -fdebug-types-section

[Cary Coutant]

> In the example of the patch below, the DIE for the definition of the
> class 'Executor', when put in the type unit section is not put under
> the DIE for the 'thread' namespace.  Rather, it's put under the DIE
> for the global namespace.

I don't think this is the problem that PR 45682 is complaining about.
Given the test case in your patch, the type unit looks like this:

  type_unit
  + namespace ("thread")
  + + L1: class_type ("Executor", declaration)
  + class_type (specification: L1)

The definition DIE is *supposed* to appear at the outer level, with
the specification DIE pointing to the declaration, which provides its
context. This is the way it would have appeared in the comp unit DIE
before splitting it out into a type unit. (And what it would look like
with -fno-debug-types-section.)

What PR 45682 is complaining about is that the skeleton DIE left
behind for class Executor is not inside the namespace. That problem,
however, is only visible with the full test case provided in the PR.
There, the DIE structure in .debug_types is good, but the DIE
structure in .debug_info is wrong:

  compile_unit
  + namespace ("thread")
  + class_type ("Executor", declaration)
  + + L1: subprogram ("CurrentExecutor", external)
  + ...
  + subprogram (specification: L1)
  + ...

Here, the DIE for class_type Executor should be under the namespace
DIE, but isn't. I don't think your patch fixes that problem.

I think the real problem is that the skeleton declaration DIE for
class Executor is being inserted under the wrong DIE -- probably in
remove_child_or_replace_with_skeleton().

Thanks for looking at this, though! It's been on my TODO list for a
while, but had dropped below the fold, and I'd forgotten about it.

-cary

Re: [PATCH] PR debug/45682 - wrong struct DIE nesting with -fdebug-types-section

[Cary Coutant]

> I think the real problem is that the skeleton declaration DIE for
> class Executor is being inserted under the wrong DIE -- probably in
> remove_child_or_replace_with_skeleton().

The problem is that we're inserting the skeleton DIE for class
Executor in place of the original specification DIE at the top level
under the compile_unit DIE. The original had a DW_AT_specification
pointing to a declaration DIE under namespace thread, but the skeleton
is a declaration, so it needs to be directly under the namespace DIE.
The original declaration DIE gets pruned because there are no
references left that point to it.

The patch below fixes that by noting that the DIE we're replacing had
a specification DIE, and when it's time to replace it with the
skeleton DIE, we need to insert it under the original declaration's
parent.

Take a look at this and see if it makes sense; if it does, I'll add a
ChangeLog and a testcase on Monday, bootstrap, test, and submit. I did
verify that gdb can read the resulting debug info for the test case in
the original PR.

-cary


diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 695b7b1..f1632ea 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -3307,11 +3307,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);
@@ -7160,11 +7161,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)
@@ -7175,6 +7177,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;
@@ -7203,6 +7209,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
@@ -7286,7 +7294,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
@@ -7294,7 +7302,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;

@@ -7304,7 +7313,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;
@@ -7332,7 +7350,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
@@ -7350,10 +7368,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);
@@ -7705,7 +7724,7 @@ size_of_die (dw_die_ref die)
   unsigned long size = 0;
   dw_attr_ref a;
   unsigned ix;
-  dwarf_form form;
+  enum dwarf_form form;

   size += size_of_uleb128 (die->die_abbrev);
   FOR_EACH_VEC_ELT (dw_attr_node, die->die_attr, ix, a)

Re: [PATCH] PR debug/45682 - wrong struct DIE nesting with -fdebug-types-section

[Dodji Seketeli]

Cary Coutant <ccoutant@google.com> writes:

> Take a look at this and see if it makes sense;

Yes, it does make sense to me.  Thanks.

-- 
		Dodji

Re: [PATCH] PR debug/45682 - wrong struct DIE nesting with -fdebug-types-section

[Cary Coutant]

Here's the final patch, with testcase. Bootstrapped and tested on
x86_64 with no regressions.

I'm not sure of the rules here -- since this patch was in process
before Stage 3 closed, is it OK for 4.7? Or do I need to hold this
until Stage 1 opens for 4.8?

-cary


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.


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..dade3d6
--- /dev/null
+++ b/gcc/testsuite/g++.dg/debug/dwarf2/nested-3.C
@@ -0,0 +1,54 @@
+// Origin: PR debug/45682
+// { dg-options "-g -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\]*DW_AT_name:
\"thread\"\[\n\r\]+(\[^\n\r\]*\[\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]+" } }

Re: [PATCH] PR debug/45682 - wrong struct DIE nesting with -fdebug-types-section

[Dodji Seketeli]

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.

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]+" } }


-- 
		Dodji

Re: [PATCH] PR debug/45682 - wrong struct DIE nesting with -fdebug-types-section

[Jason Merrill]

> +      /* 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

Re: [PATCH] PR debug/45682 - wrong struct DIE nesting with -fdebug-types-section

[Cary Coutant]

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

Re: [PATCH] PR debug/45682 - wrong struct DIE nesting with -fdebug-types-section

[Jason Merrill]

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

Re: [PATCH] PR debug/45682 - wrong struct DIE nesting with -fdebug-types-section

[Cary Coutant]

[-- Attachment #1: Type: text/plain, Size: 1009 bytes --]

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

Updated patch attached. I fixed the regex so the test is independent
of whether the strings are emitted with DW_FORM_string or
DW_FORM_strp.

Bootstrapped on x86_64, no regressions.

OK for trunk, or hold for next stage 1?

-cary


2012-01-19   Cary Coutant  <ccoutant@google.com>
	     Dodji Seketeli  <dodji@redhat.com>

gcc/ChangeLog:

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

gcc/testsuite/ChangeLog:

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

[-- Attachment #2: gcc-pr45682-patch-2.txt --]
[-- Type: text/plain, Size: 8634 bytes --]

2012-01-19   Cary Coutant  <ccoutant@google.com>
	     Dodji Seketeli  <dodji@redhat.com>

gcc/ChangeLog:

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

gcc/testsuite/ChangeLog:

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


commit 959d3b18ff6812a0316960ac49b283c11fd20460
Author: Cary Coutant <ccoutant@google.com>
Date:   Thu Jan 19 14:00:59 2012 -0800

    PR debug/45682: Place skeleton DIE under parent of original decl.

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index f9f4295..6101087 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);
@@ -7070,16 +7071,17 @@ clone_as_declaration (dw_die_ref die)
   return clone;
 }
 
-/* Copy the declaration context to the new compile unit DIE.  This includes
+/* Copy the declaration context to the new type unit DIE.  This includes
    any surrounding namespace or type declarations.  If the DIE has an
    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 orig_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.  */
+      orig_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 orig_parent;
 }
 
 /* Generate the skeleton ancestor tree for the given NODE, then clone
@@ -7201,17 +7209,23 @@ generate_skeleton (dw_die_ref die)
   return node.new_die;
 }
 
-/* Remove the 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
-   replace the original DIE with a skeleton tree and move the
-   declarations back into the skeleton tree.  */
+/* Remove the CHILD DIE from its parent, possibly replacing it with a cloned
+   declaration.  The original DIE is 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 replace the
+   original DIE with a skeleton tree and move the 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 unit, dw_die_ref child,
+				       dw_die_ref prev)
 {
-  dw_die_ref skeleton;
+  dw_die_ref skeleton, orig_parent;
+
+  /* Copy the declaration context to the type unit DIE.  If the returned
+     ORIG_PARENT is not NULL, the skeleton needs to be added as a child of
+     that DIE.  */
+  orig_parent = copy_declaration_context (unit, c);
 
   skeleton = generate_skeleton (child);
   if (skeleton == NULL)
@@ -7219,7 +7233,19 @@ 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.
+	 This leaves the original declaration in the tree, but
+	 it will be pruned later since there are no longer any
+	 references to it.  */
+      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 +7273,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
@@ -7264,11 +7290,9 @@ break_out_comdat_types (dw_die_ref die)
         generate_type_signature (c, type_node);
 
         /* Copy the declaration context, attributes, and children of the
-           declaration into the new compile unit DIE.  */
-	copy_declaration_context (unit, c);
-
-        /* Remove this DIE from the main CU.  */
-	replacement = remove_child_or_replace_with_skeleton (c, prev);
+           declaration into the new type unit DIE, then remove this DIE
+	   from the main CU (or replace it with a skeleton if necessary).  */
+	replacement = remove_child_or_replace_with_skeleton (unit, c, prev);
 
         /* 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..707f02d
--- /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)
+//	.ascii "thread\0"	# DW_AT_name
+//	.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   .LASF0	# DW_AT_name: "Executor"
+//			# DW_AT_declaration
+//	.uleb128 0x5	# (DIE (0x39) DW_TAG_subprogram)
+//			# DW_AT_external
+//	.long   .LASF1	# 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   .LASF2	# 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\]+(\[^\n\r\]*\[\n\r\]+)+\[^\n\r\]*\\(DIE \\(0x(\[0-9a-f\]+)\\) DW_TAG_class_type\\)\[\n\r\]+\[^\n\r\]*\"Executor\[^\n\r\]+\[\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\]*\"CurrentExecutor\[^\n\r\]+\[\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]+" } }

Re: [PATCH] PR debug/45682 - wrong struct DIE nesting with -fdebug-types-section

[Jason Merrill]

On 01/19/2012 05:47 PM, Cary Coutant wrote:
> +/* Copy the declaration context to the new type unit DIE.  This includes
>      any surrounding namespace or type declarations.  If the DIE has an
>      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)

Need to update the comment to cover the return value.  OK for 4.7 with 
that change.

Jason

Re: [PATCH] PR debug/45682 - wrong struct DIE nesting with -fdebug-types-section

[Cary Coutant]

[-- Attachment #1: Type: text/plain, Size: 158 bytes --]

> Need to update the comment to cover the return value.  OK for 4.7 with that
> change.

Thanks, committed as r183348. Here's the final patch...

-cary

[-- Attachment #2: gcc-pr45682-patch-3.txt --]
[-- Type: text/plain, Size: 8433 bytes --]

2012-01-20   Cary Coutant  <ccoutant@google.com>
	     Dodji Seketeli  <dodji@redhat.com>

gcc/ChangeLog:

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

gcc/testsuite/ChangeLog:

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


commit bdde2c9d7232fcefe4bc578658f2397d49334b90
Author: Cary Coutant <ccoutant@google.com>
Date:   Thu Jan 19 14:00:59 2012 -0800

    PR debug/45682: Place skeleton DIE under parent of original decl.

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index f9f4295..ed279ba 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);
@@ -7070,16 +7071,18 @@ clone_as_declaration (dw_die_ref die)
   return clone;
 }
 
-/* Copy the declaration context to the new compile unit DIE.  This includes
+/* Copy the declaration context to the new type unit DIE.  This includes
    any surrounding namespace or type declarations.  If the DIE has an
    AT_specification attribute, it also includes attributes and children
-   attached to the specification.  */
+   attached to the specification, and returns a pointer to the original
+   parent of the declaration DIE.  Returns NULL otherwise.  */
 
-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 orig_parent = NULL;
 
   decl = get_AT_ref (die, DW_AT_specification);
   if (decl == NULL)
@@ -7090,6 +7093,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.  */
+      orig_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 +7125,8 @@ copy_declaration_context (dw_die_ref unit, dw_die_ref die)
           add_AT_specification (die, new_decl);
         }
     }
+
+  return orig_parent;
 }
 
 /* Generate the skeleton ancestor tree for the given NODE, then clone
@@ -7201,17 +7210,23 @@ generate_skeleton (dw_die_ref die)
   return node.new_die;
 }
 
-/* Remove the 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
-   replace the original DIE with a skeleton tree and move the
-   declarations back into the skeleton tree.  */
+/* Remove the CHILD DIE from its parent, possibly replacing it with a cloned
+   declaration.  The original DIE is 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 replace the
+   original DIE with a skeleton tree and move the 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 unit, dw_die_ref child,
+				       dw_die_ref prev)
 {
-  dw_die_ref skeleton;
+  dw_die_ref skeleton, orig_parent;
+
+  /* Copy the declaration context to the type unit DIE.  If the returned
+     ORIG_PARENT is not NULL, the skeleton needs to be added as a child of
+     that DIE.  */
+  orig_parent = copy_declaration_context (unit, child);
 
   skeleton = generate_skeleton (child);
   if (skeleton == NULL)
@@ -7219,7 +7234,19 @@ 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.
+	 This leaves the original declaration in the tree, but
+	 it will be pruned later since there are no longer any
+	 references to it.  */
+      if (orig_parent != NULL)
+	{
+	  remove_child_with_prev (child, prev);
+	  add_child_die (orig_parent, skeleton);
+	}
+      else
+	replace_child (child, skeleton, prev);
     }
 
   return skeleton;
@@ -7264,11 +7291,9 @@ break_out_comdat_types (dw_die_ref die)
         generate_type_signature (c, type_node);
 
         /* Copy the declaration context, attributes, and children of the
-           declaration into the new compile unit DIE.  */
-	copy_declaration_context (unit, c);
-
-        /* Remove this DIE from the main CU.  */
-	replacement = remove_child_or_replace_with_skeleton (c, prev);
+           declaration into the new type unit DIE, then remove this DIE
+	   from the main CU (or replace it with a skeleton if necessary).  */
+	replacement = remove_child_or_replace_with_skeleton (unit, c, prev);
 
         /* 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..707f02d
--- /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)
+//	.ascii "thread\0"	# DW_AT_name
+//	.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   .LASF0	# DW_AT_name: "Executor"
+//			# DW_AT_declaration
+//	.uleb128 0x5	# (DIE (0x39) DW_TAG_subprogram)
+//			# DW_AT_external
+//	.long   .LASF1	# 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   .LASF2	# 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\]+(\[^\n\r\]*\[\n\r\]+)+\[^\n\r\]*\\(DIE \\(0x(\[0-9a-f\]+)\\) DW_TAG_class_type\\)\[\n\r\]+\[^\n\r\]*\"Executor\[^\n\r\]+\[\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\]*\"CurrentExecutor\[^\n\r\]+\[\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]+" } }