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

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

[Michael Elizabeth Chastain]

Looks like a good idea to me.  "set demangle" currently shows five
different styles (gnu, lucid, arm, hp, edg).  We know that gnu encompasses
two different manglers by itself.  We gotta move these things into
virtual functions.

I won't comment on the directory structure (cp/cp-abi versus cp-abi);
maybe a maintainer would like to think about that.

Some specific comments:

> ! 		   constructor_prefix_p (physname) 
> ! 		   || destructor_prefix_p (physname)
> ! 		   || STREQN (method_name, "~", 1);

Does that lingering STREQN have to be there?

>   		  demangled_name =
>   		    cplus_demangle (mangled_name,
> ! 				    DMGL_ANSI | DMGL_PARAMS | DMGL_VERBOSE);

This looks like you mixed in a bit from a different patch -- there is no
DMGL_VERBOSE in the gdb cvs tree yet.

Beyond that I haven't proof-read the code yet.

Michael

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

[Daniel Berlin]

Michael Elizabeth Chastain <chastain@cygnus.com> writes:

> Looks like a good idea to me.  "set demangle" currently shows five
> different styles (gnu, lucid, arm, hp, edg).  We know that gnu encompasses
> two different manglers by itself.  We gotta move these things into
> virtual functions.

Well, actually, we only support 2 C++ abi's right now (4 if you really
want to try to call cfront an ABI), HP, GNUv2.

We can demangle functions for lucid/arm/edg, but we can't actually do
anything advanced with programs using those abi's (if they exist),
like virtual function calling/virtual base searching/etc.

> 
> I won't comment on the directory structure (cp/cp-abi versus cp-abi);
> maybe a maintainer would like to think about that.
> 
> Some specific comments:
> 
> > ! 		   constructor_prefix_p (physname) 
> > ! 		   || destructor_prefix_p (physname)
> > ! 		   || STREQN (method_name, "~", 1);
> 
> Does that lingering STREQN have to be there?

I can make it strncmp, or just check method_name[1], but it doesn't
make sense otherwise. It's just seeing if the first letter is a "~" in
the actual demangled method name, rather than the mangled name.

> 
> >   		  demangled_name =
> >   		    cplus_demangle (mangled_name,
> > ! 				    DMGL_ANSI | DMGL_PARAMS | DMGL_VERBOSE);
> 
> This looks like you mixed in a bit from a different patch -- there is no
> DMGL_VERBOSE in the gdb cvs tree yet.

Yup. It's from the demangle changes I haven't gotten approved yet.
I'll cut it out.
> 
> Beyond that I haven't proof-read the code yet.
> 
> Michael

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

[Michael Elizabeth Chastain]

Andrew Cagney asks:
> I'm pretty sure Dan is already doing before/after testing on v2 and v3. 
> As for HP/UX, does it even build?

I keep forgetting.  HP/UX builds, but it doesn't run.

Michael

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

[Andrew Cagney]

Michael Elizabeth Chastain wrote:

> andrew> This, unfortunatly, makes it sound like more than a cosmetic
> andrew> change :-(
> 
> daniel> How so?
> daniel> It wasn't able to detect destructors before, because it was looking
> daniel> for the v2 abi stuff.
> 
> I think that Andrew is saying: something that fixes bug is "*more*
> than cosmetic", which means that it needs more review than a purely
> cosmetic change would.

Yes.

> I would like to see before-and-after test suite runs on two different
> platforms with both v2 and v3 g++, and maybe hpux aCC.  That's a lot of
> testing but this kind of change is prone to regression errors.

I'm pretty sure Dan is already doing before/after testing on v2 and v3. 
As for HP/UX, does it even build?  Fixing HPUX getting carried away a
little.

Anyway, it is important to resist the temptation to mix bug fixes in
with mechanical changes  It means that if there is a problem then
someone (else) can determine if it was the mechanical change or a
later/earlier bug fix.

	enjoy,
		Andrew

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

[Daniel Berlin]

Michael Elizabeth Chastain <chastain@cygnus.com> writes:

> Hi Daniel,
> 
> > Why is it necessary to have a long discussion about creating a directory
> > for a bunch of related files?
> 
> In the best case it's a very short discussion:
> 
:)

> . contributor describes their new files and their provisional dir structure
> . head maintainer chooses the actual dir structure

