diff mbox

[RFC,1/2] gdb: Unify two copies of restore_selected_frame

Message ID 354ba2265d63d1004d2d2ea7e2611d1dcac68700.1582643363.git.andrew.burgess@embecosm.com
State New
Headers show

Commit Message

Andrew Burgess Feb. 25, 2020, 3:13 p.m. UTC
GDB currently has two copies of restore_selected_frame, this commit
unifies these two, and moves the implementation into frame.c.

The only possible user visible change after this commit is if GDB is
unable to restore the currently selected stack after performing an
inferior call.

Previously, in any situation, if GDB failed to find a frame with a
matching frame id, then the following warning would be issued, and GDB
would be left with the innermost stack frame selected:

  Unable to restore previously selected frame.

After this commit, then GDB will give a slightly different warning:

  Couldn't restore frame #%d in current thread.  Bottom (innermost) frame selected:
  ....

Where '....' is a summary of the frame that was selected, and '%d' is
the frame level GDB was looking for.  Additionally, this warning is
not given if the previously selected frame was at level 0, in that
case GDB will just silently selected the innermost frame.

If this difference is a problem then it should be easy enough to have
restore_selected_frame take an 'always-warn' flag parameter, but I
haven't done that in this commit.

Beyond this there should be no other user visible changes with this
commit.

gdb/ChangeLog:

	* frame.c (restore_selected_frame): Moved from thread.c.
	* frame.h (restore_selected_frame): Declare.
	(struct frame_id_and_level): New struct.
	* gdbthread.h (scoped_restore_current_thread)
	<m_selected_frame_id>: Delete.  <m_selected_frame_level>: Delete.
	<m_selected_frame_info>: New member variable.
	* infrun.c (infcall_control_state) <selected_frame_id>: Delete.
	<selected_frame_info>: New member variable.
	(save_infcall_control_state): Update to use selected_frame_info.
	(restore_selected_frame): Delete.
	(restore_infcall_control_state): Update to use selected_frame_info.
	* thread.c (restore_selected_frame): Delete.
	(scoped_restore_current_thread::restore): Update for changes to
	member variables.
	(scoped_restore_current_thread::scoped_restore_current_thread):
	Likewise.
---
 gdb/ChangeLog   | 19 +++++++++++++++++
 gdb/frame.c     | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/frame.h     | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/gdbthread.h |  3 +--
 gdb/infrun.c    | 26 ++++++-----------------
 gdb/thread.c    | 66 +++------------------------------------------------------
 6 files changed, 150 insertions(+), 85 deletions(-)
diff mbox

Patch

diff --git a/gdb/frame.c b/gdb/frame.c
index d74d1d5c7c5..863df107009 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -2910,6 +2910,67 @@  frame_prepare_for_sniffer (struct frame_info *frame,
   frame->unwind = unwind;
 }
 
+/* See frame.h.  */
+
+void
+restore_selected_frame (struct frame_id a_frame_id, int frame_level)
+{
+  struct frame_info *frame;
+  int count;
+
+  /* This means there was no selected frame.  */
+  if (frame_level == -1)
+    {
+      select_frame (NULL);
+      return;
+    }
+
+  gdb_assert (frame_level >= 0);
+
+  /* Restore by level first, check if the frame id is the same as
+     expected.  If that fails, try restoring by frame id.  If that
+     fails, nothing to do, just warn the user.  */
+
+  count = frame_level;
+  frame = find_relative_frame (get_current_frame (), &count);
+  if (count == 0
+      && frame != NULL
+      /* The frame ids must match - either both valid or both outer_frame_id.
+	 The latter case is not failsafe, but since it's highly unlikely
+	 the search by level finds the wrong frame, it's 99.9(9)% of
+	 the time (for all practical purposes) safe.  */
+      && frame_id_eq (get_frame_id (frame), a_frame_id))
+    {
+      /* Cool, all is fine.  */
+      select_frame (frame);
+      return;
+    }
+
+  frame = frame_find_by_id (a_frame_id);
+  if (frame != NULL)
+    {
+      /* Cool, refound it.  */
+      select_frame (frame);
+      return;
+    }
+
+  /* Nothing else to do, the frame layout really changed.  Select the
+     innermost stack frame.  */
+  select_frame (get_current_frame ());
+
+  /* Warn the user.  */
+  if (frame_level > 0 && !current_uiout->is_mi_like_p ())
+    {
+      warning (_("Couldn't restore frame #%d in "
+		 "current thread.  Bottom (innermost) frame selected:"),
+	       frame_level);
+      /* For MI, we should probably have a notification about
+	 current frame change.  But this error is not very
+	 likely, so don't bother for now.  */
+      print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC, 1);
+    }
+}
+
 static struct cmd_list_element *set_backtrace_cmdlist;
 static struct cmd_list_element *show_backtrace_cmdlist;
 
