diff mbox

Fix problems with -fdebug-types-section

Message ID 20120829190024.0BAEEE0833@ccoutant.mtv.corp.google.com
State New
Headers show

Commit Message

Cary Coutant Aug. 29, 2012, 7 p.m. UTC 
I've combined these two pending patches into one:

   http://gcc.gnu.org/ml/gcc-patches/2012-08/msg00398.html
   http://gcc.gnu.org/ml/gcc-patches/2012-08/msg01968.html

The first patch fixed a problem with copying too much of a referenced
type into a type unit, by changing clone_tree_hash() to copy subprograms
as declarations.  In the second patch, I found that clone_tree_hash()
was still copying too much, and determined that it shouldn't be called
at all.

Notes from the first patch:

With --std=c++11, a template parameter can refer to a local type defined
within a function.  Because that local type doesn't qualify for its own
type unit, we copy it as an "unworthy" type into the type unit that refers
to it, but we copy too much, leading to a comdat type unit that contains a
DIE with subprogram definitions rather than declarations.  These DIEs may
have DW_AT_low_pc/high_pc or DW_AT_ranges attributes, and consequently can
refer to range list entries that don't get emitted because they're not
marked when the compile unit is scanned, eventually causing an undefined
symbol at link time.

In addition, while debugging this problem, I found that the
DW_AT_object_pointer attribute, when left in the skeletons that are left
behind in the compile unit, causes duplicate copies of the types to be
copied back into the compile unit.

This patch fixes these problems by removing the DW_AT_object_pointer
attribute from the skeleton left behind in the compile unit, and by
copying DW_TAG_subprogram DIEs as declarations when copying "unworthy"
types into a type unit.  In order to preserve information in the DIE
structure, I also added DW_AT_abstract_origin as an attribute that
should be copied when cloning a DIE as a declaration.

I also fixed the dwarf4-typedef.C test, which should be turning on
the -fdebug-types-section flag.

Notes from the second patch:

When a class template instantiation is moved into a separate type unit,
it can bring along a lot of other referenced types into the type unit,
especially if the template is derived from another (large) type that
does not have an actually have a type definition in a type unit of its
own. When there are many instantiations of the same template, we get
a lot of duplication, and in the worst case (a template with several
parameters, instantiated multiple times along each dimension), GCC
can end up taking a long time and exhausting available memory.

This combinatorial explosion is being caused by copy_decls_walk, where
it finds a type DIE that is referenced by the type unit, but is not
itself a type unit, and copies a declaration for that type into the
type unit in order to resolve the reference within the type unit.
In the process, copy_decls_walk also copies all of the children of
that DIE. In the case of a base class with member function templates,
every one of the instantiated member functions is copied into every
type unit that references the base class.

I don't believe that it's necessary to copy the children of the class
declaration at all, and this patch simply removes the code that copies
those children. If there's a reference in the type unit to one of the
children of that class, that one child will get copied in as needed.

Bootstraps and passes regression tests. Also tested with a large
internal test case that previously resulted in out-of-memory during
compilation.

OK for trunk?


2012-08-29   Cary Coutant  <ccoutant@google.com>

gcc/
	* dwarf2out.c (clone_as_declaration): Copy DW_AT_abstract_origin
	attribute.
	(generate_skeleton_bottom_up): Remove DW_AT_object_pointer attribute
	from original DIE.
	(clone_tree_hash): Remove.
	(copy_decls_walk): Don't copy children of a declaration into a
	type unit.

gcc/testsuite/
	* testsuite/g++.dg/debug/dwarf2/dwarf4-nested.C: New test case.
	* testsuite/g++.dg/debug/dwarf2/dwarf4-typedef.C: Add
	-fdebug-types-section flag.

Comments

Cary Coutant Sept. 1, 2012, 12:03 a.m. UTC | #1 
> I don't believe that it's necessary to copy the children of the class
> declaration at all, and this patch simply removes the code that copies
> those children. If there's a reference in the type unit to one of the
> children of that class, that one child will get copied in as needed.
>
> Bootstraps and passes regression tests. Also tested with a large
> internal test case that previously resulted in out-of-memory during
> compilation.

Sorry, but GDB testing exposed a problem with this patch. It turns out
that it IS necessary to copy children of a non-declaration DIE, such
as DW_TAG_array_type, so my patch was a little too aggressive in
completely removing that loop. I'll have a replacement patch on the
way after a bit more GDB testing.

-cary
diff mbox

Patch

