Re: [PATCH] MI: PR20684, preserve user selected thread and frame when invoking MI commands

[Jan Vrany <jan.vrany@labware.com>]

On Thu, 2021-03-25 at 14:25 -0400, Simon Marchi wrote:
> 
> 
> > 
> > This was problematic for frontends that provide access to CLI
> > because UI
> > may silently change the context for CLI commands (as demonstrated
> > above).
> 
> Hi Jan,
> 
> Sorry for not getting at this sooner, I was away for a while.
> 
> We'll definitely want a test for this, because it's something that
> can
> easily break unexpectedly.

Hi Simon, 

Sorry for not getting at this sooner, this time I was away. 

I have written some tests, though I do not test notifications
when having separate CLI and MI channel. I tried to look at 
user-selected-context-sync.exp but failed to understand how 
it works. 

> 
> One behavior change with this patch is: assuming thread 1 is
> selected,
> if you do:
> 
>     -thread-select --thread 2 3
> 
> Before: thread 3 becomes selected
> After: thread 1 stays selected
> 
> Even though it looks a bit silly, I think we should keep the "before"
> behavior.  An existing frontend may add `--thread X` to every
> command,
> including -thread-select.  So let's say you click on thread 4 in the
> interface, it sends to GDB:
> 
>     -thread-select --thread 4 4
> 
> With this patch, the selection change wouldn't work.

Good catch, thanks! It did not occur to me someone might do that. 
Similar thing may happen for frame and -frame-select. 

The only way fixing it I can think of now is to special-case it like:

@@ -2034,7 +2034,12 @@ mi_cmd_execute (struct mi_parse *parse)
       if (tp->state == THREAD_EXITED)
        error (_("Thread id: %d has terminated"), parse->thread);
 
-      thread_saver.emplace ();
+      /* The -thread-select and -select-frame are the only commands
+         that change the currently selected thread, so do not restore
+         thread after executing any of them.  */
+      if (parse->cmd->argv_func != mi_cmd_thread_select
+         && parse->cmd->argv_func != mi_cmd_stack_select_frame)
+       thread_saver.emplace ();
       switch_to_thread (tp);
     }

This fixes your usecase, but is there a better way? I need to 
think of it. 
 
> 
> > gdb/Changelog
> > 
> >         PR 20684
> >         * gdb/mi/mi-main.c (mi_cmd_execute): Preserve user selected
> > thread
> 
> Remove leading "gdb/".
> 
> >         and frame when invoking MI commands.
> > ---
> >  gdb/ChangeLog    |  6 ++++++
> >  gdb/mi/mi-main.c | 36 +++++++++++++++---------------------
> >  2 files changed, 21 insertions(+), 21 deletions(-)
> > 
> > diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> > index e67668d315c..ea232ff1b3f 100644
> > --- a/gdb/ChangeLog
> > +++ b/gdb/ChangeLog
> > @@ -1,3 +1,9 @@
> > +2021-02-09  Jan Vrany  <jan.vrany@labware.com>
> > +
> > +       PR 20684
> > +       * gdb/mi/mi-main.c (mi_cmd_execute): Preserve user selected
> > thread
> > +       and frame when invoking MI commands.
> > +
> >  2021-02-08  Shahab Vahedi  <shahab@synopsys.com>
> >  
> >         PR tdep/27369
> > diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
> > index 9a14d78e1e2..c5103800314 100644
> > --- a/gdb/mi/mi-main.c
> > +++ b/gdb/mi/mi-main.c
> > @@ -1971,25 +1971,13 @@ mi_execute_command (const char *cmd, int
> > from_tty)
> >              again.  */
> >           && !command_notifies_uscc_observer (command.get ()))
> >         {
> > -         int report_change = 0;
> > -
> > -         if (command->thread == -1)
> > -           {
> > -             report_change = (previous_ptid != null_ptid
> > -                              && inferior_ptid != previous_ptid
> > -                              && inferior_ptid != null_ptid);
> > -           }
> > -         else if (inferior_ptid != null_ptid)
> > -           {
> > -             struct thread_info *ti = inferior_thread ();
> > -
> > -             report_change = (ti->global_num != command->thread);
> > -           }
> > -
> > -         if (report_change)
> > -           {
> > -             gdb::observers::user_selected_context_changed.notify
> > -               (USER_SELECTED_THREAD | USER_SELECTED_FRAME);
> > +         if (command->thread == -1
> > +             && previous_ptid != null_ptid
> > +             && inferior_ptid != previous_ptid
> > +             && inferior_ptid != null_ptid)
> > +            {
> > +              gdb::observers::user_selected_context_changed.notify
> > +                      (USER_SELECTED_THREAD |
> > USER_SELECTED_FRAME);
> 
> Can you explain the what and why of this change?  It's not obvious to
> me.

I actually wrote this patch a long time ago and using it since then,
but I believe the reason for removing 

     else if (inferior_ptid != null_ptid)
       {
         ...
       }


branch was that if --thread is specified, we do not want any
notifications that context has changed. Imagine the currently
selected thread is 1, frontend passes --thread 2 to a command. 
Then 

    report_change = (ti->global_num != command->thread);

will be true (because 1 != 2) and CLI channel (if any) would get
notification which is not what we want [1]

[1]: https://sourceware.org/legacy-ml/gdb/2019-06/msg00057.html

Thanks! 

Jan