[PATCH 1/5] Simplify child_terminal_inferior

[PATCH 1/5] Simplify child_terminal_inferior

[Pedro Alves]

The comment about Lynx in child_terminal_init reads a bit odd, since
it's not exactly clear what "This" in "This is for Lynx" is referring
to.  Looking back in history makes it clearer.  When the comment was
originally added, in commit 91ecc8efa9b9, back in 1994, the code
looked like this:

~~~
#ifdef PROCESS_GROUP_TYPE
#ifdef PIDGET
      /* This is for Lynx, and should be cleaned up by having Lynx be
         a separate debugging target with a version of
         target_terminal_init_inferior which passes in the process
         group to a generic routine which does all the work (and the
         non-threaded child_terminal_init_inferior can just pass in
         inferior_pid to the same routine).  */
      inferior_process_group = PIDGET (inferior_pid);
#else
      inferior_process_group = inferior_pid;
#endif
#endif
~~~

So this looked like it was about when GDB was growing support for
multi-threading, and inferior_pid was still a single int for most
ports.

Eventually we got ptid_t, so the comment isn't really useful today.
Particularly more so since we no longer support Lynx as a GDB host.

The only caller left of child_terminal_init_with_pgrp is gnu-nat.c
(the Hurd), and that target uses fork-child, so when we reach
gnu_terminal_init after spawning a new child, the current inferior
must already have the PID set, and the child must be a process group
leader.

