Re: [python][patch] Inferior and Thread information support.

[Tom Tromey <tromey@redhat.com>]

>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:

Phil> I've done this.  Threads do not have the same registry mechanism yet, so
Phil> I've had to keep the old observer mechanism there.  But inferiors use
Phil> the new registry cleanup.

Phil> I've also simplified the search_memory api as requested.  We now take
Phil> an address, a length and a buffer only.  We now only return the first
Phil> match found in the buffer. (and let the user do the iteration in the
Phil> scripting side).

Phil> Latest patch attached. WDYT?

Phil> +PyObject *
Phil> +inferior_to_inferior_object (struct inferior *inferior)
Phil> +{
Phil> +  inferior_object *inf_obj;
Phil> +
Phil> +  inf_obj = inferior_data (inferior, infpy_inf_data_key);
Phil> +  if (!inf_obj)
Phil> +    {
Phil> +      struct cleanup *cleanup;
Phil> +      cleanup = ensure_python_env (python_gdbarch, python_language);
Phil> +
Phil> +      inf_obj = PyObject_New (inferior_object, &inferior_object_type);
[...]
Phil> +      do_cleanups (cleanup);
Phil> +    }
Phil> +  else
Phil> +    Py_INCREF (inf_obj);
Phil> +
Phil> +  return (PyObject *) inf_obj;

Right now the reference counting is inconsistent here.  Either a new
reference should be returned, and the Py_INCREF always done; or it
should return a borrowed reference, and this Py_INCREF removed.  I
prefer the borrowed reference approach.  I try to document this sort of
thing in the function's header comment.

Phil> +  cleanup = ensure_python_env (python_gdbarch, python_language);
Phil> +
Phil> +  thread_obj = create_thread_object (tp);
Phil> +  if (!thread_obj)
Phil> +    {
Phil> +      warning (_("Cannot create Python InferiorThread object."));
Phil> +      gdbpy_print_stack ();
Phil> +      do_cleanups (cleanup);

I don't think there is any need for a warning here.
The exception should suffice.

Phil> +static PyObject *
Phil> +infpy_get_was_attached (PyObject *self, void *closure)
Phil> +{
Phil> +  inferior_object *inf = (inferior_object *) self;
Phil> +  INFPY_REQUIRE_VALID (inf);

You need a blank line between these two lines now.

Phil> +static int
Phil> +build_inferior_list (struct inferior *inf, void *arg)
Phil> +{
Phil> +  PyObject *list = arg;
Phil> +  PyObject *inferior = inferior_to_inferior_object (inf);
Phil> +  PyList_Append (list, inferior);

Likewise.

Phil> +/* Implementation of gdb.read_memory (address, length).
Phil> +   Returns a Python buffer object with LENGTH bytes of the inferior's
Phil> +   memory at ADDRESS.  Both arguments are integers.  */
Phil> +static PyObject *
Phil> +infpy_read_memory (PyObject *self, PyObject *args)
Phil> +{
Phil> +  int error = 0;
Phil> +  CORE_ADDR addr, length;
Phil> +  void *buffer = NULL;
Phil> +  membuf_object *membuf_obj;
Phil> +  PyObject *addr_obj, *length_obj;
Phil> +  struct cleanup *cleanups;
Phil> +  volatile struct gdb_exception except;
Phil> +
Phil> +  if (! PyArg_ParseTuple (args, "OO", &addr_obj, &length_obj))
Phil> +    return NULL;

I hate to add on, but I think all functions taking 2 or more arguments
should accept keyword arguments.

Phil> +  cleanups = make_cleanup (null_cleanup, NULL);
Phil> +
Phil> +  TRY_CATCH (except, RETURN_MASK_ALL)
Phil> +    {
Phil> +      if (!get_addr_from_python (addr_obj, &addr)
Phil> +	  || !get_addr_from_python (length_obj, &length))
Phil> +	{
Phil> +	  error = 1;
Phil> +	  break;
Phil> +	}
Phil> +
Phil> +      buffer = xmalloc (length);
Phil> +      make_cleanup (xfree, buffer);
Phil> +
Phil> +      read_memory (addr, buffer, length);
Phil> +    }
Phil> +  GDB_PY_HANDLE_EXCEPTION (except);

GDB_PY_HANDLE_EXCEPTION returns from the function; this will leave the
cleanups dangling.  You have to run the cleanups before the return.

Phil> +static PyObject *
Phil> +infpy_write_memory (PyObject *self, PyObject *args)
Phil> +{
Phil> +  int buf_len, error = 0;
Phil> +  const char *buffer;
Phil> +  CORE_ADDR addr, length;
Phil> +  PyObject *addr_obj, *length_obj = NULL;
Phil> +  volatile struct gdb_exception except;
Phil> +
Phil> +  if (! PyArg_ParseTuple (args, "Os#|O", &addr_obj, &buffer, &buf_len,
Phil> +			  &length_obj))
Phil> +    return NULL;

Likewise about the keyword arguments.

Phil> +/* Implementation of InferiorThread.newest_frame () -> gdb.Frame.
Phil> +   Returns the newest frame object.  */
Phil> +PyObject *
Phil> +thpy_newest_frame (PyObject *self, PyObject *args)

This function should be static...

But, do we want it at all?  If the frame comes from some other thread,
there is almost no useful operation we can do with it.

That said I think the code for the function is fine.  If we have some
use case then I'm ok with it.

Tom