* RFA: fix gdb_assert caused by 'catch signal ...' and fork
@ 2013-05-09 21:56 Philippe Waroquiers
2013-05-10 16:39 ` Doug Evans
2013-05-10 17:20 ` Pedro Alves
0 siblings, 2 replies; 9+ messages in thread
From: Philippe Waroquiers @ 2013-05-09 21:56 UTC (permalink / raw)
To: gdb-patches
The attached patch fixes a gdb_assert caused by the combination of catch
signal and fork:
break-catch-sig.c:152: internal-error: signal_catchpoint_remove_location: Assertion `signal_catch_counts[iter] > 0' failed.
The problem is that the signal_catch_counts is decremented by detach_breakpoints.
The fix consists in not detaching breakpoint locations of type bp_loc_other.
The patch introduces a new test.
Ok to commit ?
Index: gdb/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.15539
diff -u -p -r1.15539 ChangeLog
--- gdb/ChangeLog 9 May 2013 18:03:27 -0000 1.15539
+++ gdb/ChangeLog 9 May 2013 21:46:32 -0000
@@ -1,3 +1,8 @@
+2013-05-09 Philippe Waroquiers <philippe.waroquiers@skynet.be>
+
+ * breakpoints.c (detach_breakpoints): Do not
+ detach breakpoints locations with loc_type bp_loc_other.
+
2013-05-09 Doug Evans <dje@google.com>
* symfile.c (syms_from_objfile_1): Delete args offsets, num_offsets.
Index: gdb/breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.761
diff -u -p -r1.761 breakpoint.c
--- gdb/breakpoint.c 7 May 2013 17:04:28 -0000 1.761
+++ gdb/breakpoint.c 9 May 2013 21:46:33 -0000
@@ -3537,6 +3537,9 @@ detach_breakpoints (ptid_t ptid)
if (bl->pspace != inf->pspace)
continue;
+ if (bl->loc_type == bp_loc_other)
+ continue;
+
if (bl->inserted)
val |= remove_breakpoint_1 (bl, mark_inserted);
}
Index: gdb/testsuite/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/ChangeLog,v
retrieving revision 1.3653
diff -u -p -r1.3653 ChangeLog
--- gdb/testsuite/ChangeLog 8 May 2013 18:56:02 -0000 1.3653
+++ gdb/testsuite/ChangeLog 9 May 2013 21:46:36 -0000
@@ -1,3 +1,8 @@
+2013-05-09 Philippe Waroquiers <philippe.waroquiers@skynet.be>
+
+ * gdb.base/catch-signal-fork.exp: New file.
+ * gdb.base/catch-signal-fork.c: New file.
+
2013-05-08 Tom Tromey <tromey@redhat.com>
* gdb.base/solib-search.exp: Set test name for "set
Index: gdb/testsuite/gdb.base/catch-signal-fork.c
===================================================================
RCS file: gdb/testsuite/gdb.base/catch-signal-fork.c
diff -N gdb/testsuite/gdb.base/catch-signal-fork.c
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ gdb/testsuite/gdb.base/catch-signal-fork.c 9 May 2013 21:46:36 -0000
@@ -0,0 +1,58 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2013-2013 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 <stdlib.h>
+#include <signal.h>
+#include <unistd.h>
+
+void
+do_nothing (void)
+{
+}
+
+void
+handle (int sig)
+{
+ do_nothing (); /* handle marker */
+}
+
+int
+main ()
+{
+ int i;
+ signal (SIGHUP, handle);
+
+ raise (SIGHUP); /* first HUP */
+
+ signal (SIGCHLD, handle);
+ for (i = 0; i < 3; i++) /* fork loop */
+ {
+ switch (fork())
+ {
+ case -1:
+ perror ("fork");
+ exit (1);
+ case 0:
+ exit (0);
+ }
+ (void) wait(NULL);
+ }
+
+ raise (SIGHUP); /* second HUP */
+
+ raise (SIGHUP); /* third HUP */
+}
+
Index: gdb/testsuite/gdb.base/catch-signal-fork.exp
===================================================================
RCS file: gdb/testsuite/gdb.base/catch-signal-fork.exp
diff -N gdb/testsuite/gdb.base/catch-signal-fork.exp
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ gdb/testsuite/gdb.base/catch-signal-fork.exp 9 May 2013 21:46:36 -0000
@@ -0,0 +1,43 @@
+# Copyright 2013-2013 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/>.
+
+if [target_info exists gdb,nosignals] {
+ verbose "Skipping catch-signal-fork.exp because of nosignals."
+ continue
+}
+
+standard_testfile
+
+if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
+ return -1
+}
+
+if {![runto_main]} {
+ return -1
+}
+
+# Test "catch signal SIGHUP"
+gdb_test "catch signal SIGHUP" "Catchpoint .*"
+gdb_breakpoint ${srcfile}:[gdb_get_line_number "first HUP"]
+gdb_breakpoint ${srcfile}:[gdb_get_line_number "fork loop"]
+
+gdb_continue_to_breakpoint "first HUP"
+gdb_test "continue" "Catchpoint .*"
+
+# Test interaction with fork.
+# This used to cause a gdb_assert in the code detaching the
+# breakpoints for the child.
+gdb_continue_to_breakpoint "fork loop"
+gdb_test "continue" "Catchpoint .* SIGHUP.*" "got SIGHUP after fork"
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: RFA: fix gdb_assert caused by 'catch signal ...' and fork
2013-05-09 21:56 RFA: fix gdb_assert caused by 'catch signal ...' and fork Philippe Waroquiers
@ 2013-05-10 16:39 ` Doug Evans
2013-05-10 17:18 ` Pedro Alves
2013-05-10 17:20 ` Pedro Alves
1 sibling, 1 reply; 9+ messages in thread
From: Doug Evans @ 2013-05-10 16:39 UTC (permalink / raw)
To: Philippe Waroquiers; +Cc: gdb-patches
On Thu, May 9, 2013 at 2:56 PM, Philippe Waroquiers
<philippe.waroquiers@skynet.be> wrote:
> The attached patch fixes a gdb_assert caused by the combination of catch
> signal and fork:
> break-catch-sig.c:152: internal-error: signal_catchpoint_remove_location: Assertion `signal_catch_counts[iter] > 0' failed.
>
> The problem is that the signal_catch_counts is decremented by detach_breakpoints.
> The fix consists in not detaching breakpoint locations of type bp_loc_other.
> The patch introduces a new test.
>
> Ok to commit ?
>
> Index: gdb/ChangeLog
> ===================================================================
> RCS file: /cvs/src/src/gdb/ChangeLog,v
> retrieving revision 1.15539
> diff -u -p -r1.15539 ChangeLog
> --- gdb/ChangeLog 9 May 2013 18:03:27 -0000 1.15539
> +++ gdb/ChangeLog 9 May 2013 21:46:32 -0000
> @@ -1,3 +1,8 @@
> +2013-05-09 Philippe Waroquiers <philippe.waroquiers@skynet.be>
> +
> + * breakpoints.c (detach_breakpoints): Do not
> + detach breakpoints locations with loc_type bp_loc_other.
> +
> 2013-05-09 Doug Evans <dje@google.com>
>
> * symfile.c (syms_from_objfile_1): Delete args offsets, num_offsets.
> Index: gdb/breakpoint.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/breakpoint.c,v
> retrieving revision 1.761
> diff -u -p -r1.761 breakpoint.c
> --- gdb/breakpoint.c 7 May 2013 17:04:28 -0000 1.761
> +++ gdb/breakpoint.c 9 May 2013 21:46:33 -0000
> @@ -3537,6 +3537,9 @@ detach_breakpoints (ptid_t ptid)
> if (bl->pspace != inf->pspace)
> continue;
>
> + if (bl->loc_type == bp_loc_other)
> + continue;
> +
> if (bl->inserted)
> val |= remove_breakpoint_1 (bl, mark_inserted);
> }
I think a comment is required here explaining *why* we continue for
bp_loc_other.
[Assuming the patch is correct ...]
However, there's nothing in "bp_loc_other" that says we should always
continue there.
Other breakpoint kinds are marked bp_loc_other too.
Plus avoiding calling remove_breakpoint_1 feels like working around the problem.
This doesn't feel like the right fix.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: RFA: fix gdb_assert caused by 'catch signal ...' and fork
2013-05-10 16:39 ` Doug Evans
@ 2013-05-10 17:18 ` Pedro Alves
2013-05-10 22:14 ` Philippe Waroquiers
0 siblings, 1 reply; 9+ messages in thread
From: Pedro Alves @ 2013-05-10 17:18 UTC (permalink / raw)
To: Doug Evans; +Cc: Philippe Waroquiers, gdb-patches
On 05/10/2013 05:39 PM, Doug Evans wrote:
> On Thu, May 9, 2013 at 2:56 PM, Philippe Waroquiers
> <philippe.waroquiers@skynet.be> wrote:
>> The attached patch fixes a gdb_assert caused by the combination of catch
>> signal and fork:
>> break-catch-sig.c:152: internal-error: signal_catchpoint_remove_location: Assertion `signal_catch_counts[iter] > 0' failed.
>>
>> The problem is that the signal_catch_counts is decremented by detach_breakpoints.
>> The fix consists in not detaching breakpoint locations of type bp_loc_other.
>> The patch introduces a new test.
>>
>> Ok to commit ?
>>
>> Index: gdb/ChangeLog
>> ===================================================================
>> RCS file: /cvs/src/src/gdb/ChangeLog,v
>> retrieving revision 1.15539
>> diff -u -p -r1.15539 ChangeLog
>> --- gdb/ChangeLog 9 May 2013 18:03:27 -0000 1.15539
>> +++ gdb/ChangeLog 9 May 2013 21:46:32 -0000
>> @@ -1,3 +1,8 @@
>> +2013-05-09 Philippe Waroquiers <philippe.waroquiers@skynet.be>
>> +
>> + * breakpoints.c (detach_breakpoints): Do not
>> + detach breakpoints locations with loc_type bp_loc_other.
>> +
>> 2013-05-09 Doug Evans <dje@google.com>
>>
>> * symfile.c (syms_from_objfile_1): Delete args offsets, num_offsets.
>> Index: gdb/breakpoint.c
>> ===================================================================
>> RCS file: /cvs/src/src/gdb/breakpoint.c,v
>> retrieving revision 1.761
>> diff -u -p -r1.761 breakpoint.c
>> --- gdb/breakpoint.c 7 May 2013 17:04:28 -0000 1.761
>> +++ gdb/breakpoint.c 9 May 2013 21:46:33 -0000
>> @@ -3537,6 +3537,9 @@ detach_breakpoints (ptid_t ptid)
>> if (bl->pspace != inf->pspace)
>> continue;
>>
>> + if (bl->loc_type == bp_loc_other)
>> + continue;
>> +
>> if (bl->inserted)
>> val |= remove_breakpoint_1 (bl, mark_inserted);
>> }
>
> I think a comment is required here explaining *why* we continue for
> bp_loc_other.
> [Assuming the patch is correct ...]
>
> However, there's nothing in "bp_loc_other" that says we should always
> continue there.
> Other breakpoint kinds are marked bp_loc_other too.
The other breakpoint kinds (software watchpoints, catchpoints,
tracepoints) don't require detaching. The state of bp_loc_other
breakpoints, at least at present, is always on the GDB side.
Detaching is required for those breakpoints that is assumed
get auto-cloned by the target/kernel to forked children.
> Plus avoiding calling remove_breakpoint_1 feels like working around the problem.
> This doesn't feel like the right fix.
GDB doesn't have an inferior or any other state corresponding
to the process whose breakpoints are being detached.
An alternative I imagine would be something like adding
"detach breakpoint" target methods (and bl->owner->ops->detach_location,
etc.) and call that instead of remove_breakpoint_1, though it
seems like we'd get the same result (with the present state). But
I won't object to trying that.
--
Pedro Alves
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: RFA: fix gdb_assert caused by 'catch signal ...' and fork
2013-05-10 17:18 ` Pedro Alves
@ 2013-05-10 22:14 ` Philippe Waroquiers
2013-05-11 9:10 ` Pedro Alves
0 siblings, 1 reply; 9+ messages in thread
From: Philippe Waroquiers @ 2013-05-10 22:14 UTC (permalink / raw)
To: Pedro Alves; +Cc: Doug Evans, gdb-patches
On Fri, 2013-05-10 at 18:18 +0100, Pedro Alves wrote:
> On 05/10/2013 05:39 PM, Doug Evans wrote:
> > On Thu, May 9, 2013 at 2:56 PM, Philippe Waroquiers
> > <philippe.waroquiers@skynet.be> wrote:
> >> Index: gdb/breakpoint.c
> >> ===================================================================
> >> RCS file: /cvs/src/src/gdb/breakpoint.c,v
> >> retrieving revision 1.761
> >> diff -u -p -r1.761 breakpoint.c
> >> --- gdb/breakpoint.c 7 May 2013 17:04:28 -0000 1.761
> >> +++ gdb/breakpoint.c 9 May 2013 21:46:33 -0000
> >> @@ -3537,6 +3537,9 @@ detach_breakpoints (ptid_t ptid)
> >> if (bl->pspace != inf->pspace)
> >> continue;
> >>
> >> + if (bl->loc_type == bp_loc_other)
> >> + continue;
> >> +
> >> if (bl->inserted)
> >> val |= remove_breakpoint_1 (bl, mark_inserted);
> >> }
> >
> > I think a comment is required here explaining *why* we continue for
> > bp_loc_other.
> > [Assuming the patch is correct ...]
Yes, adding a comment is a good idea.
> >
> > However, there's nothing in "bp_loc_other" that says we should always
> > continue there.
> > Other breakpoint kinds are marked bp_loc_other too.
>
> The other breakpoint kinds (software watchpoints, catchpoints,
> tracepoints) don't require detaching. The state of bp_loc_other
> breakpoints, at least at present, is always on the GDB side.
> Detaching is required for those breakpoints that is assumed
> get auto-cloned by the target/kernel to forked children.
>
> > Plus avoiding calling remove_breakpoint_1 feels like working around the problem.
> > This doesn't feel like the right fix.
>
> GDB doesn't have an inferior or any other state corresponding
> to the process whose breakpoints are being detached.
>
> An alternative I imagine would be something like adding
> "detach breakpoint" target methods (and bl->owner->ops->detach_location,
> etc.) and call that instead of remove_breakpoint_1, though it
> seems like we'd get the same result (with the present state). But
> I won't object to trying that.
I do not master much of breakpoint.h/.c but it looks to
me that this implies to add quite some code which will
at the end either do nothing (for bp_loc_other) or
call remove_breakpoint_1 (for others).
What would be the advantages of the detach_breakpoint
and detach_location target methods ?
As long as there is no need (yet) for a different "detach"
behaviour depending on specialised bp_location, it looks
to me that the single "if" is simpler and corresponds to
the description of detach_breakpoints in breakpoint.h.
(maybe we just have to add 'software_breakpoint' and
'single_step_breakpoint' in the description in breakpoint.h ?)
Or do I miss something about the interest/need for detach_* methods ?
Philippe
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: RFA: fix gdb_assert caused by 'catch signal ...' and fork
2013-05-10 22:14 ` Philippe Waroquiers
@ 2013-05-11 9:10 ` Pedro Alves
0 siblings, 0 replies; 9+ messages in thread
From: Pedro Alves @ 2013-05-11 9:10 UTC (permalink / raw)
To: Philippe Waroquiers; +Cc: Doug Evans, gdb-patches
On 05/10/2013 11:14 PM, Philippe Waroquiers wrote:
> On Fri, 2013-05-10 at 18:18 +0100, Pedro Alves wrote:
>> On 05/10/2013 05:39 PM, Doug Evans wrote:
>>> However, there's nothing in "bp_loc_other" that says we should always
>>> continue there.
>>> Other breakpoint kinds are marked bp_loc_other too.
>>
>> The other breakpoint kinds (software watchpoints, catchpoints,
>> tracepoints) don't require detaching. The state of bp_loc_other
>> breakpoints, at least at present, is always on the GDB side.
>> Detaching is required for those breakpoints that is assumed
>> get auto-cloned by the target/kernel to forked children.
>>
>>> Plus avoiding calling remove_breakpoint_1 feels like working around the problem.
>>> This doesn't feel like the right fix.
>>
>> GDB doesn't have an inferior or any other state corresponding
>> to the process whose breakpoints are being detached.
>>
>> An alternative I imagine would be something like adding
>> "detach breakpoint" target methods (and bl->owner->ops->detach_location,
>> etc.) and call that instead of remove_breakpoint_1, though it
>> seems like we'd get the same result (with the present state). But
>> I won't object to trying that.
> I do not master much of breakpoint.h/.c but it looks to
> me that this implies to add quite some code which will
> at the end either do nothing (for bp_loc_other) or
> call remove_breakpoint_1 (for others).
Not exactly remove_breakpoint_1, but something like it, that
ends up calling (a new) target_detach_breakpoint.
> What would be the advantages of the detach_breakpoint
> and detach_location target methods ?
It'd be one way forward if we actually needed to
detach some kind of catchpoint, while not others.
Note the idea wasn't that much cooked, I'd leave that to
whoever attempts to implement it. It could end up being
the target method wouldn't be necessary. The gist
would be to use breakpoint_ops dispatching somehow instead
of explicit type checks.
Or maybe some other deeper design change would be better.
Changing how core gdb reacts to forks throughout, making
it have an inferior for the new fork sooner comes to mind.
I don't have a substantially formed idea of what that would
be the ideal design, and I can't afford spending hours/days
investigating this myself at the moment, unfortunately.
> As long as there is no need (yet) for a different "detach"
> behaviour depending on specialised bp_location, it looks
> to me that the single "if" is simpler and corresponds to
> the description of detach_breakpoints in breakpoint.h.
Right, that was my reasoning for suggesting going with
the location type originally.
--
Pedro Alves
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: RFA: fix gdb_assert caused by 'catch signal ...' and fork
2013-05-09 21:56 RFA: fix gdb_assert caused by 'catch signal ...' and fork Philippe Waroquiers
2013-05-10 16:39 ` Doug Evans
@ 2013-05-10 17:20 ` Pedro Alves
2013-05-16 20:30 ` Philippe Waroquiers
1 sibling, 1 reply; 9+ messages in thread
From: Pedro Alves @ 2013-05-10 17:20 UTC (permalink / raw)
To: Philippe Waroquiers; +Cc: gdb-patches
Hi Philippe,
(Please mark newer posts/revisions of patches in new threads
with v2, v3, etc. in the subject line.)
> Index: gdb/testsuite/gdb.base/catch-signal-fork.c
> ===================================================================
> RCS file: gdb/testsuite/gdb.base/catch-signal-fork.c
> diff -N gdb/testsuite/gdb.base/catch-signal-fork.c
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ gdb/testsuite/gdb.base/catch-signal-fork.c 9 May 2013 21:46:36 -0000
> @@ -0,0 +1,58 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> + Copyright 2013-2013 Free Software Foundation, Inc.
Write only "2013" when we only have a single year.
But, if the file is based on another existing file, we should
preserve the existing file's copyright years in the new file.
> +int
> +main ()
> +{
> + int i;
> + signal (SIGHUP, handle);
> +
> + raise (SIGHUP); /* first HUP */
> +
> + signal (SIGCHLD, handle);
> + for (i = 0; i < 3; i++) /* fork loop */
> + {
> + switch (fork())
> + {
> + case -1:
> + perror ("fork");
> + exit (1);
> + case 0:
> + exit (0);
> + }
> + (void) wait(NULL);
Missing space before parens. (do you really need the cast?)
> Index: gdb/testsuite/gdb.base/catch-signal-fork.exp
> ===================================================================
> RCS file: gdb/testsuite/gdb.base/catch-signal-fork.exp
> diff -N gdb/testsuite/gdb.base/catch-signal-fork.exp
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ gdb/testsuite/gdb.base/catch-signal-fork.exp 9 May 2013 21:46:36 -0000
> @@ -0,0 +1,43 @@
> +# Copyright 2013-2013 Free Software Foundation, Inc.
2013-2013 again.
--
Pedro Alves
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: RFA: fix gdb_assert caused by 'catch signal ...' and fork
2013-05-10 17:20 ` Pedro Alves
@ 2013-05-16 20:30 ` Philippe Waroquiers
2013-05-17 18:18 ` Pedro Alves
0 siblings, 1 reply; 9+ messages in thread
From: Philippe Waroquiers @ 2013-05-16 20:30 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On Fri, 2013-05-10 at 18:20 +0100, Pedro Alves wrote:
> Hi Philippe,
>
> (Please mark newer posts/revisions of patches in new threads
> with v2, v3, etc. in the subject line.)
Hello,
Sorry for the slow update after the fast review.
Here is an updated v3 patch, fixing the review comments.
> Write only "2013" when we only have a single year.
> But, if the file is based on another existing file, we should
> preserve the existing file's copyright years in the new file.
These files were based on a previous files, so restored 2012-2013.
> > + (void) wait(NULL);
> Missing space before parens. (do you really need the cast?)
space added.
I added the cast to explicitely show that the return value is ignored.
I might instead not ignore it if you think this is needed/better.
I also added a comment in breakpoint.c, to explain why
the bp_loc_other locations are not removed.
Ok to apply ?
Philippe
Index: gdb/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.15563
diff -u -p -r1.15563 ChangeLog
--- gdb/ChangeLog 16 May 2013 07:39:42 -0000 1.15563
+++ gdb/ChangeLog 16 May 2013 20:19:10 -0000
@@ -1,3 +1,8 @@
+2013-05-16 Philippe Waroquiers <philippe.waroquiers@skynet.be>
+
+ * breakpoints.c (detach_breakpoints): Do not
+ detach breakpoints locations with loc_type bp_loc_other.
+
2013-05-16 Joel Brobecker <brobecker@adacore.com>
* dwarf2read.c (set_cu_language): Add DW_LANG_UPC handling.
Index: gdb/breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.761
diff -u -p -r1.761 breakpoint.c
--- gdb/breakpoint.c 7 May 2013 17:04:28 -0000 1.761
+++ gdb/breakpoint.c 16 May 2013 20:19:11 -0000
@@ -3537,6 +3537,15 @@ detach_breakpoints (ptid_t ptid)
if (bl->pspace != inf->pspace)
continue;
+ /* This function must physically remove breakpoints locations
+ from the specified ptid, without modifying the breakpoint
+ package's state. Locations of type bp_loc_other are only
+ maintained at GDB side. So, there is no need to remove
+ these bp_loc_other locations. Moreover, removing these
+ would modify the breakpoint package's state. */
+ if (bl->loc_type == bp_loc_other)
+ continue;
+
if (bl->inserted)
val |= remove_breakpoint_1 (bl, mark_inserted);
}
Index: gdb/testsuite/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/ChangeLog,v
retrieving revision 1.3661
diff -u -p -r1.3661 ChangeLog
--- gdb/testsuite/ChangeLog 16 May 2013 10:13:33 -0000 1.3661
+++ gdb/testsuite/ChangeLog 16 May 2013 20:19:14 -0000
@@ -1,3 +1,8 @@
+2013-05-16 Philippe Waroquiers <philippe.waroquiers@skynet.be>
+
+ * gdb.base/catch-signal-fork.exp: New file.
+ * gdb.base/catch-signal-fork.c: New file.
+
2013-05-16 Pedro Alves <palves@redhat.com>
* gdb.ada/complete.exp (test_gdb_no_completion): Fix typo in
Index: gdb/testsuite/gdb.base/catch-signal-fork.c
===================================================================
RCS file: gdb/testsuite/gdb.base/catch-signal-fork.c
diff -N gdb/testsuite/gdb.base/catch-signal-fork.c
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ gdb/testsuite/gdb.base/catch-signal-fork.c 16 May 2013 20:19:14 -0000
@@ -0,0 +1,58 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2012-2013 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 <stdlib.h>
+#include <signal.h>
+#include <unistd.h>
+
+void
+do_nothing (void)
+{
+}
+
+void
+handle (int sig)
+{
+ do_nothing (); /* handle marker */
+}
+
+int
+main ()
+{
+ int i;
+ signal (SIGHUP, handle);
+
+ raise (SIGHUP); /* first HUP */
+
+ signal (SIGCHLD, handle);
+ for (i = 0; i < 3; i++) /* fork loop */
+ {
+ switch (fork())
+ {
+ case -1:
+ perror ("fork");
+ exit (1);
+ case 0:
+ exit (0);
+ }
+ (void) wait (NULL);
+ }
+
+ raise (SIGHUP); /* second HUP */
+
+ raise (SIGHUP); /* third HUP */
+}
+
Index: gdb/testsuite/gdb.base/catch-signal-fork.exp
===================================================================
RCS file: gdb/testsuite/gdb.base/catch-signal-fork.exp
diff -N gdb/testsuite/gdb.base/catch-signal-fork.exp
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ gdb/testsuite/gdb.base/catch-signal-fork.exp 16 May 2013 20:19:14 -0000
@@ -0,0 +1,43 @@
+# Copyright 2012-2013 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/>.
+
+if [target_info exists gdb,nosignals] {
+ verbose "Skipping catch-signal-fork.exp because of nosignals."
+ continue
+}
+
+standard_testfile
+
+if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
+ return -1
+}
+
+if {![runto_main]} {
+ return -1
+}
+
+# Test "catch signal SIGHUP"
+gdb_test "catch signal SIGHUP" "Catchpoint .*"
+gdb_breakpoint ${srcfile}:[gdb_get_line_number "first HUP"]
+gdb_breakpoint ${srcfile}:[gdb_get_line_number "fork loop"]
+
+gdb_continue_to_breakpoint "first HUP"
+gdb_test "continue" "Catchpoint .*"
+
+# Test interaction with fork.
+# This used to cause a gdb_assert in the code detaching the
+# breakpoints for the child.
+gdb_continue_to_breakpoint "fork loop"
+gdb_test "continue" "Catchpoint .* SIGHUP.*" "got SIGHUP after fork"
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: RFA: fix gdb_assert caused by 'catch signal ...' and fork
2013-05-16 20:30 ` Philippe Waroquiers
@ 2013-05-17 18:18 ` Pedro Alves
2013-05-21 18:49 ` Philippe Waroquiers
0 siblings, 1 reply; 9+ messages in thread
From: Pedro Alves @ 2013-05-17 18:18 UTC (permalink / raw)
To: Philippe Waroquiers; +Cc: gdb-patches
On 05/16/2013 09:30 PM, Philippe Waroquiers wrote:
>>> + (void) wait(NULL);
>> Missing space before parens. (do you really need the cast?)
> space added.
> I added the cast to explicitely show that the return value is ignored.
> I might instead not ignore it if you think this is needed/better.
IMO, that's more noise than signal. We ignore return values in
many many cases, and don't do that (including other wait calls in
the testsuite).
> I also added a comment in breakpoint.c, to explain why
> the bp_loc_other locations are not removed.
>
> Ok to apply ?
Looks good to me. Give it a couple days more, in case Doug or
others wants to comment, and check it in then.
Thanks,
--
Pedro Alves
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: RFA: fix gdb_assert caused by 'catch signal ...' and fork
2013-05-17 18:18 ` Pedro Alves
@ 2013-05-21 18:49 ` Philippe Waroquiers
0 siblings, 0 replies; 9+ messages in thread
From: Philippe Waroquiers @ 2013-05-21 18:49 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On Fri, 2013-05-17 at 19:17 +0100, Pedro Alves wrote:
> >>> + (void) wait(NULL);
> IMO, that's more noise than signal. We ignore return values in
> many many cases, and don't do that (including other wait calls in
> the testsuite).
Removed the '(void)'
>
> > I also added a comment in breakpoint.c, to explain why
> > the bp_loc_other locations are not removed.
> >
> > Ok to apply ?
>
> Looks good to me. Give it a couple days more, in case Doug or
> others wants to comment, and check it in then.
and committed.
Thanks
Philippe
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-05-21 18:49 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-05-09 21:56 RFA: fix gdb_assert caused by 'catch signal ...' and fork Philippe Waroquiers
2013-05-10 16:39 ` Doug Evans
2013-05-10 17:18 ` Pedro Alves
2013-05-10 22:14 ` Philippe Waroquiers
2013-05-11 9:10 ` Pedro Alves
2013-05-10 17:20 ` Pedro Alves
2013-05-16 20:30 ` Philippe Waroquiers
2013-05-17 18:18 ` Pedro Alves
2013-05-21 18:49 ` Philippe Waroquiers
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox