[Dwarf Patch] Improve pubnames and pubtypes generation.

[saugustine, 2012-05-10 16:08:35]

The enclosed patch fixes many issues with pubnames and pubtypes. It generates
them for many more variables and with mostly correct and canonical dwarf names.

This patch should not affect any target that does not use pubnames.

The exceptions to the canonical names are addressed in a separate patch in
to the front end under review at
http://gcc.gnu.org/ml/gcc-patches/2012-05/msg00512.html.

Tested with bootstrap and running the test_pubnames_and_indices.py script
recently contributed to the GDB project.

OK for mainline?

Sterling

2012-05-10   Sterling Augustine  <saugustine@google.com>

	* dwarf2out.c (DEBUG_PUBNAMES_SECTION_LABEL,
	DEBUG_PUBTYPES_SECTION_LABEL): New macros.
	(debug_pubnames_section_label, debug_pubtypes_section_label): New
	globals.
	(is_cu_die, is_namespace_die, is_class_die, add_AT_pubnames,
	add_enumerator_pubname): New functions.
	(add_pubname): Rework logic.  Call is_class_die, is_cu_die and
	is_namespace_die.  Fix minor style violation.
	(add_pubtype): Rework logic for calculating type name.  Call
	is_namespace_die.
	(output_pubnames): Move conditional logic deciding when to produce the
	section from dwarf2out_finish.  Output debug_pubnames_section_label
	and debug_pubtypes_section_label.
	(base_type_die): Call add_pubtype.
	(gen_enumeration_type_die): Unconditionally call add_pubtype.
	(gen_namespace_die): Call add_pubname_string.
	(dwarf2out_init): Generate debug_pubnames_section_label and
	debug_pubtypes_section_label from DEBUG_PUBNAMES_SECTION_LABEL and
	DEBUG_PUBTYPES_SECTION_LABEL respectively.
	(dwarf2out_finish): Call add_AT_pubnames; Move logic on when to
	produce pubnames and pubtypes sections to output_pubnames.

Index: gcc/dwarf2out.c
===================================================================
--- gcc/dwarf2out.c	(revision 187271)
+++ gcc/dwarf2out.c	(working copy)
@@ -3007,6 +3007,7 @@ static void output_comp_unit (dw_die_ref, int);
 static void output_comdat_type_unit (comdat_type_node *);
 static const char *dwarf2_name (tree, int);
 static void add_pubname (tree, dw_die_ref);
+static void add_enumerator_pubname (const char *, dw_die_ref);
 static void add_pubname_string (const char *, dw_die_ref);
 static void add_pubtype (tree, dw_die_ref);
 static void output_pubnames (VEC (pubname_entry,gc) *);
@@ -3210,6 +3211,12 @@ static void gen_scheduled_generic_parms_dies (void
 #ifndef COLD_TEXT_SECTION_LABEL
 #define COLD_TEXT_SECTION_LABEL         "Ltext_cold"
 #endif
+#ifndef DEBUG_PUBNAMES_SECTION_LABEL
+#define DEBUG_PUBNAMES_SECTION_LABEL	    "Ldebug_pubnames"
+#endif
+#ifndef DEBUG_PUBTYPES_SECTION_LABEL
+#define DEBUG_PUBTYPES_SECTION_LABEL	    "Ldebug_pubtypes"
+#endif
 #ifndef DEBUG_LINE_SECTION_LABEL
 #define DEBUG_LINE_SECTION_LABEL	"Ldebug_line"
 #endif
@@ -3246,6 +3253,8 @@ static char cold_end_label[MAX_ARTIFICIAL_LABEL_BY
 static char abbrev_section_label[MAX_ARTIFICIAL_LABEL_BYTES];
 static char debug_info_section_label[MAX_ARTIFICIAL_LABEL_BYTES];
 static char debug_line_section_label[MAX_ARTIFICIAL_LABEL_BYTES];
+static char debug_pubnames_section_label[MAX_ARTIFICIAL_LABEL_BYTES];
+static char debug_pubtypes_section_label[MAX_ARTIFICIAL_LABEL_BYTES];
 static char macinfo_section_label[MAX_ARTIFICIAL_LABEL_BYTES];
 static char loc_section_label[MAX_ARTIFICIAL_LABEL_BYTES];
 static char ranges_section_label[2 * MAX_ARTIFICIAL_LABEL_BYTES];
@@ -5966,6 +5975,22 @@ is_cu_die (dw_die_ref c)
   return c && c->die_tag == DW_TAG_compile_unit;
 }
 
+/* Returns true iff C is a namespace DIE.  */
+
+static inline bool
+is_namespace_die (dw_die_ref c)
+{
+  return c && c->die_tag == DW_TAG_namespace;
+}
+
+/* Returns true iff C is a class DIE.  */
+
+static inline bool
+is_class_die (dw_die_ref c)
+{
+  return c && c->die_tag == DW_TAG_class_type;
+}
+
 static char *
 gen_internal_sym (const char *prefix)
 {
@@ -8033,6 +8058,20 @@ output_comp_unit (dw_die_ref die, int output_if_em
     }
 }
 
+/* Add the DW_AT_GNU_pubnames and DW_AT_GNU_pubtypes attributes.  */
+
+static void
+add_AT_pubnames (dw_die_ref die)
+{
+  if (targetm.want_debug_pub_sections)
+    {
+      /* FIXME: Should use add_AT_pubnamesptr.  This works because most
targets
+         don't care what the base section is.  */
+      add_AT_lineptr (die, DW_AT_GNU_pubnames, debug_pubnames_section_label);
+      add_AT_lineptr (die, DW_AT_GNU_pubtypes, debug_pubtypes_section_label);
+    }
+}
+
 /* Output a comdat type unit DIE and its children.  */
 
 static void
@@ -8116,14 +8155,32 @@ add_pubname_string (const char *str, dw_die_ref di
 static void
 add_pubname (tree decl, dw_die_ref die)
 {
-  if (targetm.want_debug_pub_sections && TREE_PUBLIC (decl))
+  if (!targetm.want_debug_pub_sections)
+    return;
+
+  if ((TREE_PUBLIC (decl) && !is_class_die (die->die_parent))
+      || is_cu_die (die->die_parent) || is_namespace_die (die->die_parent))
     {
       const char *name = dwarf2_name (decl, 1);
+
       if (name)
 	add_pubname_string (name, die);
     }
 }
 
+/* Add an enumerator to the pubnames section.  */
+
+static void
+add_enumerator_pubname (const char *scope_name, dw_die_ref die)
+{
+  pubname_entry e;
+
+  gcc_assert (scope_name);
+  e.name = concat (scope_name, get_AT_string (die, DW_AT_name), NULL);
+  e.die = die;
+  VEC_safe_push (pubname_entry, gc, pubtype_table, &e);
+}
+
 /* Add a new entry to .debug_pubtypes if appropriate.  */
 
 static void
@@ -8134,37 +8191,52 @@ add_pubtype (tree decl, dw_die_ref die)
   if (!targetm.want_debug_pub_sections)
     return;
 
-  e.name = NULL;
   if ((TREE_PUBLIC (decl)
-       || is_cu_die (die->die_parent))
+       || is_cu_die (die->die_parent) || is_namespace_die (die->die_parent))
       && (die->die_tag == DW_TAG_typedef || COMPLETE_TYPE_P (decl)))
     {
-      e.die = die;
-      if (TYPE_P (decl))
-	{
-	  if (TYPE_NAME (decl))
+      tree scope = NULL;
+      const char *scope_name = "";
+      const char *sep = is_cxx () ? "::" : ".";
+      const char *name;
+
+      scope = TYPE_P (decl) ? TYPE_CONTEXT (decl) : NULL;
+      if (scope && TREE_CODE (scope) == NAMESPACE_DECL)
 	    {
-	      if (TREE_CODE (TYPE_NAME (decl)) == IDENTIFIER_NODE)
-		e.name = IDENTIFIER_POINTER (TYPE_NAME (decl));
-	      else if (TREE_CODE (TYPE_NAME (decl)) == TYPE_DECL
-		       && DECL_NAME (TYPE_NAME (decl)))
-		e.name = IDENTIFIER_POINTER (DECL_NAME (TYPE_NAME (decl)));
+          scope_name = lang_hooks.dwarf_name (scope, 1);
+          if (scope_name != NULL && scope_name[0] != '\0')
+            scope_name = concat (scope_name, sep, NULL);
 	      else
-	       e.name = xstrdup ((const char *) get_AT_string (die, DW_AT_name));
-	    }
+            scope_name = "";
 	}
+
+      if (TYPE_P (decl))
+        name = type_tag (decl);
       else
-	{
-	  e.name = dwarf2_name (decl, 1);
-	  if (e.name)
-	    e.name = xstrdup (e.name);
-	}
+        name = lang_hooks.dwarf_name (decl, 1);
 
       /* If we don't have a name for the type, there's no point in adding
 	 it to the table.  */
-      if (e.name && e.name[0] != '\0')
+      if (name != NULL && name[0] != '\0')
+        {
+          e.die = die;
+          e.name = concat (scope_name, name, NULL);
 	VEC_safe_push (pubname_entry, gc, pubtype_table, &e);
     }
+
+      /* Although it might be more consistent to add the pubinfo for the
+         enumerators as their dies are created, they should only be added if
the
+         enum type meets the criteria above.  So rather than re-check the
parent
+         enum type whenever an enumerator die is created, just output them all
+         here.  This isn't protected by the name conditional because anoymous
+         enums don't have names.  */
+      if (die->die_tag == DW_TAG_enumeration_type)
+        {
+          dw_die_ref c;
+
+          FOR_EACH_CHILD (die, c, add_enumerator_pubname (scope_name, c));
+        }
+    }
 }
 
 /* Output the public names table used to speed up access to externally
@@ -8177,6 +8249,18 @@ output_pubnames (VEC (pubname_entry, gc) * names)
   unsigned long pubnames_length = size_of_pubnames (names);
   pubname_ref pub;
 
+  if (!targetm.want_debug_pub_sections || !info_section_emitted)
+    return;
+  if (names == pubname_table)
+    {
+      switch_to_section (debug_pubnames_section);
+      ASM_OUTPUT_LABEL (asm_out_file, debug_pubnames_section_label);
+    }
+  else
+    {
+      switch_to_section (debug_pubtypes_section);
+      ASM_OUTPUT_LABEL (asm_out_file, debug_pubtypes_section_label);
+    }
   if (DWARF_INITIAL_LENGTH_SIZE - DWARF_OFFSET_SIZE == 4)
     dw2_asm_output_data (4, 0xffffffff,
       "Initial length escape value indicating 64-bit DWARF extension");
@@ -9090,6 +9174,7 @@ base_type_die (tree type)
   add_AT_unsigned (base_type_result, DW_AT_byte_size,
 		   int_size_in_bytes (type));
   add_AT_unsigned (base_type_result, DW_AT_encoding, encoding);
+  add_pubtype (type, base_type_result);
 
   return base_type_result;
 }
@@ -16176,7 +16261,6 @@ gen_enumeration_type_die (tree type, dw_die_ref co
   else
     add_AT_flag (type_die, DW_AT_declaration, 1);
 
-  if (get_AT (type_die, DW_AT_name))
     add_pubtype (type, type_die);
 
   return type_die;
@@ -18923,6 +19007,8 @@ gen_namespace_die (tree decl, dw_die_ref context_d
       add_AT_die_ref (namespace_die, DW_AT_import, origin_die);
       equate_decl_number_to_die (decl, namespace_die);
     }
+  /* Bypass dwarf2_name's check for DECL_NAMELESS.  */
+  add_pubname_string (lang_hooks.dwarf_name (decl, 1), namespace_die);
 }
 
 /* Generate Dwarf debug information for a decl described by DECL.
@@ -20585,6 +20671,10 @@ dwarf2out_init (const char *filename ATTRIBUTE_UNU
 
   ASM_GENERATE_INTERNAL_LABEL (debug_info_section_label,
 			       DEBUG_INFO_SECTION_LABEL, 0);
+  ASM_GENERATE_INTERNAL_LABEL (debug_pubnames_section_label,
+			       DEBUG_PUBNAMES_SECTION_LABEL, 0);
+  ASM_GENERATE_INTERNAL_LABEL (debug_pubtypes_section_label,
+			       DEBUG_PUBTYPES_SECTION_LABEL, 0);
   ASM_GENERATE_INTERNAL_LABEL (debug_line_section_label,
 			       DEBUG_LINE_SECTION_LABEL, 0);
   ASM_GENERATE_INTERNAL_LABEL (ranges_section_label,
@@ -22183,6 +22273,8 @@ dwarf2out_finish (const char *filename)
     }
   htab_delete (comdat_type_table);
 
+  add_AT_pubnames (comp_unit_die ());
+
   /* Output the main compilation unit if non-empty or if .debug_macinfo
      will be emitted.  */
   output_comp_unit (comp_unit_die (), debug_info_level >=
DINFO_LEVEL_VERBOSE);
@@ -22206,42 +22298,12 @@ dwarf2out_finish (const char *filename)
       output_location_lists (comp_unit_die ());
     }
 
