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


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

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


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


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