From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Berlin To: Michael Elizabeth Chastain Cc: ac131313@cygnus.com, gdb-patches@sources.redhat.com Subject: Re: [PATCH] Start abstraction of C++ abi's Date: Mon, 19 Feb 2001 15:01:00 -0000 Message-id: References: <200102192211.OAA18590@bosch.cygnus.com> X-SW-Source: 2001-02/msg00386.html Michael Elizabeth Chastain 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