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

Pedro Alves
2012-02-23T16:34:48Z

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