Re: [RFA] [4/5] Use DWARF-2 DW_AT_artificial information

[Elena Zannoni <ezannoni at redhat dot com>]

Daniel Jacobowitz writes:
 > On Wed, May 15, 2002 at 12:35:43AM -0400, Elena Zannoni wrote:
 > > Daniel Jacobowitz writes:
 > >  > I hate to be a nag, but this patch would be useful for some of my
 > >  > current work.  Do you have a chance to look at it?
 > >  > 
 > > 
 > > You are completely right, sorry.  I wonder if MichaelC could kindly
 > > run it through his test harness. That was a big help with yesterday's
 > > patch.  And if you can run the tests with the maintainers file script.
 > 
 > (Don't bother, Michael, it doesn't apply any more.)
 > 
 > I looked at this in passing and noticed it touched some of the same
 > files I was working on.  I completely forgot that my previous patches
 > completely invalidated it :)  Sorry for wasting your time.
 > 

No prob.

 > > How much does the size increase by adding this new struct?
 > 
 > I don't have any numbers, unfortunately.  I'll try to get some, unless
 > I think of a better way to do it...
 > 
 > > I have looked at it when you first posted it, and I had some
 > > questions, I have to go fish for them again.  But basically, the
 > > motivation for this change is what? You need to handle the dwarf2
 > > information for artificial arguments, right? So that needs a change in
 > > dwarf2read.c.  How does that bring about the change in the type
 > > structure? Can you explain a bit? (my brain gets lazy at this time).
 > 
 > OK, let me explain this a bit.  Right now the only information we save
 > for a method type are some flags, the return type, and a list of
 > argument types - just as an array of struct type *.  I needed another
 > bit per argument, and I couldn't find anywhere to put it.  Maintaining
 > a separate bitmap is even uglier.
 > 

How does the type structure look right now for a c++ class method?

There is a TYPE struct for the class. 

The TYPE_SPECIFIC field points to CPLUS_STUFF which has an array of
unique methods (FN_FIELDLISTS).

For each of these methods there is another list (FN_FIELDS) of all the
overloaded methods with the same name (which may be just 1 if not
overloaded).

For each of them there is an array of arguments (ARGS). (which is just
a type struct).

You need to add something to this array of ARGS to indicate if they
are artificial.

There is already an occurrence of 'artificial' but that's for methods.

Yes?

The thing I don't understand in your patch (or is this messy code in
general) is why was type_specific.arg_types changed to
type_specific.method_args.  Wouldn't it be enough to change the args
array inside the fn_field structure from an array of types to an array
of method_args?  Basically I don't think I understand why the type
system seems to be storing this info in 2 places:

in the cplus_struct:
            /* The argument list.  Only valid if is_stub is clear.  Contains
               the type of each argument, including `this', and ending with
               a NULL pointer after the last argument.  Should not contain
               a `this' pointer for static member functions.  */

            struct type **args;

in type_specific:
        /* ARG_TYPES is for TYPE_CODE_METHOD.
           Contains the type of each argument, ending with a void type
           after the last argument for normal member functions or a NULL
           pointer after the last argument for functions with variable
           arguments.  */

        struct type **arg_types;

Argh!!!
This occurrence of args in fn_field was *removed* in 1996 by Peter Schauer,
and *replaced* by arg_types. 
But guess what?? The HP merge reintroduced the args, forgetting about the
existance of arg_types.

Grrrr. No wonder I didn't understand the layout.  Ok, let's get rid of
the cplus_struct args again, I would suggest, and stick with the other.
This may require a bit of careful surgery. Proably uses of args have
propagated all over the place.

Then you would have only one place to modify for the artificial attribute.
I bet your patch would become much much simpler.


 > > In the meantime, I noticed that the hp-symtab-read.c changes are not
 > > mentioned in the ChangeLog (well, now it is hpread.c).  Also now you
 > > have a bunch of type->code instead of TYPE_CODE().  I tried to apply
 > > the patch to the current sources, but it failed in several files.  I
 > > tried to resolve the conflicts by hand but the compilation died in
 > > valops.c, gdbypes.c and c-typeprint.c. I think the rename of type to
 > > main_type needs to be taken into account as well.
 > 
 > > I get these 2 kinds of errors:
 > > /home/ezannoni/sources/src/gdb/c-typeprint.c:171: structure has no member named `code'
 > > /home/ezannoni/sources/src/gdb/gdbtypes.c:2750: structure has no member named `type_specific'
 > 
 > Yep, those are symptomatic of missing accessor macros.  Those used to
 > be members of struct type.
 > 
 > 
 > > I noticed this quite ugly syntax. I know you didn't introduce it, but,
 > > I wonder why it's needed:
 > >  't1[!staticp].type' and 'for (i = !staticp;....)'
 > 
 > Basically, it is just shorthand for "the first non-THIS argument".  I
 > don't know who introduced it; one of the past C++ maintainers, possibly
 > Tiemann, seems to have been very fond of that syntax.  I'm killing it
 > where I run into it.
 > 

You can just submit a cleanup patch for that separately. I would consider it
fairly obvious.

 > > I think you also need to update a few comments in gdbtypes.h when
 > > introducing the new method_args field.
 > > 
 > > Is there any better naming scheme for TYPE_FN_FIELD_ARGS and
 > > TYPE_FN_FIELD_ARG? I am always a bit worried when identifiers differ
 > > by just one char.
 > 
 > Before I clean this up, do you have any better ideas on where to record
 > the new flag?  Maybe a separate bitmap would be better after all.  Less
 > intrusive, certainly.  By the way, I intend to use this flag from stabs
 > also; the C++ ABI can tell us when the int used to control
 > constructors/destructors is artificial with acceptably high accuracy, I
 > think.  That'll improve our display of them somewhat.
 > 

If you get rid of the second instance of args, it would become easier.

Elena