[RFC 0/2] add new async events =target-connected and =target-disconnected

[RFC 0/2] add new async events =target-connected and =target-disconnected

[Jan Vrany]

This mini series add new MI async events notifying MI client
abut target changes. This is a result of discussion happened 
earlier this year on gdb mailing list [1].

[1]: https://sourceware.org/ml/gdb/2018-04/msg00004.html

Jan Vrany (2):
  gdb: add new observables target_connected and target_disconnected
  gdb/mi: add new async events =target-connected and
    =target-disconnected

 gdb/ChangeLog       | 18 +++++++++--
 gdb/doc/ChangeLog   |  5 +++
 gdb/doc/gdb.texinfo |  7 +++++
 gdb/mi/mi-interp.c  | 77 +++++++++++++++++++++++++++++++++++++++++++--
 gdb/observable.c    |  2 ++
 gdb/observable.h    |  6 ++++
 gdb/target.c        |  7 +++++
 7 files changed, 118 insertions(+), 4 deletions(-)

-- 
2.19.1

[RFC 1/2] gdb: add new observables target_connected and target_disconnected

[Jan Vrany]

Observers of these are notified whenver a target is connected or
disconnected.

gdb/ChangeLog:

	* observable.h: Define new observables target_connected and
	target_disconnected.
	* observable.c: Likewise.
	* target.c (target_stack::push, target_stack::unpush): Notify
	observers of target_connected and target_disconnected
---
 gdb/ChangeLog    | 8 ++++++++
 gdb/observable.c | 2 ++
 gdb/observable.h | 6 ++++++
 gdb/target.c     | 7 +++++++
 4 files changed, 23 insertions(+)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index b3cc64659f..d13ce2097d 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2018-07-03  Jan Vrany  <jan.vrany@fit.cvut.cz>
+
+	* observable.h: Define new observables target_connected and
+	target_disconnected.
+	* observable.c: Likewise.
+	* target.c (target_stack::push, target_stack::unpush): Notify
+	observers of target_connected and target_disconnected
+
 2018-10-11  Gary Benson <gbenson@redhat.com>
 
 	* interps.h (interp::m_name): Make private and mutable.
diff --git a/gdb/observable.c b/gdb/observable.c
index 5539b9837b..4b98612433 100644
--- a/gdb/observable.c
+++ b/gdb/observable.c
@@ -74,6 +74,8 @@ DEFINE_OBSERVABLE (inferior_call_pre);
 DEFINE_OBSERVABLE (inferior_call_post);
 DEFINE_OBSERVABLE (register_changed);
 DEFINE_OBSERVABLE (user_selected_context_changed);
+DEFINE_OBSERVABLE (target_connected);
+DEFINE_OBSERVABLE (target_disconnected);
 
 } /* namespace observers */
 } /* namespace gdb */
diff --git a/gdb/observable.h b/gdb/observable.h
index 34447b90bb..f1829db99f 100644
--- a/gdb/observable.h
+++ b/gdb/observable.h
@@ -228,6 +228,12 @@ extern observable<struct frame_info *, int> register_changed;
    frame has changed.  */
 extern observable<user_selected_what> user_selected_context_changed;
 
+/* A new target has been connected.  */
+extern observable<struct target_ops *> target_connected;
+
+/* A target has been disonnected.  */
+extern observable<struct target_ops *> target_disconnected;
+
 } /* namespace observers */
 
 } /* namespace gdb */
diff --git a/gdb/target.c b/gdb/target.c
index 2d98954b54..5c19afead9 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -50,6 +50,7 @@
 #include "terminal.h"
 #include <algorithm>
 #include <unordered_map>
+#include "observable.h"
 
 static void generic_tls_error (void) ATTRIBUTE_NORETURN;
 
@@ -644,6 +645,8 @@ target_stack::push (target_ops *t)
       target_ops *prev = m_stack[t->to_stratum];
       m_stack[t->to_stratum] = NULL;
       target_close (prev);
+
+      gdb::observers::target_disconnected.notify (t);
     }
 
   /* Now add the new one.  */
