Re: [PATCH] New C++ abstraction patch

Re: [PATCH] New C++ abstraction patch

[Michael Elizabeth Chastain]

Hi Daniel,

I have some comments on your patch.

Michael

===

+ gnu-v3-abi.o: gnu-v3-abi.c cp-abi.h
+ 
+ gnu-v2-abi.o: gnu-v2-abi.c cp-abi.h
+ 

gnu-v3-abi.o and gnu-v2-abi.o also depend on
$(defs_h) gdb_regex.h gdb_string.h.



+ int gnuv2_vtable_prefix_p (const char * name)
+ {
+   return (((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] == '_'));
+ }

Would you consider writing this as:

  int gnuv2_vtable_prefix_p (const char * name)
  {
    if (strncmp (name, "_VT", 3) == 0 && is_cplus_marker (name[3]))
      return 1;
    if (strncmp (name, "_vt", 3) == 0 && is_cplus_marker (name[3]))
      return 1;
    if (strncmp (name, "__vt_", 5) == 0)
      return 1;
    return 0;
  }

Or if that is not to your taste, something that is more readable than
the former macro?

(This is a style issue rather than a correctness issue so I would
accept whatever you decide to write, including the original).



+ //struct cp_abi_ops current_cp_abi;

This would be the first use of // comments in gdb.  There are some
instances of '//', but they are already inside comments, or they are
part of /* ... *//* ... */ constructions.



+ /* FIXME: Stop doing the re_comp's everytime. Use the other regex api
+    and compile the patterns ahead of time */

One of the other uses of re_comp/re_exec is in solib_add, which calls
symbol_file_add in a loop.  I think you're in danger of overwriting the
single global re_comp buffer.

There are other uses in target-specific solib loaders, like irix and hpux.

I like regular expressions but I think you need regcomp/regexec/regerror
rather than re_comp/re_exec.



+ int gnuv3_destructor_prefix_p (const char * name)
+ {
+   re_comp ("[^0-9]D[1-3]Ev");
+   return re_exec (name) != 0;
+ }

libiberty/cp-demangle.c says that dtor-names are D0 D1 D2, not D1 D2 D3.

What happens if the target program has a method named TOORAD2Evade?
I think your r.e. would match that mangled name.



+ int gnuv3_vtable_prefix_p (const char * name)
+ {
+   return strncmp (name, "_ZTV", 4) != 0;
+ }

This strncmp() != 0 should be strcmp() == 0.  (silly strncmp)

Re: [PATCH] New C++ abstraction patch

[Daniel Berlin]

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

> Hi Daniel,
> 
> I have some comments on your patch.
> 
> Michael
> 
> ===
> 
> + gnu-v3-abi.o: gnu-v3-abi.c cp-abi.h
> + 
> + gnu-v2-abi.o: gnu-v2-abi.c cp-abi.h
> + 
> 
> gnu-v3-abi.o and gnu-v2-abi.o also depend on
> $(defs_h) gdb_regex.h gdb_string.h.
> 

Okey-dokey.

> 
> 
> + int gnuv2_vtable_prefix_p (const char * name)
> + {
> +   return (((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] == '_'));
> + }
> 
> Would you consider writing this as:
> 
>   int gnuv2_vtable_prefix_p (const char * name)
>   {
>     if (strncmp (name, "_VT", 3) == 0 && is_cplus_marker (name[3]))
>       return 1;
>     if (strncmp (name, "_vt", 3) == 0 && is_cplus_marker (name[3]))
>       return 1;
>     if (strncmp (name, "__vt_", 5) == 0)
>       return 1;
>     return 0;
>   }
> 
> Or if that is not to your taste, something that is more readable than
> the former macro?

Sure, that's fine, I was just cutting and pasting the v2 stuff, since i
was more concerned with the v3 implementation stuff.


> 
> (This is a style issue rather than a correctness issue so I would
> accept whatever you decide to write, including the original).
> 

Hey, i'm happy to do whatever makes it easier 
> 
> 
> + //struct cp_abi_ops current_cp_abi;
> 
> This would be the first use of // comments in gdb.  There are some
> instances of '//', but they are already inside comments, or they are
> part of /* ... *//* ... */ constructions.

Whoops, I forgot to take that out (As I said, i was switching back and
forth by hand).

