Re: [PATCH] Start abstraction of C++ abi's

[Elena Zannoni <ezannoni@cygnus.com>]

Daniel Berlin writes:
 > 
 > This patch, plus the attached files, start the abstraction of the C++
 > ABI's.
 > 
 > I've started by replacing the simple things, and will incrementally
 > replace the more complex things, and the things that require real code
 > changes, as time goes on.
 > 
 > The cp-abi directory, and it's files, are attached in a gzipped tar file.
 > 
 > This fixes some problems with the new-abi already, like not being able
 > to set breakpoints on destructors (if you try it, you'll get:
 > 
 > (gdb) b foo::~foo
 > the class `foo' does not have destructor defined
 > Hint: try 'foo::~foo<TAB> or 'foo::~foo<ESC-?>
 > (Note leading single quote.)
 > (gdb)
 > )
 > 
 > I haven't added the method for detecting which C++ abi we are using,
 > so it simply defaults to the gnu v2 abi.
 > 
 > However, I have verified the gnu v3 abi parts work fine too (that's how i
 > know it fixed breakpoints on destructors).
 > 
 > This stuff touches a lot of files, but it's only removing code, or
 > changing a macro call to a function call (IE VTBL_PREFIX_P ->
 > vtbl_prefix_p, DESTRUCTOR_PREFIX_P -> destructor_prefix_p).
 > 
 > Who exactly do i need approval from to check this stuff in?
 > 
 > As I said, this is an incremental process. This is the minimum number
 > of changes necessary to start abstracting the simple things. There is
 > no way to make this patch smaller, without breaking gdb.
 > 
 > 
 > I need someone to look at the configure.in change, i'm not positive I
 > did it right.
 > 
 > --Dan
 > 

Dan, I don't see ChangeLogs entries in your patch.
Also one file in the cp-abi directory looks empty:

kwikemart.cygnus.com: 15 % tar tvf cp-abi.tar 
-rw-r--r-- dberlin/users  2551 2001-02-18 15:12 cp-abi.h
drwxr-xr-x dberlin/users     0 2001-02-18 15:45 cp-abi/
-rw-r--r-- dberlin/users  1967 2001-02-18 15:39 cp-abi/gnu-v3-abi.c
-rw-r--r-- dberlin/users     0 2001-02-18 15:37 cp-abi/cp-abi.c
-rw-r--r-- dberlin/users  2244 2001-02-18 14:58 cp-abi/gnu-v2-abi.c


Note that since your patch is incomplete, I cannot test it. So the 
'approved' is conditional.

Specific comments:



*************** c_type_print_base (struct type *type, st
*** 902,912 ****
  		{
  		  char *physname = TYPE_FN_FIELD_PHYSNAME (f, j);
  		  int is_full_physname_constructor =
! 		  ((physname[0] == '_' && physname[1] == '_'
! 		    && strchr ("0123456789Qt", physname[2]))
! 		   || STREQN (physname, "__ct__", 6)
! 		   || DESTRUCTOR_PREFIX_P (physname)
! 		   || STREQN (physname, "__dt__", 6));
  
  		  QUIT;
  		  if (TYPE_FN_FIELD_PROTECTED (f, j))
--- 903,912 ----
  		{
  		  char *physname = TYPE_FN_FIELD_PHYSNAME (f, j);
  		  int is_full_physname_constructor =
! 		   constructor_prefix_p (physname) 
! 		   || destructor_prefix_p (physname)
! 		   || STREQN (method_name, "~", 1);
! 		   
  
  		  QUIT;
  		  if (TYPE_FN_FIELD_PROTECTED (f, j))


I agree with Michael Chastain, I would prefer if there was one less
STREQN around, given that we understand what should be used in its
place, let's just use method_name[1] == '~'
 

Index: dbxread.c
===================================================================
RCS file: /cvs/src/src/gdb/dbxread.c,v
retrieving revision 1.12
diff -c -3 -p -r1.12 dbxread.c
*** dbxread.c	2001/01/19 14:53:44	1.12
--- dbxread.c	2001/02/18 20:42:42
***************
*** 58,63 ****
--- 58,64 ----
  #include "demangle.h"
  #include "language.h"		/* Needed inside partial-stab.h */
  #include "complaints.h"
+ #include "cp-abi.h"
  
  #include "aout/aout64.h"
  #include "aout/stab_gnu.h"	/* We always use GNU stabs, not native, now */
*************** record_minimal_symbol (char *name, CORE_
*** 514,520 ****
  	char *tempstring = name;
  	if (tempstring[0] == bfd_get_symbol_leading_char (objfile->obfd))
  	  ++tempstring;
! 	if (VTBL_PREFIX_P ((tempstring)))
  	  ms_type = mst_data;
        }
        section = SECT_OFF_DATA (objfile);
--- 515,521 ----
  	char *tempstring = name;
  	if (tempstring[0] == bfd_get_symbol_leading_char (objfile->obfd))
  	  ++tempstring;
! 	if (vtbl_prefix_p ((tempstring)))
  	  ms_type = mst_data;
        }
        section = SECT_OFF_DATA (objfile);

Approved. Do we really need the extra parenthesis around tempstring?

Index: linespec.c
===================================================================
RCS file: /cvs/src/src/gdb/linespec.c,v
retrieving revision 1.4
diff -c -3 -p -r1.4 linespec.c
*** linespec.c	2000/12/15 01:01:48	1.4
--- linespec.c	2001/02/18 20:42:42
***************
*** 28,34 ****
  #include "demangle.h"
  #include "value.h"
  #include "completer.h"
! 
  /* Prototype for one function in parser-defs.h,
     instead of including that entire file. */
  
--- 28,35 ----
  #include "demangle.h"
  #include "value.h"
  #include "completer.h"
! #include "gdb_regex.h"
! #include "cp-abi.h"
  /* Prototype for one function in parser-defs.h,
     instead of including that entire file. */
*************** find_methods (struct type *t, char *name
*** 119,126 ****
        int method_counter;
  
        /* FIXME: Shouldn't this just be CHECK_TYPEDEF (t)?  */
!       t = SYMBOL_TYPE (sym_class);
! 
        /* Loop over each method name.  At this level, all overloads of a name
           are counted as a single name.  There is an inner loop which loops over
           each overload.  */
--- 120,126 ----
        int method_counter;
  
        /* FIXME: Shouldn't this just be CHECK_TYPEDEF (t)?  */
!       CHECK_TYPEDEF (t);
        /* Loop over each method name.  At this level, all overloads of a name
           are counted as a single name.  There is an inner loop which loops over
           each overload.  */
*************** find_methods (struct type *t, char *name
*** 143,149 ****
  		method_name = dem_opname;
  	    }
  
! 	  if (STREQ (name, method_name))
  	    /* Find all the overloaded methods with that name.  */
  	    for (field_counter = TYPE_FN_FIELDLIST_LENGTH (t, method_counter) - 1;
  		 field_counter >= 0;
--- 143,149 ----
  		method_name = dem_opname;
  	    }
  
! 	  if (strcmp_iw (name, method_name) == 0)
  	    /* Find all the overloaded methods with that name.  */
  	    for (field_counter = TYPE_FN_FIELDLIST_LENGTH (t, method_counter) - 1;
  		 field_counter >= 0;
*************** find_methods (struct type *t, char *name
*** 167,177 ****
  		  }
  		else
  		  phys_name = TYPE_FN_FIELD_PHYSNAME (f, field_counter);
! 
  		/* Destructor is handled by caller, dont add it to the list */
! 		if (DESTRUCTOR_PREFIX_P (phys_name))
  		  continue;
- 
  		sym_arr[i1] = lookup_symbol (phys_name,
  					     NULL, VAR_NAMESPACE,
  					     (int *) NULL,
--- 167,176 ----
  		  }
  		else
  		  phys_name = TYPE_FN_FIELD_PHYSNAME (f, field_counter);
! 		
  		/* Destructor is handled by caller, dont add it to the list */
! 		if (destructor_prefix_p (phys_name) != 0)
  		  continue;
  		sym_arr[i1] = lookup_symbol (phys_name,
  					     NULL, VAR_NAMESPACE,
  					     (int *) NULL,
  

Could you please leave the blank lines that were there originally?
Could you remove/update the comment/FIXME before CHECK_TYPEDEF.
Do we need to include gdb_regexp.h? What needs it?


Index: symtab.c
===================================================================
RCS file: /cvs/src/src/gdb/symtab.c,v
retrieving revision 1.28
diff -c -3 -p -r1.28 symtab.c
*** symtab.c	2001/01/30 02:49:36	1.28
--- symtab.c	2001/02/18 20:42:43
***************
*** 44,50 ****
  #include "gdb_string.h"
  #include "gdb_stat.h"
  #include <ctype.h>
! 
  /* Prototype for one function in parser-defs.h,
     instead of including that entire file. */
  
--- 44,50 ----
  #include "gdb_string.h"
  #include "gdb_stat.h"
  #include <ctype.h>
! #include "cp-abi.h"
  /* Prototype for one function in parser-defs.h,
     instead of including that entire file. */
  
*************** gdb_mangle_name (struct type *type, int 
*** 287,293 ****
    int is_full_physname_constructor;
  
    int is_constructor;
!   int is_destructor = DESTRUCTOR_PREFIX_P (physname);
    /* Need a new type prefix.  */
    char *const_prefix = method->is_const ? "C" : "";
    char *volatile_prefix = method->is_volatile ? "V" : "";
--- 287,293 ----
    int is_full_physname_constructor;
  
    int is_constructor;
!   int is_destructor = destructor_prefix_p (physname);
    /* Need a new type prefix.  */
    char *const_prefix = method->is_const ? "C" : "";
    char *volatile_prefix = method->is_volatile ? "V" : "";
*************** gdb_mangle_name (struct type *type, int 
*** 297,306 ****
    if (OPNAME_PREFIX_P (field_name))
      return xstrdup (physname);
  
!   is_full_physname_constructor =
!     ((physname[0] == '_' && physname[1] == '_' &&
!       (isdigit (physname[2]) || physname[2] == 'Q' || physname[2] == 't'))
!      || (strncmp (physname, "__ct", 4) == 0));
  
    is_constructor =
      is_full_physname_constructor || (newname && STREQ (field_name, newname));
--- 297,303 ----
    if (OPNAME_PREFIX_P (field_name))
      return xstrdup (physname);
  
!   is_full_physname_constructor = constructor_prefix_p (physname);
  
    is_constructor =
      is_full_physname_constructor || (newname && STREQ (field_name, newname));


Approved.

Index: symtab.h
===================================================================
RCS file: /cvs/src/src/gdb/symtab.h,v
retrieving revision 1.18
diff -c -3 -p -r1.18 symtab.h
*** symtab.h	2001/02/08 06:03:54	1.18
--- symtab.h	2001/02/18 20:42:43
*************** struct partial_symtab
*** 1055,1074 ****
     '_vt$' is the old cfront-style vtables; '_VT$' is the new
     style, using thunks (where '$' is really CPLUS_MARKER). */
  
- #define VTBL_PREFIX_P(NAME) \
-   (((NAME)[0] == '_' \
-    && (((NAME)[1] == 'V' && (NAME)[2] == 'T') \
-        || ((NAME)[1] == 'v' && (NAME)[2] == 't')) \
-    && is_cplus_marker ((NAME)[3])) || ((NAME)[0]=='_' && (NAME)[1]=='_' \
-    && (NAME)[2]=='v' && (NAME)[3]=='t' && (NAME)[4]=='_'))
- 
- /* Macro that yields non-zero value iff NAME is the prefix for C++ destructor
-    names.  Note that this macro is g++ specific (FIXME).  */
- 
- #define DESTRUCTOR_PREFIX_P(NAME) \
-   ((NAME)[0] == '_' && is_cplus_marker ((NAME)[1]) && (NAME)[2] == '_')
  \f
- 
  /* External variables and functions for the objects described above. */
  
  /* This symtab variable specifies the current file for printing source lines */
--- 1055,1061 ----

OK.

Elena