Re: [PATCH 2/3] Use type_instance_flags more throughout

[Pedro Alves <pedro@palves.net>]

Hi Luis,

I should have replied to this promptly.  Sorry about that.

On 8/25/20 12:36 PM, Luis Machado wrote:
> Hi,
> 
> On 8/21/20 11:45 AM, Pedro Alves wrote:
>> The next patch in this series will rewrites enum_flags fixing some API
>> holes.  That would cause build failures around code using
>> type_instance_flags.  Or rather, that should be using it, but wasn't.
>>
>> This patch fixes it by using type_instance_flags throughout instead of
>> plain integers.
>>
>> Note that we can't make the seemingly obvious change to struct
>> type::instance_flags:
>>
>>   -  unsigned instance_flags : 9;
>>   +  ENUM_BITFIELD (type_instance_flag_value) instance_flags : 9;
>>
>> Because G++ complains then that 9 bits isn't sufficient for holding
>> all values of type_instance_flag_value.
> 
> I ran into the problem above the last time I used instance flags (address classes), but mostly because of the (rather silly) 9 bit (or whatever current number is there) limitation.
> 
> Isn't this more an artifact of trying to save space where there isn't a need to anymore? Wouldn't a 64-bit integer suffice here? That would give us 64 flags worth of data and virtually no problems until we ran out of bits.
> 

Well, I'm not exactly sure about struct type, but symbol related
structures are space sensitive, a few bytes here and there can make
a significant difference when you have thousands or millions of 
instances.  Once in a while someone tries to shrink these structures,
see if we can pack fields better by reordering fields, etc.

Here's what we have today, on x86-64:

 (top-gdb) ptype /o type
 /* offset    |  size */  type = struct type {
 /*    0      |     8 */    type *pointer_type;
 /*    8      |     8 */    type *reference_type;
 /*   16      |     8 */    type *rvalue_reference_type;
 /*   24      |     8 */    type *chain;
 /*   32: 0   |     4 */    unsigned int align_log2 : 8;
 /*   33: 0   |     4 */    unsigned int instance_flags : 9;
 /* XXX  7-bit hole   */
 /* XXX  5-byte hole  */
 /*   40      |     8 */    ULONGEST length;
 /*   48      |     8 */    main_type *main_type;
 
                            /* total size (bytes):   56 */
                          }

And if we blindly made instance_flags 64-bit:

 (top-gdb) ptype /o type
 /* offset    |  size */  type = struct type {
 /*    0      |     8 */    type *pointer_type;
 /*    8      |     8 */    type *reference_type;
 /*   16      |     8 */    type *rvalue_reference_type;
 /*   24      |     8 */    type *chain;
 /*   32: 0   |     4 */    unsigned int align_log2 : 8;
 /* XXX  7-byte hole  */
 /*   40      |     8 */    ULONGEST instance_flags;
 /*   48      |     8 */    ULONGEST length;
 /*   56      |     8 */    main_type *main_type;
 
                            /* total size (bytes):   64 */
                          }

Maybe we will need 64 bits for flags in the future anyway, I don't
know.

> Honestly, I don't particularly like the hackery that is going on in traits.h, just to catch something that may be easier fixed with a longer integer type.
> 
> I understand the C++ standard is evolving, but GDB's code base is still full of old code. We don't necessarily need to convert GDB's code base to use the latest and greatest out there, right? We just need to make it good quality and well maintained?

The "hackery" is basically switching to the detection idiom that's now standard in C++17.  We
can remove our implementation in traits.h once we start requiring C++17.  But the "hackery" is
used in the enum flags unit tests -- even if instance_flags was made a plain 64-bits enum, I'd
still want the "hackery".  It's orthogonal to instance_flags.  This patch could be the first
in the series, before the traits.h change.  In fact, I'll do just that.

> 
> Just an opinion from someone that has used these flags before and has been bitten by their storage-space-saving ugliness.
>