Works for me. Like I said, i'll put them wherever andrew wants them, I
just find it clearer to group them all in a common subdir.

> 
> Andrew Cagney asks:
> > Can you walk people through exactly how you're introducing the C++ abi.
> 
> My take on Daniel's patch is:
> 
> The current code is full of stuff like this:
> 
>   /* symtab.h */
>   /* 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] == '_')
> 
> This stuff is not just wrapped up in macros; it is all over the source code.
> Like this:
> 
>   /* c-typeprint.c */
>   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));
> 
> Daniel wants to change these to real functions like "constructor_prefix_p"
> and "destructor_prefix_p" which would make a call through a function
> pointer to the appropriate version for the abi (gcc v2, gcc v3, hp-acc,
> arm, whatever).


(Just a note, Remember, we don't support more than the v2 abi and HP
aCC's API right now. We can demangle the names of other ABI's, but we
can't do things like virtual function finding.)

Correct for the patch so far, however, it's not just these simple
hard-coded dependencies , the problem goes  deeper than names. 
Look at the valops example.

Or try to work your way through value_virtual_fn_field.

value_virtual_fn_field would be reduced to maybe 4 or 5 lines, since
it's doing completely abi specific stuff, except at the absolute end.
However, the operation it's performing, finding a virtual function
address, offsetting the this point if necessary, is the same
*operation* for both abi's (three abi's if you count the gnu-v3 abi) ,
you just do it a different way. 


Then we have the issue that you have abi specific functions floating
around that duplicate each other, but in different files/different names.
(find_rt_base_offset vs base_offset).

Look at lines 1675->1998 of gdbtypes.c

Completely hp specific abi stuff, duplicating functions with different
names in val*.c.

The other real problem is all of this stuff assumes if it's not HP
aCC, it's g++ v2 abi, which is wrong.
So i'm faced with either adding another if block to all these
functions, or abstracting them properly so the function you call is
the same, no matter what the underlying C++ abi is.  The second made
more sense, and would contribute more to gdb than the first.

So i started with the easy stuff, which is all name related, and won't
cause major problems.
Then i'll implement abi detection, and then, incrementally add things
to the cp_abi_ops structure, move things to the  approriate files, and
add the v3 abi support too.

It would make it a lot easier for others to extend the level of C++ support
we offer now. Right now, you have to handle all the abi's seperately,
and handle intricacies of each ABI, to do *anything* even slightly
tricky, like hunt down a virtual function.

> 
> The advantages are:
> 
> . code that knows things about names gets collected into one place for
>   each abi style that we support.

Like I said, it's not just names, i've only started with names because
it's easiest. Eventually, I'll encapsulate the rest of the difference.
> Right now it's a wild goose chase
>   to find all the places in the code that know about the innards of
>   abi data structures.  This makes it easier to support that new ABI,
>   "g++ v3 abi", which is coming at us.

The sad thing is, i know where it all is, for the most part.

grep on Talignent
then on aCC
and TYPE_HAS_VTABLE
and RRBC

This will start to give you an idea of where the nasty code i'm
talking about is.


