From FEDORA_PATCHES Mon Sep 17 00:00:00 2001

From: Pedro Alves <pedro@palves.net>

Date: Fri, 30 Oct 2020 18:26:15 +0100

Subject: gdb-rhbz1909902-frame_id_p-assert-1.patch

;; Backport fix for frame_id_p assertion failure (RH BZ 1909902).

Make scoped_restore_current_thread's cdtors exception free (RFC)

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.

There's a restore_selected_frame function in infrun.c that I think can

be replaced by the new one in frame.c.

Also done in this patch is make the get_selected_frame's parameter be

optional, so that we don't have to pass down nullptr explicitly all

over the place.

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): Make 'message' parameter optional.

	(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.

	* infrun.c (struct infcall_control_state) <selected_frame_level>:

	New field.

	(save_infcall_control_state): Use save_selected_frame.

	(restore_selected_frame): Delete.

	(restore_infcall_control_state): Use restore_selected_frame.

	* 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.

Change-Id: I73fd1cfc40d8513c28e5596383b7ecd8bcfe700f

diff --git a/gdb/blockframe.c b/gdb/blockframe.c

--- a/gdb/blockframe.c

+++ b/gdb/blockframe.c

@@ -464,14 +464,10 @@ struct type *

 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 ();

   while (frame != NULL)

     {

       const struct block *frame_block = get_frame_block (frame, NULL);

diff --git a/gdb/frame.c b/gdb/frame.c

--- a/gdb/frame.c

+++ b/gdb/frame.c

@@ -317,17 +317,15 @@ struct frame_info

 /* 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.  */

@@ -1685,10 +1683,63 @@ struct frame_info *

 }

 /* 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_frame_id / -1,

+   and the target has stack and is stopped, the selected frame is the

+   current (innermost) frame.  This means that SELECTED_FRAME_LEVEL is

+   never 0 and SELECTED_FRAME_ID is never the ID of the innermost

+   frame.

+

+   If SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_frame_id / -1,

+   and the target has no stack or is executing, then 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 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);

+

+  /* FRAME_ID can be null_frame_id only IFF frame_level is -1.  */

+  gdb_assert ((frame_level == -1 && !frame_id_p (frame_id))

+	      || (frame_level != -1 && frame_id_p (frame_id)));

+

+  selected_frame_id = frame_id;

+  selected_frame_level = frame_level;

+

+  /* Will be looked up later by get_selected_frame.  */

+  selected_frame = nullptr;

+}

+

 bool

 has_stack_frames ()

 {

@@ -1715,9 +1766,7 @@ struct frame_info *

   return true;

 }

-/* 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.  */

+/* See frame.h.  */

 struct frame_info *

 get_selected_frame (const char *message)

@@ -1726,24 +1775,14 @@ struct frame_info *

     {

       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().  */

@@ -1756,12 +1795,42 @@ struct frame_info *

   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.  */

diff --git a/gdb/frame.h b/gdb/frame.h

--- a/gdb/frame.h

+++ b/gdb/frame.h

@@ -186,8 +186,14 @@ 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;

+

+  /* 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;

 };

 /* Methods for constructing and comparing Frame IDs.  */

@@ -316,24 +322,49 @@ enum frame_type

    modifies the target invalidating the frame cache).  */

 extern void reinit_frame_cache (void);

-/* 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,

+/* Return the selected frame.  Always returns non-NULL.  If there

+   isn't an inferior sufficient for creating a frame, an error is

+   thrown.  When MESSAGE is non-NULL, use it for the error message,

    otherwise 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);

-

-/* If there is a selected frame, return it.  Otherwise, return NULL.  */

-extern struct frame_info *get_selected_frame_if_set (void);

+extern struct frame_info *get_selected_frame (const char *message = nullptr);

-/* 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 *);

+/* Save the frame ID and frame level of the selected frame in FRAME_ID

+   and FRAME_LEVEL, to be restored later with restore_selected_frame.

+

+   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, and thus is more efficient and doesn't throw.  */

