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

Doug Evans
2009-06-19T07:08:58Z

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