Re: [patch] Support inferior events in python

[Tom Tromey <tromey@redhat.com>]

>>>>> "Sami" == sami wagiaalla <swagiaal@redhat.com> writes:

Sami> This patch is base on the work done by Oguz Kayral in the summer of
Sami> 2009. Tom Tromey and Phil Muldoon also provided a lot of help and
Sami> guidance on this.

Thanks for doing this.  This is a very important feature.

Sami> A documentation patch will follow soon.

I think it is better for a patch to include documentation and, when
applicable (it is for this patch), a NEWS entry.

Sami> Thanks in advance for the review.

One thing I noticed is that now all the copyright headers should be
updated to include 2011 :-)

Sami> +breakpoint_event_object *
Sami> +create_breakpoint_event_object (struct bpstats *bs, thread_object *stopped_thread)

Wrap this line at the ",".

Sami> +continue_event_object *
Sami> +create_continue_event_object ()

Change to (void).

Sami> +/* Callback function which notifies observers when a continue event occurs.
Sami> +   This function will create a new Python continue event object.
Sami> +   Return -1 if emit fails.  */
Sami> +
Sami> +int
Sami> +emit_continue_event (ptid_t ptid)
Sami> +{
Sami> +  event_object *event;
Sami> +  if (evregpy_get_nlisteners (gdb_py_events.cont) == 0)

Newline between the above 2 lines.

Instead of evregpy_get_nlisteners checking the list size, I think it
would be better to have "evregpy_any_listeners_p" return a boolean, and
then check whether the list is empty.  This may be more efficient and is
what you want to do anyhow.

Sami> +    return evpy_emit_event (event,
Sami> +                            gdb_py_events.cont);

I don't think this line needs to be split.

Sami> +event_object *
Sami> +create_event_object (PyTypeObject *py_type)
Sami> +{
Sami> +  event_object *event_obj;
Sami> +
Sami> +  event_obj = PyObject_New (event_object, py_type);
Sami> +  if (!event_obj)
Sami> +    goto fail;
Sami> +
Sami> +  event_obj->dict = PyDict_New ();
Sami> +  if (!event_obj->dict)
Sami> +    goto fail;
Sami> +
Sami> +  return event_obj;
Sami> +
Sami> + fail:
Sami> +  Py_XDECREF (event_obj);
Sami> +  Py_XDECREF (event_obj->dict);

Won't decrefing event_obj automatically free the dict when needed?
I didn't look closely but maybe other create_* functions have this issue.

Sami> +/* Implementation of EventRegistry.disconnect () -> NULL.
Sami> +   Remove FUNCTION to the list of listeners.  */

s/to/from/

Sami> +  PySequence_DelItem (callback_list, PySequence_Index (callback_list, func));

I think you have to separate the calls here and then check
the PySequence_Index result.

Sami> +  eventregistry_obj->callbacks = (PyListObject *) PyList_New (0);

This needs a failure check.

Sami> +static int
Sami> +add_new_registry (eventregistry_object **registryp, char *name)
Sami> +{
Sami> +  *registryp = create_eventregistry_object ();
Sami> +  if(*registryp == NULL)

Newline between these lines.
Space before open paren.

Why not just return the new registry, or NULL on error?
That would be simpler.

Sami> +void
Sami> +gdbpy_initialize_py_events ()
Sami> +{
Sami> +

No blank line here.

Sami> +  Py_INCREF (gdb_py_events.module);
Sami> +  if(PyModule_AddObject (gdb_module,

Space before open paren.

Sami> +  PyLongObject *exit_code;

I think you could remove a lot of casts if this were just a PyObject*.

Sami> +emit_exited_event (LONGEST *exit_code)
Sami> +{
Sami> +  event_object *event;
Sami> +  if (evregpy_get_nlisteners (gdb_py_events.exited) == 0)

Blank line after declarations.

Sami> +  if (event)
Sami> +    return evpy_emit_event ((event_object *)
Sami> +                            event,

I think this cast isn't needed.

Sami> +static void
Sami> +python_inferior_exit (struct inferior *inf)
Sami> +{
Sami> +  struct cleanup *cleanup;
Sami> +  LONGEST exitcode_val;
Sami> +  LONGEST *exit_code;
Sami> +
Sami> +  cleanup = ensure_python_env (get_current_arch (), current_language);
Sami> +
Sami> +  if (get_internalvar_integer (lookup_internalvar ("_exitcode"), &exitcode_val))
Sami> +    exit_code = &exitcode_val;
Sami> +
Sami> +  if (exit_code
Sami> +      && emit_exited_event (exit_code) < 0)

You have to initialize exit_code to NULL for this to work properly.

However, I think this is pretty ugly.
It seems like there should be a better way to get this than looking up
a convenience variable.

I think we need an event representing a thread exit.
It is ok by me if this comes in a separate patch.
(FWIW I have a few new events on my branch; I'll update those once this
patch goes in.)

Sami> +  signal_event_obj->stop_signal =
Sami> +      (PyStringObject *) PyString_FromString (stop_signal);

It is more usual to just use PyObject* everywhere, and not cast to the
more specific types.

This change should let you eliminate other casts in the patch.

Sami> +stop_event_object *
Sami> +create_stop_event_object (PyTypeObject *py_type, thread_object *thread)
Sami> +{
Sami> +  stop_event_object *stop_event_obj =
Sami> +      (stop_event_object *) create_event_object (py_type);
Sami> +
Sami> +  if (!stop_event_obj)
Sami> +    goto fail;
Sami> +
Sami> +  stop_event_obj->inferior_thread = (PyObject *) thread;
Sami> +
Sami> +  if (evpy_add_attribute ((event_object *) stop_event_obj,
Sami> +                          "inferior_thread",
Sami> +                          stop_event_obj->inferior_thread) < 0)
Sami> +    goto fail;
Sami> +
Sami> +
Sami> +  return stop_event_obj;

I think it would be better to just have one cast at the end, instead of
lots of casts in the body.

Sami> +  if (non_stop)
Sami> +    stopped_thread = find_thread_object (inferior_ptid);
Sami> +  else
Sami> +    stopped_thread = (thread_object *) Py_None;

This cast is not ok.
Also this code must incref even in this case, since it is decrefed when
the event is destroyed.

Sami> +  if (bs && bs->breakpoint_at
Sami> +      && bs->breakpoint_at->type == bp_breakpoint)
Sami> +    {
Sami> +      if (evregpy_get_nlisteners (gdb_py_events.breakpoint) == 0)
Sami> +	return 0;

I think the short-circuiting logic should be hoisted to the top of the
function.  This is more efficient and also lets you avoid having to
deal with reference counting problems involving objects made earlier.

Sami> +  /* Check if the signal is "Signal 0" or "Trace/breakpoint trap".  */
Sami> +  if ((strcmp (stop_signal, "0") != 0)
Sami> +      && (strcmp (stop_signal, "SIGTRAP") != 0))

I didn't look this up, but this seems questionable.
Is this really how this is done?

Sami> +typedef struct
Sami> +{
Sami> +  PyObject *inferior_thread;
Sami> +  event_object event;
Sami> +} stop_event_object;

For the inheritance scheme to work, the 'event' field has to come first.
I didn't audit the other event object types, but please make sure they
are all correct.

Sami> +/* Returns a a copy of the give LIST.
Sami> +   Creates a new reference which must be handled by the caller.  */
Sami> +
Sami> +PyObject *
Sami> +copy_py_list (PyObject *list)

I don't think we need this function.
Just use PySequence_List.

Tom