+extern void save_selected_frame (frame_id *frame_id, int *frame_level)

+  noexcept;

+

+/* Restore selected frame as saved with save_selected_frame.

+

+   Does not try to find the corresponding frame_info object.  Instead

+   the next call to get_selected_frame will look it up and cache the

+   result.

+

+   This function does not throw.  It is designed to be safe to called

+   from the destructors of RAII types.  */

+extern void restore_selected_frame (frame_id frame_id, int frame_level)

+  noexcept;

+

+/* Lookup the frame_info object for the selected frame FRAME_ID /

+   FRAME_LEVEL and cache the result.

+

+   If FRAME_LEVEL > 0 and the originally selected frame isn't found,

+   warn and select the innermost (current) frame.  */

+extern void lookup_selected_frame (frame_id 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

--- a/gdb/gdbthread.h

+++ b/gdb/gdbthread.h

@@ -673,6 +673,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/infrun.c b/gdb/infrun.c

--- a/gdb/infrun.c

+++ b/gdb/infrun.c

@@ -9006,8 +9006,10 @@ 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.  */

+  /* ID and level of the selected frame when the inferior function

+     call was made.  */

   struct frame_id selected_frame_id {};

+  int selected_frame_level = -1;

 };

 /* Save all of the information associated with the inferior<==>gdb

@@ -9036,27 +9038,12 @@ struct 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));

+  save_selected_frame (&inf_status->selected_frame_id,

+		       &inf_status->selected_frame_level);

   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

@@ -9084,21 +9071,8 @@ struct infcall_control_state

   if (target_has_stack)

     {

-      /* The point of the try/catch is that if the stack is clobbered,

-         walking the stack might encounter a garbage pointer and

-         error() trying to dereference it.  */

-      try

-	{

-	  restore_selected_frame (inf_status->selected_frame_id);

-	}

-      catch (const gdb_exception_error &ex)

-	{

-	  exception_fprintf (gdb_stderr, ex,

-			     "Unable to restore previously selected frame:\n");

-	  /* Error in restoring the selected frame.  Select the

-	     innermost frame.  */

-	  select_frame (get_current_frame ());

-	}

+      restore_selected_frame (inf_status->selected_frame_id,

+			      inf_status->selected_frame_level);

     }

   delete inf_status;

diff --git a/gdb/stack.c b/gdb/stack.c

--- a/gdb/stack.c

+++ b/gdb/stack.c

@@ -1842,9 +1842,9 @@ struct symtab*

 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 ();

   select_frame (fi);

-  if (get_selected_frame_if_set () != prev_frame)

+  if (get_selected_frame () != prev_frame)

     gdb::observers::user_selected_context_changed.notify (USER_SELECTED_FRAME);

 }

@@ -1863,10 +1863,9 @@ struct symtab*

 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 ();

   select_frame (fi);

-  if (get_selected_frame_if_set () != prev_frame)

+  if (get_selected_frame () != 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

--- a/gdb/thread.c

+++ b/gdb/thread.c

@@ -1325,20 +1325,26 @@ struct info_threads_opts

   switch_to_thread (thr);

 }

-static void

-restore_selected_frame (struct frame_id a_frame_id, int frame_level)

+/* See frame.h.  */

+

+void

+lookup_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 current frame.  In either case, select 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

@@ -1409,64 +1415,28 @@ struct info_threads_opts

       && target_has_stack

       && target_has_memory)

     restore_selected_frame (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 ();

 }

 scoped_restore_current_thread::scoped_restore_current_thread ()

 {

   m_inf = inferior_ref::new_reference (current_inferior ());

+  m_lang = current_language->la_language;

+

   if (inferior_ptid != null_ptid)

     {

       m_thread = thread_info_ref::new_reference (inferior_thread ());

-      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;

-	}

+      save_selected_frame (&m_selected_frame_id, &m_selected_frame_level);

     }

 }