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

[Tom Tromey <tom@tromey.com>]

>>>>> "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<>