> 
> 
> 
> + /* FIXME: Stop doing the re_comp's everytime. Use the other regex api
> +    and compile the patterns ahead of time */
> 
> One of the other uses of re_comp/re_exec is in solib_add, which calls
> symbol_file_add in a loop.  I think you're in danger of overwriting the
> single global re_comp buffer.

I was unware.
I'll rewrite it.
> 
> There are other uses in target-specific solib loaders, like irix and hpux.
> 
> I like regular expressions but I think you need regcomp/regexec/regerror
> rather than re_comp/re_exec.

I do too, I was just trying to avoid having to relearn the horrible.

> 
> 
> 
> + int gnuv3_destructor_prefix_p (const char * name)
> + {
> +   re_comp ("[^0-9]D[1-3]Ev");
> +   return re_exec (name) != 0;
> + }
> 
> libiberty/cp-demangle.c says that dtor-names are D0 D1 D2, not D1 D2
> D3.
You are correct.

I wonder what possessed them to use C1,C2,C3, then D0,D1,D2.

> 
> What happens if the target program has a method named TOORAD2Evade?


> I think your r.e. would match that mangled name.
> 
Yes. There is nothing I can do about this.
There is no unambiguous destructor prefix.

Welcome to "Hey, let's hardcode this stuff in" hell.

I plan on using [^0-9]D[1-3]Ev$

So if you had TOORAD2Ev, you'd match.

I could also demangle the name, and check for the "~", but without my
ternary search tree stuff that caches demangled name, this would be a
*serious* performance hit.

I could also use [^0-9]D[1-3]Ev$, and if it matches, demangle, then
check for certain.

This would be much better, performance wise.  

> 
> 
> + int gnuv3_vtable_prefix_p (const char * name)
> + {
> +   return strncmp (name, "_ZTV", 4) != 0;
> + }
> 
> This strncmp() != 0 should be strcmp() == 0.  (silly strncmp)

Right again.
Let me rewrite the regex stuff.

--Dan

Re: [PATCH] New C++ abstraction patch

[Andrew Cagney]

Daniel Berlin wrote:
> 
> Michael Elizabeth Chastain <chastain@cygnus.com> writes:
> 
> > Hi Daniel,
> >
> > I have some comments on your patch.
> >
> > Michael
> >
> > ===
> >
> > + gnu-v3-abi.o: gnu-v3-abi.c cp-abi.h
> > +
> > + gnu-v2-abi.o: gnu-v2-abi.c cp-abi.h
> > +

FYI,

The convention (and yes it is gross) is:

	cp-abi_h = cp-abi.h
	....
	gnu-v3-abi.o: $(defs_h) $(cp-abi_h) ...

(I think ``cp-abi_h'' is a valid Makefile macro).  When things get
converted to automake this will all go away.

	Andrew

Re: [PATCH] New C++ abstraction patch

[Daniel Berlin]

Andrew Cagney <ac131313@cygnus.com> writes:

> Daniel Berlin wrote:
> > 
> > Michael Elizabeth Chastain <chastain@cygnus.com> writes:
> > 
> > > Hi Daniel,
> > >
> > > I have some comments on your patch.
> > >
> > > Michael
> > >
> > > ===
> > >
> > > + gnu-v3-abi.o: gnu-v3-abi.c cp-abi.h
> > > +
> > > + gnu-v2-abi.o: gnu-v2-abi.c cp-abi.h
> > > +
> 
> FYI,
> 
> The convention (and yes it is gross) is:
> 
> 	cp-abi_h = cp-abi.h
> 	....
> 	gnu-v3-abi.o: $(defs_h) $(cp-abi_h) ...
> 
> (I think ``cp-abi_h'' is a valid Makefile macro).  When things get
> converted to automake this will all go away.
> 
I did this at first, but it seemed those only happened for certain
headers (something to do with $(SRCDIR)), so I undid it before
submitting the patch.
I'll redo it.

--Dan

Re: [PATCH] New C++ abstraction patch

[Andrew Cagney]

Daniel Berlin wrote:

> I did this at first, but it seemed those only happened for certain
> headers (something to do with $(SRCDIR)), so I undid it before
> submitting the patch.
> I'll redo it.

Yep.  What is happening is that no one can be bothered converting the
entire file to that convention - automake will make the problem go away
right :-).  Instead things just get incrementally tweaked when a new
file is added, broken include dependency gets noticed (or someone gets
board).

	Andrew

Re: [PATCH] New C++ abstraction patch

[Michael Snyder]

