Re: [PATCH master+7.12] Send thread, frame and inferior selection events to all uis

[Simon Marchi <simon dot marchi at ericsson dot com>]

On 16-09-05 10:44 AM, Pedro Alves wrote:
> Hi Antoine,

Hi Pedro,

I am taking over for Antoine, as he went to take care of another kind of child
process.  Thanks, for the thorough review.

I'll try to send a v2 hopefully soon, in the mean time there are a few things you can
look at in this message, such as proposed procedure documentation in the test.

>> From mi: thread 1.3
>> Will generate the following mi:
>> &"thread 1.3\n"
>> ~"#0  child_sub_function () ...
>> =thread-selected,id="3",frame={level="0",...}
>> ^done
> 
> Does the later generate the event in the console too?  I assume so.

Yes:

[Switching to thread 3 (Thread 0x7ffff6ff5700 (LWP 8972))]
#0  0x00007ffff78b7dfd in nanosleep () from /lib/x86_64-linux-gnu/libc.so.6

>> Note also that this patch makes it possible to suppress notifications
>> caused by a cli command, like was done in mi-interp previously.
>>
>> This means that it's now possible to use the add_com_suppress_notification
>> function to register a command with some event suppressed like is done
>> with the select-frame command in this patch.
> 
> I should note that this suppression mechanism is broken when you
> have multiple MI uis or multiple consoles.  We end up suppressing
> the event on all uis that happen run the same top level interpreter...
> I.e., with two CLIs, do "thread" on one UI, and notice how the
> event is suppressed on the other UI.  Likewise with two MIs, etc.
> 
> TBC, it's fine with me to not address this for now.

That's right.

> This needs a manual update:
> 
> @subsubsection Threads and Frames
> 
> [...]
> it is reasonable to switch to the thread where breakpoint is hit.  For
> another example, if the user issues the CLI @samp{thread} command via
> the frontend, it is desirable to change the frontend's selected thread to the
> one specified by user.  @value{GDBN} communicates the suggestion to
> change current thread using the @samp{=thread-selected} notification.
> No such notification is available for the selected frame at the moment.
> [...]
> @item =thread-selected,id="@var{id}"
> Informs that the selected thread was changed as result of the last
> command.
> [...]

Suggested changes:

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index d1a5e7c..0c9c866 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -25807,13 +25807,13 @@ identifier for thread and frame to operate on.
 Usually, each top-level window in a frontend allows the user to select
 a thread and a frame, and remembers the user selection for further
 operations.  However, in some cases @value{GDBN} may suggest that the
-current thread be changed.  For example, when stopping on a breakpoint
-it is reasonable to switch to the thread where breakpoint is hit.  For
-another example, if the user issues the CLI @samp{thread} command via
-the frontend, it is desirable to change the frontend's selected thread to the
-one specified by user.  @value{GDBN} communicates the suggestion to
-change current thread using the @samp{=thread-selected} notification.
-No such notification is available for the selected frame at the moment.
+current thread or frame be changed.  For example, when stopping on a
+breakpoint it is reasonable to switch to the thread where breakpoint is
+hit.  For another example, if the user issues the CLI @samp{thread} or
+@samp{frame} commands via the frontend, it is desirable to change the
+frontend's selection to the one specified by user.  @value{GDBN}
+communicates the suggestion to change current thread and frame using the
+@samp{=thread-selected} notification.

 Note that historically, MI shares the selected thread with CLI, so
 frontends used the @code{-thread-select} to execute commands in the
@@ -26459,13 +26459,18 @@ A thread either was created, or has exited.  The @var{id} field
 contains the global @value{GDBN} identifier of the thread.  The @var{gid}
 field identifies the thread group this thread belongs to.

-@item =thread-selected,id="@var{id}"
-Informs that the selected thread was changed as result of the last
-command.  This notification is not emitted as result of @code{-thread-select}
-command but is emitted whenever an MI command that is not documented
-to change the selected thread actually changes it.  In particular,
-invoking, directly or indirectly (via user-defined command), the CLI
-@code{thread} command, will generate this notification.
+@item =thread-selected,id="@var{id}"[,frame="@var{frame}"]
+Informs that the selected thread or frame were changed.  This notification
+is not emitted as result of the @code{-thread-select} or
+@code{-stack-select-frame} commands, but is emitted whenever an MI command
+that is not documented to change the selected thread and frame actually
+changes them.  In particular, invoking, directly or indirectly
+(via user-defined command), the CLI @code{thread} or @code{frame} commands,
+will generate this notification.  Changing the thread of frame from another
+user interface (@xref{Interpreters}) will also generate this notification.
+
+The @var{frame} field is only present if the newly selected thread is
+stopped. See @ref{GDB/MI Frame Information} for the format of its value.

 We suggest that in response to this notification, front ends
 highlight the selected thread and cause subsequent commands to apply to


>> diff --git a/gdb/NEWS b/gdb/NEWS
>> index 6f5feb1..b427a7e 100644
>> --- a/gdb/NEWS
>> +++ b/gdb/NEWS
>> @@ -11,6 +11,10 @@
>>  
>>  *** Changes in GDB 7.12
>>  
>> +* MI =thread-selected event now includes the frame field. For example:
>> +
>> +     =thread-selected,id="3",frame={level="0",addr="0x00000000004007c0"}
>> +
> 
> 
> Please put this next to the other MI change:
> 
> * MI async record =record-started now includes the method and format used for
>   recording.  For example:
> 
>     =record-started,thread-group="i1",method="btrace",format="bts"
> 
> and I guess use "async record" too for consistency.

