This is the mail archive of the gdb-patches@sources.redhat.com mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: RFA: Correct field names for class methods


Daniel Jacobowitz writes:
 > Right now, the "name" field of a method is not always reliable.  There's
 > special-cased code all over the linespec, function calling, struct lookup,
 > etc. code to handle this.  Among the problems:
 > 
 >   - v3 stabs emit __comp_ctor etc.
 >   - v2 stabs emit constructors and destructors in the same fieldlist
 >   - v2 stabs emit mangled operator names
 > 
 > This patch does not remove any of the special cases, but renders it
 > all obsolete, to be cleaned up in a forthcoming patch.  This will greatly
 > simplify the revised method printing code that I'm working on, a necessary
 > cleanup as we move towards namespace support.  It pushes the stabs special
 > casing back into stabs related code as much as practical.
 > 
 > Elena, this does a bit of its ugliness in read_member_functions, so I'd like
 > your approval before I go ahead with it.


See below. I have some questions and some general comments on the
structure of the patch.

 > 
 > Comments, anyone?
 > 
 > -- 
 > Daniel Jacobowitz
 > MontaVista Software                         Debian GNU/Linux Developer
 > 
 > 2002-08-26  Daniel Jacobowitz  <drow@mvista.com>
 > 
 > 	* gdbtypes.c (check_stub_method): Make static.
 > 	(update_method_from_physname, check_stub_method_group)
 > 	(find_last_component, class_name_from_physname)
 > 	(method_name_from_physname): New functions.
 > 	* gdbtypes.h: Update prototypes.
 > 
 > 	* stabsread.c: Include "cp-abi.h".

