[PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC)

[Pedro Alves <pedro@palves.net>]

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.

restore_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 will do that as a preparatory 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/blockframe.c |  6 +----
 gdb/frame.c      | 73 ++++++++++++++++++++++++++++++++++++++++----------------
 gdb/frame.h      | 22 +++++++++++++----
 gdb/gdbthread.h  |  4 ++++
 gdb/stack.c      |  9 ++++---
 gdb/thread.c     | 67 ++++++++++++++++-----------------------------------
 6 files changed, 98 insertions(+), 83 deletions(-)

diff --git a/gdb/blockframe.c b/gdb/blockframe.c
index 05c26bc2c2..706b6db92c 100644
--- a/gdb/blockframe.c
+++ b/gdb/blockframe.c
@@ -448,14 +448,10 @@ find_gnu_ifunc_target_type (CORE_ADDR resolver_funaddr)
 struct frame_info *
 block_innermost_frame (const struct block *block)
 {
-  struct frame_info *frame;
-
   if (block == NULL)
     return NULL;
 
-  frame = get_selected_frame_if_set ();
-  if (frame == NULL)
-    frame = get_current_frame ();
+  frame_info *frame = get_selected_frame (NULL);
   while (frame != NULL)
     {
       const struct block *frame_block = get_frame_block (frame, NULL);
diff --git a/gdb/frame.c b/gdb/frame.c
index ff27b9f00e..e7dffd204b 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));
+  get_selected_frame_info (&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);
+  /* Use the lazy variant because we don't want to do any work here
+     that might throw, since we're in a dtor.  */
+  select_frame_lazy (m_fid, m_level);
 }
 
 /* Flag to control debugging.  */
@@ -1641,9 +1639,24 @@ 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.  */
+
+/* If SELECTED_FRAME is NULL, then the selected frame is found in
+   SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL.  Those are used by
+   get_selected_frame to re-find the selected frame.  If
+   SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_ptid / -1, there
+   is no selected frame, in which case the next time
+   get_selected_frame is called, it selects the current frame.  */
 static struct frame_info *selected_frame;
+static frame_id selected_frame_id = null_frame_id;
+static int selected_frame_level = -1;
+
+void
+get_selected_frame_info (frame_id *frame_id, int *level)
+{
+  *frame_id = selected_frame_id;
+  *level = selected_frame_level;
+}
 
 int
 has_stack_frames (void)
@@ -1671,6 +1684,8 @@ has_stack_frames (void)
   return 1;
 }
 
+void restore_selected_frame (struct frame_id a_frame_id, int frame_level);
+
 /* Return the selected frame.  Always non-NULL (unless there isn't an
    inferior sufficient for creating a frame) in which case an error is
    thrown.  */
@@ -1682,24 +1697,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 ());
+
+      restore_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 +1717,26 @@ 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 restore_selected_frame).  Also get_frame_id may access
+	 the target's registers/memory, and thus skipping get_frame_id
+	 optimizes the common case.  */
+      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.  */
 
@@ -1756,6 +1775,18 @@ select_frame (struct frame_info *fi)
     }
 }
 
+void
+select_frame_lazy (frame_id a_frame_id, int frame_level)
+{
+  /* get_selected_frame_info never returns level == 0, so we shouldn't
+     see it here either.  */
+  gdb_assert (frame_level != 0);
+
+  selected_frame = nullptr;
+  selected_frame_id = a_frame_id;
+  selected_frame_level = frame_level;
+}
+
 /* Create an arbitrary (i.e. address specified by user) or innermost frame.
    Always returns a non-NULL value.  */
 
diff --git a/gdb/frame.h b/gdb/frame.h
index e835d49f9c..48222c69d3 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -181,8 +181,9 @@ class scoped_restore_selected_frame
 
 private:
 
-  /* The ID of the previously selected frame.  */
+  /* The ID and level of the previously selected frame.  */
   struct frame_id m_fid;
+  int m_level;
 };
 
 /* Methods for constructing and comparing Frame IDs.  */
@@ -329,13 +330,24 @@ extern void reinit_frame_cache (void);
    and then return that thread's previously selected frame.  */
 extern struct frame_info *get_selected_frame (const char *message);
 
-/* If there is a selected frame, return it.  Otherwise, return NULL.  */
-extern struct frame_info *get_selected_frame_if_set (void);
+/* Return frame ID and frame level of the selected frame.  If there's
+   no selected frame or the selected frame is the current frame,
+   return null_frame_id/-1.  This is preferred over getting the same
+   info out of get_selected_frame directly because this function does
+   not create the selected-frame's frame_info object if it hasn't been
+   created yet.  */
+extern void get_selected_frame_info (frame_id *frame_id, int *level);
 
-/* Select a specific frame.  NULL, apparently implies re-select the
-   inner most frame.  */
+/* Select a specific frame.  NULL implies re-select the inner most
+   frame.  */
 extern void select_frame (struct frame_info *);
 
