Re: How to use -list-target-features MI command

On Fri, 2018-04-27 at 14:37 -0400, Simon Marchi wrote:
> On 2018-04-24 06:20, Jan Vrany wrote:
> > Hi,
> > 
> > On Mon, 2018-04-23 at 14:45 +0100, Pedro Alves wrote:
> > > 
> > > > One way to hack around it would be to use for example the new
> > > > "starti" command, which breaks at (before?) the first
> > > > instruction
> > > > of the program, issue -list-target-features, and then continue.
> > > > There might be some better/cleaner solution, but that's what
> > > > comes
> > > > to mind right now.
> > > 
> > > IMO the clean solution is to connect to the native target
> > > explicitly, with "-target-select native", before -exec-run,
> > > instead of letting -exec-run automatically connect to
> > > the native target if not connected to any other target yet.
> > > 
> > >  -gdb-set mi-async on
> > >  ^done
> > >  (gdb)
> > >  -target-select native
> > >  ^connected
> > >  (gdb)
> > >  -list-target-features
> > >  ^done,features=["async"]
> > >  (gdb)
> > >  -exec-run ...
> > > 
> > > I'd also suggest that frontends consider using
> > >   "set auto-connect-native-target off"
> > > https://sourceware.org/gdb/onlinedocs/gdb/Starting.html
> > > 
> > 
> > Thanks! This makes sense. The only problem I see is that it only
> > works
> > reliably if one does not use CLI. In my case, I want user to be
> > able
> > to use the CLI.
> > 
> > 
> > > I don't see a way right now that you could know about
> > > that.  There
> > 
> > would
> > > need to be a new asynchronous event emitted when the target stack
> > > changes.
> > 
> > So gave it a go and added a new event. Seems to work fine (for me):
> > 
> > diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
> > index 8bfb1298a5..f140a3cca2 100644
> > --- a/gdb/mi/mi-interp.c
> > +++ b/gdb/mi/mi-interp.c
> > @@ -22,6 +22,7 @@
> >  #include "event-top.h"
> >  #include "event-loop.h"
> >  #include "inferior.h"
> > +#include "target.h"
> >  #include "infrun.h"
> >  #include "ui-out.h"
> >  #include "top.h"
> > @@ -85,6 +86,7 @@ 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_current_target_changed (struct target_ops *target);
> > 
> >  static int report_initial_inferior (struct inferior *inf, void
> > *closure);
> > 
> > @@ -1280,6 +1282,38 @@ mi_user_selected_context_changed
> > (user_selected_what selection)
> >      }
> >  }
> > 
> > +static void
> > +mi_current_target_changed (struct target_ops *target)
> > +{
> > +  SWITCH_THRU_ALL_UIS ()
> > +    {
> > +      struct mi_interp *mi = as_mi_interp (top_level_interpreter
> > ());
> > +
> > +      if (mi == NULL)
> > +	      continue;
> > +
> > +      target_terminal::scoped_restore_terminal_state term_state;
> > +      target_terminal::ours_for_output ();
> > +
> > +      fprintf_unfiltered (mi->event_channel,
> > +			  "target-changed,features=[");
> > +      if (mi_async_p ())
> > +        fprintf_unfiltered (mi->event_channel,
> > +          "\"async\"");
> > +      if (target_can_execute_reverse)
> > +        {
> > +          if (mi_async_p ())
> > +            fprintf_unfiltered (mi->event_channel,",\"reverse\"");
> > +          else
> > +            fprintf_unfiltered (mi->event_channel,"\"reverse\"");
> > +        }
> > +      fprintf_unfiltered (mi->event_channel,
> > +        "]");
> > +      gdb_flush (mi->event_channel);
> > +    }
> > +}
> > +
> > +
> >  static int
> >  report_initial_inferior (struct inferior *inf, void *closure)
> >  {
> > @@ -1382,4 +1416,5 @@ _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::current_target_changed.attach
> > (mi_current_target_changed);
> >  }
> > diff --git a/gdb/observable.c b/gdb/observable.c
> > index 5539b9837b..f99a39b909 100644
> > --- a/gdb/observable.c
> > +++ b/gdb/observable.c
> > @@ -74,6 +74,7 @@ DEFINE_OBSERVABLE (inferior_call_pre);
> >  DEFINE_OBSERVABLE (inferior_call_post);
> >  DEFINE_OBSERVABLE (register_changed);
> >  DEFINE_OBSERVABLE (user_selected_context_changed);
> > +DEFINE_OBSERVABLE (current_target_changed);
> > 
> >  } /* namespace observers */
> >  } /* namespace gdb */
> > diff --git a/gdb/observable.h b/gdb/observable.h
> > index 34447b90bb..3f97cc5cb1 100644
> > --- a/gdb/observable.h
> > +++ b/gdb/observable.h
> > @@ -228,6 +228,9 @@ extern observable<struct frame_info *, int>
> > register_changed;
> >     frame has changed.  */
> >  extern observable<user_selected_what>
> > user_selected_context_changed;
> > 
> > +/* The current target has changed. */
> > +extern observable<struct target_ops *> current_target_changed;
> > +
> >  } /* namespace observers */
> > 
> >  } /* namespace gdb */
> > diff --git a/gdb/target.c b/gdb/target.c
> > index e8d4ae7ea8..5cb182c849 100644
> > --- a/gdb/target.c
> > +++ b/gdb/target.c
> > @@ -49,6 +49,7 @@
> >  #include "byte-vector.h"
> >  #include "terminal.h"
> >  #include <algorithm>
> > +#include <observable.h>
> > 
> >  static void generic_tls_error (void) ATTRIBUTE_NORETURN;
> > 
> > @@ -719,6 +720,8 @@ update_current_target (void)
> > 
> >    if (targetdebug)
> >      setup_target_debug ();
> > +
> > +  gdb::observers::current_target_changed.notify(&current_target);
> >  }
> > 
> >  /* Push a new target type into the stack of the existing target
> > accessors,
> 
> Thanks, that seems to make sense with the current
> codebase.  However, 
> Pedro is currently working on adding support for GDB to be able to 
> connect to multiple targets at the same time.  So it probably be good
> to 
> think ahead about how this event would work in a multi-target
> scenario.  
> Also, how could we make -list-target-features work when connected to 
> multiple targets.

Hmm, just thinking loud: I don't know the details of Pedro's work but 
I guess in case of multiple targets, it'd make sense to have two
"target-connected" and "target-disconnected" events instead of one
"target-changed" (as in my attempt). This would work for current 
codebase with only one target at time. 

As for -list-target-features - I presume each target would have some
kind of identifier (just like threads or breakpoints do). Then one can
just extend -list-target-features with --target <target id> option. 
If --target is not specified, it would return features of "currently
selected target". This way it'd be backward compatible. 

> 
> If you want to submit a proper patch, please post it to 
> [email protected] (so we avoid adding review noise on 
> [email protected]), and see
> 
>    http://sourceware.org/gdb/wiki/ContributionChecklist
> 

I will try. 

Thanks, Jan