[PATCH] Increase size of main_type::nfields

[PATCH] Increase size of main_type::nfields

[Tom Tromey]

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(-)

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;
 
   /* * Name of this type, or NULL if none.
 
@@ -964,13 +964,13 @@ struct type
   /* Get the number of fields of this type.  */
   int num_fields () const
   {
-    return this->main_type->nfields;
+    return this->main_type->m_nfields;
   }
 
   /* Set the number of fields of this type.  */
   void set_num_fields (int num_fields)
   {
-    this->main_type->nfields = num_fields;
+    this->main_type->m_nfields = num_fields;
   }
 
   /* Get the fields array of this type.  */
-- 
2.38.1

Re: [PATCH] Increase size of main_type::nfields

[Simon Marchi]

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

Re: [PATCH] Increase size of main_type::nfields

[Tom Tromey]

>>>>> "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

Re: [PATCH] Increase size of main_type::nfields

[Simon Marchi]

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

Re: [PATCH] Increase size of main_type::nfields

[Tom Tromey]

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

Re: [PATCH] Increase size of main_type::nfields

[Pedro Alves]

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

Re: [PATCH] Increase size of main_type::nfields

[Tom Tromey]

>>>>> "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

Re: [PATCH] Increase size of main_type::nfields

[Tom Tromey]

>>>>> "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