Make scoped_restore_current_thread's cdtors exception free (RFC)users/palves/pr26199-busy-loop-target-events

If the remote target closes while we're reading registers/memory for
restoring the selected frame in scoped_restore_current_thread's dtor,
the corresponding TARGET_CLOSE_ERROR error is swallowed by the
scoped_restore_current_thread's dtor, because letting exceptions
escape from a dtor is bad.  It isn't great to lose that errors like
that, though.  I've been thinking about how to avoid it, and I came up
with this patch.

The idea here is to make scoped_restore_current_thread's dtor do as
little as possible, to avoid any work that might throw in the first
place.  And to do that, instead of having the dtor call
restore_selected_frame, which re-finds the previously selected frame,
just record the frame_id/level of the desired selected frame, and have
get_selected_frame find the frame the next time it is called.  In
effect, this implements most of Cagney's suggestion, here:

  /* On demand, create the selected frame and then return it.  If the
     selected frame can not be created, this function prints then throws
     an error.  When MESSAGE is non-NULL, use it for the error message,
     otherwize use a generic error message.  */
  /* FIXME: cagney/2002-11-28: At present, when there is no selected
     frame, this function always returns the current (inner most) frame.
     It should instead, when a thread has previously had its frame
     selected (but not resumed) and the frame cache invalidated, find
     and then return that thread's previously selected frame.  */
  extern struct frame_info *get_selected_frame (const char *message);

The only thing missing to fully implement that would be to make
reinit_frame_cache just clear selected_frame instead of calling
select_frame(NULL), and the call select_frame(NULL) explicitly in the
places where we really wanted reinit_frame_cache to go back to the
current frame too.  That can done separately, though, I'm not
proposing to do that in this patch.

Note that this patch renames restore_selected_frame to
lookup_selected_frame, and adds a new restore_selected_frame function
that doesn't throw, to be paired with the also-new save_selected_frame
function.

lookup_selected_frame should really move from thread.c to frame.c, but
I didn't do that here, just to avoid churn in the patch while it
collects comments.  I did make it extern and declared it in frame.h
already, preparing for the move.  I will do the move as a follow up
patch if people agree with this approach.

Incidentally, this patch alone would fix the crashes fixed by the
previous patches in the series, because with this,
scoped_restore_current_thread's constructor doesn't throw either.

gdb/ChangeLog:

	* blockframe.c (block_innermost_frame): Use get_selected_frame.
	* frame.c
	(scoped_restore_selected_frame::scoped_restore_selected_frame):
	Use save_selected_frame.  Save language as well.
	(scoped_restore_selected_frame::~scoped_restore_selected_frame):
	Use restore_selected_frame, and restore language as well.
	(selected_frame_id, selected_frame_level): New.
	(selected_frame): Update comments.
	(save_selected_frame, restore_selected_frame): New.
	(get_selected_frame): Use lookup_selected_frame.
	(get_selected_frame_if_set): Delete.
	(select_frame): Record selected_frame_level and selected_frame_id.
	* frame.h (scoped_restore_selected_frame) <m_level, m_lang>: New
	fields.
	(get_selected_frame_if_set): Delete declaration.
	(select_frame): Update comments.
	(save_selected_frame, restore_selected_frame)
	(lookup_selected_frame): Declare.
	* gdbthread.h (scoped_restore_current_thread) <m_lang>: New field.
	* stack.c (select_frame_command_core, frame_command_core): Use
	get_selected_frame.
	* thread.c (restore_selected_frame): Rename to ...
	(lookup_selected_frame): ... this and make extern.  Select the
	current frame if the frame level is -1.
	(scoped_restore_current_thread::restore): Also restore the
	language.
	(scoped_restore_current_thread::~scoped_restore_current_thread):
	Don't try/catch.
	(scoped_restore_current_thread::scoped_restore_current_thread):
	Save the language as well.  Use save_selected_frame.

1 files changed, 80 insertions, 21 deletions

diff --git a/gdb/frame.c b/gdb/frame.c
index ff27b9f00e9..2a88b7ba39c 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -297,17 +297,15 @@ frame_stash_invalidate (void)
 /* See frame.h  */
 scoped_restore_selected_frame::scoped_restore_selected_frame ()
 {
-  m_fid = get_frame_id (get_selected_frame (NULL));
+  m_lang = current_language->la_language;
+  save_selected_frame (&m_fid, &m_level);
 }
 
 /* See frame.h  */
 scoped_restore_selected_frame::~scoped_restore_selected_frame ()
 {
-  frame_info *frame = frame_find_by_id (m_fid);
-  if (frame == NULL)
-    warning (_("Unable to restore previously selected frame."));
-  else
-    select_frame (frame);
+  restore_selected_frame (m_fid, m_level);
+  set_language (m_lang);
 }
 
 /* Flag to control debugging.  */
