[pushed] Move lookup_selected_frame to frame.c

Message ID f7207c40-e0a2-b7c8-0907-2cce207306b7@palves.net
State New
Headers show
Series
  • [pushed] Move lookup_selected_frame to frame.c
Related show

Commit Message

Pedro Alves Oct. 30, 2020, 1:37 a.m.
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(-)


base-commit: 79952e69634bd02029e79676a38915de60052e89
-- 
2.14.5

Patch

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