* RFC: gdb_assert with 'catch signal ...' and fork @ 2013-05-03 22:37 Philippe Waroquiers 2013-05-03 22:43 ` Philippe Waroquiers 0 siblings, 1 reply; 3+ messages in thread From: Philippe Waroquiers @ 2013-05-03 22:37 UTC (permalink / raw) To: gdb-patches gdb 7.6 'catch signal' interacts badly with fork, causing a gdb_assert such as break-catch-sig.c:152: internal-error: signal_catchpoint_remove_location: Assertion `signal_catch_counts[iter] > 0' failed. A problem internal to GDB has been detected, further debugging may prove unreliable. Quit this debugging session? (y or n) This can be reproduced with a (patched) gdb.base/catch-signal.c. The problem is that the signal_catch_counts is decremented by detach_breakpoints. The patch below modifies catch-signal.c, catch-signal.exp and modifies breakpoint.c so as to avoid the assert. 2 questions: 1. The fix is based on the assumption that detach_breakpoints only has to detach breakpoints and watchpoints, but not catchpoints (see breakpoints.h comments, that do not fully match the implementation which e.g. also detach the single step breakpoints). Unclear to me if this is the good fix ? (the fix causes no regtest failure) 2. when running the modified catch-signal test with an unpatched gdb, the test fails but gdb.log does not contain the assert. Rather, the test fails with 'the program exited'. When doing the same commands manually, it shows the gdb assert. No idea why this difference of behaviour. Philippe Index: breakpoint.c =================================================================== RCS file: /cvs/src/src/gdb/breakpoint.c,v retrieving revision 1.759 diff -r1.759 breakpoint.c 3552a3553,3555 > if (bl->owner->type == bp_catchpoint) > continue; > Index: testsuite/gdb.base/catch-signal.c =================================================================== RCS file: /cvs/src/src/gdb/testsuite/gdb.base/catch-signal.c,v retrieving revision 1.3 diff -r1.3 catch-signal.c 17a18 > #include <stdlib.h> 46a48,59 > > signal (SIGCHLD, handle); > switch (fork()) /* fork marker */ > { > case -1: > perror ("fork"); > exit (1); > case 0: > exit (0); > } > (void) wait(NULL); > raise (SIGHUP); /* fifth HUP */ Index: testsuite/gdb.base/catch-signal.exp =================================================================== RCS file: /cvs/src/src/gdb/testsuite/gdb.base/catch-signal.exp,v retrieving revision 1.4 diff -r1.4 catch-signal.exp 85a86,87 > > delete_breakpoints 86a89,95 > # Test interaction with fork. This used to cause a gdb_assert. > gdb_breakpoint ${srcfile}:[gdb_get_line_number "fork marker"] > gdb_continue_to_breakpoint "fork marker" > gdb_test "handle SIGHUP nostop noprint pass" \ > "SIGHUP.*No.*No.*Yes.*" > gdb_test "catch signal SIGHUP" "Catchpoint .*" > gdb_test "continue" "Catchpoint .* SIGHUP.*" "got SIGHUP after fork" ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: RFC: gdb_assert with 'catch signal ...' and fork 2013-05-03 22:37 RFC: gdb_assert with 'catch signal ...' and fork Philippe Waroquiers @ 2013-05-03 22:43 ` Philippe Waroquiers 2013-05-07 17:11 ` Pedro Alves 0 siblings, 1 reply; 3+ messages in thread From: Philippe Waroquiers @ 2013-05-03 22:43 UTC (permalink / raw) To: gdb-patches On Sat, 2013-05-04 at 00:37 +0200, Philippe Waroquiers wrote: > The patch below modifies catch-signal.c, catch-signal.exp > and modifies breakpoint.c so as to avoid the assert. Forgot to give -up option, here is the same with -up Index: gdb/breakpoint.c =================================================================== RCS file: /cvs/src/src/gdb/breakpoint.c,v retrieving revision 1.759 diff -u -p -r1.759 breakpoint.c --- gdb/breakpoint.c 24 Apr 2013 23:09:31 -0000 1.759 +++ gdb/breakpoint.c 3 May 2013 22:39:48 -0000 @@ -3550,6 +3550,9 @@ detach_breakpoints (ptid_t ptid) if (bl->pspace != inf->pspace) continue; + if (bl->owner->type == bp_catchpoint) + continue; + if (bl->inserted) val |= remove_breakpoint_1 (bl, mark_inserted); } Index: gdb/testsuite/gdb.base/catch-signal.c =================================================================== RCS file: /cvs/src/src/gdb/testsuite/gdb.base/catch-signal.c,v retrieving revision 1.3 diff -u -p -r1.3 catch-signal.c --- gdb/testsuite/gdb.base/catch-signal.c 3 May 2013 19:16:56 -0000 1.3 +++ gdb/testsuite/gdb.base/catch-signal.c 3 May 2013 22:39:48 -0000 @@ -15,6 +15,7 @@ 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 <stdlib.h> #include <signal.h> #include <unistd.h> @@ -44,5 +45,17 @@ main () raise (SIGHUP); /* fourth HUP */ raise (SIGINT); /* first INT */ + + signal (SIGCHLD, handle); + switch (fork()) /* fork marker */ + { + case -1: + perror ("fork"); + exit (1); + case 0: + exit (0); + } + (void) wait(NULL); + raise (SIGHUP); /* fifth HUP */ } Index: gdb/testsuite/gdb.base/catch-signal.exp =================================================================== RCS file: /cvs/src/src/gdb/testsuite/gdb.base/catch-signal.exp,v retrieving revision 1.4 diff -u -p -r1.4 catch-signal.exp --- gdb/testsuite/gdb.base/catch-signal.exp 3 May 2013 19:16:56 -0000 1.4 +++ gdb/testsuite/gdb.base/catch-signal.exp 3 May 2013 22:39:48 -0000 @@ -83,7 +83,16 @@ proc test_catch_signal {signame} { y gdb_test "catch signal SIGINT" "Catchpoint .*" gdb_test "continue" "Catchpoint .* SIGINT.*" + + delete_breakpoints + # Test interaction with fork. This used to cause a gdb_assert. + gdb_breakpoint ${srcfile}:[gdb_get_line_number "fork marker"] + gdb_continue_to_breakpoint "fork marker" + gdb_test "handle SIGHUP nostop noprint pass" \ + "SIGHUP.*No.*No.*Yes.*" + gdb_test "catch signal SIGHUP" "Catchpoint .*" + gdb_test "continue" "Catchpoint .* SIGHUP.*" "got SIGHUP after fork" } } ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: RFC: gdb_assert with 'catch signal ...' and fork 2013-05-03 22:43 ` Philippe Waroquiers @ 2013-05-07 17:11 ` Pedro Alves 0 siblings, 0 replies; 3+ messages in thread From: Pedro Alves @ 2013-05-07 17:11 UTC (permalink / raw) To: Philippe Waroquiers; +Cc: gdb-patches Hi Philippe, Thanks for working on this. On 05/03/2013 11:37 PM, Philippe Waroquiers wrote: > gdb 7.6 'catch signal' interacts badly with fork, causing a gdb_assert > such as > break-catch-sig.c:152: internal-error: signal_catchpoint_remove_location: Assertion `signal_catch_counts[iter] > 0' failed. > A problem internal to GDB has been detected, > further debugging may prove unreliable. > Quit this debugging session? (y or n) > > This can be reproduced with a (patched) gdb.base/catch-signal.c. > The problem is that the signal_catch_counts is decremented > by detach_breakpoints. > The patch below modifies catch-signal.c, catch-signal.exp > and modifies breakpoint.c so as to avoid the assert. > > 2 questions: > 1. The fix is based on the assumption that detach_breakpoints > only has to detach breakpoints and watchpoints, > but not catchpoints (see breakpoints.h comments, > that do not fully match the implementation which > e.g. also detach the single step breakpoints). > Unclear to me if this is the good fix ? > (the fix causes no regtest failure) Unfortunately not. A catchpoint can be a breakpoint behind the scenes. See break-catch-throw.c: /* We need to reset 'type' in order for code in breakpoint.c to do the right thing. */ cp->base.type = bp_breakpoint; (I don't know exactly why this is needed though.) It's better to instead look at the location type, and skip bp_loc_other locations. See init_bp_location and: enum bp_loc_type { bp_loc_software_breakpoint, bp_loc_hardware_breakpoint, bp_loc_hardware_watchpoint, bp_loc_other /* Miscellaneous... */ }; I don't think any current catchpoint type needs to be detached per inferior, so I think that'll be sufficient. > 2. when running the modified catch-signal test with an > unpatched gdb, the test fails but gdb.log does not contain > the assert. Rather, the test fails with 'the program exited'. > When doing the same commands manually, it shows the gdb assert. > No idea why this difference of behaviour. I tried it, and the test doesn't trigger a gdb_assert for me. Instead, the program exists, as when running it manually: Running ../../../src/gdb/testsuite/gdb.base/catch-signal.exp ... FAIL: gdb.base/catch-signal.exp: SIGHUP: got SIGHUP after fork (the program exited) FAIL: gdb.base/catch-signal.exp: 1: got SIGHUP after fork (the program exited) FAIL: gdb.base/catch-signal.exp: SIGHUP SIGUSR2: got SIGHUP after fork (the program exited) You must have had the test do a "next" or "step" or some such instead of a "continue" at some point, because that's one way I can trigger the assert: main () at ../../../src/gdb/testsuite/gdb.base/foll-fork.c:23 23 int v = 5; (gdb) n 25 pid = fork (); (gdb) catch signal SIGINT Catchpoint 2 (signal SIGINT) (gdb) n ../../src/gdb/break-catch-sig.c:152: internal-error: signal_catchpoint_remove_location: Assertion `signal_catch_counts[iter] > 0' failed. A problem internal to GDB has been detected, further debugging may prove unreliable. Quit this debugging session? (y or n) > Index: gdb/testsuite/gdb.base/catch-signal.c > =================================================================== > RCS file: /cvs/src/src/gdb/testsuite/gdb.base/catch-signal.c,v > retrieving revision 1.3 > diff -u -p -r1.3 catch-signal.c > --- gdb/testsuite/gdb.base/catch-signal.c 3 May 2013 19:16:56 -0000 1.3 > +++ gdb/testsuite/gdb.base/catch-signal.c 3 May 2013 22:39:48 -0000 > @@ -15,6 +15,7 @@ > 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 <stdlib.h> > #include <signal.h> > #include <unistd.h> > > @@ -44,5 +45,17 @@ main () > raise (SIGHUP); /* fourth HUP */ > > raise (SIGINT); /* first INT */ > + > + signal (SIGCHLD, handle); > + switch (fork()) /* fork marker */ > + { > + case -1: > + perror ("fork"); > + exit (1); > + case 0: > + exit (0); > + } > + (void) wait(NULL); > + raise (SIGHUP); /* fifth HUP */ > } "catch signal" works on all targets, but some targets, like mingw32 don't have 'fork', so this will make the test not compile there, losing test coverage. The easy way out is to split this to a new catch-signal-fork.c|exp test. > > Index: gdb/testsuite/gdb.base/catch-signal.exp > =================================================================== > RCS file: /cvs/src/src/gdb/testsuite/gdb.base/catch-signal.exp,v > retrieving revision 1.4 > diff -u -p -r1.4 catch-signal.exp > --- gdb/testsuite/gdb.base/catch-signal.exp 3 May 2013 19:16:56 -0000 1.4 > +++ gdb/testsuite/gdb.base/catch-signal.exp 3 May 2013 22:39:48 -0000 > @@ -83,7 +83,16 @@ proc test_catch_signal {signame} { > y > gdb_test "catch signal SIGINT" "Catchpoint .*" > gdb_test "continue" "Catchpoint .* SIGINT.*" > + > + delete_breakpoints > > + # Test interaction with fork. This used to cause a gdb_assert. As above, this set of steps does not cause the gdb_assert, so best remove or redo this comment. Also, two spaces after period. > + gdb_breakpoint ${srcfile}:[gdb_get_line_number "fork marker"] > + gdb_continue_to_breakpoint "fork marker" > + gdb_test "handle SIGHUP nostop noprint pass" \ > + "SIGHUP.*No.*No.*Yes.*" > + gdb_test "catch signal SIGHUP" "Catchpoint .*" > + gdb_test "continue" "Catchpoint .* SIGHUP.*" "got SIGHUP after fork" > } > } -- Pedro Alves ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-05-07 17:11 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-05-03 22:37 RFC: gdb_assert with 'catch signal ...' and fork Philippe Waroquiers 2013-05-03 22:43 ` Philippe Waroquiers 2013-05-07 17:11 ` Pedro Alves
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox