Bail out of processing stop if hook-stop resumes target / changes context

Message ID 1439836415-22008-1-git-send-email-palves@redhat.com
State New, archived
Headers

Commit Message

Pedro Alves Aug. 17, 2015, 6:33 p.m. UTC
  This patch, relative to a tree with
https://sourceware.org/ml/gdb-patches/2015-08/msg00295.html, fixes
issues/crashes that trigger if something unexpected happens during a
hook-stop.

E.g., if the inferior disappears while running the hook-stop, we hit
failed assertions:

 (gdb) define hook-stop
 Type commands for definition of "hook-stop".
 End with a line saying just "end".
 >kill
 >end
 (gdb) si
 Kill the program being debugged? (y or n) [answered Y; input not from terminal]
 /home/pedro/gdb/mygit/build/../src/gdb/thread.c:88: internal-error: inferior_thread: Assertion `tp' failed.
 A problem internal to GDB has been detected,
 further debugging may prove unreliable.
 Quit this debugging session? (y or n)

I noticed that if a hook-stop issues a synchronous execution command,
we print the same stop event twice:

 (gdb) define hook-stop
 Type commands for definition of "hook-stop".
 End with a line saying just "end".
 >si
 >end
 (gdb) si
 0x000000000040074a      42          args[i] = 1; /* Init value.  */  <<<<<<< once
 0x000000000040074a      42          args[i] = 1; /* Init value.  */  <<<<<<< twice
 (gdb)

In MI:

 *stopped,reason="end-stepping-range",frame={addr="0x000000000040074a",func="main",args=[],file="threads.c",fullname="/home/pedro/gdb/tests/threads.c",line="42"},thread-id="1",stopped-threads="all",core="0"
 *stopped,reason="end-stepping-range",frame={addr="0x000000000040074a",func="main",args=[],file="threads.c",fullname="/home/pedro/gdb/tests/threads.c",line="42"},thread-id="1",stopped-threads="all",core="0"
 (gdb)

The fix has GDB stop processing the event if the context changed.  I
don't expect people to be doing crazy things from the hook-stop.
E.g., it gives me headaches to try to come up a proper behavior for
handling a thread change from a hook-stop... (E.g., imagine the
hook-stop does thread N; step, with scheduler-locing on).  I think the
most important bit here is preventing crashes.

The patch adds a new hook-stop.exp test that covers the above and also
merges in the old hook-stop-continue.exp and hook-stop-frame.exp into
the same framework.

gdb/ChangeLog:
2015-08-17  Pedro Alves  <palves@redhat.com>

	* infrun.c (current_stop_id): New global.
	(get_stop_id, new_stop_id): New functions.
	(fetch_inferior_event): Handle normal_stop proceeding the target.
	(struct stop_context): New.
	(save_stop_context, release_stop_context_cleanup)
	(stop_context_changed): New functions.
	(normal_stop): Return true if the hook-stop changes the stop
	context.
	* infrun.h (get_stop_id): Declare.
	(normal_stop): Now returns int.  Add documentation.

gdb/testsuite/ChangeLog:
2015-08-17  Pedro Alves  <palves@redhat.com>

	* gdb.base/hook-stop-continue.c: Delete.
	* gdb.base/hook-stop-continue.exp: Delete.
	* gdb.base/hook-stop-frame.c: Delete.
	* gdb.base/hook-stop-frame.exp: Delete.
	* gdb.base/hook-stop.c: New file.
	* gdb.base/hook-stop.exp: New file.
---
 gdb/infrun.c                                       | 148 ++++++++++++++++--
 gdb/infrun.h                                       |  11 +-
 gdb/testsuite/gdb.base/hook-stop-continue.c        |  42 ------
 gdb/testsuite/gdb.base/hook-stop-continue.exp      |  52 -------
 gdb/testsuite/gdb.base/hook-stop-frame.exp         |  48 ------
 .../gdb.base/{hook-stop-frame.c => hook-stop.c}    |   6 +
 gdb/testsuite/gdb.base/hook-stop.exp               | 168 +++++++++++++++++++++
 7 files changed, 319 insertions(+), 156 deletions(-)
 delete mode 100644 gdb/testsuite/gdb.base/hook-stop-continue.c
 delete mode 100644 gdb/testsuite/gdb.base/hook-stop-continue.exp
 delete mode 100644 gdb/testsuite/gdb.base/hook-stop-frame.exp
 rename gdb/testsuite/gdb.base/{hook-stop-frame.c => hook-stop.c} (95%)
 create mode 100644 gdb/testsuite/gdb.base/hook-stop.exp
  

Comments

Yao Qi Aug. 19, 2015, 8:22 a.m. UTC | #1
Pedro Alves <palves@redhat.com> writes:

Hi Pedro,

> -  if (stop_command)
> -    catch_errors (hook_stop_stub, stop_command,
> -		  "Error while running hook_stop:\n", RETURN_MASK_ALL);
> +  if (stop_command != NULL)
> +    {
> +      struct stop_context *saved_context = save_stop_context ();
> +      struct cleanup *old_chain
> +	= make_cleanup (release_stop_context_cleanup, saved_context);
> +
> +      catch_errors (hook_stop_stub, stop_command,
> +		    "Error while running hook_stop:\n", RETURN_MASK_ALL);
> +
> +      /* If the stop hook resumes the target, then there's no point in
> +	 trying to notify about the previous stop; its context is
> +	 gone.  Likewise if the command switches thread or inferior --
> +	 the observers would print a stop for the wrong
> +	 thread/inferior.  */
> +      if (stop_context_changed (saved_context))
> +	{
> +	  do_cleanups (old_chain);
> +	  return 1;
> +	}
> +      do_cleanups (old_chain);
> +    }

I am wondering why don't we let interpreter in async to execute
stop_command, and we simply return here.  In this way, we don't have to
know whether stop_command resumes the target or switches the thread.
Once there is no event from event loop, the target really stops and
hook-stop is already executed.
  
Pedro Alves Aug. 25, 2015, 3:47 p.m. UTC | #2
On 08/19/2015 09:22 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
> Hi Pedro,
> 
>> -  if (stop_command)
>> -    catch_errors (hook_stop_stub, stop_command,
>> -		  "Error while running hook_stop:\n", RETURN_MASK_ALL);
>> +  if (stop_command != NULL)
>> +    {
>> +      struct stop_context *saved_context = save_stop_context ();
>> +      struct cleanup *old_chain
>> +	= make_cleanup (release_stop_context_cleanup, saved_context);
>> +
>> +      catch_errors (hook_stop_stub, stop_command,
>> +		    "Error while running hook_stop:\n", RETURN_MASK_ALL);
>> +
>> +      /* If the stop hook resumes the target, then there's no point in
>> +	 trying to notify about the previous stop; its context is
>> +	 gone.  Likewise if the command switches thread or inferior --
>> +	 the observers would print a stop for the wrong
>> +	 thread/inferior.  */
>> +      if (stop_context_changed (saved_context))
>> +	{
>> +	  do_cleanups (old_chain);
>> +	  return 1;
>> +	}
>> +      do_cleanups (old_chain);
>> +    }
> 
> I am wondering why don't we let interpreter in async to execute
> stop_command, and we simply return here.  In this way, we don't have to
> know whether stop_command resumes the target or switches the thread.
> Once there is no event from event loop, the target really stops and
> hook-stop is already executed.

Not sure I understood the suggestion -- I don't see how that would end
up being different.  If the hook-stop does "continue&", then we still need
to know that the target was resumed.  Likewise if the hook-stop just
does "thread N" and thus switches to another thread -- there's no
execution involved in that case so seems to me interpreter async/sync
makes no difference.

Thanks,
Pedro Alves
  
Yao Qi Aug. 27, 2015, 1:35 p.m. UTC | #3
Pedro Alves <palves@redhat.com> writes:

>>> -  if (stop_command)
>>> -    catch_errors (hook_stop_stub, stop_command,
>>> -		  "Error while running hook_stop:\n", RETURN_MASK_ALL);
>>> +  if (stop_command != NULL)
>>> +    {
>>> +      struct stop_context *saved_context = save_stop_context ();
>>> +      struct cleanup *old_chain
>>> +	= make_cleanup (release_stop_context_cleanup, saved_context);
>>> +
>>> +      catch_errors (hook_stop_stub, stop_command,
>>> +		    "Error while running hook_stop:\n", RETURN_MASK_ALL);
>>> +
>>> +      /* If the stop hook resumes the target, then there's no point in
>>> +	 trying to notify about the previous stop; its context is
>>> +	 gone.  Likewise if the command switches thread or inferior --
>>> +	 the observers would print a stop for the wrong
>>> +	 thread/inferior.  */
>>> +      if (stop_context_changed (saved_context))
>>> +	{
>>> +	  do_cleanups (old_chain);
>>> +	  return 1;
>>> +	}
>>> +      do_cleanups (old_chain);
>>> +    }
>> 
>> I am wondering why don't we let interpreter in async to execute
>> stop_command, and we simply return here.  In this way, we don't have to
>> know whether stop_command resumes the target or switches the thread.
>> Once there is no event from event loop, the target really stops and
>> hook-stop is already executed.
>
> Not sure I understood the suggestion -- I don't see how that would end
> up being different.  If the hook-stop does "continue&", then we still need
> to know that the target was resumed.  Likewise if the hook-stop just
> does "thread N" and thus switches to another thread -- there's no
> execution involved in that case so seems to me interpreter async/sync
> makes no difference.

I was thinking that why do we need stop_id or stop_context here.  We can
let interpreter to execute hook-stop commands in an async way, and GDB
reads events out of event loop, until all events are consumed.  In this
way, do we still need stop_id or stop_context?
  
Pedro Alves Sept. 9, 2015, 7:21 p.m. UTC | #4
Hi Yao,

On 08/27/2015 02:35 PM, Yao Qi wrote:

>>> I am wondering why don't we let interpreter in async to execute
>>> stop_command, and we simply return here.  In this way, we don't have to
>>> know whether stop_command resumes the target or switches the thread.
>>> Once there is no event from event loop, the target really stops and
>>> hook-stop is already executed.
>>
>> Not sure I understood the suggestion -- I don't see how that would end
>> up being different.  If the hook-stop does "continue&", then we still need
>> to know that the target was resumed.  Likewise if the hook-stop just
>> does "thread N" and thus switches to another thread -- there's no
>> execution involved in that case so seems to me interpreter async/sync
>> makes no difference.
> 
> I was thinking that why do we need stop_id or stop_context here.  We can
> let interpreter to execute hook-stop commands in an async way, and GDB
> reads events out of event loop, until all events are consumed.  In this
> way, do we still need stop_id or stop_context?

We don't have infrastructure to run command lists like that.  The
hook-stop might involve several execution commands, for
instance.  See:
 https://sourceware.org/ml/gdb-patches/2011-09/msg00037.html
And what motivated it:
 https://sourceware.org/ml/gdb-patches/2011-06/msg00158.html

I don't currently see any real benefit in splitting interpreter
command execution to a bigger and more complicated state machine
like I was originally attempting.  I've been thinking that longer
term, it'd be simpler/saner if we split run control and the
interpreter to separate threads.  I actually have a prototype
branch that makes it possible to have multiple consoles/CLIs
simultaneously, and there, to make it possible to handle
pagination and queries correctly in all consoles, I made gdb
run each console's interpreter in its own thread.  See
https://github.com/palves/gdb/tree/palves/console, though
warning, lots of hacks in place.  This fix actually comes out
of that branch (the "fix hook-stop" commit [1]), though the fix
there was based on proceed call counts.  I came up with counting
stops instead of proceeds/resumes while trying to clean that up and
writing the test, which uncovered the need for handling thread
switching too etc.

 [1] - https://github.com/palves/gdb/commit/f1505f9a6cf5097c83771d373e512fa9c819aa7a

Thanks,
Pedro Alves
  
Yao Qi Sept. 11, 2015, 2:55 p.m. UTC | #5
Pedro Alves <palves@redhat.com> writes:

> We don't have infrastructure to run command lists like that.  The
> hook-stop might involve several execution commands, for
> instance.  See:
>  https://sourceware.org/ml/gdb-patches/2011-09/msg00037.html
> And what motivated it:
>  https://sourceware.org/ml/gdb-patches/2011-06/msg00158.html

Yeah, I didn't strongly believe we have such infrastructure, so I raised
that question.

>
> I don't currently see any real benefit in splitting interpreter
> command execution to a bigger and more complicated state machine
> like I was originally attempting.  I've been thinking that longer
> term, it'd be simpler/saner if we split run control and the
> interpreter to separate threads.  I actually have a prototype

It sounds natural to move interpreter and run control to separate
threads.  It may be hard to convert single-threaded gdb to a
multi-threaded version.  Note that I am not against this direction.
  
Pedro Alves Sept. 14, 2015, 2:50 p.m. UTC | #6
On 09/11/2015 03:55 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> We don't have infrastructure to run command lists like that.  The
>> hook-stop might involve several execution commands, for
>> instance.  See:
>>  https://sourceware.org/ml/gdb-patches/2011-09/msg00037.html
>> And what motivated it:
>>  https://sourceware.org/ml/gdb-patches/2011-06/msg00158.html
> 
> Yeah, I didn't strongly believe we have such infrastructure, so I raised
> that question.

OK.  I've pushed the patch in as is now.

> 
>>
>> I don't currently see any real benefit in splitting interpreter
>> command execution to a bigger and more complicated state machine
>> like I was originally attempting.  I've been thinking that longer
>> term, it'd be simpler/saner if we split run control and the
>> interpreter to separate threads.  I actually have a prototype
> 
> It sounds natural to move interpreter and run control to separate
> threads.  It may be hard to convert single-threaded gdb to a
> multi-threaded version.  

I took the giant lock approach [1].  I added a big gdb lock around the
event loop's select/poll calls.  That is, at any given time, there's only
really one thread not blocked in select/poll calling into gdb handling
an event.  This gives us a stack per interpreter, which is what I was after.
For true parallelism, my idea was to then break the big lock into
finer-brained locks, incrementally, as needed.  Not unlike e.g. how Linux's
BKL was eliminated.

[1] https://en.wikipedia.org/wiki/Giant_lock

> Note that I am not against this direction.

Great.

Thanks for the review.
  

Patch

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 6206c8a..aaea3bd 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2699,6 +2699,33 @@  resume (enum gdb_signal sig)
 
 /* Proceeding.  */
 
+/* See infrun.h.  */
+
+/* Counter that tracks number of user visible stops.  This can be used
+   to tell whether a command has proceeded the inferior past the
+   current location.  This allows e.g., inferior function calls in
+   breakpoint commands to not interrupt the command list.  When the
+   call finishes successfully, the inferior is standing at the same
+   breakpoint as if nothing happened (and so we don't call
+   normal_stop).  */
+static ULONGEST current_stop_id;
+
+/* See infrun.h.  */
+
+ULONGEST
+get_stop_id (void)
+{
+  return current_stop_id;
+}
+
+/* Called when we report a user visible stop.  */
+
+static void
+new_stop_id (void)
+{
+  current_stop_id++;
+}
+
 /* Clear out all variables saying what to do when inferior is continued.
    First do this, then set the ones you want, then call `proceed'.  */
 
