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, 357 insertions, 0 deletions

diff --git a/gdb/user-selection.c b/gdb/user-selection.c
new file mode 100644
index 0000000..098f915
--- /dev/null
+++ b/gdb/user-selection.c
@@ -0,0 +1,357 @@
+#include "defs.h"
+#include "user-selection.h"
+#include "inferior.h"
+#include "gdbthread.h"
+#include "observer.h"
+#include "gdbcmd.h"
+
+/* The user-visible selection.  */
+static user_selection main_user_selection;
+
+/* Knob for user-selection related debug traces.  */
+static int debug_user_selection = 0;
+
+/* See user-selection.h.  */
+
+user_selection *
+global_user_selection ()
+{
+  return &main_user_selection;
+}
+
+/* See user-selection.h.  */
+
+void
+init_global_user_selection ()
+{
+  /* Fetch the initial inferior, which should have been added by now.  The
+     initial inferior is selected on startup.  */
+  struct inferior *inf = find_inferior_id (1);
+
+  gdb_assert (inf != nullptr);
+
+  global_user_selection ()->select_inferior (inf, false);
+}
+
+/* See user-selection.h.  */
+
+bool
+user_selection::select_inferior (struct inferior *inf, bool notify)
+{
+  const char *debug_prefix = "user_selection::select_thread";
+
+  /* There is always a selected inferior.  */
+  gdb_assert (inf != nullptr);
+
+  if (debug_user_selection)
+    printf_unfiltered ("%s: num=%d\n", debug_prefix, inf->num);
+
+  /* No-op if this is already the currently selected inferior.  */
+  if (inf == m_inferior)
+    {
+      if (debug_user_selection)
+	printf_unfiltered ("%s: already selected inferior", debug_prefix);
+
+      return false;
+    }
+
+  /* When we change inferior, thread and frame will change as well.  */
+  user_selected_what what = USER_SELECTED_INFERIOR | USER_SELECTED_THREAD | USER_SELECTED_FRAME;
+
+  /* INF becomes selected.  */
+  m_inferior = inf;
+
+  /* Clear the thread and frame fields.  */
+  if (m_thread != nullptr)
+    {
+      m_thread->put ();
+      m_thread = nullptr;
+    }
+
+  m_frame_id = null_frame_id;
+  m_frame_level = INVALID_FRAME_LEVEL;
+
+  if (m_inferior->pid != 0)
+    {
+      /* This inferior is executing, so it should have threads.  Select the first
+         one.  */
+      m_thread = iterate_over_threads (
+	[inf] (struct thread_info *thread, void *) -> int
+	  {
+	    return inf->pid == ptid_get_pid (thread->ptid);
+	  }
+      );
+
+      /* We expect this inferior to have at least one thread.  If we didn't
+         find it, we have a problem.  */
+      gdb_assert (m_thread != nullptr);
+
+      /* Acquire a strong reference, so the thread_info object doesn't get freed
+         while it's selected.  */
+      m_thread->get ();
+    }
+
+  sanity_check ();
+
+  if (notify)
+    observer_notify_global_user_selection_changed (this, what);
+
+  return true;
+}
+
+/* See user-selection.h.  */
+
+bool
+user_selection::select_thread (struct thread_info *thread, bool notify)
+{
+  const char *debug_prefix = "user_selection::select_thread";
+
+  /* When changing thread, the frame will necessarily change as well.  */
+  user_selected_what what = USER_SELECTED_THREAD | USER_SELECTED_FRAME;
+
+  if (debug_user_selection)
+    printf_unfiltered ("%s: num=%d, ptid=%s",
+		       debug_prefix, thread->global_num,
+		       target_pid_to_str (thread->ptid));
+
+  /* No-op if this is already the currently selected thread.  */
+  if (thread == m_thread)
+    {
+      if (debug_user_selection)
+	printf_unfiltered ("%s: already selected thread", debug_prefix);
+
+      return false;
+    }
+
+  /* Clear the frame fields.  */
+  m_frame_id = null_frame_id;
+  m_frame_level = INVALID_FRAME_LEVEL;
+
+  /* Release the reference.  */
+  if (m_thread != nullptr)
+    m_thread->put ();
+
+  m_thread = thread;
+
+  if (m_thread != nullptr)
+    {
+      /* Acquire a strong reference, so the thread_info object doesn't get freed
+         while it's selected.  */
+      m_thread->get ();
+
+      /* The inferior of the thread becomes the newly selected inferior, if it's
+         not already.  */
+      if (m_inferior != thread->inf)
+	{
+	  m_inferior = thread->inf;
+
+	  what |= USER_SELECTED_INFERIOR;
+	}
+    }
+
+  sanity_check ();
+
+  if (notify)
+    observer_notify_global_user_selection_changed (this, what);
+
+  return true;
+}
+
+bool
+user_selection::select_frame (struct frame_info *frame, bool notify)
+{
+  const char *debug_prefix = "user_selection::select_frame";
+
+  /* No-op if this is already the selected frame.  */
+  if (frame_id_eq (m_frame_id, get_frame_id (frame))
+      && m_frame_level == frame_relative_level (frame))
+    return false;
+
+  m_frame_id = get_frame_id (frame);
+  m_frame_level = frame_relative_level (frame);
+
+  if (debug_user_selection)
+    {
+      string_file buf;
+
+      fprint_frame_id (&buf, m_frame_id);
+      printf_unfiltered ("%s: Selected frame #%d %s\n", debug_prefix,
+			 m_frame_level, buf.c_str ());
+    }
+
+  sanity_check ();
+
+  if (notify)
+    observer_notify_global_user_selection_changed (this, USER_SELECTED_FRAME);
+
+  return true;
+}
+
+/* Do some basic checks to verify that the selection is coherent.  */
+
+void
+user_selection::sanity_check () const
+{
+  /* We always have a current inferior.  */
+  gdb_assert (m_inferior != nullptr);
+
+  /* The selected thread must match the selected inferior.  */
+  if (m_thread != nullptr)
+    gdb_assert (m_thread->inf == m_inferior);
+
+  /* Can't have a current frame without a current thread.  */
+  if (m_frame_level != INVALID_FRAME_LEVEL)
+   gdb_assert (m_thread != nullptr);
+}
+
+/* Apply US to the core of gdb.  This makes the internally selected inferior,
+   thread and frame reflect the selection in US.  */
+
+static void
+apply_user_selection (user_selection *us)
+{
+  /* Select inferior.  */
+  set_current_inferior (us->inferior ());
+  set_current_program_space (us->inferior ()->pspace);
+
+  /* Select thread.  */
+  if (us->thread () != nullptr)
+    switch_to_thread (us->thread ()->ptid);
+  else
+    switch_to_thread (null_ptid);
+
+  /* Select frame.  */
+  if (us->has_valid_frame ())
+    {
+      struct frame_info *fi = us->frame ();
+
+      select_frame (fi);
+    }
+}
+
+/* Try to make the current (as in: where the program is currently stopped) frame
+   the selected one.  */
+
+void
+user_selection::try_select_current_frame ()
+{
+  /* This function should only be called when we don't have a selected frame
+     yet.  */
+  gdb_assert (!has_valid_frame ());
+
+  /* We need to select the relevant inferior/thread internally in order for
+     get_current_frame to work.  */
+  apply_user_selection (this);
+
+  TRY
+    {
+      struct frame_info *fi = get_current_frame ();
+
+      m_frame_id = get_frame_id (fi);
+      m_frame_level = frame_relative_level (fi);
+    }
+  CATCH (exception, RETURN_MASK_ALL)
+    {
+      /* We can't determine the current frame, too bad.  */
+    }
+  END_CATCH
+}
+
+/* See user-selection.h.  */
+
+void
+apply_global_user_selection ()
+{
+  apply_user_selection (global_user_selection ());
+}
+
+/* Callback for the new_thread observer.  */
+
+static void
+global_user_selection_on_new_thread (struct thread_info *tp)
+{
+  user_selection *us = global_user_selection ();
+
+  /* If a new thread is created while:
+
+       1. We don't have a currently selected thread,
+       2. The inferior of the new thread is the currently selected inferior,
+
+     then we silently make that new thread the selected one.  It covers the case
+     of automatically selecting the initial thread when starting an
+     inferior.  */
+
+  if (us->thread () == nullptr && tp->inf == us->inferior ())
+    us->select_thread (tp, false);
+}
+
+/* Callback for the on_exited observer.  */
+
+static void
+global_user_selection_on_exited (struct inferior *inferior)
+{
+  user_selection *us = global_user_selection ();
+
+  /* When an inferior exits and it's the current inferior, it means we have one
+     of its thread currently selected.  De-select it.  */
+
+  if (inferior == us->inferior ())
+    us->select_thread (NULL, false);
+}
+
+/* Callback for the on_target_resumed observer.  */
+
+static void
+global_user_selection_on_target_resumed (ptid_t ptid)
+{
+  user_selection *us = global_user_selection ();
+
+  /* If the selected thread has been resumed, our frame isn't valid anymore.  */
+  if (us->thread () != nullptr && ptid_match (us->thread ()->ptid, ptid))
+    us->select_frame (NULL, false);
+}
+
+/* Implementation of the "maintenance print user-selection" command.  */
+
+static void
+maint_print_user_selection (char *cmd, int from_tty)
+{
+  user_selection *us = global_user_selection ();
+
+  struct inferior *inf = us->inferior ();
+  struct thread_info *tp = us->thread ();
+  struct frame_id frame_id = us->raw_frame_id ();
+  int frame_level = us->raw_frame_level ();
+
+  /* Print inferior.  */
+  fprintf_filtered(gdb_stdout, "inferior %p (num=%d)\n", inf, inf->num);
+
+  /* Print thread.  */
+  if (tp != nullptr)
+    fprintf_filtered (gdb_stdout,
+		      "thread %p (gnum=%d, per-inf-num=%d, inf=%p)\n", tp,
+		      tp->global_num, tp->per_inf_num, tp->inf);
+  else
+    fprintf_filtered(gdb_stdout, "thread null\n");
+
+  /* Print frame.  */
+  fprint_frame_id (gdb_stdout, frame_id);
+  fprintf_filtered (gdb_stdout, ", level=%d\n", frame_level);
+}
+
+/* Initialize observer callbacks and commands.  */
+
+void
+_initialize_user_selection ()
+{
+  observer_attach_new_thread (global_user_selection_on_new_thread);
+  observer_attach_inferior_exit (global_user_selection_on_exited);
+  observer_attach_target_resumed (global_user_selection_on_target_resumed);
+
+  add_setshow_boolean_cmd ("user-selection", class_maintenance,
+			   &debug_user_selection, "blah", "blah", "blah", NULL,
+			   NULL, &setdebuglist, &showdebuglist);
+
+  add_cmd ("user-selection", class_maintenance, maint_print_user_selection,
+	   "foo", &maintenanceprintlist);
+}