We can't add a 'getpgid(inf->pid) == inf->pid' assertion to
child_terminal_init though (like a previous version of this patch was
doing [1]), because child_terminal_init is also reached after
attaching to a process.  If we did, the new
gdb.base/attach-non-pgrp-leader.exp test would fail, with:

  (gdb) attach 12415
  Attaching to program: build/gdb/testsuite/outputs/gdb.base/attach-non-pgrp-leader/attach-non-pgrp-leader, process 12415
  src/gdb/inflow.c:180: internal-error: void child_terminal_init(target_ops*): Assertion `getpgid (inf->pid) == inf->pid' failed.
  A problem internal to GDB has been detected,
  further debugging may prove unreliable.
  Quit this debugging session? (y or n) FAIL: gdb.base/attach-non-pgrp-leader.exp: child: attach to child (GDB internal error)

I'm not making GDB save the pgid for attached processes with getpgid
for now, because the saved process group affects other things which
I'm leaving for following patches, like e.g., the "interrupt" command.

[1] - https://sourceware.org/ml/gdb-patches/2017-11/msg00039.html

gdb/ChangeLog:
2017-11-06  Pedro Alves  <palves@redhat.com>

	* gnu-nat.c (gnu_terminal_init): Delete.
	(gnu_target): Don't install gnu_terminal_init.
	* inflow.c (child_terminal_init_with_pgrp): Delete, merged with ...
	(child_terminal_init): ... this function.
---
 gdb/ChangeLog |  7 +++++++
 gdb/gnu-nat.c |  7 -------
 gdb/inflow.c  | 22 +++++-----------------
 3 files changed, 12 insertions(+), 24 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 7a90ea5..ac5b318 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,12 @@
 2017-11-06  Pedro Alves  <palves@redhat.com>
 
+	* gnu-nat.c (gnu_terminal_init): Delete.
+	(gnu_target): Don't install gnu_terminal_init.
+	* inflow.c (child_terminal_init_with_pgrp): Delete, merged with ...
+	(child_terminal_init): ... this function.
+
+2017-11-06  Pedro Alves  <palves@redhat.com>
+
 	* common/common.m4 (GDB_AC_COMMON): No longer check termio.h nor
 	sgtty.h.
 	* config.in, configure: Regenerate.
diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
index 7cb6e4a..2ae2031 100644
--- a/gdb/gnu-nat.c
+++ b/gdb/gnu-nat.c
@@ -2279,12 +2279,6 @@ gnu_detach (struct target_ops *ops, const char *args, int from_tty)
   inf_child_maybe_unpush_target (ops);
 }
 \f
-static void
-gnu_terminal_init (struct target_ops *self)
-{
-  gdb_assert (gnu_current_inf);
-  child_terminal_init_with_pgrp (gnu_current_inf->pid);
-}
 
 static void
 gnu_stop (struct target_ops *self, ptid_t ptid)
@@ -2693,7 +2687,6 @@ gnu_target (void)
   t->to_wait = gnu_wait;
   t->to_xfer_partial = gnu_xfer_partial;
   t->to_find_memory_regions = gnu_find_memory_regions;
-  t->to_terminal_init = gnu_terminal_init;
   t->to_kill = gnu_kill_inferior;
   t->to_create_inferior = gnu_create_inferior;
   t->to_mourn_inferior = gnu_mourn_inferior;
diff --git a/gdb/inflow.c b/gdb/inflow.c
index d46d693..2fba0fa 100644
--- a/gdb/inflow.c
+++ b/gdb/inflow.c
@@ -165,7 +165,7 @@ gdb_has_a_terminal (void)
    before we actually run the inferior.  */
 
 void
-child_terminal_init_with_pgrp (int pgrp)
+child_terminal_init (struct target_ops *self)
 {
   struct inferior *inf = current_inferior ();
   struct terminal_info *tinfo = get_inflow_inferior_data (inf);
@@ -173,8 +173,10 @@ child_terminal_init_with_pgrp (int pgrp)
 #ifdef HAVE_TERMIOS_H
   /* Store the process group even without a terminal as it is used not
      only to reset the tty foreground process group, but also to
-     interrupt the inferior.  */
-  tinfo->process_group = pgrp;
+     interrupt the inferior.  A child we spawn should be a process
+     group leader (PGID==PID) at this point, though that may not be
+     true if we're attaching to an existing process.  */
+  tinfo->process_group = inf->pid;
 #endif
 
   if (gdb_has_a_terminal ())
@@ -204,20 +206,6 @@ gdb_save_tty_state (void)
     }
 }
 
-void
-child_terminal_init (struct target_ops *self)
-{
-#ifdef HAVE_TERMIOS_H
-  /* This is for Lynx, and should be cleaned up by having Lynx be a
-     separate debugging target with a version of target_terminal::init
-     which passes in the process group to a generic routine which does
-     all the work (and the non-threaded child_terminal_init can just
-     pass in inferior_ptid to the same routine).  */
-  /* We assume INFERIOR_PID is also the child's process group.  */
-  child_terminal_init_with_pgrp (ptid_get_pid (inferior_ptid));
-#endif /* HAVE_TERMIOS_H */
-}
-
 /* Put the inferior's terminal settings into effect.
    This is preparation for starting or resuming the inferior.
 
-- 
2.5.5

[PATCH 0/5] Fix multiple Ctrl-C/Quit issues

[Pedro Alves]

As I mentioned before, I've been messing with GDB's terminal handling
in context of multi-target.  While doing some inflow.c surgery, I had
the bright idea of writing a test that did:

  break foo
  commands
    printf "hello\n"
    continue

and run that against a program that does basically:

  while (1)
    foo ();

and then while the inferior is running and hitting that breakpoint,
hit Ctrl-C, to make sure target_terminal::inferior/ours handling was
correct and that the user always re-gained control.

Unfortunately, that test hit a number of other, preexisting
problems...  (I never learn...  :-P)  This series fixes them, and then
adds a testcase similar to the above.  The test passes cleanly with
the fixes in place, but fails otherwise.

Pedro Alves (5):
  Simplify child_terminal_inferior
  Fix swallowed "Quit" when inserting breakpoints
  Fix stdin ending up not registered after a Quit
  Don't ever Quit out of resume
  Python unwinder sniffer: PyExc_KeyboardInterrupt -> Quit

 gdb/ChangeLog          |  7 +++++++
 gdb/breakpoint.c       | 41 +++++++++++++++++++++--------------------
 gdb/gnu-nat.c          |  7 -------
 gdb/inf-loop.c         | 10 +++++++++-
 gdb/inflow.c           | 22 +++++-----------------
 gdb/infrun.c           | 43 ++++++++-----------------------------------
 gdb/python/py-unwind.c |  7 +++++++
 7 files changed, 57 insertions(+), 80 deletions(-)

-- 
2.5.5

[PATCH 5/5] Python unwinder sniffer: PyExc_KeyboardInterrupt -> Quit

[Pedro Alves]

If you happen to press Ctrl-C while GDB is running the Python unwinder
machinery, the Ctrl-C is swallowed by the Python unwinder machinery.

For example, with:

 break foo
 commands
 > c
 > end

and

  while (1)
    foo ();

and then let the inferior hit "foo" repeatedly, sometimes Ctrl-C
results in:

~~~
  23        usleep (100);

  Breakpoint 2, foo () at gdb.base/bp-cmds-continue-ctrl-c.c:23
  23        usleep (100);
  ^C
  Breakpoint 2, Python Exception <class 'KeyboardInterrupt'> <class 'KeyboardInterrupt'>:
  foo () at gdb.base/bp-cmds-continue-ctrl-c.c:23
  23        usleep (100);

  Breakpoint 2, foo () at gdb.base/bp-cmds-continue-ctrl-c.c:23
  23        usleep (100);

  Breakpoint 2, foo () at gdb.base/bp-cmds-continue-ctrl-c.c:23
  23        usleep (100);
~~~

Notice the Python exception above.  The interesting thing here is that
GDB continues as if nothing happened, doesn't really stop and give
back control to the user.  Instead, the Ctrl-C aborted the Python
unwinder sniffer and GDB moved on to just use another unwinder.

Fix this by translating a PyExc_KeyboardInterrupt back into a Quit
exception once back in GDB.

This was exposed by the new gdb.base/bp-cmds-continue-ctrl-c.exp
testcase added later in the series.

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

	* python/py-unwind.c (pyuw_sniffer): Translate
	PyExc_KeyboardInterrupt to a GDB Quit exception.
---
 gdb/python/py-unwind.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
index acfce7d..b578373 100644
--- a/gdb/python/py-unwind.c
+++ b/gdb/python/py-unwind.c
@@ -539,6 +539,13 @@ pyuw_sniffer (const struct frame_unwind *self, struct frame_info *this_frame,
 				   pyo_pending_frame.get (), NULL));
   if (pyo_unwind_info == NULL)
     {
+      /* If the unwinder is cancelled due to a Ctrl-C, then propagate
+	 the Ctrl-C as a GDB exception instead of swallowing it.  */
+      if (PyErr_ExceptionMatches (PyExc_KeyboardInterrupt))
+	{
+	  PyErr_Clear ();
+	  quit ();
+	}
       gdbpy_print_stack ();
       return 0;
     }
-- 
2.5.5

[PATCH 3/5] Fix stdin ending up not registered after a Quit

[Pedro Alves]

If you press Ctrl-C while GDB is processing breakpoint commands the
TRY/CATCH in inferior_event_handler catches the Quit exception and
prints it, and then if the interpreter was running a foreground
execution command, nothing re-adds stdin back in the event loop,
meaning the debug session ends up busted, because the user can't type
anything...

This was exposed by the new gdb.base/bp-cmds-continue-ctrl-c.exp
testcase added later in the series.

gdb/ChangeLog:
2017-11-03  Pedro Alves  <palves@redhat.com>

	* inf-loop.c (inferior_event_handler): Don't swallow the exception
	if the prompt is blocked.
---
 gdb/inf-loop.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/gdb/inf-loop.c b/gdb/inf-loop.c
index bb9fa01..1d573b9 100644
--- a/gdb/inf-loop.c
+++ b/gdb/inf-loop.c
@@ -73,7 +73,15 @@ inferior_event_handler (enum inferior_event_type event_type,
 	    }
 	  CATCH (e, RETURN_MASK_ALL)
 	    {
-	      exception_print (gdb_stderr, e);
+	      /* If the user was running a foreground execution
+		 command, then propagate the error so that the prompt
+		 can be reenabled.  Otherwise, the user already has
+		 the prompt and is typing some unrelated command, so
+		 just inform the user and swallow the exception.  */
+	      if (current_ui->prompt_state == PROMPT_BLOCKED)
+		throw_exception (e);
+	      else
+		exception_print (gdb_stderr, e);
 	    }
 	  END_CATCH
 	}
-- 
2.5.5

[PATCH 4/5] Don't ever Quit out of resume

[Pedro Alves]

If you have a breakpoint command that re-resumes the target, like:

  break foo
  commands
  > c
  > end

and then let the inferior run, hitting the breakpoint, and then press
Ctrl-C at just the right time, between GDB processing the stop at
"foo", and re-resuming the target, you'll hit the QUIT call in
infrun.c:resume.

With this hack, we can reproduce the bad case consistently:

  --- a/gdb/inf-loop.c
  +++ b/gdb/inf-loop.c
  @@ -31,6 +31,8 @@
   #include "top.h"
   #include "observer.h"

  +bool continue_hack;
  +
   /* General function to handle events in the inferior.  */

   void
  @@ -64,6 +66,8 @@ inferior_event_handler (enum inferior_event_type event_type,
	  {
	    check_frame_language_change ();

  +         continue_hack = true;
  +
	    /* Don't propagate breakpoint commands errors.  Either we're
	       stopping or some command resumes the inferior.  The user will
	       be informed.  */
  diff --git a/gdb/infrun.c b/gdb/infrun.c
  index d425664..c74b14c 100644
  --- a/gdb/infrun.c
  +++ b/gdb/infrun.c
  @@ -2403,6 +2403,10 @@ resume (enum gdb_signal sig)
     gdb_assert (!tp->stop_requested);
     gdb_assert (!thread_is_in_step_over_chain (tp));

  +  extern bool continue_hack;
  +
  +  if (continue_hack)
  +    set_quit_flag ();
     QUIT;

The GDB backtrace looks like this:

  (top-gdb) bt
  ...
  #3  0x0000000000612e8b in throw_quit(char const*, ...) (fmt=0xaf84a1 "Quit") at src/gdb/common/common-exceptions.c:408
  #4  0x00000000007fc104 in quit() () at src/gdb/utils.c:748
  #5  0x00000000006a79d2 in default_quit_handler() () at src/gdb/event-top.c:954
  #6  0x00000000007fc134 in maybe_quit() () at src/gdb/utils.c:762
  #7  0x00000000006f66a3 in resume(gdb_signal) (sig=GDB_SIGNAL_0) at src/gdb/infrun.c:2406
  #8  0x0000000000700c3d in keep_going_pass_signal(execution_control_state*) (ecs=0x7ffcf3744e60) at src/gdb/infrun.c:7793
  #9  0x00000000006f5fcd in start_step_over() () at src/gdb/infrun.c:2145
  #10 0x00000000006f7b1f in proceed(unsigned long, gdb_signal) (addr=18446744073709551615, siggnal=GDB_SIGNAL_DEFAULT)
      at src/gdb/infrun.c:3135
  #11 0x00000000006ebdd4 in continue_1(int) (all_threads=0) at src/gdb/infcmd.c:842
  #12 0x00000000006ec097 in continue_command(char*, int) (args=0x0, from_tty=0) at src/gdb/infcmd.c:938
  #13 0x00000000004b5140 in do_cfunc(cmd_list_element*, char*, int) (c=0x2d18570, args=0x0, from_tty=0)
      at src/gdb/cli/cli-decode.c:106
  #14 0x00000000004b8219 in cmd_func(cmd_list_element*, char*, int) (cmd=0x2d18570, args=0x0, from_tty=0)
      at src/gdb/cli/cli-decode.c:1952
  #15 0x00000000007f1532 in execute_command(char*, int) (p=0x7ffcf37452b1 "", from_tty=0) at src/gdb/top.c:608
  #16 0x00000000004bd127 in execute_control_command(command_line*) (cmd=0x3a88ef0) at src/gdb/cli/cli-script.c:485
  #17 0x00000000005cae0c in bpstat_do_actions_1(bpstat*) (bsp=0x37edcf0) at src/gdb/breakpoint.c:4513
  #18 0x00000000005caf67 in bpstat_do_actions() () at src/gdb/breakpoint.c:4563
  #19 0x00000000006e8798 in inferior_event_handler(inferior_event_type, void*) (event_type=INF_EXEC_COMPLETE, client_data=0x0)
      at src/gdb/inf-loop.c:72
  #20 0x00000000006f9447 in fetch_inferior_event(void*) (client_data=0x0) at src/gdb/infrun.c:3970
  #21 0x00000000006e870e in inferior_event_handler(inferior_event_type, void*) (event_type=INF_REG_EVENT, client_data=0x0)
      at src/gdb/inf-loop.c:43
  #22 0x0000000000494d58 in remote_async_serial_handler(serial*, void*) (scb=0x3585ca0, context=0x2cd1b80)
      at src/gdb/remote.c:13820
  #23 0x000000000044d682 in run_async_handler_and_reschedule(serial*) (scb=0x3585ca0) at src/gdb/ser-base.c:137
  #24 0x000000000044d767 in fd_event(int, void*) (error=0, context=0x3585ca0) at src/gdb/ser-base.c:188
  #25 0x00000000006a5686 in handle_file_event(file_handler*, int) (file_ptr=0x45997d0, ready_mask=1)
      at src/gdb/event-loop.c:733
  #26 0x00000000006a5c29 in gdb_wait_for_event(int) (block=1) at src/gdb/event-loop.c:859
  #27 0x00000000006a4aa6 in gdb_do_one_event() () at src/gdb/event-loop.c:347
  #28 0x00000000006a4ade in start_event_loop() () at src/gdb/event-loop.c:371

and when that happens, you end up with GDB's run control in quite a
messed up state.  Something like this:

  thread_function1 (arg=0x1) at threads.c:107
  107             usleep (SLEEP);  /* Loop increment.  */
  Quit
  (gdb) c
  Continuing.
  ** nothing happens, time passes..., press ctrl-c again **
  ^CQuit
  (gdb) info threads
    Id   Target Id         Frame
    1    Thread 1462.1462 "threads" (running)
  * 2    Thread 1462.1466 "threads" (running)
    3    Thread 1462.1465 "function0" (running)
  (gdb) c
  Cannot execute this command while the selected thread is running.
  (gdb)

The first "Quit" above is thrown from within "resume", and cancels run
control while GDB is in the middle of stepping over a breakpoint.
with step_over_info_valid_p() true.  The next "c" didn't actually
resume anything, because GDB throught that the step-over was still in
progress.  It wasn't, because the thread that was supposed to be
stepping over the breakpoint wasn't actually resumed.

So at this point, we press Ctrl-C again, and this time, the default
quit handler is called directly from the event loop
(event-top.c:default_quit_handler -> quit()), because gdb was left
owning the terminal (because the previous resume was cancelled before
we reach target_resume -> target_terminal::inferior()).

Note that the exception called from within resume ends up calling
normal_stop via resume_cleanups.  That's very borked though, because
normal_stop is going to re-handle whatever was the last reported
event, possibly even re-running a hook stop...  I think that the only
sane way to safely cancel the run control state machinery is to push
an event via handle_inferior_event like all other events.

The fix here does two things, and either alone would fix the problem
at hand:

#1 - passes the terminal to the inferior earlier, so that any QUIT
     call from the point we declare the target as running goes to the
     inferior directly, protecting run control from unsafe QUIT calls.

#2 - gets rid of this QUIT call in resume and of its related unsafe
     resume_cleanups.

Aboout #2, the comment describing resume says:

  /* Resume the inferior, but allow a QUIT.  This is useful if the user
     wants to interrupt some lengthy single-stepping operation
     (for child processes, the SIGINT goes to the inferior, and so
     we get a SIGINT random_signal, but for remote debugging and perhaps
     other targets, that's not true).

but that's a really old comment that predates a lot of fixes to Ctrl-C
handling throughout both GDB core and the remote target, that made
sure that a Ctrl-C isn't ever lost.  In any case, if some target
depended on this, a much better fix would be to make the target return
a SIGINT stop out of target_wait the next time that is called.

This was exposed by the new gdb.base/bp-cmds-continue-ctrl-c.exp
testcase added later in the series.

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

	* infrun.c (resume_cleanups): Delete.
	(resume): No longer install a resume_cleanups cleanup nor call
	QUIT.
	(proceed): Pass the terminal to the inferior.
	(keep_going_pass_signal): No longer install a resume_cleanups
	cleanup.
---
 gdb/infrun.c | 43 ++++++++-----------------------------------
 1 file changed, 8 insertions(+), 35 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index d425664..a3a4137 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -78,8 +78,6 @@ static void sig_print_info (enum gdb_signal);
 
 static void sig_print_header (void);
 
-static void resume_cleanups (void *);
-
 static int follow_fork (void);
 
 static int follow_fork_inferior (int follow_child, int detach_fork);
@@ -2198,17 +2196,6 @@ infrun_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid)
 }
 
 \f
-/* Resuming.  */
-
-/* Things to clean up if we QUIT out of resume ().  */
-static void
-resume_cleanups (void *ignore)
-{
-  if (!ptid_equal (inferior_ptid, null_ptid))
-    delete_single_step_breakpoints (inferior_thread ());
-
-  normal_stop ();
-}
 
 static const char schedlock_off[] = "off";
 static const char schedlock_on[] = "on";
@@ -2373,17 +2360,12 @@ do_target_resume (ptid_t resume_ptid, int step, enum gdb_signal sig)
   target_commit_resume ();
 }
 
-/* Resume the inferior, but allow a QUIT.  This is useful if the user
-   wants to interrupt some lengthy single-stepping operation
-   (for child processes, the SIGINT goes to the inferior, and so
-   we get a SIGINT random_signal, but for remote debugging and perhaps
-   other targets, that's not true).
+/* Resume the inferior.  SIG is the signal to give the inferior
+   (GDB_SIGNAL_0 for none).  */
 
-   SIG is the signal to give the inferior (zero for none).  */
 void
 resume (enum gdb_signal sig)
 {
-  struct cleanup *old_cleanups = make_cleanup (resume_cleanups, 0);
   struct regcache *regcache = get_current_regcache ();
   struct gdbarch *gdbarch = regcache->arch ();
   struct thread_info *tp = inferior_thread ();
@@ -2403,8 +2385,6 @@ resume (enum gdb_signal sig)
   gdb_assert (!tp->stop_requested);
   gdb_assert (!thread_is_in_step_over_chain (tp));
 
-  QUIT;
-
   if (tp->suspend.waitstatus_pending_p)
     {
       if (debug_infrun)
@@ -2431,7 +2411,6 @@ resume (enum gdb_signal sig)
 	}
 
       tp->suspend.stop_signal = GDB_SIGNAL_0;
-      discard_cleanups (old_cleanups);
 
       if (target_can_async_p ())
 	target_async (1);
@@ -2543,7 +2522,6 @@ resume (enum gdb_signal sig)
 
 	      resume_ptid = internal_resume_ptid (user_step);
 	      do_target_resume (resume_ptid, 0, GDB_SIGNAL_0);
-	      discard_cleanups (old_cleanups);
 	      tp->resumed = 1;
 	      return;
 	    }
@@ -2581,7 +2559,6 @@ resume (enum gdb_signal sig)
 				"Got placed in step-over queue\n");
 
 	  tp->control.trap_expected = 0;
-	  discard_cleanups (old_cleanups);
 	  return;
 	}
       else if (prepared < 0)
@@ -2758,7 +2735,6 @@ resume (enum gdb_signal sig)
 
   do_target_resume (resume_ptid, step, sig);
   tp->resumed = 1;
-  discard_cleanups (old_cleanups);
 }
 \f
 /* Proceeding.  */
@@ -3061,6 +3037,12 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
   if (!tp->control.in_infcall)
    set_running (resume_ptid, 1);
 
+  /* Since we've marked the inferior running, give it the terminal.  A
+     QUIT/Ctrl-C from here on is forwarded to the target (which can
+     still detect attempts to unblock a stuck connection with repeated
+     Ctrl-C from within target_pass_ctrlc).  */
+  target_terminal::inferior ();
+
   if (debug_infrun)
     fprintf_unfiltered (gdb_stdlog,
 			"infrun: proceed (addr=%s, signal=%s)\n",
@@ -7663,10 +7645,6 @@ stop_waiting (struct execution_control_state *ecs)
 static void
 keep_going_pass_signal (struct execution_control_state *ecs)
 {
-  /* Make sure normal_stop is called if we get a QUIT handled before
-     reaching resume.  */
-  struct cleanup *old_cleanups = make_cleanup (resume_cleanups, 0);
-
   gdb_assert (ptid_equal (ecs->event_thread->ptid, inferior_ptid));
   gdb_assert (!ecs->event_thread->resumed);
 
@@ -7688,7 +7666,6 @@ keep_going_pass_signal (struct execution_control_state *ecs)
 	 non-signal event (e.g., a fork); or took a signal which we
 	 are supposed to pass through to the inferior.  Simply
 	 continue.  */
-      discard_cleanups (old_cleanups);
       resume (ecs->event_thread->suspend.stop_signal);
     }
   else if (step_over_info_valid_p ())
@@ -7716,8 +7693,6 @@ keep_going_pass_signal (struct execution_control_state *ecs)
 				"resume of %s deferred\n",
 				target_pid_to_str (tp->ptid));
 	}
-
-      discard_cleanups (old_cleanups);
     }
   else
     {
@@ -7781,14 +7756,12 @@ keep_going_pass_signal (struct execution_control_state *ecs)
 	{
 	  exception_print (gdb_stderr, e);
 	  stop_waiting (ecs);
-	  discard_cleanups (old_cleanups);
 	  return;
 	}
       END_CATCH
 
       ecs->event_thread->control.trap_expected = (remove_bp || remove_wps);
 
-      discard_cleanups (old_cleanups);
       resume (ecs->event_thread->suspend.stop_signal);
     }
 
-- 
2.5.5

[PATCH 2/5] Fix swallowed "Quit" when inserting breakpoints

[Pedro Alves]

If GDB is inserting a breakpoint and you type Ctrl-C at the exact
"right" time, you'll hit a QUIT call in target_read, and the
breakpoint insertion is cancelled.  However, the related TRY/CATCH
code in insert_bp_location does:

 		  CATCH (e, RETURN_MASK_ALL)
 		    {
		      bp_err = e.error;
		      bp_err_message = e.message;
		    }

The problem with that is that a RETURN_QUIT exception has e.error ==
0, which means that further below, in the places that check for error
with:

      if (bp_err != GDB_NO_ERROR)

because GDB_NO_ERROR == 0, GDB continues as if the breakpoint was
inserted succesfully, and resumes the inferior.  Since the breakpoint
wasn't inserted the inferior runs free, out of our control...

Fix this by having insert_bp_location store a copy of the whole
exception instead of just a error/message parts, and then checking
"gdb_exception::reason" instead.

This was exposed by the new gdb.base/bp-cmds-continue-ctrl-c.exp
testcase added later in the series.

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

	* breakpoint.c (insert_bp_location): Replace bp_err and
	bp_err_message locals by a gdb_exception local.
---
 gdb/breakpoint.c | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index adc8950..9723227 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2482,8 +2482,7 @@ insert_bp_location (struct bp_location *bl,
 		    int *hw_breakpoint_error,
 		    int *hw_bp_error_explained_already)
 {
-  enum errors bp_err = GDB_NO_ERROR;
-  const char *bp_err_message = NULL;
+  gdb_exception bp_excpt = exception_none;
 
   if (!should_be_inserted (bl) || (bl->inserted && !bl->needs_update))
     return 0;
@@ -2592,12 +2591,11 @@ insert_bp_location (struct bp_location *bl,
 
 	      val = bl->owner->ops->insert_location (bl);
 	      if (val)
-		bp_err = GENERIC_ERROR;
+		bp_excpt = gdb_exception {RETURN_ERROR, GENERIC_ERROR};
 	    }
 	  CATCH (e, RETURN_MASK_ALL)
 	    {
-	      bp_err = e.error;
-	      bp_err_message = e.message;
+	      bp_excpt = e;
 	    }
 	  END_CATCH
 	}
@@ -2632,16 +2630,16 @@ insert_bp_location (struct bp_location *bl,
 		      val = target_insert_breakpoint (bl->gdbarch,
 						      &bl->overlay_target_info);
 		      if (val)
-			bp_err = GENERIC_ERROR;
+			bp_excpt
+			  = gdb_exception {RETURN_ERROR, GENERIC_ERROR};
 		    }
 		  CATCH (e, RETURN_MASK_ALL)
 		    {
-		      bp_err = e.error;
-		      bp_err_message = e.message;
+		      bp_excpt = e;
 		    }
 		  END_CATCH
 
-		  if (bp_err != GDB_NO_ERROR)
+		  if (bp_excpt.reason != 0)
 		    fprintf_unfiltered (tmp_error_stream,
 					"Overlay breakpoint %d "
 					"failed: in ROM?\n",
@@ -2658,12 +2656,11 @@ insert_bp_location (struct bp_location *bl,
 
 	          val = bl->owner->ops->insert_location (bl);
 		  if (val)
-		    bp_err = GENERIC_ERROR;
+		    bp_excpt = gdb_exception {RETURN_ERROR, GENERIC_ERROR};
 	        }
 	      CATCH (e, RETURN_MASK_ALL)
 	        {
-		  bp_err = e.error;
-		  bp_err_message = e.message;
+		  bp_excpt = e;
 	        }
 	      END_CATCH
 	    }
@@ -2675,7 +2672,7 @@ insert_bp_location (struct bp_location *bl,
 	    }
 	}
 
-      if (bp_err != GDB_NO_ERROR)
+      if (bp_excpt.reason != 0)
 	{
 	  /* Can't set the breakpoint.  */
 
@@ -2687,7 +2684,9 @@ insert_bp_location (struct bp_location *bl,
 	     breakpoint insertion failed (e.g., the remote target
 	     doesn't define error codes), so we must treat generic
 	     errors as memory errors.  */
-	  if ((bp_err == GENERIC_ERROR || bp_err == MEMORY_ERROR)
+	  if (bp_excpt.reason == RETURN_ERROR
+	      && (bp_excpt.error == GENERIC_ERROR
+		  || bp_excpt.error == MEMORY_ERROR)
 	      && bl->loc_type == bp_loc_software_breakpoint
 	      && (solib_name_from_address (bl->pspace, bl->address)
 		  || shared_objfile_contains_address_p (bl->pspace,
@@ -2715,16 +2714,18 @@ insert_bp_location (struct bp_location *bl,
 	      if (bl->loc_type == bp_loc_hardware_breakpoint)
 		{
 		  *hw_breakpoint_error = 1;
-		  *hw_bp_error_explained_already = bp_err_message != NULL;
+		  *hw_bp_error_explained_already = bp_excpt.message != NULL;
                   fprintf_unfiltered (tmp_error_stream,
                                       "Cannot insert hardware breakpoint %d%s",
-                                      bl->owner->number, bp_err_message ? ":" : ".\n");
-                  if (bp_err_message != NULL)
-                    fprintf_unfiltered (tmp_error_stream, "%s.\n", bp_err_message);
+                                      bl->owner->number,
+				      bp_excpt.message ? ":" : ".\n");
+                  if (bp_excpt.message != NULL)
+                    fprintf_unfiltered (tmp_error_stream, "%s.\n",
+					bp_excpt.message);
 		}
 	      else
 		{
-		  if (bp_err_message == NULL)
+		  if (bp_excpt.message == NULL)
 		    {
 		      std::string message
 			= memory_error_message (TARGET_XFER_E_IO,
@@ -2740,7 +2741,7 @@ insert_bp_location (struct bp_location *bl,
 		      fprintf_unfiltered (tmp_error_stream,
 					  "Cannot insert breakpoint %d: %s\n",
 					  bl->owner->number,
-					  bp_err_message);
+					  bp_excpt.message);
 		    }
 		}
 	      return 1;
-- 
2.5.5

Re: [PATCH 0/5] Fix multiple Ctrl-C/Quit issues

[Pedro Alves]

Bummer, hit git send-email with the wrong patch at the top of
my stgit stack...  Please ignore this series.  Patch #1 is already
pushed, and this is missing the last patch in the series.
I'll just resend.

Thanks,
Pedro Alves