Andrew Cagney wrote:
> 
> Daniel Berlin wrote:
> 
> > I did this at first, but it seemed those only happened for certain
> > headers (something to do with $(SRCDIR)), so I undid it before
> > submitting the patch.
> > I'll redo it.
> 
> Yep.  What is happening is that no one can be bothered converting the
> entire file to that convention - automake will make the problem go away
> right :-).  Instead things just get incrementally tweaked when a new
> file is added, broken include dependency gets noticed (or someone gets
> board).

Actually, I thought that the practice was only done selectively.
I don't see its usefulness, except for headers that might have
different names for different configurations.

Re: [PATCH] New C++ abstraction patch

[Andrew Cagney]

Michael Snyder wrote:

> Actually, I thought that the practice was only done selectively.
> I don't see its usefulness, except for headers that might have
> different names for different configurations.

It does two things - handles include files with possibly different paths
(the SRCDIR that dan noticed); makes the include spaghetti mess easier
to manage (defs_h expands to all the files it includes, similarly for
others).

Given automake makes it all go a way it isn't really an issue.

	Andrew

Re: [PATCH] New C++ abstraction patch

[Michael Elizabeth Chastain]

> I think the right thing is to do "^_Z.*[1-9][a-zA-Z_][a-zA-Z0-9_]+D[0-2]Ev$",
> since the class name always comes right before the destructor.

I like the ^Z part.  I thought the [1-9] should be [1-9]+, but then I
realized that the ".*" in front of it takes care of classes that are
longer than 9 characters.

The [a-zA-Z_][a-zA-Z0-9_]+ does not accept 1-character classes.  How about
[a-zA-Z_][a-zA-Z0-9_]*.

GNU C++ has an "-fdollars-in-identifiers" flag, so you have to accept '$'
as an alphabetic character.

  ^_Z.*[1-9][a-zA-Z_$][a-zA-Z0-9_$]*D[0-2]Ev$

> Otherwise, i'll look at cleaning up the ternary search tree stuff for
> demangled names and submitting it.

Watch out -- dependency creep.

If it were me, I would go ahead with the v3 abi stuff and get something
in with an r.e. that works most of the time.  Today's gdb works 0%
of the time in this case.  Then pick up the ternary search trees as a
separate project.  But that's just me.

(I read up on ternary search trees last night.  They look like an awesome
data type to replace the hash tables.)

Michael

Re: [PATCH] New C++ abstraction patch

[Michael Elizabeth Chastain]

Hi Daniel,

> I plan on using [^0-9]D[1-3]Ev$

I am suspicious of the [^0-9] part.  I just made a class named "B1",
and its destructor name is "_ZN2B1D2Ev".  Indeed, I can't figure out
what problem the [^0-9] is trying to solve anyways.

(My usual setup: 2001-02-20 gcc, rhl7 native, tried both stabs and dwarf-2).

Michael

Re: [PATCH] New C++ abstraction patch

[Daniel Berlin]

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

> Hi Daniel,
> 
> > I plan on using [^0-9]D[1-3]Ev$
> 
> I am suspicious of the [^0-9] part.  I just made a class named "B1",
> and its destructor name is "_ZN2B1D2Ev".  Indeed, I can't figure out
> what problem the [^0-9] is trying to solve anyways.

Make sure it's not the beginning of a name accidently.
Of course, it's pointless, but I added it before I added the "ends in
Ev" rule.

I think the right thing is to do "^_Z.*[1-9][a-zA-Z_][a-zA-Z0-9_]+D[0-2]Ev$", since the class
name always comes right before the destructor.
This also makes sure we don't accidently get the classname, even if
it's D[0-2].

I generated this regex from staring at the demangler for a while.
:)


It's really (to break it down in case you see a part i've gotten wrong)
_Z.*<mangled classname regex><mangled destructor name regex>Ev$

1-9 because you can't have a class name of length 0.

I think i have a few valid identifier characters missing, i'll double
check it if you think this regex would work if i added them.

Otherwise, i'll look at cleaning up the ternary search tree stuff for
demangled names and submitting it.

Temporarily, of course, i'm more than happy to just take the speed hit
in the name of correctness, and getting v3 abi supported properly.


--Dan


> 
> (My usual setup: 2001-02-20 gcc, rhl7 native, tried both stabs and dwarf-2).
> 
> Michael

Re: [PATCH] New C++ abstraction patch

[Michael Snyder]

