[PATCH] gdb/python: remove some uses of Py_TPFLAGS_HAVE_ITER

[PATCH] gdb/python: remove some uses of Py_TPFLAGS_HAVE_ITER

[Andrew Burgess]

Python 2 has a bit flag Py_TPFLAGS_HAVE_ITER which can be passed as
part of the tp_flags field when defining a new object type.  This flag
is not defined in Python 3 and so we define it to 0 in
python-internal.h (when IS_PY3K is defined).

The meaning of this flag is that the object has the fields tp_iter and
tp_iternext.  The flag says nothing about the values in those fields,
just that the object has the fields.

My assumption here is that very early versions of Python didn't define
those fields, and so, there could be compiled code that uses smaller
PyTypeObject structures.  Python 2 will only try to access the
iterator related fields when that flag is present.

In GDB we seem to use the flag inconsistently.  For objects that are
really iterators we always include the flag, failure to do so would
mean GDB didn't work correctly on Python 2.

However, on classes that are not iterators (both tp_iter and
tp_iternext are nullptr) we sometimes pass the Py_TPFLAGS_HAVE_ITER
flag, and sometimes we don't.

This confused me a bit.

I'd like to propose that we make use of this flag more consistent.

My proposal is that we only use the flag for those object types that
actually set one (or both) of tp_iter or tp_iternext.  It would have
been just as valid to pass the Py_TPFLAGS_HAVE_ITER flag for every
object type (as we only support versions of Python that include these
fields).

There should be no user visible changes after this commit.
---
 gdb/python/py-inferior.c  | 2 +-
 gdb/python/py-infthread.c | 2 +-
 gdb/python/py-type.c      | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
index cfbc2f6574f..0659c28ea9c 100644
--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -1026,7 +1026,7 @@ PyTypeObject inferior_object_type =
   0,				  /* tp_getattro */
   0,				  /* tp_setattro */
   0,				  /* tp_as_buffer */
-  Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_ITER,  /* tp_flags */
+  Py_TPFLAGS_DEFAULT,		  /* tp_flags */
   "GDB inferior object",	  /* tp_doc */
   0,				  /* tp_traverse */
   0,				  /* tp_clear */
diff --git a/gdb/python/py-infthread.c b/gdb/python/py-infthread.c
index 3315af5eb59..5645442a426 100644
--- a/gdb/python/py-infthread.c
+++ b/gdb/python/py-infthread.c
@@ -408,7 +408,7 @@ PyTypeObject thread_object_type =
   0,				  /*tp_getattro*/
   0,				  /*tp_setattro*/
   0,				  /*tp_as_buffer*/
-  Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_ITER,  /*tp_flags*/
+  Py_TPFLAGS_DEFAULT,		  /*tp_flags*/
   "GDB thread object",		  /* tp_doc */
   0,				  /* tp_traverse */
   0,				  /* tp_clear */
diff --git a/gdb/python/py-type.c b/gdb/python/py-type.c
index 04d1c7a0ee7..771e971aaa7 100644
--- a/gdb/python/py-type.c
+++ b/gdb/python/py-type.c
@@ -1682,7 +1682,7 @@ PyTypeObject field_object_type =
   0,				  /*tp_getattro*/
   0,				  /*tp_setattro*/
   0,				  /*tp_as_buffer*/
-  Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_ITER,  /*tp_flags*/
+  Py_TPFLAGS_DEFAULT,		  /*tp_flags*/
   "GDB field object",		  /* tp_doc */
   0,				  /* tp_traverse */
   0,				  /* tp_clear */
-- 
2.25.4

Re: [PATCH] gdb/python: remove some uses of Py_TPFLAGS_HAVE_ITER

[Simon Marchi]

