On 02/23/2012 02:32 PM, Pedro Alves wrote:
> Hi Luis,
>
> On 02/22/2012 03:17 PM, Luis Gustavo wrote:
>> On 02/09/2012 10:32 AM, Pedro Alves wrote:
>
>>>> 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.
>
> There's no probably :-).  GDB will need to be able to tell.
> Either it does, or it doesn't.  If watchpoints aren't supported now, we'll need
> a new qSupported bit latter when we support them.  (*) No biggie, but the
> documentation should be clear (may it already is though, haven't checked
> for that).

Let's stick with the new qSupported bit later then.

>
> (*) I thought about it a bit, and I had a moment of "damn, this isn't
> going to work", thinking about the fact that we're assuming the stubs
> must ignore Z packets for the same addresses, and that with watchpoints
> that might not work, given that the remote side does reference counting
> of resources, to handle overlapping watchpoints.  Then I remembered that
> GDB will never send exact duplicates of watchpoints, so it can send
> 1 watchpoint location for ADDR1;LEN1 and another for ADDR1;LEN2, but never
> two for ADDR1;LEN1 (assuming same type).  So we're good, and supporting
> this for watchpoints shouldn't be hard.

Good! You scared me... The breakpoint code already made me dizzy.

If all the Zx packets honor what the documentation says, we should be OK.

>
>>
>>>
>>>
>>>> +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?
>
> Hmm, thinking more, I don't think GDB should ever send tokens the
> stub didn't report support for in qSupported, so in practice it
> doesn't matter that much either way, but in any case:
>
>> +/* Process options coming from Z packets for *point at address
>> +   POINT_ADDR.  PACKET is the packet buffer.  *PACKET is updated
>> +   to point to the first char after the last processed option.  */
>> +
>> +static void
>> +process_point_options (CORE_ADDR point_addr, char **packet)
>> +{
>> +  char *dataptr = *packet;
>> +
>> +  /* Check if data has the correct format.  */
>> +  if (*dataptr != ';')
>> +    return;
>> +
>> +  dataptr++;
>> +
>> +  while (*dataptr)
>> +    {
>> +      switch (*dataptr)
>> +	{
>> +	  case 'X':
>> +	    /* Conditional expression.  */
>> +	    fprintf (stderr, "Found breakpoint condition.\n");
>> +	    add_breakpoint_condition (point_addr,&dataptr);
>> +	    break;
>> +	  default:
>> +	    /* Unrecognized token, just skip it.  */
>> +	    fprintf (stderr, "Unknown token %c, ignoring.\n",
>> +		     *dataptr);
>> +	    dataptr++;
>
> That's not the proper way to skip an unknown token.  You need to skip
> until some known marker (`;' or `,' or some such).

This section of the Z packet should contain only 0~9 chars and commas. A 
semicolon may be seen in the future if someones decides to add a new 
section. In any case, better be safe than sorry.

What about the following patch?

Luis