Add a python event registry for selected frame changes

Add a python event registry for selected frame changes

[Maxime Coste]

Hello,

Here is a patch serie which permits python scripts to watch for selected
frame changes, ignoring changes which are triggered by implementation detail
(like executing a call command).

I know the real solution to select_frame would be for most function to take
a frame rather than relying on the current selected frame, however I am not
familiar enough with gdb internals to fix that.

Regards,

Maxime Coste.

[PATCH 1/3] Add a select_frame_reason enum parameter to select_frame

[Maxime Coste]

select_frame calls specify if the frame is selected due to
a breakpoint/signal (REASON_STOP), a user command (REASON_USER)
or just as an implementation detail (REASON_IMPL_DETAIL) which
should restore the previous frame once finished.
---
 gdb/ada-lang.c        |  4 ++--
 gdb/breakpoint.c      | 10 +++++-----
 gdb/frame.c           |  6 +++---
 gdb/frame.h           | 16 +++++++++++++++-
 gdb/infrun.c          |  8 ++++----
 gdb/mi/mi-main.c      |  2 +-
 gdb/python/py-frame.c |  2 +-
 gdb/stack.c           |  9 +++++----
 gdb/thread.c          |  8 ++++----
 gdb/valops.c          |  2 +-
 gdb/varobj.c          |  6 +++---
 11 files changed, 44 insertions(+), 29 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index fdfc0b4..d45731b 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -11167,7 +11167,7 @@ ada_find_printable_frame (struct frame_info *fi)
     {
       if (!is_known_support_routine (fi))
         {
-          select_frame (fi);
+          select_frame (fi, REASON_STOP);
           break;
         }
     }
@@ -11222,7 +11222,7 @@ ada_unhandled_exception_name_addr_from_raise (void)
   if (fi == NULL)
     return 0;
 
-  select_frame (fi);
+  select_frame (fi, REASON_STOP);
   return parse_and_eval_address ("id.full_name");
 }
 
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 35ada7a..843a4d1 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1761,7 +1761,7 @@ update_watchpoint (struct watchpoint *b, int reparse)
       fi = frame_find_by_id (b->watchpoint_frame);
       within_current_scope = (fi != NULL);
       if (within_current_scope)
-	select_frame (fi);
+	select_frame (fi, REASON_IMPL_DETAIL);
     }
 
   /* We don't free locations.  They are stored in the bp_location array
@@ -1996,7 +1996,7 @@ in which its expression is valid.\n"),
 
   /* Restore the selected frame.  */
   if (frame_saved)
-    select_frame (frame_find_by_id (saved_frame_id));
+    select_frame (frame_find_by_id (saved_frame_id), REASON_IMPL_DETAIL);
 }
 
 
@@ -4765,7 +4765,7 @@ watchpoint_check (void *p)
 	/* If we end up stopping, the current frame will get selected
 	   in normal_stop.  So this call to select_frame won't affect
 	   the user.  */
-	select_frame (fr);
+	select_frame (fr, REASON_IMPL_DETAIL);
     }
 
   if (within_current_scope)
@@ -5091,7 +5091,7 @@ bpstat_check_breakpoint_conditions (bpstat bs, ptid_t ptid)
 	     of the inlined function; the current frame will be the
 	     call site.  */
 	  if (w == NULL || w->cond_exp_valid_block == NULL)
-	    select_frame (get_current_frame ());
+	    select_frame (get_current_frame (), REASON_IMPL_DETAIL);
 	  else
 	    {
 	      struct frame_info *frame;
@@ -5112,7 +5112,7 @@ bpstat_check_breakpoint_conditions (bpstat bs, ptid_t ptid)
 		 intuitive.  */
 	      frame = block_innermost_frame (w->cond_exp_valid_block);
 	      if (frame != NULL)
-		select_frame (frame);
+		select_frame (frame, REASON_IMPL_DETAIL);
 	      else
 		within_current_scope = 0;
 	    }
diff --git a/gdb/frame.c b/gdb/frame.c
index 8d4e2c8..0a0acda 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1382,7 +1382,7 @@ get_selected_frame (const char *message)
       /* Hey!  Don't trust this.  It should really be re-finding the
 	 last selected frame of the currently selected thread.  This,
 	 though, is better than nothing.  */