@@ -651,6 +654,8 @@ target_stack::push (target_ops *t)
 
   if (m_top < t->to_stratum)
     m_top = t->to_stratum;
+
+  gdb::observers::target_connected.notify (t);
 }
 
 /* See target.h.  */
@@ -701,6 +706,8 @@ target_stack::unpush (target_ops *t)
      implementation don't end up in T anymore.  */
   target_close (t);
 
+  gdb::observers::target_disconnected.notify (t);
+
   return true;
 }
 
-- 
2.19.1

[RFC 2/2] gdb/mi: add new async events =target-connected and =target-disconnected

[Jan Vrany]

Whenever a target is connected or disconnected, emit new asynchronous
event =target-connected and =target-disconnected. Events report
both short name and full name of connected or disconnected target.
In addition, =target-connected report a set of target features.

This allows frontends to keep track of current target and its features
regardless whether target is changed explicitly by MI -target-select
command, CLI target command or implicitly by  native target auto-connect.

gdb/Changelog:

	* mi/mi-interp.c (mi_target_connected): New function.
	(mi_target_disconnected): New function.
	(_initialize_mi_interp): Register new observers.

gdb/doc/Changelog

	* gdb.texinfo (GDB/MI Async Records): Document new async
	records =target-connected and target-disconnected.
---
 gdb/ChangeLog       | 12 +++++--
 gdb/doc/ChangeLog   |  5 +++
 gdb/doc/gdb.texinfo |  7 +++++
 gdb/mi/mi-interp.c  | 77 +++++++++++++++++++++++++++++++++++++++++++--
 4 files changed, 96 insertions(+), 5 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index d13ce2097d..cd0ae9443e 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,4 +1,10 @@
-2018-07-03  Jan Vrany  <jan.vrany@fit.cvut.cz>
+2018-10-13  Jan Vrany  <jan.vrany@fit.cvut.cz>
+
+	* mi/mi-interp.c (mi_target_connected): New function.
+	(mi_target_disconnected): New function.
+	(_initialize_mi_interp): Register new observers.
+
+2018-10-13  Jan Vrany  <jan.vrany@fit.cvut.cz>
 
 	* observable.h: Define new observables target_connected and
 	target_disconnected.
@@ -8021,7 +8027,7 @@
 	of the new expedite_regs parameter to init_target_desc.
 
 2018-05-10  Omair Javaid  <omair.javaid@linaro.org>
-    
+
 	PR gdb/23127
 	* aarch64-linux-tdep.c (aarch64_linux_init_abi): Add call to
 	set_gdbarch_significant_addr_bit.
@@ -11228,7 +11234,7 @@
 2018-04-02  Weimin Pan  <weimin.pan@oracle.com>
 
 	PR gdb/16959
-	* cp-valprint.c: (cp_print_static_field) Fix infinite recursion when 
+	* cp-valprint.c: (cp_print_static_field) Fix infinite recursion when
 	printing static type.
 
 2018-04-01  Tom Tromey  <tom@tromey.com>
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index 0cab170456..cefc4c439b 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,8 @@
+2018-10-13  Jan Vrany  <jan.vrany@fit.cvut.cz>
+
+	* gdb.texinfo (GDB/MI Async Records): Document new async
+	records =target-connected and target-disconnected.
+
 2018-10-09  Tom Tromey  <tom@tromey.com>
 
 	* python.texi (Inferiors In Python): Link to "Frames In Python",
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index b0dc3bf67c..4579a00967 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -27902,6 +27902,13 @@ written in an inferior.  The @var{id} is the identifier of the
 thread group corresponding to the affected inferior.  The optional
 @code{type="code"} part is reported if the memory written to holds
 executable code.
+
+@item =target-connected,type="@var{type}",name="@var{name}",features=[@var{features},...]
+@itemx =target-disconnected,type="@var{type}",name="@var{name}"
+Report that a target has been connected or disconnected. The @var{type} is
+the internal target type, the @var{name} is a name of the target to be
+used for printing. The @var{features} lists connected target's features.
+See @ref{GDB/MI Support Commands} for existing target features.
 @end table
 
 @node GDB/MI Breakpoint Information
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index e055dce59e..6429184a2d 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -38,6 +38,7 @@
 #include "cli-out.h"
 #include "thread-fsm.h"
 #include "cli/cli-interp.h"