@@ -3851,12 +3878,17 @@  fetch_inferior_event (void *client_data)
 
 	  if (should_notify_stop)
 	    {
+	      int proceeded = 0;
+
 	      /* We may not find an inferior if this was a process exit.  */
 	      if (inf == NULL || inf->control.stop_soon == NO_STOP_QUIETLY)
-		normal_stop ();
+		proceeded = normal_stop ();
 
-	      inferior_event_handler (INF_EXEC_COMPLETE, NULL);
-	      cmd_done = 1;
+	      if (!proceeded)
+		{
+		  inferior_event_handler (INF_EXEC_COMPLETE, NULL);
+		  cmd_done = 1;
+		}
 	    }
 	}
     }
@@ -7822,15 +7854,83 @@  maybe_remove_breakpoints (void)
     }
 }
 
-/* Here to return control to GDB when the inferior stops for real.
-   Print appropriate messages, remove breakpoints, give terminal our modes.
+/* The execution context that just caused a normal stop.  */
+
+struct stop_context
+{
+  /* The stop ID.  */
+  ULONGEST stop_id;
 
-   STOP_PRINT_FRAME nonzero means print the executing frame
-   (pc, function, args, file, line number and line text).
-   BREAKPOINTS_FAILED nonzero means stop was due to error
-   attempting to insert breakpoints.  */
+  /* The event PTID.  */
 
-void
+  ptid_t ptid;
+
+  /* If stopp for a thread event, this is the thread that caused the
+     stop.  */
+  struct thread_info *thread;
+
+  /* The inferior that caused the stop.  */
+  int inf_num;
+};
+
+/* Returns a new stop context.  If stopped for a thread event, this
+   takes a strong reference to the thread.  */
+
+static struct stop_context *
+save_stop_context (void)
+{
+  struct stop_context *sc = xmalloc (sizeof (struct stop_context));
+
+  sc->stop_id = get_stop_id ();
+  sc->ptid = inferior_ptid;
+  sc->inf_num = current_inferior ()->num;
+
+  if (!ptid_equal (inferior_ptid, null_ptid))
+    {
+      /* Take a strong reference so that the thread can't be deleted
+	 yet.  */
+      sc->thread = inferior_thread ();
+      sc->thread->refcount++;
+    }
+  else
+    sc->thread = NULL;
+
+  return sc;
+}
+
+/* Release a stop context previously created with save_stop_context.
+   Releases the strong reference to the thread as well. */
+
+static void
+release_stop_context_cleanup (void *arg)
+{
+  struct stop_context *sc = arg;
+
+  if (sc->thread != NULL)
+    sc->thread->refcount--;
+  xfree (sc);
+}
+
+/* Return true if the current context no longer matches the saved stop
+   context.  */
+
+static int
+stop_context_changed (struct stop_context *prev)
+{
+  if (!ptid_equal (prev->ptid, inferior_ptid))
+    return 1;
+  if (prev->inf_num != current_inferior ()->num)
+    return 1;
+  if (prev->thread != NULL && prev->thread->state != THREAD_STOPPED)
+    return 1;
+  if (get_stop_id () != prev->stop_id)
+    return 1;
+  return 0;
+}
+
+/* See infrun.h.  */
+
+int
 normal_stop (void)
 {
   struct target_waitstatus last;
@@ -7840,6 +7940,8 @@  normal_stop (void)
 
   get_last_target_status (&last_ptid, &last);
 
+  new_stop_id ();
+
   /* If an exception is thrown from this point on, make sure to
      propagate GDB's knowledge of the executing state to the
      frontend/user running state.  A QUIT is an easy exception to see
@@ -7959,9 +8061,27 @@  normal_stop (void)
 
   /* Look up the hook_stop and run it (CLI internally handles problem
      of stop_command's pre-hook not existing).  */
-  if (stop_command)
-    catch_errors (hook_stop_stub, stop_command,
-		  "Error while running hook_stop:\n", RETURN_MASK_ALL);
+  if (stop_command != NULL)
+    {
+      struct stop_context *saved_context = save_stop_context ();
+      struct cleanup *old_chain
+	= make_cleanup (release_stop_context_cleanup, saved_context);
+
+      catch_errors (hook_stop_stub, stop_command,
+		    "Error while running hook_stop:\n", RETURN_MASK_ALL);
+
+      /* If the stop hook resumes the target, then there's no point in
+	 trying to notify about the previous stop; its context is
+	 gone.  Likewise if the command switches thread or inferior --
+	 the observers would print a stop for the wrong
+	 thread/inferior.  */
+      if (stop_context_changed (saved_context))
+	{
+	  do_cleanups (old_chain);
+	  return 1;
+	}
+      do_cleanups (old_chain);
+    }
 
   /* Notify observers about the stop.  This is where the interpreters
      print the stop event.  */
@@ -7986,6 +8106,8 @@  normal_stop (void)
      longer needed.  Keeping those around slows down things linearly.
      Note that this never removes the current inferior.  */
   prune_inferiors ();
+
+  return 0;
 }
 
 static int