-      select_frame (get_current_frame ());
+      select_frame (get_current_frame (), REASON_IMPL_DETAIL);
     }
   /* There is always a frame.  */
   gdb_assert (selected_frame != NULL);
@@ -1412,7 +1412,7 @@ deprecated_safe_get_selected_frame (void)
 /* Select frame FI (or NULL - to invalidate the current frame).  */
 
 void
-select_frame (struct frame_info *fi)
+select_frame (struct frame_info *fi, enum select_frame_reason reason)
 {
   selected_frame = fi;
   /* NOTE: cagney/2002-05-04: FI can be NULL.  This occurs when the
@@ -1548,7 +1548,7 @@ reinit_frame_cache (void)
     annotate_frames_invalid ();
 
   current_frame = NULL;		/* Invalidate cache */
-  select_frame (NULL);
+  select_frame (NULL, REASON_IMPL_DETAIL);
   frame_stash_invalidate ();
   if (frame_debug)
     fprintf_unfiltered (gdb_stdlog, "{ reinit_frame_cache () }\n");
diff --git a/gdb/frame.h b/gdb/frame.h
index 31b9cb7..34115b4 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -219,6 +219,20 @@ enum frame_type
   SENTINEL_FRAME
 };
 
+/* Reason why a frame is being selected, so that select_frame is aware
+   of why it is being called */
+
+enum select_frame_reason
+{
+  /* Selected for as an implementation detail, user selected
+     frame should be restored */
+  REASON_IMPL_DETAIL,
+  /* Selecting this frame was a user request. */
+  REASON_USER,
+  /* A the inferior was stopped due to breakpoint/signal... */
+  REASON_STOP,
+};
+
 /* For every stopped thread, GDB tracks two frames: current and
    selected.  Current frame is the inner most frame of the selected
    thread.  Selected frame is the one being examined by the GDB
@@ -268,7 +282,7 @@ extern struct frame_info *get_selected_frame_if_set (void);
 
 /* Select a specific frame.  NULL, apparently implies re-select the
    inner most frame.  */
-extern void select_frame (struct frame_info *);
+extern void select_frame (struct frame_info *, enum select_frame_reason);
 
 /* Given a FRAME, return the next (more inner, younger) or previous
    (more outer, older) frame.  */
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 5d6a9af..9d0e9e7 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -6029,7 +6029,7 @@ normal_stop (void)
 
   if (!stop_stack_dummy)
     {
-      select_frame (get_current_frame ());
+      select_frame (get_current_frame (), REASON_STOP);
 
       /* Print current location without a level number, if
          we have changed functions or hit a breakpoint.
@@ -6124,7 +6124,7 @@ normal_stop (void)
 	 stopped (e.g. the dummy call previously hit a breakpoint).
 	 We can't know which case we have so just always re-establish
 	 a selected frame here.  */
-      select_frame (get_current_frame ());
+      select_frame (get_current_frame (), REASON_IMPL_DETAIL);
     }
 
 done:
@@ -6925,7 +6925,7 @@ restore_selected_frame (void *args)
       return 0;
     }
 
-  select_frame (frame);
+  select_frame (frame, REASON_IMPL_DETAIL);
 
   return (1);
 }
@@ -6967,7 +6967,7 @@ restore_infcall_control_state (struct infcall_control_state *inf_status)
 	   RETURN_MASK_ERROR) == 0)
 	/* Error in restoring the selected frame.  Select the innermost
 	   frame.  */
-	select_frame (get_current_frame ());
+	select_frame (get_current_frame (), REASON_IMPL_DETAIL);
     }
 
   xfree (inf_status);
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 94fda8f..0f0f3f5 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -2116,7 +2116,7 @@ mi_cmd_execute (struct mi_parse *parse)
       fid = find_relative_frame (get_current_frame (), &frame);
       if (frame == 0)
 	/* find_relative_frame was successful */
-	select_frame (fid);
+	select_frame (fid, REASON_USER);
       else
 	error (_("Invalid frame id: %d"), frame);
     }
diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c
index e2eb9c5..d5926b0 100644
--- a/gdb/python/py-frame.c
+++ b/gdb/python/py-frame.c
@@ -511,7 +511,7 @@ frapy_select (PyObject *self, PyObject *args)
     {
       FRAPY_REQUIRE_VALID (self, fi);
 
-      select_frame (fi);
+      select_frame (fi, REASON_USER);
     }
   GDB_PY_HANDLE_EXCEPTION (except);
 