On 2021-09-06 4:04 a.m., Andrew Burgess wrote:
> Python 2 has a bit flag Py_TPFLAGS_HAVE_ITER which can be passed as
> part of the tp_flags field when defining a new object type.  This flag
> is not defined in Python 3 and so we define it to 0 in
> python-internal.h (when IS_PY3K is defined).
> 
> The meaning of this flag is that the object has the fields tp_iter and
> tp_iternext.  The flag says nothing about the values in those fields,
> just that the object has the fields.
> 
> My assumption here is that very early versions of Python didn't define
> those fields, and so, there could be compiled code that uses smaller
> PyTypeObject structures.  Python 2 will only try to access the
> iterator related fields when that flag is present.
> 
> In GDB we seem to use the flag inconsistently.  For objects that are
> really iterators we always include the flag, failure to do so would
> mean GDB didn't work correctly on Python 2.
> 
> However, on classes that are not iterators (both tp_iter and
> tp_iternext are nullptr) we sometimes pass the Py_TPFLAGS_HAVE_ITER
> flag, and sometimes we don't.
> 
> This confused me a bit.
> 
> I'd like to propose that we make use of this flag more consistent.
> 
> My proposal is that we only use the flag for those object types that
> actually set one (or both) of tp_iter or tp_iternext.  It would have
> been just as valid to pass the Py_TPFLAGS_HAVE_ITER flag for every
> object type (as we only support versions of Python that include these
> fields).

Python 2.7's doc says:

  Py_TPFLAGS_HAVE_ITER

    If this bit is set, the type object has the tp_iter and tp_iternext fields.

So pedantically, we should not set it if an object has only tp_iter and
not tp_iternext?  I don't know what was the original intention of that
flag.

But the types you changed clearly should not have the flag, since they
don't specify either, so the patch LGTM.

Simon

Re: [PATCH] gdb/python: remove some uses of Py_TPFLAGS_HAVE_ITER

[Andrew Burgess]

* Simon Marchi <simon.marchi@polymtl.ca> [2021-09-07 11:16:54 -0400]:

> 
> 
> On 2021-09-06 4:04 a.m., Andrew Burgess wrote:
> > Python 2 has a bit flag Py_TPFLAGS_HAVE_ITER which can be passed as
> > part of the tp_flags field when defining a new object type.  This flag
> > is not defined in Python 3 and so we define it to 0 in
> > python-internal.h (when IS_PY3K is defined).
> > 
> > The meaning of this flag is that the object has the fields tp_iter and
> > tp_iternext.  The flag says nothing about the values in those fields,
> > just that the object has the fields.
> > 
> > My assumption here is that very early versions of Python didn't define
> > those fields, and so, there could be compiled code that uses smaller
> > PyTypeObject structures.  Python 2 will only try to access the
> > iterator related fields when that flag is present.
> > 
> > In GDB we seem to use the flag inconsistently.  For objects that are
> > really iterators we always include the flag, failure to do so would
> > mean GDB didn't work correctly on Python 2.
> > 
> > However, on classes that are not iterators (both tp_iter and
> > tp_iternext are nullptr) we sometimes pass the Py_TPFLAGS_HAVE_ITER
> > flag, and sometimes we don't.
> > 
> > This confused me a bit.
> > 
> > I'd like to propose that we make use of this flag more consistent.
> > 
> > My proposal is that we only use the flag for those object types that
> > actually set one (or both) of tp_iter or tp_iternext.  It would have
> > been just as valid to pass the Py_TPFLAGS_HAVE_ITER flag for every
> > object type (as we only support versions of Python that include these
> > fields).
> 
> Python 2.7's doc says:
> 
>   Py_TPFLAGS_HAVE_ITER
> 
>     If this bit is set, the type object has the tp_iter and tp_iternext fields.
> 
> So pedantically, we should not set it if an object has only tp_iter and
> not tp_iternext?  I don't know what was the original intention of that
> flag.

See, I don't think that's what it means.  Notice it says "has the",
not that the fields are actually set to any particular value or not.

If we check here:

  https://docs.python.org/release/2.3/api/type-structs.html

