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

[Jan Vrany <jan.vrany@fit.cvut.cz>]

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