[RFA] dwarf2_physname

[RFA] dwarf2_physname

[Keith Seitz]

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

Hi,

I have been working on archer's expression parsing (and therefore symbol 
table madness) for the past year or so. I've managed to fix a lot of C++ 
problems on my archer-keiths-expr-cumulative branch. I would like to 
start attempting to push many of these patches into mainline FSF GDB, 
but of course, as with any large task of this type, the patches are HUGE 
(5500+ lines).

In an effort to help maintainers assess the code in smaller chunks 
without introducing new, temporary test suite failures, I'm going to try 
something "different" -- this patch, and the follow-ons, will not 
actually do anything until a final patch turns it all on. My hope is 
that this will be minimally intrusive to GDB's code base, especially 
given all the talk of a new release.

To start, I would like to submit a patch which is the beginnings of 
removing GDB's use of the DWARF2 quasi-attribute 
DW_AT_MIPS_linkage_name. Instead of using this attribute, GDB will start 
generating physical names itself.

Again, this patch will not actually change the current code path; the 
main entry, dwarf2_physname is not called (but you can imagine, it will 
replace all dwarf2_linkage_name calls). Therefore, there are no test 
suite regressions of any kind.

Keith

ChangeLog
2009-08-31  Keith Seitz  <keiths@redhat.com>

	* dwarf2read.c (die_list): New file global.
	(physname_prefix): New function.
	(physname_prefix_1): New function.
	(dwarf2_physname): New function.
	(die_needs_namespace): New function.

	Based on work from Daniel Jacobowitz  <dan@codesourcery.com>:
	* typeprint.h (c_type_print_args): Declare.
	* ui-file.h (ui_file_obsavestring): Declare.
	* c-typeprint.c (c_type_print_args): Add to global namespace.
	Add "show_articifical" parameter and only print them when
	requested.
	* ui-file.c (do_ui_file_obsavestring): New function.
	(ui_file_obsavestring): New function.

[-- Attachment #2: dwarf2_physname.patch --]
[-- Type: text/plain, Size: 12082 bytes --]

Index: dwarf2read.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.325
diff -u -p -r1.325 dwarf2read.c
--- dwarf2read.c	28 Aug 2009 10:49:05 -0000	1.325
+++ dwarf2read.c	31 Aug 2009 20:37:23 -0000
@@ -48,6 +48,9 @@
 #include "gdbcmd.h"
 #include "block.h"
 #include "addrmap.h"
+#include "typeprint.h"
+#include "jv-lang.h"
+#include "vec.h"
 
 #include <fcntl.h>
 #include "gdb_string.h"
@@ -691,6 +694,11 @@ struct field_info
     int nfnfields;
   };
 
+/* A vector used during linkage name generation.  */
+typedef struct die_info *die_info_p;
+DEF_VEC_P (die_info_p);
+static VEC(die_info_p) *die_list;
+
 /* One item on the queue of compilation units to read in full symbols
    for.  */
 struct dwarf2_queue_item