> 
> . existing macros get replaced by functions.  These particular macros
>   are full of hairy conditionals that will be a lot more readable as
>   functions.
> 
> I am in favor of this approach -- I think it is necessary to collect
> all the abi guts into files this way in order to have something that
> other people can maintain.  That's why I'm sticking my nose in.
> 
> andrew> This, unfortunatly, makes it sound like more than a cosmetic
> andrew> change :-(
> 
> daniel> How so?
> daniel> It wasn't able to detect destructors before, because it was looking
> daniel> for the v2 abi stuff.
> 
> I think that Andrew is saying: something that fixes bug is "*more*
> than cosmetic", which means that it needs more review than a purely
> cosmetic change would.

Sure.

> 
> I would like to see before-and-after test suite runs on two different
> platforms with both v2 and v3 g++, and maybe hpux aCC.  That's a lot of
> testing but this kind of change is prone to regression errors.

They won't change.
I haven't added abi detection, so i have the default cp_abi_ops struct
being set to the v2 abi struct.

Therefore, since I moved all of the v2 code from other places, it's
provably equivalent right now.

I currently do the testsuite testing by manually switching by
commenting  the approriate "cp_abi_ops = <...>" line in either
gnu-v2-abi.c or gnu-v3-abi.c, and uncommenting the other.

Let me switch back to v2 (i was implementing more of the v3 abi
functions, like operator_prefix_p), and run a testsuite anyway.

In fact, it might fix some fails that we don't catch in corner
cases, particularly the one place that was checking for chars "Qt" rather
than "123456789Qt", which is the right set of chars to look for.

It was funnily written in some places as other equivalent things, like
if is_digit (a[0]) || strchr("Qt", a[0]), which tells me it wasn't
just cut and paste, someone had no idea this stuff existed elsewhere.



--Dan


> 
> Michael

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

[Michael Elizabeth Chastain]

Hi Daniel,

> I can make it strncmp, or just check method_name[1], but it doesn't
> make sense otherwise. It's just seeing if the first letter is a "~" in
> the actual demangled method name, rather than the mangled name.

Ah, ok.  I was wondering why it wasn't part of destructor_prefix_p.

Michael

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

[Daniel Berlin]

Michael Elizabeth Chastain <chastain@cygnus.com> writes:

> Hi Daniel,
> 
> > I can make it strncmp, or just check method_name[1], but it doesn't
> > make sense otherwise. It's just seeing if the first letter is a "~" in
> > the actual demangled method name, rather than the mangled name.
> 
> Ah, ok.  I was wondering why it wasn't part of destructor_prefix_p.

Yeah. I plan on renaming the functions, so that it conveys what it's
really doing, but not until i'm far enough along in the rest of the
abstraction, since some might just go away completely, abstracted into
themselves (IE we might only need this really for the v2 abi), or done
away with in favor of better methods.

In order to minimize the code changes for the moment, so i had a
better chance of getting it approved quickly, I only made a few of the
macros (VTBL_PREFIX_P, DESTRUCTOR_PREFIX_P) into functions, added a
function for something that should have been at least a macro
(constructor prefixes, we had the same damn code copied everywhere,
and it was only somewhat right in a few places), and made it work for
both ABI's. I didn't rename anything, i just lowercased them. :)

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

[Daniel Berlin]

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

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


