[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(-)

[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 @@
[phone] 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 <***@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;
}

[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 <***@fit.cvut.cz>
[phone] 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.
+
[phone] 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 <***@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 <***@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 <***@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 @@
[phone] 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 <***@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);
}

[Eli Zaretskii]

Date: Sun, 14 Oct 2018 14:55:34 +0200

^^^^^
"list", in plural.

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

The documentation part is approved with these n its fixed.

Thanks.

[Simon Marchi]

Post by 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.

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

Post by Jan Vrany
@@ -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

[Jan Vrany]

Hi,

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

Post by Simon Marchi

Post by 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.

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

I'll add some.

Post by Simon Marchi

Post by Jan Vrany
@@ -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
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