public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] Increase size of main_type::nfields
@ 2023-01-11 20:44 Tom Tromey
  2023-01-11 21:02 ` Simon Marchi
  0 siblings, 1 reply; 8+ messages in thread
From: Tom Tromey @ 2023-01-11 20:44 UTC (permalink / raw)
  To: gdb-patches; +Cc: 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Increase size of main_type::nfields
  2023-01-11 20:44 [PATCH] Increase size of main_type::nfields Tom Tromey
@ 2023-01-11 21:02 ` Simon Marchi
  2023-01-11 21:28   ` Tom Tromey
                     ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Simon Marchi @ 2023-01-11 21:02 UTC (permalink / raw)
  To: Tom Tromey, gdb-patches



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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Increase size of main_type::nfields
  2023-01-11 21:02 ` Simon Marchi
@ 2023-01-11 21:28   ` Tom Tromey
  2023-01-11 22:33     ` Simon Marchi
  2023-01-13 12:53   ` Pedro Alves
  2023-01-27 15:13   ` Tom Tromey
  2 siblings, 1 reply; 8+ messages in thread
From: Tom Tromey @ 2023-01-11 21:28 UTC (permalink / raw)
  To: Simon Marchi via Gdb-patches; +Cc: Tom Tromey, Simon Marchi

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Increase size of main_type::nfields
  2023-01-11 21:28   ` Tom Tromey
@ 2023-01-11 22:33     ` Simon Marchi
  2023-01-12 19:14       ` Tom Tromey
  0 siblings, 1 reply; 8+ messages in thread
From: Simon Marchi @ 2023-01-11 22:33 UTC (permalink / raw)
  To: Tom Tromey, Simon Marchi via Gdb-patches; +Cc: Tom Tromey



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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Increase size of main_type::nfields
  2023-01-11 22:33     ` Simon Marchi
@ 2023-01-12 19:14       ` Tom Tromey
  0 siblings, 0 replies; 8+ messages in thread
From: Tom Tromey @ 2023-01-12 19:14 UTC (permalink / raw)
  To: Simon Marchi; +Cc: Tom Tromey, Simon Marchi via Gdb-patches, 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Increase size of main_type::nfields
  2023-01-11 21:02 ` Simon Marchi
  2023-01-11 21:28   ` Tom Tromey
@ 2023-01-13 12:53   ` Pedro Alves
  2023-01-27 14:49     ` Tom Tromey
  2023-01-27 15:13   ` Tom Tromey
  2 siblings, 1 reply; 8+ messages in thread
From: Pedro Alves @ 2023-01-13 12:53 UTC (permalink / raw)
  To: Simon Marchi, Tom Tromey, gdb-patches

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Increase size of main_type::nfields
  2023-01-13 12:53   ` Pedro Alves
@ 2023-01-27 14:49     ` Tom Tromey
  0 siblings, 0 replies; 8+ messages in thread
From: Tom Tromey @ 2023-01-27 14:49 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Simon Marchi, Tom Tromey, gdb-patches

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Increase size of main_type::nfields
  2023-01-11 21:02 ` Simon Marchi
  2023-01-11 21:28   ` Tom Tromey
  2023-01-13 12:53   ` Pedro Alves
@ 2023-01-27 15:13   ` Tom Tromey
  2 siblings, 0 replies; 8+ messages in thread
From: Tom Tromey @ 2023-01-27 15:13 UTC (permalink / raw)
  To: Simon Marchi via Gdb-patches; +Cc: Tom Tromey, Simon Marchi

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2023-01-27 15:13 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-11 20:44 [PATCH] Increase size of main_type::nfields Tom Tromey
2023-01-11 21:02 ` Simon Marchi
2023-01-11 21:28   ` Tom Tromey
2023-01-11 22:33     ` Simon Marchi
2023-01-12 19:14       ` Tom Tromey
2023-01-13 12:53   ` Pedro Alves
2023-01-27 14:49     ` Tom Tromey
2023-01-27 15:13   ` Tom Tromey

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).