-  /* Output public names table if necessary.  */
-  if (!VEC_empty (pubname_entry, pubname_table))
-    {
-      gcc_assert (info_section_emitted);
-      switch_to_section (debug_pubnames_section);
-      output_pubnames (pubname_table);
-    }
-
-  /* Output public types table if necessary.  */
+  /* Output public names and types tables if necessary.  */
+  output_pubnames (pubname_table);
   /* ??? Only defined by DWARF3, but emitted by Darwin for DWARF2.
      It shouldn't hurt to emit it always, since pure DWARF2 consumers
      simply won't look for the section.  */
-  if (!VEC_empty (pubname_entry, pubtype_table))
-    {
-      bool empty = false;
-      
-      if (flag_eliminate_unused_debug_types)
-	{
-	  /* The pubtypes table might be emptied by pruning unused items.  */
-	  unsigned i;
-	  pubname_ref p;
-	  empty = true;
-	  FOR_EACH_VEC_ELT (pubname_entry, pubtype_table, i, p)
-	    if (p->die->die_offset != 0)
-	      {
-		empty = false;
-		break;
-	      }
-	}
-      if (!empty)
-	{
-	  gcc_assert (info_section_emitted);
-	  switch_to_section (debug_pubtypes_section);
-	  output_pubnames (pubtype_table);
-	}
-    }
+  output_pubnames (pubtype_table);
 
   /* Output the address range information if a CU (.debug_info section)
      was emitted.  We output an empty table even if we had no functions

--
This patch is available for review at http://codereview.appspot.com/6197069

[saugustine, 2012-05-17 15:49:50]

On Thu, May 10, 2012 at 9:08 AM, Sterling Augustine
<saugustine@google.com> wrote:
> The enclosed patch fixes many issues with pubnames and pubtypes. It generates
> them for many more variables and with mostly correct and canonical dwarf
names.
>
> This patch should not affect any target that does not use pubnames.
>
> The exceptions to the canonical names are addressed in a separate patch in
> to the front end under review at
http://gcc.gnu.org/ml/gcc-patches/2012-05/msg00512.html.
>
> Tested with bootstrap and running the test_pubnames_and_indices.py script
> recently contributed to the GDB project.
>
> OK for mainline?
>
> Sterling
>
> 2012-05-10   Sterling Augustine  <saugustine@google.com>
>
>        * dwarf2out.c (DEBUG_PUBNAMES_SECTION_LABEL,
>        DEBUG_PUBTYPES_SECTION_LABEL): New macros.
>        (debug_pubnames_section_label, debug_pubtypes_section_label): New
>        globals.
>        (is_cu_die, is_namespace_die, is_class_die, add_AT_pubnames,
>        add_enumerator_pubname): New functions.
>        (add_pubname): Rework logic.  Call is_class_die, is_cu_die and
>        is_namespace_die.  Fix minor style violation.
>        (add_pubtype): Rework logic for calculating type name.  Call
>        is_namespace_die.
>        (output_pubnames): Move conditional logic deciding when to produce the
>        section from dwarf2out_finish.  Output debug_pubnames_section_label
>        and debug_pubtypes_section_label.
>        (base_type_die): Call add_pubtype.
>        (gen_enumeration_type_die): Unconditionally call add_pubtype.
>        (gen_namespace_die): Call add_pubname_string.
>        (dwarf2out_init): Generate debug_pubnames_section_label and
>        debug_pubtypes_section_label from DEBUG_PUBNAMES_SECTION_LABEL and
>        DEBUG_PUBTYPES_SECTION_LABEL respectively.
>        (dwarf2out_finish): Call add_AT_pubnames; Move logic on when to
>        produce pubnames and pubtypes sections to output_pubnames.
>
> Index: gcc/dwarf2out.c
> ===================================================================
> --- gcc/dwarf2out.c     (revision 187271)
> +++ gcc/dwarf2out.c     (working copy)
> @@ -3007,6 +3007,7 @@ static void output_comp_unit (dw_die_ref, int);
>  static void output_comdat_type_unit (comdat_type_node *);
>  static const char *dwarf2_name (tree, int);
>  static void add_pubname (tree, dw_die_ref);
> +static void add_enumerator_pubname (const char *, dw_die_ref);
>  static void add_pubname_string (const char *, dw_die_ref);
>  static void add_pubtype (tree, dw_die_ref);
>  static void output_pubnames (VEC (pubname_entry,gc) *);
> @@ -3210,6 +3211,12 @@ static void gen_scheduled_generic_parms_dies (void
>  #ifndef COLD_TEXT_SECTION_LABEL
>  #define COLD_TEXT_SECTION_LABEL         "Ltext_cold"
>  #endif
> +#ifndef DEBUG_PUBNAMES_SECTION_LABEL
> +#define DEBUG_PUBNAMES_SECTION_LABEL       "Ldebug_pubnames"
> +#endif
> +#ifndef DEBUG_PUBTYPES_SECTION_LABEL
> +#define DEBUG_PUBTYPES_SECTION_LABEL       "Ldebug_pubtypes"
> +#endif
>  #ifndef DEBUG_LINE_SECTION_LABEL
>  #define DEBUG_LINE_SECTION_LABEL       "Ldebug_line"
>  #endif
> @@ -3246,6 +3253,8 @@ static char cold_end_label[MAX_ARTIFICIAL_LABEL_BY
>  static char abbrev_section_label[MAX_ARTIFICIAL_LABEL_BYTES];
>  static char debug_info_section_label[MAX_ARTIFICIAL_LABEL_BYTES];
>  static char debug_line_section_label[MAX_ARTIFICIAL_LABEL_BYTES];
> +static char debug_pubnames_section_label[MAX_ARTIFICIAL_LABEL_BYTES];
> +static char debug_pubtypes_section_label[MAX_ARTIFICIAL_LABEL_BYTES];
>  static char macinfo_section_label[MAX_ARTIFICIAL_LABEL_BYTES];
>  static char loc_section_label[MAX_ARTIFICIAL_LABEL_BYTES];
>  static char ranges_section_label[2 * MAX_ARTIFICIAL_LABEL_BYTES];
> @@ -5966,6 +5975,22 @@ is_cu_die (dw_die_ref c)
>   return c && c->die_tag == DW_TAG_compile_unit;
>  }
>
> +/* Returns true iff C is a namespace DIE.  */
> +
> +static inline bool
> +is_namespace_die (dw_die_ref c)
> +{
> +  return c && c->die_tag == DW_TAG_namespace;
> +}
> +
> +/* Returns true iff C is a class DIE.  */
> +
> +static inline bool
> +is_class_die (dw_die_ref c)
> +{
> +  return c && c->die_tag == DW_TAG_class_type;
> +}
> +
>  static char *
>  gen_internal_sym (const char *prefix)
>  {
> @@ -8033,6 +8058,20 @@ output_comp_unit (dw_die_ref die, int output_if_em
>     }
>  }
>
> +/* Add the DW_AT_GNU_pubnames and DW_AT_GNU_pubtypes attributes.  */
> +
> +static void
> +add_AT_pubnames (dw_die_ref die)
> +{
> +  if (targetm.want_debug_pub_sections)
> +    {
> +      /* FIXME: Should use add_AT_pubnamesptr.  This works because most
targets
> +         don't care what the base section is.  */
> +      add_AT_lineptr (die, DW_AT_GNU_pubnames,
debug_pubnames_section_label);
> +      add_AT_lineptr (die, DW_AT_GNU_pubtypes,
debug_pubtypes_section_label);
> +    }
> +}
> +
>  /* Output a comdat type unit DIE and its children.  */
>
>  static void
> @@ -8116,14 +8155,32 @@ add_pubname_string (const char *str, dw_die_ref di
>  static void
>  add_pubname (tree decl, dw_die_ref die)
>  {
> -  if (targetm.want_debug_pub_sections && TREE_PUBLIC (decl))
> +  if (!targetm.want_debug_pub_sections)
> +    return;
> +
> +  if ((TREE_PUBLIC (decl) && !is_class_die (die->die_parent))
> +      || is_cu_die (die->die_parent) || is_namespace_die (die->die_parent))
>     {
>       const char *name = dwarf2_name (decl, 1);
> +
>       if (name)
>        add_pubname_string (name, die);
>     }
>  }
>
> +/* Add an enumerator to the pubnames section.  */
> +
> +static void
> +add_enumerator_pubname (const char *scope_name, dw_die_ref die)
> +{
> +  pubname_entry e;
> +
> +  gcc_assert (scope_name);
> +  e.name = concat (scope_name, get_AT_string (die, DW_AT_name), NULL);
> +  e.die = die;
> +  VEC_safe_push (pubname_entry, gc, pubtype_table, &e);
> +}
> +
>  /* Add a new entry to .debug_pubtypes if appropriate.  */
>
>  static void
> @@ -8134,37 +8191,52 @@ add_pubtype (tree decl, dw_die_ref die)
>   if (!targetm.want_debug_pub_sections)
>     return;
>
> -  e.name = NULL;
>   if ((TREE_PUBLIC (decl)
> -       || is_cu_die (die->die_parent))
> +       || is_cu_die (die->die_parent) || is_namespace_die (die->die_parent))
>       && (die->die_tag == DW_TAG_typedef || COMPLETE_TYPE_P (decl)))
>     {
> -      e.die = die;
> -      if (TYPE_P (decl))
> -       {
> -         if (TYPE_NAME (decl))
> +      tree scope = NULL;
> +      const char *scope_name = "";
> +      const char *sep = is_cxx () ? "::" : ".";
> +      const char *name;
> +
> +      scope = TYPE_P (decl) ? TYPE_CONTEXT (decl) : NULL;
> +      if (scope && TREE_CODE (scope) == NAMESPACE_DECL)
>            {
> -             if (TREE_CODE (TYPE_NAME (decl)) == IDENTIFIER_NODE)
> -               e.name = IDENTIFIER_POINTER (TYPE_NAME (decl));
> -             else if (TREE_CODE (TYPE_NAME (decl)) == TYPE_DECL
> -                      && DECL_NAME (TYPE_NAME (decl)))
> -               e.name = IDENTIFIER_POINTER (DECL_NAME (TYPE_NAME (decl)));
> +          scope_name = lang_hooks.dwarf_name (scope, 1);
> +          if (scope_name != NULL && scope_name[0] != '\0')
> +            scope_name = concat (scope_name, sep, NULL);
>              else
> -              e.name = xstrdup ((const char *) get_AT_string (die,
DW_AT_name));
> -           }
> +            scope_name = "";
>        }
> +
> +      if (TYPE_P (decl))
> +        name = type_tag (decl);
>       else
> -       {
> -         e.name = dwarf2_name (decl, 1);
> -         if (e.name)
> -           e.name = xstrdup (e.name);
> -       }
> +        name = lang_hooks.dwarf_name (decl, 1);
>
>       /* If we don't have a name for the type, there's no point in adding
>         it to the table.  */
> -      if (e.name && e.name[0] != '\0')
> +      if (name != NULL && name[0] != '\0')
> +        {
> +          e.die = die;
> +          e.name = concat (scope_name, name, NULL);
>        VEC_safe_push (pubname_entry, gc, pubtype_table, &e);
>     }
> +
> +      /* Although it might be more consistent to add the pubinfo for the
> +         enumerators as their dies are created, they should only be added if
the
> +         enum type meets the criteria above.  So rather than re-check the
parent
> +         enum type whenever an enumerator die is created, just output them
all
> +         here.  This isn't protected by the name conditional because
anoymous
> +         enums don't have names.  */
> +      if (die->die_tag == DW_TAG_enumeration_type)
> +        {
> +          dw_die_ref c;
> +
> +          FOR_EACH_CHILD (die, c, add_enumerator_pubname (scope_name, c));
> +        }
> +    }
>  }
>
>  /* Output the public names table used to speed up access to externally
> @@ -8177,6 +8249,18 @@ output_pubnames (VEC (pubname_entry, gc) * names)
>   unsigned long pubnames_length = size_of_pubnames (names);
>   pubname_ref pub;
>
> +  if (!targetm.want_debug_pub_sections || !info_section_emitted)
> +    return;
> +  if (names == pubname_table)
> +    {
> +      switch_to_section (debug_pubnames_section);
> +      ASM_OUTPUT_LABEL (asm_out_file, debug_pubnames_section_label);
> +    }
> +  else
> +    {
> +      switch_to_section (debug_pubtypes_section);
> +      ASM_OUTPUT_LABEL (asm_out_file, debug_pubtypes_section_label);
> +    }
>   if (DWARF_INITIAL_LENGTH_SIZE - DWARF_OFFSET_SIZE == 4)
>     dw2_asm_output_data (4, 0xffffffff,
>       "Initial length escape value indicating 64-bit DWARF extension");
> @@ -9090,6 +9174,7 @@ base_type_die (tree type)
>   add_AT_unsigned (base_type_result, DW_AT_byte_size,
>                   int_size_in_bytes (type));
>   add_AT_unsigned (base_type_result, DW_AT_encoding, encoding);
> +  add_pubtype (type, base_type_result);
>
>   return base_type_result;
>  }
> @@ -16176,7 +16261,6 @@ gen_enumeration_type_die (tree type, dw_die_ref co
>   else
>     add_AT_flag (type_die, DW_AT_declaration, 1);
>
> -  if (get_AT (type_die, DW_AT_name))
>     add_pubtype (type, type_die);
>
>   return type_die;
> @@ -18923,6 +19007,8 @@ gen_namespace_die (tree decl, dw_die_ref context_d
>       add_AT_die_ref (namespace_die, DW_AT_import, origin_die);
>       equate_decl_number_to_die (decl, namespace_die);
>     }
> +  /* Bypass dwarf2_name's check for DECL_NAMELESS.  */
> +  add_pubname_string (lang_hooks.dwarf_name (decl, 1), namespace_die);
>  }
>
>  /* Generate Dwarf debug information for a decl described by DECL.
> @@ -20585,6 +20671,10 @@ dwarf2out_init (const char *filename ATTRIBUTE_UNU
>
>   ASM_GENERATE_INTERNAL_LABEL (debug_info_section_label,
>                               DEBUG_INFO_SECTION_LABEL, 0);
> +  ASM_GENERATE_INTERNAL_LABEL (debug_pubnames_section_label,
> +                              DEBUG_PUBNAMES_SECTION_LABEL, 0);
> +  ASM_GENERATE_INTERNAL_LABEL (debug_pubtypes_section_label,
> +                              DEBUG_PUBTYPES_SECTION_LABEL, 0);
>   ASM_GENERATE_INTERNAL_LABEL (debug_line_section_label,
>                               DEBUG_LINE_SECTION_LABEL, 0);
>   ASM_GENERATE_INTERNAL_LABEL (ranges_section_label,
> @@ -22183,6 +22273,8 @@ dwarf2out_finish (const char *filename)
>     }
>   htab_delete (comdat_type_table);
>
> +  add_AT_pubnames (comp_unit_die ());
> +
>   /* Output the main compilation unit if non-empty or if .debug_macinfo
>      will be emitted.  */
>   output_comp_unit (comp_unit_die (), debug_info_level >=
DINFO_LEVEL_VERBOSE);
> @@ -22206,42 +22298,12 @@ dwarf2out_finish (const char *filename)
>       output_location_lists (comp_unit_die ());
>     }
>
> -  /* Output public names table if necessary.  */
> -  if (!VEC_empty (pubname_entry, pubname_table))
> -    {
> -      gcc_assert (info_section_emitted);
> -      switch_to_section (debug_pubnames_section);
> -      output_pubnames (pubname_table);
> -    }
> -
> -  /* Output public types table if necessary.  */
> +  /* Output public names and types tables if necessary.  */
> +  output_pubnames (pubname_table);
>   /* ??? Only defined by DWARF3, but emitted by Darwin for DWARF2.
>      It shouldn't hurt to emit it always, since pure DWARF2 consumers
>      simply won't look for the section.  */
> -  if (!VEC_empty (pubname_entry, pubtype_table))
> -    {
> -      bool empty = false;
> -
> -      if (flag_eliminate_unused_debug_types)
> -       {
> -         /* The pubtypes table might be emptied by pruning unused items.  */
> -         unsigned i;
> -         pubname_ref p;
> -         empty = true;
> -         FOR_EACH_VEC_ELT (pubname_entry, pubtype_table, i, p)
> -           if (p->die->die_offset != 0)
> -             {
> -               empty = false;
> -               break;
> -             }
> -       }
> -      if (!empty)
> -       {
> -         gcc_assert (info_section_emitted);
> -         switch_to_section (debug_pubtypes_section);
> -         output_pubnames (pubtype_table);
> -       }
> -    }
> +  output_pubnames (pubtype_table);
>
>   /* Output the address range information if a CU (.debug_info section)
>      was emitted.  We output an empty table even if we had no functions
>
> --
> This patch is available for review at http://codereview.appspot.com/6197069

Ping? Also, I have a couple more for debug fission that apply over
this one coming.

[Jason Merrill, 2012-05-18 22:39:18]

This patch makes a lot of changes to the behavior of .debug_pubnames that I
haven't seen any discussion of, and that don't seem obvious to me.  Can you
point me at discussion threads?

http://codereview.appspot.com/6197069/diff/1/gcc/dwarf2out.c
File gcc/dwarf2out.c (left):

http://codereview.appspot.com/6197069/diff/1/gcc/dwarf2out.c#oldcode22231
gcc/dwarf2out.c:22231: FOR_EACH_VEC_ELT (pubname_entry, pubtype_table, i, p)
You don't seem to have added anything to output_pubnames to avoid emitting
entries for pruned types.

http://codereview.appspot.com/6197069/diff/1/gcc/dwarf2out.c
File gcc/dwarf2out.c (right):

http://codereview.appspot.com/6197069/diff/1/gcc/dwarf2out.c#newcode8070
gcc/dwarf2out.c:8070: add_AT_lineptr (die, DW_AT_GNU_pubnames,
debug_pubnames_section_label);
What are these attributes for?  Is there a proposal to add them?  pubnames/types
are used for lookup from a name to a DIE, so there seems to be no reason to have
a pointer the other way.

http://codereview.appspot.com/6197069/diff/1/gcc/dwarf2out.c#newcode8162
gcc/dwarf2out.c:8162: || is_cu_die (die->die_parent) || is_namespace_die
(die->die_parent))
The DWARF standard says that pubnames is for names with global scope; this
change would add namespace-scope statics as well.

http://codereview.appspot.com/6197069/diff/1/gcc/dwarf2out.c#newcode9177
gcc/dwarf2out.c:9177: add_pubtype (type, base_type_result);
Why do we need pubtype entries for base types?

http://codereview.appspot.com/6197069/diff/1/gcc/dwarf2out.c#newcode19010
gcc/dwarf2out.c:19010: /* Bypass dwarf2_name's check for DECL_NAMELESS.  */
Why bypass the DECL_NAMELESS check?

