Re: [PATCH v3 1/5] Add the target_ops needed for software breakpoints in GDBServer.

[Pedro Alves <palves@redhat.com>]

On 10/20/2015 05:48 PM, Antoine Tremblay wrote:

> --- a/gdb/gdbserver/linux-low.h
> +++ b/gdb/gdbserver/linux-low.h
> @@ -141,8 +141,17 @@ struct linux_target_ops
>  
>    CORE_ADDR (*get_pc) (struct regcache *regcache);
>    void (*set_pc) (struct regcache *regcache, CORE_ADDR newpc);
> -  const unsigned char *breakpoint;
> -  int breakpoint_len;
> +
> +  /* Return the breakpoint kind for this target based on PC.  The PCPTR is
> +     adjusted to the real memory location in case a flag (e.g., the Thumb bit on
> +     ARM)  was present in the PC.  */

Spurious double space before "was present".

> +  int (*breakpoint_kind_from_pc) (CORE_ADDR *pcptr);
> +
> +  /* Return the software breakpoint from KIND.  KIND can have target
> +     specific meaning like the z0 kind parameter.

It's more usual to talk in terms of the "insert" packet.  That is,
write uppercase Z0.

> --- a/gdb/gdbserver/target.h
> +++ b/gdb/gdbserver/target.h
> @@ -441,6 +441,16 @@ struct target_ops
>       readlink(2).  */
>    ssize_t (*multifs_readlink) (int pid, const char *filename,
>  			       char *buf, size_t bufsiz);
> +
> +  /* Return the breakpoint kind for this target based on PC.  The PCPTR is
> +     adjusted to the real memory location in case a flag (e.g., the Thumb bit on
> +     ARM)  was present in the PC.  */
> +  int (*breakpoint_kind_from_pc) (CORE_ADDR *pcptr);
> +
> +  /* Return the software breakpoint from KIND.  KIND can have target
> +     specific meaning like the z0 kind parameter.
> +     SIZE is set to the software breakpoint's length in memory.  */
> +  const gdb_byte *(*sw_breakpoint_from_kind) (int kind, int *size);

Ditto.

I think the linux-low.h comments should just say "See target.h for details."
or some such, avoiding the duplication (and the forgetting to update
linux-low.h when target.h next changes).

Otherwise LGTM.

Thanks,
Pedro Alves