@@ -804,6 +812,8 @@ static void add_partial_symbol (struct p
 
 static int pdi_needs_namespace (enum dwarf_tag tag);
 
+static int die_needs_namespace (struct die_info *, struct dwarf2_cu *);
+
 static void add_partial_namespace (struct partial_die_info *pdi,
 				   CORE_ADDR *lowpc, CORE_ADDR *highpc,
 				   int need_pc, struct dwarf2_cu *cu);
@@ -949,6 +959,11 @@ static struct type *read_type_die (struc
 
 static char *determine_prefix (struct die_info *die, struct dwarf2_cu *);
 
+static char *physname_prefix (struct die_info *die, struct dwarf2_cu *);
+
+static void physname_prefix_1 (struct ui_file *, struct die_info *,
+			       struct dwarf2_cu *);
+
 static char *typename_concat (struct obstack *,
                               const char *prefix, 
                               const char *suffix,
@@ -1036,6 +1051,8 @@ static void process_die (struct die_info
 
 static char *dwarf2_linkage_name (struct die_info *, struct dwarf2_cu *);
 
+static char *dwarf2_physname (struct die_info *, struct dwarf2_cu *);
+
 static char *dwarf2_canonicalize_name (char *, struct dwarf2_cu *,
 				       struct obstack *);
 
@@ -2577,6 +2594,44 @@ pdi_needs_namespace (enum dwarf_tag tag)
     }
 }
 
+/* Determine whether DIE needs to have the name of the scope prepended
+   to the name listed in the die.  */
+
+static int
+die_needs_namespace (struct die_info *die, struct dwarf2_cu *cu)
+{
+  switch (die->tag)
+    {
+    case DW_TAG_namespace:
+    case DW_TAG_typedef:
+    case DW_TAG_class_type:
+    case DW_TAG_interface_type:
+    case DW_TAG_structure_type:
+    case DW_TAG_union_type:
+    case DW_TAG_enumeration_type:
+    case DW_TAG_enumerator:
+    case DW_TAG_subprogram:
+    case DW_TAG_member:
+      return 1;
+
+    case DW_TAG_variable:
+      {
+	struct attribute *attr;
+	attr = dwarf2_attr (die, DW_AT_specification, cu);
+	if (attr)
+	  return 1;
+	attr = dwarf2_attr (die, DW_AT_external, cu);
+	if (attr == NULL && die->parent->tag != DW_TAG_namespace)
+	  return 0;
+	return 1;
+      }
+      break;
+
+    default:
+      return 0;
+    }
+}
+
 /* Read a partial die corresponding to a namespace; also, add a symbol
    corresponding to that namespace to the symbol table.  NAMESPACE is
    the name of the enclosing namespace.  */
@@ -8914,6 +8969,96 @@ determine_prefix (struct die_info *die, 
       }
 }
 
+/* Determines the prefix for a symbol's physname. Unlike determine_prefix,
+   this method does not simply look at the DIE's immediate parent.
+   It will compute the symbol's physname by scanning through all parent
+   DIEs until it gets to the compilation unit's DIE. */
+
+static char *
+physname_prefix (struct die_info *die, struct dwarf2_cu *cu)
+{
+  long length;
+  struct ui_file *buf;
+  struct die_info *d, *spec_die;
+  struct dwarf2_cu *spec_cu;
+  char *name;
+
+  /* Construct a stack containing all of the DIE's parents.  Caution
+     must be observed for dealing with DW_AT_specification. */
+  spec_cu = cu;
+  spec_die = die_specification (die, &spec_cu);
+  if (spec_die != NULL)
+    d = spec_die->parent;
+  else
+    d = die->parent;
+  while (d != NULL && d->tag != DW_TAG_compile_unit)
+    {
+      struct attribute *attr;
+
+      spec_die = die_specification (d, &spec_cu);
+      if (spec_die != NULL)
+	d = spec_die;
+
+      VEC_quick_push (die_info_p, die_list, d);
+      d = d->parent;
+    }
+
+  /* Now pop all the elements, printing their names as we go */
+  buf = mem_fileopen ();
+  while (!VEC_empty (die_info_p, die_list))
+    {
+      d = VEC_pop (die_info_p, die_list);
+      physname_prefix_1 (buf, d, cu);
+
+      if (!VEC_empty (die_info_p, die_list))
+	{
+	  if (cu->language == language_cplus)
+	    fputs_unfiltered ("::", buf);
+	  else
+	    fputs_unfiltered (".", buf);
+	}
+    }
+
+  name = ui_file_obsavestring (buf, &cu->objfile->objfile_obstack, &length);
+  ui_file_delete (buf);
+  return name;
+}
+
+static void
+physname_prefix_1 (struct ui_file *buf, struct die_info *die,
+		   struct dwarf2_cu *cu)
+{
+  const char *name = NULL;
+  gdb_assert (buf != NULL);
+
+  if (die != NULL)
+    {
+      switch (die->tag)
+	{
+	case DW_TAG_namespace:
+	  name = dwarf2_name (die, cu);
+	  if (name == NULL)
+	    name = "(anonymous namespace)";
+	  break;
+
+	case DW_TAG_class_type:
+	case DW_TAG_structure_type:
+	case DW_TAG_union_type:
+	case DW_TAG_enumeration_type:
+	case DW_TAG_interface_type:
+	case DW_TAG_subprogram:
+	  name = dwarf2_name (die, cu);
+	  break;
+
+	default:
+	  break;
+	}
+    }
+
+  if (name != NULL)
+    fputs_unfiltered (name, buf);
+}
+
 /* Return a newly-allocated string formed by concatenating PREFIX and
    SUFFIX with appropriate separator.  If PREFIX or SUFFIX is NULL or empty, then
    simply copy the SUFFIX or PREFIX, respectively.  If OBS is non-null,
@@ -8976,6 +9121,80 @@ dwarf2_linkage_name (struct die_info *di
   return dwarf2_name (die, cu);
 }
 
+/* Construct a physname for the given DIE in CU. */
+
+static char *
+dwarf2_physname (struct die_info *die, struct dwarf2_cu *cu)
+{
+  struct attribute *attr;
+  char *name;
+
+  name = dwarf2_name (die, cu);
+
+  /* These are the only languages we know how to qualify names in.  */
+  if (cu->language != language_cplus
+      && cu->language != language_java)
+    return name;
+
+  if (die_needs_namespace (die, cu))
+    {
+      long length;
+      char *prefix;
+      struct ui_file *buf;
+
+      prefix = physname_prefix (die, cu);
+      buf = mem_fileopen ();
+      if (*prefix != '\0')
+	{
+	  char *prefixed_name = typename_concat (NULL, prefix, name, cu);
+	  fputs_unfiltered (prefixed_name, buf);
+	  xfree (prefixed_name);
+	}
+      else
+	fputs_unfiltered (name ? name : "", buf);
+
+      /* For Java and C++ methods, append formal parameter type
+	 information. */
+      if ((cu->language == language_cplus || cu->language == language_java)
+	  && die->tag == DW_TAG_subprogram)
+	{
+	  struct type *type = read_type_die (die, cu);
+
+	  c_type_print_args (type, buf, 0);
+
+	  if (cu->language == language_java)
+	    {
+	      /* For java, we must append the return type to method names. */
+	      if (die->tag == DW_TAG_subprogram)
+		java_print_type (TYPE_TARGET_TYPE (type), "", buf, 0, 0);
+	    }
+	  else if (cu->language == language_cplus)
+	    {
+	      /* c_type_print_args adds argument types, but it does
+		 not add any necessary "const". */
+	      if (TYPE_NFIELDS (type) > 0 && TYPE_FIELD_ARTIFICIAL (type, 0)
+		  && TYPE_CONST (TYPE_TARGET_TYPE (TYPE_FIELD_TYPE (type, 0))))
+		fputs_unfiltered (" const", buf);
+	    }
+	}
+
+      name = ui_file_obsavestring (buf, &cu->objfile->objfile_obstack,
+				   &length);
+      ui_file_delete (buf);
+
+      if (cu->language == language_cplus)
+	{
+	  char *cname
+	    = dwarf2_canonicalize_name (name, cu,
+					&cu->objfile->objfile_obstack);
+	  if (cname != NULL)
+	    name = cname;
+	}
+    }
+
+  return name;
+}
+
 /* Get name of a die, return NULL if not found.  */
 
 static char *
Index: typeprint.h
===================================================================
RCS file: /cvs/src/src/gdb/typeprint.h,v
retrieving revision 1.8
diff -u -p -r1.8 typeprint.h
--- typeprint.h	3 Jan 2009 05:57:53 -0000	1.8
+++ typeprint.h	31 Aug 2009 20:37:23 -0000
@@ -26,4 +26,6 @@ void print_type_scalar (struct type * ty
 
 void c_type_print_varspec_suffix (struct type *, struct ui_file *, int,
 				  int, int);
+
+void c_type_print_args (struct type *, struct ui_file *, int);
 #endif
Index: ui-file.h
===================================================================
RCS file: /cvs/src/src/gdb/ui-file.h,v
retrieving revision 1.10
diff -u -p -r1.10 ui-file.h
--- ui-file.h	14 Aug 2009 00:32:32 -0000	1.10
+++ ui-file.h	31 Aug 2009 20:37:23 -0000
@@ -19,6 +19,7 @@
 #ifndef UI_FILE_H
 #define UI_FILE_H
 
+struct obstack;
 struct ui_file;
 
 /* Create a generic ui_file object with null methods. */
@@ -77,7 +78,10 @@ extern void ui_file_put (struct ui_file 
    minus that appended NUL. */
 extern char *ui_file_xstrdup (struct ui_file *file, long *length);
 
-
+/* Similar to ui_file_xstrdup, but return a new string allocated on
+   OBSTACK.  */
+extern char *ui_file_obsavestring (struct ui_file *file,
+				   struct obstack *obstack, long *length);
 
 extern long ui_file_read (struct ui_file *file, char *buf, long length_buf);
 
Index: c-typeprint.c
===================================================================
RCS file: /cvs/src/src/gdb/c-typeprint.c,v
retrieving revision 1.47
diff -u -p -r1.47 c-typeprint.c
--- c-typeprint.c	2 Jul 2009 12:57:14 -0000	1.47
+++ c-typeprint.c	31 Aug 2009 20:37:23 -0000
@@ -40,8 +40,6 @@ static void cp_type_print_method_args (s
 				       char *varstring, int staticp,
 				       struct ui_file *stream);
 
-static void c_type_print_args (struct type *, struct ui_file *);
-
 static void cp_type_print_derivation_info (struct ui_file *, struct type *);
 
 static void c_type_print_varspec_prefix (struct type *, struct ui_file *, int,
@@ -355,10 +353,12 @@ c_type_print_modifier (struct type *type
 
 /* Print out the arguments of TYPE, which should have TYPE_CODE_METHOD
    or TYPE_CODE_FUNC, to STREAM.  Artificial arguments, such as "this"
-   in non-static methods, are displayed.  */
+   in non-static methods, are displayed if SHOW_ARTIFICIAL is
+   non-zero.  */
 
-static void
-c_type_print_args (struct type *type, struct ui_file *stream)
+void
+c_type_print_args (struct type *type, struct ui_file *stream,
+		   int show_artificial)
 {
   int i, len;
   struct field *args;
@@ -370,6 +370,9 @@ c_type_print_args (struct type *type, st
 
   for (i = 0; i < TYPE_NFIELDS (type); i++)
     {
+      if (TYPE_FIELD_ARTIFICIAL (type, i) && !show_artificial)
+	continue;
+
       if (printed_any)
 	{
 	  fprintf_filtered (stream, ", ");
@@ -593,7 +596,7 @@ c_type_print_varspec_suffix (struct type
       if (passed_a_ptr)
 	fprintf_filtered (stream, ")");
       if (!demangled_args)
-	c_type_print_args (type, stream);
+	c_type_print_args (type, stream, 1);
       c_type_print_varspec_suffix (TYPE_TARGET_TYPE (type), stream, show,
 				   passed_a_ptr, 0);
       break;
Index: ui-file.c
===================================================================
RCS file: /cvs/src/src/gdb/ui-file.c,v
retrieving revision 1.18
diff -u -p -r1.18 ui-file.c
--- ui-file.c	14 Aug 2009 00:32:32 -0000	1.18
+++ ui-file.c	31 Aug 2009 20:37:23 -0000
@@ -22,6 +22,7 @@
 
 #include "defs.h"
 #include "ui-file.h"
+#include "gdb_obstack.h"
 #include "gdb_string.h"
 
 #include <errno.h>
@@ -297,6 +298,23 @@ ui_file_xstrdup (struct ui_file *file, l
     *length = acc.length;
   return acc.buffer;
 }
+
+static void
+do_ui_file_obsavestring (void *context, const char *buffer, long length)
+{
+  struct obstack *obstack = (struct obstack *) context;
+  obstack_grow (obstack, buffer, length);
+}
+
+char *
+ui_file_obsavestring (struct ui_file *file, struct obstack *obstack,
+		      long *length)
+{
+  ui_file_put (file, do_ui_file_obsavestring, obstack);
+  *length = obstack_object_size (obstack);
+  obstack_1grow (obstack, '\0');
+  return obstack_finish (obstack);
+}
 \f
 /* A pure memory based ``struct ui_file'' that can be used an output
    buffer. The buffers accumulated contents are available via

Re: [RFA] dwarf2_physname

[Michael Eager]

Keith Seitz wrote:
> 
> To start, I would like to submit a patch which is the beginnings of 
> removing GDB's use of the DWARF2 quasi-attribute 
> DW_AT_MIPS_linkage_name. Instead of using this attribute, GDB will start 
> generating physical names itself.

Does this mean that (eventually) the DW_AT_MIPS_linkage_name attribute
will not be needed by GDB?

Coincidentally, the Dwarf Committee just approved adding DW_AT_linkage_name
to DWARF Version 4.  The semantics are substantially the same as 
DW_AT_MIPS_linkage_name.
(http://www.dwarfstd.org/ShowIssue.php?issue=090715.1&type=closed3)
The semantics are substantially the same as DW_AT_MIPS_linkage_name.

There was a significant amount of discussion about whether this was
really needed.  There were a couple examples where it might provide
information which was not otherwise available or where it compensated
for linkers which didn't support weak externs.

-- 
Michael Eager	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077

Re: [RFA] dwarf2_physname

[Keith Seitz]

On 08/31/2009 03:55 PM, Michael Eager wrote:

> Does this mean that (eventually) the DW_AT_MIPS_linkage_name attribute
> will not be needed by GDB?

That is exactly what it is intended to do. MIPS_linkage_name is not 
needed in any case I've been able to invent on my 
archer-keiths-expr-cumulative branch, and that branch has MUCH tougher 
C++ tests than FSF gdb does.

> There was a significant amount of discussion about whether this was
> really needed. There were a couple examples where it might provide
> information which was not otherwise available or where it compensated
> for linkers which didn't support weak externs.

This is the first I've heard of this -- thank you for pointing it out. 
My cursory reading of the proposal leaves me torn about whether this 
really changes anything. I've clearly had better results WITHOUT 
DW_AT_MIPS_linkage_name than with it, but I can imagine how having 
DW_AT_linkage_name for certain special situations might be useful.

Perhaps this might just be the beginning of using DW_AT_linkage_name for 
these "special" situations, as opposed to assuming that every object has 
a DW_AT_linkage_name. I don't know. I guess time will tell.

Keith

Re: [RFA] dwarf2_physname

[Michael Eager]

Keith Seitz wrote:
> On 08/31/2009 03:55 PM, Michael Eager wrote:
> 
>> Does this mean that (eventually) the DW_AT_MIPS_linkage_name attribute
>> will not be needed by GDB?
> 
> That is exactly what it is intended to do. MIPS_linkage_name is not 
> needed in any case I've been able to invent on my 
> archer-keiths-expr-cumulative branch, and that branch has MUCH tougher 
> C++ tests than FSF gdb does.
> 
>> There was a significant amount of discussion about whether this was
>> really needed. There were a couple examples where it might provide
>> information which was not otherwise available or where it compensated
>> for linkers which didn't support weak externs.
> 
> This is the first I've heard of this -- thank you for pointing it out. 
> My cursory reading of the proposal leaves me torn about whether this 
> really changes anything. I've clearly had better results WITHOUT 
> DW_AT_MIPS_linkage_name than with it, but I can imagine how having 
> DW_AT_linkage_name for certain special situations might be useful.

The proposal essentially renames DW_AT_MIPS_linkage_name, without
saying very much about how it might be used or why some producers
or consumers might find it necessary.

> Perhaps this might just be the beginning of using DW_AT_linkage_name for 
> these "special" situations, as opposed to assuming that every object has 
> a DW_AT_linkage_name. I don't know. I guess time will tell.

I've long had the opinion that DW_AT_MIPS_linkage_name was unnecessary.
As far as I could tell, standard DWARF contains all of the information
which was obtained by parsing the linkage name.  The few special cases
seemed more contrived than persuasive.

-- 
Michael Eager	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077

Re: [RFA] dwarf2_physname

[Michael Eager]

Tom Tromey wrote:
>>>>>> "Michael" == Michael Eager <eager@eagercon.com> writes:
> 
> Michael> (http://www.dwarfstd.org/ShowIssue.php?issue=090715.1&type=closed3)
> 
> Michael> There was a significant amount of discussion about whether this was
> Michael> really needed.  There were a couple examples where it might provide
> Michael> information which was not otherwise available or where it compensated
> Michael> for linkers which didn't support weak externs.
> 
> I read that page and didn't see these examples.  Do you know where we
> could find them?

They were raised as part of the DWARF Committee discussions:

1)  Finding the address of the in-charge constructor when evaluating
     "new foo()".
2)  Look up address of Fortran common block.
3)  Demangled to display name of instance of overloaded function.
4)  Compensate for compilers which do not generate adequate DWARF.
5)  Demangle to find fully qualified name for type.
6)  Avoid unresolved external references for symbols which are removed
     by the linker, if weak externs are not supported.
7)  Optimize access with Apple's scheme where DWARF is left in the .o's.

Not all of these examples were compelling, nor did it appear that
DW_AT_linkage_name was the only means for obtaining the desired information.
Some were clearly Quality of Implementation issues, like 4.

> It seems to me that if we can do the job without DW_AT_*linkage_name,
> then all other things being equal that is what we ought to do,
> considering that the new attribute is optional.  In practice this only
> means that we must determine whether all other things actually are
> equal.

Almost all attributes in DWARF are optional.  Leaving out useful ones
only makes it difficult to debug your program.  :-(

-- 
Michael Eager	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077

Re: [RFA] dwarf2_physname

[Tom Tromey]

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> In an effort to help maintainers assess the code in smaller chunks
Keith> without introducing new, temporary test suite failures, I'm going to
Keith> try something "different" -- this patch, and the follow-ons, will not
Keith> actually do anything until a final patch turns it all on. My hope is
Keith> that this will be minimally intrusive to GDB's code base, especially
Keith> given all the talk of a new release.

This seems fine to me, though as a procedural issue I would suggest that
you not commit anything until the whole series is approved.  (The
alternative seems to be committing unused code, which I think we should
not do in general.)

If this is a hardship for some reason we can discuss it.


I haven't read your patch in depth yet but I wanted to mention a couple
of things.  I'll look at the rest soon.

Keith> 	Based on work from Daniel Jacobowitz  <dan@codesourcery.com>:
Keith> 	* typeprint.h (c_type_print_args): Declare.
Keith> 	* ui-file.h (ui_file_obsavestring): Declare.
Keith> 	* c-typeprint.c (c_type_print_args): Add to global namespace.
Keith> 	Add "show_articifical" parameter and only print them when
Keith> 	requested.
Keith> 	* ui-file.c (do_ui_file_obsavestring): New function.
Keith> 	(ui_file_obsavestring): New function.

All of this part looked ok.

Keith> +/* A vector used during linkage name generation.  */
Keith> +typedef struct die_info *die_info_p;
Keith> +DEF_VEC_P (die_info_p);
Keith> +static VEC(die_info_p) *die_list;

I wonder if a global is really necessary.
I didn't check.

Keith> +      VEC_quick_push (die_info_p, die_list, d);

AFAICT the vector is only added to with VEC_quick_push.
This will die, though, if there is insufficient space in the vec.

So, something must be missing here -- either an explicit initialization
of the vec, or this should use VEC_safe_push.

Tom

Re: [RFA] dwarf2_physname

[Keith Seitz]

On 09/01/2009 02:26 PM, Tom Tromey wrote:
> If this is a hardship for some reason we can discuss it.

I don't have a problem *not* committing it, especially with all the 7.0 
talk about. But it will be difficult for me to keep posting patches 
which refine earlier (uncommitted patches). Well, actually, it's not as 
much a problem for me as it is for maintainers. Stuff will be really 
hard to read/diff/try. [I think (TM).]

But it's your (collective) call. Tell me what you'd like me to do, and 
I'll do it.

> Keith>  +/* A vector used during linkage name generation.  */
> Keith>  +typedef struct die_info *die_info_p;
> Keith>  +DEF_VEC_P (die_info_p);
> Keith>  +static VEC(die_info_p) *die_list;
>
> I wonder if a global is really necessary.
> I didn't check.

The global is there to avoid having to reallocate the thing every time 
dwarf2_physname is called. In a large C++ application, that could be 
MANY MANY MANY times.

> So, something must be missing here -- either an explicit initialization
> of the vec, or this should use VEC_safe_push.

Yes, there *is* an explicit initialization in _initialize_dwarf2_read, 
but I did not include it in this revision of the patch, since it would 
not actually be used *until* I "flipped" the switch. In other words, you 
would have seen the initialization when the final patch was submitted.

I apologize, I know this is going to be very confusing. Think of this 
first patch like a "big picture" design overview. Subsequent patches 
will build on/refine this until everything is ready to go in and be 
"turned on."

The problem is that the necessary changes are /so/ pervasive that in 
order to avoid test suite failures, I would have to submit a several 
thousand line patch which touches dozens of files, and I was hoping to 
avoid doing that kind of "patch dumping" ('cause I don't like it when 
that is done).

Again, let me know how you would like me to proceed, and I will happily 
try to accommodate you. The burden of review is on you (maintainers). I 
want to do what I can to make it as easy as possible for you.

Keith

Re: [RFA] dwarf2_physname

[Daniel Jacobowitz]

On Mon, Aug 31, 2009 at 04:19:02PM -0700, Keith Seitz wrote:
> On 08/31/2009 03:55 PM, Michael Eager wrote:
> 
> >Does this mean that (eventually) the DW_AT_MIPS_linkage_name attribute
> >will not be needed by GDB?
> 
> That is exactly what it is intended to do. MIPS_linkage_name is not
> needed in any case I've been able to invent on my
> archer-keiths-expr-cumulative branch, and that branch has MUCH
> tougher C++ tests than FSF gdb does.

I assume this patch is a distant descendant of the one I sent you a
while back.  Is its goal to create a useful name for each symbol, or
to match the one produced by some other source - probably the
demangler?  What sort of template cases have you looked at?

As you know, I spent several some time trying to do without
DW_AT_MIPS_linkage_name.  My main goals were cleanliness and
compatibility with a non-GCC compiler (ARM RealView).  I came across a
lot of cases where the output from neither GCC nor any other compiler
I could get my hands on made sense without a linkage name in the debug
info - and I couldn't find a way to get sensible representation in
DWARF, either.

Here's one case I remember having trouble with.

template<char *S> int f()
{
  return S[0];
}

char Foo[3];
char Bar[3];

int main()
{
  return f<Foo>() + f<Bar>();
}

0000000000000000 W int f<&Bar>()
0000000000000000 W int f<&Foo>()

 <1><31>: Abbrev Number: 2 (DW_TAG_subprogram)
    <32>   DW_AT_external    : 1
    <33>   DW_AT_name        : (indirect string, offset: 0x47): f<((char*)(& Foo))>
    <37>   DW_AT_decl_file   : 1
    <38>   DW_AT_decl_line   : 1
    <39>   DW_AT_MIPS_linkage_name: (indirect string, offset: 0x1f): _Z1fIXadL_Z3FooEEEiv
    <3d>   DW_AT_type        : <0x55>
    <41>   DW_AT_low_pc      : 0x0
    <49>   DW_AT_high_pc     : 0x10
    <51>   DW_AT_frame_base  : 0x0      (location list)

You have to represent the name "Foo" in the debug info - the address
isn't enough, there could be other symbols apparently at the same
address.  The name this older GCC emits is not useful.  Most other
compilers just punted.  While hopefully Dodji has made HEAD GCC do
something sensible now, we have to handle all those field compilers :-)

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] dwarf2_physname

[Daniel Jacobowitz]

On Tue, Sep 01, 2009 at 06:11:22PM -0400, Daniel Jacobowitz wrote:
> I assume this patch is a distant descendant of the one I sent you a
> while back.  Is its goal to create a useful name for each symbol, or
> to match the one produced by some other source - probably the
> demangler?  What sort of template cases have you looked at?

I guess the big picture is that I want to know how these pieces fit
together, before trying to figure out if each individual piece is
right.  It's a hack (IMO!) that we need to generate physnames - but
it may be the only practical path which is how I ended up there myself
:-)

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] dwarf2_physname

[Michael Eager]

Daniel Jacobowitz wrote:
> On Mon, Aug 31, 2009 at 04:19:02PM -0700, Keith Seitz wrote:
>> On 08/31/2009 03:55 PM, Michael Eager wrote:
>>
>>> Does this mean that (eventually) the DW_AT_MIPS_linkage_name attribute
>>> will not be needed by GDB?
>> That is exactly what it is intended to do. MIPS_linkage_name is not
>> needed in any case I've been able to invent on my
>> archer-keiths-expr-cumulative branch, and that branch has MUCH
>> tougher C++ tests than FSF gdb does.
> 
> I assume this patch is a distant descendant of the one I sent you a
> while back.  Is its goal to create a useful name for each symbol, or
> to match the one produced by some other source - probably the
> demangler?  What sort of template cases have you looked at?

I'd be concerned about trying to embed name mangling into gdb.
Not all compilers (or versions) use the same scheme.

> As you know, I spent several some time trying to do without
> DW_AT_MIPS_linkage_name.  My main goals were cleanliness and
> compatibility with a non-GCC compiler (ARM RealView).  I came across a
> lot of cases where the output from neither GCC nor any other compiler
> I could get my hands on made sense without a linkage name in the debug
> info - and I couldn't find a way to get sensible representation in
> DWARF, either.
> 
> Here's one case I remember having trouble with.
> 
> template<char *S> int f()
> {
>   return S[0];
> }
> 
> char Foo[3];
> char Bar[3];
> 
> int main()
> {
>   return f<Foo>() + f<Bar>();
> }
> 
> 0000000000000000 W int f<&Bar>()
> 0000000000000000 W int f<&Foo>()
> 
>  <1><31>: Abbrev Number: 2 (DW_TAG_subprogram)
>     <32>   DW_AT_external    : 1
>     <33>   DW_AT_name        : (indirect string, offset: 0x47): f<((char*)(& Foo))>
>     <37>   DW_AT_decl_file   : 1
>     <38>   DW_AT_decl_line   : 1
>     <39>   DW_AT_MIPS_linkage_name: (indirect string, offset: 0x1f): _Z1fIXadL_Z3FooEEEiv
>     <3d>   DW_AT_type        : <0x55>
>     <41>   DW_AT_low_pc      : 0x0
>     <49>   DW_AT_high_pc     : 0x10
>     <51>   DW_AT_frame_base  : 0x0      (location list)
> 
> You have to represent the name "Foo" in the debug info - the address
> isn't enough, there could be other symbols apparently at the same
> address.  The name this older GCC emits is not useful.  Most other
> compilers just punted.  While hopefully Dodji has made HEAD GCC do
> something sensible now, we have to handle all those field compilers :-)

I think that this is a problem with how the template instantiation is
described.

 From DWARF 3, Sec. 3.3.7:

   1. Each formal parameterized type declaration appearing in the template
      definition is represented by a debugging information entry with the tag
      DW_TAG_template_type_parameter. Each such entry has a DW_AT_name attribute,
      whose value is a null-terminated string containing the name of the formal
      type parameter as it appears in the source program. The template type
      parameter entry also has a DW_AT_type attribute describing the actual
      type by which the formal is replaced for this instantiation. All template
      type parameter entries should appear before the entries describing the
      instantiated formal parameters to the function.

I think that there should be something like
    DW_TAG_template_value_parameter
      DW_AT_name : Foo
      DW_AT_type : <int *>
as a child of  of f<Foo>.

(I'm pretty rusty on how template functions are described, and, unfortunately,
the DWARF Standard doesn't have any examples.)

My feeling is that in this case, DW_AT_MIPS_linkage_name compensates
for the compiler not generating more complete DWARF.  If it did generate
DW_TAG_template_value_parameter, there would be no need for DW_AT_MIPS_linkage_name.


-- 
Michael Eager	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077

Re: [RFA] dwarf2_physname

[Keith Seitz]

On 09/01/2009 03:11 PM, Daniel Jacobowitz wrote:
> I assume this patch is a distant descendant of the one I sent you a
> while back.  Is its goal to create a useful name for each symbol, or
> to match the one produced by some other source - probably the
> demangler?  What sort of template cases have you looked at?

Yes, this is based on the patches you sent me a long, long time ago. The 
goal is to generate a physname that is unique; it may match the output 
of the demangler, but that is not a requirement. It is supposed to 
construct valid, fully-qualified names, i.e, you won't see "std::string" 
as a physname. It's really "std::basic_string<blah blah blah>".

The template cases I've looked at are checked-in on the 
archer-keiths-realcpp-test branch. I've just committed the latest and 
greatest version of realcpp.{exp,cc} here: http://tinyurl.com/lkaadh 
(git.sourceware.org -- sorry for the tinyurl thing, git URL names are 
loooooooong).

Unfortunately, I cannot speak to other compilers, only gcc.

> Here's one case I remember having trouble with.
>
> template<char *S>  int f()
> {
>    return S[0];
> }
>
> char Foo[3];
> char Bar[3];
>
> int main()
> {
>    return f<Foo>() + f<Bar>();
> }

What do you want to do with this? Print it and break/list it?

On my archer-keiths-expr-cumulative branch (which does NOT use 
linkage_name):

(gdb) info func f
All functions matching regular expression "f":

File a.cc:
int f<(char*)(&Bar)>();
int f<(char*)(&Foo)>();

Non-debugging symbols:
0x08048420  frame_dummy
0x08048490  __libc_csu_fini
0x0804852c  _fini
(gdb) p f<(char*)(&Foo)>
$1 = {int (void)} 0x8048465 <f<(char*)(&Foo)>()>
(gdb) list 'f<(char*)(&Foo)>'
1	template<char *S> int f()
2	{
3	  return S[0];
4	}
5	
6	char Foo[3];
7	char Bar[3];
8	
9	int main()
10	{
(gdb) b 'f<(char*)(&Foo)>'
Breakpoint 1 at 0x8048468: file a.cc, line 3.
(gdb) r
Starting program: /home/keiths/tmp/a

Breakpoint 1, f<(char*)(&Foo)> () at a.cc:3
3	  return S[0];
(gdb) bt
#0  f<(char*)(&Foo)> () at a.cc:3
#1  0x08048453 in main () at a.cc:11
(gdb)

I know that "f<(char*)(&Foo)>" is ugly, but that is the name that gcc 
gives us (DW_AT_name for this DIE), so we have to use it. [I am assuming 
we could get this cleaned up/"fixed" in gcc.]

On FSF gdb HEAD, you can do something similar with "int f<&Foo>" (yes, 
you *have* to add the return type) instead of "f<(char*)(&Foo)>"...

Keith

Re: [RFA] dwarf2_physname

[Daniel Jacobowitz]

On Tue, Sep 01, 2009 at 04:17:56PM -0700, Michael Eager wrote:
> I think that this is a problem with how the template instantiation is
> described.

For your example DWARF:

> I think that there should be something like
>    DW_TAG_template_value_parameter
>      DW_AT_name : Foo
>      DW_AT_type : <int *>
> as a child of  of f<Foo>.

I believe DW_AT_name would be S here and not Foo - the name by which
the argument is known inside the instantiation.  It's representing Foo
that's a problem; it has to go in the DW_AT_const_value attribute.

Here's a trickier example:

template<const char *S> int f()
{
  return S[0];
}

template <int I> struct S {};

template<int I, const char *STR> int g(S<I + 1 + sizeof(STR)>*)
{
  return 0;
}

char Foo[3];
char Bar[3];

int main()
{
  return f<Foo>() + f<Bar>() + g<5, Foo>(0);
}

0000000000000000 W _Z1gILi5EXadL_Z3FooEEEiP1SIXplplT_Li1EszT0_EE
0000000000000000 W int g<5, &Foo>(S<((5)+(1))+(sizeof (&Foo))>*)

A patch that can't handle this may still be an improvement - but I'd
like to know how we're approaching this problem, and whether (A) the
lack of compilers generating adequate data, and (B) to the best of my
knowledge, the lack of DWARF constructs for such expressions, is
going to interfere with debugging of heavily templated programs.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] dwarf2_physname

[Daniel Jacobowitz]

On Tue, Sep 01, 2009 at 04:26:46PM -0700, Keith Seitz wrote:
> On 09/01/2009 03:11 PM, Daniel Jacobowitz wrote:
> >I assume this patch is a distant descendant of the one I sent you a
> >while back.  Is its goal to create a useful name for each symbol, or
> >to match the one produced by some other source - probably the
> >demangler?  What sort of template cases have you looked at?
> 
> Yes, this is based on the patches you sent me a long, long time ago.
> The goal is to generate a physname that is unique; it may match the
> output of the demangler, but that is not a requirement. It is
> supposed to construct valid, fully-qualified names, i.e, you won't
> see "std::string" as a physname. It's really "std::basic_string<blah
> blah blah>".

OK.  That'd be a big improvement over the last state of my tree; in
order to call a memeber function, we had to match the ELF symbol
exactly or GDB would bogusly report it as "inlined".

If uniqueness is all that's required, maybe there's some less
expensive representation than a string encoding of the arguments?

> 
> The template cases I've looked at are checked-in on the
> archer-keiths-realcpp-test branch. I've just committed the latest and
> greatest version of realcpp.{exp,cc} here: http://tinyurl.com/lkaadh
> (git.sourceware.org -- sorry for the tinyurl thing, git URL names are
> loooooooong).
> 
> Unfortunately, I cannot speak to other compilers, only gcc.

I'm happy to test with RealView.  I might be able to test with ICC
also, but I haven't tried that in forever.  I believe it's freely
available.

> What do you want to do with this? Print it and break/list it?

Yes, but especially call it.  Even more so if it were a member
function - sorry, my C++ is rusty, but hopefully one or the other of
us could construct such an example.  I'm at my two-a-day limit on
templates.

> I know that "f<(char*)(&Foo)>" is ugly, but that is the name that gcc
> gives us (DW_AT_name for this DIE), so we have to use it. [I am
> assuming we could get this cleaned up/"fixed" in gcc.]

We could, as long as we do something not horribly nonsensical with the
names there now - which are a bit random.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] dwarf2_physname

[Michael Eager]

Daniel Jacobowitz wrote:
> On Tue, Sep 01, 2009 at 04:17:56PM -0700, Michael Eager wrote:
>> I think that this is a problem with how the template instantiation is
>> described.
> 
> For your example DWARF:
> 
>> I think that there should be something like
>>    DW_TAG_template_value_parameter
>>      DW_AT_name : Foo
>>      DW_AT_type : <int *>
>> as a child of  of f<Foo>.
> 
> I believe DW_AT_name would be S here and not Foo - the name by which
> the argument is known inside the instantiation.  It's representing Foo
> that's a problem; it has to go in the DW_AT_const_value attribute.

Yeah.  I was trying to figure out whether it was the formal or
actual parameter.  I wanted "Foo" to appear somewhere.  I'm not
sure DW_AT_const_value is really appropriate either.

> Here's a trickier example:
> 
> template<const char *S> int f()
> {
>   return S[0];
> }
> 
> template <int I> struct S {};
> 
> template<int I, const char *STR> int g(S<I + 1 + sizeof(STR)>*)
> {
>   return 0;
> }
> 
> char Foo[3];
> char Bar[3];
> 
> int main()
> {
>   return f<Foo>() + f<Bar>() + g<5, Foo>(0);
> }
> 
> 0000000000000000 W _Z1gILi5EXadL_Z3FooEEEiP1SIXplplT_Li1EszT0_EE
> 0000000000000000 W int g<5, &Foo>(S<((5)+(1))+(sizeof (&Foo))>*)
> 
> A patch that can't handle this may still be an improvement - but I'd
> like to know how we're approaching this problem, and whether (A) the
> lack of compilers generating adequate data, and (B) to the best of my
> knowledge, the lack of DWARF constructs for such expressions, is
> going to interfere with debugging of heavily templated programs.

The DWARF Committee had some discussions about how templates
are described in DWARF.  Only the instantiations are described, not
the declaration.  DWARF tries (and sometimes succeeds) in providing
enough info for the instantiation that a debugger can infer either
the template declaration or how it was instantiated.  This breaks
down in several areas.

There was brief discussion about replacing the current description of
templates.  The existing scheme was developed in the early 90's with
little real-world experience.  C++ has evolved significantly since
then.  On the other hand, I'd be concerned that we would come up with
a new description which requires a significant effort to implement and
that neither compiler writers nor debugger developers want to adopt it.
Sort of like the resistance to the transition from DWARF 1 to DWARF 2.

In any case, changes in template descriptions will not be considered
for DWARF 4, but possibly for DWARF 5.  Suggestions always welcome.


-- 
Michael Eager	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077

Re: [RFA] dwarf2_physname

[Keith Seitz]

On 09/01/2009 04:42 PM, Daniel Jacobowitz wrote:
> OK.  That'd be a big improvement over the last state of my tree; in
> order to call a memeber function, we had to match the ELF symbol
> exactly or GDB would bogusly report it as "inlined".

A lot has changed since the patch you sent me. I'd be surprised if you 
even recognized your patch inside mine. :-)

> If uniqueness is all that's required, maybe there's some less
> expensive representation than a string encoding of the arguments?

That makes searching for a symbol a lot more difficult and slower. The 
point of this is to reduce all classes/templates/methods/etc names to 
their most basic source code representations to make searching easier 
and more reliable. The sad fact of the matter is, linkage_names are NOT 
reliable.

> I'm happy to test with RealView.  I might be able to test with ICC
> also, but I haven't tried that in forever.  I believe it's freely
> available.

I've spent the last 1.5 weeks investigating proprietary compilers, and I 
can only assume that no one seriously ever runs gdb's test suite on 
either RealView or ICC. CVS HEAD shows almost 1,000 test failures on 
each compiler.

I could not even get dejagnu to cope with non-gcc compilers without 
resorting to writing wrappers around ICC and RealView to translate gcc 
command-line options into something appropriate for those other 
proprietary compilers.

But here's what I've discovered.

ARM RealView: My expr-cumulative branch introduces three regression 
failures -- two of them in C++ tests. However, it also allows RealView 
to pass over two hundred previously failing or erroring C++ tests. I can 
provide diffs. On my "realcpp" test, RealView fails almost all of the 
template tests. This is because RealView relies on DW_AT_template_* 
attributes which gdb does not yet know about. [This will likely be my 
next task.]

ICC: This compiler really relies on DW_AT_MIPS_linkage_name because its 
DWARF output is *seriously* messed up. As expected on expr-cumulative, 
ICC fails approximately 60 additional C++ tests. The main problem is not 
that it doesn't really pass the test, but because of ICC's foobar'd 
DWARF output, the user has to do things a little differently, e.g., 
"break foo::foo" doesn't work, but "break foo::foo()" does.

As I mentioned, this is because ICC plays fast and loose with the DWARF 
standard. Take, for example, the simple case of a constructor, 
foo::foo(void), ICC will output (this is the first DIE after the comp 
unit DIE):

  <1><e0>: Abbrev Number: 3 (DW_TAG_subprogram)
     <e1>   DW_AT_decl_line   : 4
     <e2>   DW_AT_decl_column : 3
     <e3>   DW_AT_decl_file   : 1
     <e4>   DW_AT_specification: <0x11a>
     <e8>   DW_AT_name        : foo
     <ec>   DW_AT_MIPS_linkage_name: _ZN3fooC1Ev
     <f8>   DW_AT_low_pc      : 0x8048434
     <fc>   DW_AT_high_pc     : 0x804843a
     <100>   DW_AT_external    : 1
  <2><101>: Abbrev Number: 4 (DW_TAG_formal_parameter)
     <102>   DW_AT_type        : <0x147>
     <106>   DW_AT_artificial  : 1
     <107>   DW_AT_name        : this
     <10c>   DW_AT_location    : 2 byte block: 75 8      (DW_OP_breg5: 8)

So far so good. However, if you look at the actual definition of the 
class foo (<11a>, the specification listed in the DIE above):

  <1><110>: Abbrev Number: 5 (DW_TAG_class_type)
     <111>   DW_AT_decl_line   : 1
     <112>   DW_AT_decl_column : 7
     <113>   DW_AT_decl_file   : 1
     <114>   DW_AT_accessibility: 1      (public)
     <115>   DW_AT_byte_size   : 1
     <116>   DW_AT_name        : foo
  <2><11a>: Abbrev Number: 6 (DW_TAG_subprogram)
     <11b>   DW_AT_decl_line   : 4
     <11c>   DW_AT_decl_column : 3
     <11d>   DW_AT_decl_file   : 1
     <11e>   DW_AT_accessibility: 1      (public)
     <11f>   DW_AT_type        : <0x13f>
     <123>   DW_AT_prototyped  : 1
     <124>   DW_AT_name        : foo
     <128>   DW_AT_MIPS_linkage_name: _ZN3fooC1Ev
  <3><134>: Abbrev Number: 7 (DW_TAG_formal_parameter)
     <135>   DW_AT_type        : <0x147>
     <139>   DW_AT_name        : $01

Here you can see that DIE <134> is NOT marked artificial. Additionally, 
its DW_AT_name conflicts with the one previously listed, which I think 
is playing pretty loose with the standard (and certainly makes life 
difficult for the debugger). I did not even bother with my realcpp tests 
on this compiler.

>> I know that "f<(char*)(&Foo)>" is ugly, but that is the name that gcc
>> gives us (DW_AT_name for this DIE), so we have to use it. [I am
>> assuming we could get this cleaned up/"fixed" in gcc.]

> We could, as long as we do something not horribly nonsensical with the
> names there now - which are a bit random.

Dodji is already working on this and a bunch of other DWARF 
cleanups/additions, especially in the area of template debug info. 
AFAICT his interpretation of the DWARF spec reads more like mine (and 
ARM's) than Intel's. O:-)

Is there anything else you want me to investigate? [PLEASE don't say 
another proprietary compiler!!]

Keith

Re: [RFA] dwarf2_physname

[Daniel Jacobowitz]

On Mon, Sep 14, 2009 at 11:39:23AM -0700, Keith Seitz wrote:
> I've spent the last 1.5 weeks investigating proprietary compilers,
> and I can only assume that no one seriously ever runs gdb's test
> suite on either RealView or ICC. CVS HEAD shows almost 1,000 test
> failures on each compiler.

Hi Keith,

I'm really sorry about the rathole.  I expected you'd just ask me to
do it... you're correct that you are going to get a lot of failures
with RealView.  I have patches for some of the worst, both in
testsuite patterns and in GDB proper.  Part of the reason I haven't
posted them yet, though, is this exact issue - one of the patches in
our tree for RealView support is the stone-age ancestor of your
patches :-)

ICC, though, I'm pretty surprised; we get bug reports about people
using ICC once in a while, and I thought someone had tested it...

Now that you've gone to the trouble, though, let's make the most of
your work.  Could I persuade you to put the wrappers you needed and
where to get appropriate compilers onto the GDB wiki?

> ARM RealView: My expr-cumulative branch introduces three regression
> failures -- two of them in C++ tests. However, it also allows
> RealView to pass over two hundred previously failing or erroring C++
> tests. I can provide diffs. On my "realcpp" test, RealView fails
> almost all of the template tests. This is because RealView relies on
> DW_AT_template_* attributes which gdb does not yet know about. [This
> will likely be my next task.]

This is excellent news.

Some things will blow up in new and exciting ways when we start
relying on the template arguments emitted by the compiler; they're
incomplete for some of the nasty examples I tried.  But if we can
debug normal-ish templates, we've made great progress.

>  <2><101>: Abbrev Number: 4 (DW_TAG_formal_parameter)
>     <102>   DW_AT_type        : <0x147>
>     <106>   DW_AT_artificial  : 1
>     <107>   DW_AT_name        : this
>     <10c>   DW_AT_location    : 2 byte block: 75 8      (DW_OP_breg5: 8)

>  <3><134>: Abbrev Number: 7 (DW_TAG_formal_parameter)
>     <135>   DW_AT_type        : <0x147>
>     <139>   DW_AT_name        : $01

> Here you can see that DIE <134> is NOT marked artificial.
> Additionally, its DW_AT_name conflicts with the one previously
> listed, which I think is playing pretty loose with the standard (and
> certainly makes life difficult for the debugger). I did not even
> bother with my realcpp tests on this compiler.

Ouch.  Though, why does this make life difficult for the debugger?
The problem of THIS not being artificial I've seen before - I think
RealView has the same problem; I may have a patch to work around it -
but I'm not sure why we care about the name of the parameter in the
declaration.

> Is there anything else you want me to investigate?

No, I'm happy.  Is this patch still current?  If so, I'll review it
tonight.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] dwarf2_physname

[Keith Seitz]

On 09/14/2009 03:47 PM, Daniel Jacobowitz wrote:
> I'm really sorry about the rathole.  I expected you'd just ask me to
> do it... you're correct that you are going to get a lot of failures
> with RealView.  I have patches for some of the worst, both in
> testsuite patterns and in GDB proper.  Part of the reason I haven't
> posted them yet, though, is this exact issue - one of the patches in
> our tree for RealView support is the stone-age ancestor of your
> patches :-)
>
> ICC, though, I'm pretty surprised; we get bug reports about people
> using ICC once in a while, and I thought someone had tested it...
>
> Now that you've gone to the trouble, though, let's make the most of
> your work.  Could I persuade you to put the wrappers you needed and
> where to get appropriate compilers onto the GDB wiki?

Yeah, I'll work on that immediately. Of course, that means I have to 
take out all the, umm, colorful language that litters my scripts. O:-)

