On 02/09/2012 10:32 AM, Pedro Alves wrote:
> Some comments below, but overall looks good.

Thanks for the input.

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

This probably means all Z packets, but only breakpoints are being 
implemented now on both gdbserver's and gdb's side.

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

Fixed.

>
>> +	{
>> +	  /* 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.  :-)

Silently ignore. I thought further about the "conditions" marker and i 
decided to drop it. We may want to pass more attributes in the future, 
and these markers will be an overhead and will possibly get in the way.

I'm passing plain expressions now, with the first char identifying the 
action. This is in synch with how tracepoint actions/attributes are 
passed down to the target.

This also makes it easier to ignore unknown tokens, which in turn allows 
people to easily extend the list of attributes in Z packets in the future.

What do you think?

>> +  /* 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?
>

I agree. Thanks for pointing this out.

All the other things were fixed.

Luis