Can you point me at discussion about adding enumerators and namespaces to
pubnames?  Historically it has just been for things with addresses (the standard
says "objects and functions").

[saugustine, 2012-05-18 23:34:28]

Hi Jasaon,

Thanks so much for reviewing this patch. I realize it is a lot to see.

The motivation and new dwarf attributes and tags all stem from the debug fission
project as described at http://gcc.gnu.org/wiki/DebugFission. I have several
more patches dealing with fission coming.

Fission has been discussed in the Dwarf standardization meetings.

Thanks again, I am happy to make the changes you deem necessary.

http://codereview.appspot.com/6197069/diff/1/gcc/dwarf2out.c
File gcc/dwarf2out.c (left):

http://codereview.appspot.com/6197069/diff/1/gcc/dwarf2out.c#oldcode22231
gcc/dwarf2out.c:22231: FOR_EACH_VEC_ELT (pubname_entry, pubtype_table, i, p)
On 2012/05/18 22:39:18, Jason Merrill wrote:
> You don't seem to have added anything to output_pubnames to avoid emitting
> entries for pruned types.

The code I removed doesn't deal with omitting individual entries. Instead, it
decides whether to emit the pubtypes section at all--if it is empty, then don't
emit it. Pruned entries are handled as they always were in output_pubnames.

What this code did was to check if the pubtypes section was emptied via pruning.
If it was, then don't emit it.

However, now that there is the DW_AT_GNU_pubtypes attribute that points to the
section, we need to emit the label no matter what. The presence of this
attribute helps the consumer know the difference between "No pubtypes were
present in the source" and "The compiler didn't generate pubtypes."

http://codereview.appspot.com/6197069/diff/1/gcc/dwarf2out.c
File gcc/dwarf2out.c (right):

http://codereview.appspot.com/6197069/diff/1/gcc/dwarf2out.c#newcode8070
gcc/dwarf2out.c:8070: add_AT_lineptr (die, DW_AT_GNU_pubnames,
debug_pubnames_section_label);
On 2012/05/18 22:39:18, Jason Merrill wrote:
> What are these attributes for?  Is there a proposal to add them? 
pubnames/types
> are used for lookup from a name to a DIE, so there seems to be no reason to
have
> a pointer the other way.

The entire motivation for this patch, including the proposed new attributes is
at:

http://gcc.gnu.org/wiki/DebugFission

In particular, the section titled, "Building a GDB Index". (Unfortunately, there
isn't an anchor to that section easily linkable.) This was discussed at the past
couple of dwarf standardization meetings.

http://codereview.appspot.com/6197069/diff/1/gcc/dwarf2out.c#newcode8162
gcc/dwarf2out.c:8162: || is_cu_die (die->die_parent) || is_namespace_die
(die->die_parent))
On 2012/05/18 22:39:18, Jason Merrill wrote:
> The DWARF standard says that pubnames is for names with global scope; this
> change would add namespace-scope statics as well.

I am matching what gold and gdb do when building the gdb index. I suppose Cary
will need to add this subtle change to the proposal, and I can also guard it
with "dwarf_split_debug_info".

http://codereview.appspot.com/6197069/diff/1/gcc/dwarf2out.c#newcode9177
gcc/dwarf2out.c:9177: add_pubtype (type, base_type_result);
On 2012/05/18 22:39:18, Jason Merrill wrote:
> Why do we need pubtype entries for base types?

Same as above--to make these addable to the index, which in turn makes gdb
faster. I'll add this to the note to Cary.

http://codereview.appspot.com/6197069/diff/1/gcc/dwarf2out.c#newcode19010
gcc/dwarf2out.c:19010: /* Bypass dwarf2_name's check for DECL_NAMELESS.  */
On 2012/05/18 22:39:18, Jason Merrill wrote:
> Why bypass the DECL_NAMELESS check?

So objects within anonymous namespaces get pubnames:

