From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9802 invoked by alias); 15 May 2002 18:16:07 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 9776 invoked from network); 15 May 2002 18:16:03 -0000 Received: from unknown (HELO cygnus.com) (205.180.83.203) by sources.redhat.com with SMTP; 15 May 2002 18:16:03 -0000 Received: from localhost.redhat.com (remus.sfbay.redhat.com [172.16.27.252]) by runyon.cygnus.com (8.8.7-cygnus/8.8.7) with ESMTP id LAA06061; Wed, 15 May 2002 11:15:57 -0700 (PDT) Received: by localhost.redhat.com (Postfix, from userid 469) id 609D610FC9; Wed, 15 May 2002 14:15:24 -0400 (EDT) From: Elena Zannoni MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-ID: <15586.42427.974873.473657@localhost.redhat.com> Date: Wed, 15 May 2002 11:16:00 -0000 To: Daniel Jacobowitz Cc: Elena Zannoni , gdb-patches@sources.redhat.com Subject: Re: [RFA] [4/5] Use DWARF-2 DW_AT_artificial information In-Reply-To: <20020515045247.GA14330@nevyn.them.org> References: <20020115153157.A28714@nevyn.them.org> <20020307154311.A19496@nevyn.them.org> <20020514211335.GA11766@nevyn.them.org> <15585.58783.368594.458392@localhost.redhat.com> <20020515045247.GA14330@nevyn.them.org> X-SW-Source: 2002-05/txt/msg00614.txt.bz2 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