@@ -1641,10 +1639,51 @@ get_current_frame (void)
 }
 
 /* The "selected" stack frame is used by default for local and arg
-   access.  May be zero, for no selected frame.  */
-
+   access.  */
+
+/* The "single source of truth" for the selected frame is the
+   SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL pair.  Frame IDs can be
+   saved/restored across reinitializing the frame cache, while
+   frame_info pointers can't (frame_info objects are invalidated).  If
+   we know the corresponding frame_info object, it is cached in
+   SELECTED_FRAME.  If SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are
+   null_ptid / -1, and the target has stack and is stopped, the
+   selected frame is the current (innermost) frame, otherwise there's
+   no selected frame.  */
+static frame_id selected_frame_id = null_frame_id;
+static int selected_frame_level = -1;
+
+/* The cached frame_info object pointing to the selected frame.
+   Looked up on demand by get_selected_frame.  */
 static struct frame_info *selected_frame;
 
+/* See frame.h.  */
+
+void
+save_selected_frame (frame_id *frame_id, int *frame_level)
+  noexcept
+{
+  *frame_id = selected_frame_id;
+  *frame_level = selected_frame_level;
+}
+
+/* See frame.h.  */
+
+void
+restore_selected_frame (frame_id a_frame_id, int frame_level)
+  noexcept
+{
+  /* save_selected_frame never returns level == 0, so we shouldn't see
+     it here either.  */
+  gdb_assert (frame_level != 0);
+
+  selected_frame_id = a_frame_id;
+  selected_frame_level = frame_level;
+
+  /* Will be looked up latter by get_seleted_frame.  */
+  selected_frame = nullptr;
+}
+
 int
 has_stack_frames (void)
 {
@@ -1682,24 +1721,14 @@ get_selected_frame (const char *message)
     {
       if (message != NULL && !has_stack_frames ())
 	error (("%s"), message);
-      /* Hey!  Don't trust this.  It should really be re-finding the
-	 last selected frame of the currently selected thread.  This,
-	 though, is better than nothing.  */
-      select_frame (get_current_frame ());
+
+      lookup_selected_frame (selected_frame_id, selected_frame_level);
     }
   /* There is always a frame.  */
   gdb_assert (selected_frame != NULL);
   return selected_frame;
 }
 
-/* If there is a selected frame, return it.  Otherwise, return NULL.  */
-
-struct frame_info *
-get_selected_frame_if_set (void)
-{
-  return selected_frame;
-}
-
 /* This is a variant of get_selected_frame() which can be called when
    the inferior does not have a frame; in that case it will return
    NULL instead of calling error().  */
@@ -1712,12 +1741,42 @@ deprecated_safe_get_selected_frame (void)
   return get_selected_frame (NULL);
 }
 
-/* Select frame FI (or NULL - to invalidate the current frame).  */
+/* Select frame FI (or NULL - to invalidate the selected frame).  */
 
 void
 select_frame (struct frame_info *fi)
 {
   selected_frame = fi;
+  selected_frame_level = frame_relative_level (fi);
+  if (selected_frame_level == 0)
+    {
+      /* Treat the current frame especially -- we want to always
+	 save/restore it without warning, even if the frame ID changes
+	 (see lookup_selected_frame).  E.g.:
+
+	  // The current frame is selected, the target had just stopped.
+	  {
+	    scoped_restore_selected_frame restore_frame;
+	    some_operation_that_changes_the_stack ();
+	  }
+	  // scoped_restore_selected_frame's dtor runs, but the
+	  // original frame_id can't be found.  No matter whether it
+	  // is found or not, we still end up with the now-current
+	  // frame selected.  Warning in lookup_selected_frame in this
+	  // case seems pointless.
+
+	 Also get_frame_id may access the target's registers/memory,
+	 and thus skipping get_frame_id optimizes the common case.
+
+	 Saving the selected frame this way makes get_selected_frame
+	 and restore_current_frame return/re-select whatever frame is
+	 the innermost (current) then.  */
+      selected_frame_level = -1;
+      selected_frame_id = null_frame_id;
+    }
+  else
+    selected_frame_id = get_frame_id (fi);
+
   /* NOTE: cagney/2002-05-04: FI can be NULL.  This occurs when the
      frame is being invalidated.  */