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

[Andrew Burgess <aburgess@redhat.com>]

* 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://sourceware.org/bugzilla/show_bug.cgi?id=20684
> ---
>  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.

> +}
> +
>  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,
and it's caused by the 'command->thread == -1' check above.

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.

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 <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.30.2
>