Daniel Berlin wrote:
> 
> Michael Elizabeth Chastain <chastain@cygnus.com> writes:
> 
> > Hi Daniel,
> >
> > > I plan on using [^0-9]D[1-3]Ev$
> >
> > I am suspicious of the [^0-9] part.  I just made a class named "B1",
> > and its destructor name is "_ZN2B1D2Ev".  Indeed, I can't figure out
> > what problem the [^0-9] is trying to solve anyways.
> 
> Make sure it's not the beginning of a name accidently.
> Of course, it's pointless, but I added it before I added the "ends in
> Ev" rule.
> 
> I think the right thing is to do "^_Z.*[1-9][a-zA-Z_][a-zA-Z0-9_]+D[0-2]Ev$", since the class
> name always comes right before the destructor.
> This also makes sure we don't accidently get the classname, even if
> it's D[0-2].
> 
> I generated this regex from staring at the demangler for a while.
> :)
> 
> It's really (to break it down in case you see a part i've gotten wrong)
> _Z.*<mangled classname regex><mangled destructor name regex>Ev$
> 
> 1-9 because you can't have a class name of length 0.
> 
> I think i have a few valid identifier characters missing, i'll double
> check it if you think this regex would work if i added them.
> 
> Otherwise, i'll look at cleaning up the ternary search tree stuff for
> demangled names and submitting it.
> 
> Temporarily, of course, i'm more than happy to just take the speed hit
> in the name of correctness, and getting v3 abi supported properly.