diff --git a/gdb/infrun.h b/gdb/infrun.h
index a5d6814..b214645 100644
--- a/gdb/infrun.h
+++ b/gdb/infrun.h
@@ -62,6 +62,11 @@  extern int non_stop;
    starting an inferior.  */
 extern int disable_randomization;
 
+/* Returns a unique identifier for the current stop.  This can be used
+   to tell whether a command has proceeded the inferior past the
+   current location.  */
+extern ULONGEST get_stop_id (void);
+
 /* Reverse execution.  */
 enum exec_direction_kind
   {
@@ -100,7 +105,11 @@  extern ptid_t user_visible_resume_ptid (int step);
 
 extern void wait_for_inferior (void);
 
-extern void normal_stop (void);
+/* Return control to GDB when the inferior stops for real.  Print
+   appropriate messages, remove breakpoints, give terminal our modes,
+   and run the stop hook.  Returns true if the stop hook proceeded the
+   target, false otherwise.  */
+extern int normal_stop (void);
 
 extern void get_last_target_status (ptid_t *ptid,
 				    struct target_waitstatus *status);
diff --git a/gdb/testsuite/gdb.base/hook-stop-continue.c b/gdb/testsuite/gdb.base/hook-stop-continue.c
deleted file mode 100644
index 47090fc..0000000
--- a/gdb/testsuite/gdb.base/hook-stop-continue.c
+++ /dev/null
@@ -1,42 +0,0 @@ 
-/* This testcase is part of GDB, the GNU debugger.
-
-   Copyright 2008-2015 Free Software Foundation, Inc.
-
-   This program is free software; you can redistribute it and/or modify
-   it under the terms of the GNU General Public License as published by
-   the Free Software Foundation; either version 3 of the License, or
-   (at your option) any later version.
-
-   This program is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-   GNU General Public License for more details.
-
-   You should have received a copy of the GNU General Public License
-   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
-
-int
-funbreak (int i)
-{
-  i = i * 2; /* set breakpoint here */
-  i = i + 10;
-  return i;
-}
-
-int
-func (int i)
-{
-  return i * 2;
-}
-
-int
-main (int argc, char **argv, char **envp)
-{
-  func (1);
-  func (2);
-  func (3);
-  func (4);
-  funbreak (5);
-
-  return 0;
-}
diff --git a/gdb/testsuite/gdb.base/hook-stop-continue.exp b/gdb/testsuite/gdb.base/hook-stop-continue.exp
deleted file mode 100644
index 06c2fe6..0000000
--- a/gdb/testsuite/gdb.base/hook-stop-continue.exp
+++ /dev/null
@@ -1,52 +0,0 @@ 
-# Copyright 2008-2015 Free Software Foundation, Inc.
-
-# This program is free software; you can redistribute it and/or modify
-# it under the terms of the GNU General Public License as published by
-# the Free Software Foundation; either version 3 of the License, or
-# (at your option) any later version.
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with this program.  If not, see <http://www.gnu.org/licenses/>.
-
-standard_testfile .c
-
-if { [prepare_for_testing ${testfile}.exp "${testfile}" "${testfile}.c" {debug nowarnings}] } {
-    return -1
-}
-
-if ![runto_main] then {
-    perror "Couldn't run to main"
-}
-
-set bp_location [gdb_get_line_number "set breakpoint here"]
-
-gdb_test "break $bp_location" \
-    "Breakpoint.*at.* file .*$srcfile, line $bp_location\\." \
-    "breakpoint line number"
-
-gdb_test "print \$do_continue = 1" "1"
-
-send_gdb "define hook-stop\n"
-gdb_expect {
-  -re "Type commands for definition of \"hook-stop\".\r\nEnd with a line saying just \"end\".\r\n>$"\
-          {send_gdb "if \$do_continue\nset \$do_continue = 0\ncontinue\nend\nend\n"
-           gdb_expect {
-             -re "$gdb_prompt $"\
-                       {pass "define hook-stop command"}
-             timeout {fail "(timeout) define hook-stop command"}
-           }
-          }
-  -re "$gdb_prompt $"\
-          {fail "define hook-stop command"}
-  timeout {fail "(timeout) define hook-stop command"}
-}
-
-gdb_test "next" "Breakpoint.*funbreak \\(i=5\\) at .*:$bp_location\r\n$bp_location.*set breakpoint here \\*/" \
-    "next triggering hook-stop"
-
-gdb_test "next" "i = i \\+ 10;" "next no hook-stop"
diff --git a/gdb/testsuite/gdb.base/hook-stop-frame.exp b/gdb/testsuite/gdb.base/hook-stop-frame.exp
deleted file mode 100644
index 5b443ff..0000000
--- a/gdb/testsuite/gdb.base/hook-stop-frame.exp
+++ /dev/null
@@ -1,48 +0,0 @@ 
-# Copyright 2009-2015 Free Software Foundation, Inc.
-
-# This program is free software; you can redistribute it and/or modify
-# it under the terms of the GNU General Public License as published by
-# the Free Software Foundation; either version 3 of the License, or
-# (at your option) any later version.
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with this program.  If not, see <http://www.gnu.org/licenses/>.
-
-standard_testfile
-
-if { [prepare_for_testing ${testfile}.exp "${testfile}" $srcfile {debug nowarnings}] } {
-    return -1
-}
-
-if ![runto_main] then {
-    perror "Couldn't run to main"
-}
-
-set bp_location [gdb_get_line_number "set breakpoint here"]
-
-gdb_test "break func" \
-    "Breakpoint.*at.* file .*$srcfile.*\\." \
-    "breakpoint line number"
-
-set test "define hook-stop command"
-gdb_test_multiple "define hook-stop" "$test" {
-    -re "Type commands for definition of \"hook-stop\".\r\nEnd with a line saying just \"end\".\r\n>$" {
-	gdb_test "echo \"Hello.\"\nend" "" "$test"
-    }
-}
-
-set test "hook-stop runs before frame print"
-gdb_test_multiple "continue" "$test" {
-    -re "\"Hello\\.\"\r\nBreakpo.*func.*set breakpoint here.*${gdb_prompt} $" {
-	pass $test
-    }
-
-    -re "Breakpo.*func.*set breakpoint here.*\"Hello\\.\".*${gdb_prompt} $" {
-	fail $test
-    }
-}
diff --git a/gdb/testsuite/gdb.base/hook-stop-frame.c b/gdb/testsuite/gdb.base/hook-stop.c
similarity index 95%
rename from gdb/testsuite/gdb.base/hook-stop-frame.c
rename to gdb/testsuite/gdb.base/hook-stop.c
index 8dbb8f1..4152f28 100644
--- a/gdb/testsuite/gdb.base/hook-stop-frame.c
+++ b/gdb/testsuite/gdb.base/hook-stop.c
@@ -21,11 +21,17 @@  func (void)
   int a;
 
   a = 1; /* set breakpoint here */