Done.

>> +/* Suppress notification struct.  */
>> +struct cli_suppress_notification cli_suppress_notification =
>> +  {
>> +    0   /* user_selected_inf_thread_frame */
> 
> Maybe rename this to user_selected_context?  Using "inf_thread_frame"
> will get weird once we add more "what"s.

Makes sense.  I changed all the occurrences.

I named the observer "user_selected_context_changed", to be more in line
with the other names.

>> +{
>> +  struct switch_thru_all_uis state;
>> +  struct thread_info *tp = find_thread_ptid (inferior_ptid);
>> +
>> +  /* This event is suppressed.  */
>> +  if (cli_suppress_notification.user_selected_inf_thread_frame)
>> +    return;
> 
> Move the find_thread_ptid call after this, to avoid an
> unnecessary lookup?

Done.

>> diff --git a/gdb/defs.h b/gdb/defs.h
>> index fee5f41..cb180a9 100644
>> --- a/gdb/defs.h
>> +++ b/gdb/defs.h
>> @@ -750,6 +750,21 @@ enum block_enum
>>    FIRST_LOCAL_BLOCK = 2
>>  };
>>  
>> +/* User selection used in observer.h and multiple print functions.  */
>> +
>> +enum user_selected_what_flag
>> +  {
>> +    /* Inferior selected.  */
>> +    USER_SELECTED_INFERIOR = 1 << 1,
>> +
>> +    /* Thread selected.  */
>> +    USER_SELECTED_THREAD = 1 << 2,
>> +
>> +    /* Frame selected.  */
>> +    USER_SELECTED_FRAME = 1 << 3
>> +  };
>> +DEF_ENUM_FLAGS_TYPE (enum user_selected_what_flag, user_selected_what);
>> +
> 
> This needs to #include "common/enum-flags.h".
> 
> If this didn't cause you a compile error, then it must be some other
> header is already pulling the first one in.  I got curious and checked
> what it was.  It's this sequence:
> 
> In file included from /home/pedro/gdb/mygit/src/gdb/symtab.h:26:0,
>                  from /home/pedro/gdb/mygit/src/gdb/language.h:26,
>                  from /home/pedro/gdb/mygit/src/gdb/frame.h:72,
>                  from /home/pedro/gdb/mygit/src/gdb/gdbarch.h:38,
>                  from /home/pedro/gdb/mygit/src/gdb/defs.h:652,
>                  from /home/pedro/gdb/mygit/src/gdb/gdb.c:19:
> /home/pedro/gdb/mygit/src/gdb/common/enum-flags.h:1:2: error: #error "enum-flags.h" included
> 
> In any case, we should #include what we depend on directly
> instead of relying on some accidental indirect dependency.

Agreed, done.

>> diff --git a/gdb/doc/observer.texi b/gdb/doc/observer.texi
>> index fc7aac4..4f8f0bb 100644
>> --- a/gdb/doc/observer.texi
>> +++ b/gdb/doc/observer.texi
>> @@ -307,3 +307,7 @@ This observer is used for internal testing.  Do not use.
>>  See testsuite/gdb.gdb/observer.exp.
>>  @end deftypefun
>>  
>> +@deftypefun void user_selected_inf_thread_frame (user_selected_what @var{selection})
>> +The user-selected inferior,thread and/or frame has changed.  The user_select_what
>> +flag specifies if the inferior, thread or frame has changed.
> 
> Missing space in "inferior,thread".

Done.

>> +/* See inferior.h.  */
>> +
>> +void
>> +print_selected_inferior (struct ui_out *uiout)
>> +{
>> +  char buf[PATH_MAX+256];
> 
> Missing spaces.

Done.

>> +static void
>> +mi_user_selected_inf_thread_frame (user_selected_what selection)
>> +{
>> +  struct switch_thru_all_uis state;
>> +  struct thread_info *tp = find_thread_ptid (inferior_ptid);
>> +
>> +  /* Don't send an event if we're responding to an MI command.  */
>> +  if (mi_suppress_notification.user_selected_inf_thread_frame)
>> +    return;
> 
> Same comment about moving find_thread_ptid.

Done.

>> +
>> +  SWITCH_THRU_ALL_UIS (state)
>> +  {
>> +    struct mi_interp *mi = as_mi_interp (top_level_interpreter ());
>> +    struct ui_out *mi_uiout;
>> +    struct cleanup *old_chain;
>> +
>> +    if (mi == NULL)
>> +      continue;
>> +
>> +    mi_uiout = interp_ui_out (top_level_interpreter ());
>> +
>> +    ui_out_redirect (mi_uiout, mi->event_channel);
>> +
>> +    old_chain = make_cleanup_restore_target_terminal ();
>> +    target_terminal_ours_for_output ();
>> +
>> +    if (selection & USER_SELECTED_INFERIOR)
>> +      print_selected_inferior (mi->cli_uiout);
>> +
>> +    if (tp != NULL
>> +	&& ((selection & (USER_SELECTED_THREAD | USER_SELECTED_FRAME))))
> 
> One level of parens too many?

Done.

>> +      {
>> +	print_selected_thread_frame (mi->cli_uiout, selection);
>> +
>> +	fprintf_unfiltered (mi->event_channel,
>> +			    "thread-selected,id=\"%d\"",
>> +			    tp->global_num);
>> +
>> +	if (tp->state != THREAD_RUNNING)
>> +	  {
>> +	    if (has_stack_frames ())
>> +	      print_stack_frame_to_uiout (mi_uiout, get_selected_frame (NULL),
>> +					  1, SRC_AND_LOC, 1);
>> +	  }
>> +      }
>> +
>> +    ui_out_redirect (mi_uiout, NULL);
> 
> Use make_cleanup_ui_out_redirect_pop instead.

Like this?

@@ -1360,14 +1362,16 @@ mi_user_selected_inf_thread_frame (user_selected_what selection)

     ui_out_redirect (mi_uiout, mi->event_channel);

-    old_chain = make_cleanup_restore_target_terminal ();
+    old_chain = make_cleanup_ui_out_redirect_pop (mi_uiout);
+
+    make_cleanup_restore_target_terminal ();
     target_terminal_ours_for_output ();

     if (selection & USER_SELECTED_INFERIOR)
       print_selected_inferior (mi->cli_uiout);

>> @@ -2102,6 +2106,7 @@ mi_execute_command (const char *cmd, int from_tty)
>>  {
>>    char *token;
>>    struct mi_parse *command = NULL;
>> +  struct cleanup *cleanup = NULL;
> 
> Move this to the scope that needs it.

Done.

>>  
>>    /* This is to handle EOF (^D). We just quit gdb.  */
>>    /* FIXME: we should call some API function here.  */
>> @@ -2161,10 +2166,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.  */
>> -	  && strcmp (command->command, "thread-select") != 0)
>> +	  /* For the cli commands thread and inferior, the event is already sent
>> +	     by the command, 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)
>> +	      || (command->op == MI_COMMAND && command->argc > 1
>> +		  && strncmp (command->argv[1], "thread", 6) != 0
>> +		  && strncmp (command->argv[1], "inferior", 8) != 0)))
> 
> Urgh.  :-/  I wish we'd find some better way...
> 
> - This is matching "threadfoo", "inferiorfoo" as well.
> 
> - This is matching the "thread" subcommands too.  Probably OK.
> 
> - Does this match command aliases?  (alias foo=thread)
> 
> - The suppression used to be for "thread-select" only, now
>   it's done for all MI commands.  Not sure whether this was intended
>   or not.  IOW, this needs an explicit rationale/comments.

Are you talking about this line?

  (command->op == MI_COMMAND && command->argc <= 1)

I am not sure what it's for...

> - Is the argc checking meant to handle "-interpreter-exec"?
>   Shouldn't there be an explicit check for that?

It shouldn't be too hard to do.

> I guess Simon's work on decoupling user-selected state a
> little better might help clean this up.
> But meanwhile, we need to make sure to err on the
> "emit notification" side and the latter two points above worry me.

Agreed, I think it's going to help clean up a few places.  Until then,
I don't see a better solution than what Antoine did.

>>  	{
>>  	  struct mi_interp *mi
>>  	    = (struct mi_interp *) top_level_interpreter_data ();
>> @@ -2185,18 +2195,21 @@ mi_execute_command (const char *cmd, int from_tty)
>>  
>>  	  if (report_change)
>>  	    {
>> -	      struct thread_info *ti = inferior_thread ();
>> -	      struct cleanup *old_chain;
>> -
>> -	      old_chain = make_cleanup_restore_target_terminal ();
>> -	      target_terminal_ours_for_output ();
>> -
>> -	      fprintf_unfiltered (mi->event_channel,
>> -				  "thread-selected,id=\"%d\"",
>> -				  ti->global_num);
>> -	      gdb_flush (mi->event_channel);
>> -
>> -	      do_cleanups (old_chain);
>> +	      /* Make sure we still keep event suppression.  This is
>> +		 handled in mi_cmd_execute so at this point this has been
>> +		 reset.  We still need it here however.  */
> 
> This raises the question of why is the event suppression done in
> mi_cmd_execute instead of in this function, before calling
> mi_cmd_execute?

I moved the handling of event suppression to this function, it
seems to work fine.

>> +	        if (command->cmd->suppress_notification != NULL)
>> +		  {
>> +		    cleanup = make_cleanup_restore_integer
>> +		      (command->cmd->suppress_notification);
>> +		    *command->cmd->suppress_notification = 1;
>> +		  }
>> +
>> +		observer_notify_user_selected_inf_thread_frame
>> +		  (USER_SELECTED_THREAD | USER_SELECTED_FRAME);
>> +
>> +		if (cleanup != NULL)
>> +		  do_cleanups (cleanup);
>>  	    }
>>  	}
>>  
>> diff --git a/gdb/mi/mi-main.h b/gdb/mi/mi-main.h
>> index 18000cf..f5b6bcf 100644
>> --- a/gdb/mi/mi-main.h
>> +++ b/gdb/mi/mi-main.h
>> @@ -49,6 +49,8 @@ struct mi_suppress_notification
>>    int traceframe;
>>    /* Memory changed notification suppressed?  */
>>    int memory;
>> +  /* Inferior, thread, frame selected notification suppressed?  */
>> +  int user_selected_inf_thread_frame;
>>  };
>>  extern struct mi_suppress_notification mi_suppress_notification;
>>  
>> diff --git a/gdb/stack.c b/gdb/stack.c
>> index 417e887..80ce00b 100644
>> --- a/gdb/stack.c
>> +++ b/gdb/stack.c
>> @@ -51,6 +51,7 @@
>>  #include "safe-ctype.h"
>>  #include "symfile.h"
>>  #include "extension.h"
>> +#include "observer.h"
>>  
>>  /* The possible choices of "set print frame-arguments", and the value
>>     of this setting.  */
>> @@ -141,6 +142,22 @@ frame_show_address (struct frame_info *frame,
>>    return get_frame_pc (frame) != sal.pc;
>>  }
>>  
>> +/* See frame.h  */
>> +
>> +void
>> +print_stack_frame_to_uiout (struct ui_out *uiout, struct frame_info *frame,
>> +			    int print_level, enum print_what print_what,
>> +			    int set_current_sal)
>> +{
>> +  struct ui_out *saved_uiout;
>> +  saved_uiout = current_uiout;
>> +  current_uiout = uiout;
>> +
>> +  print_stack_frame (frame, print_level, print_what, set_current_sal);
>> +
>> +  current_uiout = saved_uiout;
> 
> Use a cleanup.

Done, I factored it out from infrun.c:print_stop_event and will post it in a
preparatory patch.

>> diff --git a/gdb/testsuite/gdb.mi/mi-pthreads.exp b/gdb/testsuite/gdb.mi/mi-pthreads.exp
>> index 88a600a..a49856e 100644
>> --- a/gdb/testsuite/gdb.mi/mi-pthreads.exp
>> +++ b/gdb/testsuite/gdb.mi/mi-pthreads.exp
>> @@ -53,7 +53,7 @@ proc check_mi_thread_command_set {} {
>>  
>>    foreach thread $thread_list {
>>        mi_gdb_test "-interpreter-exec console \"thread $thread\"" \
>> -          ".*\\^done\r\n=thread-selected,id=\"$thread\"" \
>> +	  ".*=thread-selected,id=\"$thread\".*\[r\n\]+\\^done\[\r\n\]+" \
>>            "check =thread-selected: thread $thread"
> 
> Not sure I'm reading this right, but I think this just means that
> the notification is now sent before the ^done.  Is that right?
> Probably not a problem, just checking.

That would make sense.  The =thread-selected event used to be emitted after the command
complete, when GDB realized that currently selected thread != previously selected thread.
Now, it's done during the execution of the command, so before the ^done.

>>    }
>>  }
>> diff --git a/gdb/testsuite/gdb.mi/thread-selected-sync.c b/gdb/testsuite/gdb.mi/thread-selected-sync.c
>> new file mode 100644
>> index 0000000..4113d26
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.mi/thread-selected-sync.c
> 
> Maybe rename this to user-selected-context-sync.c ?

Makes sense, done.  Should it be mi-user-selected-context-sync.{c,exp} though, since
all the test cases in gdb.mi start with mi-?  By the way, why do they all start with
mi-, when they are already in the "gdb.mi" directory?

>> @@ -0,0 +1,64 @@
>> +/* This testcase is part of GDB, the GNU debugger.
>> +
>> +   Copyright 2016 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/>.
>> +
>> +*/
>> +
>> +/* Note that this test is not expected to exit cleanly.  All threads will
>> +   block at the barrier and they won't be waken up. */
> 
> Please add an alarm call then:
> 
>  https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Don.27t_write_tests_that_run_forever

Instead, I made the main thread call sleep (30), and then call
pthread_barrier_wait, which unlocks the other threads and allows
the program to end.

>> +void
>> +child_sub_function ()
> 
> (void)

Done.

>> +# It also considers the case were the console commands are sent directly in
>> +# the mi channel as described in PR 20487.
> 
> s/were/where/

Done.

>> +#
>> +# It does so by starting 2 inferiors with 3 threads each.
>> +# Thread 1 is the main thread starting the others.
>> +# Thread 2 is stopped in non-stop mode.
>> +# Thread 3 is the thread used for testing, in non-stop mode this thread is running.
>> +
>> +load_lib mi-support.exp
>> +
>> +standard_testfile
>> +
>> +# Multiple inferiors are needed, therefore only native gdb and extended
>> +# gdbserver modes are supported.
> 
> Could we restrict this to the specific tests that need multiple inferiors?
> I think you'd just need to guard the creation of the second inferior
> in test_setup, and then make test_*_select_inferior bail out with
> unsupported or some such.

Probably.  I just tried to do that, and here are a few things to consider:

- With a single inferior, thread numbers are only TH, instead of INF.TH, so many functions need to take that into account.
- By skipping some *_inferior tests, the user selection is left a different state for the next procedure.  For example:

  * thread 1.3 is currently selected
  * test_inferior switches inferior, making thread 2.1 the selected one
  * test_thread switches to thread 1.3 and expects an event

If we skip test_inferior, there won't be an event when test_thread tries to switch
to thread 1.3, as it will already be selected.  That kind of dependency between test
procedures should be avoided as much as possible I think, as it will make the test
very hard to modify (as we see now).  I think that we could add some kind of
"reset_selection" procedure that takes care of restoring a known, stable selection
at the beginning of each test procedure.


>> +if [use_gdb_stub] {
>> +    untested ${testfile}.exp
>> +    return
>> +}
>> +
>> +set compile_options "debug pthreads"
>> +if {[build_executable $testfile.exp $testfile ${srcfile} ${compile_options}] == -1} {
>> +    untested "failed to compile $testfile"
>> +    return -1
>> +}
>> +
>> +set bp_lineno [gdb_get_line_number "set break here"]
>> +set caller_lineno [gdb_get_line_number "caller"]
>> +
>> +# Make a regular expression for cli.
> 
> It'd be nice to extend this comment.  What does the
> regular expression match?  What are the parameters for?

What about:


# Make a regular expression to match the "thread selected" event for CLI.
#
# MODE can be either "all-stop" or "non-stop", indicating which one is currently
#   in use.
# INF is the inferior number we are expecting GDB to switch to, or -1 if we are
#   not expecting GDB to announce an inferior switch.
# THREAD is the thread number we are expecting GDB to switch to, or -1 if we are
#   not expecting GDB to announce a thread switch.
# FRAME is the frame number we are expecting GDB to switch to, or -1 if we are
#   not expecting GDB to announce a frame switch.  See the FRAME_RE variable for
#   details.
# If LINEEND is 1, expect and end of line at the end of the regular expression.

>> +
>> +proc make_cli_re {mode inf thread frame lineend} {
>> +    global srcfile bp_lineno caller_lineno
>> +
>> +    set inf_re "\\\[Switching to inferior $inf.*\\\]"
>> +    set all_stop_thread_re "\\\[Switching to thread $thread.*\\\]"
>> +    set non_stop_thread_re "$all_stop_thread_re\\\(running\\\)"
>> +    set frame_re(0) "#0.*child_sub_function.*$srcfile:$bp_lineno\[\r\n\]+.*set break here \\\*/"
>> +    set frame_re(1) "#1.*child_function \\\(args=0x0\\\) at .*$srcfile:$caller_lineno\[\r\n\]+$caller_lineno.*caller \\\*/"
>> +    # Special frame for main threads.
>> +    set frame_re(2) "#0.*"
>> +    set end "\[\r\n\]"
>> +
>> +    set cli_re ""
>> +
>> +    if {$inf != -1} {
>> +	append cli_re $inf_re
>> +    }
>> +
>> +    if {$thread != "-1"} {
>> +	if {$inf != -1} {
>> +	    append cli_re "\[\r\n\]+"
>> +	}
>> +	set thread_re ""
>> +	if {$mode == "all-stop"} {
>> +	    set thread_re $all_stop_thread_re
>> +	} elseif {$mode == "non-stop"} {
>> +	    set thread_re $non_stop_thread_re
>> +	}
>> +	append cli_re $thread_re
>> +    }
>> +
>> +    if {$mode == "all-stop" && $frame != -1} {
>> +	if {$thread != -1} {
>> +	    append cli_re "\[\r\n\]+"
>> +	}
>> +	append cli_re $frame_re($frame)
>> +    }
>> +
>> +    if {$lineend == 1} {
>> +	append cli_re $end
>> +    }
>> +
>> +    return $cli_re
>> +}
>> +
>> +# Make a regular expression for mi.
>> +
> 
> Ditto.

Essentially the same, but without the INF parameter:

# Make a regular expression to match the "thread selected" event for MI.
#
# MODE can be either "all-stop" or "non-stop", indicating which one is currently
#   in use.
# THREAD is the thread number we are expecting GDB to switch to, or -1 if we are
#   not expecting GDB to announce a thread switch.
# FRAME is the frame number we are expecting GDB to switch to, or -1 if we are
#   not expecting GDB to announce a frame switch.  See the FRAME_RE variable for
#   details.
# If LINEEND is 1, expect and end of line at the end of the regular expression.

>> +proc make_mi_re {mode thread event frame lineend} {
>> +    global srcfile bp_lineno caller_lineno hex
>> +
>> +    set mi_re ""
>> +    set thread_event_re "=thread-selected,id=\"$thread\""
>> +    set thread_answer_re ".*\\^done,new-thread-id=\"$thread\""
>> +    set frame_re(0) ",frame=\{level=\"0\",addr=\"$hex\",func=\"child_sub_function\",args=\\\[\\\],file=\".*$srcfile\",.*line=\"$bp_lineno\"\}"
>> +    set frame_re(1) ",frame=\{level=\"1\",addr=\"$hex\",func=\"child_function\",args=\\\[\{name=\"args\",value=\"0x0\"\}\\\],file=\".*$srcfile\",.*line=\"$caller_lineno\"\}"
>> +    #Special frame for main thread.
>> +    set frame_re(2) ",frame=\{level=\"0\",addr=\"$hex\",func=\".*\",args=\\\[\\\],from=\".*\"\}"
>> +    set end "\[\r\n\]"
>> +
>> +    if {$thread != "-1"} {
>> +	if {$event == 1} {
>> +	    append mi_re $thread_event_re
>> +	} elseif {$event == 0} {
>> +	    append mi_re $thread_answer_re
>> +	}
>> +    }
>> +
>> +    if {$mode == "all-stop" && $frame != -1} {
>> +	append mi_re $frame_re($frame)
>> +    }
>> +
>> +    if {$lineend == 1} {
>> +	append mi_re $end
>> +    }
>> +
>> +    return $mi_re
>> +}
>> +
>> +# Make a regular expression for cli in mi.

And here too:

# Make a regular expression to match the "thread selected" event for CLI-in-MI.
#
# COMMAND is the CLI command that was sent to GDB, which will be output in the
#   console output stream.
# MODE can be either "all-stop" or "non-stop", indicating which one is currently
#   in use.
# INF is the inferior number we are expecting GDB to switch to, or -1 if we are
#   not expecting GDB to announce an inferior switch.
# CLI_THREAD is the thread number as seen in the CLI (inferior-qualified) we are
#   expecting GDB to switch to, or -1 if we are not expecting GDB to announce a
#   thread switch.
# MI_THREAD is the thread number as seen in the MI (global number) we are
#   expecting GDB to switch to, or -1 if we are not expecting GDB to announce a
#   thread switch.
# FRAME is the frame number we are expecting GDB to switch to, or -1 if we are
#   not expecting GDB to announce a frame switch.  See the FRAME_RE variable for
#   details.
# If LINEEND is 1, expect and end of line at the end of the regular expression.

>> +proc make_cli_in_mi_re {command mode inf cli_thread mi_thread event frame lineend} {

...

>> +# Continue to the breakpoint indicating the start position of the threads.
>> +
>> +proc test_continue_to_start {mode inf} {
>> +    global gdb_prompt mi_spawn_id
>> +
>> +    if {$mode == "all-stop"} {
>> +	for {set i 1} { $i <= 2 } { incr i } {
>> +	    gdb_continue_to_breakpoint "barrier breakpoint"
>> +	}
>> +    } else {
>> +	set test "continue&"
>> +
>> +	gdb_test_multiple $test $test {
>> +	    -re "Continuing\\.\[\r\n\]+$gdb_prompt " {
>> +		pass $test
>> +	    }
>> +	}
>> +
>> +	# Wait until we've hit the breakpoint for the 2 threads.
>> +	for {set i 1} { $i <= 2 } { incr i } {
>> +	    set test "thread $i started"
>> +	    gdb_test_multiple "" $test {
>> +		-re "hit Breakpoint" {
>> +		    # The prompt was already matched in the "continue &"
>> +		    # test above.  We're now consuming asynchronous output
>> +		    # that comes after the prompt.
>> +		    pass $test
>> +		}
>> +	    }
>> +	}
>> +	# Switch to the test thread.
>> +	if {$inf == 1} {
>> +	    gdb_test "thread 3" [make_cli_re "all-stop" -1 "3" 0 -1] "Switch to test thread"
>> +	} else {
>> +	    gdb_test "thread 2.3" [make_cli_re "all-stop" -1 "2\\.3" 0 -1] "Switch to test thread"
>
> How came these pass "all-stop", when you're in the "non-stop" if/then branch?

That's odd.  When I switch them to non-stop it fails.  I'll look into it.

>> +proc test_setup {mode} {
>> +    global srcfile srcdir subdir testfile
>> +    global gdb_main_spawn_id mi_spawn_id
>> +    global decimal binfile bp_lineno
>> +    global GDBFLAGS
>> +
>> +    mi_gdb_exit
>> +
>> +    set saved_gdbflags $GDBFLAGS
>> +
>> +    if {$mode == "non-stop"} {
>> +	set GDBFLAGS [concat $GDBFLAGS " -ex \"set non-stop 1\""]
>> +    }
>> +
>> +    if {[mi_gdb_start "separate-mi-tty"] != 0} {
>> +	return
>> +    }
>> +
>> +    mi_delete_breakpoints
>> +    mi_gdb_reinitialize_dir $srcdir/$subdir
>> +    mi_gdb_load $binfile
>> +
>> +    if {[mi_runto main] < 0} {
>> +	fail "Can't run to main"
>> +	return
>> +    }
>> +
>> +    with_spawn_id $gdb_main_spawn_id {
>> +
>> +	gdb_test "break $srcfile:$bp_lineno" \
>> +	    "Breakpoint $decimal .*$srcfile, line $bp_lineno\\\." \
>> +	    "set breakpoint"
>> +
>> +	test_continue_to_start $mode 1
>> +
>> +	# Add a second inferior to test inferior selection.
>> +	gdb_test "add-inferior" "Added inferior 2" "Add inferior 2"
>> +	gdb_test "inferior 2" [make_cli_re $mode 2 "-1" -1 -1]
>> +	gdb_load ${binfile}
>> +	gdb_test "start" "Temporary breakpoint.*Starting program.*"
>> +	test_continue_to_start $mode 2
>> +	gdb_test "inferior 1" [make_cli_re $mode 1 "1\\.1" 2 -1]
>> +    }
>> +
>> +    set GDBFLAGS $saved_gdbflags
> 
> There are early returns before you reach here.
> Use 'save_vars { GDBFLAGS } { ... }' around the mi_gdb_start
> call instead.

Ok.  if there is a return in the save_vars, will it return from the test
setup as we expect?  For example:

    save_vars { GDBFLAGS } {
	if {$mode == "non-stop"} {
	    set GDBFLAGS [concat $GDBFLAGS " -ex \"set non-stop 1\""]
	}

	if {[mi_gdb_start "separate-mi-tty"] != 0} {
	    return
	}	
    }

Otherwise, when using MI, couldn't we just set non-stop after having started GDB?
mi_gdb_start only starts GDB, but doesn't connect to anything, so it wouldn't be
too late to send a "set non-stop 1" after.

>> +proc test_cli_select_thread_twice {mode} {
>> +    global gdb_main_spawn_id mi_spawn_id
>> +
>> +    set mi_re [make_mi_re $mode "3" 1 0 1]
>> +    set cli_re [make_cli_re $mode -1 "1\\.3" 0 0]
>> +
>> +    with_spawn_id $gdb_main_spawn_id {
>> +	gdb_test "thread 1.3" $cli_re "cli select thread twice, first call"
>> +    }
>> +
>> +    with_spawn_id $mi_spawn_id {
>> +	set test "mi select thread twice, first call"
>> +	gdb_test_multiple "" $test {
>> +	    -re $mi_re {
>> +		pass $test
>> +	    }
>> +	}
>> +    }
>> +
>> +    # No event for that one.
>> +    set mi_re ""
>> +    set cli_re [make_cli_re $mode -1 "1\\.3" 0 0]
>> +
>> +    with_spawn_id $gdb_main_spawn_id {
>> +	gdb_test "thread 1.3" $cli_re "cli select thread twice, second call"
>> +    }
>> +
>> +    with_spawn_id $mi_spawn_id {
>> +	set test "mi select thread twice, second call"
>> +	gdb_test_multiple "" $test {
>> +	    -re $mi_re {
> 
> "$mi_re" is empty here, so this doesn't look it's actually
> testing anything?  Several other instances in other tests.
> 
> See ensure_no_output in new-ui.exp for an idea of how to
> check that no output has been sent.
> 
> I don't think we should be using gdb_test_multiple on MI.
> It's mostly useless, compared to gdb_expect, since
> its internal matchings expect CLI's "$gdb_prompt $", which
> won't match in MI.  Ideally we'd refactor out something
> similar to gdb_test_multiple out of mi_gdb_test (and then
> even try to factor out common bits between the result and
> gdb_test_multiple), but nobody's ever bothered.  So you
> get to use gdb_expect directly.

I'll look into it, along with the other comments about the test.

>> +		pass $test
>> +	    }
>> +	}
>> +    }
>> +
>> +}
>> +
>> +# Test frame selection from cli.
>> +
>> +proc test_cli_select_frame {mode frame} {
>> +    global gdb_main_spawn_id mi_spawn_id
>> +
>> +    if {$mode == "all-stop"} {
>> +	set mi_re [make_mi_re $mode "3" 1 $frame 1]
>> +	set cli_re [make_cli_re $mode -1 "-1" $frame 0]
>> +    } elseif {$mode == "non-stop"} {
>> +	set cli_re "Selected thread is running\\."
>> +	# No output
>> +	set mi_re ""
>> +    }
>> +
>> +    with_spawn_id $gdb_main_spawn_id {
>> +	gdb_test "frame $frame" $cli_re "cli select frame"
>> +    }
>> +
>> +    with_spawn_id $mi_spawn_id {
>> +	set test "mi select frame"
>> +	gdb_test_multiple "" $test {
>> +	    -re $mi_re {
>> +		pass $test
>> +	    }
>> +	}
>> +    }
>> +}
>> +
>> +# Test frame selection from cli with the select-frame command.
>> +
>> +proc test_cli_select_select_frame {mode frame} {
>> +    global gdb_main_spawn_id mi_spawn_id
>> +
>> +    if {$mode == "all-stop"} {
>> +	set cli_re ""
>> +	set mi_re [make_mi_re $mode "3" 1 $frame 1]
>> +    } elseif {$mode == "non-stop"} {
>> +	set cli_re "Selected thread is running\\."
>> +	# No output
>> +	set mi_re ""
>> +    }
>> +
>> +    with_spawn_id $gdb_main_spawn_id {
>> +	if {$cli_re != ""} {
>> +	    gdb_test "select-frame $frame" $cli_re "cli select frame with select-frame"
>> +	} else {
>> +	    gdb_test_no_output "select-frame $frame" $cli_re "cli select frame with select-frame"
>> +	}
>> +    }
>> +
>> +    with_spawn_id $mi_spawn_id {
>> +	set test "mi select frame with select-frame"
>> +	gdb_test_multiple "" $test {
>> +	    -re $mi_re {
>> +		pass $test
>> +	    }
>> +	}
>> +    }
>> +}
>> +
>> +# Test doing and up and then down command from cli.
> 
> "an up", I guess.

Done.

>> +
>> +proc test_cli_up_down {mode} {
>> +    global gdb_main_spawn_id mi_spawn_id
>> +
>> +    if {$mode == "all-stop"} {
>> +	set cli_up_re [make_cli_re $mode -1 "-1" 1 0]
>> +	set mi_up_re [make_mi_re $mode "3" 1 1 1]
>> +	set cli_down_re [make_cli_re $mode -1 "-1" 0 0]
>> +	set mi_down_re [make_mi_re $mode "3" 1 0 1]
>> +    } elseif {$mode == "non-stop"} {
>> +	set cli_up_re "No stack\\."
>> +	set mi_up_re ""
>> +	set cli_down_re "No stack\\."
>> +	set mi_down_re ""
>> +    }
>> +
>> +    with_spawn_id $gdb_main_spawn_id {
>> +	gdb_test "up" $cli_up_re "cli up"
>> +    }
>> +
>> +    with_spawn_id $mi_spawn_id {
>> +	set test "mi up"
>> +	gdb_test_multiple "" $test {
>> +	    -re $mi_up_re {
>> +		pass $test
>> +	    }
>> +	}
>> +    }
>> +
>> +    with_spawn_id $gdb_main_spawn_id {
>> +	gdb_test "down" $cli_down_re "cli down"
>> +    }
>> +
>> +    with_spawn_id $mi_spawn_id {
>> +	set test "mi down"
>> +	gdb_test_multiple "" $test {
>> +	    -re $mi_down_re {
>> +		pass $test
>> +	    }
>> +	}
>> +    }
>> +}
>> +
>> +# Test same frame selection from cli.
>> +
>> +proc test_cli_select_frame_twice {mode frame} {
>> +    global gdb_main_spawn_id mi_spawn_id
>> +
>> +    if {$mode == "all-stop"} {
>> +	set mi_re [make_mi_re $mode "3" 1 $frame 1]
>> +	set cli_re [make_cli_re $mode -1 "-1" $frame 0]
>> +    } elseif {$mode == "non-stop"} {
>> +	set cli_re "Selected thread is running\\."
>> +	# No output
>> +	set mi_re ""
>> +    }
>> +
>> +    with_spawn_id $gdb_main_spawn_id {
>> +	gdb_test "frame $frame" $cli_re "cli select frame twice, first call"
>> +    }
>> +
>> +    with_spawn_id $mi_spawn_id {
>> +	set test "mi select frame twice, first call"
>> +	gdb_test_multiple "" $test {
>> +	    -re $mi_re {
>> +		pass $test
>> +	    }
>> +	}
>> +    }
>> +
>> +    if {$mode == "all-stop"} {
>> +	#No output thread has not changed.
> 
> Did you mean to put a period or something else after "No output"?
> Reads strange as is.

Changed to:

  # No output, thread has not changed.

>> +	set mi_re ""
>> +	set cli_re [make_cli_re $mode -1 "-1" $frame 0]
>> +    } elseif {$mode == "non-stop"} {
>> +	set cli_re "Selected thread is running\\."
>> +	# No output
>> +	set mi_re ""
>> +    }
>> +
>> +    with_spawn_id $gdb_main_spawn_id {
>> +	gdb_test "frame $frame" $cli_re "cli select frame twice, second call"
>> +    }
>> +
>> +    with_spawn_id $mi_spawn_id {
>> +	set test "mi select frame, second call"
>> +	gdb_test_multiple "" $test {
>> +	    -re $mi_re {
>> +		pass $test
>> +	    }
>> +	}
>> +    }
>> +}
>> +
>> +# Test thread command without any arguments fromt the cli.
> 
> "from".

Done.

>> +    with_spawn_id $mi_spawn_id {
>> +	mi_gdb_test "-stack-select-frame $frame" $mi_re "mi select frame twice, frist call"
> 
> 
> "first"

Done.

>> +    }
>> +
>> +    with_spawn_id $gdb_main_spawn_id {
>> +	set test "cli select frame twice, frist call"
> 
> 
> "first"

Done.

>> +  /* Print if the thread has not changed, otherwise an event will be sent.  */
>> +  if (ptid_equal (inferior_ptid, previous_ptid))
>> +    {
>> +      print_selected_thread_frame (current_uiout, USER_SELECTED_THREAD
>> +				   | USER_SELECTED_FRAME);
> 
> The "|" should be aligned under "USER_", and you'd need an extra set of
> parens to make emacs happy:
> 
>       print_selected_thread_frame (current_uiout, (USER_SELECTED_THREAD
> 						   | USER_SELECTED_FRAME));
> 
> but in this case it's more readable IMO to put the USER_SELECTED_THREAD
> on the next line too:
> 
>       print_selected_thread_frame (current_uiout,
> 				   USER_SELECTED_THREAD | USER_SELECTED_FRAME);


Done.

>> +/* Observer for the user_selected_thread_frame notification.  */
>> +
>> +static void
>> +tui_on_user_selected_inf_thread_frame (user_selected_what selection)
>> +{
>> +  struct switch_thru_all_uis state;
>> +  struct thread_info *tp = find_thread_ptid (inferior_ptid);
> 
> Same comment as in CLI/MI.

Done.

>> +
>> +  /* This event is suppressed.  */
>> +  if (cli_suppress_notification.user_selected_inf_thread_frame)
>> +    return;
>> +
> 
> I tried running the new test here, and got:
> 
> Running /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.mi/thread-selected-sync.exp ...
> FAIL: gdb.mi/thread-selected-sync.exp: all-stop: from cli: mi select inferior (timeout)
> FAIL: gdb.mi/thread-selected-sync.exp: all-stop: from cli: mi select inferior (timeout)
> FAIL: gdb.mi/thread-selected-sync.exp: all-stop: from mi: mi select inferior (timeout)
> FAIL: gdb.mi/thread-selected-sync.exp: all-stop: from mi: mi select inferior (timeout)
> FAIL: gdb.mi/thread-selected-sync.exp: all-stop: from cli in mi: mi select inferior
> FAIL: gdb.mi/thread-selected-sync.exp: all-stop: from cli in mi: mi select inferior
> FAIL: gdb.mi/thread-selected-sync.exp: all-stop: interpreter-exec: mi select inferior
> FAIL: gdb.mi/thread-selected-sync.exp: all-stop: interpreter-exec: mi select inferior
> 
> gdb.log shows, for the first FAIL above:
> 
> [...]
> ~"[Switching to inferior 2 [process 10838] (/home/pedro/brno/pedro/gdb/mygit/build/gdb/testsuite/outputs/gdb.mi/thread-selected-sync/thread-selected-sync)]\n"
> ~"[Switching to thread 2.1 (Thread 0x7ffff7fc7700 (LWP 10838))]\n"
> ~"#0  0x00007ffff7bc66bd in pthread_join (threadid=140737342580480, thread_return=0x0) at pthread_join.c:90\n"
> ~"90\t    lll_wait_tid (pd->tid);\n"
> =thread-selected,id="4",frame={level="0",addr="0x00007ffff7bc66bd",func="pthread_join",args=[{name="threadid",value="140737342580480"},{name="thread_return",value="0x0"}],file="pthread_join.c",fullname="/usr/src/debug/glibc-2.22/nptl/pthread_join.c",line="90"}
> FAIL: gdb.mi/thread-selected-sync.exp: all-stop: from cli: mi select inferior (timeout)
> [...]
> 
> 
> Try to check with "make check-read1" too.

I'll look into it.

> I also notice that the test has many duplicated test messages:
>  https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique

I'll look into it.

Thanks,

Simon