[3/4] Change thread_to_thread_object to return a new reference
Commit Message
>>>>> "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.
Comments
>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
Tom> I think it is best to have a single style - returning NULL should also
Tom> set the Python exception.
Maybe the gdbpy_ref (1-argument) constructor and release methods could
assert that the Python exception is set if the underlying pointer is
NULL. That would not get full checking but maybe it would catch some
problems. And maybe we should simply use gdbpy_ref in many more places
in the Python layer -- ideally, reserve raw pointers solely for
parameters which are borrowed references.
Tom
On 2018-09-16 10:05, Tom Tromey wrote:
>>>>>> "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.
Oh right. While working on this code I though adding a
GDB_Py_REF_RETURN_NONE macro, which would be like Py_RETURN_NONE but
returning a gdbpy_ref. Not sure it's really that useful, since what you
wrote is explicit and clear.
> 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 am not sure how these two mode of failure differ. Don't we expect the
passed thread_info object to always be valid, and therefore both cases
returning NULL would be an internal GDB logic error?
> I think it is best to have a single style - returning NULL should also
> set the Python exception.
That makes sense, so the caller can just check for NULL and return NULL
right away (as the snippet above does). We should expect
inferior_to_inferior_object to set the exception on failure.
> It seems to me that the second NULL return should just set an
> exception. It's not a normal case.
Right.
Responding directly to your next message:
> Maybe the gdbpy_ref (1-argument) constructor and release methods could
> assert that the Python exception is set if the underlying pointer is
> NULL. That would not get full checking but maybe it would catch some
> problems. And maybe we should simply use gdbpy_ref in many more places
> in the Python layer -- ideally, reserve raw pointers solely for
> parameters which are borrowed references.
Makes sense.
> Let me know what you think of this.
>
> My apologies again for missing this review and pushing too eagerly.
No problem, LGTM.
Simon
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
Simon> On 2018-09-16 10:05, Tom Tromey wrote:
>> 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".
Simon> I am not sure how these two mode of failure differ. Don't we expect
Simon> the passed thread_info object to always be valid, and therefore both
Simon> cases returning NULL would be an internal GDB logic error?
I just meant that the difference here is whether or not the Python
exception is set -- that this function isn't following the normal,
simple convention here, but that I think it should.
Tom
@@ -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):
@@ -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. */
@@ -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<>