We see this:

    /* Added in release 2.2 */
    /* Iterators */
    getiterfunc tp_iter;
    iternextfunc tp_iternext;

So, I think what the flag does is allow backward compatibility with
code compiled for Python < 2.2.

If you compiled code against Python 2.1 but then ran it against Python
2.2+ you would have PyTypeObject instances that don't even _have_ the
tp_iter fields, accessing them would be undefined behaviour, but
that's OK, you also would not have set Py_TPFLAGS_HAVE_ITER so Python
never will try to access those fields.

In GDB we mostly define our PyTypeObject assuming that tp_iter fields
are present, so if anyone was crazy enough to try and compile against
Python 2.1 we'd get lots of build errors, some of which would be GCC
complaining that we are passing too many fields to PyTypeObject.

So, we can safely assume that GDB is always compiled against a Python
that include the tp_iter fields.

So then I thought, we could really do with a single flag that defines
some set of defaults, these defaults would indicate all of the fields
that are present in the version of Python we're building against.  We
could call it maybe Py_TPFLAGS_DEFAULT?  Oh wait....

If we check out:

  https://docs.python.org/2.7/c-api/typeobj.html

We find:

  Py_TPFLAGS_DEFAULT

    This is a bitmask of all the bits that pertain to the existence of
    certain fields in the type object and its extension
    structures. Currently, it includes the following bits:
    Py_TPFLAGS_HAVE_GETCHARBUFFER, Py_TPFLAGS_HAVE_SEQUENCE_IN,
    Py_TPFLAGS_HAVE_INPLACEOPS, Py_TPFLAGS_HAVE_RICHCOMPARE,
    Py_TPFLAGS_HAVE_WEAKREFS, Py_TPFLAGS_HAVE_ITER, and
    Py_TPFLAGS_HAVE_CLASS.

Notice it includes Py_TPFLAGS_HAVE_ITER!

Given we always pass Py_TPFLAGS_DEFAULT then my new proposal is just
to remove all references to Py_TPFLAGS_HAVE_ITER from GDB, it's
pointless noise.

Thoughts?

Thanks,
Andrew

---

