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, 82 insertions, 85 deletions

diff --git a/gdb/thread.c b/gdb/thread.c
index 99fe424..b40d8cd 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -44,6 +44,7 @@
 #include "cli/cli-utils.h"
 #include "thread-fsm.h"
 #include "tid-parse.h"
+#include "user-selection.h"
 
 /* Definition of struct thread_info exported to gdbthread.h.  */
 
@@ -283,6 +284,8 @@ add_thread_silent (ptid_t ptid)
 	 new template thread in the list with an invalid ptid, switch
 	 to it, delete the original thread, reset the new thread's
 	 ptid, and switch to it.  */
+      if (tp == global_user_selection ()->thread ())
+	global_user_selection ()->select_thread (NULL, false);
 
       if (ptid_equal (inferior_ptid, ptid))
 	{
@@ -439,7 +442,7 @@ delete_thread_1 (ptid_t ptid, int silent)
   /* If this is the current thread, or there's code out there that
      relies on it existing (refcount > 0) we can't delete yet.  Mark
      it as exited, and notify it.  */
-  if (tp->refcount > 0
+  if (tp->refcount_ > 0
       || ptid_equal (tp->ptid, inferior_ptid))
     {
       if (tp->state != THREAD_EXITED)
@@ -541,7 +544,7 @@ find_thread_ptid (ptid_t ptid)
  */
 
 struct thread_info *
-iterate_over_threads (int (*callback) (struct thread_info *, void *),
+iterate_over_threads (thread_callback_func callback,
 		      void *data)
 {
   struct thread_info *tp, *next;
@@ -549,7 +552,7 @@ iterate_over_threads (int (*callback) (struct thread_info *, void *),
   for (tp = thread_list; tp; tp = next)
     {
       next = tp->next;
-      if ((*callback) (tp, data))
+      if (callback (tp, data))
 	return tp;
     }
 
@@ -989,12 +992,24 @@ is_stopped (ptid_t ptid)
   return is_thread_state (ptid, THREAD_STOPPED);
 }
 
+bool
+is_stopped (struct thread_info *thread)
+{
+  return thread->state == THREAD_STOPPED;
+}
+
 int
 is_exited (ptid_t ptid)
 {
   return is_thread_state (ptid, THREAD_EXITED);
 }
 
+bool
+is_exited (struct thread_info *thread)
+{
+  return thread->state == THREAD_EXITED;
+}
+
 int
 is_running (ptid_t ptid)
 {
@@ -1218,14 +1233,14 @@ print_thread_info_1 (struct ui_out *uiout, char *requested_threads,
 		     int show_global_ids)
 {
   struct thread_info *tp;
-  ptid_t current_ptid;
+  const struct thread_info *current_thread
+    = global_user_selection ()->thread ();
   struct cleanup *old_chain;
   const char *extra_info, *name, *target_id;
   struct inferior *inf;
   int default_inf_num = current_inferior ()->num;
 
   update_thread_list ();
-  current_ptid = inferior_ptid;
 
   /* We'll be switching threads temporarily.  */
   old_chain = make_cleanup_restore_current_thread ();
@@ -1289,14 +1304,14 @@ print_thread_info_1 (struct ui_out *uiout, char *requested_threads,
       if (uiout->is_mi_like_p ())
 	{
 	  /* Compatibility.  */
-	  if (ptid_equal (tp->ptid, current_ptid))
+	  if (tp == current_thread)
 	    uiout->text ("* ");
 	  else
 	    uiout->text ("  ");
 	}
       else
 	{
-	  if (ptid_equal (tp->ptid, current_ptid))
+	  if (tp == current_thread)
 	    uiout->field_string ("current", "*");
 	  else
 	    uiout->field_skip ("current");
@@ -1632,7 +1647,8 @@ restore_current_thread_cleanup_dtor (void *arg)
 
   tp = find_thread_ptid (old->inferior_ptid);
   if (tp)
-    tp->refcount--;
+    tp->put ();
+
   inf = find_inferior_id (old->inf_id);
   if (inf != NULL)
     inf->removable = old->was_removable;
@@ -1649,7 +1665,7 @@ set_thread_refcount (void *data)
     = (struct thread_array_cleanup *) data;
 
   for (k = 0; k != ta_cleanup->count; k++)
-    ta_cleanup->tp_array[k]->refcount--;
+    ta_cleanup->tp_array[k]->put ();
 }
 
 struct cleanup *
@@ -1689,7 +1705,7 @@ make_cleanup_restore_current_thread (void)
 
       tp = find_thread_ptid (inferior_ptid);
       if (tp)
-	tp->refcount++;
+	tp->get ();
     }
 
   current_inferior ()->removable = 0;
@@ -1806,7 +1822,7 @@ thread_apply_all_command (char *cmd, int from_tty)
       ALL_NON_EXITED_THREADS (tp)
         {
           tp_array[i] = tp;
-          tp->refcount++;
+          tp->get ();
           i++;
         }
       /* Because we skipped exited threads, we may end up with fewer
@@ -1940,50 +1956,34 @@ thread_apply_command (char *tidlist, int from_tty)
 void
 thread_command (char *tidstr, int from_tty)
 {
+  user_selection *us = global_user_selection ();
+
   if (tidstr == NULL)
     {
-      if (ptid_equal (inferior_ptid, null_ptid))
+      struct thread_info *thread = us->thread ();
+
+      if (thread == nullptr)
 	error (_("No thread selected"));
 
       if (target_has_stack)
 	{
-	  struct thread_info *tp = inferior_thread ();
-
-	  if (is_exited (inferior_ptid))
+	  if (is_exited (thread))
 	    printf_filtered (_("[Current thread is %s (%s) (exited)]\n"),
-			     print_thread_id (tp),
-			     target_pid_to_str (inferior_ptid));
+			     print_thread_id (thread),
+			     target_pid_to_str (thread->ptid));
 	  else
 	    printf_filtered (_("[Current thread is %s (%s)]\n"),
-			     print_thread_id (tp),
-			     target_pid_to_str (inferior_ptid));
+			     print_thread_id (thread),
+			     target_pid_to_str (thread->ptid));
 	}
       else
 	error (_("No stack."));
     }
   else
     {
-      ptid_t previous_ptid = inferior_ptid;
-      enum gdb_rc result;
-
-      result = gdb_thread_select (current_uiout, tidstr, NULL);
-
-      /* If thread switch did not succeed don't notify or print.  */
-      if (result == GDB_RC_FAIL)
-	return;
-
-      /* 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);
-	}
-      else
-	{
-	  observer_notify_user_selected_context_changed (USER_SELECTED_THREAD
-							 | USER_SELECTED_FRAME);
-	}
+      if (!thread_select (tidstr, true))
+	print_selected_thread_frame (current_uiout, us,
+				     USER_SELECTED_THREAD | USER_SELECTED_FRAME);
     }
 }
 
@@ -2069,48 +2069,56 @@ show_print_thread_events (struct ui_file *file, int from_tty,
                     value);
 }
 
-static int
-do_captured_thread_select (struct ui_out *uiout, void *tidstr_v)
+bool
+thread_select (const char *tidstr, bool tid_is_qualified)
 {
-  const char *tidstr = (const char *) tidstr_v;
-  struct thread_info *tp;
+  struct thread_info *thread;
 
-  if (uiout->is_mi_like_p ())
+  if (tid_is_qualified)
     {
-      int num = value_as_long (parse_and_eval (tidstr));
+      thread = parse_thread_id (tidstr, NULL);
 
-      tp = find_thread_global_id (num);
-      if (tp == NULL)
-	error (_("Thread ID %d not known."), num);
+      /* parse_thread_id is not supposed to return NULL.  */
+      gdb_assert (thread != NULL);
     }
   else
     {
-      tp = parse_thread_id (tidstr, NULL);
-      gdb_assert (tp != NULL);
+      int num = value_as_long (parse_and_eval (tidstr));
+
+      thread = find_thread_global_id (num);
+      if (thread == NULL)
+	error (_("Thread ID %d not known."), num);
     }
 
-  if (!thread_alive (tp))
+  if (!thread_alive (thread))
     error (_("Thread ID %s has terminated."), tidstr);
 
-  switch_to_thread (tp->ptid);
-
   annotate_thread_changed ();
 
-  /* Since the current thread may have changed, see if there is any
-     exited thread we can now delete.  */
-  prune_threads ();
+  if (global_user_selection ()->select_thread (thread, true))
+    {
+      /* Since the current thread may have changed, see if there is any
+	 exited thread we can now delete.  */
+      prune_threads ();
 
-  return GDB_RC_OK;
+      return true;
+    }
+  else
+    return false;
 }
 
 /* Print thread and frame switch command response.  */
 
 void
 print_selected_thread_frame (struct ui_out *uiout,
+			     user_selection *us,
 			     user_selected_what selection)
 {
-  struct thread_info *tp = inferior_thread ();
-  struct inferior *inf = current_inferior ();
+  struct inferior *inf = us->inferior ();
+  struct thread_info *thread = us->thread ();
+
+  if (thread == nullptr)
+    return;
 
   if (selection & USER_SELECTED_THREAD)
     {
@@ -2121,39 +2129,28 @@ print_selected_thread_frame (struct ui_out *uiout,
 	}
       else
 	{
-	  uiout->text ("[Switching to thread ");
-	  uiout->field_string ("new-thread-id", print_thread_id (tp));
-	  uiout->text (" (");
-	  uiout->text (target_pid_to_str (inferior_ptid));
-	  uiout->text (")]");
+	  const char *thread_id = print_thread_id (thread);
+	  const char *pid_str = target_pid_to_str (thread->ptid);
+	  const char *running_str
+	    = thread->state == THREAD_RUNNING ? " (running)" : "";
+
+	  uiout->field_fmt (NULL, "[Switching to thread %s (%s)]%s\n",
+			    thread_id, pid_str, running_str);
 	}
     }
 
-  if (tp->state == THREAD_RUNNING)
+  if ((selection & USER_SELECTED_FRAME)
+      && thread->state == THREAD_STOPPED)
     {
-      if (selection & USER_SELECTED_THREAD)
-	uiout->text ("(running)\n");
-    }
-  else if (selection & USER_SELECTED_FRAME)
-    {
-      if (selection & USER_SELECTED_THREAD)
-	uiout->text ("\n");
+      struct frame_info *frame = us->frame ();
 
-      if (has_stack_frames ())
-	print_stack_frame_to_uiout (uiout, get_selected_frame (NULL),
-				    1, SRC_AND_LOC, 1);
+      if (frame != nullptr)
+	print_stack_frame_to_uiout (uiout, frame, 1, SRC_AND_LOC, 1);
+      else
+	uiout->message (_("No selected frame.\n"));
     }
 }
 
-enum gdb_rc
-gdb_thread_select (struct ui_out *uiout, char *tidstr, char **error_message)
-{
-  if (catch_exceptions_with_msg (uiout, do_captured_thread_select, tidstr,
-				 error_message, RETURN_MASK_ALL) < 0)
-    return GDB_RC_FAIL;
-  return GDB_RC_OK;
-}
-
 /* Update the 'threads_executing' global based on the threads we know
    about right now.  */