diff --git a/gdb/frame.h b/gdb/frame.h
index cfc15022ed5..d8e000eda8b 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -957,5 +957,65 @@  extern void set_frame_previous_pc_masked (struct frame_info *frame);
 
 extern bool get_frame_pc_masked (const struct frame_info *frame);
 
+/* Try to find a previously selected frame described by A_FRAME_ID and
+   select it.  If no matching frame can be found then the innermost frame
+   will be selected and a warning printed.  FRAME_LEVEL is the level at
+   which A_FRAME_ID was previously found, and can be -1 to indicate no
+   frame was previously selected, in which case the innermost frame will
+   be selected (without a warning).  */
+
+extern void restore_selected_frame (struct frame_id a_frame_id, int frame_level);
+
+/* When GDB needs to backup and then later restore the currently selected
+   frame this is done by storing the frame id, and then looking up a frame
+   with that stored frame id.
+
+   However, if the previously selected frame can't be restored then GDB
+   should give the user a warning in most cases.  If the previously
+   selected frame was level 0 then GDB will just reselect the innermost
+   frame silently without a warning.
+
+   And so, when we backup and restore the currently selected frame we need
+   to track both the frame id, and the frame level, so GDB knows if a
+   warning should be given or not.  */
+
+struct frame_id_and_level
+{
+  /* Setup this structure to track FI as the previously selected frame.  */
+  void reset (struct frame_info *fi)
+  {
+    m_id = get_frame_id (fi);
+    m_level = frame_relative_level (fi);
+  }
+
+  /* Reset this structure to indicate there was no previously selected
+     frame.  */
+  void reset ()
+  {
+    m_level = -1;
+  }
+
+  /* The frame id of the previously selected frame.  This value is only
+     defined when LEVEL() is greater than -1.  */
+  struct frame_id id () const
+  {
+    return m_id;
+  }
+
+  /* The level of the previously selected frame, or -1 if no frame was
+     previously selected.  */
+  int level () const
+  {
+    return m_level;
+  }
+
+private:
+  /* The frame id.  */
+  struct frame_id m_id;
+
+  /* The level at which ID was found.  Set to -1 to indicate that this
+     structure is uninitialised.  */
+  int m_level = -1;
+};
 
 #endif /* !defined (FRAME_H)  */
diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 717a2ad08c2..26e572d1e98 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -681,8 +681,7 @@  private:
      function in the Darwin API.  */
   class thread_info *m_thread;
   inferior *m_inf;
-  frame_id m_selected_frame_id;
-  int m_selected_frame_level;
+  struct frame_id_and_level m_selected_frame_info;
   bool m_was_stopped;
 };
 
diff --git a/gdb/infrun.c b/gdb/infrun.c
index d9a6f733519..3cb46041c9a 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -9104,8 +9104,9 @@  struct infcall_control_state
   enum stop_stack_kind stop_stack_dummy = STOP_NONE;
   int stopped_by_random_signal = 0;
 
