Re: RFA/types: Clean up use of field bitsize

[Andrew Cagney <ac131313@redhat.com>]

> Does anyone have a comment on this patch?  If not, I'll commit it in a
> couple of days, after I'm added to the global write list.
> 
> (The type code has no specific maintainer, the debug reader and
> language parts I consider obvious, and the patch is over a month old
> now.)

I'm mainly wondering if we're that desperate for memory space.

I thought a data structure was added to GDB so that it could spot 
duplicate type info and, hence, keep its memory size down.

Andrew


>> Right now, we have this really disturbing comment:
>> 
>>      /* Size of this field, in bits, or zero if not packed.
>>         For an unpacked field, the field's type's length
>>         says how many bytes the field occupies.
>>         A value of -1 or -2 indicates a static field;  -1 means the location
>>         is specified by the label loc.physname;  -2 means that loc.physaddr
>>         specifies the actual address. */
>> 
>> Think about this for a moment.  While in practice a static member is never
>> going to be packed, and in at least C++ can not be a bit-field, that's not
>> logically obvious for other languages.  I don't know Ada but I wouldn't be
>> surprised if there were some construct which violated this assumption.
>> 
>> Worse, all sorts of places don't check for negative bitsize at all.  It may
>> be that they're all safe - I didn't spend a lot of time working out problem
>> cases - but I have my doubts.
>> 
>> So, since I needed to gain a new field here anyway, and since I have no
>> compunctions about shrinking this field a little (packed bitfields of size
>> greater than a couple of words are allowed in some languages IIRC (including
>> GNU C maybe?  Although they are not allowed in ISO C99), but they're
>> definitely dodgy), and since signed bitfields are not portable, I cleaned up
>> the construct.  It turned out to be painless except for making sure symbol
>> readers initialized it, which was a little tedious.
>> 
>> This patch:
>>   Moves 'artificial' out from 'loc' and makes it a bitfield
>>   Creates a 'static_kind' bitfield
>>   Makes 'bitsize' into a bitfield
>> 
>> The goal is to allow more kinds of fields to be marked artificial -
>> particularly data members.  After this patch I'll submit the followup to
>> mark DW_AT_artificial members as artificial types.
>> 
>> OK?
>> 
>> -- 
>> Daniel Jacobowitz
>> MontaVista Software                         Debian GNU/Linux Developer
>> 
>> 2002-09-29  Daniel Jacobowitz  <drow@mvista.com>
>> 
>> 	* gdbtypes.h (struct main_type): Move artificial flag out of
>> 	loc.  New member of ``struct field'' named static_kind.  Reduce
>> 	overloaded meaning of bitsize.
>> 	(FIELD_ARTIFICIAL, SET_FIELD_PHYSNAME, SET_FIELD_PHYSADDR)
>> 	(TYPE_FIELD_STATIC, TYPE_FIELD_STATIC_HAS_ADDR): Likewise.
>> 	(FIELD_STATIC_KIND, TYPE_FIELD_STATIC_KIND): New macros.
>> 
>> 	* ada-lang.c (fill_in_ada_prototype): Initialize static_kind for
>> 	new fields.
>> 	(template_to_fixed_record_type, template_to_static_fixed_type)
>> 	(to_record_with_fixed_variant_part): Likewise.
>> 	* coffread.c (coff_read_struct_type, coff_read_enum_type): Likewise.
>> 	* dwarf2read.c (dwarf2_add_field, read_enumeration): Likewise.
>> 	* dwarfread.c (struct_type, enum_type): Likewise.
>> 	* hpread.c (hpread_read_enum_type)
>> 	(hpread_read_function_type, hpread_read_doc_function_type)
>> 	(hpread_read_struct_type): Likewise.
>> 	* mdebugread.c (parse_symbol): Likewise.
>> 
>> Index: ada-lang.c
>> ===================================================================
>> RCS file: /cvs/src/src/gdb/ada-lang.c,v
>> retrieving revision 1.9
>> diff -u -p -r1.9 ada-lang.c
>> --- ada-lang.c	8 Sep 2002 17:43:26 -0000	1.9
>> +++ ada-lang.c	30 Sep 2002 00:43:30 -0000
>> @@ -4189,6 +4189,7 @@ fill_in_ada_prototype (struct symbol *fu
>>        case LOC_REGPARM_ADDR:
>>  	TYPE_FIELD_BITPOS (ftype, nargs) = nargs;
>>  	TYPE_FIELD_BITSIZE (ftype, nargs) = 0;
>> +	TYPE_FIELD_STATIC_KIND (ftype, nargs) = 0;
>>  	TYPE_FIELD_TYPE (ftype, nargs) =
>>  	  lookup_pointer_type (check_typedef (SYMBOL_TYPE (sym)));
>>  	TYPE_FIELD_NAME (ftype, nargs) = SYMBOL_NAME (sym);
>> @@ -4202,6 +4203,7 @@ fill_in_ada_prototype (struct symbol *fu
>>        case LOC_BASEREG_ARG:
>>  	TYPE_FIELD_BITPOS (ftype, nargs) = nargs;
>>  	TYPE_FIELD_BITSIZE (ftype, nargs) = 0;
>> +	TYPE_FIELD_STATIC_KIND (ftype, nargs) = 0;
>>  	TYPE_FIELD_TYPE (ftype, nargs) = check_typedef (SYMBOL_TYPE (sym));
>>  	TYPE_FIELD_NAME (ftype, nargs) = SYMBOL_NAME (sym);
>>  	nargs += 1;
>> @@ -6046,6 +6048,7 @@ template_to_fixed_record_type (struct ty
>>         * rediscover why we needed field_offset and fix it properly. */
>>        TYPE_FIELD_BITPOS (rtype, f) = off;
>>        TYPE_FIELD_BITSIZE (rtype, f) = 0;
>> +      TYPE_FIELD_STATIC_KIND (rtype, f) = 0;
>>  
>>        if (ada_is_variant_part (type, f))
>>  	{
>> @@ -6149,6 +6152,7 @@ template_to_static_fixed_type (struct ty
>>      {
>>        TYPE_FIELD_BITPOS (type, f) = 0;
>>        TYPE_FIELD_BITSIZE (type, f) = 0;
>> +      TYPE_FIELD_STATIC_KIND (type, f) = 0;
>>  
>>        if (is_dynamic_field (templ_type, f))
>>  	{
>> @@ -6218,6 +6222,7 @@ to_record_with_fixed_variant_part (struc
>>        TYPE_FIELD_TYPE (rtype, nfields - 1) = branch_type;
>>        TYPE_FIELD_NAME (rtype, nfields - 1) = "S";
>>        TYPE_FIELD_BITSIZE (rtype, nfields - 1) = 0;
>> +      TYPE_FIELD_STATIC_KIND (rtype, nfields - 1) = 0;
>>        TYPE_LENGTH (rtype) += TYPE_LENGTH (branch_type);
>>        -TYPE_LENGTH (TYPE_FIELD_TYPE (type, nfields - 1));
>>      }
>> Index: coffread.c
>> ===================================================================
>> RCS file: /cvs/src/src/gdb/coffread.c,v
>> retrieving revision 1.29
>> diff -u -p -r1.29 coffread.c
>> --- coffread.c	22 Aug 2002 05:50:11 -0000	1.29
>> +++ coffread.c	30 Sep 2002 00:43:30 -0000
>> @@ -1997,6 +1997,7 @@ coff_read_struct_type (int index, int le
>>  	  FIELD_TYPE (list->field) = decode_type (ms, ms->c_type, &sub_aux);
>>  	  FIELD_BITPOS (list->field) = 8 * ms->c_value;
>>  	  FIELD_BITSIZE (list->field) = 0;
>> +	  FIELD_STATIC_KIND (list->field) = 0;
>>  	  nfields++;
>>  	  break;
>>  
>> @@ -2015,6 +2016,7 @@ coff_read_struct_type (int index, int le
>>  	  FIELD_TYPE (list->field) = decode_type (ms, ms->c_type, &sub_aux);
>>  	  FIELD_BITPOS (list->field) = ms->c_value;
>>  	  FIELD_BITSIZE (list->field) = sub_aux.x_sym.x_misc.x_lnsz.x_size;
>> +	  FIELD_STATIC_KIND (list->field) = 0;
>>  	  nfields++;
>>  	  break;
>>  
>> @@ -2135,6 +2137,7 @@ coff_read_enum_type (int index, int leng
>>  	  if (SYMBOL_VALUE (xsym) < 0)
>>  	    unsigned_enum = 0;
>>  	  TYPE_FIELD_BITSIZE (type, n) = 0;
>> +	  TYPE_FIELD_STATIC_KIND (type, n) = 0;
>>  	}
>>        if (syms == osyms)
>>  	break;
>> Index: dwarf2read.c
>> ===================================================================
>> RCS file: /cvs/src/src/gdb/dwarf2read.c,v
>> retrieving revision 1.66
>> diff -u -p -r1.66 dwarf2read.c
>> --- dwarf2read.c	3 Sep 2002 17:32:11 -0000	1.66
>> +++ dwarf2read.c	30 Sep 2002 00:43:33 -0000
>> @@ -2055,6 +2055,8 @@ dwarf2_add_field (struct field_info *fip
>>        /* Get type of field.  */
>>        fp->type = die_type (die, objfile, cu_header);
>>  
>> +      FIELD_STATIC_KIND (*fp) = 0;
>> +
>>        /* Get bit size of field (zero if none).  */
>>        attr = dwarf_attr (die, DW_AT_bit_size);
>>        if (attr)
>> @@ -2163,6 +2165,7 @@ dwarf2_add_field (struct field_info *fip
>>  	FIELD_BITPOS (*fp) = (decode_locdesc (DW_BLOCK (attr), objfile, cu_header)
>>  			      * bits_per_byte);
>>        FIELD_BITSIZE (*fp) = 0;
>> +      FIELD_STATIC_KIND (*fp) = 0;
>>        FIELD_TYPE (*fp) = die_type (die, objfile, cu_header);
>>        FIELD_NAME (*fp) = type_name_no_tag (fp->type);
>>        fip->nbaseclasses++;
>> @@ -2667,6 +2670,7 @@ read_enumeration (struct die_info *die, 
>>  		  FIELD_TYPE (fields[num_fields]) = NULL;
>>  		  FIELD_BITPOS (fields[num_fields]) = SYMBOL_VALUE (sym);
>>  		  FIELD_BITSIZE (fields[num_fields]) = 0;
>> +		  FIELD_STATIC_KIND (fields[num_fields]) = 0;
>>  
>>  		  num_fields++;
>>  		}
>> Index: dwarfread.c
>> ===================================================================
>> RCS file: /cvs/src/src/gdb/dwarfread.c,v
>> retrieving revision 1.14
>> diff -u -p -r1.14 dwarfread.c
>> --- dwarfread.c	1 Aug 2002 17:18:32 -0000	1.14
>> +++ dwarfread.c	30 Sep 2002 00:43:33 -0000
>> @@ -1027,6 +1027,7 @@ struct_type (struct dieinfo *dip, char *
>>  			  &objfile->type_obstack);
>>  	  FIELD_TYPE (list->field) = decode_die_type (&mbr);
>>  	  FIELD_BITPOS (list->field) = 8 * locval (&mbr);
>> +	  FIELD_STATIC_KIND (list->field) = 0;
>>  	  /* Handle bit fields. */
>>  	  FIELD_BITSIZE (list->field) = mbr.at_bit_size;
>>  	  if (BITS_BIG_ENDIAN)
>> @@ -1694,6 +1695,7 @@ enum_type (struct dieinfo *dip, struct o
>>  	  list = new;
>>  	  FIELD_TYPE (list->field) = NULL;
>>  	  FIELD_BITSIZE (list->field) = 0;
>> +	  FIELD_STATIC_KIND (list->field) = 0;
>>  	  FIELD_BITPOS (list->field) =
>>  	    target_to_host (scan, TARGET_FT_LONG_SIZE (objfile), GET_SIGNED,
>>  			    objfile);
>> Index: gdbtypes.h
>> ===================================================================
>> RCS file: /cvs/src/src/gdb/gdbtypes.h,v
>> retrieving revision 1.36
>> diff -u -p -r1.36 gdbtypes.h
>> --- gdbtypes.h	14 Sep 2002 02:09:39 -0000	1.36
>> +++ gdbtypes.h	30 Sep 2002 00:43:34 -0000
>> @@ -381,22 +381,25 @@ struct main_type
>>  
>>        CORE_ADDR physaddr;
>>        char *physname;
>> -
>> -      /* For a function or member type, this is 1 if the argument is marked
>> -	 artificial.  Artificial arguments should not be shown to the
>> -	 user.  */
>> -      int artificial;
>>      }
>>      loc;
>>  
>> +    /* For a function or member type, this is 1 if the argument is marked
>> +       artificial.  Artificial arguments should not be shown to the
>> +       user.  */
>> +    unsigned int artificial : 1;
>> +
>> +    /* This flag is zero for non-static fields, 1 for fields whose location
>> +       is specified by the label loc.physname, and 2 for fields whose location
>> +       is specified by loc.physaddr.  */
>> +
>> +    unsigned int static_kind : 2;
>> +
>>      /* Size of this field, in bits, or zero if not packed.
>>         For an unpacked field, the field's type's length
>> -       says how many bytes the field occupies.
>> -       A value of -1 or -2 indicates a static field;  -1 means the location
>> -       is specified by the label loc.physname;  -2 means that loc.physaddr
>> -       specifies the actual address. */
>> +       says how many bytes the field occupies.  */
>>  
>> -    int bitsize;
>> +    unsigned int bitsize : 29;
>>  
>>      /* In a struct or union type, type of this field.
>>         In a function or member type, type of this argument.
>> @@ -793,14 +796,15 @@ extern void allocate_cplus_struct_type (
>>  #define FIELD_TYPE(thisfld) ((thisfld).type)
>>  #define FIELD_NAME(thisfld) ((thisfld).name)
>>  #define FIELD_BITPOS(thisfld) ((thisfld).loc.bitpos)
>> -#define FIELD_ARTIFICIAL(thisfld) ((thisfld).loc.artificial)
>> +#define FIELD_ARTIFICIAL(thisfld) ((thisfld).artificial)
>>  #define FIELD_BITSIZE(thisfld) ((thisfld).bitsize)
>> +#define FIELD_STATIC_KIND(thisfld) ((thisfld).static_kind)
>>  #define FIELD_PHYSNAME(thisfld) ((thisfld).loc.physname)
>>  #define FIELD_PHYSADDR(thisfld) ((thisfld).loc.physaddr)
>>  #define SET_FIELD_PHYSNAME(thisfld, name) \
>> -  ((thisfld).bitsize = -1, FIELD_PHYSNAME(thisfld) = (name))
>> +  ((thisfld).static_kind = 1, FIELD_PHYSNAME(thisfld) = (name))
>>  #define SET_FIELD_PHYSADDR(thisfld, name) \
>> -  ((thisfld).bitsize = -2, FIELD_PHYSADDR(thisfld) = (name))
>> +  ((thisfld).static_kind = 2, FIELD_PHYSADDR(thisfld) = (name))
>>  #define TYPE_FIELD(thistype, n) TYPE_MAIN_TYPE(thistype)->fields[n]
>>  #define TYPE_FIELD_TYPE(thistype, n) FIELD_TYPE(TYPE_FIELD(thistype, n))
>>  #define TYPE_FIELD_NAME(thistype, n) FIELD_NAME(TYPE_FIELD(thistype, n))
>> @@ -840,8 +844,9 @@ extern void allocate_cplus_struct_type (
>>    (TYPE_CPLUS_SPECIFIC(thistype)->virtual_field_bits == NULL ? 0 \
>>      : B_TST(TYPE_CPLUS_SPECIFIC(thistype)->virtual_field_bits, (n)))
>>  
>> -#define TYPE_FIELD_STATIC(thistype, n) (TYPE_MAIN_TYPE (thistype)->fields[n].bitsize < 0)
>> -#define TYPE_FIELD_STATIC_HAS_ADDR(thistype, n) (TYPE_MAIN_TYPE (thistype)->fields[n].bitsize == -2)
>> +#define TYPE_FIELD_STATIC(thistype, n) (TYPE_MAIN_TYPE (thistype)->fields[n].static_kind != 0)
>> +#define TYPE_FIELD_STATIC_KIND(thistype, n) TYPE_MAIN_TYPE (thistype)->fields[n].static_kind
>> +#define TYPE_FIELD_STATIC_HAS_ADDR(thistype, n) (TYPE_MAIN_TYPE (thistype)->fields[n].static_kind == 2)
>>  #define TYPE_FIELD_STATIC_PHYSNAME(thistype, n) FIELD_PHYSNAME(TYPE_FIELD(thistype, n))
>>  #define TYPE_FIELD_STATIC_PHYSADDR(thistype, n) FIELD_PHYSADDR(TYPE_FIELD(thistype, n))
>>  
>> Index: hpread.c
>> ===================================================================
>> RCS file: /cvs/src/src/gdb/hpread.c,v
>> retrieving revision 1.22
>> diff -u -p -r1.22 hpread.c
>> --- hpread.c	4 Aug 2002 19:11:12 -0000	1.22
>> +++ hpread.c	30 Sep 2002 00:43:35 -0000
>> @@ -3186,6 +3186,7 @@ hpread_read_enum_type (dnttpointer hp_ty
>>  	  TYPE_FIELD_NAME (type, n) = SYMBOL_NAME (xsym);
>>  	  TYPE_FIELD_BITPOS (type, n) = SYMBOL_VALUE (xsym);
>>  	  TYPE_FIELD_BITSIZE (type, n) = 0;
>> +	  TYPE_FIELD_STATIC_KIND (type, n) = 0;
>>  	}
>>        if (syms == osyms)
>>  	break;
>> @@ -3347,6 +3348,7 @@ hpread_read_function_type (dnttpointer h
>>  	  TYPE_FIELD_TYPE (type, n) = SYMBOL_TYPE (xsym);
>>  	  TYPE_FIELD_ARTIFICIAL (type, n) = 0;
>>  	  TYPE_FIELD_BITSIZE (type, n) = 0;
>> +	  TYPE_FIELD_STATIC_KIND (type, n) = 0;
>>  	}
>>      }
>>    /* Mark it as having been processed */
>> @@ -3520,6 +3522,7 @@ hpread_read_doc_function_type (dnttpoint
>>  	  TYPE_FIELD_TYPE (type, n) = SYMBOL_TYPE (xsym);
>>  	  TYPE_FIELD_ARTIFICIAL (type, n) = 0;
>>  	  TYPE_FIELD_BITSIZE (type, n) = 0;
>> +	  TYPE_FIELD_STATIC_KIND (type, n) = 0;
>>  	}
>>      }
>>  
>> @@ -3704,6 +3707,7 @@ hpread_read_struct_type (dnttpointer hp_
>>  	  list = new;
>>  
>>  	  FIELD_BITSIZE (list->field) = 0;
>> +	  FIELD_STATIC_KIND (list->field) = 0;
>>  
>>  	  /* The "classname" field is actually a DNTT pointer to the base class */
>>  	  baseclass = hpread_type_lookup (parentp->dinheritance.classname,
>> @@ -4101,6 +4105,7 @@ hpread_read_struct_type (dnttpointer hp_
>>  	      list->field.name = VT (objfile) + fn_fieldp->dsvar.name;
>>  	      FIELD_BITPOS (list->field) = 0;	/* FIXME is this always true? */
>>  	      FIELD_BITSIZE (list->field) = 0;	/* use length from type */
>> +	      FIELD_STATIC_KIND (list->field) = 0;
>>  	      memtype = hpread_type_lookup (fn_fieldp->dsvar.type, objfile);
>>  	      list->field.type = memtype;
>>  	      list->attributes = 0;
>> @@ -4120,6 +4125,7 @@ hpread_read_struct_type (dnttpointer hp_
>>  	      list->field.name = VT (objfile) + fn_fieldp->ddvar.name;
>>  	      FIELD_BITPOS (list->field) = 0;	/* FIXME is this always true? */
>>  	      FIELD_BITSIZE (list->field) = 0;	/* use length from type */
>> +	      FIELD_STATIC_KIND (list->field) = 0;
>>  	      memtype = hpread_type_lookup (fn_fieldp->ddvar.type, objfile);
>>  	      list->field.type = memtype;
>>  	      list->attributes = 0;
>> @@ -4168,6 +4174,7 @@ hpread_read_struct_type (dnttpointer hp_
>>  
>>  
>>  	  /* A FIELD by itself (without a GENFIELD) can also be a static member */
>> +	  FIELD_STATIC_KIND (list->field) = 0;
>>  	  if (fieldp->dfield.staticmem)
>>  	    {
>>  	      FIELD_BITPOS (list->field) = -1;
>> Index: mdebugread.c
>> ===================================================================
>> RCS file: /cvs/src/src/gdb/mdebugread.c,v
>> retrieving revision 1.29
>> diff -u -p -r1.29 mdebugread.c
>> --- mdebugread.c	18 Sep 2002 20:47:39 -0000	1.29
>> +++ mdebugread.c	30 Sep 2002 00:43:36 -0000
>> @@ -1092,6 +1092,7 @@ parse_symbol (SYMR *sh, union aux_ext *a
>>  		FIELD_TYPE (*f) = t;
>>  		FIELD_NAME (*f) = debug_info->ss + cur_fdr->issBase + tsym.iss;
>>  		FIELD_BITSIZE (*f) = 0;
>> +		FIELD_STATIC_KIND (*f) = 0;
>>  
>>  		enum_sym = ((struct symbol *)
>>  			    obstack_alloc (&current_objfile->symbol_obstack,
>> @@ -1284,6 +1285,7 @@ parse_symbol (SYMR *sh, union aux_ext *a
>>        bitsize = 0;
>>        FIELD_TYPE (*f) = parse_type (cur_fd, ax, sh->index, &bitsize, bigend, name);
>>        FIELD_BITSIZE (*f) = bitsize;
>> +      FIELD_STATIC_KIND (*f) = 0;
>>        break;
>>  
>>      case stIndirect:		/* forward declaration on Irix5 */
>> 
> 
> 
> -- Daniel Jacobowitz MontaVista Software Debian GNU/Linux Developer