namespace { void some_function...

> 
> Can you point me at discussion about adding enumerators and namespaces to
> pubnames?  Historically it has just been for things with addresses (the
standard
> says "objects and functions").

This is all about making every name gdb might need public. Yet another note to
add to the proposal, I suppose. I will see to it that Cary does.

[Cary, 2012-05-21 18:39:51]

>> The entire motivation for this patch, including the proposed new
>> attributes is at:
>>
>> http://gcc.gnu.org/wiki/DebugFission
>>
>> In particular, the section titled, "Building a GDB Index".
>
> OK, I've read that section and I still don't understand why the consumer
> would need a pointer from the CU to the pubnames section to build an index.
>  If we're looking for something by name, we go from pubnames to the CU.  If
> we're looking at the CU because we're in a function body, presumably we need
> to parse most of it anyway; is there really a significant benefit to having
> a separate "what names does this CU define" attribute?  The index is all
> name->die lookup, isn't it?

The DW_AT_GNU_pubnames/pubtypes attributes serve two purposes: (1)
they let the linker know which CUs have pubnames & pubtypes sections,
and (2) they let us know that the pubnames and pubtypes are "reliable"
(relative to the incomplete sections that GCC has generated in the
past). When building the .gdb_index, the linker starts with the top
compile_unit DIE of the CU. If there are pubnames/pubtypes attributes,
it can go read those sections; otherwise, it needs to parse the whole
DIE tree (slow) to extract the information needed for .gdb_index. It
would be possible to use the pointer from the pubnames section to the
CU, but that's a reverse pointer, and it would take time to set up the
links to figure out which pubnames/pubtypes sections go with which
CUs.


> I'm also puzzled by what the proposal says about .debug_types.  Why would a
> pubtypes entry point to a CU that doesn't actually have a definition of the
> type?  Is it so that the consumer knows which .dwo to look in?  Perhaps we
> could do something else with the sig8 instead of pointing to the wrong unit.

Yes, it's to find the .dwo. I never did anything to accomodate
.debug_types sections with .debug_pubtypes -- probably a more
substantial change to the format is in order, but I wanted to save
that for what I expect to be a complete revamping of the accelerated
lookup tables in DWARF.


>>> The DWARF standard says that pubnames is for names with global scope;
>>> this
>>> change would add namespace-scope statics as well.
>>
>> I am matching what gold and gdb do when building the gdb index.
>
> Ah.  If that's what GDB wants I guess it makes sense, but I'd like to see
> reaction from the committee to this particular aspect.  I haven't noticed it
> come up in the committee discussion of Fission.

We had some discussion in the DWARF workgroup during the DWARF 4
development about what should go into pubnames and pubtypes, but
didn't really settle on any specific wording change for the standard.
I think it was generally agreed that it's ultimately a
quality-of-implementation issue where the producer and consumer need
to agree on what's useful there. For the purposes of a debugger, the
only thing that really make sense is to have a name-to-CU index that
indexes all the names that a user might type at the user interface.
Without the fixes we're putting in here, the pubnames section has been
essentially useless, and has been ignored entirely by gdb -- to the
point that a change went in a year or so ago to disable the section by
default except for Darwin.

I anticipate further discussion of accelerated lookup in the DWARF 5
discussions, and I expect these tables to get a major overhaul. Until
then, I think it's reasonable to make use of them as best we can
without making any major structural changes.


>>> Why do we need pubtype entries for base types?
>>
>> Same as above--to make these addable to the index, which in turn makes
>> gdb faster. I'll add this to the note to Cary.
>
> Really?  GDB wants to look up built-in types in the index rather than just
> knowing what "int" means?

It doesn't actually always mean the same thing, though. It might be
less surprising if you ask that about some other base type like
"double" (and consider that GCC has options like -fshort-double).
Also, GDB can narrow the set of CUs that it has to read sometimes by
knowing which CUs actually contain a declaration of a given base type
(again, it's easier to see the advantage for "double" than for "int").


>>> Why bypass the DECL_NAMELESS check?
>>
>> So objects within anonymous namespaces get pubnames:
>>
>> namespace { void some_function...
>
> Hmm, it would give the anonymous namespace itself a pubname, but I don't see
> how it would make any difference to objects within the namespace.

The items within an anonymous namespace need to be indexed so that a
GDB user can just type "some_function" instead of "(anonymous
namespace)::some_function".

-cary

[saugustine, 2012-05-22 17:27:27]

On Tue, May 22, 2012 at 10:19 AM, Jason Merrill <jason@redhat.com> wrote:

I'll let Cary handle the other questions.

> Yes, I agree that we want to put "some_function" in pubnames.  I still don't
> see how putting the anonymous namespace itself in pubnames helps at all.  As
> far as pubnames is concerned, the anonymous namespace should be transparent,
> and "some_function" should appear unqualified.

Anonymous namespaces are funky in general, but gdb would like to be
able to do things like tab completion on:

(gdb) b '(anonymous namespace)::
(gdb) b 'foo::(anonymous namespace)::

Without being able to lookup anonymous namespaces, gdb has no idea
where to continue from.

This works today without pubnames because gdb goes and constructs a
big list that includes anonymous namespaces by hand. Without anonymous
namespaces in the index, gdb would either lose that functionality, or
would have to parse the dwarf to find them. If it does that, then the
index isn't very helpful.

[Cary, 2012-05-22 17:49:01]

> I would expect the linker to start by processing the pubnames/pubtypes
> sections, and then if it wants to look through the CUs as well it already
> knows which ones it can skip.
>
> The presence or absence of an attribute seems like a fragile way to
> determine whether or not particular debug info is sufficiently reliable.  If
> a consumer wants to make decisions based on changing behavior in different
> versions of a producer, it could look at the DW_AT_producer string.

In the context of Tom's original suggestion to add .gdb_index, I
suggested fixing the problems with .debug_pubnames and using the
producer string to distinguish. Tom had several objections to the use
of the producer string for that:

   http://sourceware.org/ml/archer/2009-q4/msg00065.html

I'm simply trying to do something that's fairly simple and that works.
These attributes are GNU extensions, and I do not intend to propose
these as part of the DWARF 5 standard -- they're stopgap measures
until we have a better plan for accelerated lookup tables.

Starting with the pubnames sections then going back and reading the
debug_info sections would require an extra table and a lookup for each
CU to see if it's already been covered by a pubnames section. Building
the .gdb_index in gold is still too slow, but these attributes help
make it a bit faster than it was without them.

Are these two attributes a showstopper issue for you?


> Hmm.  Looking at the code, it seems that the pubtypes entries for these
> types will point to the main CU, but with the offset in the type_unit, so a
> consumer trying to follow the pointer will find garbage.  If you want to
> have a pubtypes entry, it needs to point to a type skeleton to avoid
> crashing standard-conforming consumers.

Yes, I understand that's broken, but there are no consumers at this
point that make any use of that offset. Would it be acceptable if we
just put 0 there? (Given that I expect .debug_pub* to go away soon, I
don't think it's worth the trouble of filling in the offset with
anything more meaningful.)

-cary

[Cary, 2012-05-22 20:04:16]

> Yes, but I would expect this table lookup to be faster than going to the
> disk to read the CU DIE and abbrev in order to check for the attribute.
>  OTOH, I suppose you need to read it anyway if you want to check somehow
> whether you should trust the pub* information.

Right. I also need to read the top-level DIE to get the range list for the CU.

>> Building
>> the .gdb_index in gold is still too slow, but these attributes help
>> make it a bit faster than it was without them.
>>
>> Are these two attributes a showstopper issue for you?
>
> No, I'm just reluctant to add more relocations.  Could we make them just
> flags?

That might be workable. Let me take a look at the gold changes I'd
need to make for that.

They don't have to be relocations, though -- since they're only used
by the linker, a raw section-relative offset would be sufficient.

>> Yes, I understand that's broken, but there are no consumers at this
>> point that make any use of that offset. Would it be acceptable if we
>> just put 0 there? (Given that I expect .debug_pub* to go away soon, I
>> don't think it's worth the trouble of filling in the offset with
>> anything more meaningful.)
>
> I wouldn't expect it to be much harder to put the skeleton there than plain
> 0.

I was concerned that we might not always have a skeleton type to point
to, but I confess I haven't looked closely enough to know whether
that's a valid concern.

-cary

[Cary, 2012-05-29 22:55:39]

>>> Yes, I understand that's broken, but there are no consumers at this
>>> point that make any use of that offset. Would it be acceptable if we
>>> just put 0 there? (Given that I expect .debug_pub* to go away soon, I
>>> don't think it's worth the trouble of filling in the offset with
>>> anything more meaningful.)
>>
>> I wouldn't expect it to be much harder to put the skeleton there than plain
>> 0.
>
> I was concerned that we might not always have a skeleton type to point
> to, but I confess I haven't looked closely enough to know whether
> that's a valid concern.

At the time we emit the pubtypes table, we have a pointer to the DIE
that has been moved to the type unit, and there's no mapping from that
back to the skeleton DIE. As it stands, we don't even emit a skeleton
DIE unless one of its descendants is a declaration, so we can't count
on always having a skeleton DIE to point to. In the case of
enumeration constants, if we did have a skeleton DIE, it would only be
for the parent enumeration type.

How about we modify the patch to just emit a 0 for the DIE offset in a
pubtype entry?

-cary

[Cary, 2012-05-30 17:52:27]

> At the time we emit the pubtypes table, we have a pointer to the DIE
> that has been moved to the type unit, and there's no mapping from that
> back to the skeleton DIE. As it stands, we don't even emit a skeleton
> DIE unless one of its descendants is a declaration, so we can't count
> on always having a skeleton DIE to point to. In the case of
> enumeration constants, if we did have a skeleton DIE, it would only be
> for the parent enumeration type.
>
> How about we modify the patch to just emit a 0 for the DIE offset in a
> pubtype entry?

I can add a field to the comdat_type_node structure to keep track of
the skeleton DIE for a given type unit, so that I can easily get the
right DIE offset for cases where there is a skeleton DIE. When there
is no skeleton DIE, I'll change it to emit 0 for the DIE offset. Sound
OK?

-cary

[saugustine, 2012-06-01 17:58:41]

The enclosed patch updates the earlier patch to address all of the feedback I
have gotten regarding generating pubnames. It fixes the offset problem in
the pubtypes table; switches DW_AT_pubtypes to a flag and so on.

It also adds and documents a new option "-g[no-]pubtypes" which allows users
to generate pubtypes even if the target disables them by default.

Tested with bootstrap and running the test_pubnames_and_indices.py script
recently contributed to the GDB project.

OK for mainline?

Sterling

2012-05-10   Sterling Augustine  <saugustine@google.com>
        Cary Coutant  <ccoutant@google.com>

	* gcc/dwarf2out.c (is_cu_die, is_namespace_die, is_class_die,
	add_AT_pubnames, add_enumerator_pubname, want_pubnames): New functions.
	(comdat_type_struct): New field 'skeleton_die'.
	(breakout_comdat_types): Update it.
	(add_pubname): Rework logic.  Call is_class_die, is_cu_die and
	is_namespace_die.  Fix minor style violation.  Call want_pubnames.
	(add_pubname_string): Call want_pubnames.
	(add_pubtype): Rework logic for calculating type name.  Call
	is_namespace_die.  Call want_pubnames.
	(output_pubnames): Move conditional logic deciding when to produce the
	section from dwarf2out_finish.  Use new skeleton_die field.
	(base_type_die): Call add_pubtype.
	(gen_enumeration_type_die): Unconditionally call add_pubtype.
	(gen_subprogram_die): Adjust calls to add_pubname.
	(gen_namespace_die): Call add_pubname_string.
	(dwarf2out_finish): Call add_AT_pubnames; Move logic on when to
	produce pubnames and pubtypes sections to output_pubnames.
	(gcc/common.opt): New option '-gpubnames'.
	(gcc/invoke.texi): Document it.

Index: gcc/dwarf2out.c
===================================================================
--- gcc/dwarf2out.c	(revision 188035)
+++ gcc/dwarf2out.c	(working copy)
@@ -2539,6 +2539,7 @@
 {
   dw_die_ref root_die;
   dw_die_ref type_die;
+  dw_die_ref skeleton_die;
   char signature[DWARF_TYPE_SIGNATURE_SIZE];
   struct comdat_type_struct *next;
 }
@@ -3013,6 +3014,7 @@
 static void output_comdat_type_unit (comdat_type_node *);
 static const char *dwarf2_name (tree, int);
 static void add_pubname (tree, dw_die_ref);
+static void add_enumerator_pubname (const char *, dw_die_ref);
 static void add_pubname_string (const char *, dw_die_ref);
 static void add_pubtype (tree, dw_die_ref);
 static void output_pubnames (VEC (pubname_entry,gc) *);
@@ -3142,6 +3144,7 @@
 				     const char *, const char *);
 static void output_loc_list (dw_loc_list_ref);
 static char *gen_internal_sym (const char *);
+static bool want_pubnames (void);
 
