Re: [PATCH 3/4] Change thread_to_thread_object to return a new reference

Tom Tromey
2018-09-16T14:05:16Z

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> + if (result == NULL)
>> + result = gdbpy_ref<>::new_reference (Py_None);

Simon> I would suggest using Py_RETURN_NONE, which we already use at many places.

Simon> Instead of setting that result variable, is there something preventing you
Simon> from returning directly above, like this?

Simon> if (thread_info != NULL)
Simon> return thread_to_thread_object (thread_info).release ();

Simon> The bottom line could just be unconditionally Py_RETURN_NONE.

I made this change.

In the past you couldn't return from inside a try/catch but I think it
is ok now.

I couldn't use Py_RETURN_NONE in py_get_event_thread because it returns
a gdbpy_ref<> -- and I think the latter is more important.

I re-read all of this and I think thread_to_thread_object has a latent
bug. It can return NULL early due to an error:

gdbpy_ref<inferior_object> inf_obj (inferior_to_inferior_object (thr->inf));
if (inf_obj == NULL)
return NULL;

or it can return NULL at the end meaning "thread not found".

I think it is best to have a single style - returning NULL should also
set the Python exception.

It seems to me that the second NULL return should just set an
exception. It's not a normal case.

Let me know what you think of this.

My apologies again for missing this review and pushing too eagerly.

Tom

commit 0a58a05fb6574c0e9c84c5160e99f5c922a6a0eb
Author: Tom Tromey <tom@tromey.com>
Date: Sun Sep 16 08:02:22 2018 -0600

Simplify uses of thread_to_thread_object

An review by Simon of an earlier showed a few spots related to
thread_to_thread_object that could be simplified. This also detected
a latent bug, where thread_to_thread_object was inconsistent about
setting the Python exception before a NULL return.

Tested on x86-64 Fedora 28.

gdb/ChangeLog
2018-09-16 Tom Tromey <tom@tromey.com>

* python/py-threadevent.c (py_get_event_thread): Simplify.
* python/py-inferior.c (infpy_thread_from_thread_handle):
Return immediately after calling thread_to_thread_object. Use
Py_RETURN_NONE.
(thread_to_thread_object): Set the exception on a NULL return.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index d36f6cde426..492fcad4120 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2018-09-16 Tom Tromey <tom@tromey.com>
+
+ * python/py-threadevent.c (py_get_event_thread): Simplify.
+ * python/py-inferior.c (infpy_thread_from_thread_handle):
+ Return immediately after calling thread_to_thread_object. Use
+ Py_RETURN_NONE.
+ (thread_to_thread_object): Set the exception on a NULL return.
+
2018-09-16 Tom Tromey <tom@tromey.com>

* python/python-internal.h (CPYCHECKER_RETURNS_BORROWED_REF):
diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
index 68c4c9d411e..145f53d14af 100644
--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -319,6 +319,8 @@ thread_to_thread_object (thread_info *thr)
if (thread->thread_obj->thread == thr)
return gdbpy_ref<>::new_reference ((PyObject *) thread->thread_obj);

+ PyErr_SetString (PyExc_SystemError,
+ _("could not find gdb thread object"));
return NULL;
}

@@ -849,7 +851,6 @@ infpy_thread_from_thread_handle (PyObject *self, PyObject *args, PyObject *kw)
return NULL;
}

- gdbpy_ref<> result;
TRY
{
struct thread_info *thread_info;
@@ -857,7 +858,7 @@ infpy_thread_from_thread_handle (PyObject *self, PyObject *args, PyObject *kw)

thread_info = find_thread_by_handle (val, inf_obj->inferior);
if (thread_info != NULL)
- result = thread_to_thread_object (thread_info);
+ return thread_to_thread_object (thread_info).release ();
}
CATCH (except, RETURN_MASK_ALL)
{
@@ -865,10 +866,7 @@ infpy_thread_from_thread_handle (PyObject *self, PyObject *args, PyObject *kw)
}
END_CATCH

- if (result == NULL)
- result = gdbpy_ref<>::new_reference (Py_None);
-
- return result.release ();
+ Py_RETURN_NONE;
}

/* Implement repr() for gdb.Inferior. */
diff --git a/gdb/python/py-threadevent.c b/gdb/python/py-threadevent.c
index 13af1c840ba..ea62540ff84 100644
--- a/gdb/python/py-threadevent.c
+++ b/gdb/python/py-threadevent.c
@@ -25,24 +25,15 @@
gdbpy_ref<>
py_get_event_thread (ptid_t ptid)
{
- gdbpy_ref<> pythread;
-
if (non_stop)
{
thread_info *thread = find_thread_ptid (ptid);
if (thread != nullptr)
- pythread = thread_to_thread_object (thread);
- }
- else
- pythread = gdbpy_ref<>::new_reference (Py_None);
-
- if (pythread == nullptr)
- {
+ return thread_to_thread_object (thread);
PyErr_SetString (PyExc_RuntimeError, "Could not find event thread");
return NULL;
}
-
- return pythread;
+ return gdbpy_ref<>::new_reference (Py_None);
}

gdbpy_ref<>