diff --git a/gdb/stack.c b/gdb/stack.c
index c072a2e..e4aaab6 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -2116,7 +2116,7 @@ args_plus_locals_info (char *ignore, int from_tty)
 static void
 select_and_print_frame (struct frame_info *frame)
 {
-  select_frame (frame);
+  select_frame (frame, REASON_USER);
   if (frame)
     print_stack_frame (frame, 1, SRC_AND_LOC);
 }
@@ -2185,7 +2185,8 @@ find_relative_frame (struct frame_info *frame, int *level_offset_ptr)
 void
 select_frame_command (char *level_exp, int from_tty)
 {
-  select_frame (parse_frame_specification_1 (level_exp, "No stack.", NULL));
+  select_frame (parse_frame_specification_1 (level_exp, "No stack.", NULL),
+                REASON_USER);
 }
 
 /* The "frame" command.  With no argument, print the selected frame
@@ -2222,7 +2223,7 @@ up_silently_base (char *count_exp)
   frame = find_relative_frame (get_selected_frame ("No stack."), &count);
   if (count != 0 && count_exp == NULL)
     error (_("Initial frame selected; you cannot go up."));
-  select_frame (frame);
+  select_frame (frame, REASON_USER);
 }
 
 static void
@@ -2261,7 +2262,7 @@ down_silently_base (char *count_exp)
       error (_("Bottom (innermost) frame selected; you cannot go down."));
     }
 
-  select_frame (frame);
+  select_frame (frame, REASON_USER);
 }
 
 static void
diff --git a/gdb/thread.c b/gdb/thread.c
index 2a1d723..0a11ce9 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1016,7 +1016,7 @@ restore_selected_frame (struct frame_id a_frame_id, int frame_level)
   /* This means there was no selected frame.  */
   if (frame_level == -1)
     {
-      select_frame (NULL);
+      select_frame (NULL, REASON_IMPL_DETAIL);
       return;
     }
 
@@ -1037,7 +1037,7 @@ restore_selected_frame (struct frame_id a_frame_id, int frame_level)
       && frame_id_eq (get_frame_id (frame), a_frame_id))
     {
       /* Cool, all is fine.  */
-      select_frame (frame);
+      select_frame (frame, REASON_IMPL_DETAIL);
       return;
     }
 
@@ -1045,13 +1045,13 @@ restore_selected_frame (struct frame_id a_frame_id, int frame_level)
   if (frame != NULL)
     {
       /* Cool, refound it.  */
-      select_frame (frame);
+      select_frame (frame, REASON_IMPL_DETAIL);
       return;
     }
 
   /* Nothing else to do, the frame layout really changed.  Select the
      innermost stack frame.  */
-  select_frame (get_current_frame ());
+  select_frame (get_current_frame (), REASON_IMPL_DETAIL);
 
   /* Warn the user.  */
   if (frame_level > 0 && !ui_out_is_mi_like_p (current_uiout))
diff --git a/gdb/valops.c b/gdb/valops.c
index 93c09d8..d801809 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -1419,7 +1419,7 @@ value_assign (struct value *toval, struct value *fromval)
 	struct frame_info *fi = frame_find_by_id (old_frame);
 
 	if (fi != NULL)
-	  select_frame (fi);
+	  select_frame (fi, REASON_IMPL_DETAIL);
       }
 
       break;
diff --git a/gdb/varobj.c b/gdb/varobj.c
index 467e59a..c158d69 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -707,7 +707,7 @@ varobj_create (char *objname,
 	  var->root->frame = get_frame_id (fi);
 	  var->root->thread_id = pid_to_thread_id (inferior_ptid);
 	  old_id = get_frame_id (get_selected_frame (NULL));
-	  select_frame (fi);	 
+	  select_frame (fi, REASON_IMPL_DETAIL);
 	}
 
       /* We definitely need to catch errors here.
@@ -746,7 +746,7 @@ varobj_create (char *objname,
 
       /* Reset the selected frame.  */
       if (frame_id_p (old_id))
