Decouple user selection from internal selectionusers/simark/user-selection-rfc

I am sending this as an RFC because it's far from complete and
definitive, but I'd like to gather some comments and opinions before
going further in this direction.

The goal of this patch is to decouple the notion of the user-selected
inferior/thread/frame from GDB's internally selected
inferior/thread/frame.

Currently, for example, the inferior_ptid variable has two jobs:

 - it's the user-selected thread: it's changed by the "thread" command.
   Other commands (continue, backtrace, etc) apply to this thread.
 - it's the internally-selected thread: it defines the thread GDB is
   currently "working" on.  For example, implementations of
   to_xfer_partial will refer to it to know from which thread to
   read/write memory.

Because of this dual usage, if we want to do some operations on a thread
other than the currently selected one, we have to save the current
inferior/thread/frame and restore them when we're done.  Failing to do
so would result in an unexpected selection switch for the user.

To improve this, Pedro suggested in [1] to decouple the two concepts.  This
is essentially what this patch is trying to do.

A new "user_selection" object is introduced, which contains the selected
inferior/thread/frame from the point of view of the user.  Before every
command, we "apply" this selection to the core of GDB to make sure the
internal selection matches the user selection.

There is a single user selection for the whole GDB (named "global
user-selection"), but as was mentioned in the linked thread, it opens
the door to having different selections for different UIs.  This means
that each UI would have its own user-selection object, which would be
applied to the core prior to executing commands from this UI.

The global user-selection object only gets modified when we really
intend to change it.  It can be because of the thread / -thread-select /
up / down / frame / inferior commands, a breakpoint hit in all-stop, an
inferior exit, etc.

The problem that initially prompted this effort is that the "--thread"
flag of MI commands changes the user-selected thread under the user's
feet.  My initial attempt to fix it was to restore the selection after
the MI command execution.  However, some cases are hard to get right.
For example:

  (thread 1 is currently selected)
  -interpreter-exec --thread 2 console "thread 3"

Restoring the selected thread to thread 1 after the MI command execution
wrongfully cancels the switch to thread 3.  So it's hard to determine
when we should or shouldn't restore.   With the current patch, it works
naturally: the --thread flag doesn't touch the user-selected thread,
only the internal one.  The "thread 3" command updates the user
selection.

Another difficulty is to send the right notifications to MI when the
user selection changes.  That means to not miss any, but not send too
many either.  Getting it somewhat right lead to ugly hacks (see the
command_notifies_uscc_observer function) and even then it's not perfect
(see the kfails in user-selected-context-sync.exp test).  With the
proposed method, it's easy to know when the user-selection changes and
send notifications.

With this patch, there are probably a few usage of
make_cleanup_restore_current_thread that are not needed anymore, if they
are only used to restore the user selection.  I kept removing them for a
later time though.

In the current state, there are a few minor regressions in the testsuite
(especially some follow-fork stuff I'm not sure how to handle), but the
vast majority of the previously passing tests still pass.

Comments are welcome!

Thanks,

Simon

[1] https://sourceware.org/ml/gdb-patches/2016-08/msg00031.html

1 files changed, 53 insertions, 32 deletions

diff --git a/gdb/stack.c b/gdb/stack.c
index aa3a80e9a82..26fb40dbec7 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -52,6 +52,9 @@
 #include "symfile.h"
 #include "extension.h"
 #include "observer.h"
+#include "user-selection.h"
+#include "common/scoped_restore.h"
+#include "cli-out.h"
 
 /* The possible choices of "set print frame-arguments", and the value
    of this setting.  */
@@ -1281,7 +1284,7 @@ print_frame (struct frame_info *frame, int print_level,
    this function never returns NULL).  When SELECTED_FRAME_P is non-NULL
    set its target to indicate that the default selected frame was used.  */
 
-static struct frame_info *
+struct frame_info *
 parse_frame_specification (const char *frame_exp, int *selected_frame_p)
 {
   int numargs;
@@ -2300,14 +2303,21 @@ find_relative_frame (struct frame_info *frame, int *level_offset_ptr)
    See parse_frame_specification for more info on proper frame
    expressions.  */
 
-void
+static void
 select_frame_command (char *level_exp, int from_tty)
 {
-  struct frame_info *prev_frame = get_selected_frame_if_set ();
+  cli_ui_out *uiout = dynamic_cast<cli_ui_out *> (current_uiout);
+  scoped_restore_suppress_output<cli_ui_out> restore (uiout);
+  user_selection *us = global_user_selection ();
+
+  uiout->suppress_output (true);
 
-  select_frame (parse_frame_specification (level_exp, NULL));
-  if (get_selected_frame_if_set () != prev_frame)
-    observer_notify_user_selected_context_changed (USER_SELECTED_FRAME);
+  if (level_exp != nullptr)
+    {
+      struct frame_info *frame = parse_frame_specification (level_exp, NULL);
+
+      us->select_frame (frame, true);
+    }
 }
 
 /* The "frame" command.  With no argument, print the selected frame
@@ -2317,20 +2327,30 @@ select_frame_command (char *level_exp, int from_tty)
 static void
 frame_command (char *level_exp, int from_tty)
 {
-  struct frame_info *prev_frame = get_selected_frame_if_set ();
+  user_selection *us = global_user_selection ();
 
-  select_frame (parse_frame_specification (level_exp, NULL));
-  if (get_selected_frame_if_set () != prev_frame)
-    observer_notify_user_selected_context_changed (USER_SELECTED_FRAME);
+  if (level_exp == nullptr)
+    {
+      if (us->thread () != nullptr
+	  && is_stopped (us->thread ()))
+	print_selected_thread_frame (current_uiout, us, USER_SELECTED_FRAME);
+      else
+	current_uiout->message (_("No stack.\n"));
+    }
   else
-    print_selected_thread_frame (current_uiout, USER_SELECTED_FRAME);
+    {
+      struct frame_info *frame = parse_frame_specification (level_exp, NULL);
+
+      if (!us->select_frame (frame, true))
+	print_selected_thread_frame (current_uiout, us, USER_SELECTED_FRAME);
+    }
 }
 
 /* Select the frame up one or COUNT_EXP stack levels from the
    previously selected frame, and print it briefly.  */
 
 static void
-up_silently_base (const char *count_exp)
+up_command (char *count_exp, int from_tty)
 {
   struct frame_info *frame;
   int count = 1;
@@ -2341,27 +2361,29 @@ up_silently_base (const char *count_exp)
   frame = find_relative_frame (get_selected_frame ("No stack."), &count);
   if (count != 0 && count_exp == NULL)
     error (_("Initial frame selected; you cannot go up."));
-  select_frame (frame);
+
+  user_selection *us = global_user_selection ();
+  us->select_frame (frame, true);
 }
 
 static void
 up_silently_command (char *count_exp, int from_tty)
 {
-  up_silently_base (count_exp);
-}
+  cli_ui_out *uiout = dynamic_cast<cli_ui_out *> (current_uiout);
 
-static void
-up_command (char *count_exp, int from_tty)
-{
-  up_silently_base (count_exp);
-  observer_notify_user_selected_context_changed (USER_SELECTED_FRAME);
+  gdb_assert (uiout != nullptr);
+
+  scoped_restore_suppress_output<cli_ui_out> restore (uiout);
+  uiout->suppress_output (true);
+
+  up_command (count_exp, from_tty);
 }
 
 /* Select the frame down one or COUNT_EXP stack levels from the previously
    selected frame, and print it briefly.  */
 
 static void
-down_silently_base (const char *count_exp)
+down_command (char *count_exp, int from_tty)
 {
   struct frame_info *frame;
   int count = -1;
@@ -2380,20 +2402,21 @@ down_silently_base (const char *count_exp)
       error (_("Bottom (innermost) frame selected; you cannot go down."));
     }
 
-  select_frame (frame);
+  user_selection *us = global_user_selection ();
+  us->select_frame (frame, true);
 }
 
 static void
 down_silently_command (char *count_exp, int from_tty)
 {
-  down_silently_base (count_exp);
-}
+  cli_ui_out *uiout = dynamic_cast<cli_ui_out *> (current_uiout);
 
-static void
-down_command (char *count_exp, int from_tty)
-{
-  down_silently_base (count_exp);
-  observer_notify_user_selected_context_changed (USER_SELECTED_FRAME);
+  gdb_assert (uiout != nullptr);
+
+  scoped_restore_suppress_output<cli_ui_out> restore (uiout);
+  uiout->suppress_output (true);
+
+  down_command (count_exp, from_tty);
 }
 
 void
@@ -2518,9 +2541,7 @@ return_command (char *retval_exp, int from_tty)
     frame_pop (get_current_frame ());
 
   select_frame (get_current_frame ());
-  /* If interactive, print the frame that is now current.  */
-  if (from_tty)
-    print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC, 1);
+  global_user_selection ()->select_frame (get_selected_frame (NULL), true);
 }
 
 /* Sets the scope to input function name, provided that the function