Re: [rfa] space reduction in gdbtypes.h

Re: [rfa] space reduction in gdbtypes.h

[Michael Elizabeth Chastain]

Hi Jim,

jimb> - I think it's important that this struct start with something that
jimb>   indicates which sort of type it represents.  It's weird to have the
jimb>   discriminant buried in the middle.  So leave 'code' the first member
jimb>   of the structure, and move the {upper,lower}_bound_type fields up.
jimb> - Add a short comment indicating that the {upper,lower}_bound_type
jimb>   fields pack better at that location.

Okay, I will do that, and re-test, and then commit.

I thought about trying the __packed__ attribute but I am allergic to
__packed__ because I've seen that word in too many bug reports.  My gut
feeling is that hand layout is safer.

This is the last of the "easy re-arrangement" patches.  Next I will
write an essay about the minsyms 'info' pointer, which really bites,
and submit some cleanup patches for that.

Michael C

Re: [rfa] space reduction in gdbtypes.h

[Michael Elizabeth Chastain]

Andrew C says:

>  #define BOUND_BY_REF_IN_REG          2
>  #define BOUND_BY_VALUE_IN_REG        1
>  #define BOUND_SIMPLE                 0
> -  int upper_bound_type;
> -  int lower_bound_type;
> +  int upper_bound_type : 4;
> +  int lower_bound_type : 4;
> 
> Hmm, doesn't this part scream ENUM?

I agree, it does, but I wanted to patch one thing at a time,
so I left it as an int.

This data field is not actually functional at the moment.
There are a few places that test the bounds.  But there is only one
place that sets any bounds at all.  parse.c sets upper_bound_type
to BOUND_CANNOT_BE_DETERMINED in one place.  That's the only
assignment I found.

I suspect we're going to need some functional code in this area
to fix several FORTRAN array bounds PR's.

> Without that the packing is unsafe: 
> adding an extra variant that overflows the field won't be detected; 
> compilers capable of checking enum assignments won't do anything useful.

Well, the definitions are right next to the use, which would make
it obvious.

And I hope that a person who adds new variants would test their new code
at least ONCE to see if the new variants worked!

Would you like me to go ahead and make this an enum right now?

Michael C

Re: [rfa] space reduction in gdbtypes.h

[Andrew Cagney]

> Andrew C says:
> 
> 
>>  #define BOUND_BY_REF_IN_REG          2
>>  #define BOUND_BY_VALUE_IN_REG        1
>>  #define BOUND_SIMPLE                 0
>> -  int upper_bound_type;
>> -  int lower_bound_type;
>> +  int upper_bound_type : 4;
>> +  int lower_bound_type : 4;
>> 
>> Hmm, doesn't this part scream ENUM?
> 
> 
> I agree, it does, but I wanted to patch one thing at a time,
> so I left it as an int.

Try a different ordering of the changes:

- make the cleanups first (no functional change)
- make the functional change last (here reducing the size of the gdb 
footprint)

>> Without that the packing is unsafe: 
>> adding an extra variant that overflows the field won't be detected; 
>> compilers capable of checking enum assignments won't do anything useful.
> 
> 
> Well, the definitions are right next to the use, which would make
> it obvious.