> Ouch.  Though, why does this make life difficult for the debugger?
> The problem of THIS not being artificial I've seen before - I think
> RealView has the same problem; I may have a patch to work around it -
> but I'm not sure why we care about the name of the parameter in the
> declaration.

The problem is that now we are appending the arguments to the "linkage 
name" that we are computing. So when the dwarf reader sees this, it 
outputs "foo::foo (foo *)" instead of "foo::foo ()". This messes up 
symbol searching in some very interesting ways.

>> Is there anything else you want me to investigate?
>
> No, I'm happy.  Is this patch still current?  If so, I'll review it
> tonight.

Yes, the patch still applies to CVS HEAD.

Let me know if you need anything else.

Keith

Re: [RFA] dwarf2_physname

[Daniel Jacobowitz]

On Mon, Sep 14, 2009 at 04:56:32PM -0700, Keith Seitz wrote:
> >Ouch.  Though, why does this make life difficult for the debugger?
> >The problem of THIS not being artificial I've seen before - I think
> >RealView has the same problem; I may have a patch to work around it -
> >but I'm not sure why we care about the name of the parameter in the
> >declaration.
> 
> The problem is that now we are appending the arguments to the
> "linkage name" that we are computing. So when the dwarf reader sees
> this, it outputs "foo::foo (foo *)" instead of "foo::foo ()". This
> messes up symbol searching in some very interesting ways.