Makefile?

 > 	(read_member_functions): Correct method names for operators
 > 	and v3 constructors/destructors.  Separate v2 constructors and
 > 	destructors.
 > 
 > 	* cp-valprint.c (cp_print_class_method): Call
 > 	check_stub_method_group instead of check_stub_method.
 > 	* p-valprint.c (pascal_object_print_class_method): Likewise.
 > 	* valops.c (search_struct_method): Likewise.
 > 	(find_method_list, value_struct_elt_for_reference): Likewise.
 > 
 > Index: cp-valprint.c
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/cp-valprint.c,v
 > retrieving revision 1.13
 > diff -u -p -r1.13 cp-valprint.c
 > --- cp-valprint.c	29 Jul 2002 22:55:26 -0000	1.13
 > +++ cp-valprint.c	27 Aug 2002 02:02:55 -0000
 > @@ -97,13 +97,12 @@ cp_print_class_method (char *valaddr,
 >  	  f = TYPE_FN_FIELDLIST1 (domain, i);
 >  	  len2 = TYPE_FN_FIELDLIST_LENGTH (domain, i);
 >  
 > +	  check_stub_method_group (domain, i);
 >  	  for (j = 0; j < len2; j++)
 >  	    {
 >  	      QUIT;

Did you mean to leave this QUIT?

 >  	      if (TYPE_FN_FIELD_VOFFSET (f, j) == offset)
 >  		{
 > -		  if (TYPE_FN_FIELD_STUB (f, j))
 > -		    check_stub_method (domain, i, j);
 >  		  kind = "virtual ";
 >  		  goto common;
 >  		}
 > @@ -129,15 +128,11 @@ cp_print_class_method (char *valaddr,
 >  	  f = TYPE_FN_FIELDLIST1 (domain, i);
 >  	  len2 = TYPE_FN_FIELDLIST_LENGTH (domain, i);
 >  
 > +	  check_stub_method_group (f, j);
 >  	  for (j = 0; j < len2; j++)
 >  	    {
 > -	      QUIT;
 > -	      if (TYPE_FN_FIELD_STUB (f, j))
 > -		check_stub_method (domain, i, j);
 >  	      if (STREQ (SYMBOL_NAME (sym), TYPE_FN_FIELD_PHYSNAME (f, j)))
 > -		{
 > -		  goto common;
 > -		}
 > +		goto common;
 >  	    }
 >  	}
 >      }
 > Index: gdbtypes.c
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/gdbtypes.c,v
 > retrieving revision 1.56
 > diff -u -p -r1.56 gdbtypes.c
 > --- gdbtypes.c	20 Aug 2002 19:57:32 -0000	1.56
 > +++ gdbtypes.c	27 Aug 2002 02:02:55 -0000
 > @@ -1672,7 +1672,7 @@ safe_parse_type (char *p, int length)
 >     which info used to be in the stab's but was removed to hack back
 >     the space required for them.  */
 >  
 > -void
 > +static void
 >  check_stub_method (struct type *type, int method_id, int signature_id)
 >  {
 >    struct fn_field *f;
 > @@ -1781,6 +1781,54 @@ check_stub_method (struct type *type, in
 >    xfree (demangled_name);
 >  }
 >  

Could you add some comments about what this function does?
Actually, since it is used only in stabsread.c, why not move it there?
I think a lot of this c++ specific stuff really doesn't belong in gdbtypes.c.
You can move all the new functions, except for check_stub_method_group to
stabsread.c. They are called from there only.


 > +void
 > +update_method_name_from_physname (char **old_name, char *physname)
 > +{
 > +  char *method_name;
 > +
 > +  method_name = method_name_from_physname (physname);
 > +
 > +  if (method_name == NULL)
 > +    error ("bad physname %s\n", physname);
 > +
 > +  if (strcmp (*old_name, method_name) != 0)
 > +    *old_name = method_name;
 > +  else
 > +    xfree (method_name);
 > +}
 > +

Could you add some comments here too?

 > +void
 > +check_stub_method_group (struct type *type, int method_id)
 > +{
 > +  int len = TYPE_FN_FIELDLIST_LENGTH (type, method_id);
 > +  struct fn_field *f = TYPE_FN_FIELDLIST1 (type, method_id);
 > +  int j, found_stub;
 > +
 > +  for (j = 0; j < len; j++)
 > +    if (TYPE_FN_FIELD_STUB (f, j))
 > +      {
 > +	found_stub = 1;
 > +	check_stub_method (type, method_id, j);
 > +      }
 > +
 > +  /* We can handle only the v2 case here, because the only stabs with
 > +     incorrect field names in v3 are constructors and destructors, and
 > +     the only stub methods in v3 are static methods.  */
 > +  if (found_stub && strncmp (TYPE_FN_FIELD_PHYSNAME (f, 0), "_Z", 2) != 0)
 > +    {
 > +      int ret;
 > +      char dem_opname[256];
 > +
 > +      ret = cplus_demangle_opname (TYPE_FN_FIELDLIST_NAME (type, method_id),
 > +				   dem_opname, DMGL_ANSI);
 > +      if (!ret)
 > +	ret = cplus_demangle_opname (TYPE_FN_FIELDLIST_NAME (type, method_id),
 > +				     dem_opname, 0);
 > +      if (ret)
 > +	TYPE_FN_FIELDLIST_NAME (type, method_id) = xstrdup (dem_opname);
 > +    }
 > +}
 > +
 >  const struct cplus_struct_type cplus_struct_default;
 >  
 >  void
 > @@ -3435,6 +3483,118 @@ build_gdbtypes (void)
 >  	       "__bfd_vma", (struct objfile *) NULL);
 >  }
 >  
 > +/* Find the last component of the demangled C++ name NAME.
 > +
 > +   This function return a pointer to the first colon before the
 > +   last component, or NULL if the name had only one component.  */
 > +
 > +static const char *
 > +find_last_component (const char *name)
 > +{
 > +  const char *p;
 > +  int depth;
 > +
 > +  /* Functions can have local classes, so we need to find the
 > +     beginning of the last argument list, not the end of the first
 > +     one.  */
 > +  p = name + strlen (name) - 1;
 > +  while (p > name && *p != ')')
 > +    p--;
 > +
 > +  if (p == name)
 > +    return NULL;
 > +
 > +  /* P now points at the `)' at the end of the argument list.  Walk
 > +     back to the beginning.  */
 > +  p--;
 > +  depth = 1;
 > +  while (p > name && depth > 0)
 > +    {
 > +      if (*p == '<' || *p == '(')
 > +	depth--;
 > +      else if (*p == '>' || *p == ')')
 > +	depth++;
 > +      p--;
 > +    }
 > +
 > +  if (p == name)
 > +    return NULL;
 > +
 > +  while (p > name && *p != ':')
 > +    p--;
 > +
 > +  if (p == name || p == name + 1 || p[-1] != ':')
 > +    return NULL;
 > +
 > +  return p - 1;
 > +}
 > +