People have this habit of taking shortcuts when ever possible ->  it's 
never obvious :-(

> Would you like me to go ahead and make this an enum right now?

Yes please.  Macro's are bad m'kay.

Andrew

Re: [rfa] space reduction in gdbtypes.h

[Michael Elizabeth Chastain]

Committed, with JimB's requested changes.

Michael C

===

  2003-08-20  Michael Chastain  <mec@shout.net>

	* gdbtypes.h (struct main_type): Rearrange to save space.

[rfa] space reduction in gdbtypes.h

[Michael Elizabeth Chastain]

More space reduction!  This is the last of the low-hanging fruit.

  before: 90001408
  after:  87068672

This is a re-arrangement of 'struct main_type'.

  struct main_type  52  40

Testing: native i686-pc-linux-gnu, gcc v2 and v3, dwarf-2 and stabs+.
Full test suite, not regressions.

Okay to commit?

Michael C

2003-08-20  Michael Chastain  <mec@shout.net>

	* gdbtypes.h (struct main_type): Rearrange to save space.

Index: gdbtypes.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbtypes.h,v
retrieving revision 1.48
diff -u -r1.48 gdbtypes.h
--- gdbtypes.h	23 Jun 2003 21:05:40 -0000	1.48
+++ gdbtypes.h	20 Aug 2003 02:53:46 -0000
@@ -274,10 +274,6 @@
 
 struct main_type
 {
-  /* Code for kind of type */
-
-  enum type_code code;
-
   /* Name of this type, or NULL if none.
 
      This is used for printing only, except by poorly designed C++ code.
@@ -298,6 +294,10 @@
 
   char *tag_name;
 
+  /* Code for kind of type */
+
+  ENUM_BITFIELD(type_code) code : 8;
+
   /* FIXME, these should probably be restricted to a Fortran-specific
      field in some fashion.  */
 #define BOUND_CANNOT_BE_DETERMINED   5
@@ -306,8 +306,8 @@
 #define BOUND_BY_REF_IN_REG          2
 #define BOUND_BY_VALUE_IN_REG        1
 #define BOUND_SIMPLE                 0
-  int upper_bound_type;
-  int lower_bound_type;
+  int upper_bound_type : 4;
+  int lower_bound_type : 4;
 
   /* Every type is now associated with a particular objfile, and the
      type is allocated on the type_obstack for that objfile.  One problem
@@ -340,6 +340,15 @@
 
   short nfields;
 
+  /* Field number of the virtual function table pointer in
+     VPTR_BASETYPE.  If -1, we were unable to find the virtual
+     function table pointer in initial symbol reading, and
+     fill_in_vptr_fieldno should be called to find it if possible.
+
+     Unused if this type does not have virtual functions.  */
+
+  short vptr_fieldno;
+
   /* For structure and union types, a description of each field.
      For set and pascal array types, there is one "field",
      whose type is the domain type of the set or array.
@@ -420,15 +429,6 @@
      Unused otherwise.  */
 
   struct type *vptr_basetype;
-
-  /* Field number of the virtual function table pointer in
-     VPTR_BASETYPE.  If -1, we were unable to find the virtual
-     function table pointer in initial symbol reading, and
-     fill_in_vptr_fieldno should be called to find it if possible.
-
-     Unused if this type does not have virtual functions.  */
-
-  int vptr_fieldno;
 
   /* Slot to point to additional language-specific fields of this type.  */
 

Re: [rfa] space reduction in gdbtypes.h

[Jim Blandy]

Michael Elizabeth Chastain <mec@shout.net> writes:

> More space reduction!  This is the last of the low-hanging fruit.
> 
>   before: 90001408
>   after:  87068672
> 
> This is a re-arrangement of 'struct main_type'.
> 
>   struct main_type  52  40
> 
> Testing: native i686-pc-linux-gnu, gcc v2 and v3, dwarf-2 and stabs+.
> Full test suite, not regressions.
> 
> Okay to commit?

Yes, with some minor changes:

- I think it's important that this struct start with something that
  indicates which sort of type it represents.  It's weird to have the
  discriminant buried in the middle.  So leave 'code' the first member
  of the structure, and move the {upper,lower}_bound_type fields up.

- Add a short comment indicating that the {upper,lower}_bound_type
  fields pack better at that location.

It's a shame that one needs to make things less clear (e.g., moving
vptr_fieldno away from vptr_basetype) in order to save space.  I wish
there were some way to tell the compiler, "I don't care about the
in-memory ordering of these fields, just pack them as tightly as you
can."

Re: [rfa] space reduction in gdbtypes.h

[Andrew Cagney]

>    /* FIXME, these should probably be restricted to a Fortran-specific
>       field in some fashion.  */
>  #define BOUND_CANNOT_BE_DETERMINED   5
> @@ -306,8 +306,8 @@
>  #define BOUND_BY_REF_IN_REG          2
>  #define BOUND_BY_VALUE_IN_REG        1
>  #define BOUND_SIMPLE                 0
> -  int upper_bound_type;
> -  int lower_bound_type;
> +  int upper_bound_type : 4;
> +  int lower_bound_type : 4;

Hmm, doesn't this part scream ENUM?  Without that the packing is unsafe: 
adding an extra variant that overflows the field won't be detected; 
compilers capable of checking enum assignments won't do anything useful.

enjoy,
Andrew