Oh, I see.  Is there any other differentiation in the ICC output
between the two relevant cases?  Namely bar and baz in:

class foo {
  void bar();
  static void baz(foo*);
};

From discussions on the dwarf-discuss list, I've gathered that pretty
much every debugger is full of compiler-specific quirking.  In
practice, to provide a good user experience, we have to interpret the
DWARF in our best understanding of each compiler's intentions rather
than just through the standard.

Of course, that relies on someone wanting to do the work for ICC support.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] dwarf2_physname

[Keith Seitz]

On 09/15/2009 06:32 AM, Daniel Jacobowitz wrote:

> Oh, I see.  Is there any other differentiation in the ICC output
> between the two relevant cases?  Namely bar and baz in:
>
> class foo {
>    void bar();
>    static void baz(foo*);
> };

In this case, the DWARF output looks pretty much as expected. There is 
only one DIE defining foo::baz, and that is in the class definition. One 
quirk: the parameter's DW_AT_name is "$01".

I played with this a bit, and it seems that only ctors and dtors have 
DIEs outside the class definition (ignoring "normal" C functions). 
However, in every case I tried, ICC does not output DW_AT_artificial for 
ANY class method's first parameter. Does ICC not use hidden parameter 
passing to implement method calls? It seems odd that it would do this 
for ctors/dtors and no other methods.

