Re: [PATCH master+7.12 v2 2/3] Emit inferior, thread and frame selection events to all UIs

[Pedro Alves <palves@redhat.com>]

Hi Simon,

On 09/14/2016 06:45 PM, Simon Marchi wrote:
> From: Antoine Tremblay <antoine.tremblay@ericsson.com>
> 
> With this patch, when an inferior, thread or frame is explicitly
> selected by the user, notifications will appear on all CLI and MI UIs.
> When a GDB console is integrated in a front-end, this allows the
> front-end to follow a selection made by the user ont he CLI, and it
> informs the user about selection changes made behind the scenes by the
> front-end.
> 
> This patch fixes PR gdb/20487.
> 
> In order to communicate frame changes to the front-end, this patch adds
> a new field to the =thread-selected event for the selected frame.  The
> idea is that since inferior/thread/frame can be seen as a composition,
> it makes sense to send them together in the same event.  The vision
> would be to eventually send the inferior information as well, if we find
> that it's needed, although the "=thread-selected" event would be
> ill-named for that job.

Given per-inferior thread numbers, we could also say that we switch to
thread 0 of inferior INF (i.e., "INF.0").  Then it wouldn't sound that
strange, maybe.  The problem is that MI talks in terms of the global
thread id, not the per-inferior id.

Anyway, since is for machine consumption, odd naming should not
be a big deal, IMO.

> frame
> -----
> 
> 1. CLI command:
> 
>      frame 1
> 
>    MI event:
> 
>      =thread-selected,id="3",frame={level="1",...}
> 
> 2. MI command:
> 
>      -stack-select-frame 1
> 
>    CLI event:
> 
>      #1  0x00000000004007f0 in child_function...
> 

I think it's likely that experience will show that will want to tweak
what we print in the CLI in the future, along with whether
we print at all, but that's fine for now.  Making all user-selection
change handling be consistent makes sense.

> 3. MI command (CLI-in-MI):
> 
>      frame 1
> 
>    MI event/reply:
> 
>      &"frame 1\n"
>      ~"#1  0x00000000004007f9 in ..."
>      =thread-selected,id="3",frame={level="1"...}
>      ^done
> 
> inferior
> --------
> 
> Inferior selection events only go from the console to MI, since there's
> no way to select the inferior in pure MI.
> 
> 1. CLI command:
> 
>      inferior 2
> 
>    MI event:
> 
>      =thread-selected,id="3"
> 
> Note that if the user selects an inferior that is not started or exited,
> the MI doesn't receive a notification.  Since there is no threads to
> select, the =thread-selected event does not apply...

We could solve that by adding the thread group id (inferior id) to
the notification, I think:

 =thread-selected,id="3",thread-group="i1",frame="..."

 =thread-selected,id="0",thread-group="i2",frame="..."

...

If you select an inferior that is not running yet, thread 0 is what
you effectively get:

(gdb) p $_inferior
$1 = 1
(gdb) p $_thread
$2 = 1
(gdb) p $_gthread
$3 = 1
(gdb) add-inferior 
Added inferior 2
(gdb) inferior 2
[Switching to inferior 2 [<null>] (<noexec>)]
(gdb) p $_inferior
$4 = 2
(gdb) p $_thread
$5 = 0
(gdb) p $_gthread
$6 = 0
(gdb) 

> @@ -2124,9 +2137,16 @@ mi_execute_command (const char *cmd, int from_tty)
>    if (command != NULL)
>      {
>        ptid_t previous_ptid = inferior_ptid;
> +      struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
>  
>        command->token = token;
>  
> +      if (command->cmd != NULL && command->cmd->suppress_notification != NULL)
> +        {
> +          make_cleanup_restore_integer (command->cmd->suppress_notification);
> +          *command->cmd->suppress_notification = 1;
> +        }
> +
>        if (do_timings)
>  	{
>  	  command->cmd_start = XNEW (struct mi_timestamp);
> @@ -2161,10 +2181,15 @@ mi_execute_command (const char *cmd, int from_tty)
>  	  /* Don't try report anything if there are no threads --
>  	     the program is dead.  */
>  	  && thread_count () != 0
> -	  /* -thread-select explicitly changes thread. If frontend uses that
> -	     internally, we don't want to emit =thread-selected, since
> -	     =thread-selected is supposed to indicate user's intentions.  */

I'm still uneasy about this.  Do we now emit a =thread-selected
event when the frontend uses -thread-select?  Is that a deliberate change?

> -	  && strcmp (command->command, "thread-select") != 0)
> +	  /* For CLI commands "thread" and "inferior", the event is already sent
> +	     by the command, so don't send it again.  */
> +	  && ((command->op == CLI_COMMAND
> +	       && strncmp (command->command, "thread", 6) != 0
> +	       && strncmp (command->command, "inferior", 8) != 0)
> +	      || (command->op == MI_COMMAND && command->argc > 1
> +		  && strcmp (command->command, "interpreter-exec") == 0
> +		  && strncmp (command->argv[1], "thread", 6) != 0
> +		  && strncmp (command->argv[1], "inferior", 8) != 0)))


These "strncmp" calls return 0 when the command is "threadfoo"
or "inferiorfoo"  I think we need to check the next character
too, somehow?

I think it doesn't make a difference for any of the current "thread"
subcommands ("thread apply", etc.), so probably not a big deal.
(though it'd be nice to clean it up sooner than later to avoid
this getting forgotten and breaking in the future.)

But, I suspect that we end up suppressing this case:

(gdb) define thread-foo
>thread $arg0
>end
(gdb) thread-foo 2

Contrived, but certainly not hard to imagine user-commands doing
something useful along with changing the selected thread.

What happens in this case?

Thanks,
Pedro Alves