[pushed] Move lookup_selected_frame to frame.c

[Pedro Alves <pedro@palves.net>]

On 10/30/20 1:13 AM, Pedro Alves wrote:
> On 7/10/20 3:55 AM, Simon Marchi wrote:
>>>> 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.
>>
>> Oh, didn't know that!  I thought it was just "undefined behavior".
>>
>>>> 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 (); };
>>> }
>>
>> That's fancier than what I hoped for :).  My goal was just to make sure
>> we catch it if we ever make a change that causes an exception to escape.
>> Although I wouldn't be against what you proposed.
>>
>>> Here's the current version of the patch.
>>
>> That looks fine to me.
> 
> FYI, I've finally merged this to master.
> 

I've merged this obvious follow up patch as well.

From d70bdd3cc4cfdfd30bed44a271ff80f98b96eebf Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 9 Jul 2020 20:12:15 +0100
Subject: [PATCH] Move lookup_selected_frame to frame.c

This function is now external, and isn't really threads related.  Move
it to frame.c.

gdb/ChangeLog:

	* thread.c (lookup_selected_frame): Move ...
	* frame.c (lookup_selected_frame): ... here.

Change-Id: Ia96b79c15767337c68efd3358bcc715ce8e26c15
---
 gdb/ChangeLog |  5 +++++
 gdb/frame.c   | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/thread.c  | 63 --------------------------------------------------------
 3 files changed, 71 insertions(+), 63 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 978576ac7c..d2df843b24 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2020-10-30  Pedro Alves  <pedro@palves.net>
+
+	* thread.c (lookup_selected_frame): Move ...
+	* frame.c (lookup_selected_frame): ... here.
+
 2020-10-30  Pedro Alves  <pedro@palves.net>
 
 	* blockframe.c (block_innermost_frame): Use get_selected_frame.
diff --git a/gdb/frame.c b/gdb/frame.c
index bb835e2dba..d0a4ce4d63 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1740,6 +1740,72 @@ restore_selected_frame (frame_id frame_id, int frame_level)
   selected_frame = nullptr;
 }
 
+/* See frame.h.  */
+
+void
+lookup_selected_frame (struct frame_id a_frame_id, int frame_level)
+{
+  struct frame_info *frame = NULL;
+  int count;
+
+  /* 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 (get_current_frame ());
+      return;
+    }
+
+  /* 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
+     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);
+    }
+}
+
 bool
 has_stack_frames ()
 {
diff --git a/gdb/thread.c b/gdb/thread.c
index 193f9d4c44..32d14e8662 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1327,69 +1327,6 @@ switch_to_thread (process_stratum_target *proc_target, ptid_t ptid)
 
 /* See frame.h.  */
 
-void
-lookup_selected_frame (struct frame_id a_frame_id, int frame_level)
-{
-  struct frame_info *frame = NULL;
-  int count;
-
-  /* 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 (get_current_frame ());
-      return;
-    }
-
-  /* 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
-     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 ()
 {

base-commit: 79952e69634bd02029e79676a38915de60052e89
-- 
2.14.5