-	select_frame (frame_find_by_id (old_id));
+	select_frame (frame_find_by_id (old_id), REASON_IMPL_DETAIL);
     }
 
   /* If the variable object name is null, that means this
@@ -3405,7 +3405,7 @@ check_scope (struct varobj *var)
 	  pc >= BLOCK_END (var->root->valid_block))
 	scope = 0;
       else
-	select_frame (fi);
+	select_frame (fi, REASON_IMPL_DETAIL);
     }
   return scope;
 }
-- 
1.8.2.1

[PATCH 2/3] Add a frame_changed observer

[Maxime Coste]

---
 gdb/doc/observer.texi | 4 ++++
 gdb/frame.c           | 6 ++++++
 2 files changed, 10 insertions(+)

diff --git a/gdb/doc/observer.texi b/gdb/doc/observer.texi
index adb7085..9653d6b 100644
--- a/gdb/doc/observer.texi
+++ b/gdb/doc/observer.texi
@@ -105,6 +105,10 @@ been modified since being loaded by the debugger (by being recompiled,
 for instance).
 @end deftypefun
 
+@deftypefun void frame_changed (struct frame_info *@var{frame})
+the selected frame has changed;
+@end deftypefun
+
 @deftypefun void inferior_created (struct target_ops *@var{objfile}, int @var{from_tty})
 @value{GDBN} has just connected to an inferior.  For @samp{run},
 @value{GDBN} calls this observer while the inferior is still stopped
diff --git a/gdb/frame.c b/gdb/frame.c
index 0a0acda..0e21e33 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1414,6 +1414,9 @@ deprecated_safe_get_selected_frame (void)
 void
 select_frame (struct frame_info *fi, enum select_frame_reason reason)
 {
+  int changed = (reason == REASON_USER || reason == REASON_STOP)
+                && (fi != selected_frame);
+
   selected_frame = fi;
   /* NOTE: cagney/2002-05-04: FI can be NULL.  This occurs when the
      frame is being invalidated.  */
@@ -1453,6 +1456,9 @@ select_frame (struct frame_info *fi, enum select_frame_reason reason)
 	    set_language (s->language);
 	}
     }
