[PATCH/RFC] Introduce =target-changed MI notification

[Simon Marchi <simon dot marchi at ericsson dot com>]

So this patch/RFC is reviving this 4 years old thread.

  http://thread.gmane.org/gmane.comp.gdb.patches/80427

I'll do doc/NEWS changes once an agreement is reached.

I'll first try to give an overview of the problem and the past
discussion, so you don't have to dig in the archives.  The train of
thought goes like this:

The problem: in an MI front-end, the user can use the embedded GDB
console to type:

  (gdb) set $rbx = 0xabcd

Because of the lack of "register changed" notification, the registers
view in the frontend won't update.  The same happens if the user updates
a variable and that variable is stored in a register.  Both the
registers and variables view won't update.  When changing memory though,
we have the =memory-changed event, we see that something is missing.

Note that this only concerns cases where something (e.g. the user or a
script) changes the target state while it's stopped, behind the
front-end's back.  Cases where the target runs are not considered here,
since the front-end is expected to refresh all of its views.

The initial solution proposed in the thread mentioned above was to add a
=register-changed notification (and that's what I would have
instinctively suggested as well).  However, in order for the frontend to
always be correctly synced, we would need to handle some edge cases
which can be pretty hard to get right efficiently.  For example:

 - The user manually sets the value of a register in a frame other than
   the current frame.  It's possible that this register value isn't in a
   register at the moment, but is rather saved on the stack, in which
   case GDB will update the value in the stack.  For the front-end to
   stay synced, we would need to send a =register-changed event as well
   as a =memory-changed event, since both the registers and memory views
   need to be updated.  I guess that this one is not too hard, because
   if GDB changed the value, it knows whether it changed a register or a
   memory location.

 - The user manually changes a value in memory that happens to be a
   register from a previous frame that is saved on the stack.  This
   seems much harder to do.  From my very shy DWARF knowledge, I
   understand that it's easy to find out where a register belonging to a
   previous frame is stored (.debug_frames), but not the other way
   around.  In that case, the registers view would go out of sync.

Also, when you factor in memory-mapped registers, and other edge cases
we haven't imagined, this all sounds like a very complicated problem.

The other approach suggested in the thread was not to even try being
smart, and just emit a generic =target-changed notification when
anything was changed by the user behind the back of the MI frontend.
After all, changing anything (register or memory) can impact anything
else (registers, variables, frame stack, memory, etc.).  So all of those
views should be considered stale and be updated.

An argument for such an event was that front-ends need to do a full
refresh any time they single step or stop at a breakpoint.  These
operations happen in general much more frequently than the user
changing data by hand.  So if users don't complain about single step
being slow, they shouldn't complain about changing a register being
slow.

An in-between option that was considered was to add the
=register-changed event, but make it clear in the doc that even though
this is the "origin" of the change, the front-end should refresh pretty
much everything.  I liked how Pedro illustrated it: "We may be better
off not giving ourselves and the frontends rope to hang ourselves
with.".  Indeed, what point is there to give some info to the front-end
but also tell it to not trust that info?

So that's why I think that a =target-changed event makes the most sense.
The =memory-changed event could co-exist with =target-changed for some
time, but if we go that route I guess it should be deprecated.

What do you think?

PS: This is partly motivated by the new console work from Pedro.  When
we will have a good GDB console in Eclipse, users will be more likely to
use it and therefore make register changes like this.

gdb/ChangeLog:

	* mi/mi-cmd-var.c (mi_cmd_var_assign): Suppress target-changed
	MI notification.
	* mi/mi-interp.c (mi_target_changed): New declaration and
	function.
	(mi_interpreter_init): Attach to target_changed observer.
	* mi/mi-main.h (mi_suppress_notification) <target_changed>: New
	field.

