* [PATCH 5/5] Add missing incref when creating Inferior Python object
2017-01-23 22:43 [PATCH 0/5] Improve Python Inferior reference handling + fix a bug Simon Marchi
@ 2017-01-23 22:43 ` Simon Marchi
2017-02-25 18:41 ` Simon Marchi
2017-01-23 22:43 ` [PATCH 3/5] Make Python inferior-related internal functions return inferior_object* Simon Marchi
` (3 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Simon Marchi @ 2017-01-23 22:43 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
The test py-inferior.exp fails when using my debug build of Python 3.6.
I don't see it failing with my system's default Python, but it might be
related to the different memory allocation scheme used when doing a
build with pydebug.
The issue is that we are missing a Py_INCREF in
inferior_to_inferior_object. The PyObject_New function initializes the
object with a refcount of 1. If we assume that this refcount
corresponds to the reference we are keeping in the inferior data, then
we are missing an incref for the reference we are returning. We can
also see it the other way. If the refcount added by PyObject_New is for
the reference we are returning, then we are missing one for the inferior
data.
The counterpart for this incref is in py_free_inferior.
Here's how I can get it to crash:
$ ./gdb -nx -ex "set debug python 1"
(gdb) add-inferior
Added inferior 2
(gdb) python infs = gdb.inferiors()
Creating Python Inferior object inf = 1
Creating Python Inferior object inf = 2
(gdb) remove-inferiors 2
py_free_inferior inf = 2
infpy_dealloc inf = <unknown>
(gdb) python infs = None
Fatal Python error: Objects/tupleobject.c:243 object at 0x7f9cf1a568d8 has negative ref count -1
Current thread 0x00007f9cf1b68780 (most recent call first):
File "<string>", line 1 in <module>
[1] 408 abort (core dumped) ./gdb -nx -ex "set debug python 1"
After having created the inferiors object, their refcount is 1 (which
comes from PyObject_New), but it should be two. The gdb inferior object
has a reference and the "infs" list has a reference.
When invoking remove-inferiors, py_free_inferior gets called. It does
the decref that corresponds to the reference that the gdb inferior
object kept. At this moment, the refcount drops to 0 and the object
gets deallocated, even though the "infs" list still has a reference.
When we set "infs" to None, Python tries to decref the already zero
refcount and the assert triggers.
With this patch, it looks better:
(gdb) add-inferior
Added inferior 2
(gdb) python infs = gdb.inferiors()
Creating Python Inferior object inf = 1
Creating Python Inferior object inf = 2
(gdb) remove-inferiors 2
py_free_inferior inf = 2
(gdb) python infs = None
infpy_dealloc inf = <unknown>
gdb/ChangeLog:
* python/py-inferior.c (inferior_to_inferior_object): Manually
increment reference count when creating the object as well.
---
gdb/python/py-inferior.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
index 340dddcfbd..24ef4f0ec8 100644
--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -227,10 +227,13 @@ inferior_to_inferior_object (struct inferior *inferior)
inf_obj->threads = NULL;
inf_obj->nthreads = 0;
+ /* PyObject_New initializes the new object with a refcount of 1. This
+ counts for the reference we are keeping in the inferior data. */
set_inferior_data (inferior, infpy_inf_data_key, inf_obj);
}
- else
- Py_INCREF ((PyObject *)inf_obj);
+
+ /* We are returning a new reference. */
+ Py_INCREF (inf_obj);
return gdbpy_inf_ref (inf_obj);
}
--
2.11.0
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 5/5] Add missing incref when creating Inferior Python object
2017-01-23 22:43 ` [PATCH 5/5] Add missing incref when creating Inferior Python object Simon Marchi
@ 2017-02-25 18:41 ` Simon Marchi
2017-04-27 21:13 ` [pushed master+8.0] " Simon Marchi
0 siblings, 1 reply; 16+ messages in thread
From: Simon Marchi @ 2017-02-25 18:41 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches
On 2017-01-23 17:40, Simon Marchi wrote:
> The test py-inferior.exp fails when using my debug build of Python 3.6.
> I don't see it failing with my system's default Python, but it might be
> related to the different memory allocation scheme used when doing a
> build with pydebug.
>
> The issue is that we are missing a Py_INCREF in
> inferior_to_inferior_object. The PyObject_New function initializes the
> object with a refcount of 1. If we assume that this refcount
> corresponds to the reference we are keeping in the inferior data, then
> we are missing an incref for the reference we are returning. We can
> also see it the other way. If the refcount added by PyObject_New is
> for
> the reference we are returning, then we are missing one for the
> inferior
> data.
>
> The counterpart for this incref is in py_free_inferior.
>
> Here's how I can get it to crash:
>
> $ ./gdb -nx -ex "set debug python 1"
> (gdb) add-inferior
> Added inferior 2
> (gdb) python infs = gdb.inferiors()
> Creating Python Inferior object inf = 1
> Creating Python Inferior object inf = 2
> (gdb) remove-inferiors 2
> py_free_inferior inf = 2
> infpy_dealloc inf = <unknown>
> (gdb) python infs = None
> Fatal Python error: Objects/tupleobject.c:243 object at
> 0x7f9cf1a568d8 has negative ref count -1
>
> Current thread 0x00007f9cf1b68780 (most recent call first):
> File "<string>", line 1 in <module>
> [1] 408 abort (core dumped) ./gdb -nx -ex "set debug python 1"
>
> After having created the inferiors object, their refcount is 1 (which
> comes from PyObject_New), but it should be two. The gdb inferior
> object
> has a reference and the "infs" list has a reference.
>
> When invoking remove-inferiors, py_free_inferior gets called. It does
> the decref that corresponds to the reference that the gdb inferior
> object kept. At this moment, the refcount drops to 0 and the object
> gets deallocated, even though the "infs" list still has a reference.
> When we set "infs" to None, Python tries to decref the already zero
> refcount and the assert triggers.
>
> With this patch, it looks better:
>
> (gdb) add-inferior
> Added inferior 2
> (gdb) python infs = gdb.inferiors()
> Creating Python Inferior object inf = 1
> Creating Python Inferior object inf = 2
> (gdb) remove-inferiors 2
> py_free_inferior inf = 2
> (gdb) python infs = None
> infpy_dealloc inf = <unknown>
>
> gdb/ChangeLog:
>
> * python/py-inferior.c (inferior_to_inferior_object): Manually
> increment reference count when creating the object as well.
> ---
> gdb/python/py-inferior.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
> index 340dddcfbd..24ef4f0ec8 100644
> --- a/gdb/python/py-inferior.c
> +++ b/gdb/python/py-inferior.c
> @@ -227,10 +227,13 @@ inferior_to_inferior_object (struct inferior
> *inferior)
> inf_obj->threads = NULL;
> inf_obj->nthreads = 0;
>
> + /* PyObject_New initializes the new object with a refcount of 1.
> This
> + counts for the reference we are keeping in the inferior data. */
> set_inferior_data (inferior, infpy_inf_data_key, inf_obj);
> }
> - else
> - Py_INCREF ((PyObject *)inf_obj);
> +
> + /* We are returning a new reference. */
> + Py_INCREF (inf_obj);
>
> return gdbpy_inf_ref (inf_obj);
> }
Ping for this patch only. It's actually not dependent on the rest of
the series and fixes an actual bug, so I think it could go in by itself.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [pushed master+8.0] Add missing incref when creating Inferior Python object
2017-02-25 18:41 ` Simon Marchi
@ 2017-04-27 21:13 ` Simon Marchi
0 siblings, 0 replies; 16+ messages in thread
From: Simon Marchi @ 2017-04-27 21:13 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
I have pushed this patch (by itself) to master and 8.0. It just needed a
little refresh, here's the pushed version.
The test py-inferior.exp fails when using a debug build of Python 3.6. I don't
see it failing with my system's default Python, but it might be related to the
different memory allocation scheme used when doing a build with pydebug.
The issue is that we are missing a Py_INCREF in
inferior_to_inferior_object. The PyObject_New function initializes the
object with a refcount of 1. If we assume that this refcount
corresponds to the reference we are returning, then we are missing an
incref for the reference in the inferior data.
The counterpart for the incref that corresponds to the reference in the
inferior data is in py_free_inferior, in the form the gdbpy_ref instance.
Here's how I can get it to crash (with some debug output):
$ ./gdb -nx -ex "set debug python 1"
(gdb) add-inferior
Added inferior 2
(gdb) python infs = gdb.inferiors()
Creating Python Inferior object inf = 1
Creating Python Inferior object inf = 2
(gdb) remove-inferiors 2
py_free_inferior inf = 2
infpy_dealloc inf = <unknown>
(gdb) python infs = None
Fatal Python error: Objects/tupleobject.c:243 object at 0x7f9cf1a568d8 has negative ref count -1
Current thread 0x00007f9cf1b68780 (most recent call first):
File "<string>", line 1 in <module>
[1] 408 abort (core dumped) ./gdb -nx -ex "set debug python 1"
After having created the inferiors object, their refcount is 1 (which
comes from PyObject_New), but it should be two. The gdb inferior object
has a reference and the "infs" list has a reference.
When invoking remove-inferiors, py_free_inferior gets called. It does
the decref that corresponds to the reference that the gdb inferior
object kept. At this moment, the refcount drops to 0 and the object
gets deallocated, even though the "infs" list still has a reference.
When we set "infs" to None, Python tries to decref the already zero
refcount and the assert triggers.
With this patch, it looks better:
(gdb) add-inferior
Added inferior 2
(gdb) python infs = gdb.inferiors()
Creating Python Inferior object inf = 1
Creating Python Inferior object inf = 2
(gdb) remove-inferiors 2
py_free_inferior inf = 2
(gdb) python infs = None
infpy_dealloc inf = <unknown>
gdb/ChangeLog:
* python/py-inferior.c (inferior_to_inferior_object): Increment reference
count when creating the object.
---
gdb/ChangeLog | 5 +++++
gdb/python/py-inferior.c | 7 +++++--
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 2b265a1301..e356e0b7ea 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2017-04-27 Simon Marchi <simon.marchi@ericsson.com>
+
+ * python/py-inferior.c (inferior_to_inferior_object): Increment reference
+ count when creating the object.
+
2017-04-25 Yao Qi <yao.qi@linaro.org>
* aarch64-tdep.c (aarch64_gdbarch_init): Don't call
diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
index 3d2cb1ddbe..f6a24a090f 100644
--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -223,11 +223,14 @@ inferior_to_inferior_object (struct inferior *inferior)
inf_obj->threads = NULL;
inf_obj->nthreads = 0;
+ /* PyObject_New initializes the new object with a refcount of 1. This
+ counts for the reference we are keeping in the inferior data. */
set_inferior_data (inferior, infpy_inf_data_key, inf_obj);
}
- else
- Py_INCREF ((PyObject *)inf_obj);
+
+ /* We are returning a new reference. */
+ Py_INCREF ((PyObject *)inf_obj);
return (PyObject *) inf_obj;
}
--
2.11.0
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/5] Make Python inferior-related internal functions return inferior_object*
2017-01-23 22:43 [PATCH 0/5] Improve Python Inferior reference handling + fix a bug Simon Marchi
2017-01-23 22:43 ` [PATCH 5/5] Add missing incref when creating Inferior Python object Simon Marchi
@ 2017-01-23 22:43 ` Simon Marchi
2017-01-24 0:03 ` Pedro Alves
2017-01-23 22:43 ` [PATCH 1/5] Introduce specialized versions of gdbpy_ref Simon Marchi
` (2 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Simon Marchi @ 2017-01-23 22:43 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
From: Simon Marchi <simon.marchi@polymtl.ca>
This patch changes the functions that return an Inferior Python object
so they return an inferior_object pointer instead of a generic PyObject
pointer. This should remove some casting back and forth from PyObject*
to inferior_object* in our codebase, helping a bit for type-safety.
On the other hand, this requires adding some casts from inferior_object*
to PyObject* when we deal with the Python API, which obviously accepts
only PyObject*. I think this is a more appropriate place for casts
though.
It defines and uses gdbpy_inf_ref, a specialized version of
gdbpy_ref_base for inferior_object.
The next patch changes those functions again to make them return a
gdbpy_inf_ref, but I think that this intermediary step is useful to
verify the correctness, since the two changes have different goals
(type-safety for this one, reference counting for the next one).
One point I am not sure about is whether it breaks the gcc Python
checking plugin:
extern PyTypeObject inferior_object_type
CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("inferior_object");
It says _FOR_TYPEDEF, but the inferior_object type is no longer a
typedef, because I need to forward declare the type in
python-internal.h, and it's not possible to forward-declare a typedef.
I tried to use the cpychecker plugin but have to admit I am bit lost. I
understand some manual fiddling with the gcc flags are required in order
load the plugin (which can be done with gcc-with-cpychecker). It would
be nice (if it's not the case yet) to have a configure switch to
automatically enable that for the files in the python subdirectory.
gdb/ChangeLog:
* python/py-ref.h (gdbpy_inf_ref): New typedef.
* python/python-internal.h (struct inferior_object):
Forward-declare.
(thread_object) <inf_obj>: Change type to inferior_object*.
(find_inferior_object): Change return type to inferior_object*.
(inferior_to_inferior_object): Likewise.
* python/py-exitedevent.c (create_exited_event_object): Use
gdbpy_inf_ref.
* python/py-inferior.c (inferior_object): Change from typedef +
anonymous struct to simple struct.
(inferior_to_inferior_object): Change return type to
inferior_object*.
(find_inferior_object): Likewise.
(find_thread_object): Use gdbpy_inf_ref.
(build_inferior_list): Likewise.
(gdbpy_selected_inferior): Add cast.
* python/py-infthread.c (thpy_get_inferior): Add cast.
---
gdb/python/py-exitedevent.c | 4 ++--
gdb/python/py-inferior.c | 18 +++++++++---------
gdb/python/py-infthread.c | 2 +-
gdb/python/py-ref.h | 1 +
gdb/python/python-internal.h | 7 ++++---
5 files changed, 17 insertions(+), 15 deletions(-)
diff --git a/gdb/python/py-exitedevent.c b/gdb/python/py-exitedevent.c
index 4590077f8b..30dce9030b 100644
--- a/gdb/python/py-exitedevent.c
+++ b/gdb/python/py-exitedevent.c
@@ -42,10 +42,10 @@ create_exited_event_object (const LONGEST *exit_code, struct inferior *inf)
return NULL;
}
- gdbpy_ref inf_obj (inferior_to_inferior_object (inf));
+ gdbpy_inf_ref inf_obj (inferior_to_inferior_object (inf));
if (inf_obj == NULL || evpy_add_attribute (exited_event.get (),
"inferior",
- inf_obj.get ()) < 0)
+ inf_obj.get_py_obj ()) < 0)
return NULL;
return exited_event.release ();
diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
index 5c215fef44..b6b43af7cd 100644
--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -35,7 +35,7 @@ struct threadlist_entry {
struct threadlist_entry *next;
};
-typedef struct
+struct inferior_object
{
PyObject_HEAD
@@ -48,7 +48,7 @@ typedef struct
/* Number of threads in the list. */
int nthreads;
-} inferior_object;
+};
extern PyTypeObject inferior_object_type
CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("inferior_object");
@@ -207,7 +207,7 @@ python_new_objfile (struct objfile *objfile)
representing INFERIOR. If the object has already been created,
return it and increment the reference count, otherwise, create it.
Return NULL on failure. */
-PyObject *
+inferior_object *
inferior_to_inferior_object (struct inferior *inferior)
{
inferior_object *inf_obj;
@@ -233,13 +233,13 @@ inferior_to_inferior_object (struct inferior *inferior)
else
Py_INCREF ((PyObject *)inf_obj);
- return (PyObject *) inf_obj;
+ return inf_obj;
}
/* Finds the Python Inferior object for the given PID. Returns a
reference, or NULL if PID does not match any inferior object. */
-PyObject *
+inferior_object *
find_inferior_object (int pid)
{
struct inferior *inf = find_inferior_pid (pid);
@@ -260,7 +260,7 @@ find_thread_object (ptid_t ptid)
if (pid == 0)
return NULL;
- gdbpy_ref inf_obj (find_inferior_object (pid));
+ gdbpy_inf_ref inf_obj (find_inferior_object (pid));
if (inf_obj == NULL)
return NULL;
@@ -409,12 +409,12 @@ static int
build_inferior_list (struct inferior *inf, void *arg)
{
PyObject *list = (PyObject *) arg;
- gdbpy_ref inferior (inferior_to_inferior_object (inf));
+ gdbpy_inf_ref inferior (inferior_to_inferior_object (inf));
if (inferior == NULL)
return 0;
- return PyList_Append (list, inferior.get ()) ? 1 : 0;
+ return PyList_Append (list, inferior.get_py_obj ()) ? 1 : 0;
}
/* Implementation of gdb.inferiors () -> (gdb.Inferior, ...).
@@ -815,7 +815,7 @@ py_free_inferior (struct inferior *inf, void *datum)
PyObject *
gdbpy_selected_inferior (PyObject *self, PyObject *args)
{
- return inferior_to_inferior_object (current_inferior ());
+ return (PyObject *) inferior_to_inferior_object (current_inferior ());
}
int
diff --git a/gdb/python/py-infthread.c b/gdb/python/py-infthread.c
index 5482bf9ea1..c7553310c3 100644
--- a/gdb/python/py-infthread.c
+++ b/gdb/python/py-infthread.c
@@ -163,7 +163,7 @@ thpy_get_inferior (PyObject *self, void *ignore)
THPY_REQUIRE_VALID (thread_obj);
- return thread_obj->inf_obj;
+ return (PyObject *) thread_obj->inf_obj;
}
/* Implementation of InferiorThread.switch ().
diff --git a/gdb/python/py-ref.h b/gdb/python/py-ref.h
index 8b3b7732cc..b212ef195f 100644
--- a/gdb/python/py-ref.h
+++ b/gdb/python/py-ref.h
@@ -67,5 +67,6 @@ public:
/* Specializations of gdbpy_ref_base for concrete Python object types. */
typedef gdbpy_ref_base<PyObject> gdbpy_ref;
+typedef gdbpy_ref_base<inferior_object> gdbpy_inf_ref;
#endif /* GDB_PYTHON_REF_H */
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index ac98f12ab3..62a834d403 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -303,6 +303,7 @@ typedef struct gdbpy_breakpoint_object
constructor and the breakpoint-created hook function. */
extern gdbpy_breakpoint_object *bppy_pending_object;
+struct inferior_object;
typedef struct
{
@@ -312,7 +313,7 @@ typedef struct
struct thread_info *thread;
/* The Inferior object to which this thread belongs. */
- PyObject *inf_obj;
+ inferior_object *inf_obj;
} thread_object;
extern struct cmd_list_element *set_python_list;
@@ -420,8 +421,8 @@ PyObject *gdbarch_to_arch_object (struct gdbarch *gdbarch);
thread_object *create_thread_object (struct thread_info *tp);
thread_object *find_thread_object (ptid_t ptid)
CPYCHECKER_RETURNS_BORROWED_REF;
-PyObject *find_inferior_object (int pid);
-PyObject *inferior_to_inferior_object (struct inferior *inferior);
+inferior_object *find_inferior_object (int pid);
+inferior_object *inferior_to_inferior_object (struct inferior *inferior);
const struct block *block_object_to_block (PyObject *obj);
struct symbol *symbol_object_to_symbol (PyObject *obj);
--
2.11.0
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH 1/5] Introduce specialized versions of gdbpy_ref
2017-01-23 22:43 [PATCH 0/5] Improve Python Inferior reference handling + fix a bug Simon Marchi
2017-01-23 22:43 ` [PATCH 5/5] Add missing incref when creating Inferior Python object Simon Marchi
2017-01-23 22:43 ` [PATCH 3/5] Make Python inferior-related internal functions return inferior_object* Simon Marchi
@ 2017-01-23 22:43 ` Simon Marchi
2017-01-24 15:54 ` Tom Tromey
2017-02-09 11:58 ` Pedro Alves
2017-01-23 22:43 ` [PATCH 4/5] Make Python inferior-related internal functions return a gdbpy_inf_ref Simon Marchi
2017-01-23 22:43 ` [PATCH 2/5] Add Python Inferior object debug traces Simon Marchi
4 siblings, 2 replies; 16+ messages in thread
From: Simon Marchi @ 2017-01-23 22:43 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
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(-)
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 */
--
2.11.0
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/5] Introduce specialized versions of gdbpy_ref
2017-01-23 22:43 ` [PATCH 1/5] Introduce specialized versions of gdbpy_ref Simon Marchi
@ 2017-01-24 15:54 ` Tom Tromey
2017-01-24 16:18 ` Simon Marchi
2017-02-09 11:58 ` Pedro Alves
1 sibling, 1 reply; 16+ messages in thread
From: Tom Tromey @ 2017-01-24 15:54 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches, Simon Marchi
>>>>> "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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/5] Introduce specialized versions of gdbpy_ref
2017-01-24 15:54 ` Tom Tromey
@ 2017-01-24 16:18 ` Simon Marchi
0 siblings, 0 replies; 16+ messages in thread
From: Simon Marchi @ 2017-01-24 16:18 UTC (permalink / raw)
To: Tom Tromey; +Cc: Simon Marchi, gdb-patches
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/5] Introduce specialized versions of gdbpy_ref
2017-01-23 22:43 ` [PATCH 1/5] Introduce specialized versions of gdbpy_ref Simon Marchi
2017-01-24 15:54 ` Tom Tromey
@ 2017-02-09 11:58 ` Pedro Alves
2017-02-09 16:18 ` Simon Marchi
1 sibling, 1 reply; 16+ messages in thread
From: Pedro Alves @ 2017-02-09 11:58 UTC (permalink / raw)
To: Simon Marchi, gdb-patches; +Cc: Simon Marchi
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/5] Introduce specialized versions of gdbpy_ref
2017-02-09 11:58 ` Pedro Alves
@ 2017-02-09 16:18 ` Simon Marchi
0 siblings, 0 replies; 16+ messages in thread
From: Simon Marchi @ 2017-02-09 16:18 UTC (permalink / raw)
To: Pedro Alves; +Cc: Simon Marchi, gdb-patches
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 4/5] Make Python inferior-related internal functions return a gdbpy_inf_ref
2017-01-23 22:43 [PATCH 0/5] Improve Python Inferior reference handling + fix a bug Simon Marchi
` (2 preceding siblings ...)
2017-01-23 22:43 ` [PATCH 1/5] Introduce specialized versions of gdbpy_ref Simon Marchi
@ 2017-01-23 22:43 ` Simon Marchi
2017-01-24 16:15 ` Simon Marchi
2017-02-09 12:30 ` Pedro Alves
2017-01-23 22:43 ` [PATCH 2/5] Add Python Inferior object debug traces Simon Marchi
4 siblings, 2 replies; 16+ messages in thread
From: Simon Marchi @ 2017-01-23 22:43 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
From: Simon Marchi <simon.marchi@polymtl.ca>
The functions inferior_to_inferior_object and find_inferior_object
return a new reference to an inferior_object. This means that the
caller owns that reference and is responsible for decrementing it when
it's done. To avoid the possibility of the caller forgetting to DECREF
when it's done with the reference, make those functions return a
gdbpy_inf_ref instead of a plain pointer.
If the caller doesn't need the reference after it has used it,
gdbpy_inf_ref will take care of removing that reference. If the
reference needs to outlive the gdbpy_inf_ref object (e.g. because we are
return the value to Python, which will take ownership of the reference),
the caller will have to release the pointer. At least it will be
explicit and it won't be ambiguous.
I added comments in inferior_to_inferior_object for the poor souls who
will have to deal with this again in the future.
A couple of things I am not sure about:
* I am not sure whether the behaviour is right with the assignment
operator in delete_thread_object, so if somebody could take a look at
that in particular it would be appreciated:
gdbpy_inf_ref inf_obj_ref = find_inferior_object (ptid_get_pid (tp->ptid));
I suppose it's the operator= version which moves the reference that is
invoked?
* I used .release() on the reference in create_thread_object with a
comment explaining why, but I would need some more pairs of eyes on
that to see if it's right.
gdb/ChangeLog:
* python/py-inferior.c (inferior_to_inferior_object): Return a
gdbpy_inf_ref.
(find_inferior_object): Return a gdbpy_inf_ref.
(delete_thread_object): Use gdbpy_inf_ref.
(gdbpy_selected_inferior): Likewise.
* python/py-infthread.c (create_thread_object): Adapt to
gdbpy_inf_ref.
* python/python-internal.h: Include py-ref.h.
(inferior_to_inferior_object): Return a gdbpy_inf_ref.
(find_inferior_object): Likewise.
---
gdb/python/py-inferior.c | 41 +++++++++++++++++++----------------------
gdb/python/py-infthread.c | 6 +++++-
gdb/python/py-ref.h | 2 ++
gdb/python/python-internal.h | 6 ++++--
4 files changed, 30 insertions(+), 25 deletions(-)
diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
index b6b43af7cd..340dddcfbd 100644
--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -207,39 +207,38 @@ python_new_objfile (struct objfile *objfile)
representing INFERIOR. If the object has already been created,
return it and increment the reference count, otherwise, create it.
Return NULL on failure. */
-inferior_object *
+gdbpy_inf_ref
inferior_to_inferior_object (struct inferior *inferior)
{
inferior_object *inf_obj;
inf_obj = (inferior_object *) inferior_data (inferior, infpy_inf_data_key);
- if (!inf_obj)
+ if (inf_obj == NULL)
{
if (debug_python)
printf_filtered ("Creating Python Inferior object inf = %d\n",
inferior->num);
inf_obj = PyObject_New (inferior_object, &inferior_object_type);
- if (!inf_obj)
- return NULL;
+ if (inf_obj == NULL)
+ return gdbpy_inf_ref ();
inf_obj->inferior = inferior;
inf_obj->threads = NULL;
inf_obj->nthreads = 0;
set_inferior_data (inferior, infpy_inf_data_key, inf_obj);
-
}
else
Py_INCREF ((PyObject *)inf_obj);
- return inf_obj;
+ return gdbpy_inf_ref (inf_obj);
}
/* Finds the Python Inferior object for the given PID. Returns a
reference, or NULL if PID does not match any inferior object. */
-inferior_object *
+gdbpy_inf_ref
find_inferior_object (int pid)
{
struct inferior *inf = find_inferior_pid (pid);
@@ -247,7 +246,7 @@ find_inferior_object (int pid)
if (inf)
return inferior_to_inferior_object (inf);
- return NULL;
+ return gdbpy_inf_ref ();
}
thread_object *
@@ -304,39 +303,34 @@ add_thread_object (struct thread_info *tp)
static void
delete_thread_object (struct thread_info *tp, int ignore)
{
- inferior_object *inf_obj;
struct threadlist_entry **entry, *tmp;
if (!gdb_python_initialized)
return;
gdbpy_enter enter_py (python_gdbarch, python_language);
+ gdbpy_inf_ref inf_obj_ref = find_inferior_object (ptid_get_pid (tp->ptid));
- inf_obj
- = (inferior_object *) find_inferior_object (ptid_get_pid (tp->ptid));
- if (!inf_obj)
+ if (inf_obj_ref == NULL)
return;
/* Find thread entry in its inferior's thread_list. */
- for (entry = &inf_obj->threads; *entry != NULL; entry =
- &(*entry)->next)
+ for (entry = &inf_obj_ref.get ()->threads;
+ *entry != NULL;
+ entry = &(*entry)->next)
if ((*entry)->thread_obj->thread == tp)
break;
- if (!*entry)
- {
- Py_DECREF (inf_obj);
- return;
- }
+ if (*entry == NULL)
+ return;
tmp = *entry;
tmp->thread_obj->thread = NULL;
*entry = (*entry)->next;
- inf_obj->nthreads--;
+ inf_obj_ref.get ()->nthreads--;
Py_DECREF (tmp->thread_obj);
- Py_DECREF (inf_obj);
xfree (tmp);
}
@@ -815,7 +809,10 @@ py_free_inferior (struct inferior *inf, void *datum)
PyObject *
gdbpy_selected_inferior (PyObject *self, PyObject *args)
{
- return (PyObject *) inferior_to_inferior_object (current_inferior ());
+ gdbpy_inf_ref inf_obj_ref (inferior_to_inferior_object (current_inferior ()));
+
+ /* Release the reference, it will now be managed by Python. */
+ return (PyObject *) inf_obj_ref.release ();
}
int
diff --git a/gdb/python/py-infthread.c b/gdb/python/py-infthread.c
index c7553310c3..79fb5d12d1 100644
--- a/gdb/python/py-infthread.c
+++ b/gdb/python/py-infthread.c
@@ -46,7 +46,11 @@ create_thread_object (struct thread_info *tp)
return NULL;
thread_obj->thread = tp;
- thread_obj->inf_obj = find_inferior_object (ptid_get_pid (tp->ptid));
+
+ /* The thread holds a weak reference to its inferior to avoid creating a
+ reference loop between the inferior and its threads. */
+ gdbpy_inf_ref inf_obj_ref = find_inferior_object (ptid_get_pid (tp->ptid));
+ thread_obj->inf_obj = inf_obj_ref.release ();
return thread_obj;
}
diff --git a/gdb/python/py-ref.h b/gdb/python/py-ref.h
index b212ef195f..03c0304c74 100644
--- a/gdb/python/py-ref.h
+++ b/gdb/python/py-ref.h
@@ -67,6 +67,8 @@ public:
/* Specializations of gdbpy_ref_base for concrete Python object types. */
typedef gdbpy_ref_base<PyObject> gdbpy_ref;
+
+struct inferior_object;
typedef gdbpy_ref_base<inferior_object> gdbpy_inf_ref;
#endif /* GDB_PYTHON_REF_H */
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 62a834d403..a7fa9aa4bc 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -94,6 +94,8 @@
#include <Python.h>
#include <frameobject.h>
+#include "py-ref.h"
+
#if PY_MAJOR_VERSION >= 3
#define IS_PY3K 1
#endif
@@ -421,8 +423,8 @@ PyObject *gdbarch_to_arch_object (struct gdbarch *gdbarch);
thread_object *create_thread_object (struct thread_info *tp);
thread_object *find_thread_object (ptid_t ptid)
CPYCHECKER_RETURNS_BORROWED_REF;
-inferior_object *find_inferior_object (int pid);
-inferior_object *inferior_to_inferior_object (struct inferior *inferior);
+gdbpy_inf_ref find_inferior_object (int pid);
+gdbpy_inf_ref inferior_to_inferior_object (struct inferior *inferior);
const struct block *block_object_to_block (PyObject *obj);
struct symbol *symbol_object_to_symbol (PyObject *obj);
--
2.11.0
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 4/5] Make Python inferior-related internal functions return a gdbpy_inf_ref
2017-01-23 22:43 ` [PATCH 4/5] Make Python inferior-related internal functions return a gdbpy_inf_ref Simon Marchi
@ 2017-01-24 16:15 ` Simon Marchi
2017-02-09 12:30 ` Pedro Alves
1 sibling, 0 replies; 16+ messages in thread
From: Simon Marchi @ 2017-01-24 16:15 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches
On 2017-01-23 17:40, Simon Marchi wrote:
> * I used .release() on the reference in create_thread_object with a
> comment explaining why, but I would need some more pairs of eyes on
> that to see if it's right.
Ok, so I've looked at this a bit more, and I think that my comment is
wrong, but the code is right (meaning that the comment didn't match the
code in the first place).
> diff --git a/gdb/python/py-infthread.c b/gdb/python/py-infthread.c
> index c7553310c3..79fb5d12d1 100644
> --- a/gdb/python/py-infthread.c
> +++ b/gdb/python/py-infthread.c
> @@ -46,7 +46,11 @@ create_thread_object (struct thread_info *tp)
> return NULL;
>
> thread_obj->thread = tp;
> - thread_obj->inf_obj = find_inferior_object (ptid_get_pid
> (tp->ptid));
> +
> + /* The thread holds a weak reference to its inferior to avoid
> creating a
> + reference loop between the inferior and its threads. */
> + gdbpy_inf_ref inf_obj_ref = find_inferior_object (ptid_get_pid
> (tp->ptid));
> + thread_obj->inf_obj = inf_obj_ref.release ();
The Thread objects do hold strong references to their Inferior object,
and it's not really a problem. The only way for an Inferior object to
be deallocated is if the corresponding gdb inferior object is removed.
A prerequisite of this happening is that all its threads have exited
(either by finishing themselves or by being killed by gdb). When a
thread exits, we remove the reference from the Inferior to the Thread.
If no other reference to the Thread exist (e.g. a Python variable), the
Thread will be deallocated immediately, removing the reference it had to
the Inferior.
The reference cycle should only exist while the threads are running,
which is not a state in which we can nor want to deallocate the Inferior
object. As soon as the threads exit, the cycle is broken.
So the .release() is right, the corresponding DECREF is in thpy_dealloc.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 4/5] Make Python inferior-related internal functions return a gdbpy_inf_ref
2017-01-23 22:43 ` [PATCH 4/5] Make Python inferior-related internal functions return a gdbpy_inf_ref Simon Marchi
2017-01-24 16:15 ` Simon Marchi
@ 2017-02-09 12:30 ` Pedro Alves
2017-02-09 16:39 ` Simon Marchi
1 sibling, 1 reply; 16+ messages in thread
From: Pedro Alves @ 2017-02-09 12:30 UTC (permalink / raw)
To: Simon Marchi, gdb-patches; +Cc: Simon Marchi
On 01/23/2017 10:40 PM, Simon Marchi wrote:
> From: Simon Marchi <simon.marchi@polymtl.ca>
>
> The functions inferior_to_inferior_object and find_inferior_object
> return a new reference to an inferior_object. This means that the
> caller owns that reference and is responsible for decrementing it when
> it's done. To avoid the possibility of the caller forgetting to DECREF
> when it's done with the reference, make those functions return a
> gdbpy_inf_ref instead of a plain pointer.
I like this style of API. I've argued for it before too.
> If the caller doesn't need the reference after it has used it,
> gdbpy_inf_ref will take care of removing that reference. If the
> reference needs to outlive the gdbpy_inf_ref object (e.g. because we are
> return the value to Python, which will take ownership of the reference),
> the caller will have to release the pointer. At least it will be
> explicit and it won't be ambiguous.
>
> I added comments in inferior_to_inferior_object for the poor souls who
> will have to deal with this again in the future.
>
> A couple of things I am not sure about:
>
> * I am not sure whether the behaviour is right with the assignment
> operator in delete_thread_object, so if somebody could take a look at
> that in particular it would be appreciated:
>
> gdbpy_inf_ref inf_obj_ref = find_inferior_object (ptid_get_pid (tp->ptid));
>
> I suppose it's the operator= version which moves the reference that is
> invoked?
Since this is initialization, op= is not called. This either
calls the copy constructor, or find_inferior_object constructs the
object that it returns directly on top of &inf_obj_ref
(i.e., no copy at all) [RVO/NRVO].
http://stackoverflow.com/questions/2847787/constructor-or-assignment-operator
https://en.wikipedia.org/wiki/Return_value_optimization
> @@ -207,39 +207,38 @@ python_new_objfile (struct objfile *objfile)
> representing INFERIOR. If the object has already been created,
> return it and increment the reference count, otherwise, create it.
> Return NULL on failure. */
> -inferior_object *
> +gdbpy_inf_ref
> inferior_to_inferior_object (struct inferior *inferior)
> {
...
> - if (!inf_obj)
> - return NULL;
> + if (inf_obj == NULL)
> + return gdbpy_inf_ref ();
You shouldn't need changes like this one. gdbpy_ref has an
implicit ctor that takes nullptr_t exactly to allow implicit
construction from null.
>
> @@ -304,39 +303,34 @@ add_thread_object (struct thread_info *tp)
> static void
> delete_thread_object (struct thread_info *tp, int ignore)
> {
> - inferior_object *inf_obj;
> struct threadlist_entry **entry, *tmp;
>
> if (!gdb_python_initialized)
> return;
>
> gdbpy_enter enter_py (python_gdbarch, python_language);
> + gdbpy_inf_ref inf_obj_ref = find_inferior_object (ptid_get_pid (tp->ptid));
>
> - inf_obj
> - = (inferior_object *) find_inferior_object (ptid_get_pid (tp->ptid));
> - if (!inf_obj)
> + if (inf_obj_ref == NULL)
> return;
>
> /* Find thread entry in its inferior's thread_list. */
> - for (entry = &inf_obj->threads; *entry != NULL; entry =
> - &(*entry)->next)
> + for (entry = &inf_obj_ref.get ()->threads;
Hmm, changes like these are odd. gdbpy_ref has an operator->
implementation, so inf_obj->threads should do the right thing?
> @@ -815,7 +809,10 @@ py_free_inferior (struct inferior *inf, void *datum)
> PyObject *
> gdbpy_selected_inferior (PyObject *self, PyObject *args)
> {
> - return (PyObject *) inferior_to_inferior_object (current_inferior ());
> + gdbpy_inf_ref inf_obj_ref (inferior_to_inferior_object (current_inferior ()));
If the function returns gdbpy_inf_ref already, I much prefer
using = initialization over (), like:
gdbpy_inf_ref inf_obj_ref
= inferior_to_inferior_object (current_inferior ());
The reason is that this makes it more obvious what is going on.
The ctor taking a PyObject* is explicit so inferior_to_inferior_object
must be returning a gdbpy_inf_ref.
With:
gdbpy_inf_ref inf_obj_ref (inferior_to_inferior_object (current_inferior ()));
one has to wonder what constructor is being called, and whether there's
some kind of explicit conversion going on.
So the = version is more to the point and thus makes it
for a clearer read because there's less to reason about.
> +
> + /* Release the reference, it will now be managed by Python. */
> + return (PyObject *) inf_obj_ref.release ();
> }
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 4/5] Make Python inferior-related internal functions return a gdbpy_inf_ref
2017-02-09 12:30 ` Pedro Alves
@ 2017-02-09 16:39 ` Simon Marchi
0 siblings, 0 replies; 16+ messages in thread
From: Simon Marchi @ 2017-02-09 16:39 UTC (permalink / raw)
To: Pedro Alves; +Cc: Simon Marchi, gdb-patches, Tom Tromey
Thanks for the comments. I'll update my branch, but I'll wait until
Tom's series is pushed and see what's still relevant in mine.
On 2017-02-09 07:30, Pedro Alves wrote:
>> @@ -207,39 +207,38 @@ python_new_objfile (struct objfile *objfile)
>> representing INFERIOR. If the object has already been created,
>> return it and increment the reference count, otherwise, create
>> it.
>> Return NULL on failure. */
>> -inferior_object *
>> +gdbpy_inf_ref
>> inferior_to_inferior_object (struct inferior *inferior)
>> {
> ...
>> - if (!inf_obj)
>> - return NULL;
>> + if (inf_obj == NULL)
>> + return gdbpy_inf_ref ();
>
> You shouldn't need changes like this one. gdbpy_ref has an
> implicit ctor that takes nullptr_t exactly to allow implicit
> construction from null.
Ok. This required adding the corresponding constructor in
gdbpy_ref_base:
gdbpy_ref_base (const std::nullptr_t)
: gdb::ref_ptr<T, gdbpy_ref_policy<T>> (nullptr)
{
}
>> /* Find thread entry in its inferior's thread_list. */
>> - for (entry = &inf_obj->threads; *entry != NULL; entry =
>> - &(*entry)->next)
>> + for (entry = &inf_obj_ref.get ()->threads;
>
> Hmm, changes like these are odd. gdbpy_ref has an operator->
> implementation, so inf_obj->threads should do the right thing?
Hmm you're right, not sure why I added those.
>> @@ -815,7 +809,10 @@ py_free_inferior (struct inferior *inf, void
>> *datum)
>> PyObject *
>> gdbpy_selected_inferior (PyObject *self, PyObject *args)
>> {
>> - return (PyObject *) inferior_to_inferior_object (current_inferior
>> ());
>> + gdbpy_inf_ref inf_obj_ref (inferior_to_inferior_object
>> (current_inferior ()));
>
> If the function returns gdbpy_inf_ref already, I much prefer
> using = initialization over (), like:
>
> gdbpy_inf_ref inf_obj_ref
> = inferior_to_inferior_object (current_inferior ());
>
> The reason is that this makes it more obvious what is going on.
> The ctor taking a PyObject* is explicit so inferior_to_inferior_object
> must be returning a gdbpy_inf_ref.
>
> With:
>
> gdbpy_inf_ref inf_obj_ref (inferior_to_inferior_object
> (current_inferior ()));
>
> one has to wonder what constructor is being called, and whether there's
> some kind of explicit conversion going on.
>
> So the = version is more to the point and thus makes it
> for a clearer read because there's less to reason about.
Right, it's more obvious.
Thanks,
Simon
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/5] Add Python Inferior object debug traces
2017-01-23 22:43 [PATCH 0/5] Improve Python Inferior reference handling + fix a bug Simon Marchi
` (3 preceding siblings ...)
2017-01-23 22:43 ` [PATCH 4/5] Make Python inferior-related internal functions return a gdbpy_inf_ref Simon Marchi
@ 2017-01-23 22:43 ` Simon Marchi
4 siblings, 0 replies; 16+ messages in thread
From: Simon Marchi @ 2017-01-23 22:43 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
I added those printfs while trying to debug the refcount issue fixed by
a following patch. I thought it could be useful to have debug traces
for anything Python-related. I have only added prints to track Inferior
the lifetime of Inferior objects because that's all I needed, but we
might want to add the same for other types in the future.
While debugging, I also made some kind of "maint python refcounts" that
would print the refcounts of each known Inferior object. If you think
it could be useful I could dig it up.
gdb/ChangeLog:
* python/python-internal.h (debug_python): New variable
declaration.
* python/python.c (debug_python): New variable.
(_initialize_python): Add "set debug python" command.
* python/py-inferior.c (inferior_to_inferior_object): Add debug
print.
(infpy_dealloc): Likewise.
(py_free_inferior): Likewise.
---
gdb/python/py-inferior.c | 15 +++++++++++++++
gdb/python/python-internal.h | 4 ++++
gdb/python/python.c | 12 ++++++++++++
3 files changed, 31 insertions(+)
diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
index b2aaf25e8a..5c215fef44 100644
--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -215,6 +215,10 @@ inferior_to_inferior_object (struct inferior *inferior)
inf_obj = (inferior_object *) inferior_data (inferior, infpy_inf_data_key);
if (!inf_obj)
{
+ if (debug_python)
+ printf_filtered ("Creating Python Inferior object inf = %d\n",
+ inferior->num);
+
inf_obj = PyObject_New (inferior_object, &inferior_object_type);
if (!inf_obj)
return NULL;
@@ -758,6 +762,14 @@ infpy_dealloc (PyObject *obj)
inferior_object *inf_obj = (inferior_object *) obj;
struct inferior *inf = inf_obj->inferior;
+ if (debug_python)
+ {
+ if (inf != NULL)
+ printf_filtered ("infpy_dealloc inf = %d\n", inf->num);
+ else
+ printf_filtered ("infpy_dealloc inf = <unknown>\n");
+ }
+
if (! inf)
return;
@@ -777,6 +789,9 @@ py_free_inferior (struct inferior *inf, void *datum)
gdbpy_enter enter_py (python_gdbarch, python_language);
+ if (debug_python)
+ printf_filtered ("py_free_inferior inf = %d\n", inf->num);
+
inf_obj->inferior = NULL;
/* Deallocate threads list. */
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index e2ebc1b8a2..ac98f12ab3 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -240,6 +240,10 @@ struct inferior;
extern int gdb_python_initialized;
+/* True if debug output about Python features is enabled. */
+
+extern int debug_python;
+
extern PyObject *gdb_module;
extern PyObject *gdb_python_module;
extern PyTypeObject value_object_type
diff --git a/gdb/python/python.c b/gdb/python/python.c
index ab5a6a416d..dc0fa25054 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -106,6 +106,10 @@ const struct extension_language_defn extension_language_python =
int gdb_python_initialized;
+/* See python-internal.h */
+
+int debug_python = 0;
+
extern PyMethodDef python_GdbMethods[];
#ifdef IS_PY3K
@@ -1605,6 +1609,14 @@ message == an error message without a stack will be printed."),
&user_set_python_list,
&user_show_python_list);
+ add_setshow_boolean_cmd ("python", class_maintenance,
+ &debug_python, _("\
+Set Python debug output."), _("\
+Show Python debug output."), _("\
+When on, debugging output for Python-related features is displayed."),
+ NULL, NULL,
+ &setdebuglist, &showdebuglist);
+
#ifdef HAVE_PYTHON
#ifdef WITH_PYTHON_PATH
/* Work around problem where python gets confused about where it is,
--
2.11.0
^ permalink raw reply [flat|nested] 16+ messages in thread