Increase size of main_type::nfields

  main_type::nfields is a 'short', and has been for many years.  PR
c++/29985 points out that 'short' is too narrow for an enum that
contains more than 2^15 constants.

This patch bumps the size of 'nfields'.  To verify that the field
isn't directly used, it is also renamed.  Note that this does not
affect the size of main_type on x86-64 Fedora 36.  And, if it does
have a negative effect somewhere, it's worth considering that types
could be shrunk more drastically by using subclasses for the different
codes.

I wasn't sure whether a test case for this would be desirable.  It
would be a bit large.
---
 gdb/gdb-gdb.py.in | 4 ++--
 gdb/gdbtypes.h    | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)
  

On 1/11/23 15:44, Tom Tromey via Gdb-patches wrote:
> main_type::nfields is a 'short', and has been for many years.  PR
> c++/29985 points out that 'short' is too narrow for an enum that
> contains more than 2^15 constants.
> 
> This patch bumps the size of 'nfields'.  To verify that the field
> isn't directly used, it is also renamed.  Note that this does not
> affect the size of main_type on x86-64 Fedora 36.  And, if it does
> have a negative effect somewhere, it's worth considering that types
> could be shrunk more drastically by using subclasses for the different
> codes.
> 
> I wasn't sure whether a test case for this would be desirable.  It
> would be a bit large.

We could make a test case that generates a source file on the fly in
standard_output_directory and compiles it.

> ---
>  gdb/gdb-gdb.py.in | 4 ++--
>  gdb/gdbtypes.h    | 6 +++---
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/gdb/gdb-gdb.py.in b/gdb/gdb-gdb.py.in
> index dbc4d773e0b..95b7d84966f 100644
> --- a/gdb/gdb-gdb.py.in
> +++ b/gdb/gdb-gdb.py.in
> @@ -261,8 +261,8 @@ class StructMainTypePrettyPrinter:
>          fields.append("flags = [%s]" % self.flags_to_string())
>          fields.append("owner = %s" % self.owner_to_string())
>          fields.append("target_type = %s" % self.val["m_target_type"])
> -        if self.val["nfields"] > 0:
> -            for fieldno in range(self.val["nfields"]):
> +        if self.val["m_nfields"] > 0:
> +            for fieldno in range(self.val["m_nfields"]):
>                  fields.append(self.struct_field_img(fieldno))
>          if self.val["code"] == gdb.TYPE_CODE_RANGE:
>              fields.append(self.bounds_img())
> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
> index a9abb0d8071..da1d0f79d1f 100644
> --- a/gdb/gdbtypes.h
> +++ b/gdb/gdbtypes.h
> @@ -836,7 +836,7 @@ struct main_type
>    /* * Number of fields described for this type.  This field appears
>       at this location because it packs nicely here.  */
>  
> -  short nfields;
> +  int m_nfields;

Should it be unsigned?  I don't recall this field needing to be
negative.

Otherwise, LGTM.

Simon
  

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

>> -  short nfields;
>> +  int m_nfields;

Simon> Should it be unsigned?  I don't recall this field needing to be
Simon> negative.

I considered it, but the accessor returns int and the setter accepts
int.  So, changing it would perhaps result in a larger patch, but the
only benefit would be that one could have more than 2 billion constants.
Even in the realm of generated code this seemed implausible to me.

I could take a look if you think it's worthwhile.  I do agree it makes
more sense, if that helps.

Tom
  

On 1/11/23 16:28, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>>> -  short nfields;
>>> +  int m_nfields;
> 
> Simon> Should it be unsigned?  I don't recall this field needing to be
> Simon> negative.
> 
> I considered it, but the accessor returns int and the setter accepts
> int.  So, changing it would perhaps result in a larger patch, but the
> only benefit would be that one could have more than 2 billion constants.
> Even in the realm of generated code this seemed implausible to me.
> 
> I could take a look if you think it's worthwhile.  I do agree it makes
> more sense, if that helps.

It's not really important from a practical point of view.  But also, I'd
be surprised if it broke anything, even without touching the callers.

Simon
  

Simon> It's not really important from a practical point of view.  But also, I'd
Simon> be surprised if it broke anything, even without touching the callers.

The simplest change needed one cast, but it seems to me that if we want
this to work properly, all callers of num_fields and set_num_fields must
be updated to use unsigned types.  I'm looking into that.

Tom
  

On 2023-01-11 9:02 p.m., Simon Marchi via Gdb-patches wrote:
> 
> On 1/11/23 15:44, Tom Tromey via Gdb-patches wrote:
>> main_type::nfields is a 'short', and has been for many years.  PR
>> c++/29985 points out that 'short' is too narrow for an enum that
>> contains more than 2^15 constants.
>>
>> This patch bumps the size of 'nfields'.  To verify that the field
>> isn't directly used, it is also renamed.  Note that this does not
>> affect the size of main_type on x86-64 Fedora 36.  And, if it does
>> have a negative effect somewhere, it's worth considering that types
>> could be shrunk more drastically by using subclasses for the different
>> codes.
>>
>> I wasn't sure whether a test case for this would be desirable.  It
>> would be a bit large.
> We could make a test case that generates a source file on the fly in
> standard_output_directory and compiles it.
> 

+1
  

>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

>> We could make a test case that generates a source file on the fly in
>> standard_output_directory and compiles it.

Pedro> +1

I tried this today and learned that GCC cannot compile the source file.

I wonder if I can use a loop in the DWARF assembler.

Tom
  

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> We could make a test case that generates a source file on the fly in
Simon> standard_output_directory and compiles it.
...
>> -  short nfields;
>> +  int m_nfields;

Simon> Should it be unsigned?  I don't recall this field needing to be
Simon> negative.

I sent v2 with these changes.

Tom