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

[Pedro Alves <pedro@palves.net>]

On 7/9/20 4:40 PM, Simon Marchi wrote:
>> I've also polished the patch some more.  I now renamed
>> the current restore_selected_frame to lookup_selected_frame,
>> to give space to the new save_selected_frame/restore_selected_frame
>> pair.  select_frame_lazy is now restore_selected_frame.
>> save_selected_frame/restore_selected_frame are now noexcept, and
>> their intro comments explain why.
> 
> I noticed there is also a `restore_selected_frame` in infrun.c... I don't
> know if it's replicating features also implemented in frame.c?

Yeah, I saw that too, but didn't look too deeply, given I was more
looking at validating the whole idea.  I think that can be replaced,
yes.  Done now.

BTW, I don't know why I missed it before, but we _already_ don't
warn the user if we fail to restore frame #0:

  /* Warn the user.  */
  if (frame_level > 0 && !current_uiout->is_mi_like_p ())
      ^^^^^^^^^^^^^^^
    {

And that came in with:

 commit 6c95b8df7fef5273da71c34775918c554aae0ea8
 Author:     Pedro Alves <palves@redhat.com>
 AuthorDate: Mon Oct 19 09:51:43 2009 +0000

    2009-10-19  Pedro Alves  <pedro@codesourcery.com>
                Stan Shebs  <stan@codesourcery.com>
    
            Add base multi-executable/process support to GDB.

I'm pretty sure I wrote that bit of the patch...

> 
>> @@ -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.  */
> 
> Pedantically, the `otherwise` could let the reader think that if
> SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are not null_ptid / -1, then there
> is no selected frame.  I'd suggest moving this out to its own sentence to be
> clear.
> 
> I'd also make it more explicit here that when the innermost frame is selected,
> SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL.  Currently, it says that if
> SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_ptid / -1, it means the
> innermost frame is selected, but it doesn't imply that if the innermost frame
> is selected, then SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_ptid / -1
> (difference between if and "if and only if").
> 
> I think it would help to separate that in paragraphes to ease reading.
> 
> Also, I just noticed that you comment uses `null_ptid` instead of `null_frame_id`,
> which just shows how much `null_ptid` is engraved in your brain :).  I also
> re-worked the comment for 15 minutes before noticing :).
> 
> So:
> 
> /* 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, the selected, then there's no selected
>    frame.  */

I like it.

> 
>> +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;
> 
> Ah ok, in my previous reply I almost said: "so this essentially becomes
> a cache for selected_frame_id/selected_frame_level", but changed it because
> I *thought* I had understood that it wasn't the case when the frame_level
> was > 0.  But if we can word it this way, that makes it simpler to understand.
> 
>>  
>> +/* 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
>> +{
>> +  /* get_selected_frame_info never returns level == 0, so we shouldn't
>> +     see it here either.  */
>> +  gdb_assert (frame_level != 0);
> 
> We could also assert that a_frame_id can be null_frame_id IFF frame_level is -1.

Done.

> 
>> +
>> +  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);
> 
> Could you fix (in this patch or another one) the comment of `get_selected_frame`
> to be `/* See frame.h.  */` and make sure that the version in frame.h is the
> most up to date one?

Done.

> 
>> @@ -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
> 
> restore_selected_frame

Fixed.

> 
>> @@ -329,13 +335,33 @@ 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);
>> -
>> -/* 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 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);
> 
> As I mentioned above, I think the comments would be easier to read with
> a bit more newlines.  In general, I like to have one short sentence by
> itself first, that says what the function does at a very high level.  A
> bit like in man pages you have a short description next to the name:
> 
>   rm - remove files or directories
>   sudo, sudoedit — execute a command as another user
>   ssh — OpenSSH remote login client
> 
> Then, you can go in details about the behavior of the function in following
> paragraphs, making sure to split different ideas in different paragraphs.
> 
> At first, I thought it would just be nitpicking to ask people to space
> out their comments and code a bit more, but now I think it really does
> make a difference in readability (at least, it helps me).
> 

Done.

>> 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)
> 
> I'm telling people that we try to use `nullptr` for new code.  I don't
> know what's your opinion on this.  I would just like to have a simple
> and clear so we don't have to wonder which of `NULL` and `nullptr` to
> use.

I just tend to use nullptr for new code, unless the surrounding code
is already using NULL.  In this case I just copy/pasted from elsewhere
and didn't even realize it.  NULL and nullptr are both cyan and thus
look the same on my emacs.  :-)

I do tend to stick with writing "NULL" in comments.

> 
>>      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)
> 
> ... especially that you've used `nullptr` here :).
> 
> Although maybe that in this case, get_selected_frame could have a default
> parameter value of `nullptr`.
> 

Done.

>> diff --git a/gdb/thread.c b/gdb/thread.c
>> index a3c2be7dd0..e0b49abf0c 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
>> -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 innermost (the current frame).  */
> 
> It's a bit heavy to always precise that `innermost` means the `current`
> frame.  Let's choose one name and stick to it, it will be clear enough
> on its own.  I'd use `current`, since that's how the functions are named
> (e.g. get_current_frame).  I understand there might be a bit confusion
> between `selected` and `current` for newcomers, but once you know the
> distinction, it's pretty clear.

Sure.