 static void prune_unmark_dies (dw_die_ref);
 static void prune_unused_types_mark_generic_parms_dies (dw_die_ref);
@@ -5972,6 +5975,23 @@
   return c && c->die_tag == DW_TAG_compile_unit;
 }
 
+/* Returns true iff C is a namespace DIE.  */
+
+static inline bool
+is_namespace_die (dw_die_ref c)
+{
+  return c && c->die_tag == DW_TAG_namespace;
+}
+
+/* Returns true iff C is a class or structure DIE.  */
+
+static inline bool
+is_class_die (dw_die_ref c)
+{
+  return c && (c->die_tag == DW_TAG_class_type
+               || c->die_tag == DW_TAG_structure_type);
+}
+
 static char *
 gen_internal_sym (const char *prefix)
 {
@@ -6559,6 +6579,7 @@
            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);
+	type_node->skeleton_die = replacement;
 
         /* Break out nested types into their own type units.  */
         break_out_comdat_types (c);
@@ -8034,6 +8055,27 @@
     }
 }
 
+/* Whether to generate the DWARF accelerator tables in .debug_pubnames
+   and .debug_pubtypes.  This is configured per-target, but can be
+   overridden by the -gpubnames or -gno-pubnames options.  */
+
+static inline bool
+want_pubnames (void)
+{
+  return (debug_generate_pub_sections != -1
+	  ? debug_generate_pub_sections
+	  : targetm.want_debug_pub_sections);
+}
+
+/* Add the DW_AT_GNU_pubnames and DW_AT_GNU_pubtypes attributes.  */
+
+static void
+add_AT_pubnames (dw_die_ref die)
+{
+  if (want_pubnames ())
+    add_AT_flag (die, DW_AT_GNU_pubnames, 1);
+}
+
 /* Output a comdat type unit DIE and its children.  */
 
 static void
@@ -8104,7 +8146,7 @@
 static void
 add_pubname_string (const char *str, dw_die_ref die)
 {
-  if (targetm.want_debug_pub_sections)
+  if (want_pubnames ())
     {
       pubname_entry e;
 
@@ -8117,14 +8159,32 @@
 static void
 add_pubname (tree decl, dw_die_ref die)
 {
-  if (targetm.want_debug_pub_sections && TREE_PUBLIC (decl))
+  if (!want_pubnames ())
+    return;
+
+  if ((TREE_PUBLIC (decl) && !is_class_die (die->die_parent))
+      || is_cu_die (die->die_parent) || is_namespace_die (die->die_parent))
     {
       const char *name = dwarf2_name (decl, 1);
+
       if (name)
 	add_pubname_string (name, die);
     }
 }
 
+/* Add an enumerator to the pubnames section.  */
+
+static void
+add_enumerator_pubname (const char *scope_name, dw_die_ref die)
+{
+  pubname_entry e;
+
+  gcc_assert (scope_name);
+  e.name = concat (scope_name, get_AT_string (die, DW_AT_name), NULL);
+  e.die = die;
+  VEC_safe_push (pubname_entry, gc, pubtype_table, &e);
+}
+
 /* Add a new entry to .debug_pubtypes if appropriate.  */
 
 static void
@@ -8132,40 +8192,55 @@
 {
   pubname_entry e;
 
-  if (!targetm.want_debug_pub_sections)
+  if (!want_pubnames ())
     return;
 
-  e.name = NULL;
   if ((TREE_PUBLIC (decl)
-       || is_cu_die (die->die_parent))
+       || is_cu_die (die->die_parent) || is_namespace_die (die->die_parent))
       && (die->die_tag == DW_TAG_typedef || COMPLETE_TYPE_P (decl)))
     {
-      e.die = die;
-      if (TYPE_P (decl))
-	{
-	  if (TYPE_NAME (decl))
+      tree scope = NULL;
+      const char *scope_name = "";
+      const char *sep = is_cxx () ? "::" : ".";
+      const char *name;
+
+      scope = TYPE_P (decl) ? TYPE_CONTEXT (decl) : NULL;
+      if (scope && TREE_CODE (scope) == NAMESPACE_DECL)
 	    {
-	      if (TREE_CODE (TYPE_NAME (decl)) == IDENTIFIER_NODE)
-		e.name = IDENTIFIER_POINTER (TYPE_NAME (decl));
-	      else if (TREE_CODE (TYPE_NAME (decl)) == TYPE_DECL
-		       && DECL_NAME (TYPE_NAME (decl)))
-		e.name = IDENTIFIER_POINTER (DECL_NAME (TYPE_NAME (decl)));
+          scope_name = lang_hooks.dwarf_name (scope, 1);
+          if (scope_name != NULL && scope_name[0] != '\0')
+            scope_name = concat (scope_name, sep, NULL);
 	      else
-	       e.name = xstrdup ((const char *) get_AT_string (die, DW_AT_name));
-	    }
+            scope_name = "";
 	}
+
+      if (TYPE_P (decl))
+        name = type_tag (decl);
       else
-	{
-	  e.name = dwarf2_name (decl, 1);
-	  if (e.name)
-	    e.name = xstrdup (e.name);
-	}
+        name = lang_hooks.dwarf_name (decl, 1);
 
       /* If we don't have a name for the type, there's no point in adding
 	 it to the table.  */
-      if (e.name && e.name[0] != '\0')
+      if (name != NULL && name[0] != '\0')
+        {
+          e.die = die;
+          e.name = concat (scope_name, name, NULL);
 	VEC_safe_push (pubname_entry, gc, pubtype_table, &e);
     }
+
+      /* Although it might be more consistent to add the pubinfo for the
+         enumerators as their dies are created, they should only be added if
the
+         enum type meets the criteria above.  So rather than re-check the
parent
+         enum type whenever an enumerator die is created, just output them all
+         here.  This isn't protected by the name conditional because anoymous
+         enums don't have names.  */
+      if (die->die_tag == DW_TAG_enumeration_type)
+        {
+          dw_die_ref c;
+
+          FOR_EACH_CHILD (die, c, add_enumerator_pubname (scope_name, c));
+        }
+    }
 }
 
 /* Output the public names table used to speed up access to externally
@@ -8178,6 +8253,12 @@
   unsigned long pubnames_length = size_of_pubnames (names);
   pubname_ref pub;
 
+  if (!want_pubnames () || !info_section_emitted)
+    return;
+  if (names == pubname_table)
+    switch_to_section (debug_pubnames_section);
+  else
+    switch_to_section (debug_pubtypes_section);
   if (DWARF_INITIAL_LENGTH_SIZE - DWARF_OFFSET_SIZE == 4)
     dw2_asm_output_data (4, 0xffffffff,
       "Initial length escape value indicating 64-bit DWARF extension");
@@ -8205,9 +8286,22 @@
 	  || pub->die->die_offset != 0
 	  || !flag_eliminate_unused_debug_types)
 	{
-	  dw2_asm_output_data (DWARF_OFFSET_SIZE, pub->die->die_offset,
-			       "DIE offset");
+	  dw_offset die_offset = pub->die->die_offset;
 
+	  /* If we're putting types in their own .debug_types sections,
+	     the .debug_pubtypes table will still point to the compile
+	     unit (not the type unit), so we want to use the offset of
+	     the skeleton DIE (if there is one).  */
+	  if (use_debug_types && names == pubtype_table)
+	    {
+	      comdat_type_node_ref type_node = pub->die->die_id.die_type_node;
+
+	      if (type_node != NULL && type_node->skeleton_die != NULL)
+		die_offset = type_node->skeleton_die->die_offset;
+	    }
+
+	  dw2_asm_output_data (DWARF_OFFSET_SIZE, die_offset, "DIE offset");
+
 	  dw2_asm_output_nstring (pub->name, -1, "external name");
 	}
     }
@@ -9091,6 +9185,7 @@
   add_AT_unsigned (base_type_result, DW_AT_byte_size,
 		   int_size_in_bytes (type));
   add_AT_unsigned (base_type_result, DW_AT_encoding, encoding);
+  add_pubtype (type, base_type_result);
 
   return base_type_result;
 }
@@ -16173,7 +16268,6 @@
   else
     add_AT_flag (type_die, DW_AT_declaration, 1);
 
-  if (get_AT (type_die, DW_AT_name))
     add_pubtype (type, type_die);
 
   return type_die;
@@ -16813,6 +16907,7 @@
 	{
 	  subr_die = new_die (DW_TAG_subprogram, context_die, decl);
 	  add_AT_specification (subr_die, old_die);
+          add_pubname (decl, subr_die);
 	  if (get_AT_file (old_die, DW_AT_decl_file) != file_index)
 	    add_AT_file (subr_die, DW_AT_decl_file, file_index);
 	  if (get_AT_unsigned (old_die, DW_AT_decl_line) != (unsigned) s.line)
@@ -16827,6 +16922,7 @@
 	add_AT_flag (subr_die, DW_AT_external, 1);
 
       add_name_and_src_coords_attributes (subr_die, decl);
+      add_pubname (decl, subr_die);
       if (debug_info_level > DINFO_LEVEL_TERSE)
 	{
 	  add_prototyped_attribute (subr_die, TREE_TYPE (decl));
@@ -16938,7 +17034,6 @@
       }
 #endif
 
-	  add_pubname (decl, subr_die);
 	}
       else
 	{
@@ -16959,7 +17054,6 @@
 		  add_ranges_by_labels (subr_die, fde->dw_fde_second_begin,
 					fde->dw_fde_second_end,
 					&range_list_added);
-		  add_pubname (decl, subr_die);
 		  if (range_list_added)
 		    add_ranges (NULL);
 		}
@@ -16981,8 +17075,6 @@
 				 fde->dw_fde_begin);
 		  add_AT_lbl_id (subr_die, DW_AT_high_pc,
 				 fde->dw_fde_end);
-		  /* Add it.   */
-		  add_pubname (decl, subr_die);
 
 		  /* Build a minimal DIE for the secondary section.  */
 		  seg_die = new_die (DW_TAG_subprogram,
@@ -17018,7 +17110,6 @@
 	    {
 	      add_AT_lbl_id (subr_die, DW_AT_low_pc, fde->dw_fde_begin);
 	      add_AT_lbl_id (subr_die, DW_AT_high_pc, fde->dw_fde_end);
-	      add_pubname (decl, subr_die);
 	    }
 	}
 
@@ -19049,6 +19140,8 @@
       add_AT_die_ref (namespace_die, DW_AT_import, origin_die);
       equate_decl_number_to_die (decl, namespace_die);
     }
+  /* Bypass dwarf2_name's check for DECL_NAMELESS.  */
+  add_pubname_string (lang_hooks.dwarf_name (decl, 1), namespace_die);
 }
 
 /* Generate Dwarf debug information for a decl described by DECL.
@@ -22309,6 +22402,8 @@
     }
   htab_delete (comdat_type_table);
 
+  add_AT_pubnames (comp_unit_die ());
+
   /* Output the main compilation unit if non-empty or if .debug_macinfo
      or .debug_macro will be emitted.  */
   output_comp_unit (comp_unit_die (), have_macinfo);
@@ -22332,42 +22427,12 @@
       output_location_lists (comp_unit_die ());
     }
 
-  /* Output public names table if necessary.  */
-  if (!VEC_empty (pubname_entry, pubname_table))
-    {
-      gcc_assert (info_section_emitted);
-      switch_to_section (debug_pubnames_section);
-      output_pubnames (pubname_table);
-    }
-
-  /* Output public types table if necessary.  */
+  /* Output public names and types tables if necessary.  */
+  output_pubnames (pubname_table);
   /* ??? Only defined by DWARF3, but emitted by Darwin for DWARF2.
      It shouldn't hurt to emit it always, since pure DWARF2 consumers
      simply won't look for the section.  */
