Re: [patch] Support inferior events in python

[Tom Tromey <tromey@redhat.com>]

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

Sami> This patch contains changes requested by Eli and Tom, and includes
Sami> documentation for the new features.

Thank you very much

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

Sami> How about evregpy_no_listeners_p ? Fits better with the way the code
Sami> was written.

Perfect.

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

Sami> Adding the registry, and doing the needed error checking here, makes
Sami> the calling code simpler. The calling code is what will be extended in
Sami> future development.

I looked at this in the new patch, and I see what you mean.
I agree with your approach.

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

Sami> Hmm I looked through the code to find another way but could
Sami> not. handle_inferior_event which sets the convenience variable uses
Sami> execution_control_state which I don't have access to.

I don't want to hold up this patch for a detail like this, but I wonder
if we could put the exit code into the struct inferior as well as in the
convenience variable.

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

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

Sami> Done. I also, updated breakpoint_event_object. I was going with the
Sami> oposite mindset of keeping the type information until a cast is
Sami> required.

Yeah, ordinarily that would be better, but Python pretty much assumes
PyObject* everywhere.

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

Sami> Hmm if I change stop_event_object* to event_object it would eliminate
Sami> two casts but also add two. One when setting the inferior thread and
Sami> one for returning. Same goes for all the create_* functions or should
Sami> I change all of those to return more generic objects ?

It seems to me that the callers of the create_* functions then proceed
to downcast anyway:

Sami> +  event = (event_object *) create_continue_event_object ();
Sami> +  if (event)
Sami> +    return evpy_emit_event (event, gdb_py_events.cont);
Sami> +  return -1;

Given this, I think just returning the more generic type is ok.


Sami> I improved this by using enum target_signal and target_signal_to_name
Sami> to convert the signal to a string when notifying python
Sami> listeners. That looks OK IMO, but we can also create a module
Sami> gdb.signal, create a pyhon Signal type, add Signal types for all
Sami> signals to gdb.signal, and use a Signal object to notify python
Sami> listeners.

I think it is ok to use strings.

Sami> +* Python Support for Inferior events.
Sami> +  Python scripts can add observers to be notified of events
Sami> +  occurring the in process being debugged.
Sami> +  The following events are currently supported:
Sami> +  - gdb.events.breakpoint Breakpoint hit event.
Sami> +  - gdb.events.cont Continue event.
Sami> +  - gdb.events.signal Signal received event.
Sami> +  - gdb.events.exited Inferior exited event.

There is a "Python scripting" section in NEWS already.  This should be
in that section.

Sami> +* Inferior Events In Python::   Listening for events from the process being debugged.

Line wraps.  I think the node name should not have "Inferior" in it,
since we may well use this facility for events not originating from the
inferior.

Sami> +The Python API allows scripts to listen for events coming from the inferior process
Sami> +and its threads. In order to listen for events the script must register an observer
Sami> +by connecting it to the appropriate event registry. Event registries can be accessed
Sami> +through the @code{gdb.events} module.

All the lines in this paragraph wrap.  This occurs in a few places in
the documentation patch.  Also, we use two spaces after a period.

I think the introduction could be expanded on a bit.
How about something like:

    GDB provides a general event facility so that Python code can be
    notified of various state changes, particularly changes that occur in
    the inferior.

    An @defn{event} is just an object that describes some state change.  The
    type of the object and its attributes will vary depending on the details
    of the change.  All the existing events are described below.

    In order to be notified of an event, you must register an event handler
    with an event registry.  An @defn{event registry} is just an object in
    the @code{gdb.event} module which dispatches particular events.  A
    registry provides methods to register and unregister event handlers.

Sami> +Here is an example:
Sami> +
Sami> +@smallexample
Sami> +def exit_handler (event):
Sami> +    if (isinstance (event, gdb.ExitedEvent)):

We shouldn't use isinstance in an example.  I think that is sort of bad
style, particularly if the `exited' event registry can only ever
dispatch ExitedEvent objects.

Sami> +@table @code
Sami> +@item events.breakpoint
Sami> +@item events.cont
Sami> +@item events.exited
Sami> +@item events.signal
Sami> +@end table

This seems weird.  At least each of these should have a description
indicating what it is good for.

And, before this, there should be a section describing the event
registry class itself.  I didn't see any documentation for the connect
and disconnect methods.

Instead of separate signal and breakpoint registries, why not a general
"stopped" registry that emits different events depending on why the
inferior stopped?

Sami> +These registries emit the following events in respective order:

I think it would be better to use nested tables here.  That is, it
should be pretty easy to read the text and see what a given registry
does and what kind of events it can generate.

Sami> +@defivar SignalEvent stop_signal
Sami> +The signal received by the inferior
Sami> +@end defivar

This should describe the possible values for this attribute.
I think all the attributes should have this sort of documentation.

Sami> +@node gdb.events
Sami> +@subsubsection gdb.events
Sami> +@cindex gdb.events

I understand why you put this here, but I think it isn't the best place
for it.  True, gdb.events is a module -- but I think it would be clearer
for the reader if it were just documented where the discussion of events
takes place.

(This node was really created for modules actually written in Python.
But, that doesn't actually matter to users, either... perhaps this
existing text should be moved, but of course that isn't related to this
patch.)

Sami> +  callback_list = (PyObject *) registry->callbacks;

Right now `callbacks' is a PyListObject*.
If it were a PyObject* then I think all the casts associated with it
would go away.

Sami> +extern int emit_stop_event (struct bpstats *bs, enum target_signal stop_signal);

This line is too long.

Sami> +extern event_object * create_event_object (PyTypeObject *py_type);

Remove space after "*".

Sami> +  PyList_Append (callback_list, func);

Needs an error check.

Sami> +static PyObject *
Sami> +evregpy_disconnect (PyObject *self, PyObject *function)
[...]
Sami> +  if (!PyCallable_Check (func))
Sami> +    {
Sami> +      PyErr_SetString (PyExc_RuntimeError, "Function is not callable");
Sami> +      return NULL;

I don't think we need to bother with this check here.

Sami> +  if (index < 0)
Sami> +    {
Sami> +      PyErr_SetString (PyExc_RuntimeError, "Function not found");
Sami> +      return NULL;

And I think it is ok to just ignore this case.
It seems ok to me to have an attempted removal of an item simply do
nothing if the item isn't on the list.

Sami> +  if (get_internalvar_integer (lookup_internalvar ("_exitcode"), &exitcode_val))

This line is too long.

Sami> +  signal_name = (char *) target_signal_to_name (stop_signal);
Sami> +  signal_event_obj->stop_signal = PyString_FromString (signal_name);

I think you can make signal_name a const char * and avoid the cast here.
Or did we have some problem with this elsewhere?  I can't remember.

Sami> +PyObject *
Sami> +get_stopped_thread ()

`(void)'

Sami> +extern stop_event_object * create_stop_event_object (PyTypeObject *py_type,

Remove space after "*".

Tom