+
+    if (changed)
+      observer_notify_frame_changed (fi);
 }
 
 /* Create an arbitrary (i.e. address specified by user) or innermost frame.
-- 
1.8.2.1

[PATCH 3/3] Add a gdb.events.frame_change event registry

[Maxime Coste]

---
 gdb/Makefile.in                  |  6 ++++
 gdb/python/py-events.h           |  1 +
 gdb/python/py-evts.c             |  3 ++
 gdb/python/py-framechangeevent.c | 76 ++++++++++++++++++++++++++++++++++++++++
 gdb/python/py-framechangeevent.h | 29 +++++++++++++++
 gdb/python/python-internal.h     |  1 +
 gdb/python/python.c              | 18 ++++++++++
 7 files changed, 134 insertions(+)
 create mode 100644 gdb/python/py-framechangeevent.c
 create mode 100644 gdb/python/py-framechangeevent.h

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index c891c83..748d0ca 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -288,6 +288,7 @@ SUBDIR_PYTHON_OBS = \
 	py-exitedevent.o \
 	py-finishbreakpoint.o \
 	py-frame.o \
+	py-framechangeevent.o \
 	py-function.o \
 	py-gdb-readline.o \
 	py-inferior.o \
@@ -322,6 +323,7 @@ SUBDIR_PYTHON_SRCS = \
 	python/py-exitedevent.c \
 	python/py-finishbreakpoint.c \
 	python/py-frame.c \
+	python/py-framechangeevent.c \
 	python/py-function.c \
 	python/py-gdb-readline.c \
 	python/py-inferior.c \
@@ -2141,6 +2143,10 @@ py-frame.o: $(srcdir)/python/py-frame.c
 	$(COMPILE) $(PYTHON_CFLAGS) $(srcdir)/python/py-frame.c
 	$(POSTCOMPILE)
 
+py-framechangeevent.o: $(srcdir)/python/py-framechangeevent.c
+	$(COMPILE) $(PYTHON_CFLAGS) $(srcdir)/python/py-framechangeevent.c
+	$(POSTCOMPILE)
+
 py-function.o: $(srcdir)/python/py-function.c
 	$(COMPILE) $(PYTHON_CFLAGS) $(srcdir)/python/py-function.c
 	$(POSTCOMPILE)
diff --git a/gdb/python/py-events.h b/gdb/python/py-events.h
index 537bcc9..a61858c 100644
--- a/gdb/python/py-events.h
+++ b/gdb/python/py-events.h
@@ -45,6 +45,7 @@ typedef struct
   eventregistry_object *cont;
   eventregistry_object *exited;
   eventregistry_object *new_objfile;
+  eventregistry_object *frame_change;
 
   PyObject *module;
 
diff --git a/gdb/python/py-evts.c b/gdb/python/py-evts.c
index 4c079e2..f4d1fbf 100644
--- a/gdb/python/py-evts.c
+++ b/gdb/python/py-evts.c
@@ -81,6 +81,9 @@ gdbpy_initialize_py_events (void)
   if (add_new_registry (&gdb_py_events.new_objfile, "new_objfile") < 0)
     goto fail;
 
+  if (add_new_registry (&gdb_py_events.frame_change, "frame_change") < 0)
+    goto fail;
+
 #ifndef IS_PY3K
   Py_INCREF (gdb_py_events.module);
 #endif
diff --git a/gdb/python/py-framechangeevent.c b/gdb/python/py-framechangeevent.c
new file mode 100644
index 0000000..f18ef39
--- /dev/null
+++ b/gdb/python/py-framechangeevent.c
@@ -0,0 +1,76 @@
+/* Python interface to inferior frame events.
+
+   Copyright (C) 2009-2013 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "defs.h"
+#include "py-framechangeevent.h"
+
+static PyTypeObject frame_change_event_object_type;
+
+static PyObject *
+create_frame_change_event_object (struct frame_info *frame)
+{
+  PyObject *frame_change_event;
+  PyObject *py_frame;
+
+  frame_change_event = create_event_object (&frame_change_event_object_type);
+  if (!frame_change_event)
+    goto fail;
+
+  /* Note that frame_to_frame_object returns a borrowed reference,
+     so we don't need a decref here.  */
+  py_frame = frame ? frame_info_to_frame_object (frame) : Py_None;
+  if (!py_frame || evpy_add_attribute (frame_change_event,
+                                         "frame",
+                                         py_frame) < 0)
+    goto fail;
+
+  return frame_change_event;
+
+ fail:
+  Py_XDECREF (frame_change_event);
+  return NULL;
+}
+
+/* Callback observers when a frame event occurs.  This function will create a
+   new Python frame event object.  If only a specific thread is frameped the
+   thread object of the event will be set to that thread.  Otherwise, if all
+   threads are frameped thread object will be set to None.
+   return 0 if the event was created and emitted successfully otherwise
+   returns -1.  */
+
+int
+emit_frame_change_event (struct frame_info* frame)
+{
+  PyObject *frame_change_event_obj = NULL;
+
+  if (evregpy_no_listeners_p (gdb_py_events.frame_change))
+    return 0;
+
+  frame_change_event_obj = create_frame_change_event_object(frame);
+  if (frame_change_event_obj)
+    return evpy_emit_event (frame_change_event_obj, gdb_py_events.frame_change);
+  return -1;
+}
+
+GDBPY_NEW_EVENT_TYPE (frame_change,
+                      "gdb.FrameChangeEvent",
+                      "FrameChangeEvent",
+                      "GDB frame event object",
+                      event_object_type,
+                      static);
diff --git a/gdb/python/py-framechangeevent.h b/gdb/python/py-framechangeevent.h
new file mode 100644
index 0000000..e7564b0
--- /dev/null
+++ b/gdb/python/py-framechangeevent.h
@@ -0,0 +1,29 @@
+/* Python interface to inferior events.
+
+   Copyright (C) 2009-2013 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef GDB_PY_FRAMECHANGEEVENT_H
+#define GDB_PY_FRAMECHANGEEVENT_H
+
+#include "py-event.h"
+
+extern void frame_change_evpy_dealloc (PyObject *self);
+
+extern int emit_frame_change_event (struct frame_info* frame);
+
+#endif /* GDB_PY_FRAMECHANGEEVENT_H */
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index ea97226..676096d 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -301,6 +301,7 @@ void gdbpy_initialize_continue_event (void);
 void gdbpy_initialize_exited_event (void);
 void gdbpy_initialize_thread_event (void);
 void gdbpy_initialize_new_objfile_event (void);
+void gdbpy_initialize_frame_change_event (void);
 void gdbpy_initialize_arch (void);
 
 struct cleanup *make_cleanup_py_decref (PyObject *py);
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 67d06e5..ded99c7 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -72,6 +72,7 @@ static const char *gdbpy_should_print_stack = python_excp_message;
 #include "observer.h"
 #include "interps.h"
 #include "event-top.h"