+#include "target.h"
 
 /* These are the interpreter setup, etc. functions for the MI
    interpreter.  */
@@ -85,6 +86,9 @@ 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_on_sync_execution_done (void);
+static void mi_target_connected (struct target_ops *target);
+static void mi_target_disconnected (struct target_ops *target);
+
 
 static int report_initial_inferior (struct inferior *inf, void *closure);
 
@@ -658,7 +662,7 @@ mi_on_normal_stop_1 (struct bpstats *bs, int print_frame)
       if (core != -1)
 	mi_uiout->field_int ("core", core);
     }
-  
+
   fputs_unfiltered ("*stopped", mi->raw_stdout);
   mi_out_put (mi_uiout, mi->raw_stdout);
   mi_out_rewind (mi_uiout);
@@ -1271,6 +1275,73 @@ mi_user_selected_context_changed (user_selected_what selection)
     }
 }
 
+static void
+mi_target_connected (struct target_ops *target)
+{
+  SWITCH_THRU_ALL_UIS ()
+    {
+      struct mi_interp *mi = as_mi_interp (top_level_interpreter ());
+      struct ui_out *mi_uiout;
+
+      if (mi == NULL)
+        continue;
+
+      mi_uiout = top_level_interpreter ()->interp_ui_out ();
+
+      target_terminal::scoped_restore_terminal_state term_state;
+      target_terminal::ours_for_output ();
+
+      fprintf_unfiltered (mi->event_channel,"target-connected");
+
+      mi_uiout->redirect (mi->event_channel);
+
+      mi_uiout->field_string ("type", target->shortname());
+      mi_uiout->field_string ("name", target->longname());
+
+      {
+        ui_out_emit_list list_emitter (mi_uiout, "features");
+
+        if (mi_async_p ())
+          mi_uiout->field_string (NULL, "async");
+        if (target_can_execute_reverse)
+          mi_uiout->field_string (NULL, "reverse");
+      }
+
+      mi_uiout->redirect (NULL);
+
+      gdb_flush (mi->event_channel);
+    }
+}
+
+static void
+mi_target_disconnected (struct target_ops *target)
+{
+  SWITCH_THRU_ALL_UIS ()
+    {
+      struct mi_interp *mi = as_mi_interp (top_level_interpreter ());
+      struct ui_out *mi_uiout;
+
+      if (mi == NULL)
+        continue;
+
+      mi_uiout = top_level_interpreter ()->interp_ui_out ();
+
+      target_terminal::scoped_restore_terminal_state term_state;
+      target_terminal::ours_for_output ();
+
+      fprintf_unfiltered (mi->event_channel, "target-disonnected");
+
+      mi_uiout->redirect (mi->event_channel);
+
+      mi_uiout->field_string ("type", target->shortname());
+      mi_uiout->field_string ("name", target->longname());
+
+      mi_uiout->redirect (NULL);
+
+      gdb_flush (mi->event_channel);
+    }
+}
+
 static int
 report_initial_inferior (struct inferior *inf, void *closure)
 {
@@ -1319,7 +1390,7 @@ mi_interp::set_logging (ui_file_up logfile, bool logging_redirect)
       mi->raw_stdout = mi->saved_raw_stdout;
       mi->saved_raw_stdout = NULL;
     }
-  
+
   mi->out->set_raw (mi->raw_stdout);
   mi->err->set_raw (mi->raw_stdout);
   mi->log->set_raw (mi->raw_stdout);
@@ -1373,4 +1444,6 @@ _initialize_mi_interp (void)
   gdb::observers::sync_execution_done.attach (mi_on_sync_execution_done);
   gdb::observers::user_selected_context_changed.attach
     (mi_user_selected_context_changed);
+  gdb::observers::target_connected.attach (mi_target_connected);
+  gdb::observers::target_disconnected.attach (mi_target_disconnected);
 }
-- 
2.19.1

Re: [RFC 2/2] gdb/mi: add new async events =target-connected and =target-disconnected

