[PATCH] remote: allow aborting long operations (e.g., file transfers) (Re: [PATCH] Prelimit number of bytes to read in "vFile:pread:")

[Pedro Alves <palves at redhat dot com>]

On 08/20/2015 07:21 PM, Sandra Loosemore wrote:
> On 08/20/2015 11:00 AM, Pedro Alves wrote:
>> On 08/20/2015 04:51 PM, Pedro Alves wrote:
>>
>>> I don't see a quick, easy and good way around waiting for the whole file to read.
>>> Having GDB handle ctrl-c differently while it is handling internal events
>>> is already a source of trouble, and the fix always seemed to me to be to make ctrl-c
>>> work like what is happening here.  Specifically, I mean that e.g., say that the
>>> user sets a conditional breakpoint that is evaluated on gdb's side, and then the
>>> target stops for that conditional breakpoint, gdb evaluates the expression,
>>> and then resumes the target again, on and on.  If the user presses ctrl-c just
>>> while the conditional breakpoint's expression is being evaluated, and something along
>>> that code path called target_terminal_ours (thus pulling input/ctrl-c to the
>>> regular "Quit" SIGINT handler), gdb behaves differently (shows a "Quit" and the
>>> debug session ends up likely broken) from when the ctrl-c is pressed while the target
>>> is really running.  I'd argue that the ctrl-c in both cases should be passed
>>> down all the way to the target the same way, and that any internal stop and
>>> breakpoint condition evaluation is magic that should be hidden.  Just like
>>> what is happening here with file reading.
>>>
>>> Though having said that, it does look like even that isn't working properly,
>>> as I'd expect this:
>>>
>>> (top-gdb) c
>>> Continuing.
>>> (no debugging symbols found)...done.
>>>
>>> Breakpoint 1, main (argc=1, argv=0x7fffffffe3d8) at ../../../src/gdb/gdbserver/server.c:3635
>>> 3635    ../../../src/gdb/gdbserver/server.c: No such file or directory.
>>> (gdb)
>>>
>>> to be slow (that is, the file reading isn't really interrupted), but then
>>> the target stops with SIGINT as soon as gdb resumes it again after reading
>>> the DSOs.  But it is reaching main anyway, and there's no sign
>>> of "Program stopped with SIGINT"...
>>>
>>> Not sure where the bug is.  It may be on the gdbserver side.
>>
>> OK, so gdbserver receives the \003, but since the target is stopped,
>> the normal packet processing loop discards the \003, as it doesn't
>> look like a start of a RSP packet frame (that is, it isn't a $).
>>
>> I'm thinking that maybe the best way to handle this may be to still
>> leave SIGINT forwarding to the target, so that in case gdb re-resumes
>> the target quick enough, the ctrl-c turns into a real SIGINT on the
>> target.  But then if it takes long for gdb or gdbserver or the target
>> to react to the ctrl-c, then the user presses ctrl-c again, and gdb
>> shows the old:
>>
>>    Interrupted while waiting for the program.
>>    Give up (and stop debugging it)? (y or n)
>>
>> AFAICS, that query is never issued anywhere if the target
>> is async.  And the remote target is always async nowadays.
>>
>> To make that query appear promptly, we'd hook it to the
>> QUIT macro.  Something like this:
>>
>> (gdb) c
>> Continuing.
>> Reading symbols from target:/lib/x86_64-linux-gnu/libdl.so.2...(no debugging symbols found)...done.
>> Reading symbols from target:/lib/x86_64-linux-gnu/libc.so.6...(no debugging symbols found)...done.
>> ^CInterrupted while waiting for the program.
>> Give up (and stop debugging it)? (y or n) y
>> Quit
>> (gdb)
>>
>> Could you try the hacky patch below just to see if it makes a
>> difference to you?  It seems that GDB still doesn't react
>> as soon as it could, but I'd guess that Gary's previous patch
>> to add a QUIT around vFile:pread's would fix that.
> 
> Thanks!  The combination of these two patches does make GDB respond 
> quickly to the second ^C and abort the file transfer.  However, both GDB 
> and gdbserver seem to be left in a wonky state.  On the GDB side, I'm 
> seeing:
> 
> (gdb) c
> Continuing.
> ^C^CInterrupted while waiting for the program.
> Give up (and stop debugging it)? (y or n) Interrupted while waiting for 
> the program.
> y
> You can't do that when your target is `exec'
> No registers.
> (gdb)

Those are caused by the abrupt disconnection.  This should be triggerable
even without the patch (though of course unexpected disconnections
are supposedly rare).  But please try the new patch below.

> 
> And, meanwhile on the target, gdbserver has done this:
> 
> Process a.out created; pid = 856
> Listening on port 6789
> Remote debugging from host 134.86.188.141
> readchar: Got EOF
> Remote side has terminated connection.  GDBserver will reopen the 
> connection.
> /scratch/sandra/nios2-linux-trunk/src/gdb-nios2r2/gdb/gdbserver/../common/cleanups.c:265: 
> A problem internal to GDBserver has been detected.
> restore_my_cleanups has found a stale cleanup
> Listening on port 6789

Yeah, I see that too.  It's orthogonal though.  Seems to trigger for
all disconnections.

> I was thinking that the "right" behavior here would be for GDB to just 
> try to continue without library symbol or debug information if the file 
> transfer is interrupted, 

Maybe.  I'd want that at least behind a query though.  The patch
below gets you back to the prompt, and given a hint about "set sysroot",
the user can then decide what to do.  I think that's sufficient, at least 
for 7.10.

> but a clean shutdown with fewer confusing 
> messages would be OK, too.  Especially if we've given users a hint that 
> they need "set sysroot", the probable scenario is for the user to start 
> over with a fresh GDB and add that command before "target remote".

Agreed.

>From efc760942467848780cab6891e6d5ff4d86487de Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 21 Aug 2015 15:42:48 +0100
Subject: [PATCH] remote: allow aborting long operations (e.g., file transfers)

Currently, when remote debugging, if you type Ctrl-c just while the
target stopped for an internal event, and GDB is busy doing something
that takes a while (e.g., fetching chunks of a shared library off of
the target, with vFile, to process ELF headers and debug info), the
Ctrl-c is lost.

The patch hooks up the QUIT macro to a new target method that lets the
target react to the double-Ctrl-c before the event loop is reached,
which allows reacting to a double-Ctrl-c even when GDB is busy doing
some long operation and not waiting for a stop reply.  That end result
is:

 (gdb) c
 Continuing.
 ^C
 ^C
 Interrupted while waiting for the program.
 Give up waiting? (y or n) y
 Quit
 (gdb) info threads
   Id   Target Id         Frame
 * 1    Thread 11673      0x00007ffff7deb240 in _dl_debug_state () from target:/lib64/ld-linux-x86-64.so.2
 (gdb)

If, however, GDB is waiting for a stop reply (because the target has
been resumed, with e.g., vCont;c), but the target isn't responding, we
now get:

 (gdb) c
 Continuing.
 ^C
 ^C
 The target is not responding to interrupt requests.
 Stop debugging it? (y or n) y
 Disconnected from target.
 (gdb) info threads
 No threads.

This offers to disconnect, because when we're waiting for a stop
reply, there's nothing else we can send the target other than an
interrupt request.  And if that doesn't work, there's nothing else we
can do.

The ctrl-c is presently lost because until we get to a user-visible
stop, the SIGINT handler that is installed is the one that forwards
the interrupt to the remote side, with the \003 "packet" [1].  But,
gdbserver ignores an interrupt request if the program is stopped.
Still, even if it didn't, the server can only report back a
stop-because-of-SIGINT when the program is next resumed.  And it may
take a while to actually re-resume the target.

[1] - In the old sync days, the remote target would react to a
double-Ctrl-c by asking users whether they wanted to give up waiting
and disconnect.  The code is still there, but it it isn't reacheable
on most hosts, which support serial connections in async mode
(probably only DJGPP doesn't).  Even then, in sync mode, remote.c's
SIGINT handler is only installed while the target is resumed, and is
removed as soon as the target sends back a stop reply.  That means
that a Ctrl-c just while GDB is processing an internal event can end
up with an odd "Quit" at the prompt instead of "Program stopped by
SIGINT".  In contrast, in async mode, remote.c's SIGINT handler is set
up as long as target_terminal_inferior or
target_terminal_ours_for_output are in effect (IOW, until we get a
user-visible stop and call target_terminal_ours), so the user
shouldn't get back a spurious Quit.  However, it's still desirable to
be able to interrupt a long-running GDB operation, if GDB takes a
while to re-resume the target or get back to the event loop.

Tested on x86_64 Fedora 20.

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

	* defs.h (maybe_quit): Declare.
	(QUIT): Now calls maybe_quit.
	* event-loop.c (clear_async_signal_handler)
	(async_signal_handler_is_marked): New functions.
	* event-loop.h (async_signal_handler_is_marked)
	(clear_async_signal_handler): New declarations.
	* remote.c (remote_check_pending_interrupt): New function.
	(interrupt_query): Use make_cleanup_restore_target_terminal.  No
	longer check whether the target is async.  If waiting for a stop
	reply, and a ctrl-c as been sent to the target, offer to
	disconnect, and throw TARGET_CLOSE_ERROR instead of a quit.
	Otherwise do not disconnect and throw a quit.
	(_initialize_remote): Install remote_check_pending_interrupt as
	to_check_pending_interrupt.
	* target.c (target_check_pending_interrupt): New function.
	* target.h (struct target_ops) <to_check_pending_interrupt>: New
	field.
	(target_check_pending_interrupt): New declaration.
	* utils.c (maybe_quit): New function.
	* target-delegates.c: Regenerate.
---
 gdb/defs.h             | 20 +++++++++-----------
 gdb/event-loop.c       | 16 ++++++++++++++++
 gdb/event-loop.h       |  9 +++++++++
 gdb/remote.c           | 40 ++++++++++++++++++++++++++++++----------
 gdb/target-delegates.c | 26 ++++++++++++++++++++++++++
 gdb/target.c           |  8 ++++++++
 gdb/target.h           | 10 ++++++++++
 gdb/utils.c            | 12 ++++++++++++
 8 files changed, 120 insertions(+), 21 deletions(-)

diff --git a/gdb/defs.h b/gdb/defs.h
index f4951ab..464a3f8 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -149,17 +149,15 @@ extern int immediate_quit;
 
 extern void quit (void);
 
-/* FIXME: cagney/2000-03-13: It has been suggested that the peformance
-   benefits of having a ``QUIT'' macro rather than a function are
-   marginal.  If the overhead of a QUIT function call is proving
-   significant then its calling frequency should probably be reduced
-   [kingdon].  A profile analyzing the current situtation is
-   needed.  */
-
-#define QUIT { \
-  if (check_quit_flag () || sync_quit_force_run) quit (); \
-  if (deprecated_interactive_hook) deprecated_interactive_hook (); \
-}
+/* Helper for the QUIT macro.  */
+
+extern void maybe_quit (void);
+
+/* Check whether a Ctrl-C was typed, and if so, call quit.  The target
+   is given a chance to process the ctrl-c.  E.g., it may detect that
+   repeated Ctrl-c requests were issued, and choose to close the
+   connection.  */
+#define QUIT maybe_quit ()
 
 /* * Languages represented in the symbol table and elsewhere.
    This should probably be in language.h, but since enum's can't
diff --git a/gdb/event-loop.c b/gdb/event-loop.c
index 9ac4908..9e54c94 100644
--- a/gdb/event-loop.c
+++ b/gdb/event-loop.c
@@ -908,6 +908,22 @@ mark_async_signal_handler (async_signal_handler * async_handler_ptr)
   async_handler_ptr->ready = 1;
 }
 
+/* See event-loop.h.  */
+
+void
+clear_async_signal_handler (async_signal_handler *async_handler_ptr)
+{
+  async_handler_ptr->ready = 0;
+}
+
+/* See event-loop.h.  */
+
+int
+async_signal_handler_is_marked (async_signal_handler *async_handler_ptr)
+{
+  return async_handler_ptr->ready;
+}
+
 /* Call all the handlers that are ready.  Returns true if any was
    indeed ready.  */
 static int
diff --git a/gdb/event-loop.h b/gdb/event-loop.h
index 6ae4013..598d0da 100644
--- a/gdb/event-loop.h
+++ b/gdb/event-loop.h
@@ -102,6 +102,15 @@ void call_async_signal_handler (struct async_signal_handler *handler);
    below, with IMMEDIATE_P == 0.  */
 void mark_async_signal_handler (struct async_signal_handler *handler);
 
+/* Returns true if HANDLER is marked ready.  */
+
+extern int
+  async_signal_handler_is_marked (struct async_signal_handler *handler);
+
+/* Mark HANDLER as NOT ready.  */
+
+extern void clear_async_signal_handler (struct async_signal_handler *handler);
+
 /* Wrapper for the body of signal handlers.  Call this function from
    any SIGINT handler which needs to access GDB data structures or
    escape via longjmp.  If IMMEDIATE_P is set, this triggers either
diff --git a/gdb/remote.c b/gdb/remote.c
index 068d079..2b9debc 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5280,6 +5280,20 @@ async_handle_remote_sigint_twice (int sig)
   gdb_call_async_signal_handler (async_sigint_remote_twice_token, 0);
 }
 
+/* Implementation of to_check_pending_interrupt.  */
+
+static void
+remote_check_pending_interrupt (struct target_ops *self)
+{
+  struct async_signal_handler *token = async_sigint_remote_twice_token;
+
+  if (async_signal_handler_is_marked (token))
+    {
+      clear_async_signal_handler (token);
+      call_async_signal_handler (token);
+    }
+}
+
 /* Perform the real interruption of the target execution, in response
    to a ^C.  */
 static void
@@ -5452,24 +5466,29 @@ remote_interrupt (struct target_ops *self, ptid_t ptid)
 static void
 interrupt_query (void)
 {
+  struct remote_state *rs = get_remote_state ();
+  struct cleanup *old_chain;
+
+  old_chain = make_cleanup_restore_target_terminal ();
   target_terminal_ours ();
 
-  if (target_is_async_p ())
-    {
-      signal (SIGINT, handle_sigint);
-      quit ();
-    }
-  else
+  if (rs->waiting_for_stop_reply && rs->ctrlc_pending_p)
     {
-      if (query (_("Interrupted while waiting for the program.\n\
-Give up (and stop debugging it)? ")))
+      if (query (_("The target is not responding to interrupt requests.\n"
+		   "Stop debugging it? ")))
 	{
 	  remote_unpush_target ();
-	  quit ();
+	  throw_error (TARGET_CLOSE_ERROR, _("Disconnected from target."));
 	}
     }
+  else
+    {
+      if (query (_("Interrupted while waiting for the program.\n"
+		   "Give up waiting? ")))
+	quit ();
+    }
 
-  target_terminal_inferior ();
+  do_cleanups (old_chain);
 }
 
 /* Enable/disable target terminal ownership.  Most targets can use
@@ -12511,6 +12530,7 @@ Specify the serial device it is connected to\n\
   remote_ops.to_get_ada_task_ptid = remote_get_ada_task_ptid;
   remote_ops.to_stop = remote_stop;
   remote_ops.to_interrupt = remote_interrupt;
+  remote_ops.to_check_pending_interrupt = remote_check_pending_interrupt;
   remote_ops.to_xfer_partial = remote_xfer_partial;
   remote_ops.to_rcmd = remote_rcmd;
   remote_ops.to_pid_to_exec_file = remote_pid_to_exec_file;
diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
index 892cf9d..ddcad94 100644
--- a/gdb/target-delegates.c
+++ b/gdb/target-delegates.c
@@ -1561,6 +1561,28 @@ debug_interrupt (struct target_ops *self, ptid_t arg1)
 }
 
 static void
+delegate_check_pending_interrupt (struct target_ops *self)
+{
+  self = self->beneath;
+  self->to_check_pending_interrupt (self);
+}
+
+static void
+tdefault_check_pending_interrupt (struct target_ops *self)
+{
+}
+
+static void
+debug_check_pending_interrupt (struct target_ops *self)
+{
+  fprintf_unfiltered (gdb_stdlog, "-> %s->to_check_pending_interrupt (...)\n", debug_target.to_shortname);
+  debug_target.to_check_pending_interrupt (&debug_target);
+  fprintf_unfiltered (gdb_stdlog, "<- %s->to_check_pending_interrupt (", debug_target.to_shortname);
+  target_debug_print_struct_target_ops_p (&debug_target);
+  fputs_unfiltered (")\n", gdb_stdlog);
+}
+
+static void
 delegate_rcmd (struct target_ops *self, const char *arg1, struct ui_file *arg2)
 {
   self = self->beneath;
@@ -4042,6 +4064,8 @@ install_delegators (struct target_ops *ops)
     ops->to_stop = delegate_stop;
   if (ops->to_interrupt == NULL)
     ops->to_interrupt = delegate_interrupt;
+  if (ops->to_check_pending_interrupt == NULL)
+    ops->to_check_pending_interrupt = delegate_check_pending_interrupt;
   if (ops->to_rcmd == NULL)
     ops->to_rcmd = delegate_rcmd;
   if (ops->to_pid_to_exec_file == NULL)
@@ -4280,6 +4304,7 @@ install_dummy_methods (struct target_ops *ops)
   ops->to_thread_name = tdefault_thread_name;
   ops->to_stop = tdefault_stop;
   ops->to_interrupt = tdefault_interrupt;
+  ops->to_check_pending_interrupt = tdefault_check_pending_interrupt;
   ops->to_rcmd = default_rcmd;
   ops->to_pid_to_exec_file = tdefault_pid_to_exec_file;
   ops->to_log_command = tdefault_log_command;
@@ -4430,6 +4455,7 @@ init_debug_target (struct target_ops *ops)
   ops->to_thread_name = debug_thread_name;
   ops->to_stop = debug_stop;
   ops->to_interrupt = debug_interrupt;
+  ops->to_check_pending_interrupt = debug_check_pending_interrupt;
   ops->to_rcmd = debug_rcmd;
   ops->to_pid_to_exec_file = debug_pid_to_exec_file;
   ops->to_log_command = debug_log_command;
diff --git a/gdb/target.c b/gdb/target.c
index e41a338..0861d24 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3285,6 +3285,14 @@ target_interrupt (ptid_t ptid)
   (*current_target.to_interrupt) (&current_target, ptid);
 }
 
+/* See target.h.  */
+
+void
+target_check_pending_interrupt (void)
+{
+  (*current_target.to_check_pending_interrupt) (&current_target);
+}
+
 /* See target/target.h.  */
 
 void
diff --git a/gdb/target.h b/gdb/target.h
index e283c86..27612a0 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -642,6 +642,8 @@ struct target_ops
       TARGET_DEFAULT_IGNORE ();
     void (*to_interrupt) (struct target_ops *, ptid_t)
       TARGET_DEFAULT_IGNORE ();
+    void (*to_check_pending_interrupt) (struct target_ops *)
+      TARGET_DEFAULT_IGNORE ();
     void (*to_rcmd) (struct target_ops *,
 		     const char *command, struct ui_file *output)
       TARGET_DEFAULT_FUNC (default_rcmd);
@@ -1681,6 +1683,14 @@ extern void target_stop (ptid_t ptid);
 
 extern void target_interrupt (ptid_t ptid);
 
+/* Some targets install their own SIGINT handler while the target is
+   running.  This method is called from the QUIT macro to give such
+   targets a chance to process a Ctrl-C.  The target may e.g., choose
+   to interrupt the (potentially) long running operation, or give up
+   waiting and disconnect.  */
+
+extern void target_check_pending_interrupt (void);
+
 /* Send the specified COMMAND to the target's monitor
    (shell,interpreter) for execution.  The result of the query is
    placed in OUTBUF.  */
diff --git a/gdb/utils.c b/gdb/utils.c
index e5ad195..2b3ec72 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1041,6 +1041,18 @@ quit (void)
 #endif
 }
 
+/* See defs.h.  */
+
+void
+maybe_quit (void)
+{
+  if (check_quit_flag () || sync_quit_force_run)
+    quit ();
+  if (deprecated_interactive_hook)
+    deprecated_interactive_hook ();
+  target_check_pending_interrupt ();
+}
+
 
 /* Called when a memory allocation fails, with the number of bytes of
    memory requested in SIZE.  */
-- 
1.9.3