[-- Attachment #2: cp-abi.tar.gz --]
[-- Type: application/x-gzip, Size: 2023 bytes --]

[-- Attachment #3: cpabidiffs --]
[-- Type: text/x-c, Size: 15584 bytes --]

Index: c-typeprint.c
===================================================================
RCS file: /cvs/src/src/gdb/c-typeprint.c,v
retrieving revision 1.4
diff -c -3 -p -r1.4 c-typeprint.c
*** c-typeprint.c	2000/12/15 01:01:46	1.4
--- c-typeprint.c	2001/02/18 20:42:41
***************
*** 38,43 ****
--- 38,44 ----
  #include "gdb_string.h"
  #include <errno.h>
  #include <ctype.h>
+ #include "cp-abi.h"
  
  /* Flag indicating target was compiled by HP compiler */
  extern int hp_som_som_object_present;
*************** 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))
*************** c_type_print_base (struct type *type, st
*** 963,969 ****
  
  		  demangled_name =
  		    cplus_demangle (mangled_name,
! 				    DMGL_ANSI | DMGL_PARAMS);
  		  if (demangled_name == NULL)
  		    {
  		      /* in some cases (for instance with the HP demangling),
--- 963,969 ----
  
  		  demangled_name =
  		    cplus_demangle (mangled_name,
! 				    DMGL_ANSI | DMGL_PARAMS | DMGL_VERBOSE);
  		  if (demangled_name == NULL)
  		    {
  		      /* in some cases (for instance with the HP demangling),
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);
Index: gdbtypes.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbtypes.c,v
retrieving revision 1.16
diff -c -3 -p -r1.16 gdbtypes.c
*** gdbtypes.c	2000/12/15 01:01:47	1.16
--- gdbtypes.c	2001/02/18 20:42:42
***************
*** 33,39 ****
--- 33,41 ----
  #include "demangle.h"
  #include "complaints.h"
  #include "gdbcmd.h"
+ #include "gdb_regex.h"
  #include "wrapper.h"
+ #include "cp-abi.h"
  
  /* These variables point to the objects
     representing the predefined C data types.  */
*************** int
*** 1018,1024 ****
  get_destructor_fn_field (struct type *t, int *method_indexp, int *field_indexp)
  {
    int i;
- 
    for (i = 0; i < TYPE_NFN_FIELDS (t); i++)
      {
        int j;
--- 1020,1025 ----
*************** get_destructor_fn_field (struct type *t,
*** 1026,1032 ****
  
        for (j = 0; j < TYPE_FN_FIELDLIST_LENGTH (t, i); j++)
  	{
! 	  if (DESTRUCTOR_PREFIX_P (TYPE_FN_FIELD_PHYSNAME (f, j)))
  	    {
  	      *method_indexp = i;
  	      *field_indexp = j;
--- 1027,1033 ----
  
        for (j = 0; j < TYPE_FN_FIELDLIST_LENGTH (t, i); j++)
  	{
! 	  if (destructor_prefix_p (TYPE_FN_FIELD_PHYSNAME (f, j)) != 0)
  	    {
  	      *method_indexp = i;
  	      *field_indexp = j;
Index: jv-typeprint.c
===================================================================
RCS file: /cvs/src/src/gdb/jv-typeprint.c,v
retrieving revision 1.3
diff -c -3 -p -r1.3 jv-typeprint.c
*** jv-typeprint.c	2000/12/15 01:01:47	1.3
--- jv-typeprint.c	2001/02/18 20:42:42
***************
*** 28,34 ****
  #include "gdb_string.h"
  #include "typeprint.h"
  #include "c-lang.h"
! 
  /* Local functions */
  
  static void java_type_print_base (struct type * type,
--- 28,34 ----
  #include "gdb_string.h"
  #include "typeprint.h"
  #include "c-lang.h"
! #include "cp-abi.h"
  /* Local functions */
  
  static void java_type_print_base (struct type * type,
*************** java_type_print_base (struct type *type,
*** 224,235 ****
  
  		  physname = TYPE_FN_FIELD_PHYSNAME (f, j);
  
! 		  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;
  
--- 224,230 ----
  
  		  physname = TYPE_FN_FIELD_PHYSNAME (f, j);
  
! 		  is_full_physname_constructor = constructor_prefix_p (physname) != 0 || destructor_prefix_p (physname) != 0;
  
  		  QUIT;
  
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,
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));
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 ----
Index: valops.c
===================================================================
RCS file: /cvs/src/src/gdb/valops.c,v
retrieving revision 1.31
diff -c -3 -p -r1.31 valops.c
*** valops.c	2001/02/06 20:05:41	1.31
--- valops.c	2001/02/18 20:42:44
***************
*** 30,35 ****
--- 30,36 ----
  #include "demangle.h"
  #include "language.h"
  #include "gdbcmd.h"
+ #include "cp-abi.h"
  
  #include <errno.h>
  #include "gdb_string.h"
*************** value_rtti_type (value_ptr v, int *full,
*** 3295,3301 ****
  
        /* Try to find a symbol that is the vtable */
        minsym=lookup_minimal_symbol_by_pc(vtbl);
!       if (minsym==NULL || (demangled_name=SYMBOL_NAME(minsym))==NULL || !VTBL_PREFIX_P(demangled_name))
  	return NULL;
  
        /* If we just skip the prefix, we get screwed by namespaces */
--- 3296,3302 ----
  
        /* Try to find a symbol that is the vtable */
        minsym=lookup_minimal_symbol_by_pc(vtbl);
!       if (minsym==NULL || (demangled_name=SYMBOL_NAME(minsym))==NULL || !vtbl_prefix_p(demangled_name))
  	return NULL;
  
        /* If we just skip the prefix, we get screwed by namespaces */
Index: Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/Makefile.in,v
retrieving revision 1.62
diff -c -3 -p -r1.62 Makefile.in
*** Makefile.in	2001/02/18 07:22:16	1.62
--- Makefile.in	2001/02/18 20:42:44
*************** INTL_DEPS = @INTLDEPS@
*** 137,143 ****
--- 137,158 ----
  INTL_SRC = $(srcdir)/$(INTL_DIR)
  INTL_CFLAGS = -I$(INTL_DIR) -I$(INTL_SRC)
  
+ # 
+ # CP-ABI sub directory definitions
  #
+ 
+ SUBDIR_CPABI_OBS = \
+ 	cp-abi.o gnu-v3-abi.o gnu-v2-abi.o
+ SUBDIR_CPABI_SRCS = \
+ 	cp-abi/cp-abi.c cp-abi/gnu-v3-abi.c cp-abi/gnu-v2-abi.c
+ SUBDIR_CPABI_DEPS =
+ SUBDIR_CPABI_INITS = cp-abi/cp-abi.c cp-abi/gnu-v3-abi.c cp-abi/gnu-v2-abi.c
+ SUBDIR_CPABI_CFLAGS =
+ SUBDIR_CPABI_ALL = 
+ SUBDIR_CPABI_CLEAN =
+ SUBDIR_CPABI_INSTALL =
+ SUBDIR_CPABI_UNINSTALL =
+ #
  # CLI sub directory definitons
  #
  SUBDIR_CLI_OBS = \
*************** varobj.o: varobj.c $(defs_h) $(frame_h) 
*** 2057,2062 ****
--- 2072,2088 ----
  	$(language_h) valprint.h varobj.h wrapper.h
  
  wrapper.o: wrapper.c $(defs_h) $(frame_h) $(value_h) wrapper.h
+ 
+ # CPABI dependencies
+ # Need to explicitly specify the compile rule as make will do nothing
+ # or try to compile the object file into the cp-abi directory.
+ 
+ cp-abi.o: $(srcdir)/cp-abi/cp-abi.c cp-abi.h 
+ 	$(CC) -c $(INTERNAL_CFLAGS) $(srcdir)/cp-abi/cp-abi.c
+ gnu-v3-abi.o: $(srcdir)/cp-abi/gnu-v3-abi.c cp-abi.h
+ 	$(CC) -c $(INTERNAL_CFLAGS) $(srcdir)/cp-abi/gnu-v3-abi.c
+ gnu-v2-abi.o: $(srcdir)/cp-abi/gnu-v2-abi.c cp-abi.h
+ 	$(CC) -c $(INTERNAL_CFLAGS) $(srcdir)/cp-abi/gnu-v2-abi.c
  
  #
  # CLI dependencies
Index: configure.in
===================================================================
RCS file: /cvs/src/src/gdb/configure.in,v
retrieving revision 1.56
diff -c -3 -p -r1.56 configure.in
*** configure.in	2001/02/14 18:48:40	1.56
--- configure.in	2001/02/18 20:42:44
*************** esac
*** 499,504 ****
--- 499,527 ----
  
  dnl Handle optional features that can be enabled.
  
+ dnl Handle cp-abi sub-directory configury.
+ AC_ARG_ENABLE(cp-abi,
+ [  --enable-cp-abi	     Enable C++ ABI abstraction],
+ [
+   case "${enable_cp_abi}" in
+     "yes" | "no") ;;
+     "") enable_cp_abi=yes ;;
+      *)
+       AC_MSG_ERROR(Bad value for --enable-cp-abi: ${enableval})
+     ;;
+     esac
+ ], [enable_cp_abi=yes])
+ case ${enable_cp_abi} in
+   "yes" )
+     if test -d "${srcdir}/cp-abi" ; then
+       CONFIG_OBS="${CONFIG_OBS} \$(SUBDIR_CPABI_OBS)"
+       CONFIG_DEPS="${CONFIG_DEPS} \$(SUBDIR_CPABI_DEPS)"
+       CONFIG_SRCS="${CONFIG_SRCS} \$(SUBDIR_CPABI_SRCS)"
+       CONFIG_INITS="${CONFIG_INITS} \$(SUBDIR_CPABI_INITS)"
+       ENABLE_CFLAGS="${ENABLE_CFLAGS} \$(SUBDIR_CPABI_CFLAGS)"
+     fi
+     ;;
+ esac
  dnl Handle MI sub-directory configury.
  AC_ARG_ENABLE(gdbmi,
  [  --enable-gdbmi            Enable GDB-MI interface],
*************** case ${enable_gdbmi} in
*** 526,531 ****
--- 549,555 ----
      fi
      ;;
  esac
+ 
  
  # Configure UI_OUT by default (before 5.2 it can be disabled)
  # It must be configured if gdbmi is configured

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

[Eli Zaretskii]

On 18 Feb 2001, Daniel Berlin wrote:

> This patch, plus the attached files, start the abstraction of the C++
> ABI's.

I know this is too early, but please consider updating gdb.texinfo
and/or gdbint.texinfo if this stuff should be mentioned there.

If you have problems with technicalities of Texinfo, I'm willing to
help.

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

[Daniel Berlin]

Eli Zaretskii <eliz@is.elta.co.il> writes:

> On 18 Feb 2001, Daniel Berlin wrote:
> 
> > This patch, plus the attached files, start the abstraction of the C++
> > ABI's.
> 
> I know this is too early, but please consider updating gdb.texinfo
> and/or gdbint.texinfo if this stuff should be mentioned there.
> 
> If you have problems with technicalities of Texinfo, I'm willing to
> help.


It'll all be in gdbint.texinfo. The rest is completely transparent to
the user.

I've already got a chapter started in gdbint.texinfo, but it's just an
outline and notes right now, since as you said, it's early.
I didn't want to rewrite the details over and over.

But rest assured, this stuff will be documented, unlike the stuff
it's replacing.

--Dan

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

[Eli Zaretskii]

On 19 Feb 2001, Daniel Berlin wrote:

> But rest assured, this stuff will be documented, unlike the stuff
> it's replacing.

Thank you!

(I'm sorry to be such a nag lately on these matters, but after reading 
gdbint.texinfo from the first line to the last, I came to the conclusion 
that it leaves a lot to be desired, to put it gently...)

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

[Daniel Berlin]

Eli Zaretskii <eliz@is.elta.co.il> writes:

> On 19 Feb 2001, Daniel Berlin wrote:
> 
> > But rest assured, this stuff will be documented, unlike the stuff
> > it's replacing.
> 
> Thank you!
> 
> (I'm sorry to be such a nag lately on these matters, but after reading 
> gdbint.texinfo from the first line to the last, I came to the conclusion 
> that it leaves a lot to be desired, to put it gently...)

Yup. Somewhere along the line, the idea that you should be able to
familiarize yourself with an area of gdb by 
1. Reading the approriate section of gdbint
2. Reading the code

got lost, and now we are only left with:

1. Reading the code

which is bad. This gives us no way to know what it's *supposed* to be
doing, or how it's *supposed* to be working, among other things.


Every couple weeks I find some new piece of hardcoded C++ code in gdb
in a random file that I never noticed before.  It was obvious no one
ever tried to document this stuff, or else they would have realized
they file they were trying to put something in was the *wrong one*.

--Dan

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

[Elena Zannoni]

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

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

[Daniel Berlin]

Elena Zannoni <ezannoni@cygnus.com> writes:

> 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.
I'll resend with the changelog entries, and the things you ask for
below fixed.


> 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
> 

Yes, it's temporarily empty.
It'll be filled in very shortly (read: tomorrow at the latest).

> 
> Note that since your patch is incomplete, I cannot test it. So the 
> 'approved' is conditional.
Okeydokey.
> 
> 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] == '~'
you mean method_name[0] == '~' 
:).
But, Okeydokey.

>  
> 
> 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?

No, I was just doing search and replace, i'll remove them.

> 
> 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?
You mean the one where gdb_regex is now?
I don't know what i did to the one before destructor_prefix_p, it's
still there, it's just "not as blank" or something, I guess.

> Could you remove/update the comment/FIXME before CHECK_TYPEDEF.
Yup.

> Do we need to include gdb_regexp.h? What needs it?

No. I don't know how that slipped in there.

> 
> 
> 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.
> 

I forgot to cut the VTBL_PREFIX_P comment out, i'll do this as well.

> Elena

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

[Andrew Cagney]

Daniel Berlin wrote:
> 
> 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.

FYI,

Moving stuff into a sub directory is a significant change and should
really be discussed separatly - at present the only think in sub
directories are UIs.  I tend to recommend leaving such cosmetic changes
as a latter pass.

Also remember that it is always best to figure out what the issues with
this change are likely to be and then discuss them publically.  Can you
walk people through exactly how you're introducing the C++ abi.

> This fixes some problems with the new-abi already, like not
> being able to set breakpoints on destructors (if you try it,

This, unfortunatly, makes it sound like more than a cosmetic change :-(

As they say, sounds good in theory.

	Andrew

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

[Daniel Berlin]

Andrew Cagney <ac131313@cygnus.com> writes:

> Daniel Berlin wrote:
> > 
> > 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.
> 
> FYI,
> 
> Moving stuff into a sub directory is a significant change and should
> really be discussed separatly - at present the only think in sub
> directories are UIs.  I tend to recommend leaving such cosmetic changes
> as a latter pass.

It's not moving stuff into a subdir, as it didn't exist before. I'm
creating it in a subdir. Why is it necessary to have a long discussion
about creating a directory for a bunch of related files? I'm happy to
see if anyone objects, but I don't see it as a significant change.
Maybe this is why it never gets done.

if you really want me to dirty the root directory with these new
files, i will, but i felt it made a lot more sense not to.There are
currently over 400 files in the root gdb directory. Besides the -nat
and -tdep files, I can't tell what a file does from it's name, because
they aren't logically divided into subdirs by functionality, and there
are just too damn many files.  Not that I expect to see this happen,
but to prove a point: 
Quick, what's kod.h?
ocd.c?
stuff.c?
Can you tell that ppc-bdm.c is related to ocd.c?

> 
> Also remember that it is always best to figure out what the issues with
> this change are likely to be and then discuss them publically.

None, as of yet. I've simply replaced macros with abstracted
functions, and provided the implementations of those functions for the
v2, and v3 abi.


> Can you
> walk people through exactly how you're introducing the C++ abi.

Errr, i'm not introducing a C++ abi, i'm introducing a structure so we
can remove the hardcoded dependencies on the C++ ABI's, by abstracting
what they do, and do differently, into functions that will have
different implemenation specifics, but do the same thing.

Fer instance, determining if some mangled name is a destructor.
We currently have a G++ specific macro used all over the place,
DESTRUCTOR_PREFIX_P.
This won't work for the v3 abi. 

We also have a bunch of code that does one thing if it's HP aCC, and
another if it's G++, but they both are performing the same actual
operation, just doing it in different ways because of abi specifics.

This happens all over the place. For instance, we have duplicate sets
of functions  that have large implementation differences (because the
abi's are very different) to search through virtual base classes for
HP aCC, and  G++. However, they both take the same actual parameters, and
return the same values.  We then have if blocks that say

if (hp_som_object)
{
<Whole bunch of HP specific code>
}
else
{
/* Assume it's G++ */
<Whole bunch of G++ specific code that does the same thing, with G++'s
abi>
}

all over the place (literally)


For a random example, from valops.c: 

        if (TYPE_HAS_VTABLE (type))
            {
              /* HP aCC compiled type, search for virtual base offset
               * according to HP/Taligent runtime spec.  */
              int skip;
              find_rt_vbase_offset (type, TYPE_BASECLASS (type, i),
                                    VALUE_CONTENTS_ALL (*argp),
                                    offset + VALUE_EMBEDDED_OFFSET (*argp),
                                    &base_offset, &skip);
              if (skip >= 0)
                error ("Virtual base class offset not found in vtable");
            }
          else
            {
              /* probably g++ runtime model */
              base_offset = VALUE_OFFSET (*argp) + offset;
              base_offset =
                baseclass_offset (type, i,
                                  VALUE_CONTENTS (*argp) + base_offset,
                                  VALUE_ADDRESS (*argp) + base_offset);
              if (base_offset == -1)
                error ("virtual baseclass botch");
            }

This is pretty dumb, they both do the same thing, just for different
ABI's.
This stuff is rampant.

This type of thing  will also be abstracted away, so it's just

base_offset = baseclass_offset (...)



This is necessary for the addition of the v3 G++ abi, unless you want
me to extend all the if blocks, and add more inline code into fifty
five places. And add another set of functions that do the same thing,
just have different implementations.


> 
> > This fixes some problems with the new-abi already, like not
> > being able to set breakpoints on destructors (if you try it,
> 
> This, unfortunatly, makes it sound like more than a cosmetic change :-(
> 

How so?
It wasn't able to detect destructors before, because it was looking
for the v2 abi stuff.
Now, it has the ability to detect both, without any changes to any
non C++ abi specific source. This is due to the added code in gnu-v3-abi.c.

Currently, you could view the stuff i've submitted so far as cosmetic,
if you really wanted to, but it's necessary to support both ABI's at
the same time. I also didn't want to submit too much at once, so that
nobody complained that i should break the patch down. Now it sounds
like your saying it doesn't do *enough* yet.

> As they say, sounds good in theory.


> 
> 	Andrew

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

[Andrew Cagney]

Daniel Berlin wrote:
> 
> Andrew Cagney <ac131313@cygnus.com> writes:
> 
> > Daniel Berlin wrote:
> > >
> > > 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.
> >
> > FYI,
> >
> > Moving stuff into a sub directory is a significant change and should
> > really be discussed separatly - at present the only think in sub
> > directories are UIs.  I tend to recommend leaving such cosmetic changes
> > as a latter pass.

I probably phrased this badly.  The trick with getting a patch through
quickly is to go over it repeatedly with a fine tooth comb until you've
removed everything that isn't directly related to the intended change.

> It's not moving stuff into a subdir, as it didn't exist before. I'm
> creating it in a subdir. Why is it necessary to have a long discussion
> about creating a directory for a bunch of related files? I'm happy to
> see if anyone objects, but I don't see it as a significant change.
> Maybe this is why it never gets done.

There isn't a necessity for a long discussion however I think it is
reasonable for me to ask you to explaine the rationale behind the
introduction of a new directory.  Remember, you're putting in place
conventions that will last a very long time - it is important to get
them reasonably right first up.

If it were me doing this work, I would have broken it down into several
stages:

	o	create cp-abi.[hc]

		Here, the only thing
		anyone could possibly object
		to is the name :-)

	o	re-structure language
		directories

		I'd be seeking idea's from all
		the players (C, Pascal, O-C,
		et.al.)

enjoy,
	Andrew

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

[Stan Shebs]

Daniel Berlin wrote:
> 
> [...] Why is it necessary to have a long discussion
> about creating a directory for a bunch of related files? I'm happy to
> see if anyone objects, but I don't see it as a significant change.
> Maybe this is why it never gets done.

Having sources in a subdir can be a significant pain for building
if done wrong.  It also breaks people's search patterns and scripts
that look for stuff.  So what's helpful for you may be a hindrance
for somebody else.

> if you really want me to dirty the root directory with these new
> files, i will, but i felt it made a lot more sense not to.There are
> currently over 400 files in the root gdb directory. Besides the -nat
> and -tdep files, I can't tell what a file does from it's name, because
> they aren't logically divided into subdirs by functionality, and there
> are just too damn many files.  Not that I expect to see this happen,
> but to prove a point:
> Quick, what's kod.h?

Kernel object display header file...

> ocd.c?

On-chip debugging support...

> stuff.c?

Stuff that probably should have been deleted long ago, but I wasn't sure
whether it was used by anybody or not. :-)

> Can you tell that ppc-bdm.c is related to ocd.c?

Sure, because BDM is Motorola's version of OCD. :-) :-)

This isn't so much an argument for subdir organization, it's an argument
to keep the number of files down and to give them good names.  Your
examples are also a good argument for public discussion prior to
creating directories and files, since bad choices will confuse future
implementors, as you've just noted.

I will cop to being one of those who've tended to discourage subdirs
for GDB sources.  Ironically, this is not because I like flat layouts
(for Xconq, I put *everything* in subdirs), but because GDB's current
build machinery heavily favors flatness in the way that it works with
files.  Some of this is simply historical - GDB originally had literally
everything in the one dir, and years ago Fred Fish moved target config
files to subdirs.

So if someone were to come up with a good design for repartitioning
GDB sources, *and* were to follow up by implementing it, as Fred did,
I think a lot of people would support it.  If it's possible to
implement the design incrementally, that's OK too, but I'd like to
know that there is a well-defined end state, so that the sources don't
end up in a sort of half-organized mush.

Stan

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

[Michael Snyder]

Daniel Berlin wrote:
> 
> Andrew Cagney <ac131313@cygnus.com> writes:
> 
> > Daniel Berlin wrote:
> > >
> > > 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.
> >
> > FYI,
> >
> > Moving stuff into a sub directory is a significant change and should
> > really be discussed separatly - at present the only think in sub
> > directories are UIs.  I tend to recommend leaving such cosmetic changes
> > as a latter pass.
> 
> It's not moving stuff into a subdir, as it didn't exist before. I'm
> creating it in a subdir. Why is it necessary to have a long discussion
> about creating a directory for a bunch of related files?

Because it's a departure.  Except for certain specific targets in the
distant past, it hasn't been done much.  If we're gonna start doing
it regularly, it's worth discussion.  It might require additional 
configury.  It might impact the way people work.  People might like
seeing all of gdb spread out in a single directory instead of in a tree.