[Eli Zaretskii]

> From: Jan Vrany <jan.vrany@fit.cvut.cz>
> Cc: Jan Vrany <jan.vrany@fit.cvut.cz>
> Date: Sun, 14 Oct 2018 14:55:34 +0200
> 
> +@item =target-connected,type="@var{type}",name="@var{name}",features=[@var{features},...]
> +@itemx =target-disconnected,type="@var{type}",name="@var{name}"
> +Report that a target has been connected or disconnected. The @var{type} is
> +the internal target type, the @var{name} is a name of the target to be
> +used for printing. The @var{features} lists connected target's features.
                                         ^^^^^
"list", in plural.

Also, please leave 2 spaces between sentences, per our conventions.

The documentation part is approved with these n its fixed.

Thanks.

Re: [RFC 2/2] gdb/mi: add new async events =target-connected and =target-disconnected

[Simon Marchi]

On 2018-10-14 8:55 a.m., Jan Vrany wrote:
> Whenever a target is connected or disconnected, emit new asynchronous
> event =target-connected and =target-disconnected. Events report
> both short name and full name of connected or disconnected target.
> In addition, =target-connected report a set of target features.
> 
> This allows frontends to keep track of current target and its features
> regardless whether target is changed explicitly by MI -target-select
> command, CLI target command or implicitly by  native target auto-connect.

Thanks, I like the idea.  A non-RFC version of this would require corresponding
tests to be accepted.

> @@ -1271,6 +1275,73 @@ mi_user_selected_context_changed (user_selected_what selection)
>      }
>  }
>  
> +static void
> +mi_target_connected (struct target_ops *target)
> +{
> +  SWITCH_THRU_ALL_UIS ()
> +    {
> +      struct mi_interp *mi = as_mi_interp (top_level_interpreter ());
> +      struct ui_out *mi_uiout;
> +
> +      if (mi == NULL)
> +        continue;
> +
> +      mi_uiout = top_level_interpreter ()->interp_ui_out ();
> +
> +      target_terminal::scoped_restore_terminal_state term_state;
> +      target_terminal::ours_for_output ();
> +
> +      fprintf_unfiltered (mi->event_channel,"target-connected");
> +
> +      mi_uiout->redirect (mi->event_channel);
> +
> +      mi_uiout->field_string ("type", target->shortname());
> +      mi_uiout->field_string ("name", target->longname());
> +
> +      {
> +        ui_out_emit_list list_emitter (mi_uiout, "features");
> +
> +        if (mi_async_p ())
> +          mi_uiout->field_string (NULL, "async");
> +        if (target_can_execute_reverse)
> +          mi_uiout->field_string (NULL, "reverse");
> +      }
> +
> +      mi_uiout->redirect (NULL);
> +
> +      gdb_flush (mi->event_channel);
> +    }
> +}

I think there is a (kind of corner-case, but still) bug with using
mi_async_p and target_can_execute_reverse.  Here are some CLI commands
I type in a "gdb -i mi", and the corresponding
=target-connected/disconnected event:

set mi-async on
file test
=target-connected,type="exec",name="Local exec file",features=[]

start
=target-connected,type="native",name="Native process",features=["async"]

record
=target-connected,type="record-full",name="Process record and replay target",features=["async","reverse"]

file
=target-disonnected,type="exec",name="Local exec file"

file /bin/ls
=target-connected,type="exec",name="Local exec file",features=["async","reverse"]

That last event says the exec target supports async and reverse, which is wrong.
So you would need to write an equivalent of mi_async_p/target_can_execute_reverse
to which you can pass a target_ops*, it should not be too hard.

Simon

Re: [RFC 2/2] gdb/mi: add new async events =target-connected and =target-disconnected

[Jan Vrany]

Hi, 

sorry for late answer, I was traveling last couple weeks. 

