[RFA] Remove cleanup from frame_prepare_for_sniffer

[RFA] Remove cleanup from frame_prepare_for_sniffer

[Tom Tromey]

Currently frame_prepare_for_sniffer returns a cleanup.  This patch
changes it to return void, and exposes frame_cleanup_after_sniffer to
the caller.

Normally I would write an RAII class for this sort of thing; but
because there was just a single caller of frame_prepare_for_sniffer,
and because this caller is already using try/catch, I thought it
seemed ok to require explicit calls in this instance.

Regression tested by the buildbot.

gdb/ChangeLog
2017-10-08  Tom Tromey  <tom@tromey.com>

	* frame-unwind.c (frame_unwind_try_unwinder): Update.
	* frame.h (frame_cleanup_after_sniffer): Declare.
	(frame_prepare_for_sniffer): Return void.
	* frame.c (frame_cleanup_after_sniffer): No longer static.  Change
	type of argument.
	(frame_prepare_for_sniffer): Return void.
---
 gdb/ChangeLog      |  9 +++++++++
 gdb/frame-unwind.c | 12 ++++--------
 gdb/frame.c        | 13 +++++--------
 gdb/frame.h        | 13 +++++++++----
 4 files changed, 27 insertions(+), 20 deletions(-)

diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
index 096631c58b..ede226abb0 100644
--- a/gdb/frame-unwind.c
+++ b/gdb/frame-unwind.c
@@ -97,10 +97,9 @@ static int
 frame_unwind_try_unwinder (struct frame_info *this_frame, void **this_cache,
                           const struct frame_unwind *unwinder)
 {
-  struct cleanup *old_cleanup;
   int res = 0;
 
-  old_cleanup = frame_prepare_for_sniffer (this_frame, unwinder);
+  frame_prepare_for_sniffer (this_frame, unwinder);
 
   TRY
     {
@@ -117,7 +116,7 @@ frame_unwind_try_unwinder (struct frame_info *this_frame, void **this_cache,
 	     thus most unwinders aren't able to determine if they're
 	     the best fit.  Keep trying.  Fallback prologue unwinders
 	     should always accept the frame.  */
-	  do_cleanups (old_cleanup);
+	  frame_cleanup_after_sniffer (this_frame);
 	  return 0;
 	}
       throw_exception (ex);
@@ -125,15 +124,12 @@ frame_unwind_try_unwinder (struct frame_info *this_frame, void **this_cache,
   END_CATCH
 
   if (res)
-    {
-      discard_cleanups (old_cleanup);
-      return 1;
-    }
+    return 1;
   else
     {
       /* Don't set *THIS_CACHE to NULL here, because sniffer has to do
 	 so.  */
-      do_cleanups (old_cleanup);
+      frame_cleanup_after_sniffer (this_frame);
       return 0;
     }
   gdb_assert_not_reached ("frame_unwind_try_unwinder");
diff --git a/gdb/frame.c b/gdb/frame.c
index afdc157ebd..17f7397aee 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -2864,11 +2864,9 @@ frame_stop_reason_symbol_string (enum unwind_stop_reason reason)
 /* Clean up after a failed (wrong unwinder) attempt to unwind past
    FRAME.  */
 
-static void
-frame_cleanup_after_sniffer (void *arg)
+void
+frame_cleanup_after_sniffer (struct frame_info *frame)
 {
-  struct frame_info *frame = (struct frame_info *) arg;
-
   /* The sniffer should not allocate a prologue cache if it did not
      match this frame.  */
   gdb_assert (frame->prologue_cache == NULL);
@@ -2893,16 +2891,15 @@ frame_cleanup_after_sniffer (void *arg)
 }
 
 /* Set FRAME's unwinder temporarily, so that we can call a sniffer.
-   Return a cleanup which should be called if unwinding fails, and
-   discarded if it succeeds.  */
+   If sniffing fails, the caller should be sure to call
+   frame_cleanup_after_sniffer.  */
 
-struct cleanup *
+void
 frame_prepare_for_sniffer (struct frame_info *frame,
 			   const struct frame_unwind *unwind)
 {
   gdb_assert (frame->unwind == NULL);
   frame->unwind = unwind;
-  return make_cleanup (frame_cleanup_after_sniffer, frame);
 }
 
 static struct cmd_list_element *set_backtrace_cmdlist;
diff --git a/gdb/frame.h b/gdb/frame.h
index 1b726108b9..190ce7623f 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -791,11 +791,16 @@ extern void info_locals_command (char *, int);
 extern void return_command (char *, int);
 
 /* Set FRAME's unwinder temporarily, so that we can call a sniffer.
-   Return a cleanup which should be called if unwinding fails, and
-   discarded if it succeeds.  */
+   If sniffing fails, the caller should be sure to call
+   frame_cleanup_after_sniffer.  */
 
-struct cleanup *frame_prepare_for_sniffer (struct frame_info *frame,
-					   const struct frame_unwind *unwind);
+extern void frame_prepare_for_sniffer (struct frame_info *frame,
+				       const struct frame_unwind *unwind);
+
+/* Clean up after a failed (wrong unwinder) attempt to unwind past
+   FRAME.  */
+
+extern void frame_cleanup_after_sniffer (struct frame_info *frame);
 
 /* Notes (cagney/2002-11-27, drow/2003-09-06):
 
-- 
2.13.6

Re: [RFA] Remove cleanup from frame_prepare_for_sniffer

[Simon Marchi]

On 2017-10-08 06:52 PM, Tom Tromey wrote:
> Currently frame_prepare_for_sniffer returns a cleanup.  This patch
> changes it to return void, and exposes frame_cleanup_after_sniffer to
> the caller.
> 
> Normally I would write an RAII class for this sort of thing; but
> because there was just a single caller of frame_prepare_for_sniffer,
> and because this caller is already using try/catch, I thought it
> seemed ok to require explicit calls in this instance.
> 
> Regression tested by the buildbot.

I think it makes sense, LGTM.

[PATCH] Fix GDB crash after Quit thrown from unwinder sniffer (Re: [RFA] Remove cleanup from frame_prepare_for_sniffer)

[Pedro Alves]

Hi Tom,

On 10/08/2017 11:52 PM, Tom Tromey wrote:
> Currently frame_prepare_for_sniffer returns a cleanup.  This patch
> changes it to return void, and exposes frame_cleanup_after_sniffer to
> the caller.
> 
> Normally I would write an RAII class for this sort of thing; but
> because there was just a single caller of frame_prepare_for_sniffer,
> and because this caller is already using try/catch, I thought it
> seemed ok to require explicit calls in this instance.
> 
> Regression tested by the buildbot.
> 
> gdb/ChangeLog
> 2017-10-08  Tom Tromey  <tom@tromey.com>
> 
> 	* frame-unwind.c (frame_unwind_try_unwinder): Update.
> 	* frame.h (frame_cleanup_after_sniffer): Declare.
> 	(frame_prepare_for_sniffer): Return void.
> 	* frame.c (frame_cleanup_after_sniffer): No longer static.  Change
> 	type of argument.
> 	(frame_prepare_for_sniffer): Return void.

I think this caused a regression.  See fix below.

WDYT?

From 8db37aa3966407c9190820a75e23a75688af9f95 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 14 Feb 2018 15:11:58 +0000
Subject: [PATCH] Fix GDB crash after Quit thrown from unwinder sniffer

I ran into a GDB crash in gdb.base/bp-cmds-continue-ctrl-c.exp in my
multi-target branch, which turns out exposed a bug that exists in
master too.

That testcase has a breakpoint with a "continue" command associated.
Then the breakpoint is constantly being hit.  At the same time, the
testcase is continualy interrupting the program with Ctrl-C, and
re-resuming it, in a loop.

Running that testcase manually under Valgrind, after a few sequences
of 'Ctrl-C' + 'continue', I got:

 Breakpoint 1, Quit
 (gdb) ==21270== Invalid read of size 8
 ==21270==    at 0x4D8185: pyuw_this_id(frame_info*, void**, frame_id*) (py-unwind.c:461)
 ==21270==    by 0x6D426A: compute_frame_id(frame_info*) (frame.c:505)
 ==21270==    by 0x6D43B7: get_frame_id(frame_info*) (frame.c:537)
 ==21270==    by 0x84F3B8: scoped_restore_current_thread::scoped_restore_current_thread() (thread.c:1678)
 ==21270==    by 0x718E3D: fetch_inferior_event(void*) (infrun.c:4076)
 ==21270==    by 0x7067C9: inferior_event_handler(inferior_event_type, void*) (inf-loop.c:43)
 ==21270==    by 0x45BEF9: handle_target_event(int, void*) (linux-nat.c:4419)
 ==21270==    by 0x6C4255: handle_file_event(file_handler*, int) (event-loop.c:733)
 ==21270==    by 0x6C47F8: gdb_wait_for_event(int) (event-loop.c:859)
 ==21270==    by 0x6C3666: gdb_do_one_event() (event-loop.c:322)
 ==21270==    by 0x6C3712: start_event_loop() (event-loop.c:371)
 ==21270==    by 0x746801: captured_command_loop() (main.c:329)
 ==21270==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
 ==21270==
 ==21270==
 ==21270== Process terminating with default action of signal 11 (SIGSEGV): dumping core
 ==21270==  Access not within mapped region at address 0x0
 ==21270==    at 0x4D8185: pyuw_this_id(frame_info*, void**, frame_id*) (py-unwind.c:461)
 ==21270==    by 0x6D426A: compute_frame_id(frame_info*) (frame.c:505)
 ==21270==    by 0x6D43B7: get_frame_id(frame_info*) (frame.c:537)
 ==21270==    by 0x84F3B8: scoped_restore_current_thread::scoped_restore_current_thread() (thread.c:1678)
 ==21270==    by 0x718E3D: fetch_inferior_event(void*) (infrun.c:4076)
 ==21270==    by 0x7067C9: inferior_event_handler(inferior_event_type, void*) (inf-loop.c:43)
 ==21270==    by 0x45BEF9: handle_target_event(int, void*) (linux-nat.c:4419)
 ==21270==    by 0x6C4255: handle_file_event(file_handler*, int) (event-loop.c:733)
 ==21270==    by 0x6C47F8: gdb_wait_for_event(int) (event-loop.c:859)
 ==21270==    by 0x6C3666: gdb_do_one_event() (event-loop.c:322)
 ==21270==    by 0x6C3712: start_event_loop() (event-loop.c:371)
 ==21270==    by 0x746801: captured_command_loop() (main.c:329)
 ==21270==  If you believe this happened as a result of a stack
 ==21270==  overflow in your program's main thread (unlikely but
 ==21270==  possible), you can try to increase the size of the
 ==21270==  main thread stack using the --main-stacksize= flag.
 ==21270==  The main thread stack size used in this run was 8388608.
 ==21270==

Above, when we get to compute_frame_id, fi->unwind is non-NULL,
meaning, we found an unwinder, in this case the Python unwinder, but
somehow, fi->prologue_cache is left NULL.  pyuw_this_id then crashes
because it assumes fi->prologue_cache is non-NULL:

  static void
  pyuw_this_id (struct frame_info *this_frame, void **cache_ptr,
		struct frame_id *this_id)
  {
    *this_id = ((cached_frame_info *) *cache_ptr)->frame_id;
                                      ^^^^^^^^^^

'*cache_ptr' here is 'fi->prologue_cache'.

There's a quit() call in pyuw_sniffer that I believe is the one that
sometimes triggers the crash above.  The crash can be reproduced
easily with this hack to force a quit out of the python unwinder:

 --- a/gdb/python/py-unwind.c
 +++ b/gdb/python/py-unwind.c
 @@ -497,6 +497,8 @@ pyuw_sniffer (const struct frame_unwind *self, struct frame_info *this_frame,
    struct gdbarch *gdbarch = (struct gdbarch *) (self->unwind_data);
    cached_frame_info *cached_frame;

 +  quit ();
 +
    gdbpy_enter enter_py (gdbarch, current_language);

    TRACE_PY_UNWIND (3, "%s (SP=%s, PC=%s)\n", __FUNCTION__,

After that quit is thrown, any subsequent operation that involves
unwinding results in GDB crashing with SIGSEGV like above.

The problem is that this commit:

  commit 30a9c02feff56bd58a276c2a7262f364baa558ac
  CommitDate: Sun Oct 8 23:16:42 2017 -0600
  Subject: Remove cleanup from frame_prepare_for_sniffer

missed that we need to call frame_cleanup_after_sniffer before
rethrowing the exception too.

Without the fix, the "bt" added to
gdb.base/bp-cmds-continue-ctrl-c.exp in this commit makes GDB crash:

  Running src/gdb/testsuite/gdb.base/bp-cmds-continue-ctrl-c.exp ...
  ERROR: Process no longer exists

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* frame-unwind.c (frame_unwind_try_unwinder): Always call
	frame_cleanup_after_sniffer on exception.

gdb/testsuite/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* gdb.base/bp-cmds-continue-ctrl-c.exp (do_test): Test "bt" after
	getting a "Quit".
---
 gdb/frame-unwind.c                                 |  3 ++-
 gdb/testsuite/gdb.base/bp-cmds-continue-ctrl-c.exp | 13 +++++++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
index 66a28ae1c9..e6e63539ad 100644
--- a/gdb/frame-unwind.c
+++ b/gdb/frame-unwind.c
@@ -110,13 +110,14 @@ frame_unwind_try_unwinder (struct frame_info *this_frame, void **this_cache,
       /* Catch all exceptions, caused by either interrupt or error.
 	 Reset *THIS_CACHE.  */
       *this_cache = NULL;
+      frame_cleanup_after_sniffer (this_frame);
+
       if (ex.error == NOT_AVAILABLE_ERROR)
 	{
 	  /* This usually means that not even the PC is available,
 	     thus most unwinders aren't able to determine if they're
 	     the best fit.  Keep trying.  Fallback prologue unwinders
 	     should always accept the frame.  */
-	  frame_cleanup_after_sniffer (this_frame);
 	  return 0;
 	}
       throw_exception (ex);
diff --git a/gdb/testsuite/gdb.base/bp-cmds-continue-ctrl-c.exp b/gdb/testsuite/gdb.base/bp-cmds-continue-ctrl-c.exp
index 8c2fa77d71..43600fcf81 100644
--- a/gdb/testsuite/gdb.base/bp-cmds-continue-ctrl-c.exp
+++ b/gdb/testsuite/gdb.base/bp-cmds-continue-ctrl-c.exp
@@ -89,6 +89,19 @@ proc do_test {} {
 	    }
 	    -re "Quit\r\n$gdb_prompt $" {
 		send_log "$internal_pass (Quit)\n"
+
+		# Check that if we managed to quit somewhere deep in
+		# the unwinders, we can still unwind again.
+		set ok 0
+		gdb_test_multiple "bt" "$internal_pass (bt)" {
+		    -re "#0.*$gdb_prompt $" {
+			send_log "$internal_pass (bt)\n"
+			set ok 1
+		    }
+		}
+		if {!$ok} {
+		    return
+		}
 	    }
 	    -re "Quit\r\n\r\nCommand aborted.\r\n$gdb_prompt $" {
 		send_log "$internal_pass (Command aborted)\n"
-- 
2.14.3

Re: [PATCH] Fix GDB crash after Quit thrown from unwinder sniffer (Re: [RFA] Remove cleanup from frame_prepare_for_sniffer)

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> I think this caused a regression.  See fix below.
Pedro> WDYT?

Yes, this makes sense to me.  I missed that re-throwing would run the
cleanup that was installed.  Thank you.

Tom