Prelimit number of bytes to read in "vFile:pread:"

Message ID 55D607B8.70103@redhat.com
State New, archived
Headers

Commit Message

Pedro Alves Aug. 20, 2015, 5 p.m. UTC
  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.
  

Comments

Sandra Loosemore Aug. 20, 2015, 6:21 p.m. UTC | #1
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)

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

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, 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".

-Sandra
  

Patch

diff --git a/gdb/extension.c b/gdb/extension.c
index dac203b..d754eb1 100644
--- a/gdb/extension.c
+++ b/gdb/extension.c
@@ -822,6 +822,8 @@  set_quit_flag (void)
     quit_flag = 1;
 }
 
+void target_check_quit_flag (void);
+
 /* Return true if the quit flag has been set, false otherwise.
    Note: The flag is cleared as a side-effect.
    The flag is checked in all extension languages that support cooperative
@@ -833,6 +835,8 @@  check_quit_flag (void)
   int i, result = 0;
   const struct extension_language_defn *extlang;
 
+  target_check_quit_flag ();
+
   ALL_ENABLED_EXTENSION_LANGUAGES (i, extlang)
     {
       if (extlang->ops->check_quit_flag != NULL)
diff --git a/gdb/remote.c b/gdb/remote.c
index bfa5469..e0c61b7 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5206,6 +5206,8 @@  async_handle_remote_sigint (int sig)
   gdb_call_async_signal_handler (async_sigint_remote_token, 0);
 }
 
+static int sigint_twice = 0;
+
 /* Signal handler for SIGINT, installed after SIGINT has already been
    sent once.  It will take effect the second time that the user sends
    a ^C.  */
@@ -5215,6 +5217,19 @@  async_handle_remote_sigint_twice (int sig)
   signal (sig, async_handle_remote_sigint);
   /* See note in async_handle_remote_sigint.  */
   gdb_call_async_signal_handler (async_sigint_remote_twice_token, 0);
+  sigint_twice = 1;
+}
+
+void target_check_quit_flag (void);
+
+void
+target_check_quit_flag (void)
+{
+  if (sigint_twice)
+    {
+      sigint_twice = 0;
+      gdb_call_async_signal_handler (async_sigint_remote_twice_token, 1);
+    }
 }
 
 /* Perform the real interruption of the target execution, in response
@@ -5391,20 +5406,12 @@  interrupt_query (void)
 {
   target_terminal_ours ();
 
-  if (target_is_async_p ())
+  if (query (_("Interrupted while waiting for the program.\n\
+Give up (and stop debugging it)? ")))
     {
-      signal (SIGINT, handle_sigint);
+      remote_unpush_target ();
       quit ();
     }
-  else
-    {
-      if (query (_("Interrupted while waiting for the program.\n\
-Give up (and stop debugging it)? ")))
-	{
-	  remote_unpush_target ();
-	  quit ();
-	}
-    }
 
   target_terminal_inferior ();
 }