Did you forget to post a piece of the patch? I don't see the function
below being called anywhere.


 > +/* Return the name of the class containing method PHYSNAME.  */
 > +
 > +char *
 > +class_name_from_physname (const char *physname)
 > +{
 > +  char *ret = NULL;
 > +  const char *end;
 > +  int depth = 0;
 > +  char *demangled_name = cplus_demangle (physname, DMGL_ANSI);
 > +
 > +  if (demangled_name == NULL)
 > +    return NULL;
 > +
 > +  end = find_last_component (demangled_name);
 > +  if (end != NULL)
 > +    {
 > +      ret = xmalloc (end - demangled_name + 1);
 > +      memcpy (ret, demangled_name, end - demangled_name);
 > +      ret[end - demangled_name] = '\0';
 > +    }
 > +
 > +  xfree (demangled_name);
 > +  return ret;
 > +}
 > +
 > +/* Return the name of the method whose linkage name is PHYSNAME.  */
 > +
 > +char *
 > +method_name_from_physname (const char *physname)
 > +{
 > +  char *ret = NULL;
 > +  const char *end;
 > +  int depth = 0;
 > +  char *demangled_name = cplus_demangle (physname, DMGL_ANSI);
 > +
 > +  if (demangled_name == NULL)
 > +    return NULL;
 > +
 > +  end = find_last_component (demangled_name);
 > +  if (end != NULL)
 > +    {
 > +      char *args;
 > +      int len;
 > +
 > +      /* Skip "::".  */
 > +      end = end + 2;
 > +
 > +      /* Find the argument list, if any.  */
 > +      args = strchr (end, '(');
 > +      if (args == NULL)
 > +	len = strlen (end + 2);
 > +      else
 > +	{
 > +	  args --;
 > +	  while (*args == ' ')
 > +	    args --;
 > +	  len = args - end + 1;
 > +	}
 > +      ret = xmalloc (len + 1);
 > +      memcpy (ret, end, len);
 > +      ret[len] = 0;
 > +    }
 > +
 > +  xfree (demangled_name);
 > +  return ret;
 > +}
 >  
 >  extern void _initialize_gdbtypes (void);
 >  void
 > Index: gdbtypes.h
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/gdbtypes.h,v
 > retrieving revision 1.35
 > diff -u -p -r1.35 gdbtypes.h
 > --- gdbtypes.h	10 Aug 2002 05:12:40 -0000	1.35
 > +++ gdbtypes.h	27 Aug 2002 02:02:56 -0000
 > @@ -1124,11 +1124,17 @@ extern struct type *check_typedef (struc
 >  
 >  #define CHECK_TYPEDEF(TYPE) (TYPE) = check_typedef (TYPE)
 >  
 > -extern void check_stub_method (struct type *, int, int);
 > +extern void check_stub_method_group (struct type *, int);
 > +
 > +extern void update_method_name_from_physname (char **old_name, char *physname);
 >  
 >  extern struct type *lookup_primitive_typename (char *);
 >  
 >  extern char *gdb_mangle_name (struct type *, int, int);
 > +
 > +extern char *class_name_from_physname (const char *physname);
 > +
 > +extern char *method_name_from_physname (const char *physname);
 >  
 >  extern struct type *builtin_type (char **);
 >  
 > Index: p-valprint.c
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/p-valprint.c,v
 > retrieving revision 1.13
 > diff -u -p -r1.13 p-valprint.c
 > --- p-valprint.c	19 Aug 2002 13:12:09 -0000	1.13
 > +++ p-valprint.c	27 Aug 2002 02:02:56 -0000
 > @@ -620,13 +620,11 @@ pascal_object_print_class_method (char *
 >  	  f = TYPE_FN_FIELDLIST1 (domain, i);
 >  	  len2 = TYPE_FN_FIELDLIST_LENGTH (domain, i);
 >  
 > +	  check_stub_method_group (domain, i);
 >  	  for (j = 0; j < len2; j++)
 >  	    {
 > -	      QUIT;
 >  	      if (TYPE_FN_FIELD_VOFFSET (f, j) == offset)
 >  		{
 > -		  if (TYPE_FN_FIELD_STUB (f, j))
 > -		    check_stub_method (domain, i, j);
 >  		  kind = "virtual ";
 >  		  goto common;
 >  		}
 > @@ -646,15 +644,11 @@ pascal_object_print_class_method (char *
 >  	  f = TYPE_FN_FIELDLIST1 (domain, i);
 >  	  len2 = TYPE_FN_FIELDLIST_LENGTH (domain, i);
 >  
 > +	  check_stub_method_group (domain, i);
 >  	  for (j = 0; j < len2; j++)
 >  	    {
 > -	      QUIT;
 > -	      if (TYPE_FN_FIELD_STUB (f, j))
 > -		check_stub_method (domain, i, j);
 >  	      if (STREQ (SYMBOL_NAME (sym), TYPE_FN_FIELD_PHYSNAME (f, j)))
 > -		{
 > -		  goto common;
 > -		}
 > +		goto common;
 >  	    }
 >  	}
 >      }
 > Index: stabsread.c
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/stabsread.c,v
 > retrieving revision 1.38
 > diff -u -p -r1.38 stabsread.c
 > --- stabsread.c	1 Aug 2002 17:18:32 -0000	1.38
 > +++ stabsread.c	27 Aug 2002 02:02:57 -0000
 > @@ -44,6 +44,7 @@
 >  #include "demangle.h"
 >  #include "language.h"
 >  #include "doublest.h"
 > +#include "cp-abi.h"
 >  
 >  #include <ctype.h>
 >  
 > @@ -3377,6 +3378,127 @@ read_member_functions (struct field_info
 >  	}
 >        else
 >  	{
 > +	  int has_stub = 0;
 > +	  int has_nondestructor = 0, has_destructor = 0;
 > +	  int is_v3 = 0;
 > +	  struct next_fnfield *tmp_sublist;
 > +
 > +	  /* Various versions of GCC emit various mostly-useless
 > +	     strings in the name field for special member functions.
 > +	     For some methods, we have a complete physname, so we
 > +	     could fix this up now.  For stub methods we will do this
 > +	     later, in check_stub_method_group.  For non-stub methods,
 > +	     we have no clear way to know whether or not the physname
 > +	     is correct; g++ 2.95.x only reliably emits full physnames
 > +	     for operators which do not start with the method name
 > +	     (constructors, destructors fall in this category; there
 > +	     may be others?).  But for other methods it may or may not
 > +	     emit a full physname depending on the platform (if
 > +	     CPLUS_MARKER can be `$' or `.', it will use minimal debug
 > +	     information, but not otherwise).
 > +
 > +	     Rather than dealing with this, we take a different approach.
 > +	     For v3 mangled names, we can use the full physname; for v2,
 > +	     we use cplus_demangle_opname, because the only interesting
 > +	     names are all operators.  Skip if any method in the group
 > +	     is a stub, to prevent our fouling up the workings of
 > +	     gdb_mangle_name.
 > +
 > +	     Another thing that we need to clean up here: GCC 2.95.x
 > +	     puts constructors and destructors in the same group.  We
 > +	     need to split this into two groups.  */
 > +


Since you are at it, could you insert examples of mangled names,
physnames, etc in the comment?


 > +	  tmp_sublist = sublist;
 > +	  while (tmp_sublist != NULL)
 > +	    {
 > +	      if (tmp_sublist->fn_field.is_stub)
 > +		has_stub = 1;
 > +	      if (tmp_sublist->fn_field.physname[0] == '_'
 > +		  && tmp_sublist->fn_field.physname[1] == 'Z')
 > +		is_v3 = 1;
 > +
 > +	      if (is_destructor_name (tmp_sublist->fn_field.physname))
 > +		has_destructor++;
 > +	      else

 > +		has_nondestructor++;

Dumb question, but, what is a nondestructor? Maybe a different variable
name would be a bit more enlightening.

 > +
 > +	      tmp_sublist = tmp_sublist->next;
 > +	    }
 > +
 > +	  if (has_destructor && has_nondestructor)


Comments needed, for the c++ challenged. What does it mean to have both
a destructor and a nondestructor?

 > +	    {
 > +	      struct next_fnfieldlist *destr_fnlist;
 > +	      struct next_fnfield *last_sublist;
 > +
 > +	      /* Create a new fn_fieldlist for the destructors.  */;
 > +	      destr_fnlist = (struct next_fnfieldlist *)
 > +		xmalloc (sizeof (struct next_fnfieldlist));
 > +	      make_cleanup (xfree, destr_fnlist);
 > +	      memset (destr_fnlist, 0, sizeof (struct next_fnfieldlist));
 > +	      destr_fnlist->fn_fieldlist.name
 > +		= obconcat (&objfile->type_obstack, "", "~",
 > +			    new_fnlist->fn_fieldlist.name);
 > +
 > +	      destr_fnlist->fn_fieldlist.fn_fields = (struct fn_field *)
 > +		obstack_alloc (&objfile->type_obstack,
 > +			       sizeof (struct fn_field) * has_destructor);
 > +	      memset (destr_fnlist->fn_fieldlist.fn_fields, 0,
 > +		  sizeof (struct fn_field) * has_destructor);
 > +	      tmp_sublist = sublist;
 > +	      last_sublist = NULL;


 > +	      i = 0;

I am confused. Why is i always 0? Or it isn't?


Elena

 > +	      while (tmp_sublist != NULL)
 > +		{
 > +		  if (!is_destructor_name (tmp_sublist->fn_field.physname))
 > +		    {
 > +		      tmp_sublist = tmp_sublist->next;
 > +		      continue;
 > +		    }
 > +		  
 > +		  destr_fnlist->fn_fieldlist.fn_fields[i]
 > +		    = tmp_sublist->fn_field;
 > +		  if (last_sublist)
 > +		    last_sublist->next = tmp_sublist->next;
 > +		  else
 > +		    sublist = tmp_sublist->next;
 > +		  last_sublist = tmp_sublist;
 > +		  tmp_sublist = tmp_sublist->next;
 > +		}
 > +
 > +	      destr_fnlist->fn_fieldlist.length = has_destructor;
 > +	      destr_fnlist->next = fip->fnlist;
 > +	      fip->fnlist = destr_fnlist;
 > +	      nfn_fields++;
 > +	      total_length += has_destructor;
 > +	      length -= has_destructor;
 > +	    }
 > +	  else if (is_v3)
 > +	    {
 > +	      /* v3 mangling prevents the use of abbreviated physnames,
 > +		 so we can do this here.  There are stubbed methods in v3
 > +		 only:
 > +		 - in -gstabs instead of -gstabs+
 > +		 - or for static methods, who are output as a function type
 > +		   instead of a method type.  */
 > +
 > +	      update_method_name_from_physname (&new_fnlist->fn_fieldlist.name,
 > +						sublist->fn_field.physname);
 > +	    }
 > +	  else if (!has_stub)
 > +	    {
 > +	      char dem_opname[256];
 > +	      int ret;
 > +	      ret = cplus_demangle_opname (new_fnlist->fn_fieldlist.name,
 > +					      dem_opname, DMGL_ANSI);
 > +	      if (!ret)
 > +		ret = cplus_demangle_opname (new_fnlist->fn_fieldlist.name,
 > +					     dem_opname, 0);
 > +	      if (ret)
 > +		new_fnlist->fn_fieldlist.name
 > +		  = obsavestring (dem_opname, strlen (dem_opname),
 > +				  &objfile->type_obstack);
 > +	    }
 > +
 >  	  new_fnlist->fn_fieldlist.fn_fields = (struct fn_field *)
 >  	    obstack_alloc (&objfile->type_obstack,
 >  			   sizeof (struct fn_field) * length);
 > Index: valops.c
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/valops.c,v
 > retrieving revision 1.69
 > diff -u -p -r1.69 valops.c
 > --- valops.c	21 Aug 2002 17:24:31 -0000	1.69
 > +++ valops.c	27 Aug 2002 02:02:58 -0000
 > @@ -2302,12 +2302,11 @@ search_struct_method (char *name, struct
 >  	  struct fn_field *f = TYPE_FN_FIELDLIST1 (type, i);
 >  	  name_matched = 1;
 >  
 > +	  check_stub_method_group (type, i);
 >  	  if (j > 0 && args == 0)
 >  	    error ("cannot resolve overloaded method `%s': no arguments supplied", name);
 >  	  else if (j == 0 && args == 0)
 >  	    {
 > -	      if (TYPE_FN_FIELD_STUB (f, j))
 > -		check_stub_method (type, i, j);
 >  	      v = value_fn_field (arg1p, f, j, type, offset);
 >  	      if (v != NULL)
 >  		return v;
 > @@ -2315,8 +2314,6 @@ search_struct_method (char *name, struct
 >  	  else
 >  	    while (j >= 0)
 >  	      {
 > -		if (TYPE_FN_FIELD_STUB (f, j))
 > -		  check_stub_method (type, i, j);
 >  		if (!typecmp (TYPE_FN_FIELD_STATIC_P (f, j),
 >  			      TYPE_VARARGS (TYPE_FN_FIELD_TYPE (f, j)),
 >  			      TYPE_NFIELDS (TYPE_FN_FIELD_TYPE (f, j)),
 > @@ -2555,20 +2552,15 @@ find_method_list (struct value **argp, c
 >        char *fn_field_name = TYPE_FN_FIELDLIST_NAME (type, i);
 >        if (fn_field_name && (strcmp_iw (fn_field_name, method) == 0))
 >  	{
 > -	  /* Resolve any stub methods.  */
 >  	  int len = TYPE_FN_FIELDLIST_LENGTH (type, i);
 >  	  struct fn_field *f = TYPE_FN_FIELDLIST1 (type, i);
 > -	  int j;
 >  
 >  	  *num_fns = len;
 >  	  *basetype = type;
 >  	  *boffset = offset;
 >  
 > -	  for (j = 0; j < len; j++)
 > -	    {
 > -	      if (TYPE_FN_FIELD_STUB (f, j))
 > -		check_stub_method (type, i, j);
 > -	    }
 > +	  /* Resolve any stub methods.  */
 > +	  check_stub_method_group (type, i);
 >  
 >  	  return f;
 >  	}
 > @@ -3094,6 +3086,8 @@ value_struct_elt_for_reference (struct t
 >  	  int j = TYPE_FN_FIELDLIST_LENGTH (t, i);
 >  	  struct fn_field *f = TYPE_FN_FIELDLIST1 (t, i);
 >  
 > +	  check_stub_method_group (t, i);
 > +
 >  	  if (intype == 0 && j > 1)
 >  	    error ("non-unique member `%s' requires type instantiation", name);
 >  	  if (intype)
 > @@ -3107,8 +3101,6 @@ value_struct_elt_for_reference (struct t
 >  	  else
 >  	    j = 0;
 >  
 > -	  if (TYPE_FN_FIELD_STUB (f, j))
 > -	    check_stub_method (t, i, j);
 >  	  if (TYPE_FN_FIELD_VIRTUAL_P (f, j))
 >  	    {
 >  	      return value_from_longest


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]