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

[Pedro Alves <pedro@codesourcery.com>]

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