Index: gcc/testsuite/g++.dg/debug/dwarf2/dwarf4-nested.C
===================================================================
--- gcc/testsuite/g++.dg/debug/dwarf2/dwarf4-nested.C	(revision 0)
+++ gcc/testsuite/g++.dg/debug/dwarf2/dwarf4-nested.C	(revision 0)
@@ -0,0 +1,55 @@ 
+// { dg-do compile }
+// { dg-options "--std=c++11 -dA -gdwarf-4 -fdebug-types-section -fno-merge-debug-strings" }
+
+// Check that -fdebug-types-sections does not copy a full referenced type
+// into a type unit.
+
+// Checks that at least one type unit is generated.
+//
+// { dg-final { scan-assembler "DIE \\(\[^\n\]*\\) DW_TAG_type_unit" } }
+//
+// Check that func is declared exactly twice in the debug info:
+// once in the type unit for struct D, and once in the compile unit.
+//
+// { dg-final { scan-assembler-times "\\.ascii \"func\\\\0\"\[^\n\]*DW_AT_name" 2 } }
+//
+// Check to make sure that no type unit contains a DIE with DW_AT_low_pc
+// or DW_AT_ranges.  These patterns assume that the compile unit is always
+// emitted after all type units.
+//
+// { dg-final { scan-assembler-not "\\.quad\[^\n\]*DW_AT_low_pc.*DIE \\(\[^\n\]*\\) DW_TAG_compile_unit" } }
+// { dg-final { scan-assembler-not "\\.quad\[^\n\]*DW_AT_ranges.*DIE \\(\[^\n\]*\\) DW_TAG_compile_unit" } }
+
+struct A {
+  A();
+  virtual ~A();
+  virtual void foo();
+ private:
+  int data;
+};
+
+struct B {
+  B();
+  virtual ~B();
+};
+
+extern B* table[];
+
+struct D {
+  template <typename T>
+  T* get(int i)
+  {
+    B*& cell = table[i];
+    if (cell == 0)
+      cell = new T();
+    return static_cast<T*>(cell);
+  }
+};
+
+void func(D* d)
+{
+  struct C : B {
+    A a;
+  };
+  d->get<C>(0)->a.foo();
+}
Index: gcc/testsuite/g++.dg/debug/dwarf2/dwarf4-typedef.C
===================================================================
--- gcc/testsuite/g++.dg/debug/dwarf2/dwarf4-typedef.C	(revision 190780)
+++ gcc/testsuite/g++.dg/debug/dwarf2/dwarf4-typedef.C	(working copy)
@@ -1,5 +1,5 @@ 
 /* { dg-do compile } */
-/* { dg-options "-gdwarf-4" } */
+/* { dg-options "-gdwarf-4 -fdebug-types-section" } */
 
 /* Regression test for an ICE in output_die when using -gdwarf-4.  */
 
Index: gcc/dwarf2out.c
===================================================================
--- gcc/dwarf2out.c	(revision 190780)
+++ gcc/dwarf2out.c	(working copy)
@@ -6383,6 +6383,7 @@  clone_as_declaration (dw_die_ref die)
 
       switch (a->dw_attr)
         {
+        case DW_AT_abstract_origin:
         case DW_AT_artificial:
         case DW_AT_containing_type:
         case DW_AT_external:
@@ -6515,6 +6516,12 @@  generate_skeleton_bottom_up (skeleton_ch
         dw_die_ref clone = clone_die (c);
         move_all_children (c, clone);
 
+        /* If the original has a DW_AT_object_pointer attribute,
+           it would now point to a child DIE just moved to the
+           cloned tree, so we need to remove that attribute from
+           the original.  */
+        remove_AT (c, DW_AT_object_pointer);
+
         replace_child (c, clone, prev);
         generate_skeleton_ancestor_tree (parent);
         add_child_die (parent->new_die, c);
@@ -6742,32 +6749,6 @@  copy_ancestor_tree (dw_die_ref unit, dw_
   return copy;
 }
 
-/* Like clone_tree, but additionally enter all the children into
-   the hash table decl_table.  */
-
-static dw_die_ref
-clone_tree_hash (dw_die_ref die, htab_t decl_table)
-{
-  dw_die_ref c;
-  dw_die_ref clone = clone_die (die);
-  struct decl_table_entry *entry;
-  void **slot = htab_find_slot_with_hash (decl_table, die,
-					  htab_hash_pointer (die), INSERT);
-  /* Assert that DIE isn't in the hash table yet.  If it would be there
-     before, the ancestors would be necessarily there as well, therefore
-     clone_tree_hash wouldn't be called.  */
-  gcc_assert (*slot == HTAB_EMPTY_ENTRY);
-  entry = XCNEW (struct decl_table_entry);
-  entry->orig = die;
-  entry->copy = clone;
-  *slot = entry;
-
-  FOR_EACH_CHILD (die, c,
-		  add_child_die (clone, clone_tree_hash (c, decl_table)));
-
-  return clone;
-}
-
 /* Walk the DIE and its children, looking for references to incomplete
    or trivial types that are unmarked (i.e., that are not in the current
    type_unit).  */
@@ -6814,10 +6795,6 @@  copy_decls_walk (dw_die_ref unit, dw_die
               entry->copy = copy;
               *slot = entry;
 
-	      FOR_EACH_CHILD (targ, c,
-			      add_child_die (copy,
-					     clone_tree_hash (c, decl_table)));
-
               /* Make sure the cloned tree is marked as part of the
                  type unit.  */
               mark_dies (copy);