> 
> I'd also add to that comment, what is the action we take as a result.
> 
> So, maybe:
> 
>   /* This either means there was no selected frame, or the selected frame was
>      the current one.  In either case, try to select the current frame.  */

OK.  I'll remove the "try" though.

> 
> 
>> @@ -1409,22 +1414,14 @@ scoped_restore_current_thread::restore ()
>>        && 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 ();
> 
> I don't know if it would be worth it, but I'd like if we could assert (abort
> GDB) if an exception does try to exit the destructor.  The `restore` method
> is non-trivial and calls into other non-trivial functions, so it would be
> possible for a change far far away to cause that to happen.

It will already abort.  Destructors are noexcept by default, so if an exception
escapes a destructor, std::terminate() is called, and that calls abort by default.

> 
> What do you think of keeping the try/catch, but using `gdb_assert_not_reached`
> in it?

Not sure.  If we do that, we do get a nicer error message.  However if the user
says "n" to "Quit this debugging session" we still abort.

/home/pedro/brno/pedro/gdb/binutils-gdb-2/build/../src/gdb/thread.c:1441: internal-error: scoped_restore_current_thread::~scoped_restore_current_thread(): unexpected exception thrown from destructor: hello
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n) n

This is a bug, please report it.  For instructions, see:
<https://www.gnu.org/software/gdb/bugs/>.

/home/pedro/brno/pedro/gdb/binutils-gdb-2/build/../src/gdb/thread.c:1441: internal-error: scoped_restore_current_thread::~scoped_restore_current_thread(): unexpected exception thrown from destructor: hello
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Create a core file of GDB? (y or n) n
terminate called after throwing an instance of 'gdb_exception_quit'
Aborted (core dumped)

Maybe it would be interesting to add a variant of internal_error that did
not throw a quit, so the user could swallow the exception...  Maybe consider
wrapping that as a generic facility to add to all non-trivial RAII destructors
we have?  Like a function that takes a function_view as parameter, so
we would write:

foo::~foo ()
{
  safe_dtor (__FILE__, __LINE__, [&] () 
    {
      restore ();
    });
}

Even better, add a SAFE_DTOR macro using similar magic SCOPE_EXIT
macro uses to be able to write:

foo::~foo ()
{
  SAFE_DTOR { restore (); };
}

Here's the current version of the patch.

From 63310d189bb2516e0fec2b0922041388a5a96374 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Thu, 9 Jul 2020 18:26:15 +0100
Subject: [PATCH] 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.
---
 gdb/blockframe.c |   6 +--
 gdb/frame.c      | 117 +++++++++++++++++++++++++++++++++++++++++++------------
 gdb/frame.h      |  53 +++++++++++++++++++------
 gdb/gdbthread.h  |   4 ++
 gdb/infrun.c     |  40 ++++---------------
 gdb/stack.c      |   9 ++---
 gdb/thread.c     |  64 ++++++++----------------------
 7 files changed, 168 insertions(+), 125 deletions(-)

diff --git a/gdb/blockframe.c b/gdb/blockframe.c
index 05c26bc2c2..0b868d8341 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 ();
   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..86cf9e4b69 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,63 @@ 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_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_seleted_frame.  */
+  selected_frame = nullptr;
+}
+
 int
 has_stack_frames (void)
 {
@@ -1671,9 +1722,7 @@ has_stack_frames (void)
   return 1;
 }
 
-/* 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)
@@ -1682,24 +1731,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 +1751,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.  */
 
diff --git a/gdb/frame.h b/gdb/frame.h
index e835d49f9c..4901de1bf5 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -181,8 +181,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.  */
@@ -318,24 +324,49 @@ extern int has_stack_frames (void);
    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,
-   otherwize use a generic 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
index ab5771fdb4..da20dd4b4e 100644
--- 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
index 31266109a6..ad16385029 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -9266,8 +9266,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
@@ -9296,27 +9298,12 @@ 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));
+  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
@@ -9344,21 +9331,8 @@ restore_infcall_control_state (struct infcall_control_state *inf_status)
 
   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
index 265e764dc2..dbc02f5ff0 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 ();
   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);
 }
 
@@ -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 ();
   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
index 4dce1ef82a..32f2ccdbd6 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1325,20 +1325,26 @@ 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)
+/* 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 @@ scoped_restore_current_thread::restore ()
       && 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);
     }
 }
 

base-commit: ad8464f799a4c96c7ab8bdfec3f95846cf54f9b0
prerequisite-patch-id: 32ffdda7d7d774bc4df88bf848bcb796559b53ce
prerequisite-patch-id: 02021b74355b70debd344a6e445285c67dfef7d6
prerequisite-patch-id: c87fcf5a54f6805967cbf8ab107606c57d9ecf52
prerequisite-patch-id: ac7dee583d0ffa519c9d1cd89d27664bca68d8c1
prerequisite-patch-id: eac59ae2ea85d2d51e5be1b03e88a5641cc12c22
prerequisite-patch-id: 13da42ad04dc8e2e3bd6a556a0be0e17cf23669b
prerequisite-patch-id: fd3f09fdb58ddc1c595ea014716851f4c8fca48c
prerequisite-patch-id: 8230262cb24020a5b37e915843180e940807ba1f
prerequisite-patch-id: b06932368faf983af6404ae711dfed17e5e32c6f
-- 
2.14.5