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

Pedro Alves
2010-07-27T11:59:32Z

On Sunday 25 July 2010 19:55:12, Jan Kratochvil wrote:

> gdb/
> 2010-07-25 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.

Looks okay. A few comments below, but I'm feeling passive today,
and I'll let this go in unmodified.

> +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.

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.

> +{
> + /* 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 altarnative SIGTRAP-like events recognizer. If
> breakpoint_inserted_here_p there then gdbarch_decr_pc_after_break will be
> applied. */
> @@ -2823,8 +2837,7 @@ count_events_callback (struct lwp_info *lp, void *data)
> 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)++;

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. I actually wonder
why we only select SIGTRAP-like events instead of all reportable events
(as opposed to our own SIGSTOPs).

>
> return 0;
> @@ -2851,8 +2864,7 @@ select_event_lwp_callback (struct lwp_info *lp, void *data)
> 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;

(Same comment here.)

>
> @@ -2912,9 +2924,7 @@ cancel_breakpoints_callback (struct lwp_info *lp, void *data)
> 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 @@ retry:
> 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. */
>

--
Pedro Alves