In any case, once again, DW_AT_name for method formal parameters is also 
generated ("$02", "$03", etc). It seems that the only time I see a 
useful DW_AT_name for anything function-like is for the ctor/dtor (all 
parameters, not just hidden "this") and C functions.

[Anecdote: Every time I run readelf -w on an ICC-generated object, it 
complains of "Bogus end-of-siblings marker" -- perhaps there are some 
bytes in the debuginfo (which readelf/gdb cannot read/interpret) which 
helps mitigate some of these problems?]

>  From discussions on the dwarf-discuss list, I've gathered that pretty
> much every debugger is full of compiler-specific quirking.  In
> practice, to provide a good user experience, we have to interpret the
> DWARF in our best understanding of each compiler's intentions rather
> than just through the standard.

Sure, that doesn't really surprise me. But these three quirks 
(conflicting DIEs for same object, missing DW_AT_artificial, nonsensical 
DW_AT_name) would seem truly unnecessary and fairly trivial to fix.

> Of course, that relies on someone wanting to do the work for ICC support.

It may just be useful to provide (yet another) switch to turn on 
DW_AT_MIPS_linkage_name processing (and disable the proposed physname 
work). We could do this switch manually (set dwarf-linkage-name on?) 
and/or by producer (turn it on for ICC by default).