On Thu, 2018-10-18 at 01:31 +0000, Simon Marchi wrote:
> On 2018-10-14 8:55 a.m., Jan Vrany wrote:
> > Whenever a target is connected or disconnected, emit new asynchronous
> > event =target-connected and =target-disconnected. Events report
> > both short name and full name of connected or disconnected target.
> > In addition, =target-connected report a set of target features.
> > 
> > This allows frontends to keep track of current target and its features
> > regardless whether target is changed explicitly by MI -target-select
> > command, CLI target command or implicitly by  native target auto-connect.
> 
> Thanks, I like the idea.  A non-RFC version of this would require corresponding
> tests to be accepted.

I'll add some. 

> 
> > @@ -1271,6 +1275,73 @@ mi_user_selected_context_changed (user_selected_what selection)
> >      }
> >  }
> >  
> > +static void
> > +mi_target_connected (struct target_ops *target)
> > +{
> > +  SWITCH_THRU_ALL_UIS ()
> > +    {
> > +      struct mi_interp *mi = as_mi_interp (top_level_interpreter ());
> > +      struct ui_out *mi_uiout;
> > +
> > +      if (mi == NULL)
> > +        continue;
> > +
> > +      mi_uiout = top_level_interpreter ()->interp_ui_out ();
> > +
> > +      target_terminal::scoped_restore_terminal_state term_state;
> > +      target_terminal::ours_for_output ();
> > +
> > +      fprintf_unfiltered (mi->event_channel,"target-connected");
> > +
> > +      mi_uiout->redirect (mi->event_channel);
> > +
> > +      mi_uiout->field_string ("type", target->shortname());
> > +      mi_uiout->field_string ("name", target->longname());
> > +
> > +      {
> > +        ui_out_emit_list list_emitter (mi_uiout, "features");
> > +
> > +        if (mi_async_p ())
> > +          mi_uiout->field_string (NULL, "async");
> > +        if (target_can_execute_reverse)
> > +          mi_uiout->field_string (NULL, "reverse");
> > +      }
> > +
> > +      mi_uiout->redirect (NULL);
> > +
> > +      gdb_flush (mi->event_channel);
> > +    }
> > +}
> 
> I think there is a (kind of corner-case, but still) bug with using
> mi_async_p and target_can_execute_reverse.  Here are some CLI commands
> I type in a "gdb -i mi", and the corresponding
> =target-connected/disconnected event:
> 
> set mi-async on
> file test
> =target-connected,type="exec",name="Local exec file",features=[]
> 
> start
> =target-connected,type="native",name="Native process",features=["async"]
> 
> record
> =target-connected,type="record-full",name="Process record and replay target",features=["async","reverse"]
> 
> file
> =target-disonnected,type="exec",name="Local exec file"
> 
> file /bin/ls
> =target-connected,type="exec",name="Local exec file",features=["async","reverse"]
> 
> That last event says the exec target supports async and reverse, which is wrong.
> So you would need to write an equivalent of mi_async_p/target_can_execute_reverse
> to which you can pass a target_ops*, it should not be too hard.

Yes, you're right, will fix that. 
But this example shows a deeper problem - it is hard to interpret these events. 

Perhaps I'm wrong but: from the user point of view, GDB can be connected to only 
one target, no? But if you look at events, it looks like targets 
"native" and "record-full" are still connected. They are, really, the inferiror
is still running, can be single-stepped and so on. 

So actually I'd expect something like: 


file test
=target-connected,type="exec",name="Local exec file",features=[]
 
start
=target-disonnected,type="exec",name="Local exec file"Simon=target-connected,type="native",name="Native process",features=["async"]
 
record
=target-disconnected,type="native",name="Native process"
=target-connected,type="record-full",name="Process record and replay target",features=["async","reverse"]

file
# Nothing, we're still connected and still recording

file /bin/ls
# Nothing, we're still connected and still recording

record stop
=target-disconnected,type="record-full",name="Process record and replay target"
=target-connected,type="native",name="Native process",features=["async"]

kill
=target-disconnected,type="native",name="Native process"
=target-connected,type="exec",name="Local exec file",features=[]

Makes sense? 
We may say that =target-connected means that previously connected target has disconnected.
I'd prefer not to, since you mentionedsome time ago that Pedro is working on multiple
target support (in that case all we need is to add a kind of "id" field to target 
notification events)

In any case I find =target-connected after issuing "file" command rather confusing. 

Makes sense? 

Jan