+#include "py-framechangeevent.h"
 
 static PyMethodDef GdbMethods[];
 
@@ -881,6 +882,21 @@ gdbpy_initialize_events (void)
     }
 }
 
+
+static void
+python_frame_changed (struct frame_info *frame)
+{
+  struct cleanup *cleanup;
+  struct gdbarch *arch = frame ? get_frame_arch(frame) : target_gdbarch();
+
+  cleanup = ensure_python_env (arch, current_language);
+
+  if (emit_frame_change_event (frame) < 0)
+    gdbpy_print_stack ();
+
+  do_cleanups (cleanup);
+}
+
 \f
 
 static void
@@ -1630,9 +1646,11 @@ message == an error message without a stack will be printed."),
   gdbpy_initialize_exited_event ();
   gdbpy_initialize_thread_event ();
   gdbpy_initialize_new_objfile_event () ;
+  gdbpy_initialize_frame_change_event () ;
   gdbpy_initialize_arch ();
 
   observer_attach_before_prompt (before_prompt_hook);
+  observer_attach_frame_changed (python_frame_changed);
 
   gdbpy_to_string_cst = PyString_FromString ("to_string");
   gdbpy_children_cst = PyString_FromString ("children");
-- 
1.8.2.1

Re: [PATCH 2/3] Add a frame_changed observer

[Tom Tromey]

>>>>> "Maxime" == Maxime Coste <frrrwww@gmail.com> writes:

Maxime> +@deftypefun void frame_changed (struct frame_info *@var{frame})
Maxime> +the selected frame has changed;
Maxime> +@end deftypefun

I think this needs more text and should probably be a full sentence.
Like

    The selected frame has changed in a user-visible way.  @var{frame}
    is the new frame.

Tom

Re: Add a python event registry for selected frame changes

[Tom Tromey]

>>>>> "Maxime" == Maxime Coste <frrrwww@gmail.com> writes:

Maxime> Here is a patch serie which permits python scripts to watch for
Maxime> selected frame changes, ignoring changes which are triggered by
Maxime> implementation detail (like executing a call command).

Thanks.

I think this series needs 3 things --

1. ChangeLog entries.  See the GNU Coding Standards for details.
2. Test cases.
3. Copyright assignment paperwork on file.

I can get you started with #3 if you contact me off-list.

Tom

Re: [PATCH 1/3] Add a select_frame_reason enum parameter to select_frame

[Tom Tromey]

>>>>> "Maxime" == Maxime Coste <frrrwww@gmail.com> writes:

Maxime> select_frame calls specify if the frame is selected due to
Maxime> a breakpoint/signal (REASON_STOP), a user command (REASON_USER)
Maxime> or just as an implementation detail (REASON_IMPL_DETAIL) which
Maxime> should restore the previous frame once finished.

I don't mind this approach but I would like to hear from other
maintainers.

Tom

Re: [PATCH 3/3] Add a gdb.events.frame_change event registry

[Tom Tromey]

>>>>> "Maxime" == Maxime Coste <frrrwww@gmail.com> writes:

Maxime> +  /* Note that frame_to_frame_object returns a borrowed reference,
Maxime> +     so we don't need a decref here.  */
Maxime> +  py_frame = frame ? frame_info_to_frame_object (frame) : Py_None;

I don't think that comment is true.
Also we recently agreed in gdb to do explicit checks against NULL, so
"frame != NULL" here.