Keith

Re: [RFA] dwarf2_physname

[Daniel Jacobowitz]

On Tue, Sep 15, 2009 at 08:53:12AM -0700, Keith Seitz wrote:
> I played with this a bit, and it seems that only ctors and dtors have
> DIEs outside the class definition (ignoring "normal" C functions).
> However, in every case I tried, ICC does not output DW_AT_artificial
> for ANY class method's first parameter. Does ICC not use hidden
> parameter passing to implement method calls? It seems odd that it
> would do this for ctors/dtors and no other methods.

From the standard:

===
If the member function entry describes a non-static member function,
then that entry has a DW_AT_object_pointer attribute whose value is a
reference to the formal parameter entry that corresponds to the object
for which the function is called. The name attribute of that formal
parameter is defined by the current language (for example, this for
C++ or self for Objective-C and some other languages). That parameter
also has a DW_AT_artificial attribute whose value is true.
Conversely, if the member function entry describes a static member
function, the entry does not have a DW_AT_object_pointer attribute.
===

So, if icc is failing to mark things artificial, does it make up for
it by having DW_AT_object_pointer?  I don't believe GCC generates that
attribtue (guess it should!).

> It may just be useful to provide (yet another) switch to turn on
> DW_AT_MIPS_linkage_name processing (and disable the proposed physname
> work). We could do this switch manually (set dwarf-linkage-name on?)
> and/or by producer (turn it on for ICC by default).