+/* Setup frame A_FRAME_ID, with level FRAME_LEVEL as the selected
+   frame, but don't actually try to find it right now.  The next call
+   to get_selected_frame will look it up (and cache the result).
+   null_frame_id/-1 implies re-select the inner most frame.  */
+extern void select_frame_lazy (frame_id a_frame_id, int frame_level);
+
 /* Given a FRAME, return the next (more inner, younger) or previous
    (more outer, older) frame.  */
 extern struct frame_info *get_prev_frame (struct frame_info *);
diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 0166b2000f..edfdf98b3d 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -667,6 +667,10 @@ class scoped_restore_current_thread
   frame_id m_selected_frame_id;
   int m_selected_frame_level;
   bool m_was_stopped;
+  /* Save/restore the language as well, because selecting a frame
+     changes the current language to the frame's language if "set
+     language auto".  */
+  enum language m_lang;
 };
 
 /* Returns a pointer into the thread_info corresponding to
diff --git a/gdb/stack.c b/gdb/stack.c
index 265e764dc2..93de451a12 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -1836,9 +1836,9 @@ trailing_outermost_frame (int count)
 static void
 select_frame_command_core (struct frame_info *fi, bool ignored)
 {
-  struct frame_info *prev_frame = get_selected_frame_if_set ();
+  frame_info *prev_frame = get_selected_frame (NULL);
   select_frame (fi);
-  if (get_selected_frame_if_set () != prev_frame)
+  if (get_selected_frame (NULL) != prev_frame)
     gdb::observers::user_selected_context_changed.notify (USER_SELECTED_FRAME);
 }
 
@@ -1857,10 +1857,9 @@ select_frame_for_mi (struct frame_info *fi)
 static void
 frame_command_core (struct frame_info *fi, bool ignored)
 {
-  struct frame_info *prev_frame = get_selected_frame_if_set ();
-
+  frame_info *prev_frame = get_selected_frame (nullptr);
   select_frame (fi);
-  if (get_selected_frame_if_set () != prev_frame)
+  if (get_selected_frame (nullptr) != prev_frame)
     gdb::observers::user_selected_context_changed.notify (USER_SELECTED_FRAME);
   else
     print_selected_thread_frame (current_uiout, USER_SELECTED_FRAME);
diff --git a/gdb/thread.c b/gdb/thread.c
index 1ec047e35b..8a5634eae3 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1325,20 +1325,25 @@ switch_to_thread (process_stratum_target *proc_target, ptid_t ptid)
   switch_to_thread (thr);
 }
 
-static void
+void restore_selected_frame (struct frame_id a_frame_id, int frame_level);
+
+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.  */
+  /* This either means there was no selected frame, or the selected
+     frame was the inner most (the current frame).  */
   if (frame_level == -1)
     {
-      select_frame (NULL);
+      select_frame (get_current_frame ());
       return;
     }
 
-  gdb_assert (frame_level >= 0);
+  /* select_frame never saves 0 in selected_frame_level, so we
+     shouldn't see it here.  */
+  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
@@ -1408,23 +1413,19 @@ 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);
+    {
+      /* Use the lazy variant because we don't want to do any work
+	 here that might throw, since we're in a dtor.  */
+      select_frame_lazy (m_selected_frame_id, m_selected_frame_level);
+    }
+
+  set_language (m_lang);
 }
 
 scoped_restore_current_thread::~scoped_restore_current_thread ()
 {
   if (!m_dont_restore)
-    {
-      try
-	{
-	  restore ();
-	}
-      catch (const gdb_exception &ex)
-	{
-	  /* We're in a dtor, there's really nothing else we can do
-	     but swallow the exception.  */
-	}
-    }
+    restore ();
 
   if (m_thread != NULL)
     m_thread->decref ();
@@ -1436,43 +1437,15 @@ scoped_restore_current_thread::scoped_restore_current_thread ()
   m_inf = current_inferior ();
   m_inf->incref ();
 
+  m_lang = current_language->la_language;
+
   if (inferior_ptid != null_ptid)
     {
       m_thread = inferior_thread ();
       m_thread->incref ();
 
-      struct frame_info *frame;
-
       m_was_stopped = m_thread->state == THREAD_STOPPED;
-      if (m_was_stopped
-	  && target_has_registers
-	  && target_has_stack
-	  && target_has_memory)
-	{
-	  /* When processing internal events, there might not be a
-	     selected frame.  If we naively call get_selected_frame
-	     here, then we can end up reading debuginfo for the
-	     current frame, but we don't generally need the debuginfo
-	     at this point.  */
-	  frame = get_selected_frame_if_set ();
-	}
-      else
-	frame = NULL;
-
-      try
-	{
-	  m_selected_frame_id = get_frame_id (frame);
-	  m_selected_frame_level = frame_relative_level (frame);
-	}
-      catch (const gdb_exception_error &ex)
-	{
-	  m_selected_frame_id = null_frame_id;
-	  m_selected_frame_level = -1;
-
-	  /* Better let this propagate.  */
-	  if (ex.error == TARGET_CLOSE_ERROR)
-	    throw;
-	}
+      get_selected_frame_info (&m_selected_frame_id, &m_selected_frame_level);
     }
   else
     m_thread = NULL;
-- 
2.14.5