* [PATCH 0/2] Issue reconnecting to remote target afte fork
@ 2018-06-25 13:09 Andrew Burgess
2018-06-25 13:09 ` [PATCH 2/2] gdb: Clean up inferior list when reconnecting to new target Andrew Burgess
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Andrew Burgess @ 2018-06-25 13:09 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
In this series two isses are addressed relating to reconnecting to an
extended-remote target after a fork.
The first patch is the most series, a GDB assertion failure, while the
second is more cosmetic, GDB leaving dead inferiors in the inferior
list after reconnecting.
---
Andrew Burgess (2):
gdb: Fix assert for extended-remote target
gdb: Clean up inferior list when reconnecting to new target
gdb/ChangeLog | 11 ++
gdb/target.c | 41 ++++++
gdb/testsuite/ChangeLog | 9 ++
gdb/testsuite/gdb.base/extended-remote-restart.c | 60 +++++++++
gdb/testsuite/gdb.base/extended-remote-restart.exp | 138 +++++++++++++++++++++
5 files changed, 259 insertions(+)
create mode 100644 gdb/testsuite/gdb.base/extended-remote-restart.c
create mode 100644 gdb/testsuite/gdb.base/extended-remote-restart.exp
--
2.12.2
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 2/2] gdb: Clean up inferior list when reconnecting to new target 2018-06-25 13:09 [PATCH 0/2] Issue reconnecting to remote target afte fork Andrew Burgess @ 2018-06-25 13:09 ` Andrew Burgess 2018-06-25 13:09 ` [PATCH 1/2] gdb: Fix assert for extended-remote target Andrew Burgess 2018-07-17 17:47 ` [PATCH 0/2] Issue reconnecting to remote target afte fork Andrew Burgess 2 siblings, 0 replies; 13+ messages in thread From: Andrew Burgess @ 2018-06-25 13:09 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess When connected to a target and running an inferior, if the inferior forks then a new, inferior is created to track the child process. Usually the second inferior (the child process) will be automatically cleaned up (removed) by GDB once the child has completed. However, if we connect to a new target then the second inferior is left around, and we can even end up using the second inferior in order to run a program on the new target, for example in this session: (gdb) target extended-remote :2347 (gdb) file /path/to/exe (gdb) set remote exec-file /path/to/exe (gdb) set detach-on-fork off (gdb) break breakpt (gdb) run # ... hits breakpoint (gdb) info inferiors Num Description Executable * 1 process 15406 /path/to/exe 2 process 15407 /path/to/exe (gdb) kill Kill the program being debugged? (y or n) y (gdb) info inferiors Num Description Executable * 1 <null> /path/to/exe 2 process 15433 /path/to/exe (gdb) target extended-remote :2348 (gdb) file /path/to/exe (gdb) set remote exec-file /path/to/exe (gdb) run # ... hits breakpoint (gdb) info inferiors Num Description Executable 1 <null> /path/to/exe * 2 process 15408 /path/to/exe 3 process 15409 /path/to/exe (gdb) Notice how after connecting to the second extended-remote target, and then running the test program we now have inferiors 1, 2, and 3. There's nothing really _wrong_ with this, but a better experience would, I think, be to have only inferiors 1 and 2 in the new target's session. The issue here is that in target.c:dispose_inferior GDB uses switch_to_thread, which also switches the current inferior. This leaves the last inferior in the list selected after target.c:target_preopen has completed. We could change target.c:dispose_inferior to ensure the selected inferior is not left changed, however, in the above example, I think that, even if the user manually selected inferior 2 _before_ connecting to the new target, the best behaviour would still be for GDB to switch back to inferior 1. This seems to suggest that the right thing to do is have GDB _choose_ a new inferior as part of target_preopen, after which is can call prune_inferiors to remove transient inferiors from the list. An existing test is extended to cover this behaviour. gdb/ChangeLog: * target.c (find_first_killed_inferior): New function. (target_preopen): Use find_first_killed_inferior to reset selected inferior, and prune killed inferior as appropriate. gdb/testsuite/ChangeLog: * gdb.base/extended-remote-restart.exp: Extend. --- gdb/ChangeLog | 6 ++++ gdb/target.c | 35 ++++++++++++++++++++++ gdb/testsuite/ChangeLog | 4 +++ gdb/testsuite/gdb.base/extended-remote-restart.exp | 6 ++++ 4 files changed, 51 insertions(+) diff --git a/gdb/target.c b/gdb/target.c index 7c297495703..870b2357688 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -2046,6 +2046,31 @@ dispose_inferior (struct inferior *inf, void *args) return 0; } +/* Callback for iterate_over_inferiors. Record in ARG the first killed + inferior. If we find an inferior that is both killed and not removable, + this is returned in preference. The definition of "first" here is + pretty loose, and depends on the order in the inferior list. */ + +static int +find_first_killed_inferior (struct inferior *inf, void *arg) +{ + struct inferior **infp = (struct inferior **) arg; + + if (inf->pid == 0) + { + if (!inf->removable) + { + *infp = inf; + return 1; + } + else if (*infp == nullptr) + *infp = inf; + } + + return 0; +} + + /* This is to be called by the open routine before it does anything. */ @@ -2062,6 +2087,16 @@ target_preopen (int from_tty) iterate_over_inferiors (dispose_inferior, NULL); else error (_("Program not killed.")); + + /* The call to DISPOSE_INFERIOR will leave the last inferior we + killed selected. Reset the selection to the earliest inferior + that is killed and not removable. The prune any other killed + inferiors. */ + struct inferior *inf = nullptr; + iterate_over_inferiors (find_first_killed_inferior, &inf); + if (inf != nullptr) + set_current_inferior (inf); + prune_inferiors (); } /* Calling target_kill may remove the target from the stack. But if diff --git a/gdb/testsuite/gdb.base/extended-remote-restart.exp b/gdb/testsuite/gdb.base/extended-remote-restart.exp index 39fcb9e2e58..fb4bdd1755c 100644 --- a/gdb/testsuite/gdb.base/extended-remote-restart.exp +++ b/gdb/testsuite/gdb.base/extended-remote-restart.exp @@ -119,6 +119,12 @@ proc test_reload { do_kill_p follow_child_p } { } else { fail "reconnect after fork" } + + gdb_test "info inferiors" \ + [multi_line \ + " Num Description Executable.*" \ + "\\* 1 +${dead_inf_ptn} \[^\r\n\]+" ] \ + "Check inferiors after reconnect" } # Run all combinations of the test. -- 2.12.2 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] gdb: Fix assert for extended-remote target 2018-06-25 13:09 [PATCH 0/2] Issue reconnecting to remote target afte fork Andrew Burgess 2018-06-25 13:09 ` [PATCH 2/2] gdb: Clean up inferior list when reconnecting to new target Andrew Burgess @ 2018-06-25 13:09 ` Andrew Burgess 2018-07-17 17:47 ` [PATCH 0/2] Issue reconnecting to remote target afte fork Andrew Burgess 2 siblings, 0 replies; 13+ messages in thread From: Andrew Burgess @ 2018-06-25 13:09 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess Consider the following GDB session: (gdb) target extended-remote :2347 (gdb) file /path/to/exe (gdb) set remote exec-file /path/to/exe (gdb) set detach-on-fork off (gdb) break breakpt (gdb) run # ... hits breakpoint (gdb) info inferiors Num Description Executable * 1 process 17001 /path/to/exe 2 process 17002 /path/to/exe (gdb) kill (gdb) info inferiors Num Description Executable * 1 <null> /path/to/exe 2 process 17002 /path/to/exe (gdb) target extended-remote :2348 ../../src/gdb/thread.c:660: internal-error: thread_info* any_thread_of_process(int): Assertion `pid != 0' failed. A problem internal to GDB has been detected, further debugging may prove unreliable. The issue is calling target.c:dispose_inferior with a killed inferior in the inferior list. This assertion is fixed in this commit. The new test for this issue only runs on platforms that support 'detach-on-fork', and when using '--target_board=native-extended-gdbserver'. gdb/ChangeLog: * target.c (dispose_inferior): Don't dispose of inferiors that are already killed. gdb/testsuite/ChangeLog: * gdb.base/extended-remote-restart.c: New file. * gdb.base/extended-remote-restart.exp: New file. --- gdb/ChangeLog | 5 + gdb/target.c | 6 + gdb/testsuite/ChangeLog | 5 + gdb/testsuite/gdb.base/extended-remote-restart.c | 60 ++++++++++ gdb/testsuite/gdb.base/extended-remote-restart.exp | 132 +++++++++++++++++++++ 5 files changed, 208 insertions(+) create mode 100644 gdb/testsuite/gdb.base/extended-remote-restart.c create mode 100644 gdb/testsuite/gdb.base/extended-remote-restart.exp diff --git a/gdb/target.c b/gdb/target.c index a082957d5bd..7c297495703 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -2025,6 +2025,12 @@ target_pre_inferior (int from_tty) static int dispose_inferior (struct inferior *inf, void *args) { + /* Not all killed inferiors can, or will ever be, removed from the + inferior list. Killed inferiors clearly don't need to be killed + again, so, we're done. */ + if (inf->pid == 0) + return 0; + thread_info *thread = any_thread_of_inferior (inf); if (thread != NULL) { diff --git a/gdb/testsuite/gdb.base/extended-remote-restart.c b/gdb/testsuite/gdb.base/extended-remote-restart.c new file mode 100644 index 00000000000..e195f48e080 --- /dev/null +++ b/gdb/testsuite/gdb.base/extended-remote-restart.c @@ -0,0 +1,60 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2018 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +#include <unistd.h> +#include <stdlib.h> + +static void +breakpt () +{ + asm ("" ::: "memory"); +} + +static void +go_child () +{ + breakpt (); + + while (1) + sleep (1); +} + +static void +go_parent () +{ + breakpt (); + + while (1) + sleep (1); +} + +int +main () +{ + pid_t pid; + + pid = fork (); + if (pid == -1) + abort (); + + if (pid == 0) + go_child (); + else + go_parent (); + + exit (EXIT_SUCCESS); +} diff --git a/gdb/testsuite/gdb.base/extended-remote-restart.exp b/gdb/testsuite/gdb.base/extended-remote-restart.exp new file mode 100644 index 00000000000..39fcb9e2e58 --- /dev/null +++ b/gdb/testsuite/gdb.base/extended-remote-restart.exp @@ -0,0 +1,132 @@ +# Copyright 2018 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +# This test is about restarting execution of a forked application when +# using gdb extended remote target. +# +# There are two issues that the test tries to expose in GDB: +# +# 1. GDB would throw an assertion upon reconnecting to a remote target +# if there was more than one inferior already active in GDB, and +# +# 2. GDB would not prune transient inferiors from the inferior list +# when reconnecting to a remote target. So, for example, an inferior +# created by GDB to track the child of a fork would usually be removed +# from the inferior list once the child exited. However, reconnecting +# to a remote target would result in the child inferior remaining in +# the inferior list. + +# This test is only for extended remote targets. +if {[target_info gdb_protocol] != "extended-remote"} { + continue +} + +# This test also makes use of 'detach-on-fork' which is not supported +# on all platforms. +if { ![istarget "*-*-linux*"] && ![istarget "*-*-openbsd*"] } then { + continue +} + +# And we need to be able to reconnect to gdbserver. +set gdbserver_reconnect_p 1 +if { [info proc gdb_reconnect] == "" } { + return 0 +} + +standard_testfile + +if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} { + return -1 +} + +# Core of the test. DO_KILL_P controls whether we kill one of the +# inferiors before reconnecting. And FOLLOW_CHILD_P controls whether +# we follow the child or the parent at the fork. +proc test_reload { do_kill_p follow_child_p } { + global decimal + global binfile + + clean_restart ${binfile} + + if ![runto_main] then { + fail "can't run to main" + return 0 + } + + # Set detach-on-fork off + gdb_test_no_output "set detach-on-fork off" + + set live_inf_ptn "process $decimal" + set dead_inf_ptn "<null>" + + if ${follow_child_p} { + gdb_test_no_output "set follow-fork child" + set parent_prefix " " + set child_prefix "\\*" + set parent_inf_after_kill_ptn ${live_inf_ptn} + set child_inf_after_kill_ptn ${dead_inf_ptn} + } else { + gdb_test_no_output "set follow-fork parent" + set parent_prefix "\\*" + set child_prefix " " + set parent_inf_after_kill_ptn ${dead_inf_ptn} + set child_inf_after_kill_ptn ${live_inf_ptn} + } + + gdb_breakpoint "breakpt" + gdb_continue_to_breakpoint "breakpt" + + # Check we have the expected inferiors. + gdb_test "info inferiors" \ + [multi_line \ + " Num Description Executable.*" \ + "${parent_prefix} 1 +${live_inf_ptn} \[^\r\n\]+" \ + "${child_prefix} 2 +${live_inf_ptn} \[^\r\n\]+" ] \ + "Check inferiors at breakpoint" + + if { $do_kill_p } { + # (Optional) Kill one of the inferiors. + gdb_test "kill" \ + "" \ + "Kill inferior" \ + "Kill the program being debugged.*y or n. $" \ + "y" + + # Check the first inferior really did die. + gdb_test "info inferiors" \ + [multi_line \ + " Num Description Executable.*" \ + "${parent_prefix} 1 +${parent_inf_after_kill_ptn} \[^\r\n\]+" \ + "${child_prefix} 2 +${child_inf_after_kill_ptn} \[^\r\n\]+" ] \ + "Check inferior was killed" + } + + # Reconnect to the target. + if { [gdb_reconnect] == 0 } { + pass "reconnect after fork" + } else { + fail "reconnect after fork" + } +} + +# Run all combinations of the test. +foreach do_kill_p [list 1 0] { + foreach follow_child_p [list 1 0] { + with_test_prefix \ + "kill: ${do_kill_p}, follow-child ${follow_child_p}" { + test_reload ${do_kill_p} ${follow_child_p} + } + } +} -- 2.12.2 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] Issue reconnecting to remote target afte fork 2018-06-25 13:09 [PATCH 0/2] Issue reconnecting to remote target afte fork Andrew Burgess 2018-06-25 13:09 ` [PATCH 2/2] gdb: Clean up inferior list when reconnecting to new target Andrew Burgess 2018-06-25 13:09 ` [PATCH 1/2] gdb: Fix assert for extended-remote target Andrew Burgess @ 2018-07-17 17:47 ` Andrew Burgess 2018-08-01 21:19 ` PING: [PATCHv2 " Andrew Burgess ` (2 more replies) 2 siblings, 3 replies; 13+ messages in thread From: Andrew Burgess @ 2018-07-17 17:47 UTC (permalink / raw) To: gdb-patches Ping. Thanks, Andrew * Andrew Burgess <andrew.burgess@embecosm.com> [2018-06-25 14:09:28 +0100]: > In this series two isses are addressed relating to reconnecting to an > extended-remote target after a fork. > > The first patch is the most series, a GDB assertion failure, while the > second is more cosmetic, GDB leaving dead inferiors in the inferior > list after reconnecting. > > --- > > Andrew Burgess (2): > gdb: Fix assert for extended-remote target > gdb: Clean up inferior list when reconnecting to new target > > gdb/ChangeLog | 11 ++ > gdb/target.c | 41 ++++++ > gdb/testsuite/ChangeLog | 9 ++ > gdb/testsuite/gdb.base/extended-remote-restart.c | 60 +++++++++ > gdb/testsuite/gdb.base/extended-remote-restart.exp | 138 +++++++++++++++++++++ > 5 files changed, 259 insertions(+) > create mode 100644 gdb/testsuite/gdb.base/extended-remote-restart.c > create mode 100644 gdb/testsuite/gdb.base/extended-remote-restart.exp > > -- > 2.12.2 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* PING: [PATCHv2 0/2] Issue reconnecting to remote target afte fork 2018-07-17 17:47 ` [PATCH 0/2] Issue reconnecting to remote target afte fork Andrew Burgess @ 2018-08-01 21:19 ` Andrew Burgess 2018-08-01 21:19 ` [PATCHv2 2/2] gdb: Clean up inferior list when reconnecting to new target Andrew Burgess 2018-08-01 21:19 ` [PATCHv2 1/2] gdb: Fix assert for extended-remote target Andrew Burgess 2 siblings, 0 replies; 13+ messages in thread From: Andrew Burgess @ 2018-08-01 21:19 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess Ping! Patches rebased onto latest master. Nothing else has changed. Thanks, Andrew --- Andrew Burgess (2): gdb: Fix assert for extended-remote target gdb: Clean up inferior list when reconnecting to new target gdb/ChangeLog | 11 ++ gdb/target.c | 41 ++++++ gdb/testsuite/ChangeLog | 9 ++ gdb/testsuite/gdb.base/extended-remote-restart.c | 60 +++++++++ gdb/testsuite/gdb.base/extended-remote-restart.exp | 138 +++++++++++++++++++++ 5 files changed, 259 insertions(+) create mode 100644 gdb/testsuite/gdb.base/extended-remote-restart.c create mode 100644 gdb/testsuite/gdb.base/extended-remote-restart.exp -- 2.14.4 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCHv2 2/2] gdb: Clean up inferior list when reconnecting to new target 2018-07-17 17:47 ` [PATCH 0/2] Issue reconnecting to remote target afte fork Andrew Burgess 2018-08-01 21:19 ` PING: [PATCHv2 " Andrew Burgess @ 2018-08-01 21:19 ` Andrew Burgess 2018-08-08 12:31 ` Andrew Burgess 2018-09-14 13:36 ` Pedro Alves 2018-08-01 21:19 ` [PATCHv2 1/2] gdb: Fix assert for extended-remote target Andrew Burgess 2 siblings, 2 replies; 13+ messages in thread From: Andrew Burgess @ 2018-08-01 21:19 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess When connected to a target and running an inferior, if the inferior forks then a new, inferior is created to track the child process. Usually the second inferior (the child process) will be automatically cleaned up (removed) by GDB once the child has completed. However, if we connect to a new target then the second inferior is left around, and we can even end up using the second inferior in order to run a program on the new target, for example in this session: (gdb) target extended-remote :2347 (gdb) file /path/to/exe (gdb) set remote exec-file /path/to/exe (gdb) set detach-on-fork off (gdb) break breakpt (gdb) run # ... hits breakpoint (gdb) info inferiors Num Description Executable * 1 process 15406 /path/to/exe 2 process 15407 /path/to/exe (gdb) kill Kill the program being debugged? (y or n) y (gdb) info inferiors Num Description Executable * 1 <null> /path/to/exe 2 process 15433 /path/to/exe (gdb) target extended-remote :2348 (gdb) file /path/to/exe (gdb) set remote exec-file /path/to/exe (gdb) run # ... hits breakpoint (gdb) info inferiors Num Description Executable 1 <null> /path/to/exe * 2 process 15408 /path/to/exe 3 process 15409 /path/to/exe (gdb) Notice how after connecting to the second extended-remote target, and then running the test program we now have inferiors 1, 2, and 3. There's nothing really _wrong_ with this, but a better experience would, I think, be to have only inferiors 1 and 2 in the new target's session. The issue here is that in target.c:dispose_inferior GDB uses switch_to_thread, which also switches the current inferior. This leaves the last inferior in the list selected after target.c:target_preopen has completed. We could change target.c:dispose_inferior to ensure the selected inferior is not left changed, however, in the above example, I think that, even if the user manually selected inferior 2 _before_ connecting to the new target, the best behaviour would still be for GDB to switch back to inferior 1. This seems to suggest that the right thing to do is have GDB _choose_ a new inferior as part of target_preopen, after which is can call prune_inferiors to remove transient inferiors from the list. An existing test is extended to cover this behaviour. gdb/ChangeLog: * target.c (find_first_killed_inferior): New function. (target_preopen): Use find_first_killed_inferior to reset selected inferior, and prune killed inferior as appropriate. gdb/testsuite/ChangeLog: * gdb.base/extended-remote-restart.exp: Extend. --- gdb/ChangeLog | 6 ++++ gdb/target.c | 35 ++++++++++++++++++++++ gdb/testsuite/ChangeLog | 4 +++ gdb/testsuite/gdb.base/extended-remote-restart.exp | 6 ++++ 4 files changed, 51 insertions(+) diff --git a/gdb/target.c b/gdb/target.c index f352e0d8283..d6b0bf42271 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -2043,6 +2043,31 @@ dispose_inferior (struct inferior *inf, void *args) return 0; } +/* Callback for iterate_over_inferiors. Record in ARG the first killed + inferior. If we find an inferior that is both killed and not removable, + this is returned in preference. The definition of "first" here is + pretty loose, and depends on the order in the inferior list. */ + +static int +find_first_killed_inferior (struct inferior *inf, void *arg) +{ + struct inferior **infp = (struct inferior **) arg; + + if (inf->pid == 0) + { + if (!inf->removable) + { + *infp = inf; + return 1; + } + else if (*infp == nullptr) + *infp = inf; + } + + return 0; +} + + /* This is to be called by the open routine before it does anything. */ @@ -2059,6 +2084,16 @@ target_preopen (int from_tty) iterate_over_inferiors (dispose_inferior, NULL); else error (_("Program not killed.")); + + /* The call to DISPOSE_INFERIOR will leave the last inferior we + killed selected. Reset the selection to the earliest inferior + that is killed and not removable. The prune any other killed + inferiors. */ + struct inferior *inf = nullptr; + iterate_over_inferiors (find_first_killed_inferior, &inf); + if (inf != nullptr) + set_current_inferior (inf); + prune_inferiors (); } /* Calling target_kill may remove the target from the stack. But if diff --git a/gdb/testsuite/gdb.base/extended-remote-restart.exp b/gdb/testsuite/gdb.base/extended-remote-restart.exp index 39fcb9e2e58..fb4bdd1755c 100644 --- a/gdb/testsuite/gdb.base/extended-remote-restart.exp +++ b/gdb/testsuite/gdb.base/extended-remote-restart.exp @@ -119,6 +119,12 @@ proc test_reload { do_kill_p follow_child_p } { } else { fail "reconnect after fork" } + + gdb_test "info inferiors" \ + [multi_line \ + " Num Description Executable.*" \ + "\\* 1 +${dead_inf_ptn} \[^\r\n\]+" ] \ + "Check inferiors after reconnect" } # Run all combinations of the test. -- 2.14.4 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCHv2 2/2] gdb: Clean up inferior list when reconnecting to new target 2018-08-01 21:19 ` [PATCHv2 2/2] gdb: Clean up inferior list when reconnecting to new target Andrew Burgess @ 2018-08-08 12:31 ` Andrew Burgess 2018-08-29 10:46 ` PING: " Andrew Burgess 2018-09-14 13:36 ` Pedro Alves 1 sibling, 1 reply; 13+ messages in thread From: Andrew Burgess @ 2018-08-08 12:31 UTC (permalink / raw) To: gdb-patches After pushing the first patch with a change of location for the test, this patch needed updating. Nothing else has changed except the location of the modified test script. Thanks, Andrew --- gdb: Clean up inferior list when reconnecting to new target When connected to a target and running an inferior, if the inferior forks then a new, inferior is created to track the child process. Usually the second inferior (the child process) will be automatically cleaned up (removed) by GDB once the child has completed. However, if we connect to a new target then the second inferior is left around, and we can even end up using the second inferior in order to run a program on the new target, for example in this session: (gdb) target extended-remote :2347 (gdb) file /path/to/exe (gdb) set remote exec-file /path/to/exe (gdb) set detach-on-fork off (gdb) break breakpt (gdb) run # ... hits breakpoint (gdb) info inferiors Num Description Executable * 1 process 15406 /path/to/exe 2 process 15407 /path/to/exe (gdb) kill Kill the program being debugged? (y or n) y (gdb) info inferiors Num Description Executable * 1 <null> /path/to/exe 2 process 15433 /path/to/exe (gdb) target extended-remote :2348 (gdb) file /path/to/exe (gdb) set remote exec-file /path/to/exe (gdb) run # ... hits breakpoint (gdb) info inferiors Num Description Executable 1 <null> /path/to/exe * 2 process 15408 /path/to/exe 3 process 15409 /path/to/exe (gdb) Notice how after connecting to the second extended-remote target, and then running the test program we now have inferiors 1, 2, and 3. There's nothing really _wrong_ with this, but a better experience would, I think, be to have only inferiors 1 and 2 in the new target's session. The issue here is that in target.c:dispose_inferior GDB uses switch_to_thread, which also switches the current inferior. This leaves the last inferior in the list selected after target.c:target_preopen has completed. We could change target.c:dispose_inferior to ensure the selected inferior is not left changed, however, in the above example, I think that, even if the user manually selected inferior 2 _before_ connecting to the new target, the best behaviour would still be for GDB to switch back to inferior 1. This seems to suggest that the right thing to do is have GDB _choose_ a new inferior as part of target_preopen, after which is can call prune_inferiors to remove transient inferiors from the list. An existing test is extended to cover this behaviour. gdb/ChangeLog: * target.c (find_first_killed_inferior): New function. (target_preopen): Use find_first_killed_inferior to reset selected inferior, and prune killed inferior as appropriate. gdb/testsuite/ChangeLog: * gdb.server/extended-remote-restart.exp: Extend. --- gdb/ChangeLog | 6 ++++ gdb/target.c | 35 ++++++++++++++++++++++ gdb/testsuite/ChangeLog | 4 +++ .../gdb.server/extended-remote-restart.exp | 6 ++++ 4 files changed, 51 insertions(+) diff --git a/gdb/target.c b/gdb/target.c index 115e9ae4948..7dfb5693f11 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -2043,6 +2043,31 @@ dispose_inferior (struct inferior *inf, void *args) return 0; } +/* Callback for iterate_over_inferiors. Record in ARG the first killed + inferior. If we find an inferior that is both killed and not removable, + this is returned in preference. The definition of "first" here is + pretty loose, and depends on the order in the inferior list. */ + +static int +find_first_killed_inferior (struct inferior *inf, void *arg) +{ + struct inferior **infp = (struct inferior **) arg; + + if (inf->pid == 0) + { + if (!inf->removable) + { + *infp = inf; + return 1; + } + else if (*infp == nullptr) + *infp = inf; + } + + return 0; +} + + /* This is to be called by the open routine before it does anything. */ @@ -2059,6 +2084,16 @@ target_preopen (int from_tty) iterate_over_inferiors (dispose_inferior, NULL); else error (_("Program not killed.")); + + /* The call to DISPOSE_INFERIOR will leave the last inferior we + killed selected. Reset the selection to the earliest inferior + that is killed and not removable. The prune any other killed + inferiors. */ + struct inferior *inf = nullptr; + iterate_over_inferiors (find_first_killed_inferior, &inf); + if (inf != nullptr) + set_current_inferior (inf); + prune_inferiors (); } /* Calling target_kill may remove the target from the stack. But if diff --git a/gdb/testsuite/gdb.server/extended-remote-restart.exp b/gdb/testsuite/gdb.server/extended-remote-restart.exp index 39fcb9e2e58..fb4bdd1755c 100644 --- a/gdb/testsuite/gdb.server/extended-remote-restart.exp +++ b/gdb/testsuite/gdb.server/extended-remote-restart.exp @@ -119,6 +119,12 @@ proc test_reload { do_kill_p follow_child_p } { } else { fail "reconnect after fork" } + + gdb_test "info inferiors" \ + [multi_line \ + " Num Description Executable.*" \ + "\\* 1 +${dead_inf_ptn} \[^\r\n\]+" ] \ + "Check inferiors after reconnect" } # Run all combinations of the test. -- 2.14.4 ^ permalink raw reply [flat|nested] 13+ messages in thread
* PING: Re: [PATCHv2 2/2] gdb: Clean up inferior list when reconnecting to new target 2018-08-08 12:31 ` Andrew Burgess @ 2018-08-29 10:46 ` Andrew Burgess 2018-09-13 10:52 ` Andrew Burgess 0 siblings, 1 reply; 13+ messages in thread From: Andrew Burgess @ 2018-08-29 10:46 UTC (permalink / raw) To: gdb-patches Ping! Thanks, Andrew * Andrew Burgess <andrew.burgess@embecosm.com> [2018-08-08 13:30:59 +0100]: > After pushing the first patch with a change of location for the test, > this patch needed updating. Nothing else has changed except the > location of the modified test script. > > Thanks, > Andrew > > --- > > gdb: Clean up inferior list when reconnecting to new target > > When connected to a target and running an inferior, if the inferior > forks then a new, inferior is created to track the child > process. Usually the second inferior (the child process) will be > automatically cleaned up (removed) by GDB once the child has completed. > However, if we connect to a new target then the second inferior is left > around, and we can even end up using the second inferior in order to run > a program on the new target, for example in this session: > > (gdb) target extended-remote :2347 > (gdb) file /path/to/exe > (gdb) set remote exec-file /path/to/exe > (gdb) set detach-on-fork off > (gdb) break breakpt > (gdb) run > # ... hits breakpoint > (gdb) info inferiors > Num Description Executable > * 1 process 15406 /path/to/exe > 2 process 15407 /path/to/exe > (gdb) kill > Kill the program being debugged? (y or n) y > (gdb) info inferiors > Num Description Executable > * 1 <null> /path/to/exe > 2 process 15433 /path/to/exe > (gdb) target extended-remote :2348 > (gdb) file /path/to/exe > (gdb) set remote exec-file /path/to/exe > (gdb) run > # ... hits breakpoint > (gdb) info inferiors > Num Description Executable > 1 <null> /path/to/exe > * 2 process 15408 /path/to/exe > 3 process 15409 /path/to/exe > (gdb) > > Notice how after connecting to the second extended-remote target, and > then running the test program we now have inferiors 1, 2, and 3. > There's nothing really _wrong_ with this, but a better experience would, > I think, be to have only inferiors 1 and 2 in the new target's session. > > The issue here is that in target.c:dispose_inferior GDB uses > switch_to_thread, which also switches the current inferior. This leaves > the last inferior in the list selected after target.c:target_preopen has > completed. > > We could change target.c:dispose_inferior to ensure the selected > inferior is not left changed, however, in the above example, I think > that, even if the user manually selected inferior 2 _before_ connecting > to the new target, the best behaviour would still be for GDB to switch > back to inferior 1. This seems to suggest that the right thing to do is > have GDB _choose_ a new inferior as part of target_preopen, after which > is can call prune_inferiors to remove transient inferiors from the list. > > An existing test is extended to cover this behaviour. > > gdb/ChangeLog: > > * target.c (find_first_killed_inferior): New function. > (target_preopen): Use find_first_killed_inferior to reset selected > inferior, and prune killed inferior as appropriate. > > gdb/testsuite/ChangeLog: > > * gdb.server/extended-remote-restart.exp: Extend. > --- > gdb/ChangeLog | 6 ++++ > gdb/target.c | 35 ++++++++++++++++++++++ > gdb/testsuite/ChangeLog | 4 +++ > .../gdb.server/extended-remote-restart.exp | 6 ++++ > 4 files changed, 51 insertions(+) > > diff --git a/gdb/target.c b/gdb/target.c > index 115e9ae4948..7dfb5693f11 100644 > --- a/gdb/target.c > +++ b/gdb/target.c > @@ -2043,6 +2043,31 @@ dispose_inferior (struct inferior *inf, void *args) > return 0; > } > > +/* Callback for iterate_over_inferiors. Record in ARG the first killed > + inferior. If we find an inferior that is both killed and not removable, > + this is returned in preference. The definition of "first" here is > + pretty loose, and depends on the order in the inferior list. */ > + > +static int > +find_first_killed_inferior (struct inferior *inf, void *arg) > +{ > + struct inferior **infp = (struct inferior **) arg; > + > + if (inf->pid == 0) > + { > + if (!inf->removable) > + { > + *infp = inf; > + return 1; > + } > + else if (*infp == nullptr) > + *infp = inf; > + } > + > + return 0; > +} > + > + > /* This is to be called by the open routine before it does > anything. */ > > @@ -2059,6 +2084,16 @@ target_preopen (int from_tty) > iterate_over_inferiors (dispose_inferior, NULL); > else > error (_("Program not killed.")); > + > + /* The call to DISPOSE_INFERIOR will leave the last inferior we > + killed selected. Reset the selection to the earliest inferior > + that is killed and not removable. The prune any other killed > + inferiors. */ > + struct inferior *inf = nullptr; > + iterate_over_inferiors (find_first_killed_inferior, &inf); > + if (inf != nullptr) > + set_current_inferior (inf); > + prune_inferiors (); > } > > /* Calling target_kill may remove the target from the stack. But if > diff --git a/gdb/testsuite/gdb.server/extended-remote-restart.exp b/gdb/testsuite/gdb.server/extended-remote-restart.exp > index 39fcb9e2e58..fb4bdd1755c 100644 > --- a/gdb/testsuite/gdb.server/extended-remote-restart.exp > +++ b/gdb/testsuite/gdb.server/extended-remote-restart.exp > @@ -119,6 +119,12 @@ proc test_reload { do_kill_p follow_child_p } { > } else { > fail "reconnect after fork" > } > + > + gdb_test "info inferiors" \ > + [multi_line \ > + " Num Description Executable.*" \ > + "\\* 1 +${dead_inf_ptn} \[^\r\n\]+" ] \ > + "Check inferiors after reconnect" > } > > # Run all combinations of the test. > -- > 2.14.4 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: PING: Re: [PATCHv2 2/2] gdb: Clean up inferior list when reconnecting to new target 2018-08-29 10:46 ` PING: " Andrew Burgess @ 2018-09-13 10:52 ` Andrew Burgess 0 siblings, 0 replies; 13+ messages in thread From: Andrew Burgess @ 2018-09-13 10:52 UTC (permalink / raw) To: gdb-patches * Andrew Burgess <andrew.burgess@embecosm.com> [2018-08-29 11:46:00 +0100]: Ping! > Ping! > > Thanks, > Andrew > > * Andrew Burgess <andrew.burgess@embecosm.com> [2018-08-08 13:30:59 +0100]: > > > After pushing the first patch with a change of location for the test, > > this patch needed updating. Nothing else has changed except the > > location of the modified test script. > > > > Thanks, > > Andrew > > > > --- > > > > gdb: Clean up inferior list when reconnecting to new target > > > > When connected to a target and running an inferior, if the inferior > > forks then a new, inferior is created to track the child > > process. Usually the second inferior (the child process) will be > > automatically cleaned up (removed) by GDB once the child has completed. > > However, if we connect to a new target then the second inferior is left > > around, and we can even end up using the second inferior in order to run > > a program on the new target, for example in this session: > > > > (gdb) target extended-remote :2347 > > (gdb) file /path/to/exe > > (gdb) set remote exec-file /path/to/exe > > (gdb) set detach-on-fork off > > (gdb) break breakpt > > (gdb) run > > # ... hits breakpoint > > (gdb) info inferiors > > Num Description Executable > > * 1 process 15406 /path/to/exe > > 2 process 15407 /path/to/exe > > (gdb) kill > > Kill the program being debugged? (y or n) y > > (gdb) info inferiors > > Num Description Executable > > * 1 <null> /path/to/exe > > 2 process 15433 /path/to/exe > > (gdb) target extended-remote :2348 > > (gdb) file /path/to/exe > > (gdb) set remote exec-file /path/to/exe > > (gdb) run > > # ... hits breakpoint > > (gdb) info inferiors > > Num Description Executable > > 1 <null> /path/to/exe > > * 2 process 15408 /path/to/exe > > 3 process 15409 /path/to/exe > > (gdb) > > > > Notice how after connecting to the second extended-remote target, and > > then running the test program we now have inferiors 1, 2, and 3. > > There's nothing really _wrong_ with this, but a better experience would, > > I think, be to have only inferiors 1 and 2 in the new target's session. > > > > The issue here is that in target.c:dispose_inferior GDB uses > > switch_to_thread, which also switches the current inferior. This leaves > > the last inferior in the list selected after target.c:target_preopen has > > completed. > > > > We could change target.c:dispose_inferior to ensure the selected > > inferior is not left changed, however, in the above example, I think > > that, even if the user manually selected inferior 2 _before_ connecting > > to the new target, the best behaviour would still be for GDB to switch > > back to inferior 1. This seems to suggest that the right thing to do is > > have GDB _choose_ a new inferior as part of target_preopen, after which > > is can call prune_inferiors to remove transient inferiors from the list. > > > > An existing test is extended to cover this behaviour. > > > > gdb/ChangeLog: > > > > * target.c (find_first_killed_inferior): New function. > > (target_preopen): Use find_first_killed_inferior to reset selected > > inferior, and prune killed inferior as appropriate. > > > > gdb/testsuite/ChangeLog: > > > > * gdb.server/extended-remote-restart.exp: Extend. > > --- > > gdb/ChangeLog | 6 ++++ > > gdb/target.c | 35 ++++++++++++++++++++++ > > gdb/testsuite/ChangeLog | 4 +++ > > .../gdb.server/extended-remote-restart.exp | 6 ++++ > > 4 files changed, 51 insertions(+) > > > > diff --git a/gdb/target.c b/gdb/target.c > > index 115e9ae4948..7dfb5693f11 100644 > > --- a/gdb/target.c > > +++ b/gdb/target.c > > @@ -2043,6 +2043,31 @@ dispose_inferior (struct inferior *inf, void *args) > > return 0; > > } > > > > +/* Callback for iterate_over_inferiors. Record in ARG the first killed > > + inferior. If we find an inferior that is both killed and not removable, > > + this is returned in preference. The definition of "first" here is > > + pretty loose, and depends on the order in the inferior list. */ > > + > > +static int > > +find_first_killed_inferior (struct inferior *inf, void *arg) > > +{ > > + struct inferior **infp = (struct inferior **) arg; > > + > > + if (inf->pid == 0) > > + { > > + if (!inf->removable) > > + { > > + *infp = inf; > > + return 1; > > + } > > + else if (*infp == nullptr) > > + *infp = inf; > > + } > > + > > + return 0; > > +} > > + > > + > > /* This is to be called by the open routine before it does > > anything. */ > > > > @@ -2059,6 +2084,16 @@ target_preopen (int from_tty) > > iterate_over_inferiors (dispose_inferior, NULL); > > else > > error (_("Program not killed.")); > > + > > + /* The call to DISPOSE_INFERIOR will leave the last inferior we > > + killed selected. Reset the selection to the earliest inferior > > + that is killed and not removable. The prune any other killed > > + inferiors. */ > > + struct inferior *inf = nullptr; > > + iterate_over_inferiors (find_first_killed_inferior, &inf); > > + if (inf != nullptr) > > + set_current_inferior (inf); > > + prune_inferiors (); > > } > > > > /* Calling target_kill may remove the target from the stack. But if > > diff --git a/gdb/testsuite/gdb.server/extended-remote-restart.exp b/gdb/testsuite/gdb.server/extended-remote-restart.exp > > index 39fcb9e2e58..fb4bdd1755c 100644 > > --- a/gdb/testsuite/gdb.server/extended-remote-restart.exp > > +++ b/gdb/testsuite/gdb.server/extended-remote-restart.exp > > @@ -119,6 +119,12 @@ proc test_reload { do_kill_p follow_child_p } { > > } else { > > fail "reconnect after fork" > > } > > + > > + gdb_test "info inferiors" \ > > + [multi_line \ > > + " Num Description Executable.*" \ > > + "\\* 1 +${dead_inf_ptn} \[^\r\n\]+" ] \ > > + "Check inferiors after reconnect" > > } > > > > # Run all combinations of the test. > > -- > > 2.14.4 > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCHv2 2/2] gdb: Clean up inferior list when reconnecting to new target 2018-08-01 21:19 ` [PATCHv2 2/2] gdb: Clean up inferior list when reconnecting to new target Andrew Burgess 2018-08-08 12:31 ` Andrew Burgess @ 2018-09-14 13:36 ` Pedro Alves 1 sibling, 0 replies; 13+ messages in thread From: Pedro Alves @ 2018-09-14 13:36 UTC (permalink / raw) To: Andrew Burgess, gdb-patches On 08/01/2018 10:19 PM, Andrew Burgess wrote: > When connected to a target and running an inferior, if the inferior > forks then a new, inferior is created to track the child > process. Usually the second inferior (the child process) will be > automatically cleaned up (removed) by GDB once the child has completed. > However, if we connect to a new target then the second inferior is left > around, and we can even end up using the second inferior in order to run > a program on the new target, for example in this session: > > (gdb) target extended-remote :2347 > (gdb) file /path/to/exe > (gdb) set remote exec-file /path/to/exe > (gdb) set detach-on-fork off > (gdb) break breakpt > (gdb) run > # ... hits breakpoint > (gdb) info inferiors > Num Description Executable > * 1 process 15406 /path/to/exe > 2 process 15407 /path/to/exe > (gdb) kill > Kill the program being debugged? (y or n) y > (gdb) info inferiors > Num Description Executable > * 1 <null> /path/to/exe > 2 process 15433 /path/to/exe Does this PID here really correspond to the same debug session? I'm confused, since 15433 wasn't in the list above. I assume it should be 15407? > (gdb) target extended-remote :2348 > (gdb) file /path/to/exe > (gdb) set remote exec-file /path/to/exe > (gdb) run > # ... hits breakpoint > (gdb) info inferiors > Num Description Executable > 1 <null> /path/to/exe > * 2 process 15408 /path/to/exe > 3 process 15409 /path/to/exe > (gdb) And here, same confusion. What really happened? I think this is showing that process 15433 (or was it 15407) was killed/disposed, and a new process (15408) that forked was spawned, and then that process forked 15409. Right? > > Notice how after connecting to the second extended-remote target, and > then running the test program we now have inferiors 1, 2, and 3. > There's nothing really _wrong_ with this, but a better experience would, > I think, be to have only inferiors 1 and 2 in the new target's session. > > The issue here is that in target.c:dispose_inferior GDB uses > switch_to_thread, which also switches the current inferior. This leaves > the last inferior in the list selected after target.c:target_preopen has > completed. > > We could change target.c:dispose_inferior to ensure the selected > inferior is not left changed, however, in the above example, I think > that, even if the user manually selected inferior 2 _before_ connecting > to the new target, the best behaviour would still be for GDB to switch > back to inferior 1. This seems to suggest that the right thing to do is > have GDB _choose_ a new inferior as part of target_preopen, after which > is can call prune_inferiors to remove transient inferiors from the list. > > An existing test is extended to cover this behaviour. I'm a bit confused with the examples above, but off hand, I'm having trouble liking the idea of GDB selecting an inferior on its own. It seems to me like this conflicts with multi-target? For example, consider we have a setup like this one: (gdb) info inferiors Num Description Connection Executable 1 <null> 1 (extended-remote :11111) /path/to/exe 2 <null> /path/to/exe 3 <null> 2 (native) /path/to/exe 4 <null> 1 (extended-remote :11111) /path/to/exe 5 <null> 3 (extended-remote :33333) /path/to/foo * 6 process 15433 3 (extended-remote :33333) /path/to/bar 7 process 15433 3 (extended-remote :33333) /path/to/qux If we connect to a new target while inferior 6 is selected, it would seem quite natural to me to stay with inferior 6 selected. Selecting inferiors 1 to 4 would seem unnatural to me. Inferiors 1, 3 and 4 are connected to a different target. Inferior 2 is not connected to a target yet, but then again, it has a different executable loaded. Similarly, inferior 5 is connected to the same target and not running, but, it has an executable loaded which is different from inferior 5's executable. It's really not clear at all to me why should GDB pick a different inferior. It seems to me that staying with the current inferior is the least surprising and best option. > > gdb/ChangeLog: > > * target.c (find_first_killed_inferior): New function. > (target_preopen): Use find_first_killed_inferior to reset selected > inferior, and prune killed inferior as appropriate. > > gdb/testsuite/ChangeLog: > > * gdb.base/extended-remote-restart.exp: Extend. > --- > gdb/ChangeLog | 6 ++++ > gdb/target.c | 35 ++++++++++++++++++++++ > gdb/testsuite/ChangeLog | 4 +++ > gdb/testsuite/gdb.base/extended-remote-restart.exp | 6 ++++ > 4 files changed, 51 insertions(+) > > diff --git a/gdb/target.c b/gdb/target.c > index f352e0d8283..d6b0bf42271 100644 > --- a/gdb/target.c > +++ b/gdb/target.c > @@ -2043,6 +2043,31 @@ dispose_inferior (struct inferior *inf, void *args) > return 0; > } > > +/* Callback for iterate_over_inferiors. Record in ARG the first killed > + inferior. If we find an inferior that is both killed and not removable, > + this is returned in preference. The definition of "first" here is > + pretty loose, and depends on the order in the inferior list. */ > + > +static int > +find_first_killed_inferior (struct inferior *inf, void *arg) > +{ > + struct inferior **infp = (struct inferior **) arg; > + > + if (inf->pid == 0) > + { > + if (!inf->removable) > + { > + *infp = inf; > + return 1; > + } > + else if (*infp == nullptr) > + *infp = inf; > + } > + > + return 0; > +} > + > + > /* This is to be called by the open routine before it does > anything. */ > > @@ -2059,6 +2084,16 @@ target_preopen (int from_tty) > iterate_over_inferiors (dispose_inferior, NULL); > else > error (_("Program not killed.")); > + > + /* The call to DISPOSE_INFERIOR will leave the last inferior we > + killed selected. Reset the selection to the earliest inferior > + that is killed and not removable. The prune any other killed > + inferiors. */ > + struct inferior *inf = nullptr; > + iterate_over_inferiors (find_first_killed_inferior, &inf); > + if (inf != nullptr) > + set_current_inferior (inf); > + prune_inferiors (); > } > > /* Calling target_kill may remove the target from the stack. But if > diff --git a/gdb/testsuite/gdb.base/extended-remote-restart.exp b/gdb/testsuite/gdb.base/extended-remote-restart.exp > index 39fcb9e2e58..fb4bdd1755c 100644 > --- a/gdb/testsuite/gdb.base/extended-remote-restart.exp > +++ b/gdb/testsuite/gdb.base/extended-remote-restart.exp > @@ -119,6 +119,12 @@ proc test_reload { do_kill_p follow_child_p } { > } else { > fail "reconnect after fork" > } > + > + gdb_test "info inferiors" \ > + [multi_line \ > + " Num Description Executable.*" \ > + "\\* 1 +${dead_inf_ptn} \[^\r\n\]+" ] \ > + "Check inferiors after reconnect" > } > > # Run all combinations of the test. > Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCHv2 1/2] gdb: Fix assert for extended-remote target 2018-07-17 17:47 ` [PATCH 0/2] Issue reconnecting to remote target afte fork Andrew Burgess 2018-08-01 21:19 ` PING: [PATCHv2 " Andrew Burgess 2018-08-01 21:19 ` [PATCHv2 2/2] gdb: Clean up inferior list when reconnecting to new target Andrew Burgess @ 2018-08-01 21:19 ` Andrew Burgess 2018-08-02 19:21 ` Tom Tromey 2 siblings, 1 reply; 13+ messages in thread From: Andrew Burgess @ 2018-08-01 21:19 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess Consider the following GDB session: (gdb) target extended-remote :2347 (gdb) file /path/to/exe (gdb) set remote exec-file /path/to/exe (gdb) set detach-on-fork off (gdb) break breakpt (gdb) run # ... hits breakpoint (gdb) info inferiors Num Description Executable * 1 process 17001 /path/to/exe 2 process 17002 /path/to/exe (gdb) kill (gdb) info inferiors Num Description Executable * 1 <null> /path/to/exe 2 process 17002 /path/to/exe (gdb) target extended-remote :2348 ../../src/gdb/thread.c:660: internal-error: thread_info* any_thread_of_process(int): Assertion `pid != 0' failed. A problem internal to GDB has been detected, further debugging may prove unreliable. The issue is calling target.c:dispose_inferior with a killed inferior in the inferior list. This assertion is fixed in this commit. The new test for this issue only runs on platforms that support 'detach-on-fork', and when using '--target_board=native-extended-gdbserver'. gdb/ChangeLog: * target.c (dispose_inferior): Don't dispose of inferiors that are already killed. gdb/testsuite/ChangeLog: * gdb.base/extended-remote-restart.c: New file. * gdb.base/extended-remote-restart.exp: New file. --- gdb/ChangeLog | 5 + gdb/target.c | 6 + gdb/testsuite/ChangeLog | 5 + gdb/testsuite/gdb.base/extended-remote-restart.c | 60 ++++++++++ gdb/testsuite/gdb.base/extended-remote-restart.exp | 132 +++++++++++++++++++++ 5 files changed, 208 insertions(+) create mode 100644 gdb/testsuite/gdb.base/extended-remote-restart.c create mode 100644 gdb/testsuite/gdb.base/extended-remote-restart.exp diff --git a/gdb/target.c b/gdb/target.c index eba07cc9174..f352e0d8283 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -2022,6 +2022,12 @@ target_pre_inferior (int from_tty) static int dispose_inferior (struct inferior *inf, void *args) { + /* Not all killed inferiors can, or will ever be, removed from the + inferior list. Killed inferiors clearly don't need to be killed + again, so, we're done. */ + if (inf->pid == 0) + return 0; + thread_info *thread = any_thread_of_inferior (inf); if (thread != NULL) { diff --git a/gdb/testsuite/gdb.base/extended-remote-restart.c b/gdb/testsuite/gdb.base/extended-remote-restart.c new file mode 100644 index 00000000000..e195f48e080 --- /dev/null +++ b/gdb/testsuite/gdb.base/extended-remote-restart.c @@ -0,0 +1,60 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2018 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +#include <unistd.h> +#include <stdlib.h> + +static void +breakpt () +{ + asm ("" ::: "memory"); +} + +static void +go_child () +{ + breakpt (); + + while (1) + sleep (1); +} + +static void +go_parent () +{ + breakpt (); + + while (1) + sleep (1); +} + +int +main () +{ + pid_t pid; + + pid = fork (); + if (pid == -1) + abort (); + + if (pid == 0) + go_child (); + else + go_parent (); + + exit (EXIT_SUCCESS); +} diff --git a/gdb/testsuite/gdb.base/extended-remote-restart.exp b/gdb/testsuite/gdb.base/extended-remote-restart.exp new file mode 100644 index 00000000000..39fcb9e2e58 --- /dev/null +++ b/gdb/testsuite/gdb.base/extended-remote-restart.exp @@ -0,0 +1,132 @@ +# Copyright 2018 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +# This test is about restarting execution of a forked application when +# using gdb extended remote target. +# +# There are two issues that the test tries to expose in GDB: +# +# 1. GDB would throw an assertion upon reconnecting to a remote target +# if there was more than one inferior already active in GDB, and +# +# 2. GDB would not prune transient inferiors from the inferior list +# when reconnecting to a remote target. So, for example, an inferior +# created by GDB to track the child of a fork would usually be removed +# from the inferior list once the child exited. However, reconnecting +# to a remote target would result in the child inferior remaining in +# the inferior list. + +# This test is only for extended remote targets. +if {[target_info gdb_protocol] != "extended-remote"} { + continue +} + +# This test also makes use of 'detach-on-fork' which is not supported +# on all platforms. +if { ![istarget "*-*-linux*"] && ![istarget "*-*-openbsd*"] } then { + continue +} + +# And we need to be able to reconnect to gdbserver. +set gdbserver_reconnect_p 1 +if { [info proc gdb_reconnect] == "" } { + return 0 +} + +standard_testfile + +if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} { + return -1 +} + +# Core of the test. DO_KILL_P controls whether we kill one of the +# inferiors before reconnecting. And FOLLOW_CHILD_P controls whether +# we follow the child or the parent at the fork. +proc test_reload { do_kill_p follow_child_p } { + global decimal + global binfile + + clean_restart ${binfile} + + if ![runto_main] then { + fail "can't run to main" + return 0 + } + + # Set detach-on-fork off + gdb_test_no_output "set detach-on-fork off" + + set live_inf_ptn "process $decimal" + set dead_inf_ptn "<null>" + + if ${follow_child_p} { + gdb_test_no_output "set follow-fork child" + set parent_prefix " " + set child_prefix "\\*" + set parent_inf_after_kill_ptn ${live_inf_ptn} + set child_inf_after_kill_ptn ${dead_inf_ptn} + } else { + gdb_test_no_output "set follow-fork parent" + set parent_prefix "\\*" + set child_prefix " " + set parent_inf_after_kill_ptn ${dead_inf_ptn} + set child_inf_after_kill_ptn ${live_inf_ptn} + } + + gdb_breakpoint "breakpt" + gdb_continue_to_breakpoint "breakpt" + + # Check we have the expected inferiors. + gdb_test "info inferiors" \ + [multi_line \ + " Num Description Executable.*" \ + "${parent_prefix} 1 +${live_inf_ptn} \[^\r\n\]+" \ + "${child_prefix} 2 +${live_inf_ptn} \[^\r\n\]+" ] \ + "Check inferiors at breakpoint" + + if { $do_kill_p } { + # (Optional) Kill one of the inferiors. + gdb_test "kill" \ + "" \ + "Kill inferior" \ + "Kill the program being debugged.*y or n. $" \ + "y" + + # Check the first inferior really did die. + gdb_test "info inferiors" \ + [multi_line \ + " Num Description Executable.*" \ + "${parent_prefix} 1 +${parent_inf_after_kill_ptn} \[^\r\n\]+" \ + "${child_prefix} 2 +${child_inf_after_kill_ptn} \[^\r\n\]+" ] \ + "Check inferior was killed" + } + + # Reconnect to the target. + if { [gdb_reconnect] == 0 } { + pass "reconnect after fork" + } else { + fail "reconnect after fork" + } +} + +# Run all combinations of the test. +foreach do_kill_p [list 1 0] { + foreach follow_child_p [list 1 0] { + with_test_prefix \ + "kill: ${do_kill_p}, follow-child ${follow_child_p}" { + test_reload ${do_kill_p} ${follow_child_p} + } + } +} -- 2.14.4 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCHv2 1/2] gdb: Fix assert for extended-remote target 2018-08-01 21:19 ` [PATCHv2 1/2] gdb: Fix assert for extended-remote target Andrew Burgess @ 2018-08-02 19:21 ` Tom Tromey 2018-08-08 12:23 ` Andrew Burgess 0 siblings, 1 reply; 13+ messages in thread From: Tom Tromey @ 2018-08-02 19:21 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes: Andrew> gdb/ChangeLog: Andrew> * target.c (dispose_inferior): Don't dispose of inferiors that are Andrew> already killed. I think this is PR gdb/18050, so please mention that in the ChangeLog. Thanks for the patch. This is ok with that little change. Tom ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCHv2 1/2] gdb: Fix assert for extended-remote target 2018-08-02 19:21 ` Tom Tromey @ 2018-08-08 12:23 ` Andrew Burgess 0 siblings, 0 replies; 13+ messages in thread From: Andrew Burgess @ 2018-08-08 12:23 UTC (permalink / raw) To: gdb-patches; +Cc: Tom Tromey * Tom Tromey <tom@tromey.com> [2018-08-02 13:21:36 -0600]: > >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes: > > Andrew> gdb/ChangeLog: > > Andrew> * target.c (dispose_inferior): Don't dispose of inferiors that are > Andrew> already killed. > > I think this is PR gdb/18050, so please mention that in the ChangeLog. > > Thanks for the patch. This is ok with that little change. Thanks for the review. The version I pushed is below, I made the following changes: * Updated description to mention PR gdb/18050 * Moved the test into gdb.server/ as this seemed a better location. Thanks, Andrew --- gdb: Fix assert for extended-remote target (PR gdb/18050) Consider the following GDB session: (gdb) target extended-remote :2347 (gdb) file /path/to/exe (gdb) set remote exec-file /path/to/exe (gdb) set detach-on-fork off (gdb) break breakpt (gdb) run # ... hits breakpoint (gdb) info inferiors Num Description Executable * 1 process 17001 /path/to/exe 2 process 17002 /path/to/exe (gdb) kill (gdb) info inferiors Num Description Executable * 1 <null> /path/to/exe 2 process 17002 /path/to/exe (gdb) target extended-remote :2348 ../../src/gdb/thread.c:660: internal-error: thread_info* any_thread_of_process(int): Assertion `pid != 0' failed. A problem internal to GDB has been detected, further debugging may prove unreliable. Or, from bug PR gdb/18050: (gdb) start (gdb) add-inferior -exec /path/to/exe (gdb) target extended-remote :2347 ../../src/gdb/thread.c:660: internal-error: thread_info* any_thread_of_process(int): Assertion `pid != 0' failed. A problem internal to GDB has been detected, further debugging may prove unreliable. The issue is calling target.c:dispose_inferior with a killed inferior in the inferior list. This assertion is fixed in this commit. The new test for this issue only runs on platforms that support 'detach-on-fork', and when using '--target_board=native-extended-gdbserver'. gdb/ChangeLog: PR gdb/18050: * target.c (dispose_inferior): Don't dispose of inferiors that are already killed. gdb/testsuite/ChangeLog: PR gdb/18050: * gdb.server/extended-remote-restart.c: New file. * gdb.server/extended-remote-restart.exp: New file. --- gdb/ChangeLog | 6 + gdb/target.c | 6 + gdb/testsuite/ChangeLog | 6 + gdb/testsuite/gdb.server/extended-remote-restart.c | 60 ++++++++++ .../gdb.server/extended-remote-restart.exp | 132 +++++++++++++++++++++ 5 files changed, 210 insertions(+) create mode 100644 gdb/testsuite/gdb.server/extended-remote-restart.c create mode 100644 gdb/testsuite/gdb.server/extended-remote-restart.exp diff --git a/gdb/target.c b/gdb/target.c index a5245abb281..115e9ae4948 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -2022,6 +2022,12 @@ target_pre_inferior (int from_tty) static int dispose_inferior (struct inferior *inf, void *args) { + /* Not all killed inferiors can, or will ever be, removed from the + inferior list. Killed inferiors clearly don't need to be killed + again, so, we're done. */ + if (inf->pid == 0) + return 0; + thread_info *thread = any_thread_of_inferior (inf); if (thread != NULL) { diff --git a/gdb/testsuite/gdb.server/extended-remote-restart.c b/gdb/testsuite/gdb.server/extended-remote-restart.c new file mode 100644 index 00000000000..e195f48e080 --- /dev/null +++ b/gdb/testsuite/gdb.server/extended-remote-restart.c @@ -0,0 +1,60 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2018 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +#include <unistd.h> +#include <stdlib.h> + +static void +breakpt () +{ + asm ("" ::: "memory"); +} + +static void +go_child () +{ + breakpt (); + + while (1) + sleep (1); +} + +static void +go_parent () +{ + breakpt (); + + while (1) + sleep (1); +} + +int +main () +{ + pid_t pid; + + pid = fork (); + if (pid == -1) + abort (); + + if (pid == 0) + go_child (); + else + go_parent (); + + exit (EXIT_SUCCESS); +} diff --git a/gdb/testsuite/gdb.server/extended-remote-restart.exp b/gdb/testsuite/gdb.server/extended-remote-restart.exp new file mode 100644 index 00000000000..39fcb9e2e58 --- /dev/null +++ b/gdb/testsuite/gdb.server/extended-remote-restart.exp @@ -0,0 +1,132 @@ +# Copyright 2018 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +# This test is about restarting execution of a forked application when +# using gdb extended remote target. +# +# There are two issues that the test tries to expose in GDB: +# +# 1. GDB would throw an assertion upon reconnecting to a remote target +# if there was more than one inferior already active in GDB, and +# +# 2. GDB would not prune transient inferiors from the inferior list +# when reconnecting to a remote target. So, for example, an inferior +# created by GDB to track the child of a fork would usually be removed +# from the inferior list once the child exited. However, reconnecting +# to a remote target would result in the child inferior remaining in +# the inferior list. + +# This test is only for extended remote targets. +if {[target_info gdb_protocol] != "extended-remote"} { + continue +} + +# This test also makes use of 'detach-on-fork' which is not supported +# on all platforms. +if { ![istarget "*-*-linux*"] && ![istarget "*-*-openbsd*"] } then { + continue +} + +# And we need to be able to reconnect to gdbserver. +set gdbserver_reconnect_p 1 +if { [info proc gdb_reconnect] == "" } { + return 0 +} + +standard_testfile + +if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} { + return -1 +} + +# Core of the test. DO_KILL_P controls whether we kill one of the +# inferiors before reconnecting. And FOLLOW_CHILD_P controls whether +# we follow the child or the parent at the fork. +proc test_reload { do_kill_p follow_child_p } { + global decimal + global binfile + + clean_restart ${binfile} + + if ![runto_main] then { + fail "can't run to main" + return 0 + } + + # Set detach-on-fork off + gdb_test_no_output "set detach-on-fork off" + + set live_inf_ptn "process $decimal" + set dead_inf_ptn "<null>" + + if ${follow_child_p} { + gdb_test_no_output "set follow-fork child" + set parent_prefix " " + set child_prefix "\\*" + set parent_inf_after_kill_ptn ${live_inf_ptn} + set child_inf_after_kill_ptn ${dead_inf_ptn} + } else { + gdb_test_no_output "set follow-fork parent" + set parent_prefix "\\*" + set child_prefix " " + set parent_inf_after_kill_ptn ${dead_inf_ptn} + set child_inf_after_kill_ptn ${live_inf_ptn} + } + + gdb_breakpoint "breakpt" + gdb_continue_to_breakpoint "breakpt" + + # Check we have the expected inferiors. + gdb_test "info inferiors" \ + [multi_line \ + " Num Description Executable.*" \ + "${parent_prefix} 1 +${live_inf_ptn} \[^\r\n\]+" \ + "${child_prefix} 2 +${live_inf_ptn} \[^\r\n\]+" ] \ + "Check inferiors at breakpoint" + + if { $do_kill_p } { + # (Optional) Kill one of the inferiors. + gdb_test "kill" \ + "" \ + "Kill inferior" \ + "Kill the program being debugged.*y or n. $" \ + "y" + + # Check the first inferior really did die. + gdb_test "info inferiors" \ + [multi_line \ + " Num Description Executable.*" \ + "${parent_prefix} 1 +${parent_inf_after_kill_ptn} \[^\r\n\]+" \ + "${child_prefix} 2 +${child_inf_after_kill_ptn} \[^\r\n\]+" ] \ + "Check inferior was killed" + } + + # Reconnect to the target. + if { [gdb_reconnect] == 0 } { + pass "reconnect after fork" + } else { + fail "reconnect after fork" + } +} + +# Run all combinations of the test. +foreach do_kill_p [list 1 0] { + foreach follow_child_p [list 1 0] { + with_test_prefix \ + "kill: ${do_kill_p}, follow-child ${follow_child_p}" { + test_reload ${do_kill_p} ${follow_child_p} + } + } +} -- 2.14.4 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-09-14 13:36 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-25 13:09 [PATCH 0/2] Issue reconnecting to remote target afte fork Andrew Burgess 2018-06-25 13:09 ` [PATCH 2/2] gdb: Clean up inferior list when reconnecting to new target Andrew Burgess 2018-06-25 13:09 ` [PATCH 1/2] gdb: Fix assert for extended-remote target Andrew Burgess 2018-07-17 17:47 ` [PATCH 0/2] Issue reconnecting to remote target afte fork Andrew Burgess 2018-08-01 21:19 ` PING: [PATCHv2 " Andrew Burgess 2018-08-01 21:19 ` [PATCHv2 2/2] gdb: Clean up inferior list when reconnecting to new target Andrew Burgess 2018-08-08 12:31 ` Andrew Burgess 2018-08-29 10:46 ` PING: " Andrew Burgess 2018-09-13 10:52 ` Andrew Burgess 2018-09-14 13:36 ` Pedro Alves 2018-08-01 21:19 ` [PATCHv2 1/2] gdb: Fix assert for extended-remote target Andrew Burgess 2018-08-02 19:21 ` Tom Tromey 2018-08-08 12:23 ` Andrew Burgess
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).