Maxime> +  if (!py_frame || evpy_add_attribute (frame_change_event,

Likewise for py_frame.

Maxime> +emit_frame_change_event (struct frame_info* frame)

Space before the "*", not after.

Maxime> +
Maxime> +static void
Maxime> +python_frame_changed (struct frame_info *frame)

Every new function needs an introductory comment.

Maxime> +  struct gdbarch *arch = frame ? get_frame_arch(frame) : target_gdbarch();

Space before open paren.

This patch needs a documentation change for the new Python API.

Tom

Re: [PATCH 3/3] Add a gdb.events.frame_change event registry

[Tom Tromey]

Tom> This patch needs a documentation change for the new Python API.

... oh, and a NEWS entry as well.

Tom

Re: [PATCH 1/3] Add a select_frame_reason enum parameter to select_frame

[Joel Brobecker]

> Maxime> select_frame calls specify if the frame is selected due to
> Maxime> a breakpoint/signal (REASON_STOP), a user command (REASON_USER)
> Maxime> or just as an implementation detail (REASON_IMPL_DETAIL) which
> Maxime> should restore the previous frame once finished.
> 
> I don't mind this approach but I would like to hear from other
> maintainers.

I wasn't super fond when I saw this yesterday, TBH, particuarly
after seeing how it's used in the patch. But I haven't really had
time to think this over to see if I can propose something else.
So do not block the patch on my account.

-- 
Joel

Re: Add a python event registry for selected frame changes

[Maxime Coste]

Hello,

Here are the corrected patches.

Maxime.

Re: [PATCH 1/3] Add a select_frame_reason enum parameter to select_frame

[Pedro Alves]

On 04/29/2013 09:27 PM, Tom Tromey wrote:
>>>>>> "Maxime" == Maxime Coste <frrrwww@gmail.com> writes:
> 
> Maxime> select_frame calls specify if the frame is selected due to
> Maxime> a breakpoint/signal (REASON_STOP), a user command (REASON_USER)
> Maxime> or just as an implementation detail (REASON_IMPL_DETAIL) which
> Maxime> should restore the previous frame once finished.
> 
> I don't mind this approach but I would like to hear from other
> maintainers.

I'm not sure I like the approach.

I couldn't find any use for the REASON_STOP/REASON_USER
distinction in the series.  If there's a rationale for it,
it'd be good to hear it, otherwise it just looks like over engineering.

The (fi != selected_frame) check in the REASON_STOP
looks suspicious.  Was the intent to prevent a notification
if the program stopped in the same function it was stopped
before it was resumed?

IMO, it'd be better to leave select_frame unaware of why it's being
called, and just add a wrapper for user visible frame selection.
It feels like different conceptual levels to me.  IOW, something like:

void
select_user_frame (struct frame_info *fi)
{
  int changed = (fi != selected_frame);

  select_frame (fi);

  if (changed)
   observer_notify_frame_changed (fi);
}

But,
note that in non-stop mode, e.g., thread #2 may be running while
you inspect thread #1.  If thread #2 stops for any reason, say, a breakpoint,
GDB switches temporarily to thread #2 to handle the event, and then that ends
up calling normal_stop and select_frame.  The patch makes that a
user visible frame change.  But, what happens is that GDB immediately
afterwards changes back to thread #1 (and its frame), using a cleanup, giving
the illusion that thread #1 was selected the whole time.  This frame restore
is an IMPL_DETAIL select_frame in this approach, so I think the frame
change observer (python, in this case) ends up confused.

Another problematic case I can think of off hand, is around
errors.  Say, the target is running, and while handling an
event, we error out, and normal_stop isn't reached.

I'm wondering whether we really need to call the frame change
observers for stops.  We have stop notifications/observers in
both Python and MI, so it seems to be a script/frontend can
already get the info it needs from those.

If we limit frame change notifications to user frame changes
(CLI up/down/frame and MI equivalents), then these issues
with stops would be non-issues by design...

-- 
Pedro Alves

Re: [PATCH 1/3] Add a select_frame_reason enum parameter to select_frame

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> If we limit frame change notifications to user frame changes
Pedro> (CLI up/down/frame and MI equivalents), then these issues
Pedro> with stops would be non-issues by design...

That sounds like a reasonable plan to me.

Tom

Re: [PATCH 1/3] Add a select_frame_reason enum parameter to select_frame

[Maxime Coste]

Hello

On Mon, 06 May 2013 17:54:01 +0100, Pedro Alves <palves@redhat.com> wrote:
> IMO, it'd be better to leave select_frame unaware of why it's being
> called, and just add a wrapper for user visible frame selection.
> It feels like different conceptual levels to me.  IOW, something like:
> 
> void
> select_user_frame (struct frame_info *fi)
> [...]

That seems better to me too, I went the other way because at first I tried
to add checks for frame restoration (there were IMPL_DETAIL_{PUSH,POP}
with a counter checking that only IMPL_DETAIL_* were nested...

I'll prepare a patch using this approach.

> If we limit frame change notifications to user frame changes
> (CLI up/down/frame and MI equivalents), then these issues
> with stops would be non-issues by design...

Yes, this seems more reasonable as well.

Thanks for the review.

Maxime.