diff mbox

[1/5] Introduce specialized versions of gdbpy_ref

Message ID 20170123224004.8893-2-simon.marchi@ericsson.com
State New, archived
Headers

Commit Message

Simon Marchi Jan. 23, 2017, 10:40 p.m. UTC 
  From: Simon Marchi <simon.marchi@polymtl.ca>

I was playing with the Python API implementation a bit, and I thought we
could improve/simplify the gdbpy reference counting a bit for our own
Python types.  We currently have a single type, gdbpy_ref, which holds a
reference to a PyObject*.  This means that if we want to wrap an
inferior_object* (which is a sub-class-ish of PyObject), we need to cast
it when constructing the reference:

  gdbpy_ref ref ((PyObject *) an_inferior_object);

as well as when getting the reference

  inferior_object *obj = (inferior_object *) ref.get ();

This patch generalizes the gdbpy_ref concept so that it can hold a reference to
other types than PyObject.  This should mean less casting when dealing with
references to our specialized Python objects.

The main new part is the template class gdbpy_ref_base, which extends
gdb::ref_ptr with the gdbpy_ref_policy policy, much like gdbpy_ref did
previoulsy.  However, the referenced object type is now the generic type
T instead of PyObject*.  The gdbpy_ref_policy is generalized as well to
match.  Note that we don't need to cast the pointers in incref/decref
since Py_INCREF/Py_DECREF already do that.

Specializations of gdbpy_ref_base can then be added for our various
Python types.  This patch only adds gdbpy_ref to match the one that was
already there.  So no behavioral changes are expected.

We must make sure to only use gdbpy_ref_base on objects that actually
are Python objects.  For example, gdbpy_ref_base<thread_info> would make
not sense.  Since the "inheritance" from the PyObject type is done in a
C way (using PyObject_HEAD), I don't know how we can check at
compile-time that we are not using it with a wrong type.  If you have an
idea on how to do that, let me know.  We would need to check that there
exists a field named ob_base.  Bonus points for ensuring that its type
is PyObject.  More bonus points for ensuring that it's the first field
in the structure.

For convenience, I added a get_py_obj method to gdbpy_ref_base, which
returns the pointer casted to PyObject*, something we need to do
relatively often).

gdb/ChangeLog:

	* python/py-ref.h (gdbpy_ref_policy): Add template.
	(gdbpy_ref_base): New class.
	(gdbpy_ref): Define in terms of gdbpy_ref_base instead of
	gdb::ref_ptr.
---
 gdb/python/py-ref.h | 37 +++++++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

Comments

Tom Tromey Jan. 24, 2017, 3:53 p.m. UTC | #1 
>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:

Simon> We currently have a single type, gdbpy_ref, which holds a
Simon> reference to a PyObject*.  This means that if we want to wrap an
Simon> inferior_object* (which is a sub-class-ish of PyObject), we need to cast
Simon> it when constructing the reference:

This was the subject of this patch:

    https://sourceware.org/ml/gdb-patches/2017-01/msg00277.html

I applied it all over the Python layer.

Tom
Simon Marchi Jan. 24, 2017, 4:18 p.m. UTC | #2 
On 2017-01-24 10:53, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:
> 
> Simon> We currently have a single type, gdbpy_ref, which holds a
> Simon> reference to a PyObject*.  This means that if we want to wrap an
> Simon> inferior_object* (which is a sub-class-ish of PyObject), we need 
> to cast
> Simon> it when constructing the reference:
> 
> This was the subject of this patch:
> 
>     https://sourceware.org/ml/gdb-patches/2017-01/msg00277.html
> 
> I applied it all over the Python layer.
> 
> Tom

Ah damn, shame on me for missing it.  At least doing it by myself made 
me learn a lot :).  I'll take a look at that series.

There might still be some bits relevant in the current series, like the 
bug fix in patch 5 (unless you also addressed it).  I'll re-evaluate it 
once your series is in.

Simon
Pedro Alves Feb. 9, 2017, 11:58 a.m. UTC | #3 
On 01/23/2017 10:40 PM, Simon Marchi wrote:
> 
> We must make sure to only use gdbpy_ref_base on objects that actually
> are Python objects.  For example, gdbpy_ref_base<thread_info> would make
> not sense.  Since the "inheritance" from the PyObject type is done in a
> C way (using PyObject_HEAD), I don't know how we can check at
> compile-time that we are not using it with a wrong type.  If you have an
> idea on how to do that, let me know.  We would need to check that there
> exists a field named ob_base.  Bonus points for ensuring that its type
> is PyObject.  More bonus points for ensuring that it's the first field
> in the structure.