-  /* ID if the selected frame when the inferior function call was made.  */
-  struct frame_id selected_frame_id {};
+  /* ID and level of the selected frame when the inferior function call was
+     made.  */
+  struct frame_id_and_level selected_frame_info;
 };
 
 /* Save all of the information associated with the inferior<==>gdb
@@ -9134,27 +9135,11 @@  save_infcall_control_state ()
   inf_status->stop_stack_dummy = stop_stack_dummy;
   inf_status->stopped_by_random_signal = stopped_by_random_signal;
 
-  inf_status->selected_frame_id = get_frame_id (get_selected_frame (NULL));
+  inf_status->selected_frame_info.reset (get_selected_frame (NULL));
 
   return inf_status;
 }
 
-static void
-restore_selected_frame (const frame_id &fid)
-{
-  frame_info *frame = frame_find_by_id (fid);
-
-  /* If inf_status->selected_frame_id is NULL, there was no previously
-     selected frame.  */
-  if (frame == NULL)
-    {
-      warning (_("Unable to restore previously selected frame."));
-      return;
-    }
-
-  select_frame (frame);
-}
-
 /* Restore inferior session state to INF_STATUS.  */
 
 void
@@ -9187,7 +9172,8 @@  restore_infcall_control_state (struct infcall_control_state *inf_status)
          error() trying to dereference it.  */
       try
 	{
-	  restore_selected_frame (inf_status->selected_frame_id);
+	  restore_selected_frame (inf_status->selected_frame_info.id (),
+				  inf_status->selected_frame_info.level ());
 	}
       catch (const gdb_exception_error &ex)
 	{
diff --git a/gdb/thread.c b/gdb/thread.c
index 54b59e22448..d0c795c5864 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1356,65 +1356,6 @@  switch_to_thread (process_stratum_target *proc_target, ptid_t ptid)
   switch_to_thread (thr);
 }
 
-static void
-restore_selected_frame (struct frame_id a_frame_id, int frame_level)
-{
-  struct frame_info *frame = NULL;
-  int count;
-
-  /* This means there was no selected frame.  */
-  if (frame_level == -1)
-    {
-      select_frame (NULL);
-      return;
-    }
-
-  gdb_assert (frame_level >= 0);
-
-  /* Restore by level first, check if the frame id is the same as
-     expected.  If that fails, try restoring by frame id.  If that
-     fails, nothing to do, just warn the user.  */
-
-  count = frame_level;
-  frame = find_relative_frame (get_current_frame (), &count);
-  if (count == 0
-      && frame != NULL
-      /* The frame ids must match - either both valid or both outer_frame_id.
-	 The latter case is not failsafe, but since it's highly unlikely
-	 the search by level finds the wrong frame, it's 99.9(9)% of
-	 the time (for all practical purposes) safe.  */
-      && frame_id_eq (get_frame_id (frame), a_frame_id))
-    {
-      /* Cool, all is fine.  */
-      select_frame (frame);
-      return;
-    }
-
-  frame = frame_find_by_id (a_frame_id);
-  if (frame != NULL)
-    {
-      /* Cool, refound it.  */
-      select_frame (frame);
-      return;
-    }
-
-  /* Nothing else to do, the frame layout really changed.  Select the
-     innermost stack frame.  */
-  select_frame (get_current_frame ());
-
-  /* Warn the user.  */
-  if (frame_level > 0 && !current_uiout->is_mi_like_p ())
-    {
-      warning (_("Couldn't restore frame #%d in "
-		 "current thread.  Bottom (innermost) frame selected:"),
-	       frame_level);
-      /* For MI, we should probably have a notification about
-	 current frame change.  But this error is not very
-	 likely, so don't bother for now.  */
-      print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC, 1);
-    }
-}
-
 void
 scoped_restore_current_thread::restore ()
 {
@@ -1439,7 +1380,8 @@  scoped_restore_current_thread::restore ()
       && target_has_registers
       && target_has_stack
       && target_has_memory)
-    restore_selected_frame (m_selected_frame_id, m_selected_frame_level);
+    restore_selected_frame (m_selected_frame_info.id (),
+			    m_selected_frame_info.level ());
 }
 
 scoped_restore_current_thread::~scoped_restore_current_thread ()
@@ -1488,9 +1430,7 @@  scoped_restore_current_thread::scoped_restore_current_thread ()
       else
 	frame = NULL;
 
-      m_selected_frame_id = get_frame_id (frame);
-      m_selected_frame_level = frame_relative_level (frame);
-
+      m_selected_frame_info.reset (frame);
       tp->incref ();
       m_thread = tp;
     }