+  a = 2;
 }
 
 int
 main (int argc, char **argv)
 {
+  int i;
+
+  i = 1;
+  i = 2;
+  i = 3;
   func ();
 
   return 0;
diff --git a/gdb/testsuite/gdb.base/hook-stop.exp b/gdb/testsuite/gdb.base/hook-stop.exp
new file mode 100644
index 0000000..cdee7db
--- /dev/null
+++ b/gdb/testsuite/gdb.base/hook-stop.exp
@@ -0,0 +1,168 @@ 
+# Copyright 2009-2015 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+standard_testfile
+
+if { [build_executable ${testfile}.exp "${testfile}" $srcfile {debug nowarnings}] } {
+    return -1
+}
+
+# Define the hook-stop that runs COMMANDS.
+
+proc define_hook_stop {commands} {
+    set test "define hook-stop command"
+    gdb_test_multiple "define hook-stop" "$test" {
+	-re "Type commands for definition of \"hook-stop\".\r\nEnd with a line saying just \"end\".\r\n>$" {
+	    gdb_test "$commands\nend" "" "$test"
+	}
+    }
+}
+
+# Restart GDB, run to main, set a breakpoint, and define a hook-stop
+# that runs COMMANDS.  If running to main fails, this returns to the
+# caller's caller directly.
+
+proc setup {commands} {
+    global srcfile binfile
+
+    clean_restart $binfile
+
+    if ![runto_main] then {
+	fail "Can't run to main"
+	return -code return
+    }
+
+    gdb_test "break func" \
+	"Breakpoint.*at.* file .*$srcfile.*\\." \
+	"breakpoint line number"
+
+    define_hook_stop $commands
+}
+
+# Check that the hook-stop runs before the frame is printed.
+
+proc hook_stop_before_frame {} {
+    with_test_prefix "hook-stop runs before frame print" {
+	global gdb_prompt
+
+	setup "echo \"Hello.\""
+
+	set test "run hook-stop"
+	gdb_test_multiple "continue" "$test" {
+	    -re "\"Hello\\.\"\r\nBreakpo.*func.*set breakpoint here.*${gdb_prompt} $" {
+		pass $test
+	    }
+
+	    -re "Breakpo.*func.*set breakpoint here.*\"Hello\\.\".*${gdb_prompt} $" {
+		fail $test
+	    }
+	}
+    }
+}
+
+# Check that GDB gracefully handles the case of the inferior dying
+# while running the hook-stop.
+
+proc hook_stop_kill {} {
+    with_test_prefix "hook-stop kills inferior" {
+	global gdb_prompt
+
+	setup "kill"
+
+	gdb_test_no_output "set confirm off"
+
+	set test "run hook-stop"
+	gdb_test_multiple "continue" "$test" {
+	    -re "Continuing.\r\n${gdb_prompt} $" {
+		pass $test
+	    }
+	}
+
+	gdb_test "info threads" "No threads.*"
+    }
+}
+
+# Check that GDB gracefully handles the case of the hook-stop
+# continuing the inferior in the foreground.
+
+proc hook_stop_continue_fg {} {
+    with_test_prefix "hook-stop runs continue" {
+	global gdb_prompt
+
+	setup "if \$do_continue\nset \$do_continue = 0\ncontinue\nend"
+
+	gdb_test "print \$do_continue = 1" " = 1"
+
+	gdb_test "next" "Breakpoint.*func \\(\\) at .*set breakpoint here \\*/" \
+	    "next triggering hook-stop"
+
+	gdb_test "next" "a = 2;" "next no hook-stop"
+    }
+}
+
+# Check that GDB gracefully handles the case of the hook-stop
+# continuing the inferior in the background.
+
+proc hook_stop_continue_bg {} {
+    with_test_prefix "hook-stop runs continue&" {
+	global gdb_prompt
+
+	setup "if \$do_continue\nset \$do_continue = 0\ncontinue&\nend"
+
+	gdb_test "print \$do_continue = 1" " = 1"
+
+	set test "run hook-stop"
+	gdb_test_multiple "continue" "$test" {
+	    -re "Continuing.\r\n.*${gdb_prompt} " {
+		pass $test
+	    }
+	}
+
+	set test "inferior exits normally"
+	gdb_test_multiple "" "$test" {
+	    -re "exited normally" {
+		pass $test
+	    }
+	}
+	gdb_test "info threads" "No threads.*"
+    }
+}
+
+# Check that GDB gracefully handles the case of the hook-stop running
+# "next".  GDB used to print the stop event twice.
+
+proc hook_stop_next {} {
+    with_test_prefix "hook-stop runs next" {
+	global gdb_prompt
+
+	setup "next"
+
+	set test "run hook-stop"
+	gdb_test_multiple "continue" "$test" {
+	    -re "a = 2.*a = 2${gdb_prompt} $" {
+		fail $test
+	    }
+	    -re "a = 2.*${gdb_prompt} $" {
+		pass $test
+	    }
+	}
+    }
+}
+
+hook_stop_before_frame
+hook_stop_kill
+hook_stop_continue_fg
+hook_stop_continue_bg
+hook_stop_next