This is an area where, pain or not, I'd really prefer to have only one
way to do things.

I looked over your patch again.  I have no objection and Tom's already
approved it upthread; I think you're good to go.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] dwarf2_physname

[Keith Seitz]

On 09/15/2009 09:11 AM, Daniel Jacobowitz wrote:
> So, if icc is failing to mark things artificial, does it make up for
> it by having DW_AT_object_pointer?  I don't believe GCC generates that
> attribtue (guess it should!).

Unfortunately, ICC does not emit DW_AT_object_pointer. It outputs only 
what I've previously posted.

> This is an area where, pain or not, I'd really prefer to have only one
> way to do things.

I can certainly understand that (and I agree): I was just throwing that 
out as an option. Afterall, there is a bigger picture (for you) to 
consider -- I'm just a linux-centered, who-cares-about-anything-else 
hacker. O:-)

> I looked over your patch again.  I have no objection and Tom's already
> approved it upthread; I think you're good to go.

Tom has suggested to me to conform and start posting bigger bits of the 
patches. Unless I hear anything to the contrary, I'll consider this a 
general approval of the approach, and I will attempt to submit any 
outstanding isolated patches before posting the whole (ginormous) patch 
which implements everything.

I've also added a page to the wiki about running the test suite on 
non-gcc compilers (specifically ICC and RealView). It's not complete in 
the sense that I think some careful Dejagnu configury and/or compiler 
wrapper trickery might produce better (or more meaningful) results, but 
it's a start.

For example, I completely ignore the whole multilib issue. In my case, I 
was really just interested in the C++ DWARF implementations of these 
compilers, and I believe the setup is sufficient for this purpose. In a 
real world scenario, it probably isn't. I hope that people who 
remember/know more about this stuff add their two bits to the page.

Keith

Re: [RFA] dwarf2_physname

[Tom Tromey]

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> Tom has suggested to me to conform and start posting bigger bits of
Keith> the patches. Unless I hear anything to the contrary, I'll consider
Keith> this a general approval of the approach, and I will attempt to submit
Keith> any outstanding isolated patches before posting the whole (ginormous)
Keith> patch which implements everything.

What people have done here in the past is just send a big patch series
1..N, with "patch" 0/N explaining the overview to any potential readers.
Sometimes folks also post the mega-patch as well.

I think this has worked pretty well in practice.

I also think it is ok if one patch is a lot bigger than the others.  It
doesn't always make sense to try to split things up too much, it just
depends on the problem.

Tom