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

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

On 2021-02-09 5:08 a.m., 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).

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.

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.

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

Simon