Re: [rfc target-side break conditions 5/5 v2] GDBServer-side changes

[Pedro Alves <palves@redhat.com>]

Some comments below, but overall looks good.

On 02/08/2012 11:14 PM, Luis Gustavo wrote:
> 
> 2012-02-08  Luis Machado  <lgustavo@codesourcery>
> 
> 	gdbserver/
> 	* server.c (handle_query): Advertise support for target-side
> 	breakpoint condition evaluation.
> 	(process_point_options): New function.
> 	(process_serial_event): When inserting a breakpoint, check for
> 	a target-side condition that should be evaluated.
> 
> 	* mem-break.c: Include regcache.h and ax.h.
> 	(point_cond_list_t): New data structure.
> 	(breakpoint) <cond_list>: New field.
> 	(find_gdb_breakpoint_at): Make non-static.
> 	(delete_gdb_breakpoint_at): Clear any target-side
> 	conditions.
> 	(clear_gdb_breakpoint_conditions): New function.
> 	(add_condition_to_breakpoint): Likewise.
> 	(add_breakpoint_condition): Likewise.
> 	(gdb_condition_true_at_breakpoint): Likewise.
> 	(gdb_breakpoint_here): Return result directly instead
> 	of going through a local variable.
> 
> 	* mem-break.h (find_gdb_breakpoint_at): New prototype.
> 	(clear_gdb_breakpoint_conditions): Likewise.
> 	(add_breakpoint_condition): Likewise.
> 	(gdb_condition_true_at_breakpoint): Likewise.
> 
> 	* linux-low.c (linux_wait_1): Evaluate target-side breakpoint condition.
> 	(need_step_over_p): Take target-side breakpoint condition into
> 	consideration.
> 
> Index: gdb/gdb/gdbserver/server.c
> ===================================================================
> --- gdb.orig/gdb/gdbserver/server.c	2012-02-08 15:57:07.399075002 -0200
> +++ gdb/gdb/gdbserver/server.c	2012-02-08 15:57:33.139074999 -0200
> @@ -1621,6 +1621,9 @@ handle_query (char *own_buf, int packet_
>  	  strcat (own_buf, ";tracenz+");
>  	}
>  
> +      /* Support target-side breakpoint conditions.  */
> +      strcat (own_buf, ";ConditionalBreakpoints+");

I still think it's a shame this doesn't mean all Z packets
understand target side conditionals...


> +static void
> +process_point_options (CORE_ADDR point_addr, char **packet)
> +{
> +  char *dataptr = *packet;
> +
> +  while (dataptr[0] == ';')
> +    {
> +      dataptr++;
> +
> +      if (!strncmp (dataptr, "conditions=", strlen ("conditions=")))

strncmp's return is not a boolean.  Please write as

   if (strncmp (dataptr, "conditions=", strlen ("conditions=")) == 0)

> +	{
> +	  /* We have conditions to parse.  */
> +	  dataptr += strlen ("conditions=");
> +
> +	  /* Insert conditions.  */
> +	  do
> +	    {
> +	      add_breakpoint_condition (point_addr, &dataptr);
> +	    } while (*dataptr == 'X');
> +	}
> +    }

Should we silently ignore unknown options, or error?  If the former,
then you should skip to the next `;' and go from there.  If the latter,
well, and error is missing.  :-)

> +typedef struct point_cond_list_t
> +{
> +  /* Pointer to the agent expression that is the breakpoint's
> +     conditional.  */
> +  struct agent_expr *cond;
> +
> +  /* Pointer to the next condition.  */
> +  struct point_cond_list_t *next;
> +} point_cond_list_t;

I know where the `typedef struct foo_t {} foo_t' style
comes from, but it's not gdb's or gdbserver's style.  Please drop the
typedef and the _t.

> +
>  /* A high level (in gdbserver's perspective) breakpoint.  */
>  struct breakpoint
>  {
> @@ -93,6 +105,12 @@ struct breakpoint
>    /* The breakpoint's type.  */
>    enum bkpt_type type;
>  
> +  /* Pointer to the condition list that should be evaluated on
> +     the target or NULL if the breakpoint is unconditional or
> +     if GDB doesn't want us to evaluate the conditionals on the
> +     target's side.  */
> +  struct point_cond_list_t *cond_list;
> +
>    /* Link to this breakpoint's raw breakpoint.  This is always
>       non-NULL.  */
>    struct raw_breakpoint *raw;
> @@ -632,7 +650,7 @@ delete_breakpoint (struct breakpoint *to
>    return delete_breakpoint_1 (proc, todel);
>  }
>  

> +/* Clear all conditions associated with this breakpoint address.  */
> +
> +void
> +clear_gdb_breakpoint_conditions (CORE_ADDR addr)
> +{
> +  struct breakpoint *bp = find_gdb_breakpoint_at (addr);
> +  struct point_cond_list_t *cond, **cond_p;
> +
> +  if (!bp || (bp && !bp->cond_list))
> +    return;

The "bp && " check is redundant.

  if (bp == NULL || bp->cond_list == NULL)
    return;


> +/* Add condition CONDITION to GDBServer's breakpoint BP.  */

GDBserver

>  int
> -gdb_breakpoint_here (CORE_ADDR where)
> +add_breakpoint_condition (CORE_ADDR addr, char **condition)
> +{
> +  struct breakpoint *bp = find_gdb_breakpoint_at (addr);
> +  char *actparm = (char *) *condition;

Why the cast?

> +  struct agent_expr *cond;
> +
> +  if (!bp)
> +    return 1;

I'd much rather NULL check styles were consistent.  Can you make
all those be explicit == NULL checks, like you have right below?

> +
> +  if (condition == NULL)
> +    return 1;
> +
> +  cond = gdb_parse_agent_expr (&actparm);
> +
> +  if (!cond)
> +    {
> +      fprintf (stderr, "Condition evaluation failed. "
> +	       "Assuming unconditional.\n");
> +      return 0;
> +    }
> +
> +  add_condition_to_breakpoint (bp, cond);
> +
> +  *condition = actparm;
> +
> +  return 0;
> +}
> +

> +  /* Evaluate each condition in the breakpoint's list of conditions.
> +     Return true if any of the conditions evaluate to TRUE.  */
> +  for (cl = bp->cond_list; cl && !value; cl = cl->next)
> +    {
> +      /* Evaluate the condition.  */
> +      gdb_eval_agent_expr (regcache, NULL, cl->cond, &value);
> +    }

This is ignoring gdb_eval_agent_expr's return code.  If the expression
for example touches unreadable memory and errors out, I think we should
still inform gdb of the breakpoint hit, and let it re-evaluate the
condition on its side (which reinforces the idea that gdb should always
reevaluate the conditions).  WDYT?

> +
> +  return (value != 0);
> +}
> +

-- 
Pedro Alves