* [PATCH 0/2] Fix timeout in random-signal.exp @ 2015-12-21 11:30 Yao Qi 2015-12-21 11:30 ` [RFC 2/2] Change SIGINT handler for extension languages only when target terminal is ours Yao Qi 2015-12-21 11:30 ` [RFC 1/2] Check input interrupt first when reading packet Yao Qi 0 siblings, 2 replies; 12+ messages in thread From: Yao Qi @ 2015-12-21 11:30 UTC (permalink / raw) To: gdb-patches Hi, When I look into the timeout failures in random-signal.exp, I find there are two separate problems causing the timeout, so I write two patches to address them separately. The first one is that GDBserver may lose the interrupt character sent from GDB, and the second one is that GDB shouldn't change SIGINT handler for extension language if the terminal is still inferiors. Regression tested on aarch64-linux and x86_64-linux. *** BLURB HERE *** Yao Qi (2): Check input interrupt first when reading packet Change SIGINT handler for extension languages only when target terminal is ours gdb/extension.c | 51 ++++++++++++++++++++++++++------------------ gdb/gdbserver/remote-utils.c | 9 ++++++++ 2 files changed, 39 insertions(+), 21 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [RFC 2/2] Change SIGINT handler for extension languages only when target terminal is ours 2015-12-21 11:30 [PATCH 0/2] Fix timeout in random-signal.exp Yao Qi @ 2015-12-21 11:30 ` Yao Qi 2016-01-04 16:12 ` Pedro Alves 2015-12-21 11:30 ` [RFC 1/2] Check input interrupt first when reading packet Yao Qi 1 sibling, 1 reply; 12+ messages in thread From: Yao Qi @ 2015-12-21 11:30 UTC (permalink / raw) To: gdb-patches I see a timeout in gdb.base/random-signal.exp, Continuing.^M PASS: gdb.base/random-signal.exp: continue ^CPython Exception <type 'exceptions.KeyboardInterrupt'> <type exceptions.KeyboardInterrupt'>: ^M FAIL: gdb.base/random-signal.exp: stop with control-c (timeout) it can be reproduced by running random-signal.exp with native-gdbserver in a loop, like this, and the fail will be shown in about 20 runs, $ (set -e; while true; do make check RUNTESTFLAGS="--target_board=native-gdbserver random-signal.exp"; done) In the test, the program is being single-stepped for software watchpoint, and in each internal stop, python unwinder sniffer is used, #0 pyuw_sniffer (self=<optimised out>, this_frame=<optimised out>, cache_ptr=0xd554f8) at /home/yao/SourceCode/gnu/gdb/git/gdb/python/py-unwind.c:608 #1 0x00000000006a10ae in frame_unwind_try_unwinder (this_frame=this_frame@entry=0xd554e0, this_cache=this_cache@entry=0xd554f8, unwinder=0xecd540) at /home/yao/SourceCode/gnu/gdb/git/gdb/frame-unwind.c:107 #2 0x00000000006a143f in frame_unwind_find_by_frame (this_frame=this_frame@entry=0xd554e0, this_cache=this_cache@entry=0xd554f8) at /home/yao/SourceCode/gnu/gdb/git/gdb/frame-unwind.c:163 #3 0x000000000069dc6b in compute_frame_id (fi=0xd554e0) at /home/yao/SourceCode/gnu/gdb/git/gdb/frame.c:454 #4 get_prev_frame_if_no_cycle (this_frame=this_frame@entry=0xd55410) at /home/yao/SourceCode/gnu/gdb/git/gdb/frame.c:1781 #5 0x000000000069fdb9 in get_prev_frame_always_1 (this_frame=0xd55410) at /home/yao/SourceCode/gnu/gdb/git/gdb/frame.c:1955 #6 get_prev_frame_always (this_frame=this_frame@entry=0xd55410) at /home/yao/SourceCode/gnu/gdb/git/gdb/frame.c:1971 #7 0x00000000006a04b1 in get_prev_frame (this_frame=this_frame@entry=0xd55410) at /home/yao/SourceCode/gnu/gdb/git/gdb/frame.c:2213 when GDB goes to python extension, or other extension language, the SIGINT handler is changed, and is restored when GDB leaves extension language. GDB only stays in extension language for a very short period in this case, but if ctrl-c is pressed at that moment, python extension will handle the SIGINT, and exceptions.KeyboardInterrupt is shown. Language extension is used in GDB side rather than inferior side, so GDB should only change SIGINT handler for extension language when the terminal is ours (not inferior's). This is what this patch does. With this patch applied, I run random-signal.exp in a loop for 18 hours, and no fail is shown. gdb: 2015-12-21 Yao Qi <yao.qi@linaro.org> * extension.c: Include target.h. (set_active_ext_lang): Only call install_gdb_sigint_handler, check_quit_flag, and set_quit_flag if target_terminal_is_inferior returns false. (restore_active_ext_lang): Likewise. --- gdb/extension.c | 51 ++++++++++++++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 21 deletions(-) diff --git a/gdb/extension.c b/gdb/extension.c index 1b5365a..1147df9 100644 --- a/gdb/extension.c +++ b/gdb/extension.c @@ -22,6 +22,7 @@ #include "defs.h" #include <signal.h> +#include "target.h" #include "auto-load.h" #include "breakpoint.h" #include "event-top.h" @@ -746,19 +747,24 @@ set_active_ext_lang (const struct extension_language_defn *now_active) = XCNEW (struct active_ext_lang_state); previous->ext_lang = active_ext_lang; + previous->sigint_handler.handler_saved = 0; active_ext_lang = now_active; - /* If the newly active extension language uses cooperative SIGINT handling - then ensure GDB's SIGINT handler is installed. */ - if (now_active->language == EXT_LANG_GDB - || now_active->ops->check_quit_flag != NULL) - install_gdb_sigint_handler (&previous->sigint_handler); - - /* If there's a SIGINT recorded in the cooperative extension languages, - move it to the new language, or save it in GDB's global flag if the newly - active extension language doesn't use cooperative SIGINT handling. */ - if (check_quit_flag ()) - set_quit_flag (); + if (!target_terminal_is_inferior ()) + { + /* If the newly active extension language uses cooperative SIGINT + handling then ensure GDB's SIGINT handler is installed. */ + if (now_active->language == EXT_LANG_GDB + || now_active->ops->check_quit_flag != NULL) + install_gdb_sigint_handler (&previous->sigint_handler); + + /* If there's a SIGINT recorded in the cooperative extension languages, + move it to the new language, or save it in GDB's global flag if the + newly active extension language doesn't use cooperative SIGINT + handling. */ + if (check_quit_flag ()) + set_quit_flag (); + } return previous; } @@ -772,16 +778,19 @@ restore_active_ext_lang (struct active_ext_lang_state *previous) active_ext_lang = previous->ext_lang; - /* Restore the previous SIGINT handler if one was saved. */ - if (previous->sigint_handler.handler_saved) - install_sigint_handler (&previous->sigint_handler); - - /* If there's a SIGINT recorded in the cooperative extension languages, - move it to the new language, or save it in GDB's global flag if the newly - active extension language doesn't use cooperative SIGINT handling. */ - if (check_quit_flag ()) - set_quit_flag (); - + if (!target_terminal_is_inferior ()) + { + /* Restore the previous SIGINT handler if one was saved. */ + if (previous->sigint_handler.handler_saved) + install_sigint_handler (&previous->sigint_handler); + + /* If there's a SIGINT recorded in the cooperative extension languages, + move it to the new language, or save it in GDB's global flag if the + newly active extension language doesn't use cooperative SIGINT + handling. */ + if (check_quit_flag ()) + set_quit_flag (); + } xfree (previous); } -- 1.9.1 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC 2/2] Change SIGINT handler for extension languages only when target terminal is ours 2015-12-21 11:30 ` [RFC 2/2] Change SIGINT handler for extension languages only when target terminal is ours Yao Qi @ 2016-01-04 16:12 ` Pedro Alves 2016-01-05 16:52 ` Yao Qi 0 siblings, 1 reply; 12+ messages in thread From: Pedro Alves @ 2016-01-04 16:12 UTC (permalink / raw) To: Yao Qi, gdb-patches On 12/21/2015 11:30 AM, Yao Qi wrote: > I see a timeout in gdb.base/random-signal.exp, > > Continuing.^M > PASS: gdb.base/random-signal.exp: continue > ^CPython Exception <type 'exceptions.KeyboardInterrupt'> <type > exceptions.KeyboardInterrupt'>: ^M > FAIL: gdb.base/random-signal.exp: stop with control-c (timeout) > > it can be reproduced by running random-signal.exp with native-gdbserver > in a loop, like this, and the fail will be shown in about 20 runs, > > $ (set -e; while true; do make check RUNTESTFLAGS="--target_board=native-gdbserver random-signal.exp"; done) The reason this doesn't trigger with native debugging is that in that case, gdb is sharing the terminal with gdb, and with job control, then the ctrl-c always reaches the inferior instead. We can trigger the issue with native debugging as well, if we "attach" instead of "run", like in the patch at the bottom: $ (set -e; while true; do make check RUNTESTFLAGS="random-signal.exp"; done) ... FAIL: gdb.base/random-signal.exp: attach: stop with control-c (timeout) FAIL: gdb.base/random-signal.exp: attach: p wait_gdb = 0 (timeout) ... gdb.log: (gdb) PASS: gdb.base/random-signal.exp: attach: watch v continue Continuing. PASS: gdb.base/random-signal.exp: attach: continue ^CPython Exception <type 'exceptions.KeyboardInterrupt'> <type 'exceptions.KeyboardInterrupt'>: FAIL: gdb.base/random-signal.exp: attach: stop with control-c (timeout) p wait_gdb = 0 FAIL: gdb.base/random-signal.exp: attach: p wait_gdb = 0 (timeout) > > In the test, the program is being single-stepped for software watchpoint, > and in each internal stop, python unwinder sniffer is used, > > #0 pyuw_sniffer (self=<optimised out>, this_frame=<optimised out>, cache_ptr=0xd554f8) at /home/yao/SourceCode/gnu/gdb/git/gdb/python/py-unwind.c:608 > #1 0x00000000006a10ae in frame_unwind_try_unwinder (this_frame=this_frame@entry=0xd554e0, this_cache=this_cache@entry=0xd554f8, unwinder=0xecd540) > at /home/yao/SourceCode/gnu/gdb/git/gdb/frame-unwind.c:107 > #2 0x00000000006a143f in frame_unwind_find_by_frame (this_frame=this_frame@entry=0xd554e0, this_cache=this_cache@entry=0xd554f8) > at /home/yao/SourceCode/gnu/gdb/git/gdb/frame-unwind.c:163 > #3 0x000000000069dc6b in compute_frame_id (fi=0xd554e0) at /home/yao/SourceCode/gnu/gdb/git/gdb/frame.c:454 > #4 get_prev_frame_if_no_cycle (this_frame=this_frame@entry=0xd55410) at /home/yao/SourceCode/gnu/gdb/git/gdb/frame.c:1781 > #5 0x000000000069fdb9 in get_prev_frame_always_1 (this_frame=0xd55410) at /home/yao/SourceCode/gnu/gdb/git/gdb/frame.c:1955 > #6 get_prev_frame_always (this_frame=this_frame@entry=0xd55410) at /home/yao/SourceCode/gnu/gdb/git/gdb/frame.c:1971 > #7 0x00000000006a04b1 in get_prev_frame (this_frame=this_frame@entry=0xd55410) at /home/yao/SourceCode/gnu/gdb/git/gdb/frame.c:2213 > > when GDB goes to python extension, or other extension language, the > SIGINT handler is changed, and is restored when GDB leaves extension > language. GDB only stays in extension language for a very short period > in this case, but if ctrl-c is pressed at that moment, python extension > will handle the SIGINT, and exceptions.KeyboardInterrupt is shown. > > Language extension is used in GDB side rather than inferior side, > so GDB should only change SIGINT handler for extension language when > the terminal is ours (not inferior's). This is what this patch does. > With this patch applied, I run random-signal.exp in a loop for 18 > hours, and no fail is shown. > > gdb: > > 2015-12-21 Yao Qi <yao.qi@linaro.org> > > * extension.c: Include target.h. > (set_active_ext_lang): Only call install_gdb_sigint_handler, > check_quit_flag, and set_quit_flag if target_terminal_is_inferior > returns false. > (restore_active_ext_lang): Likewise. > --- > gdb/extension.c | 51 ++++++++++++++++++++++++++++++--------------------- > 1 file changed, 30 insertions(+), 21 deletions(-) > > diff --git a/gdb/extension.c b/gdb/extension.c > index 1b5365a..1147df9 100644 > --- a/gdb/extension.c > +++ b/gdb/extension.c > @@ -22,6 +22,7 @@ > > #include "defs.h" > #include <signal.h> > +#include "target.h" > #include "auto-load.h" > #include "breakpoint.h" > #include "event-top.h" > @@ -746,19 +747,24 @@ set_active_ext_lang (const struct extension_language_defn *now_active) > = XCNEW (struct active_ext_lang_state); > > previous->ext_lang = active_ext_lang; > + previous->sigint_handler.handler_saved = 0; > active_ext_lang = now_active; > > - /* If the newly active extension language uses cooperative SIGINT handling > - then ensure GDB's SIGINT handler is installed. */ > - if (now_active->language == EXT_LANG_GDB > - || now_active->ops->check_quit_flag != NULL) > - install_gdb_sigint_handler (&previous->sigint_handler); > - > - /* If there's a SIGINT recorded in the cooperative extension languages, > - move it to the new language, or save it in GDB's global flag if the newly > - active extension language doesn't use cooperative SIGINT handling. */ > - if (check_quit_flag ()) > - set_quit_flag (); > + if (!target_terminal_is_inferior ()) I think this should check for terminal_is_ours instead. We also don't want this with terminal_ours_for_output. In that case, _input_ is still sent to the inferior. Here's the test tweak: From 2cf334c489d6278ea73e1f82905b9726f7d502fc Mon Sep 17 00:00:00 2001 From: Pedro Alves <palves@redhat.com> Date: Mon, 4 Jan 2016 14:25:31 +0000 Subject: [PATCH] Test gdb.base/random-signal.exp with "attach" This exposes the issued fixed by: https://sourceware.org/ml/gdb-patches/2015-12/msg00423.html to native debugging as well. gdb/testsuite/ChangeLog: 2016-01-04 Pedro Alves <palves@redhat.com> * gdb.base/random-signal.c (wait): New global. (main): Use it. * gdb.base/random-signal.exp (do_test): New procedure, with body of testcase moved in. (top level) Call it twice, once with "run" and once with "attach". --- gdb/testsuite/gdb.base/random-signal.c | 7 +++- gdb/testsuite/gdb.base/random-signal.exp | 58 ++++++++++++++++++++++++++------ 2 files changed, 53 insertions(+), 12 deletions(-) diff --git a/gdb/testsuite/gdb.base/random-signal.c b/gdb/testsuite/gdb.base/random-signal.c index f4c4b9f..45adbf0 100644 --- a/gdb/testsuite/gdb.base/random-signal.c +++ b/gdb/testsuite/gdb.base/random-signal.c @@ -19,11 +19,16 @@ int v; +/* GDB clears this. */ +volatile int wait_gdb = 1; + int main() { /* Don't let the test case run forever. */ alarm (60); - for (;;) + while (wait_gdb) ; + + return 0; } diff --git a/gdb/testsuite/gdb.base/random-signal.exp b/gdb/testsuite/gdb.base/random-signal.exp index a6170cc..9aa5843 100644 --- a/gdb/testsuite/gdb.base/random-signal.exp +++ b/gdb/testsuite/gdb.base/random-signal.exp @@ -30,19 +30,55 @@ if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} { return -1 } -if {![runto_main]} { - return -1 +# Set a software watchpoint, continue, wait a bit and stop the target +# with ctrl-c. A software watchpoint forces the target to +# single-step. +proc do_test {} { + global binfile + + gdb_test_no_output "set can-use-hw-watchpoints 0" + gdb_test "watch v" "Watchpoint .*" + gdb_test_multiple "continue" "continue" { + -re "Continuing" { + pass "continue" + } + } + + # For this to work we must be sure to consume the "Continuing." + # message first, or GDB's signal handler may not be in place. + after 500 {send_gdb "\003"} + gdb_test "" "Program received signal SIGINT.*" "stop with control-c" + + gdb_test "p wait_gdb = 0" " = 0" } -gdb_test_no_output "set can-use-hw-watchpoints 0" -gdb_test "watch v" "Watchpoint .*" -gdb_test_multiple "continue" "continue" { - -re "Continuing" { - pass "continue" +# With native debugging and "run" (with job control), the ctrl-c +# always reaches the inferior, not gdb, even if ctrl-c is pressed +# while gdb is processing the internal software watchtpoint +# single-step. With remote debugging, the ctrl-c reaches GDB first. +with_test_prefix "run" { + clean_restart $binfile + + if {![runto_main]} { + return -1 } + + do_test } -# For this to work we must be sure to consume the "Continuing." -# message first, or GDB's signal handler may not be in place. -after 500 {send_gdb "\003"} -gdb_test "" "Program received signal SIGINT.*" "stop with control-c" +# With "attach" however, even with native debugging, the ctrl-c always +# reaches GDB first. Test that as well. +with_test_prefix "attach" { + if {[can_spawn_for_attach]} { + clean_restart $binfile + + set test_spawn_id [spawn_wait_for_attach $binfile] + set testpid [spawn_id_get_pid $test_spawn_id] + + gdb_test "attach $testpid" "Attaching to.*process $testpid.*libc.*" "attach" + + do_test + + kill_wait_spawned_process $test_spawn_id + } +} -- 1.9.3 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC 2/2] Change SIGINT handler for extension languages only when target terminal is ours 2016-01-04 16:12 ` Pedro Alves @ 2016-01-05 16:52 ` Yao Qi 2016-01-07 17:24 ` Pedro Alves 0 siblings, 1 reply; 12+ messages in thread From: Yao Qi @ 2016-01-05 16:52 UTC (permalink / raw) To: Pedro Alves; +Cc: Yao Qi, gdb-patches Pedro Alves <palves@redhat.com> writes: > The reason this doesn't trigger with native debugging is that > in that case, gdb is sharing the terminal with gdb, and with job I think you meant "gdb is sharing the terminal with the inferior". > control, then the ctrl-c always reaches the inferior instead. > > We can trigger the issue with native debugging as well, if > we "attach" instead of "run", like in the patch at the bottom: > > $ (set -e; while true; do make check RUNTESTFLAGS="random-signal.exp"; done) > ... > FAIL: gdb.base/random-signal.exp: attach: stop with control-c (timeout) > FAIL: gdb.base/random-signal.exp: attach: p wait_gdb = 0 (timeout) > ... > > gdb.log: > > (gdb) PASS: gdb.base/random-signal.exp: attach: watch v > continue > Continuing. > PASS: gdb.base/random-signal.exp: attach: continue > ^CPython Exception <type 'exceptions.KeyboardInterrupt'> <type > exceptions.KeyboardInterrupt'>: > FAIL: gdb.base/random-signal.exp: attach: stop with control-c (timeout) > p wait_gdb = 0 > FAIL: gdb.base/random-signal.exp: attach: p wait_gdb = 0 (timeout) > > Yes, it is a good test case. >> + if (!target_terminal_is_inferior ()) > > I think this should check for terminal_is_ours instead. We also > don't want this with terminal_ours_for_output. In that case, _input_ > is still sent to the inferior. OK, target_terminal_is_ours is added and is used here. Patch is updated as below, > > Here's the test tweak: > > From 2cf334c489d6278ea73e1f82905b9726f7d502fc Mon Sep 17 00:00:00 2001 > From: Pedro Alves <palves@redhat.com> > Date: Mon, 4 Jan 2016 14:25:31 +0000 > Subject: [PATCH] Test gdb.base/random-signal.exp with "attach" > > This exposes the issued fixed by: > https://sourceware.org/ml/gdb-patches/2015-12/msg00423.html > to native debugging as well. > > gdb/testsuite/ChangeLog: > 2016-01-04 Pedro Alves <palves@redhat.com> > > * gdb.base/random-signal.c (wait): New global. > (main): Use it. > * gdb.base/random-signal.exp (do_test): New procedure, with body > of testcase moved in. > (top level) Call it twice, once with "run" and once with > "attach". Your patch looks good to me. -- Yao (齐尧) From 43887b575d10f65def86f80794094404b6a3709e Mon Sep 17 00:00:00 2001 From: Yao Qi <yao.qi@linaro.org> Date: Fri, 18 Dec 2015 10:58:45 +0000 Subject: [PATCH] Change SIGINT handler for extension languages only when target terminal is ours I see a timeout in gdb.base/random-signal.exp, Continuing.^M PASS: gdb.base/random-signal.exp: continue ^CPython Exception <type 'exceptions.KeyboardInterrupt'> <type exceptions.KeyboardInterrupt'>: ^M FAIL: gdb.base/random-signal.exp: stop with control-c (timeout) it can be reproduced by running random-signal.exp with native-gdbserver in a loop, like this, and the fail will be shown in about 20 runs, $ (set -e; while true; do make check RUNTESTFLAGS="--target_board=native-gdbserver random-signal.exp"; done) In the test, the program is being single-stepped for software watchpoint, and in each internal stop, python unwinder sniffer is used, #0 pyuw_sniffer (self=<optimised out>, this_frame=<optimised out>, cache_ptr=0xd554f8) at /home/yao/SourceCode/gnu/gdb/git/gdb/python/py-unwind.c:608 #1 0x00000000006a10ae in frame_unwind_try_unwinder (this_frame=this_frame@entry=0xd554e0, this_cache=this_cache@entry=0xd554f8, unwinder=0xecd540) at /home/yao/SourceCode/gnu/gdb/git/gdb/frame-unwind.c:107 #2 0x00000000006a143f in frame_unwind_find_by_frame (this_frame=this_frame@entry=0xd554e0, this_cache=this_cache@entry=0xd554f8) at /home/yao/SourceCode/gnu/gdb/git/gdb/frame-unwind.c:163 #3 0x000000000069dc6b in compute_frame_id (fi=0xd554e0) at /home/yao/SourceCode/gnu/gdb/git/gdb/frame.c:454 #4 get_prev_frame_if_no_cycle (this_frame=this_frame@entry=0xd55410) at /home/yao/SourceCode/gnu/gdb/git/gdb/frame.c:1781 #5 0x000000000069fdb9 in get_prev_frame_always_1 (this_frame=0xd55410) at /home/yao/SourceCode/gnu/gdb/git/gdb/frame.c:1955 #6 get_prev_frame_always (this_frame=this_frame@entry=0xd55410) at /home/yao/SourceCode/gnu/gdb/git/gdb/frame.c:1971 #7 0x00000000006a04b1 in get_prev_frame (this_frame=this_frame@entry=0xd55410) at /home/yao/SourceCode/gnu/gdb/git/gdb/frame.c:2213 when GDB goes to python extension, or other language extension, the SIGINT handler is changed, and is restored when GDB leaves extension language. GDB only stays in extension language for a very short period in this case, but if ctrl-c is pressed at that moment, python extension will handle the SIGINT, and exceptions.KeyboardInterrupt is shown. Language extension is used in GDB side rather than inferior side, so GDB should only change SIGINT handler for extension language when the terminal is ours (not inferior's). This is what this patch does. With this patch applied, I run random-signal.exp in a loop for 18 hours, and no fail is shown. gdb: 2016-01-05 Yao Qi <yao.qi@linaro.org> * extension.c: Include target.h. (set_active_ext_lang): Only call install_gdb_sigint_handler, check_quit_flag, and set_quit_flag if target_terminal_is_ours returns false. (restore_active_ext_lang): Likewise. * target.c (target_terminal_is_ours): New function. * target.h (target_terminal_is_ours): Declare. diff --git a/gdb/extension.c b/gdb/extension.c index 5c23bcc..d2c5669 100644 --- a/gdb/extension.c +++ b/gdb/extension.c @@ -22,6 +22,7 @@ #include "defs.h" #include <signal.h> +#include "target.h" #include "auto-load.h" #include "breakpoint.h" #include "event-top.h" @@ -746,19 +747,24 @@ set_active_ext_lang (const struct extension_language_defn *now_active) = XCNEW (struct active_ext_lang_state); previous->ext_lang = active_ext_lang; + previous->sigint_handler.handler_saved = 0; active_ext_lang = now_active; - /* If the newly active extension language uses cooperative SIGINT handling - then ensure GDB's SIGINT handler is installed. */ - if (now_active->language == EXT_LANG_GDB - || now_active->ops->check_quit_flag != NULL) - install_gdb_sigint_handler (&previous->sigint_handler); - - /* If there's a SIGINT recorded in the cooperative extension languages, - move it to the new language, or save it in GDB's global flag if the newly - active extension language doesn't use cooperative SIGINT handling. */ - if (check_quit_flag ()) - set_quit_flag (); + if (target_terminal_is_ours ()) + { + /* If the newly active extension language uses cooperative SIGINT + handling then ensure GDB's SIGINT handler is installed. */ + if (now_active->language == EXT_LANG_GDB + || now_active->ops->check_quit_flag != NULL) + install_gdb_sigint_handler (&previous->sigint_handler); + + /* If there's a SIGINT recorded in the cooperative extension languages, + move it to the new language, or save it in GDB's global flag if the + newly active extension language doesn't use cooperative SIGINT + handling. */ + if (check_quit_flag ()) + set_quit_flag (); + } return previous; } @@ -772,16 +778,19 @@ restore_active_ext_lang (struct active_ext_lang_state *previous) active_ext_lang = previous->ext_lang; - /* Restore the previous SIGINT handler if one was saved. */ - if (previous->sigint_handler.handler_saved) - install_sigint_handler (&previous->sigint_handler); - - /* If there's a SIGINT recorded in the cooperative extension languages, - move it to the new language, or save it in GDB's global flag if the newly - active extension language doesn't use cooperative SIGINT handling. */ - if (check_quit_flag ()) - set_quit_flag (); - + if (target_terminal_is_ours ()) + { + /* Restore the previous SIGINT handler if one was saved. */ + if (previous->sigint_handler.handler_saved) + install_sigint_handler (&previous->sigint_handler); + + /* If there's a SIGINT recorded in the cooperative extension languages, + move it to the new language, or save it in GDB's global flag if the + newly active extension language doesn't use cooperative SIGINT + handling. */ + if (check_quit_flag ()) + set_quit_flag (); + } xfree (previous); } diff --git a/gdb/target.c b/gdb/target.c index d331fe4..e88d60c 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -466,6 +466,14 @@ target_terminal_is_inferior (void) /* See target.h. */ +int +target_terminal_is_ours (void) +{ + return (terminal_state == terminal_is_ours); +} + +/* See target.h. */ + void target_terminal_inferior (void) { diff --git a/gdb/target.h b/gdb/target.h index dd2896f..e1419a9 100644 --- a/gdb/target.h +++ b/gdb/target.h @@ -1503,6 +1503,10 @@ extern int target_remove_breakpoint (struct gdbarch *gdbarch, extern int target_terminal_is_inferior (void); +/* Returns true if our terminal settings are in effect. */ + +extern int target_terminal_is_ours (void); + /* Initialize the terminal settings we record for the inferior, before we actually run the inferior. */ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC 2/2] Change SIGINT handler for extension languages only when target terminal is ours 2016-01-05 16:52 ` Yao Qi @ 2016-01-07 17:24 ` Pedro Alves 2016-01-08 11:08 ` Yao Qi 0 siblings, 1 reply; 12+ messages in thread From: Pedro Alves @ 2016-01-07 17:24 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On 01/05/2016 04:52 PM, Yao Qi wrote: > Pedro Alves <palves@redhat.com> writes: > >> The reason this doesn't trigger with native debugging is that >> in that case, gdb is sharing the terminal with gdb, and with job > > I think you meant "gdb is sharing the terminal with the inferior". Indeed. >>> + if (!target_terminal_is_inferior ()) >> >> I think this should check for terminal_is_ours instead. We also >> don't want this with terminal_ours_for_output. In that case, _input_ >> is still sent to the inferior. > > OK, target_terminal_is_ours is added and is used here. Patch is updated > as below, This LGTM. > Your patch looks good to me. I'll push it after yours goes in. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC 2/2] Change SIGINT handler for extension languages only when target terminal is ours 2016-01-07 17:24 ` Pedro Alves @ 2016-01-08 11:08 ` Yao Qi 2016-01-12 13:11 ` Pedro Alves 0 siblings, 1 reply; 12+ messages in thread From: Yao Qi @ 2016-01-08 11:08 UTC (permalink / raw) To: Pedro Alves; +Cc: Yao Qi, gdb-patches Pedro Alves <palves@redhat.com> writes: > This LGTM. > Patch is pushed in. >> Your patch looks good to me. > > I'll push it after yours goes in. Please do that. Thanks for new test. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC 2/2] Change SIGINT handler for extension languages only when target terminal is ours 2016-01-08 11:08 ` Yao Qi @ 2016-01-12 13:11 ` Pedro Alves 0 siblings, 0 replies; 12+ messages in thread From: Pedro Alves @ 2016-01-12 13:11 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On 01/08/2016 11:08 AM, Yao Qi wrote: > Pedro Alves <palves@redhat.com> writes: >>> Your patch looks good to me. >> >> I'll push it after yours goes in. > > Please do that. Thanks for new test. > Done. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 12+ messages in thread
* [RFC 1/2] Check input interrupt first when reading packet 2015-12-21 11:30 [PATCH 0/2] Fix timeout in random-signal.exp Yao Qi 2015-12-21 11:30 ` [RFC 2/2] Change SIGINT handler for extension languages only when target terminal is ours Yao Qi @ 2015-12-21 11:30 ` Yao Qi 2016-01-04 17:40 ` Pedro Alves 1 sibling, 1 reply; 12+ messages in thread From: Yao Qi @ 2015-12-21 11:30 UTC (permalink / raw) To: gdb-patches Hi, I see timeout in one of several runs of random-signal.exp like this, $ (set -e; while true; do make check RUNTESTFLAGS="--target_board=native-gdbserver random-signal.exp"; done) In about every five runs, we can see a fail, PASS: gdb.base/random-signal.exp: continue ^CFAIL: gdb.base/random-signal.exp: stop with control-c (timeout) after some investigation, I find '\003' may be discarded by GDBserver when it is expecting '$'. In GDB side, both normal packets and '\003' are sent via function send, but GDBserver may receive them in one time, that is to say, in the GDBserver's receive buffer, '\003' may appear before or after normal packet. However, current GDBserver doesn't handle this case. With this patch applied, I don't see this fail in multiple runs. Although there is still another timeout fail, that is a different problem, the next patch will fix it. gdb/gdbserver: 2015-12-21 Yao Qi <yao.qi@linaro.org> * remote-utils.c (getpkt): If c is '\003', call target hook request_interrupt. --- gdb/gdbserver/remote-utils.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c index 05e3d63..8bb5b13 100644 --- a/gdb/gdbserver/remote-utils.c +++ b/gdb/gdbserver/remote-utils.c @@ -959,6 +959,15 @@ getpkt (char *buf) while (1) { c = readchar (); + + /* The '\003' may appear before or after each packet, so + check for an input interrupt. */ + if (c == '\003') + { + (*the_target->request_interrupt) (); + c = readchar (); + } + if (c == '$') break; if (remote_debug) -- 1.9.1 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC 1/2] Check input interrupt first when reading packet 2015-12-21 11:30 ` [RFC 1/2] Check input interrupt first when reading packet Yao Qi @ 2016-01-04 17:40 ` Pedro Alves 2016-01-05 16:43 ` Yao Qi 0 siblings, 1 reply; 12+ messages in thread From: Pedro Alves @ 2016-01-04 17:40 UTC (permalink / raw) To: Yao Qi, gdb-patches On 12/21/2015 11:30 AM, Yao Qi wrote: > Hi, > I see timeout in one of several runs of random-signal.exp like this, > > $ (set -e; while true; do make check RUNTESTFLAGS="--target_board=native-gdbserver random-signal.exp"; done) > > In about every five runs, we can see a fail, > > PASS: gdb.base/random-signal.exp: continue > ^CFAIL: gdb.base/random-signal.exp: stop with control-c (timeout) > > after some investigation, I find '\003' may be discarded by GDBserver when > it is expecting '$'. In GDB side, both normal packets and '\003' are sent > via function send, but GDBserver may receive them in one time, that is to By "in one time", ITYM, "at any time". Yeah, the target may have already stopped just while the user presses ctrl-c, and gdb sends '\003' over. E.g., the test is doing this in a loop: #1 -> vCont;s #2 -- process stops #3 -- kernel sends SIGCHLD to gdbserver #4 <- T05 (gdbserver processes SIGCHLD, sees SIGTRAP stop) #5 -- gdb processes the T05 #6 -- watchpoint didn't trigger, re-resume. The \003 can arrive at any time above. For example: #1 -> vCont;s #2 -- process stops #3 -- kernel sends SIGCHLD to gdbserver #4 <- T05 (gdbserver processes SIGCHLD, sees SIGTRAP stop) + #4.1 -> \003 (user presses ctrl-c) #5 -- gdb processes the T05 #6 -- watchpoint didn't trigger, re-resume. The trouble is that the manual says: "Interrupts received while the program is stopped are discarded." However, I can't see how that can work in general. This can also happen: #1 -> vCont;s #2 -- process stops + #2.1 -> \003 (user presses ctrl-c) #3 -- kernel sends SIGCHLD to gdbserver #4 <- T05 (gdbserver processes SIGCHLD, sees SIGTRAP stop) #5 -- gdb processes the T05 And in that case, if the SIGTRAP happens to cause a user-visible stop (e.g., a breakpoint hit), then the target ends up with a SIGINT queued, that is only seen on next resume (e.g., after the next "continue"). This is similar to the reason we print "Quit (expect signal SIGINT when the program is resumed)" on some ports (utils.c:quit). I once wrote a patch that would fix this while preserving that "while the program is stopped are discarded" invariant: https://sourceware.org/ml/gdb-patches/2013-05/msg00933.html https://sourceware.org/ml/gdb-patches/2013-05/msg00933/step_over.patch See long rationale at: https://sourceware.org/ml/gdb-patches/2013-05/txtHFb6rkZ8Dz.txt ... by making both gdb and gdbserver remember that the user pressed ctrl-c. But that was not complete -- a fuller fix would fix the user-interface issue of sometimes getting "Quit" instead of a SIGINT. The simpler approach of "not ignoring ctrl-c when stopped" is quite tempting. Given that the issue of a ctrl-c happening while the process had just stopped ending up producing an unexpected SIGINT when the program is next resumed can also happen with native debugging and "attach", maybe the simpler approach of always queueing is the right approach. We'll need to tweak the documentation though. > say, in the GDBserver's receive buffer, '\003' may appear before or after > normal packet. However, current GDBserver doesn't handle this case. > > With this patch applied, I don't see this fail in multiple runs. > Although there is still another timeout fail, that is a different problem, > the next patch will fix it. > > gdb/gdbserver: > > 2015-12-21 Yao Qi <yao.qi@linaro.org> > > * remote-utils.c (getpkt): If c is '\003', call target hook > request_interrupt. > --- > gdb/gdbserver/remote-utils.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c > index 05e3d63..8bb5b13 100644 > --- a/gdb/gdbserver/remote-utils.c > +++ b/gdb/gdbserver/remote-utils.c > @@ -959,6 +959,15 @@ getpkt (char *buf) > while (1) > { > c = readchar (); > + > + /* The '\003' may appear before or after each packet, so > + check for an input interrupt. */ > + if (c == '\003') > + { > + (*the_target->request_interrupt) (); > + c = readchar (); I'd write "continue;" instead of another readchar, (Pedantically, you could have another '\003' in the buffer.) > + } > + > if (c == '$') > break; > if (remote_debug) > Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC 1/2] Check input interrupt first when reading packet 2016-01-04 17:40 ` Pedro Alves @ 2016-01-05 16:43 ` Yao Qi 2016-01-07 17:26 ` Pedro Alves 0 siblings, 1 reply; 12+ messages in thread From: Yao Qi @ 2016-01-05 16:43 UTC (permalink / raw) To: Pedro Alves; +Cc: Yao Qi, gdb-patches Pedro Alves <palves@redhat.com> writes: Hi Pedro, sorry for the delayed reply. Takes much time reading these patches and discussions in archives. > The trouble is that the manual says: > > "Interrupts received while the program is stopped are discarded." > > However, I can't see how that can work in general. This can also happen: > > #1 -> vCont;s > #2 -- process stops > + #2.1 -> \003 (user presses ctrl-c) > #3 -- kernel sends SIGCHLD to gdbserver > #4 <- T05 (gdbserver processes SIGCHLD, sees SIGTRAP stop) > #5 -- gdb processes the T05 > > And in that case, if the SIGTRAP happens to cause a user-visible > stop (e.g., a breakpoint hit), then the target ends up with > a SIGINT queued, that is only seen on next resume (e.g., after > the next "continue"). This is similar to the reason we print > > "Quit (expect signal SIGINT when the program is resumed)" > > on some ports (utils.c:quit). This line of doc was added due to the request from Daniel's comments https://www.sourceware.org/ml/gdb/2005-11/msg00349.html however, I am not sure what is the conversation Daniel referred to. > > > I once wrote a patch that would fix this while preserving that > "while the program is stopped are discarded" invariant: > > https://sourceware.org/ml/gdb-patches/2013-05/msg00933.html > https://sourceware.org/ml/gdb-patches/2013-05/msg00933/step_over.patch > > See long rationale at: > > https://sourceware.org/ml/gdb-patches/2013-05/txtHFb6rkZ8Dz.txt > > ... by making both gdb and gdbserver remember that the user pressed ctrl-c. > > But that was not complete -- a fuller fix would fix the user-interface > issue of sometimes getting "Quit" instead of a SIGINT. > > The simpler approach of "not ignoring ctrl-c when stopped" is quite tempting. > Given that the issue of a ctrl-c happening while the process had just stopped > ending up producing an unexpected SIGINT when the program is next resumed can > also happen with native debugging and "attach", maybe the simpler approach > of always queueing is the right approach. We'll need to tweak the > documentation though. I am inclined to tweak the doc as well, because looks people at that moment believed that ^C is meaningless when the target is stopped. See https://www.sourceware.org/ml/gdb-patches/2005-11/msg00307.html >> diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c >> index 05e3d63..8bb5b13 100644 >> --- a/gdb/gdbserver/remote-utils.c >> +++ b/gdb/gdbserver/remote-utils.c >> @@ -959,6 +959,15 @@ getpkt (char *buf) >> while (1) >> { >> c = readchar (); >> + >> + /* The '\003' may appear before or after each packet, so >> + check for an input interrupt. */ >> + if (c == '\003') >> + { >> + (*the_target->request_interrupt) (); >> + c = readchar (); > > I'd write "continue;" instead of another readchar, > > (Pedantically, you could have another '\003' in the buffer.) Done. -- Yao (齐尧) From 266bf16a08d3ae1be6157cf360486d277e2820d4 Mon Sep 17 00:00:00 2001 From: Yao Qi <yao.qi@linaro.org> Date: Fri, 18 Dec 2015 12:29:29 +0000 Subject: [PATCH] Check input interrupt first when reading packet Hi, I see timeout in one of several runs of random-signal.exp like this, $ (set -e; while true; do make check RUNTESTFLAGS="--target_board=native-gdbserver random-signal.exp"; done) In about every five runs, we can see a fail, PASS: gdb.base/random-signal.exp: continue ^CFAIL: gdb.base/random-signal.exp: stop with control-c (timeout) after some investigation, I find '\003' may be discarded by GDBserver when it is expecting '$'. In GDB side, both normal packets and '\003' are sent via function send, but GDBserver may receive them at any time, that is to say, in the receive buffer in GDBserver, '\003' may appear before or after normal packet. However, current GDBserver doesn't handle this case. With this patch applied, I don't see this fail in multiple runs. Although there is still timeout fail, that is a different problem, the next patch will fix it. gdb/gdbserver: 2016-01-05 Yao Qi <yao.qi@linaro.org> * remote-utils.c (getpkt): If c is '\003', call target hook request_interrupt. diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c index 5f43820..c5f4647 100644 --- a/gdb/gdbserver/remote-utils.c +++ b/gdb/gdbserver/remote-utils.c @@ -959,6 +959,15 @@ getpkt (char *buf) while (1) { c = readchar (); + + /* The '\003' may appear before or after each packet, so + check for an input interrupt. */ + if (c == '\003') + { + (*the_target->request_interrupt) (); + continue; + } + if (c == '$') break; if (remote_debug) ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC 1/2] Check input interrupt first when reading packet 2016-01-05 16:43 ` Yao Qi @ 2016-01-07 17:26 ` Pedro Alves 2016-01-08 11:07 ` Yao Qi 0 siblings, 1 reply; 12+ messages in thread From: Pedro Alves @ 2016-01-07 17:26 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On 01/05/2016 04:43 PM, Yao Qi wrote: > Pedro Alves <palves@redhat.com> writes: > > Hi Pedro, > sorry for the delayed reply. Takes much time reading these patches and > discussions in archives. Likewise here. > I am inclined to tweak the doc as well, because looks people at that > moment believed that ^C is meaningless when the target is stopped. > See https://www.sourceware.org/ml/gdb-patches/2005-11/msg00307.html Alright, let's do this then. >>> diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c >>> index 05e3d63..8bb5b13 100644 >>> --- a/gdb/gdbserver/remote-utils.c >>> +++ b/gdb/gdbserver/remote-utils.c >>> @@ -959,6 +959,15 @@ getpkt (char *buf) >>> while (1) >>> { >>> c = readchar (); >>> + >>> + /* The '\003' may appear before or after each packet, so >>> + check for an input interrupt. */ >>> + if (c == '\003') >>> + { >>> + (*the_target->request_interrupt) (); >>> + c = readchar (); >> >> I'd write "continue;" instead of another readchar, >> >> (Pedantically, you could have another '\003' in the buffer.) > > Done. LGTM. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC 1/2] Check input interrupt first when reading packet 2016-01-07 17:26 ` Pedro Alves @ 2016-01-08 11:07 ` Yao Qi 0 siblings, 0 replies; 12+ messages in thread From: Yao Qi @ 2016-01-08 11:07 UTC (permalink / raw) To: Pedro Alves; +Cc: Yao Qi, gdb-patches Pedro Alves <palves@redhat.com> writes: >> I am inclined to tweak the doc as well, because looks people at that >> moment believed that ^C is meaningless when the target is stopped. >> See https://www.sourceware.org/ml/gdb-patches/2005-11/msg00307.html > > Alright, let's do this then. > > > LGTM. Patch is pushed in, and I'll work on the doc. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-01-12 13:11 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-12-21 11:30 [PATCH 0/2] Fix timeout in random-signal.exp Yao Qi 2015-12-21 11:30 ` [RFC 2/2] Change SIGINT handler for extension languages only when target terminal is ours Yao Qi 2016-01-04 16:12 ` Pedro Alves 2016-01-05 16:52 ` Yao Qi 2016-01-07 17:24 ` Pedro Alves 2016-01-08 11:08 ` Yao Qi 2016-01-12 13:11 ` Pedro Alves 2015-12-21 11:30 ` [RFC 1/2] Check input interrupt first when reading packet Yao Qi 2016-01-04 17:40 ` Pedro Alves 2016-01-05 16:43 ` Yao Qi 2016-01-07 17:26 ` Pedro Alves 2016-01-08 11:07 ` Yao Qi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).