This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 3/4] Change thread_to_thread_object to return a new reference
>>>>> "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<>