-  if (!VEC_empty (pubname_entry, pubtype_table))
-    {
-      bool empty = false;
-      
-      if (flag_eliminate_unused_debug_types)
-	{
-	  /* The pubtypes table might be emptied by pruning unused items.  */
-	  unsigned i;
-	  pubname_ref p;
-	  empty = true;
-	  FOR_EACH_VEC_ELT (pubname_entry, pubtype_table, i, p)
-	    if (p->die->die_offset != 0)
-	      {
-		empty = false;
-		break;
-	      }
-	}
-      if (!empty)
-	{
-	  gcc_assert (info_section_emitted);
-	  switch_to_section (debug_pubtypes_section);
-	  output_pubnames (pubtype_table);
-	}
-    }
+  output_pubnames (pubtype_table);
 
   /* Output the address range information if a CU (.debug_info section)
      was emitted.  We output an empty table even if we had no functions
Index: gcc/common.opt
===================================================================
--- gcc/common.opt	(revision 188034)
+++ gcc/common.opt	(working copy)
@@ -2239,6 +2239,14 @@
 Common JoinedOrMissing
 Generate debug information in default extended format
 
+gno-pubnames
+Common RejectNegative Var(debug_generate_pub_sections, 0) Init(-1)
+Don't generate DWARF pubnames and pubtypes sections.
+
+gpubnames
+Common RejectNegative Var(debug_generate_pub_sections, 1)
+Generate DWARF pubnames and pubtypes sections.
+
 gno-record-gcc-switches
 Common RejectNegative Var(dwarf_record_gcc_switches,0) Init(1)
 Don't record gcc command line switches in DWARF DW_AT_producer.
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 188034)
+++ gcc/doc/invoke.texi	(working copy)
@@ -4799,6 +4799,10 @@
 if neither of those are supported), including GDB extensions if at all
 possible.
 
+@item -gpubnames
+@opindex gpubnames
+Generate dwarf .debug_pubnames and .debug_pubtypes sections.
+
 @item -gstabs
 @opindex gstabs
 Produce debugging information in stabs format (if that is supported),

--
This patch is available for review at http://codereview.appspot.com/6197069

[saugustine, 2012-06-07 21:25:56]

On 2012/06/01 17:58:41, saugustine wrote:
> The enclosed patch updates the earlier patch to address all of the feedback I
> have gotten regarding generating pubnames. It fixes the offset problem in
> the pubtypes table; switches DW_AT_pubtypes to a flag and so on.
> 
> It also adds and documents a new option "-g[no-]pubtypes" which allows users
> to generate pubtypes even if the target disables them by default.
> 
> Tested with bootstrap and running the test_pubnames_and_indices.py script
> recently contributed to the GDB project.
> 

Ping? I have another patch that depends on this one coming.

[jason_redhat.com, 2012-06-08 18:45:11]

On 06/01/2012 01:58 PM, Sterling Augustine wrote:
> It also adds and documents a new option "-g[no-]pubtypes" which allows users
> to generate pubtypes even if the target disables them by default.

Hmm, I thought the convention for this sort of flag was to start with 
-f, that -g flags were only for selecting the type and level of debug 
info.  But I see that there are other debug dialect switches that use 
-g, so I guess this is OK.

> +  if ((TREE_PUBLIC (decl) && !is_class_die (die->die_parent))

Why not include public class members?  Is the theory that you'll need to 
read the type DIE anyway to do anything with the member, so you might as 
well read it to look up the member in the first place?  There should be 
a comment about this.

> +  VEC_safe_push (pubname_entry, gc, pubtype_table, &e);

Why add enumerators to pubtypes rather than pubnames?  They seem more 
like variables than types to me.

> +         here.  This isn't protected by the name conditional because
anoymous

Missing an 'n' in "anonymous".

A C++ opaque enumeration is considered complete, but does not yet have 
its enumerators, which can be added later.  Does this code handle that?

> +	  if (use_debug_types && names == pubtype_table)

This should check pub->die->comdat_type_p

Jason

[jakub_redhat.com, 2012-06-08 18:47:17]

On Fri, Jun 08, 2012 at 02:45:09PM -0400, Jason Merrill wrote:
> On 06/01/2012 01:58 PM, Sterling Augustine wrote:
> >It also adds and documents a new option "-g[no-]pubtypes" which allows users
> >to generate pubtypes even if the target disables them by default.
> 
> Hmm, I thought the convention for this sort of flag was to start
> with -f, that -g flags were only for selecting the type and level of
> debug info.  But I see that there are other debug dialect switches
> that use -g, so I guess this is OK.

But we have -f{,no-}debug-types-section, so perhaps consistent with that
would be -f{,no-}debug-pubtypes-section ?

	Jakub

[Cary, 2012-06-08 21:22:39]

> Hmm, I thought the convention for this sort of flag was to start with -f,
> that -g flags were only for selecting the type and level of debug info.  But
> I see that there are other debug dialect switches that use -g, so I guess
> this is OK.

I kind of prefer -g, but I did notice that it's -fdebug-types-section,
so I could go with -f[no-]pubnames (or, as Jakub suggests,
-f[no-]debug-pubnames-section). On the other hand, there's
-g[no-]record-gcc-switches. What would you prefer?

If we change it, should we also change (in a later patch)
-gsplit-dwarf (orig. -gfission) to -fsplit-dwarf?

>> +  if ((TREE_PUBLIC (decl) && !is_class_die (die->die_parent))
>
> Why not include public class members?  Is the theory that you'll need to
> read the type DIE anyway to do anything with the member, so you might as
> well read it to look up the member in the first place?  There should be a
> comment about this.

Yes, that's the theory. We're just trying to do the same thing that
GDB does when it builds .gdb_index.

>> +  VEC_safe_push (pubname_entry, gc, pubtype_table, &e);
>
> Why add enumerators to pubtypes rather than pubnames?  They seem more like
> variables than types to me.

That looks like a typo. It hasn't caused us any trouble because the
linker just throws all the names into the same table in .gdb_index,
but yes, I think it should go in pubnames_table.

-cary

[saugustine, 2012-06-08 22:03:15]

On Fri, Jun 8, 2012 at 2:22 PM, Cary Coutant <ccoutant@google.com> wrote:
>> Hmm, I thought the convention for this sort of flag was to start with -f,
>> that -g flags were only for selecting the type and level of debug info.  But
>> I see that there are other debug dialect switches that use -g, so I guess
>> this is OK.
>
> I kind of prefer -g, but I did notice that it's -fdebug-types-section,
> so I could go with -f[no-]pubnames (or, as Jakub suggests,
> -f[no-]debug-pubnames-section). On the other hand, there's
> -g[no-]record-gcc-switches. What would you prefer?
>
> If we change it, should we also change (in a later patch)
> -gsplit-dwarf (orig. -gfission) to -fsplit-dwarf?
>
>>> +  if ((TREE_PUBLIC (decl) && !is_class_die (die->die_parent))
>>
>> Why not include public class members?  Is the theory that you'll need to
>> read the type DIE anyway to do anything with the member, so you might as
>> well read it to look up the member in the first place?  There should be a
>> comment about this.
>
> Yes, that's the theory. We're just trying to do the same thing that
> GDB does when it builds .gdb_index.
>
>>> +  VEC_safe_push (pubname_entry, gc, pubtype_table, &e);
>>
>> Why add enumerators to pubtypes rather than pubnames?  They seem more like
>> variables than types to me.
>
> That looks like a typo. It hasn't caused us any trouble because the
> linker just throws all the names into the same table in .gdb_index,
> but yes, I think it should go in pubnames_table.
>
> -cary

OK, I've updated the patch with all these additional comments. Just
waiting on the decision between -f and -g. I'll repost and then commit
it when that is settled--hopefully soon.

Next up, the big fission patch!

Sterling

[saugustine, 2012-06-12 18:53:19]

On Fri, Jun 8, 2012 at 3:03 PM, Sterling Augustine
<saugustine@google.com> wrote:

[Regarding generating pubnames]

> OK, I've updated the patch with all these additional comments. Just
> waiting on the decision between -f and -g. I'll repost and then commit
> it when that is settled--hopefully soon.
>
> Next up, the big fission patch!
>
> Sterling

Ping

[jason_redhat.com, 2012-06-13 04:32:19]

On 06/08/2012 05:22 PM, Cary Coutant wrote:
> I kind of prefer -g, but I did notice that it's -fdebug-types-section,
> so I could go with -f[no-]pubnames (or, as Jakub suggests,
> -f[no-]debug-pubnames-section). On the other hand, there's
> -g[no-]record-gcc-switches. What would you prefer?
>
> If we change it, should we also change (in a later patch)
> -gsplit-dwarf (orig. -gfission) to -fsplit-dwarf?

I lean toward -g myself, since there doesn't seem to be a strong rule 
one way or the other.

Jason

[saugustine, 2012-06-13 23:26:02]

> I lean toward -g myself, since there doesn't seem to be a strong rule one
> way or the other.

Unless there are further comments, I'll stick with -g then.

I think that covers all the comments, so I think I will commit this
Friday morning unless I hear anything further.

Sterling

[jason_redhat.com, 2012-06-14 05:48:08]

On 06/13/2012 04:26 PM, Sterling Augustine wrote:
>> I lean toward -g myself, since there doesn't seem to be a strong rule one
>> way or the other.
>
> Unless there are further comments, I'll stick with -g then.
>
> I think that covers all the comments, so I think I will commit this
> Friday morning unless I hear anything further.

Weren't you going to repost the patch first?  :)

Jason

[saugustine, 2012-06-14 17:50:35]

This is a revised edition of the fix pubnames patch discussed earlier; it
addresses all the earlier comments made.

This edition of the patch has three changes from the earlier one:

1. Fix a typo (anoymous -> anonymous).
2. Move enumerator names to pubnames from pubtypes.
3. Switch to using the comdat_type_p field.

It sticks with -gpubnames as earlier discussed.

Anything else?

Sterling

2012-06-14   Sterling Augustine  <saugustine@google.com>
        Cary Coutant  <ccoutant@google.com>

	* dwarf2out.c (is_cu_die, is_namespace_die, is_class_die,
	add_AT_pubnames, add_enumerator_pubname, want_pubnames): New functions.
	(comdat_type_struct): New field 'skeleton_die'.
	(breakout_comdat_types): Update it.
	(add_pubname): Rework logic.  Call is_class_die, is_cu_die and
	is_namespace_die.  Fix minor style violation.  Call want_pubnames.
	(add_pubname_string): Call want_pubnames.
	(add_pubtype): Rework logic for calculating type name.  Call
	is_namespace_die.  Call want_pubnames.
	(output_pubnames): Move conditional logic deciding when to produce the
	section from dwarf2out_finish.  Use new skeleton_die field.
	(base_type_die): Call add_pubtype.
	(gen_enumeration_type_die): Unconditionally call add_pubtype.
	(gen_subprogram_die): Adjust calls to add_pubname.
	(gen_namespace_die): Call add_pubname_string.
	(dwarf2out_finish): Call add_AT_pubnames; Move logic on when to
	produce pubnames and pubtypes sections to output_pubnames.
	(common.opt): New option '-gpubnames'.
	(invoke.texi): Document it.



Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 188622)
+++ gcc/doc/invoke.texi	(working copy)
@@ -4795,6 +4795,10 @@
 if neither of those are supported), including GDB extensions if at all
 possible.
 
+@item -gpubnames
+@opindex gpubnames
+Generate dwarf .debug_pubnames and .debug_pubtypes sections.
+
 @item -gstabs
 @opindex gstabs
 Produce debugging information in stabs format (if that is supported),
Index: gcc/dwarf2out.c
===================================================================
--- gcc/dwarf2out.c	(revision 188622)
+++ gcc/dwarf2out.c	(working copy)
@@ -2539,6 +2539,7 @@
 {
   dw_die_ref root_die;
   dw_die_ref type_die;
+  dw_die_ref skeleton_die;
   char signature[DWARF_TYPE_SIGNATURE_SIZE];
   struct comdat_type_struct *next;
 }
@@ -3013,6 +3014,7 @@
 static void output_comdat_type_unit (comdat_type_node *);
 static const char *dwarf2_name (tree, int);
 static void add_pubname (tree, dw_die_ref);
+static void add_enumerator_pubname (const char *, dw_die_ref);
 static void add_pubname_string (const char *, dw_die_ref);
 static void add_pubtype (tree, dw_die_ref);
 static void output_pubnames (VEC (pubname_entry,gc) *);
@@ -3142,6 +3144,7 @@
 				     const char *, const char *);
 static void output_loc_list (dw_loc_list_ref);
 static char *gen_internal_sym (const char *);
+static bool want_pubnames (void);
 
 static void prune_unmark_dies (dw_die_ref);
 static void prune_unused_types_mark_generic_parms_dies (dw_die_ref);
@@ -5982,6 +5985,23 @@
 	       || c->die_tag == DW_TAG_type_unit);
 }
 
+/* Returns true iff C is a namespace DIE.  */
+
+static inline bool
+is_namespace_die (dw_die_ref c)
+{
+  return c && c->die_tag == DW_TAG_namespace;
+}
+
+/* Returns true iff C is a class or structure DIE.  */
+
+static inline bool
+is_class_die (dw_die_ref c)
+{
+  return c && (c->die_tag == DW_TAG_class_type
+               || c->die_tag == DW_TAG_structure_type);
+}
+
 static char *
 gen_internal_sym (const char *prefix)
 {
@@ -6568,6 +6588,7 @@
            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);