Change-Id: I69c90d5feab3dd2198aff37cd20709ad71a2a991
---
 gdb/mi/mi-cmd-var.c |  9 +++++++--
 gdb/mi/mi-interp.c  | 24 ++++++++++++++++++++++++
 gdb/mi/mi-main.h    |  2 ++
 3 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/gdb/mi/mi-cmd-var.c b/gdb/mi/mi-cmd-var.c
index 3bfe4f0..da28e64 100644
--- a/gdb/mi/mi-cmd-var.c
+++ b/gdb/mi/mi-cmd-var.c
@@ -609,6 +609,7 @@ mi_cmd_var_assign (char *command, char **argv, int argc)
   struct varobj *var;
   char *expression, *val;
   struct cleanup *cleanup;
+  int *p;
 
   if (argc != 2)
     error (_("-var-assign: Usage: NAME EXPRESSION."));
@@ -623,10 +624,14 @@ mi_cmd_var_assign (char *command, char **argv, int argc)
 
   /* MI command '-var-assign' may write memory, so suppress memory
      changed notification if it does.  */
-  cleanup
-    = make_cleanup_restore_integer (&mi_suppress_notification.memory);
+  p = &mi_suppress_notification.memory;
+  make_cleanup_restore_integer (p);
   mi_suppress_notification.memory = 1;
 
+  p = &mi_suppress_notification.target_changed;
+  cleanup = make_cleanup_restore_integer (p);
+  mi_suppress_notification.target_changed = 1;
+
   if (!varobj_set_value (var, expression))
     error (_("-var-assign: Could not assign "
 	     "expression to variable object"));
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index b37dc96..d97c168 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -84,6 +84,7 @@ static void mi_breakpoint_modified (struct breakpoint *b);
 static void mi_command_param_changed (const char *param, const char *value);
 static void mi_memory_changed (struct inferior *inf, CORE_ADDR memaddr,
 			       ssize_t len, const bfd_byte *myaddr);
+static void mi_target_changed (struct target_ops *target);
 static void mi_on_sync_execution_done (void);
 
 static int report_initial_inferior (struct inferior *inf, void *closure);
@@ -158,6 +159,7 @@ mi_interpreter_init (struct interp *interp, int top_level)
       observer_attach_breakpoint_modified (mi_breakpoint_modified);
       observer_attach_command_param_changed (mi_command_param_changed);
       observer_attach_memory_changed (mi_memory_changed);
+      observer_attach_target_changed (mi_target_changed);
       observer_attach_sync_execution_done (mi_on_sync_execution_done);
 
       /* The initial inferior is created before this function is
@@ -1140,6 +1142,28 @@ mi_memory_changed (struct inferior *inferior, CORE_ADDR memaddr,
   do_cleanups (old_chain);
 }
 
+/* Emit a notification that the target changed.  */
+
+static void
+mi_target_changed (struct target_ops *target)
+{
+  if (!mi_suppress_notification.target_changed)
+    {
+      struct mi_interp *mi = (struct mi_interp *) top_level_interpreter_data ();
+      struct ui_out *mi_uiout = interp_ui_out (top_level_interpreter ());
+      struct cleanup *old_chain;
+
+      old_chain = make_cleanup_restore_target_terminal ();
+      target_terminal_ours_for_output ();
+
+      fprintf_unfiltered (mi->event_channel, "target-changed");
+
+      gdb_flush (mi->event_channel);
+
+      do_cleanups (old_chain);
+    }
+}
+
 static int
 report_initial_inferior (struct inferior *inf, void *closure)
 {
diff --git a/gdb/mi/mi-main.h b/gdb/mi/mi-main.h
index b2face1..512a825 100644
--- a/gdb/mi/mi-main.h
+++ b/gdb/mi/mi-main.h
@@ -47,6 +47,8 @@ struct mi_suppress_notification
   int traceframe;
   /* Memory changed notification suppressed?  */
   int memory;
+  /* Register or memory on target changed. */
+  int target_changed;
 };
 extern struct mi_suppress_notification mi_suppress_notification;
 
-- 
2.8.1