When you speak of a speed hit, are you speaking of symbol-file-load time,
or expression evaluation time?  I think expression evaluation is far 
less important to optimize for speed than symbol loading.  Lots of people
complain about the time gdb takes to start up on a new symbol file, while
very few (that I'm aware of) complain about the performance during 
expression evaluation.

Re: [PATCH] New C++ abstraction patch

[J.T. Conklin]

>>>>> "Michael" == Michael Snyder <msnyder@cygnus.com> writes:
Michael> When you speak of a speed hit, are you speaking of
Michael> symbol-file-load time, or expression evaluation time?  I
Michael> think expression evaluation is far less important to optimize
Michael> for speed than symbol loading.  Lots of people complain about
Michael> the time gdb takes to start up on a new symbol file, while
Michael> very few (that I'm aware of) complain about the performance
Michael> during expression evaluation.

Note that expression evaluation time isn't usually an issue for
interactive use, but it becomes an issue in user defined functions
that grovel around complicated data structures, etc.

        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: [PATCH] New C++ abstraction patch

[Michael Snyder]

"J.T. Conklin" wrote:
> 
> >>>>> "Michael" == Michael Snyder <msnyder@cygnus.com> writes:
> Michael> When you speak of a speed hit, are you speaking of
> Michael> symbol-file-load time, or expression evaluation time?  I
> Michael> think expression evaluation is far less important to optimize
> Michael> for speed than symbol loading.  Lots of people complain about
> Michael> the time gdb takes to start up on a new symbol file, while
> Michael> very few (that I'm aware of) complain about the performance
> Michael> during expression evaluation.
> 
> Note that expression evaluation time isn't usually an issue for
> interactive use, but it becomes an issue in user defined functions
> that grovel around complicated data structures, etc.

Sure -- but not nearly so much an issue as in symbol loading, where
large symbol files may take minutes to load.  And symbol loading must
be done for (nearly) every debugging session, whereas user defined
functions that grovel around complicated data structures are the exception.

[PATCH] New C++ abstraction patch

[Daniel Berlin]

Includes Elena's requested changes, moves gnu-v2-abi.c and gnu-v3-abi.c to
the root directory.

cp-abi.c is gone temporarily until the next set of patches.


This patch includes the new files in diff format.


2001-02-20  Daniel Berlin  <dberlin@redhat.com>

	* Makefile.in (SFILES): Add gnu-v2-abi.c, gnu-v3-abi.c
	(COMMON_OBJS): Add gnu-v2-abi.o, gnu-v3-abi.o
	(gnu-v3-abi.o): Add.
	(gnu-v2-abi.o): Add.
	(symtab.o): Add cp-abi.h.
	(linespec.o): Add cp-abi.h.
	(dbxread.o): Add cp-abi.h.
	(c-typeprint.o): Add cp-abi.h.

	* cp-abi.h: New file. C++ abi abstraction

	* gnu-v2-abi.c: New file. C++ abi abstraction implementation for
	v2 abi.

	* gnu-v3-abi.c New file. C++ abi abstraction implementation for v3
	abi.

	* valops.c (value_rtti_type): VTBL_PREFIX_P -> vtbl_prefix_p.
	Add cp-abi.h to the include list.
	
	* symtab.c (gdb_mangle_name): DESTRUCTOR_PREFIX_P ->
	destructor_prefix_p.
	Replace hardcoded g++ v2 constructor name test with
	constructor_prefix_p. 
	Add cp-abi.h to the include list.
	
	* linespec.c (find_methods): Change SYMBOL_TYPE (sym_class) to
	CHECK_TYPEDEF (t), remove FIXME.
	Change method name comparison from STREQ to strcmp_iw.
	DESTRUCTOR_PREFIX_P -> destructor_prefix_p, fix test so it works
	(it was broken before, it was doing the reverse of what it
	should).
	Add cp-abi.h to include list.
	
	* dbxread.c (record_minimal_symbol): VTBL_PREFIX_P ->
	vtbl_prefix_p.
	Add cp-abi.h to include list.
	
	* c-typeprint.c (c_type_print_base): Remove hardcoded g++ v2
	destructor/constructor name test, replace with
	destructor_prefix_p, constructor_prefix_p. 
	Change STREQN (method_name, "~", 1) to method_name[0] == '~'.
	Add cp-abi.h to include list

	* jv-typeprint.c (java_type_print_base): Remove hardcoded g++ v2
	destructor/constructor name test, replace with
	destructor_prefix_p, constructor_prefix_p.
	
	* symtab.h: Remove VTBL_PREFIX_P macro.
	Remove DESTRUCTOR_PREFIX_P macro.

Re: [PATCH] New C++ abstraction patch

[Eli Zaretskii]

On 20 Feb 2001, Daniel Berlin wrote:

> 	* gnu-v3-abi.c New file. C++ abi abstraction implementation for v3
> 	abi.

Nitpicking: there's a colon missing from this ChangeLog entry, after
the file name.

Re: [PATCH] New C++ abstraction patch

[Andrew Cagney]

Daniel Berlin wrote:
> 
> Includes Elena's requested changes, moves gnu-v2-abi.c and gnu-v3-abi.c to
> the root directory.

Dan,

If I understand things correctly, in C++, a function signature
determines not just the calling convention but also the symbols real
name.  If either of these requirements are not met then the code won't
work.  Consequently, for C++, the mangler and other C++ stuff really
make up part of the over all application binary interface.

Hence the choice of ``cp-abi.*'' as the basename.

	Andrew

Re: [PATCH] New C++ abstraction patch

[Daniel Berlin]

Andrew Cagney <ac131313@cygnus.com> writes:

> Daniel Berlin wrote:
> > 
> > Includes Elena's requested changes, moves gnu-v2-abi.c and gnu-v3-abi.c to
> > the root directory.
> 
> Dan,
> 
> If I understand things correctly, in C++, a function signature
> determines not just the calling convention but also the symbols real
> name.  If either of these requirements are not met then the code won't
> work. 

Yes.
It won't even link, though there's probably some pathological case i'm
missing where it will. If for some reason, it does link, it certainly
won't work.



> Consequently, for C++, the mangler and other C++ stuff really
> make up part of the over all application binary interface.

Yes.

Mangling is integral to any C++ ABI, because it is through mangling
that you get unique names for overloaded methods and class names,
and these unique names are the same, given the same function
signature, as required by the One Definition Rule (which C doesn't have).

The ODR, simplified, is as follow:

1. In any one translation unit, a template, type, function, or object
can have no more than one definition. (Some of these can have any number
of declarations. A definition provides an instance.)

2. In the entire program, an object or non-inline function cannot have
more than one definition; if an object or function is used, it must
have exactly one definition. (You can declare an object or function
that is never used, in which case you don't have to provide a
definition. In no event can there be more than one definition.)

3. Some things, like types, templates, and extern inline functions,
can be defined in more than one translation unit. For a given entity,
each definition must be the same. (Non-extern objects and functions
in different translation units are different entities, even if their
names and types are the same.)



(On a side note, this is why we can ignore duplicate C++ symbols just
based on the mangled name. Anytime we see another debug symbol,
anywhere, with the same name as something we've already seen, by the
ODR, it must be the exact same, and thus, have the exact same debug
info).
> 
> Hence the choice of ``cp-abi.*'' as the basename.

Yup.

> 
> 	Andrew