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

[Simon Marchi <simon.marchi@polymtl.ca>]

Thanks, this is OK.

Simon

On 2022-01-26 08:21, Jan Vrany via Gdb-patches wrote:
> 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                             |  13 +-
>  gdb/testsuite/gdb.mi/mi-cmd-user-context.exp | 155 +++++++++++++++++++
>  3 files changed, 178 insertions(+), 2 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..b25e87a1476 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -2037,6 +2037,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 +2048,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 +2062,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"