Re: [patch] gdbserver: Add support for Z0/Z1 packets

[Doug Evans <dje@google.com>]

On Tue, Jun 16, 2009 at 6:31 PM, Aleksandar Ristovski<aristovski@qnx.com> wrote:
> Aleksandar Ristovski wrote:
>>
>> Hello,
>>
>> This adds support for Z0 and Z1 (and z0 and z1) packets.
>>
>
> I apologize, the patch should be this one. Change log corrected as well.
>
> --
> Aleksandar Ristovski
> QNX Software Systems
>
>
> ChangeLog:
>
> * target.h (insert_breakpoint, remove_breakpoint): New target functions.
> * server.c (process_serial_event): 'Z' and 'z' packet - add support
> for [Zz]0 and [Zz]1 - software and hardware breakpoints.
> * linux-low.c (linux_target_ops): Add new fields.
> * spu-low.c (spu_target_ops): Likewise.
> * win32-low.c (win32_target_ops): Likewise.

Hi.

Minor nits (fwiw):

Inserting the insert_breakpoint/remove_breakpoint fields *between* the
stopped_by_watchpoint/stopped_data_address fields is odd.
Can they go before insert_watchpoint?

I think the code in server.c that watches for 0/1/2/3/4 should use a switch.

The code previously caught invalid values (e.g. type == '5'), but you
accidentally removed that (IIUC).

The code in server.c does this:

+           if (the_target->insert_watchpoint == NULL)
+             {
+               /* No watchpoint support or not a watchpoint command;
+                  unrecognized either way.  */
+               own_buf[0] = '\0';
+               res = 1;
+             }

and later does this:

+       if (res == 0)
+         write_ok (own_buf);
+       else if (res == 1)
+         /* Unsupported.  */
+         own_buf[0] = '\0';
+       else
+         write_enn (own_buf);

How about just setting res = 1 in the first code snippet above?
[This appears for both 'z' and 'Z' packets.]