[PING] Re: [PATCH v3 1/1] gdb/mi: PR20684, preserve user selected thread and frame when invoking MI commands

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

On Fri, 2022-02-18 at 17:45 +0000, Jan Vrany wrote:
> On Thu, 2022-02-17 at 22:24 +0000, Andrew Burgess wrote:
> > * Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> [2022-01-27 14:50:11 +0000]:
> > 
> > > When invoking MI commands with --thread and/or --frame, user selected
> > > thread and frame was not preserved:
> > > 
> > >   (gdb)
> > >   info thread
> > >   &"info thread\n"
> > >   ~"  Id   Target Id                                           Frame \n"
> > >   ~"* 1    Thread 0x7ffff7c30740 (LWP 19302) \"user-selected-c\" main () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:60\n"
> > >   ~"  2    Thread 0x7ffff7c2f700 (LWP 19306) \"user-selected-c\" child_sub_function () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30\n"
> > >   ~"  3    Thread 0x7ffff742e700 (LWP 19307) \"user-selected-c\" child_sub_function () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30\n"
> > >   ^done
> > >   (gdb)
> > >   info frame
> > >   &"info frame\n"
> > >   ~"Stack level 0, frame at 0x7fffffffdf90:\n"
> > >   ~" rip = 0x555555555207 in main (/home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:60); saved rip = 0x7ffff7c5709b\n"
> > >   ~" source language c.\n"
> > >   ~" Arglist at 0x7fffffffdf80, args: \n"
> > >   ~" Locals at 0x7fffffffdf80, Previous frame's sp is 0x7fffffffdf90\n"
> > >   ~" Saved registers:\n "
> > >   ~" rbp at 0x7fffffffdf80, rip at 0x7fffffffdf88\n"
> > >   ^done
> > >   (gdb)
> > >   -stack-info-depth --thread 3
> > >   ^done,depth="4"
> > >   (gdb)
> > >   info thread
> > >   &"info thread\n"
> > >   ~"  Id   Target Id                                           Frame \n"
> > >   ~"  1    Thread 0x7ffff7c30740 (LWP 19302) \"user-selected-c\" main () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:60\n"
> > >   ~"  2    Thread 0x7ffff7c2f700 (LWP 19306) \"user-selected-c\" child_sub_function () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30\n"
> > >   ~"* 3    Thread 0x7ffff742e700 (LWP 19307) \"user-selected-c\" child_sub_function () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30\n"
> > >   ^done
> > >   (gdb)
> > >   info frame
> > >   &"info frame\n"
> > >   ~"Stack level 0, frame at 0x7ffff742dee0:\n"
> > >   ~" rip = 0x555555555169 in child_sub_function (/home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30); saved rip = 0x555555555188\n"
> > >   ~" called by frame at 0x7ffff742df00\n"
> > >   ~" source language c.\n"
> > >   ~" Arglist at 0x7ffff742ded0, args: \n"
> > >   ~" Locals at 0x7ffff742ded0, Previous frame's sp is 0x7ffff742dee0\n"
> > >   ~" Saved registers:\n "
> > >   ~" rbp at 0x7ffff742ded0, rip at 0x7ffff742ded8\n"
> > >   ^done
> > >   (gdb)
> > > 
> > > This was problematic for frontends that provide access to CLI because UI
> > > may silently change the context for CLI commands (as demonstrated above).
> > > 
> > > Bug: https://urldefense.proofpoint.com/v2/url?u=https-3A__sourceware.org_bugzilla_show-5Fbug.cgi-3Fid-3D20684&d=DwIBAg&c=sPZ6DeHLiehUHQWKIrsNwWp3t7snrE-az24ztT0w7Jc&r=WpFFGgYa98Yp-c29WHTCwU1wAGFBvszA6a4RzgpMSqc&m=soXzjKh4zT-FpxAe1wtdv7ka2r_jXNbrkEUR4XdUcOY&s=QkEM5YHXj7mpWI1y1z1AxRbosKxMhhfvfsMXmRnFoQc&e= 
> > > ---
> > >  gdb/mi/mi-cmds.h                             |  12 ++
> > >  gdb/mi/mi-main.c                             |  54 ++++---
> > >  gdb/testsuite/gdb.mi/mi-cmd-user-context.exp | 155 +++++++++++++++++++
> > >  3 files changed, 198 insertions(+), 23 deletions(-)
> > >  create mode 100644 gdb/testsuite/gdb.mi/mi-cmd-user-context.exp
> > > 
> > > diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
> > > index 2a93a9f5476..785652ee1c9 100644
> > > --- a/gdb/mi/mi-cmds.h
> > > +++ b/gdb/mi/mi-cmds.h
> > > @@ -23,6 +23,7 @@
> > >  #define MI_MI_CMDS_H
> > >  
> > >  #include "gdbsupport/gdb_optional.h"
> > > +#include "mi/mi-main.h"
> > >  
> > >  enum print_values {
> > >     PRINT_NO_VALUES,
> > > @@ -163,6 +164,17 @@ struct mi_command
> > >       wrong.  */
> > >    void invoke (struct mi_parse *parse) const;
> > >  
> > > +  /* Return whether this command preserves user selected context (thread
> > > +     and frame).  */
> > > +  bool preserve_user_selected_context () const
> > > +  {
> > > +    /* Here we exploit the fact that if MI command is supposed to change
> > > +       user context, then it should not emit change notifications.  Therefore if
> > > +       command does not suppress user context change notifications, then it should
> > > +       preserve the context.  */
> > > +    return m_suppress_notification != &mi_suppress_notification.user_selected_context;
> > > +  }
> > > +
> > >  protected:
> > >  
> > >    /* The core of command invocation, this needs to be overridden in each
> > > diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
> > > index 4860da7536a..e112707e4d1 100644
> > > --- a/gdb/mi/mi-main.c
> > > +++ b/gdb/mi/mi-main.c
> > > @@ -1907,6 +1907,16 @@ command_notifies_uscc_observer (struct mi_parse *command)
> > >      }
> > >  }
> > >  
> > > +/* Determine whether the thread has changed.  */
> > > +
> > > +static bool
> > > +command_changed_user_selected_thread (ptid_t previous_ptid, ptid_t current_ptid)
> > > +{
> > > +  return previous_ptid != null_ptid
> > > +         && current_ptid != previous_ptid
> > > +         && current_ptid != null_ptid;
> > 
> > You need to wrap multi-line conditions like this inside parenthesis.
> > 
> 
> Thanks! 
> 
> > > +}
> > > +
> > >  void
> > >  mi_execute_command (const char *cmd, int from_tty)
> > >  {
> > > @@ -1971,28 +1981,17 @@ mi_execute_command (const char *cmd, int from_tty)
> > >  	  && any_thread_p ()
> > >  	  /* If the command already reports the thread change, no need to do it
> > >  	     again.  */
> > > -	  && !command_notifies_uscc_observer (command.get ()))
> > > +	  && !command_notifies_uscc_observer (command.get ())
> > > +	  /* Don't report anything if --thread was specified -- the user selected
> > > +	     thread is preserved by mi_execute_command and therefore cannot
> > > +	     change.  */
> > > +	  && command->thread == -1
> > 
> > I think including this check is a bug.  Not a bug of your making I
> > think, as I believe the bug already existed.  I'll explain...
> > 
> > If I start GDB, and then spawn a new-ui for the mi interpreter.  Start
> > a multi-threaded inferior, and then stop at a gdb prompt.
> > 
> > Assuming thread 1 is selected, then, from the mi terminal I do:
> > 
> >   -thread-select 2
> > 
> > I will get the '^done,new-thread-id="2"....' result on the mi
> > terminal, but on the cli terminal I'll also get some output telling me
> > that the thread changed.  I think this is what you'd want, right?  The
> > cli thread _did_ just change.
> > 
> > However, now that thread 2 is selected, if I do this from the mi
> > terminal:
> > 
> >   -thread-select --thread 1 1
> > 
> > then I get nothing on the cli terminal.  This feels like a bug to me,
> 
> Yes, this looks like a bug indeed. And I think we just opened 
> can of worms...
> 
> > and it's caused by the 'command->thread == -1' check above.
> 
> I do not think so. When the command is -thread-select, then
> command_notifies_uscc_observer() return 1, therefore the whole
> condition won't hold anyway, right? 
> 
> > 
> > I think that check should just be dropped completely.  Your scoped
> > thread/frame restore that you've added will ensure that, when
> > appropriate, the thread doesn't change.  If it does change, then you
> > want to notify I think, regardless of whether --thread or --frame was
> > used.
> 
> If I do just drop "&& command->thread == -1" from the condition, it breaks
> existing test "--stack-select-frame 3" in mi-cmd-user-context.exp since it
> produces change notification the test does not expect. Clearly it should not
> produce MI notification, whether it should "Switching to..." CLI output over MI
> channel I'm not sure. 
> 
> But:
> 
> Now I think we can get rid of the whole "if" and with that command_notifies_uscc_observer() 
> and command_changed_user_selected_thread(). There are only two MI commands 
> that can change context are -thread-select and -stack-select-frame, right? 
> -thread-select implementation ( in mi_cmd_thread_select () ) does notify observers
> on its own and if we add similar logic to -stack-select-frame implementation
> ( in mi_cmd_stack_select_frame () ) we're fine. Makes sense?
> 
> I tried that (see the first patch below) and it seems to pass all MI tests
> and the code seems to me much simpler. 
> 
> Still, this change itself does not solve your original bug. That's because 
> thread is already switched when we enter mi_cmd_thread_select() so the 
> condition "inferior_ptid != previous_ptid" won't hold and so no notification
> is printed. 
> 
> I went ahead and tried to address this by lifting the logic up to mi_cmd_execute ()
> where --thread option is handled - see the second patch below. With that patch,
> all gdb.mi tests pass and your bug seems to be fixed. Still, the patch needs more
> work (test + check it works with -stack-frame-select).
> 
> That being said, I'd be cautious. It took me quite a bit to make heads and tails
> of this and still not sure I got it. My preference would be to fix the original 
> problem *WITHOUT* fixing "your" bug and once done, submit another patch fixing 
> "your" bug. 
> 
> So, if you agree and the first part of my response about dropping the 
> whole "if" makes any sense, I'll properly resubmit it as v4. 
> 
> Makes sense? 
> 
> > 
> > It would be great if we could have a test for this added too.  You'll
> > need to start gdb with a separate mi tty.  You can look at
> > gdb.mi/mi-exec-run.exp for an example, look for passing the
> > 'separate-mi-tty' string to mi_gdb_start, and then later for using
> > switch_gdb_spawn_id to switch between the main cli tty and the extra
> > mi tty.  If you have any problem, I'll be happy to help.
> > 
> > You still have some white space bugs (tabs vs spaces at the start of
> > lines), I have this in my .gitconfig:
> > 
> > [core]
> >         whitespace = space-before-tab,indent-with-non-tab,trailing-space
> > 
> > which will have git highlight anywhere I've failed to indent with a
> > tab when I should have done.
> > 
> > Additionally, some of your lines are over 80 characters, so need to be
> > wrapped.
> > 
> > Thanks,
> > Andrew
> > 
> > 
> > > +	  /* Don't report anything if the selected thread actually did not change
> > > +	     (compared to selected thread before execution the command).  */
> > > +	  && command_changed_user_selected_thread (previous_ptid, inferior_ptid))
> > >  	{
> > > -	  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);
> > > -	    }
> > > +	  gdb::observers::user_selected_context_changed.notify
> > > +			(USER_SELECTED_THREAD | USER_SELECTED_FRAME);
> > >  	}
> > >      }
> > >  }
> > > @@ -2037,6 +2036,7 @@ mi_cmd_execute (struct mi_parse *parse)
> > >        set_current_program_space (inf->pspace);
> > >      }
> > >  
> > > +  gdb::optional<scoped_restore_current_thread> thread_saver;
> > >    if (parse->thread != -1)
> > >      {
> > >        thread_info *tp = find_thread_global_id (parse->thread);
> > > @@ -2047,9 +2047,13 @@ mi_cmd_execute (struct mi_parse *parse)
> > >        if (tp->state == THREAD_EXITED)
> > >  	error (_("Thread id: %d has terminated"), parse->thread);
> > >  
> > > +      if (parse->cmd->preserve_user_selected_context ())
> > > +	thread_saver.emplace ();
> > > +
> > >        switch_to_thread (tp);
> > >      }
> > >  
> > > +  gdb::optional<scoped_restore_selected_frame> frame_saver;
> > >    if (parse->frame != -1)
> > >      {
> > >        struct frame_info *fid;
> > > @@ -2057,8 +2061,12 @@ mi_cmd_execute (struct mi_parse *parse)
> > >  
> > >        fid = find_relative_frame (get_current_frame (), &frame);
> > >        if (frame == 0)
> > > -	/* find_relative_frame was successful */
> > > -	select_frame (fid);
> > > +        {
> > > +	  if (parse->cmd->preserve_user_selected_context ())
> > > +	    frame_saver.emplace ();
> > > +
> > > +          select_frame (fid);
> > > +        }
> > >        else
> > >  	error (_("Invalid frame id: %d"), frame);
> > >      }
> > > diff --git a/gdb/testsuite/gdb.mi/mi-cmd-user-context.exp b/gdb/testsuite/gdb.mi/mi-cmd-user-context.exp
> > > new file mode 100644
> > > index 00000000000..a373dd3a4b0
> > > --- /dev/null
> > > +++ b/gdb/testsuite/gdb.mi/mi-cmd-user-context.exp
> > > @@ -0,0 +1,155 @@
> > > +# Copyright 2022 Free Software Foundation, Inc.
> > > +
> > > +# This program is free software; you can redistribute it and/or modify
> > > +# it under the terms of the GNU General Public License as published by
> > > +# the Free Software Foundation; either version 3 of the License, or
> > > +# (at your option) any later version.
> > > +#
> > > +# This program is distributed in the hope that it will be useful,
> > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > +# GNU General Public License for more details.
> > > +#
> > > +# You should have received a copy of the GNU General Public License
> > > +# along with this program.  If not, see <https://urldefense.proofpoint.com/v2/url?u=http-3A__www.gnu.org_licenses_&d=DwIBAg&c=sPZ6DeHLiehUHQWKIrsNwWp3t7snrE-az24ztT0w7Jc&r=WpFFGgYa98Yp-c29WHTCwU1wAGFBvszA6a4RzgpMSqc&m=soXzjKh4zT-FpxAe1wtdv7ka2r_jXNbrkEUR4XdUcOY&s=q6IZFROyYXzF9keb4NCS6EL93YiTxbx2HkwQEtqDr8k&e= >.
> > > +
> > > +# Test that GDB/MI commands preserve user selected context when
> > > +# passed --thread and/or --frame.
> > > +
> > > +load_lib mi-support.exp
> > > +
> > > +standard_testfile user-selected-context-sync.c
> > > +
> > > +if {[build_executable $testfile.exp $testfile ${srcfile} "debug pthreads"] == -1} {
> > > +    untested "failed to compile"
> > > +    return -1
> > > +}
> > > +
> > > +set main_break_line [gdb_get_line_number "main break line"]
> > > +
> > > +set any "\[^\r\n\]*"
> > > +set nl  "\[\r\n\]"
> > > +
> > > +mi_clean_restart $binfile
> > > +mi_create_breakpoint "$srcfile:$main_break_line" "set breakpoint in main"
> > > +mi_run_cmd
> > > +mi_expect_stop "breakpoint-hit" "main" "" $srcfile $main_break_line \
> > > +	 { "" "disp=\"keep\"" } "run to breakpoint in main"
> > > +
> > > +mi_gdb_test "thread" \
> > > +	".*Current thread is 1.*" \
> > > +	"info thread 1"
> > > +
> > > +#=========================
> > > +
> > > +mi_gdb_test "-stack-info-depth --thread 3" \
> > > +	"\\^done,depth=.*" \
> > > +	"-stack-info-depth --thread 3"
> > > +
> > > +mi_gdb_test "thread" \
> > > +	".*Current thread is 1.*" \
> > > +	"info thread 2"
> > > +
> > > +#=========================
> > > +
> > > +mi_gdb_test "-thread-select 3" \
> > > +	"\\^done,.*" \
> > > +	"-thread-select 3"
> > > +
> > > +mi_gdb_test "thread" \
> > > +	".*Current thread is 3.*" \
> > > +	"info thread 3"
> > > +
> > > +#=========================
> > > +
> > > +mi_gdb_test "-thread-select --thread 2 1" \
> > > +	"\\^done,.*" \
> > > +	"-thread-select --thread 2 1"
> > > +
> > > +mi_gdb_test "thread" \
> > > +	".*Current thread is 1.*" \
> > > +	"info thread 4"
> > > +
> > > +#=========================
> > > +
> > > +mi_gdb_test "-thread-select --thread 2 2" \
> > > +	"\\^done,.*" \
> > > +	"-thread-select --thread 2 2"
> > > +
> > > +mi_gdb_test "thread" \
> > > +	".*Current thread is 2.*" \
> > > +	"info thread 5"
> > > +
> > > +#=========================
> > > +
> > > +mi_gdb_test "frame" \
> > > +	".*#0  0x.*" \
> > > +	"frame 1"
> > > +
> > > +mi_gdb_test "-stack-info-frame --thread 2 --frame 1" \
> > > +	"\\^done,frame=\{level=\"1\".*" \
> > > +	"-stack-info-frame 1"
> > > +
> > > +mi_gdb_test "thread" \
> > > +	".*Current thread is 2.*" \
> > > +	"info thread 6"
> > > +
> > > +mi_gdb_test "frame" \
> > > +	".*#0  0x.*" \
> > > +	"frame 2"
> > > +
> > > +#=========================
> > > +
> > > +mi_gdb_test "-stack-info-frame --thread 3 --frame 1" \
> > > +	"\\^done,frame=\{level=\"1\".*" \
> > > +	"-stack-info-frame 2"
> > > +
> > > +mi_gdb_test "thread" \
> > > +	".*Current thread is 2.*" \
> > > +	"info thread 7"
> > > +
> > > +mi_gdb_test "frame" \
> > > +	".*#0  0x.*" \
> > > +	"frame 3"
> > > +
> > > +#=========================
> > > +
> > > +mi_gdb_test "-stack-select-frame --thread 2 --frame 0 1" \
> > > +	"\\^done" \
> > > +	"--stack-select-frame 1"
> > > +
> > > +mi_gdb_test "thread" \
> > > +	".*Current thread is 2.*" \
> > > +	"info thread 8"
> > > +
> > > +mi_gdb_test "frame" \
> > > +	".*#1  0x.*" \
> > > +	"frame 4"
> > > +
> > > +#=========================
> > > +
> > > +mi_gdb_test "-stack-select-frame --thread 2 --frame 2 2" \
> > > +	"\\^done" \
> > > +	"--stack-select-frame 2"
> > > +
> > > +mi_gdb_test "thread" \
> > > +	".*Current thread is 2.*" \
> > > +	"info thread 9"
> > > +
> > > +mi_gdb_test "frame" \
> > > +	".*#2  0x.*" \
> > > +	"frame 5"
> > > +
> > > +#=========================
> > > +
> > > +mi_gdb_test "-stack-select-frame --thread 1 --frame 0 0" \
> > > +	"\\^done" \
> > > +	"--stack-select-frame 3"
> > > +
> > > +mi_gdb_test "thread" \
> > > +	".*Current thread is 1.*" \
> > > +	"info thread 10"
> > > +
> > > +mi_gdb_test "frame" \
> > > +	".*#0  main.*" \
> > > +	"frame 6"
> > > -- 
> > > 2.30.2
> > > 
> > 
> 
> From db229be447707d57980e4c21f394cdba1ec86b4c Mon Sep 17 00:00:00 2001
> From: Jan Vrany <jan.vrany@labware.com>
> Date: Thu, 27 Jan 2022 13:45:09 +0000
> Subject: [PATCH 1/2] gdb/mi: PR20684, preserve user selected thread and frame
>  when invoking MI commands
> 
> When invoking MI commands with --thread and/or --frame, user selected
> thread and frame was not preserved:
> 
>   (gdb)
>   info thread
>   &"info thread\n"
>   ~"  Id   Target Id                                           Frame \n"
>   ~"* 1    Thread 0x7ffff7c30740 (LWP 19302) \"user-selected-c\" main () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:60\n"
>   ~"  2    Thread 0x7ffff7c2f700 (LWP 19306) \"user-selected-c\" child_sub_function () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30\n"
>   ~"  3    Thread 0x7ffff742e700 (LWP 19307) \"user-selected-c\" child_sub_function () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30\n"
>   ^done
>   (gdb)
>   info frame
>   &"info frame\n"
>   ~"Stack level 0, frame at 0x7fffffffdf90:\n"
>   ~" rip = 0x555555555207 in main (/home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:60); saved rip = 0x7ffff7c5709b\n"
>   ~" source language c.\n"
>   ~" Arglist at 0x7fffffffdf80, args: \n"
>   ~" Locals at 0x7fffffffdf80, Previous frame's sp is 0x7fffffffdf90\n"
>   ~" Saved registers:\n "
>   ~" rbp at 0x7fffffffdf80, rip at 0x7fffffffdf88\n"
>   ^done
>   (gdb)
>   -stack-info-depth --thread 3
>   ^done,depth="4"
>   (gdb)
>   info thread
>   &"info thread\n"
>   ~"  Id   Target Id                                           Frame \n"
>   ~"  1    Thread 0x7ffff7c30740 (LWP 19302) \"user-selected-c\" main () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:60\n"
>   ~"  2    Thread 0x7ffff7c2f700 (LWP 19306) \"user-selected-c\" child_sub_function () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30\n"
>   ~"* 3    Thread 0x7ffff742e700 (LWP 19307) \"user-selected-c\" child_sub_function () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30\n"
>   ^done
>   (gdb)
>   info frame
>   &"info frame\n"
>   ~"Stack level 0, frame at 0x7ffff742dee0:\n"
>   ~" rip = 0x555555555169 in child_sub_function (/home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30); saved rip = 0x555555555188\n"
>   ~" called by frame at 0x7ffff742df00\n"
>   ~" source language c.\n"
>   ~" Arglist at 0x7ffff742ded0, args: \n"
>   ~" Locals at 0x7ffff742ded0, Previous frame's sp is 0x7ffff742dee0\n"
>   ~" Saved registers:\n "
>   ~" rbp at 0x7ffff742ded0, rip at 0x7ffff742ded8\n"
>   ^done
>   (gdb)
> 
> This was problematic for frontends that provide access to CLI because UI
> may silently change the context for CLI commands (as demonstrated above).
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=20684
> ---
>  gdb/mi/mi-cmd-stack.c                        |  11 ++
>  gdb/mi/mi-cmds.h                             |  12 ++
>  gdb/mi/mi-main.c                             |  74 ++-------
>  gdb/testsuite/gdb.mi/mi-cmd-user-context.exp | 155 +++++++++++++++++++
>  4 files changed, 189 insertions(+), 63 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.mi/mi-cmd-user-context.exp
> 
> diff --git a/gdb/mi/mi-cmd-stack.c b/gdb/mi/mi-cmd-stack.c
> index e63f1706149..1be8aa81c3d 100644
> --- a/gdb/mi/mi-cmd-stack.c
> +++ b/gdb/mi/mi-cmd-stack.c
> @@ -36,6 +36,8 @@
>  #include "mi-parse.h"
>  #include "gdbsupport/gdb_optional.h"
>  #include "safe-ctype.h"
> +#include "inferior.h"
> +#include "observable.h"
>  
>  enum what_to_list { locals, arguments, all };
>  
> @@ -756,7 +758,16 @@ mi_cmd_stack_select_frame (const char *command, char **argv, int argc)
>    if (argc == 0 || argc > 1)
>      error (_("-stack-select-frame: Usage: FRAME_SPEC"));
>  
> +  ptid_t previous_ptid = inferior_ptid;
> +
>    select_frame_for_mi (parse_frame_specification (argv[0]));
> +
> +  /* Notify if the thread has effectively changed.  */
> +  if (inferior_ptid != previous_ptid)
> +    {
> +      gdb::observers::user_selected_context_changed.notify
> +	(USER_SELECTED_THREAD | USER_SELECTED_FRAME);
> +    }
>  }
>  
>  void
> diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
> index 2a93a9f5476..785652ee1c9 100644
> --- a/gdb/mi/mi-cmds.h
> +++ b/gdb/mi/mi-cmds.h
> @@ -23,6 +23,7 @@
>  #define MI_MI_CMDS_H
>  
>  #include "gdbsupport/gdb_optional.h"
> +#include "mi/mi-main.h"
>  
>  enum print_values {
>     PRINT_NO_VALUES,
> @@ -163,6 +164,17 @@ struct mi_command
>       wrong.  */
>    void invoke (struct mi_parse *parse) const;
>  
> +  /* Return whether this command preserves user selected context (thread
> +     and frame).  */
> +  bool preserve_user_selected_context () const
> +  {
> +    /* Here we exploit the fact that if MI command is supposed to change
> +       user context, then it should not emit change notifications.  Therefore if
> +       command does not suppress user context change notifications, then it should
> +       preserve the context.  */
> +    return m_suppress_notification != &mi_suppress_notification.user_selected_context;
> +  }
> +
>  protected:
>  
>    /* The core of command invocation, this needs to be overridden in each
> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
> index 4860da7536a..23e2373dcec 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -1879,34 +1879,6 @@ mi_print_exception (const char *token, const struct gdb_exception &exception)
>    fputs_unfiltered ("\n", mi->raw_stdout);
>  }
>  
> -/* Determine whether the parsed command already notifies the
> -   user_selected_context_changed observer.  */
> -
> -static int
> -command_notifies_uscc_observer (struct mi_parse *command)
> -{
> -  if (command->op == CLI_COMMAND)
> -    {
> -      /* CLI commands "thread" and "inferior" already send it.  */
> -      return (startswith (command->command, "thread ")
> -	      || startswith (command->command, "inferior "));
> -    }
> -  else /* MI_COMMAND */
> -    {
> -      if (strcmp (command->command, "interpreter-exec") == 0
> -	  && command->argc > 1)
> -	{
> -	  /* "thread" and "inferior" again, but through -interpreter-exec.  */
> -	  return (startswith (command->argv[1], "thread ")
> -		  || startswith (command->argv[1], "inferior "));
> -	}
> -
> -      else
> -	/* -thread-select already sends it.  */
> -	return strcmp (command->command, "thread-select") == 0;
> -    }
> -}
> -
>  void
>  mi_execute_command (const char *cmd, int from_tty)
>  {
> @@ -1932,8 +1904,6 @@ mi_execute_command (const char *cmd, int from_tty)
>  
>    if (command != NULL)
>      {
> -      ptid_t previous_ptid = inferior_ptid;
> -
>        command->token = token;
>  
>        if (do_timings)
> @@ -1963,37 +1933,6 @@ mi_execute_command (const char *cmd, int from_tty)
>  
>        bpstat_do_actions ();
>  
> -      if (/* The notifications are only output when the top-level
> -	     interpreter (specified on the command line) is MI.  */
> -	  top_level_interpreter ()->interp_ui_out ()->is_mi_like_p ()
> -	  /* Don't try report anything if there are no threads --
> -	     the program is dead.  */
> -	  && any_thread_p ()
> -	  /* If the command already reports the thread change, no need to do it
> -	     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);
> -	    }
> -	}
>      }
>  }
>  
> @@ -2037,6 +1976,7 @@ mi_cmd_execute (struct mi_parse *parse)
>        set_current_program_space (inf->pspace);
>      }
>  
> +  gdb::optional<scoped_restore_current_thread> thread_saver;
>    if (parse->thread != -1)
>      {
>        thread_info *tp = find_thread_global_id (parse->thread);
> @@ -2047,9 +1987,13 @@ mi_cmd_execute (struct mi_parse *parse)
>        if (tp->state == THREAD_EXITED)
>  	error (_("Thread id: %d has terminated"), parse->thread);
>  
> +      if (parse->cmd->preserve_user_selected_context ())
> +	thread_saver.emplace ();
> +
>        switch_to_thread (tp);
>      }
>  
> +  gdb::optional<scoped_restore_selected_frame> frame_saver;
>    if (parse->frame != -1)
>      {
>        struct frame_info *fid;
> @@ -2057,8 +2001,12 @@ mi_cmd_execute (struct mi_parse *parse)
>  
>        fid = find_relative_frame (get_current_frame (), &frame);
>        if (frame == 0)
> -	/* find_relative_frame was successful */
> -	select_frame (fid);
> +	{
> +	  if (parse->cmd->preserve_user_selected_context ())
> +	    frame_saver.emplace ();
> +
> +	  select_frame (fid);
> +	}
>        else
>  	error (_("Invalid frame id: %d"), frame);
>      }
> diff --git a/gdb/testsuite/gdb.mi/mi-cmd-user-context.exp b/gdb/testsuite/gdb.mi/mi-cmd-user-context.exp
> new file mode 100644
> index 00000000000..a373dd3a4b0
> --- /dev/null
> +++ b/gdb/testsuite/gdb.mi/mi-cmd-user-context.exp
> @@ -0,0 +1,155 @@
> +# Copyright 2022 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Test that GDB/MI commands preserve user selected context when
> +# passed --thread and/or --frame.
> +
> +load_lib mi-support.exp
> +
> +standard_testfile user-selected-context-sync.c
> +
> +if {[build_executable $testfile.exp $testfile ${srcfile} "debug pthreads"] == -1} {
> +    untested "failed to compile"
> +    return -1
> +}
> +
> +set main_break_line [gdb_get_line_number "main break line"]
> +
> +set any "\[^\r\n\]*"
> +set nl  "\[\r\n\]"
> +
> +mi_clean_restart $binfile
> +mi_create_breakpoint "$srcfile:$main_break_line" "set breakpoint in main"
> +mi_run_cmd
> +mi_expect_stop "breakpoint-hit" "main" "" $srcfile $main_break_line \
> +	 { "" "disp=\"keep\"" } "run to breakpoint in main"
> +
> +mi_gdb_test "thread" \
> +	".*Current thread is 1.*" \
> +	"info thread 1"
> +
> +#=========================
> +
> +mi_gdb_test "-stack-info-depth --thread 3" \
> +	"\\^done,depth=.*" \
> +	"-stack-info-depth --thread 3"
> +
> +mi_gdb_test "thread" \
> +	".*Current thread is 1.*" \
> +	"info thread 2"
> +
> +#=========================
> +
> +mi_gdb_test "-thread-select 3" \
> +	"\\^done,.*" \
> +	"-thread-select 3"
> +
> +mi_gdb_test "thread" \
> +	".*Current thread is 3.*" \
> +	"info thread 3"
> +
> +#=========================
> +
> +mi_gdb_test "-thread-select --thread 2 1" \
> +	"\\^done,.*" \
> +	"-thread-select --thread 2 1"
> +
> +mi_gdb_test "thread" \
> +	".*Current thread is 1.*" \
> +	"info thread 4"
> +
> +#=========================
> +
> +mi_gdb_test "-thread-select --thread 2 2" \
> +	"\\^done,.*" \
> +	"-thread-select --thread 2 2"
> +
> +mi_gdb_test "thread" \
> +	".*Current thread is 2.*" \
> +	"info thread 5"
> +
> +#=========================
> +
> +mi_gdb_test "frame" \
> +	".*#0  0x.*" \
> +	"frame 1"
> +
> +mi_gdb_test "-stack-info-frame --thread 2 --frame 1" \
> +	"\\^done,frame=\{level=\"1\".*" \
> +	"-stack-info-frame 1"
> +
> +mi_gdb_test "thread" \
> +	".*Current thread is 2.*" \
> +	"info thread 6"
> +
> +mi_gdb_test "frame" \
> +	".*#0  0x.*" \
> +	"frame 2"
> +
> +#=========================
> +
> +mi_gdb_test "-stack-info-frame --thread 3 --frame 1" \
> +	"\\^done,frame=\{level=\"1\".*" \
> +	"-stack-info-frame 2"
> +
> +mi_gdb_test "thread" \
> +	".*Current thread is 2.*" \
> +	"info thread 7"
> +
> +mi_gdb_test "frame" \
> +	".*#0  0x.*" \
> +	"frame 3"
> +
> +#=========================
> +
> +mi_gdb_test "-stack-select-frame --thread 2 --frame 0 1" \
> +	"\\^done" \
> +	"--stack-select-frame 1"
> +
> +mi_gdb_test "thread" \
> +	".*Current thread is 2.*" \
> +	"info thread 8"
> +
> +mi_gdb_test "frame" \
> +	".*#1  0x.*" \
> +	"frame 4"
> +
> +#=========================
> +
> +mi_gdb_test "-stack-select-frame --thread 2 --frame 2 2" \
> +	"\\^done" \
> +	"--stack-select-frame 2"
> +
> +mi_gdb_test "thread" \
> +	".*Current thread is 2.*" \
> +	"info thread 9"
> +
> +mi_gdb_test "frame" \
> +	".*#2  0x.*" \
> +	"frame 5"
> +
> +#=========================
> +
> +mi_gdb_test "-stack-select-frame --thread 1 --frame 0 0" \
> +	"\\^done" \
> +	"--stack-select-frame 3"
> +
> +mi_gdb_test "thread" \
> +	".*Current thread is 1.*" \
> +	"info thread 10"
> +
> +mi_gdb_test "frame" \
> +	".*#0  main.*" \
> +	"frame 6"
> -- 
> 2.34.1
> 
> 
> From 18d8dc95fe1a834ecbde319dd6e5960fd4d82a3f Mon Sep 17 00:00:00 2001
> From: Jan Vrany <jan.vrany@labware.com>
> Date: Fri, 18 Feb 2022 17:04:13 +0000
> Subject: [PATCH 2/2] [WIP] gdb/mi: properly notify user when GDB/MI client
>  uses -thread-select
> 
> If we start GDB, and then spawn a new-ui for the mi interpreter.  Start
> a multi-threaded inferior, and then stop at a gdb prompt.
> 
> Assuming thread 1 is selected, then, from the mi terminal I do:
> 
>   -thread-select 2
> 
> We will get the '^done,new-thread-id="2"....' result on the mi
> terminal, but on the cli terminal I'll also get some output telling me
> that the thread changed.
> 
> However, now that thread 2 is selected, if I do this from the mi
> terminal:
> 
>   -thread-select --thread 1 1
> 
> then I get nothing on the CLI terminal. This seems to be a bug.
> 
> The problem was that when `-thread-select --thread 1 1` then thread
> is switched to thread 1 before mi_cmd_thread_select () is called,
> therefore the condition "inferior_ptid != previous_ptid" there does
> not hold.
> 
> To address this problem, we have to move this logic up to
> mi_cmd_execute () where --thread option is processed and notify
> user selected contents observers there if context changes.
> 
> However, this in itself breaks GDB/MI because it would cause context
> notification to be sent on MI channel. This is because by the time
> we notify, MI notification suppression is already restored (done in
> mi_command::invoke(). Therefore we had to lift notification suppression
> logic also up to mi_cmd_execute ().
> 
> With this change, all gdb.mi tests pass, tested on x86_64-linux.
> 
> TODO:
>  * add test
> ---
>  gdb/mi/mi-cmd-stack.c | 10 ----------
>  gdb/mi/mi-cmds.c      |  2 --
>  gdb/mi/mi-cmds.h      | 16 ++++++++--------
>  gdb/mi/mi-main.c      | 32 +++++++++++++++++++++++---------
>  4 files changed, 31 insertions(+), 29 deletions(-)
> 
> diff --git a/gdb/mi/mi-cmd-stack.c b/gdb/mi/mi-cmd-stack.c
> index 1be8aa81c3d..afe584b7fca 100644
> --- a/gdb/mi/mi-cmd-stack.c
> +++ b/gdb/mi/mi-cmd-stack.c
> @@ -757,17 +757,7 @@ mi_cmd_stack_select_frame (const char *command, char **argv, int argc)
>  {
>    if (argc == 0 || argc > 1)
>      error (_("-stack-select-frame: Usage: FRAME_SPEC"));
> -
> -  ptid_t previous_ptid = inferior_ptid;
> -
>    select_frame_for_mi (parse_frame_specification (argv[0]));
> -
> -  /* Notify if the thread has effectively changed.  */
> -  if (inferior_ptid != previous_ptid)
> -    {
> -      gdb::observers::user_selected_context_changed.notify
> -	(USER_SELECTED_THREAD | USER_SELECTED_FRAME);
> -    }
>  }
>  
>  void
> diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
> index cd7cabdda9b..957dfb4e03d 100644
> --- a/gdb/mi/mi-cmds.c
> +++ b/gdb/mi/mi-cmds.c
> @@ -171,8 +171,6 @@ mi_command::mi_command (const char *name, int *suppress_notification)
>  void
>  mi_command::invoke (struct mi_parse *parse) const
>  {
> -  gdb::optional<scoped_restore_tmpl<int>> restore
> -    = do_suppress_notification ();
>    this->do_invoke (parse);
>  }
>  
> diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
> index 785652ee1c9..da1598a3f32 100644
> --- a/gdb/mi/mi-cmds.h
> +++ b/gdb/mi/mi-cmds.h
> @@ -175,14 +175,6 @@ struct mi_command
>      return m_suppress_notification != &mi_suppress_notification.user_selected_context;
>    }
>  
> -protected:
> -
> -  /* The core of command invocation, this needs to be overridden in each
> -     base class.  PARSE is the parsed command line from the user.  */
> -  virtual void do_invoke (struct mi_parse *parse) const = 0;
> -
> -private:
> -
>    /* If this command was created with a suppress notifications pointer,
>       then this function will set the suppress flag and return a
>       gdb::optional with its value set to an object that will restore the
> @@ -192,6 +184,14 @@ struct mi_command
>       then this function returns an empty gdb::optional.  */
>    gdb::optional<scoped_restore_tmpl<int>> do_suppress_notification () const;
>  
> +protected:
> +
> +  /* The core of command invocation, this needs to be overridden in each
> +     base class.  PARSE is the parsed command line from the user.  */
> +  virtual void do_invoke (struct mi_parse *parse) const = 0;
> +
> +private:
> +
>    /* The name of the command.  */
>    const char *m_name;
>  
> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
> index 23e2373dcec..608b226f496 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -556,19 +556,10 @@ mi_cmd_thread_select (const char *command, char **argv, int argc)
>    if (thr == NULL)
>      error (_("Thread ID %d not known."), num);
>  
> -  ptid_t previous_ptid = inferior_ptid;
> -
>    thread_select (argv[0], thr);
>  
>    print_selected_thread_frame (current_uiout,
>  			       USER_SELECTED_THREAD | USER_SELECTED_FRAME);
> -
> -  /* Notify if the thread has effectively changed.  */
> -  if (inferior_ptid != previous_ptid)
> -    {
> -      gdb::observers::user_selected_context_changed.notify
> -	(USER_SELECTED_THREAD | USER_SELECTED_FRAME);
> -    }
>  }
>  
>  void
> @@ -1936,6 +1927,16 @@ mi_execute_command (const char *cmd, int from_tty)
>      }
>  }
>  
> +/* Determine whether the thread has changed.  */
> +
> +static bool
> +command_changed_user_selected_thread (ptid_t previous_ptid, ptid_t current_ptid)
> +{
> +  return (previous_ptid != null_ptid
> +	  && current_ptid != previous_ptid
> +	  && current_ptid != null_ptid);
> +}        
> +
>  static void
>  mi_cmd_execute (struct mi_parse *parse)
>  {
> @@ -1976,6 +1977,8 @@ mi_cmd_execute (struct mi_parse *parse)
>        set_current_program_space (inf->pspace);
>      }
>  
> +  ptid_t previous_ptid = inferior_ptid;
> +
>    gdb::optional<scoped_restore_current_thread> thread_saver;
>    if (parse->thread != -1)
>      {
> @@ -2021,7 +2024,18 @@ mi_cmd_execute (struct mi_parse *parse)
>    current_context = parse;
>  
>    gdb_assert (parse->cmd != nullptr);
> +  
> +  gdb::optional<scoped_restore_tmpl<int>> restore_suppress_notification
> +    = parse->cmd->do_suppress_notification ();
> +
>    parse->cmd->invoke (parse);
> +
> +  if (!parse->cmd->preserve_user_selected_context ()
> +      && command_changed_user_selected_thread (previous_ptid, inferior_ptid))
> +    {
> +      gdb::observers::user_selected_context_changed.notify
> +      (USER_SELECTED_THREAD | USER_SELECTED_FRAME);
> +    }
>  }
>  
>  /* See mi-main.h.  */