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

[Pedro Alves <palves@redhat.com>]

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

(*) 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.

> 
>>
>>
>>> +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).
> +	}
> +    }
> +  *packet = dataptr;
> +}

Everything else looked good to me now.  Thanks.

-- 
Pedro Alves