Re: [patch] Fix linux-ia64 on SIGILL for deleted breakpoint [cleanup]

Jan Kratochvil
2010-07-27T21:22:54Z

On Tue, 27 Jul 2010 13:59:18 +0200, Pedro Alves wrote:
> > +static int
> > +linux_nat_lp_status_is_event (struct lwp_info *lp)
>
> Static helper functions that aren't a part of the target_ops
> interface (as opposed to linux_nat_resume, say), or external,
> don't usually take the "linux_nat_" prefix, so you could shorten
> the function name.

There is a problem that linux_nat_set_status_is_event is not static and
therefore it is correctly ^linux_nat_ prefixed. I thought all the other
related functions should have matching name, which assigns them the incorrect
^linux_nat_ prefix.

> I'm not particularly fond of the function names. The reason is
> that:
>
> - linux_nat_status_is_event
>
> Returns true for breakpoint events, and other SIGTRAP events.
> Returns false for other kinds of events (random signals), though
> we could call these events too.
>
> - linux_nat_lp_status_is_event
>
> Returns true for breakpoint events, and other SIGTRAP events
> Returns false for other kinds of events (random signals), and
> fork/exec/clone/exit events, though we could call these events too.
>
> Note the subtle differences.

Yes... the problem is the variables already are not systematic. There should
be that lp->status_p flag discussed in the comments, TARGET_SIGNAL_TRAP is
incorrectly used as an abstraction of inferior event (even for
SIGILL/SIGSEGV/etc.), lp->signalled should be called lp->sigstop_sent,
lp->resumed should be called lp->want_to_resume, target_signal should be
called gdb_signal, host signal should be called target signal,
lp->waitstatus.kind TARGET_WAITKIND_* I cannot describe right offhand now.

I have to admit I mostly gave up on building systematic functions and naming
on top of it.

> This appears to be changing behaviour (the new waitstatus check within
> the new function), but I doubt it makes a difference. I'm pointing
> it out, because you're presenting the patch as cleanup.

Sorry for the naming, I did not indend to consider this patch as a no
functionality change patch. (Although it should not be a user visible
change.)

Checked-in.

Thanks,
Jan

http://sourceware.org/ml/gdb-cvs/2010-07/msg00171.html

--- src/gdb/ChangeLog 2010/07/27 20:51:37 1.12026
+++ src/gdb/ChangeLog 2010/07/27 21:22:07 1.12027
@@ -1,5 +1,11 @@
2010-07-27 Jan Kratochvil <jan.kratochvil@redhat.com>

+ * linux-nat.c (linux_nat_lp_status_is_event): New function.
+ (count_events_callback, select_event_lwp_callback)
+ (cancel_breakpoints_callback, linux_nat_wait_1): Use it.
+
+2010-07-27 Jan Kratochvil <jan.kratochvil@redhat.com>
+
* ia64-linux-nat.c (ia64_linux_status_is_event): New function.
(_initialize_ia64_linux_nat): Install it.
* linux-nat.c (sigtrap_is_event, linux_nat_status_is_event)
--- src/gdb/linux-nat.c 2010/07/27 20:51:37 1.179
+++ src/gdb/linux-nat.c 2010/07/27 21:22:09 1.180
@@ -2617,6 +2617,20 @@

static int (*linux_nat_status_is_event) (int status) = sigtrap_is_event;

+/* Check for SIGTRAP-like events in LP. */
+
+static int
+linux_nat_lp_status_is_event (struct lwp_info *lp)
+{
+ /* We check for lp->waitstatus in addition to lp->status, because we can
+ have pending process exits recorded in lp->status
+ and W_EXITCODE(0,0) == 0. We should probably have an additional
+ lp->status_p flag. */
+
+ return (lp->waitstatus.kind == TARGET_WAITKIND_IGNORE
+ && linux_nat_status_is_event (lp->status));
+}
+
/* Set alternative SIGTRAP-like events recognizer. If
breakpoint_inserted_here_p there then gdbarch_decr_pc_after_break will be
applied. */
@@ -2823,8 +2837,7 @@
gdb_assert (count != NULL);

/* Count only resumed LWPs that have a SIGTRAP event pending. */
- if (lp->status != 0 && lp->resumed
- && linux_nat_status_is_event (lp->status))
+ if (lp->resumed && linux_nat_lp_status_is_event (lp))
(*count)++;

return 0;
@@ -2851,8 +2864,7 @@
gdb_assert (selector != NULL);

/* Select only resumed LWPs that have a SIGTRAP event pending. */
- if (lp->status != 0 && lp->resumed
- && linux_nat_status_is_event (lp->status))
+ if (lp->resumed && linux_nat_lp_status_is_event (lp))
if ((*selector)-- == 0)
return 1;

@@ -2912,9 +2924,7 @@
delete or disable the breakpoint, but the LWP will have already
tripped on it. */

- if (lp->waitstatus.kind == TARGET_WAITKIND_IGNORE
- && lp->status != 0
- && linux_nat_status_is_event (lp->status)
+ if (linux_nat_lp_status_is_event (lp)
&& cancel_breakpoint (lp))
/* Throw away the SIGTRAP. */
lp->status = 0;
@@ -3433,8 +3443,7 @@
always cancels breakpoint hits in all
threads. */
if (non_stop
- && lp->waitstatus.kind == TARGET_WAITKIND_IGNORE
- && linux_nat_status_is_event (lp->status)
+ && linux_nat_lp_status_is_event (lp)
&& cancel_breakpoint (lp))
{
/* Throw away the SIGTRAP. */