+	type_node->skeleton_die = replacement;
 
         /* Break out nested types into their own type units.  */
         break_out_comdat_types (c);
@@ -8041,6 +8062,27 @@
     }
 }
 
+/* Whether to generate the DWARF accelerator tables in .debug_pubnames
+   and .debug_pubtypes.  This is configured per-target, but can be
+   overridden by the -gpubnames or -gno-pubnames options.  */
+
+static inline bool
+want_pubnames (void)
+{
+  return (debug_generate_pub_sections != -1
+	  ? debug_generate_pub_sections
+	  : targetm.want_debug_pub_sections);
+}
+
+/* Add the DW_AT_GNU_pubnames and DW_AT_GNU_pubtypes attributes.  */
+
+static void
+add_AT_pubnames (dw_die_ref die)
+{
+  if (want_pubnames ())
+    add_AT_flag (die, DW_AT_GNU_pubnames, 1);
+}
+
 /* Output a comdat type unit DIE and its children.  */
 
 static void
@@ -8111,7 +8153,7 @@
 static void
 add_pubname_string (const char *str, dw_die_ref die)
 {
-  if (targetm.want_debug_pub_sections)
+  if (want_pubnames ())
     {
       pubname_entry e;
 
@@ -8124,14 +8166,32 @@
 static void
 add_pubname (tree decl, dw_die_ref die)
 {
-  if (targetm.want_debug_pub_sections && TREE_PUBLIC (decl))
+  if (!want_pubnames ())
+    return;
+
+  if ((TREE_PUBLIC (decl) && !is_class_die (die->die_parent))
+      || is_cu_die (die->die_parent) || is_namespace_die (die->die_parent))
     {
       const char *name = dwarf2_name (decl, 1);
+
       if (name)
 	add_pubname_string (name, die);
     }
 }
 
+/* Add an enumerator to the pubnames section.  */
+
+static void
+add_enumerator_pubname (const char *scope_name, dw_die_ref die)
+{
+  pubname_entry e;
+
+  gcc_assert (scope_name);
+  e.name = concat (scope_name, get_AT_string (die, DW_AT_name), NULL);
+  e.die = die;
+  VEC_safe_push (pubname_entry, gc, pubname_table, &e);
+}
+
 /* Add a new entry to .debug_pubtypes if appropriate.  */
 
 static void
@@ -8139,40 +8199,55 @@
 {
   pubname_entry e;
 
-  if (!targetm.want_debug_pub_sections)
+  if (!want_pubnames ())
     return;
 
-  e.name = NULL;
   if ((TREE_PUBLIC (decl)
-       || is_cu_die (die->die_parent))
+       || is_cu_die (die->die_parent) || is_namespace_die (die->die_parent))
       && (die->die_tag == DW_TAG_typedef || COMPLETE_TYPE_P (decl)))
     {
-      e.die = die;
-      if (TYPE_P (decl))
-	{
-	  if (TYPE_NAME (decl))
+      tree scope = NULL;
+      const char *scope_name = "";
+      const char *sep = is_cxx () ? "::" : ".";
+      const char *name;
+
+      scope = TYPE_P (decl) ? TYPE_CONTEXT (decl) : NULL;
+      if (scope && TREE_CODE (scope) == NAMESPACE_DECL)
 	    {
-	      if (TREE_CODE (TYPE_NAME (decl)) == IDENTIFIER_NODE)
-		e.name = IDENTIFIER_POINTER (TYPE_NAME (decl));
-	      else if (TREE_CODE (TYPE_NAME (decl)) == TYPE_DECL
-		       && DECL_NAME (TYPE_NAME (decl)))
-		e.name = IDENTIFIER_POINTER (DECL_NAME (TYPE_NAME (decl)));
+          scope_name = lang_hooks.dwarf_name (scope, 1);
+          if (scope_name != NULL && scope_name[0] != '\0')
+            scope_name = concat (scope_name, sep, NULL);
 	      else
-	       e.name = xstrdup ((const char *) get_AT_string (die, DW_AT_name));
-	    }
+            scope_name = "";
 	}
+
+      if (TYPE_P (decl))
+        name = type_tag (decl);
       else
-	{
-	  e.name = dwarf2_name (decl, 1);
-	  if (e.name)
-	    e.name = xstrdup (e.name);
-	}
+        name = lang_hooks.dwarf_name (decl, 1);
 
       /* If we don't have a name for the type, there's no point in adding
 	 it to the table.  */
-      if (e.name && e.name[0] != '\0')
+      if (name != NULL && name[0] != '\0')
+        {
+          e.die = die;
+          e.name = concat (scope_name, name, NULL);
 	VEC_safe_push (pubname_entry, gc, pubtype_table, &e);
     }
+
+      /* Although it might be more consistent to add the pubinfo for the
+         enumerators as their dies are created, they should only be added if
the
+         enum type meets the criteria above.  So rather than re-check the
parent
+         enum type whenever an enumerator die is created, just output them all
+         here.  This isn't protected by the name conditional because anonymous
+         enums don't have names.  */
+      if (die->die_tag == DW_TAG_enumeration_type)
+        {
+          dw_die_ref c;
+
+          FOR_EACH_CHILD (die, c, add_enumerator_pubname (scope_name, c));
+        }
+    }
 }
 
 /* Output the public names table used to speed up access to externally
@@ -8185,6 +8260,12 @@
   unsigned long pubnames_length = size_of_pubnames (names);
   pubname_ref pub;
 
+  if (!want_pubnames () || !info_section_emitted)
+    return;
+  if (names == pubname_table)
+    switch_to_section (debug_pubnames_section);
+  else
+    switch_to_section (debug_pubtypes_section);
   if (DWARF_INITIAL_LENGTH_SIZE - DWARF_OFFSET_SIZE == 4)
     dw2_asm_output_data (4, 0xffffffff,
       "Initial length escape value indicating 64-bit DWARF extension");
@@ -8212,9 +8293,22 @@
 	  || pub->die->die_offset != 0
 	  || !flag_eliminate_unused_debug_types)
 	{
-	  dw2_asm_output_data (DWARF_OFFSET_SIZE, pub->die->die_offset,
-			       "DIE offset");
+	  dw_offset die_offset = pub->die->die_offset;
 
+	  /* If we're putting types in their own .debug_types sections,
+	     the .debug_pubtypes table will still point to the compile
+	     unit (not the type unit), so we want to use the offset of
+	     the skeleton DIE (if there is one).  */
+	  if (pub->die->comdat_type_p && names == pubtype_table)
+	    {
+	      comdat_type_node_ref type_node = pub->die->die_id.die_type_node;
+
+	      if (type_node != NULL && type_node->skeleton_die != NULL)
+		die_offset = type_node->skeleton_die->die_offset;
+	    }
+
+	  dw2_asm_output_data (DWARF_OFFSET_SIZE, die_offset, "DIE offset");
+
 	  dw2_asm_output_nstring (pub->name, -1, "external name");
 	}
     }
@@ -9098,6 +9192,7 @@
   add_AT_unsigned (base_type_result, DW_AT_byte_size,
 		   int_size_in_bytes (type));
   add_AT_unsigned (base_type_result, DW_AT_encoding, encoding);
+  add_pubtype (type, base_type_result);
 
   return base_type_result;
 }
@@ -16180,7 +16275,6 @@
   else
     add_AT_flag (type_die, DW_AT_declaration, 1);
 
-  if (get_AT (type_die, DW_AT_name))
     add_pubtype (type, type_die);
 
   return type_die;
@@ -16844,6 +16938,7 @@
 	{
 	  subr_die = new_die (DW_TAG_subprogram, context_die, decl);
 	  add_AT_specification (subr_die, old_die);
+          add_pubname (decl, subr_die);
 	  if (get_AT_file (old_die, DW_AT_decl_file) != file_index)
 	    add_AT_file (subr_die, DW_AT_decl_file, file_index);
 	  if (get_AT_unsigned (old_die, DW_AT_decl_line) != (unsigned) s.line)
@@ -16858,6 +16953,7 @@
 	add_AT_flag (subr_die, DW_AT_external, 1);
 
       add_name_and_src_coords_attributes (subr_die, decl);
+      add_pubname (decl, subr_die);
       if (debug_info_level > DINFO_LEVEL_TERSE)
 	{
 	  add_prototyped_attribute (subr_die, TREE_TYPE (decl));
@@ -16969,7 +17065,6 @@
       }
 #endif
 
-	  add_pubname (decl, subr_die);
 	}
       else
 	{
@@ -16990,7 +17085,6 @@
 		  add_ranges_by_labels (subr_die, fde->dw_fde_second_begin,
 					fde->dw_fde_second_end,
 					&range_list_added);
-		  add_pubname (decl, subr_die);
 		  if (range_list_added)
 		    add_ranges (NULL);
 		}
@@ -17012,8 +17106,6 @@
 				 fde->dw_fde_begin);
 		  add_AT_lbl_id (subr_die, DW_AT_high_pc,
 				 fde->dw_fde_end);
-		  /* Add it.   */
-		  add_pubname (decl, subr_die);
 
 		  /* Build a minimal DIE for the secondary section.  */
 		  seg_die = new_die (DW_TAG_subprogram,
@@ -17049,7 +17141,6 @@
 	    {
 	      add_AT_lbl_id (subr_die, DW_AT_low_pc, fde->dw_fde_begin);
 	      add_AT_lbl_id (subr_die, DW_AT_high_pc, fde->dw_fde_end);
-	      add_pubname (decl, subr_die);
 	    }
 	}
 
@@ -19090,6 +19181,8 @@
       add_AT_die_ref (namespace_die, DW_AT_import, origin_die);
       equate_decl_number_to_die (decl, namespace_die);
     }
+  /* Bypass dwarf2_name's check for DECL_NAMELESS.  */
+  add_pubname_string (lang_hooks.dwarf_name (decl, 1), namespace_die);
 }
 
 /* Generate Dwarf debug information for a decl described by DECL.
@@ -22365,6 +22458,8 @@
     }
   htab_delete (comdat_type_table);
 
+  add_AT_pubnames (comp_unit_die ());
+
   /* Output the main compilation unit if non-empty or if .debug_macinfo
      or .debug_macro will be emitted.  */
   output_comp_unit (comp_unit_die (), have_macinfo);
@@ -22388,42 +22483,12 @@
       output_location_lists (comp_unit_die ());
     }
 
-  /* Output public names table if necessary.  */
-  if (!VEC_empty (pubname_entry, pubname_table))
-    {
-      gcc_assert (info_section_emitted);
-      switch_to_section (debug_pubnames_section);
-      output_pubnames (pubname_table);
-    }
-
-  /* Output public types table if necessary.  */
+  /* Output public names and types tables if necessary.  */
+  output_pubnames (pubname_table);
   /* ??? Only defined by DWARF3, but emitted by Darwin for DWARF2.
      It shouldn't hurt to emit it always, since pure DWARF2 consumers
      simply won't look for the section.  */
