From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 912 invoked by alias); 10 Sep 2002 03:35:27 -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 905 invoked from network); 10 Sep 2002 03:35:24 -0000 Received: from unknown (HELO localhost.redhat.com) (24.112.240.27) by sources.redhat.com with SMTP; 10 Sep 2002 03:35:24 -0000 Received: from ges.redhat.com (localhost [127.0.0.1]) by localhost.redhat.com (Postfix) with ESMTP id AB9293F43; Mon, 9 Sep 2002 23:35:19 -0400 (EDT) Message-ID: <3D7D6877.8050403@ges.redhat.com> Date: Mon, 09 Sep 2002 20:35:00 -0000 From: Andrew Cagney User-Agent: Mozilla/5.0 (X11; U; NetBSD macppc; en-US; rv:1.0.0) Gecko/20020824 X-Accept-Language: en-us, en MIME-Version: 1.0 To: Mark Kettenis Cc: mludvig@suse.cz, drow@mvista.com, gdb-patches@sources.redhat.com, ac131313@redhat.com Subject: Re: [RFA] New bitflags type and eflags on i386/x86-64 References: <3D6BF1D5.70409@ges.redhat.com> <3D6E231D.8060906@suse.cz> <20020829142120.GA5176@nevyn.them.org> <3D6E3666.7070309@suse.cz> <20020829150833.GA29973@nevyn.them.org> <3D6E40EE.5000904@ges.redhat.com> <4.2.0.58.20020830155945.00a473b0@ics.u-strasbg.fr> <3D6F81F5.2040002@suse.cz> <4.2.0.58.20020830164921.01c6fba8@ics.u-strasbg.fr> <3D747E37.7060005@suse.cz> <20020903123025.GA22555@nevyn.them.org> <3D770FFB.5030802@suse.cz> <200209061959.g86JxhK0000467@elgar.kettenis.dyndns.org> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2002-09/txt/msg00141.txt.bz2 > Date: Thu, 05 Sep 2002 10:04:11 +0200 > From: Michal Ludvig > > Daniel Jacobowitz wrote: > > On Tue, Sep 03, 2002 at 11:17:43AM +0200, Michal Ludvig wrote: > >>Thanks for help. Attached file incorporates your patch as well. Could > >>anyone approve it, please? > > > > Can't approve it, but I like it. Thanks for following this through. > > Who can approve? Andrew? MarkK? Someone else? > > Well, I'm not too happy about the introduction of i386-common-tdep.c. > I mean, those functions belong in i386-tdep.c. Can we postpone > integrating this patch until I've unified the i386 and x86-64 targets > into one truly multi-arched one? I've started working on that now, > honestly :-). > > Anyway, before approving this I'd like to see a bit more consensus > about the BitFlags type your patch introduces. Andrew, is it OK with > you now? Daniel was the one with concerns, I believe they were addressed. Michael, what does a user interaction where values are set/cleared look like? It should be pretty obvious without needing to look at a manual. Andrew > There also some coding-style problems with your code: > > Index: gdbtypes.c > =================================================================== > RCS file: /cvs/src/src/gdb/gdbtypes.c,v > retrieving revision 1.56 > diff -u -p -r1.56 gdbtypes.c > --- gdbtypes.c 20 Aug 2002 19:57:32 -0000 1.56 > +++ gdbtypes.c 30 Aug 2002 13:42:50 -0000 > @@ -782,6 +782,61 @@ create_set_type (struct type *result_typ > return (result_type); > } > > +/* > + * - The following three functions are intended to be used for BitFlags > + * types (e.g. i386's EFLAGS register). > + * - A BitFlags type is an integer where bits may have a symbolic name > + * to be printed when the bit is set. > + * - Printing is done in _val_print() under a TYPE_CODE_FLAGS label. > + * - Add symbolic names for relevant bits using add_flag_name() after > + * initializing the BitFlags type. > + */ > > That's not the right comment style. > > +void > +add_flag_ignore (struct type *type, int bitpos) > +{ > + TYPE_FIELD_BITPOS (type, bitpos) = -1; > +} > > Indentation is wrong here... > > +void > +add_flag_name (struct type *type, int bitpos, char *name) > +{ > + int namelen; > + > + gdb_assert (TYPE_CODE (type) == TYPE_CODE_FLAGS); > + gdb_assert (bitpos < TYPE_NFIELDS (type)); > + gdb_assert (bitpos >= 0); > + > + namelen=strlen(name)+1; > + TYPE_FIELD_NAME (type, bitpos) = xmalloc (namelen); > + snprintf(TYPE_FIELD_NAME (type, bitpos), namelen, "%s", name); > + > + TYPE_FIELD_BITPOS (type, bitpos) = bitpos; > +} > > ...and here. > > +struct type * > +init_flags_type (int bitlength, char *name, struct objfile *objfile) > +{ > + register struct type *type; > > We're trying to get rid of "register". > > Index: gdbtypes.h > =================================================================== > RCS file: /cvs/src/src/gdb/gdbtypes.h,v > retrieving revision 1.35 > diff -u -p -r1.35 gdbtypes.h > --- gdbtypes.h 10 Aug 2002 05:12:40 -0000 1.35 > +++ gdbtypes.h 30 Aug 2002 13:42:51 -0000 > @@ -1054,6 +1056,14 @@ extern struct type *alloc_type (struct o > > extern struct type *init_type (enum type_code, int, int, char *, > struct objfile *); > + > +/* Helper functions to construct BitField type. > + See description in gdbarch.c for details. */ > > Are you sure this reference to gdbarch.c is still correct? > > Mark >