commit efaa087dc3ae2c685fda9420f6f88cb9697d459f
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Wed Sep 1 14:24:15 2021 +0100

    gdb/python: remove all uses of Py_TPFLAGS_HAVE_ITER
    
    Python 2 has a bit flag Py_TPFLAGS_HAVE_ITER which can be passed as
    part of the tp_flags field when defining a new object type.  This flag
    is not defined in Python 3 and so we define it to 0 in
    python-internal.h (when IS_PY3K is defined).
    
    The meaning of this flag is that the object has the fields tp_iter and
    tp_iternext.  Note the use of "has" here, the flag says nothing about
    the values in those fields, just that the type object has the fields.
    
    In early versions of Python 2 these fields were no part of the
    PyTypeObject struct, they were added in version 2.2 (see
    https://docs.python.org/release/2.3/api/type-structs.html).  And so,
    there could be a some code compiled out there which has a PyTypeObject
    structure within it that doesn't even have the tp_iter and tp_iternext
    fields, attempting to access these fields would be undefined
    behaviour.
    
    And so Python added the Py_TPFLAGS_HAVE_ITER flag.  If the flag is
    present then Python is free to access the tp_iter and tp_iternext
    fields.
    
    If we consider GDB then we always assume that the tp_iter and
    tp_iternext fields are part of PyTypeObject.  If someone was crazy
    enough to try and compile GDB against Python 2.1 then we'd get lots of
    build errors saying that we were passing too many fields when
    initializing PyTypeObject structures.  And so, I claim, we can be sure
    that GDB will always be compiled with a version of Python that has the
    tp_iter and tp_iternext fields in PyTypeObject.
    
    Next we can look at the Py_TPFLAGS_DEFAULT flag.  In Python 2, each
    time additional fields are added to PyTypeObject a new Py_TPFLAGS_*
    flag would be defined to indicate whether those flags are present or
    not.  And, those new flags would be added to Py_TPFLAGS_DEFAULT.  And
    so, in the latest version of Python 2 the Py_TPFLAGS_DEFAULT flag
    includes Py_TPFLAGS_HAVE_ITER (see
    https://docs.python.org/2.7/c-api/typeobj.html).
    
    In GDB we pass Py_TPFLAGS_DEFAULT as part of the tp_flags for all
    objects we define.
    
    And so, in this commit, I propose to remove all uses of
    Py_TPFLAGS_HAVE_ITER from GDB, it's simply not needed.
    
    There should be no user visible changes after this commit.

diff --git a/gdb/python/py-block.c b/gdb/python/py-block.c
index 244ff9a6bab..fe0efd62d46 100644
--- a/gdb/python/py-block.c
+++ b/gdb/python/py-block.c
@@ -510,7 +510,7 @@ PyTypeObject block_object_type = {
   0,				  /*tp_getattro*/
   0,				  /*tp_setattro*/
   0,				  /*tp_as_buffer*/
-  Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_ITER,  /*tp_flags*/
+  Py_TPFLAGS_DEFAULT,		  /*tp_flags*/
   "GDB block object",		  /* tp_doc */
   0,				  /* tp_traverse */
   0,				  /* tp_clear */
@@ -550,7 +550,7 @@ PyTypeObject block_syms_iterator_object_type = {
   0,				  /*tp_getattro*/
   0,				  /*tp_setattro*/
   0,				  /*tp_as_buffer*/
-  Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_ITER,  /*tp_flags*/
+  Py_TPFLAGS_DEFAULT,		  /*tp_flags*/
   "GDB block syms iterator object",	      /*tp_doc */
   0,				  /*tp_traverse */
   0,				  /*tp_clear */
diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
index cfbc2f6574f..0659c28ea9c 100644
--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -1026,7 +1026,7 @@ PyTypeObject inferior_object_type =
   0,				  /* tp_getattro */
   0,				  /* tp_setattro */
   0,				  /* tp_as_buffer */
-  Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_ITER,  /* tp_flags */
+  Py_TPFLAGS_DEFAULT,		  /* tp_flags */
   "GDB inferior object",	  /* tp_doc */
   0,				  /* tp_traverse */
   0,				  /* tp_clear */
diff --git a/gdb/python/py-infthread.c b/gdb/python/py-infthread.c
index 3315af5eb59..5645442a426 100644
--- a/gdb/python/py-infthread.c
+++ b/gdb/python/py-infthread.c
@@ -408,7 +408,7 @@ PyTypeObject thread_object_type =
   0,				  /*tp_getattro*/
   0,				  /*tp_setattro*/
   0,				  /*tp_as_buffer*/
-  Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_ITER,  /*tp_flags*/
+  Py_TPFLAGS_DEFAULT,		  /*tp_flags*/
   "GDB thread object",		  /* tp_doc */
   0,				  /* tp_traverse */
   0,				  /* tp_clear */
diff --git a/gdb/python/py-linetable.c b/gdb/python/py-linetable.c
index e989204a7f5..071341f6ceb 100644
--- a/gdb/python/py-linetable.c
+++ b/gdb/python/py-linetable.c
@@ -531,7 +531,7 @@ PyTypeObject ltpy_iterator_object_type = {
   0,				  /*tp_getattro*/
   0,				  /*tp_setattro*/
   0,				  /*tp_as_buffer*/
-  Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_ITER,  /*tp_flags*/
+  Py_TPFLAGS_DEFAULT,		  /*tp_flags*/
   "GDB line table iterator object",	      /*tp_doc */
   0,				  /*tp_traverse */
   0,				  /*tp_clear */
diff --git a/gdb/python/py-registers.c b/gdb/python/py-registers.c
index 04e554f48e1..df0ba764b46 100644
--- a/gdb/python/py-registers.c
+++ b/gdb/python/py-registers.c
@@ -497,7 +497,7 @@ PyTypeObject register_descriptor_iterator_object_type = {
   0,				  /*tp_getattro*/
   0,				  /*tp_setattro*/
   0,				  /*tp_as_buffer*/
-  Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_ITER,			/*tp_flags*/
+  Py_TPFLAGS_DEFAULT,		  /*tp_flags*/
   "GDB architecture register descriptor iterator object",	/*tp_doc */
   0,				  /*tp_traverse */
   0,				  /*tp_clear */
@@ -567,7 +567,7 @@ PyTypeObject reggroup_iterator_object_type = {
   0,				  /*tp_getattro*/
   0,				  /*tp_setattro*/
   0,				  /*tp_as_buffer*/
-  Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_ITER,	/*tp_flags*/
+  Py_TPFLAGS_DEFAULT,		  /*tp_flags*/
   "GDB register groups iterator object",	/*tp_doc */
   0,				  /*tp_traverse */
   0,				  /*tp_clear */
diff --git a/gdb/python/py-type.c b/gdb/python/py-type.c
index 04d1c7a0ee7..d82bdf84957 100644
--- a/gdb/python/py-type.c
+++ b/gdb/python/py-type.c
@@ -1633,7 +1633,7 @@ PyTypeObject type_object_type =
   0,				  /*tp_getattro*/
   0,				  /*tp_setattro*/
   0,				  /*tp_as_buffer*/
-  Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_ITER,  /*tp_flags*/
+  Py_TPFLAGS_DEFAULT,		  /*tp_flags*/
   "GDB type object",		  /* tp_doc */
   0,				  /* tp_traverse */
   0,				  /* tp_clear */
@@ -1682,7 +1682,7 @@ PyTypeObject field_object_type =
   0,				  /*tp_getattro*/
   0,				  /*tp_setattro*/
   0,				  /*tp_as_buffer*/
-  Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_ITER,  /*tp_flags*/
+  Py_TPFLAGS_DEFAULT,		  /*tp_flags*/
   "GDB field object",		  /* tp_doc */
   0,				  /* tp_traverse */
   0,				  /* tp_clear */
@@ -1723,7 +1723,7 @@ PyTypeObject type_iterator_object_type = {
   0,				  /*tp_getattro*/
   0,				  /*tp_setattro*/
   0,				  /*tp_as_buffer*/
-  Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_ITER,  /*tp_flags*/
+  Py_TPFLAGS_DEFAULT,		  /*tp_flags*/
   "GDB type iterator object",	  /*tp_doc */
   0,				  /*tp_traverse */
   0,				  /*tp_clear */
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 0e140f1af61..368681b797c 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -92,7 +92,6 @@
 #endif
 
 #ifdef IS_PY3K
-#define Py_TPFLAGS_HAVE_ITER 0
 #define Py_TPFLAGS_CHECKTYPES 0
 
 #define PyInt_Check PyLong_Check

Re: [PATCH] gdb/python: remove some uses of Py_TPFLAGS_HAVE_ITER

[Simon Marchi]

On 2021-09-08 5:18 a.m., Andrew Burgess wrote:
> * Simon Marchi <simon.marchi@polymtl.ca> [2021-09-07 11:16:54 -0400]:
> 
>>
>>
>> On 2021-09-06 4:04 a.m., Andrew Burgess wrote:
>>> Python 2 has a bit flag Py_TPFLAGS_HAVE_ITER which can be passed as
>>> part of the tp_flags field when defining a new object type.  This flag
>>> is not defined in Python 3 and so we define it to 0 in
>>> python-internal.h (when IS_PY3K is defined).
>>>
>>> The meaning of this flag is that the object has the fields tp_iter and
>>> tp_iternext.  The flag says nothing about the values in those fields,
>>> just that the object has the fields.
>>>
>>> My assumption here is that very early versions of Python didn't define
>>> those fields, and so, there could be compiled code that uses smaller
>>> PyTypeObject structures.  Python 2 will only try to access the
>>> iterator related fields when that flag is present.
>>>
>>> In GDB we seem to use the flag inconsistently.  For objects that are
>>> really iterators we always include the flag, failure to do so would
>>> mean GDB didn't work correctly on Python 2.
>>>
>>> However, on classes that are not iterators (both tp_iter and
>>> tp_iternext are nullptr) we sometimes pass the Py_TPFLAGS_HAVE_ITER
>>> flag, and sometimes we don't.
>>>
>>> This confused me a bit.
>>>
>>> I'd like to propose that we make use of this flag more consistent.
>>>
>>> My proposal is that we only use the flag for those object types that
>>> actually set one (or both) of tp_iter or tp_iternext.  It would have
>>> been just as valid to pass the Py_TPFLAGS_HAVE_ITER flag for every
>>> object type (as we only support versions of Python that include these
>>> fields).
>>
>> Python 2.7's doc says:
>>
>>   Py_TPFLAGS_HAVE_ITER
>>
>>     If this bit is set, the type object has the tp_iter and tp_iternext fields.
>>
>> So pedantically, we should not set it if an object has only tp_iter and
>> not tp_iternext?  I don't know what was the original intention of that
>> flag.
> 
> See, I don't think that's what it means.  Notice it says "has the",
> not that the fields are actually set to any particular value or not.
> 
> If we check here:
> 
>   https://docs.python.org/release/2.3/api/type-structs.html
> 
> We see this:
> 
>     /* Added in release 2.2 */
>     /* Iterators */
>     getiterfunc tp_iter;
>     iternextfunc tp_iternext;
> 
> So, I think what the flag does is allow backward compatibility with
> code compiled for Python < 2.2.
> 
> If you compiled code against Python 2.1 but then ran it against Python
> 2.2+ you would have PyTypeObject instances that don't even _have_ the
> tp_iter fields, accessing them would be undefined behaviour, but
> that's OK, you also would not have set Py_TPFLAGS_HAVE_ITER so Python
> never will try to access those fields.
> 
> In GDB we mostly define our PyTypeObject assuming that tp_iter fields
> are present, so if anyone was crazy enough to try and compile against
> Python 2.1 we'd get lots of build errors, some of which would be GCC
> complaining that we are passing too many fields to PyTypeObject.
> 
> So, we can safely assume that GDB is always compiled against a Python
> that include the tp_iter fields.
> 
> So then I thought, we could really do with a single flag that defines
> some set of defaults, these defaults would indicate all of the fields
> that are present in the version of Python we're building against.  We
> could call it maybe Py_TPFLAGS_DEFAULT?  Oh wait....
> 
> If we check out:
> 
>   https://docs.python.org/2.7/c-api/typeobj.html
> 
> We find:
> 
>   Py_TPFLAGS_DEFAULT
> 
>     This is a bitmask of all the bits that pertain to the existence of
>     certain fields in the type object and its extension
>     structures. Currently, it includes the following bits:
>     Py_TPFLAGS_HAVE_GETCHARBUFFER, Py_TPFLAGS_HAVE_SEQUENCE_IN,
>     Py_TPFLAGS_HAVE_INPLACEOPS, Py_TPFLAGS_HAVE_RICHCOMPARE,
>     Py_TPFLAGS_HAVE_WEAKREFS, Py_TPFLAGS_HAVE_ITER, and
>     Py_TPFLAGS_HAVE_CLASS.
> 
> Notice it includes Py_TPFLAGS_HAVE_ITER!
> 
> Given we always pass Py_TPFLAGS_DEFAULT then my new proposal is just
> to remove all references to Py_TPFLAGS_HAVE_ITER from GDB, it's
> pointless noise.
> 
> Thoughts?

Ok, I understand what you mean and what Py_TPFLAGS_HAVE_ITER does.
Well, that last proposal sounds good to me.

Simon