-  if (!VEC_empty (pubname_entry, pubtype_table))
-    {
-      bool empty = false;
-      
-      if (flag_eliminate_unused_debug_types)
-	{
-	  /* The pubtypes table might be emptied by pruning unused items.  */
-	  unsigned i;
-	  pubname_ref p;
-	  empty = true;
-	  FOR_EACH_VEC_ELT (pubname_entry, pubtype_table, i, p)
-	    if (p->die->die_offset != 0)
-	      {
-		empty = false;
-		break;
-	      }
-	}
-      if (!empty)
-	{
-	  gcc_assert (info_section_emitted);
-	  switch_to_section (debug_pubtypes_section);
-	  output_pubnames (pubtype_table);
-	}
-    }
+  output_pubnames (pubtype_table);
 
   /* Output the address range information if a CU (.debug_info section)
      was emitted.  We output an empty table even if we had no functions
Index: gcc/common.opt
===================================================================
--- gcc/common.opt	(revision 188622)
+++ gcc/common.opt	(working copy)
@@ -2239,6 +2239,14 @@
 Common JoinedOrMissing
 Generate debug information in default extended format
 
+gno-pubnames
+Common RejectNegative Var(debug_generate_pub_sections, 0) Init(-1)
+Don't generate DWARF pubnames and pubtypes sections.
+
+gpubnames
+Common RejectNegative Var(debug_generate_pub_sections, 1)
+Generate DWARF pubnames and pubtypes sections.
+
 gno-record-gcc-switches
 Common RejectNegative Var(dwarf_record_gcc_switches,0) Init(1)
 Don't record gcc command line switches in DWARF DW_AT_producer.

--
This patch is available for review at http://codereview.appspot.com/6197069

[saugustine, 2012-06-19 17:12:32]

On Wed, Jun 13, 2012 at 10:47 PM, Jason Merrill <jason@redhat.com> wrote:
> On 06/13/2012 04:26 PM, Sterling Augustine wrote:
>>>
>>> I lean toward -g myself, since there doesn't seem to be a strong rule one
>>> way or the other.
>>
>>
>> Unless there are further comments, I'll stick with -g then.
>>
>> I think that covers all the comments, so I think I will commit this
>> Friday morning unless I hear anything further.
>
>
> Weren't you going to repost the patch first?  :)

I hate how codereview.appspot.com doesn't connect some messages properly.

After this prompting, I re-posted the patch here:

http://gcc.gnu.org/ml/gcc-patches/2012-06/msg00949.html

As this has addressed all previous comments, and barring any
objections, I'll check it in tomorrow morning.

Sterling

[jason_redhat.com, 2012-06-20 00:51:34]

On 06/19/2012 10:12 AM, Sterling Augustine wrote:
> +	  /* If we're putting types in their own .debug_types sections,
> +	     the .debug_pubtypes table will still point to the compile
> +	     unit (not the type unit), so we want to use the offset of
> +	     the skeleton DIE (if there is one).  */
> +	  if (pub->die->comdat_type_p && names == pubtype_table)
> +	    {
> +	      comdat_type_node_ref type_node = pub->die->die_id.die_type_node;
> +
> +	      if (type_node != NULL && type_node->skeleton_die != NULL)
> +		die_offset = type_node->skeleton_die->die_offset;
> +	    }

I think we had agreed that if there is no skeleton, we should use an 
offset of 0.

Jason

[Cary, 2012-06-20 21:57:34]

>> +         /* If we're putting types in their own .debug_types sections,
>> +            the .debug_pubtypes table will still point to the compile
>> +            unit (not the type unit), so we want to use the offset of
>> +            the skeleton DIE (if there is one).  */
>> +         if (pub->die->comdat_type_p && names == pubtype_table)
>>
>> +           {
>> +             comdat_type_node_ref type_node =
>> pub->die->die_id.die_type_node;
>> +
>> +             if (type_node != NULL && type_node->skeleton_die != NULL)
>> +               die_offset = type_node->skeleton_die->die_offset;
>> +           }
>
>
> I think we had agreed that if there is no skeleton, we should use an offset
> of 0.

You're right, I forgot to handle that case. How's this look?

             if (type_node != NULL)
               die_offset = (type_node->skeleton_die != NULL
                             ? type_node->skeleton_die->die_offset
                             : 0);

Is that OK if it passes regression tests?

-cary

[jason_redhat.com, 2012-06-21 00:27:32]

OK.

Jason

[saugustine, 2012-06-21 18:18:50]

Committed as attached. Thanks for your reviews.

Sterling

gcc/ChangeLog

2012-06-21   Sterling Augustine  <saugustine@google.com>
        Cary Coutant  <ccoutant@google.com>

	* dwarf2out.c (is_cu_die, is_namespace_die, is_class_die,
	add_AT_pubnames, add_enumerator_pubname, want_pubnames): New functions.
	(comdat_type_struct): New field 'skeleton_die'.
	(breakout_comdat_types): Update it.
	(add_pubname): Rework logic.  Call is_class_die, is_cu_die and
	is_namespace_die.  Fix minor style violation.  Call want_pubnames.
	(add_pubname_string): Call want_pubnames.
	(add_pubtype): Rework logic for calculating type name.  Call
	is_namespace_die.  Call want_pubnames.
	(output_pubnames): Move conditional logic deciding when to produce the
	section from dwarf2out_finish.  Use new skeleton_die field.
	(base_type_die): Call add_pubtype.
	(gen_enumeration_type_die): Unconditionally call add_pubtype.
	(gen_subprogram_die): Adjust calls to add_pubname.
	(gen_namespace_die): Call add_pubname_string.
	(dwarf2out_finish): Call add_AT_pubnames; Move logic on when to
	produce pubnames and pubtypes sections to output_pubnames.
	(common.opt): New option '-gpubnames'.
	(invoke.texi): Document it.



On Wed, Jun 20, 2012 at 5:27 PM, Jason Merrill <jason@redhat.com> wrote:
> OK.
>
> Jason

[jason_redhat.com, 2012-06-22 09:35:14]

On 06/21/2012 11:18 AM, Sterling Augustine wrote:
> Committed as attached. Thanks for your reviews.

I've heard reports of new test failures due to this patch:

FAIL: gcc.dg/pubtypes-2.c scan-assembler long+[ \\t]+0x6a+[ \\t]+[#;]+[ 
\\t]+Length of Public Type Names Info
FAIL: gcc.dg/pubtypes-3.c scan-assembler long+[ \\t]+0x6a+[ \\t]+[#;]+[ 
\\t]+Length of Public Type Names Info
FAIL: gcc.dg/pubtypes-4.c scan-assembler long+[ \\t]+0xa1+[ \\t]+[#;]+[ 
\\t]+Length of Public Type Names Info

FAIL: g++.dg/debug/dwarf2/static-data-member2.C -std=gnu++98 
scan-assembler-not DW_TAG_enumerator
FAIL: g++.dg/debug/dwarf2/static-data-member2.C -std=gnu++98 
scan-assembler-not DW_TAG_enumeration_type
FAIL: g++.dg/debug/dwarf2/static-data-member2.C -std=gnu++11 
scan-assembler-not DW_TAG_enumerator
FAIL: g++.dg/debug/dwarf2/static-data-member2.C -std=gnu++11 
scan-assembler-not DW_TAG_enumeration_type

For the first group, you probably just need to update the test.  For the 
second, we still don't want to emit unused enumerations.

Jason

[saugustine, 2012-06-22 20:44:38]

On Fri, Jun 22, 2012 at 2:35 AM, Jason Merrill <jason@redhat.com> wrote:
> FAIL: g++.dg/debug/dwarf2/static-data-member2.C -std=gnu++98
> scan-assembler-not DW_TAG_enumerator
> FAIL: g++.dg/debug/dwarf2/static-data-member2.C -std=gnu++98
> scan-assembler-not DW_TAG_enumeration_type
> FAIL: g++.dg/debug/dwarf2/static-data-member2.C -std=gnu++11
> scan-assembler-not DW_TAG_enumerator
> FAIL: g++.dg/debug/dwarf2/static-data-member2.C -std=gnu++11
> scan-assembler-not DW_TAG_enumeration_type

This is a side effect of moving enumerators from pubnames to pubtypes
as requested in an earlier message in this thread. I should have
pushed back harder on this change.

prune_unused_types marks everything in the pubnames_table. If the
enumerators go in the pubname table (but the enum itself goes in the
pubtype table), then the unused enum becomes reachable from the
pubnames table.

The least complicated solution is to put them back in the
pubtypes_table. I could also defer adding them until after types are
pruned, I guess.

One somewhat unsatisfying way to rationalize it in my mind is to say
that as fields are to structs, so are enumerators to enums.

What do you think?

Sterling

gcc/ChangeLog
2012-06-22  Sterling Augustine  <sagustine@google.com>

	* dwarf2out.c (add_enumerator_pubname): Use pubtypes_table.

[Cary, 2012-06-22 21:15:27]

> prune_unused_types marks everything in the pubnames_table. If the
> enumerators go in the pubname table (but the enum itself goes in the
> pubtype table), then the unused enum becomes reachable from the
> pubnames table.
>
> The least complicated solution is to put them back in the
> pubtypes_table. I could also defer adding them until after types are
> pruned, I guess.
>
> One somewhat unsatisfying way to rationalize it in my mind is to say
> that as fields are to structs, so are enumerators to enums.

It doesn't matter much to me -- it was working with enumerators in the
pubtypes table, as far as I can tell.

But if the consensus turns out to be that enumerators should be in
pubnames, wouldn't it also be fairly easy to change prune_unused_types
so that it doesn't mark enumerators, and change output_pubnames to
skip enumerators that have been pruned?

-cary

[jason_redhat.com, 2012-06-23 04:46:35]

On 06/22/2012 02:15 PM, Cary Coutant wrote:
> But if the consensus turns out to be that enumerators should be in
> pubnames, wouldn't it also be fairly easy to change prune_unused_types
> so that it doesn't mark enumerators, and change output_pubnames to
> skip enumerators that have been pruned?

This makes sense to me.

Jason

[saugustine, 2012-06-26 01:08:15]

On Fri, Jun 22, 2012 at 9:46 PM, Jason Merrill <jason@redhat.com> wrote:
> On 06/22/2012 02:15 PM, Cary Coutant wrote:
>>
>> But if the consensus turns out to be that enumerators should be in
>> pubnames, wouldn't it also be fairly easy to change prune_unused_types
>> so that it doesn't mark enumerators, and change output_pubnames to
>> skip enumerators that have been pruned?
>
>
> This makes sense to me.

Enclosed is a patch that does it this way. It requires special-casing
enumerators in two places.

Personally, it seems cleaner to me just to put them in the pubtypes
table, but I am happy to do it whichever way you want.

Let me know,

Sterling

gcc/ChangeLog

2012-06-25  Sterling Augustine <saugustine@google.com>

        * dwarf2out.c (output_pubnames): Add check for DW_TAG_enumerator.
        (prune_unused_types): Add check for DW_TAG_enumerator.

[jason_redhat.com, 2012-06-26 03:35:25]

OK.

Jason

[Cary, 2012-07-23 20:21:13]

>>> But if the consensus turns out to be that enumerators should be in
>>> pubnames, wouldn't it also be fairly easy to change prune_unused_types
>>> so that it doesn't mark enumerators, and change output_pubnames to
>>> skip enumerators that have been pruned?
>>
>> This makes sense to me.
>
> Enclosed is a patch that does it this way. It requires special-casing
> enumerators in two places.
>
> Personally, it seems cleaner to me just to put them in the pubtypes
> table, but I am happy to do it whichever way you want.

Sterling, I think you were right all along -- this fails with
-fdebug-types-section. When we move an enumeration type out to a
separate types section, the DIE isn't marked when we try to add the
enumerators to the pubnames table, so the enumerators never get added
to the pubnames table. I'm not sure if there's an easy way to get them
added to pubnames in that case; it might be easier to go back to
putting them in pubtypes.

-cary