Re: [RFA] New bitflags type and eflags on i386/x86-64

[Andrew Cagney <ac131313@ges.redhat.com>]

>    Date: Thu, 05 Sep 2002 10:04:11 +0200
>    From: Michal Ludvig <mludvig@suse.cz>
> 
>    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 <lang>_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
>