You can do all this with SFINAE.  For the "is first field check, you could
use something like "(PyObject*) this == &this->ob_base" as expression,
I think.

https://en.wikipedia.org/wiki/Substitution_failure_is_not_an_error

There are many examples around the web, if you search for SFINAE and
"C++ check if member exists", etc.  E.g,:

http://stackoverflow.com/questions/1005476/how-to-detect-whether-there-is-a-specific-member-variable-in-class

Though I still wonder whether just inheriting our objects from
PyObject wouldn't make things simpler.

> For convenience, I added a get_py_obj method to gdbpy_ref_base, which
> returns the pointer casted to PyObject*, something we need to do
> relatively often).

Thanks,
Pedro Alves
Simon Marchi Feb. 9, 2017, 4:17 p.m. UTC | #4 
On 2017-02-09 06:58, Pedro Alves wrote:
> On 01/23/2017 10:40 PM, Simon Marchi wrote:
>> 
>> We must make sure to only use gdbpy_ref_base on objects that actually
>> are Python objects.  For example, gdbpy_ref_base<thread_info> would 
>> make
>> not sense.  Since the "inheritance" from the PyObject type is done in 
>> a
>> C way (using PyObject_HEAD), I don't know how we can check at
>> compile-time that we are not using it with a wrong type.  If you have 
>> an
>> idea on how to do that, let me know.  We would need to check that 
>> there
>> exists a field named ob_base.  Bonus points for ensuring that its type
>> is PyObject.  More bonus points for ensuring that it's the first field
>> in the structure.
> 
> You can do all this with SFINAE.  For the "is first field check, you 
> could
> use something like "(PyObject*) this == &this->ob_base" as expression,
> I think.
> 
> https://en.wikipedia.org/wiki/Substitution_failure_is_not_an_error
> 
> There are many examples around the web, if you search for SFINAE and
> "C++ check if member exists", etc.  E.g,:
> 
> http://stackoverflow.com/questions/1005476/how-to-detect-whether-there-is-a-specific-member-variable-in-class

Thanks!  This kind of thing doesn't come to me naturally yet.

> Though I still wonder whether just inheriting our objects from
> PyObject wouldn't make things simpler.

I'll have to try that.

Thanks,

Simon
diff mbox

Patch

diff --git a/gdb/python/py-ref.h b/gdb/python/py-ref.h
index b2479bf656..8b3b7732cc 100644
--- a/gdb/python/py-ref.h
+++ b/gdb/python/py-ref.h
@@ -23,20 +23,49 @@ 
 #include "common/gdb_ref_ptr.h"
 
 /* A policy class for gdb::ref_ptr for Python reference counting.  */
+
+template <typename T>
 struct gdbpy_ref_policy
 {
-  static void incref (PyObject *ptr)
+  static void incref (T *ptr)
   {
     Py_INCREF (ptr);
   }
 
-  static void decref (PyObject *ptr)
+  static void decref (T *ptr)
   {
     Py_DECREF (ptr);
   }
 };
 
-/* A gdb::ref_ptr that has been specialized for Python objects.  */
-typedef gdb::ref_ptr<PyObject, gdbpy_ref_policy> gdbpy_ref;
+/* Reference counting specialized for Python objects.
+
+   This class must only be used with Python types, i.e. types declared with
+   PyObject_HEAD as their first "field".  */
+
+template <typename T>
+class gdbpy_ref_base : public gdb::ref_ptr<T, gdbpy_ref_policy<T>>
+{
+public:
+
+  /* Create a new NULL instance.  */
+
+  gdbpy_ref_base ()
+  : gdb::ref_ptr<T, gdbpy_ref_policy<T>> ()
+  {}
+
+  explicit gdbpy_ref_base (T *ptr)
+  : gdb::ref_ptr<T, gdbpy_ref_policy<T>> (ptr)
+  {}
+
+  /* Return a pointer to the owned Python object as a generic PyObject.  */
+
+  PyObject *get_py_obj ()
+  { return (PyObject *) this->get(); }
+};
+
+/* Specializations of gdbpy_ref_base for concrete Python object types.  */
+
+typedef gdbpy_ref_base<PyObject> gdbpy_ref;
 
 #endif /* GDB_PYTHON_REF_H */