Re: [PATCH v2 4/7] Support breakpoint kinds for software breakpoints in GDBServer.

[Pedro Alves <palves at redhat dot com>]

On 10/05/2015 05:44 PM, Antoine Tremblay wrote:
> This patch teaches GDBServer to:
> 
>  - choose the right breakpoint instruction for its own breakpoints, through API
>    set_breakpoint_at.
> 
>  - choose the right breakpoint instruction for breakpoints requested by GDB,
>   according to the information in Z packets, through API set_gdb_breakpoint.
> 
> New fields are introduced in struct raw_breakpoint:
> 
> pcfull: The PC including possible arch specific flags encoded in it.

"full" as opposed to "empty"?  Can we find a clearer term?

> data: This is the breakpoint's raw data.
> kind: As meant in a z0 packet this is the kind of breakpoint to be set.
> 
> Note this also clarifies existing fields usage :
> 
> pc: This is PC without any flags as a valid memory address.
> size: This is the breakpoint's size in memory.
> 
> Function signatures, and variables names have also been updated to make the
> distinction between size and kind clear.
> 
> Note also that an unimplemented breakpoint_from_kind operation is not an error
> as this would break targets like QNX Neutrino (nto).
> 
> To check for this kind of error a check for the breakpoint_data is done in
> insert_memory_breakpoint.
> 
> No regressions on Ubuntu 14.04 on ARMv7 and x86.
> With gdbserver-{native,extended} / { -marm -mthumb }
> 
> gdb/gdbserver/ChangeLog:
> 	* linux-low.c (initialize_low): Remove breakpoint_data initialization.
> 	* mem-break.c (struct raw_breakpoint) <pcfull>: New field.
> 	(struct raw_breakpoint) <data>: New field.
> 	(struct raw_breakpoint) <kind>: New field.
> 	(breakpoint_from_kind): New function.
> 	(insert_memory_breakpoint): Adjust to use bp->size and bp->data.
> 	(remove_memory_breakpoint): Adjust to use bp->size.
> 	(set_raw_breakpoint_at): Add pc, data, kind fields arguments and
> 	(set_breakpoint): Likewise.
> 	(set_breakpoint_at): Call breakpoint_from_pc.
> 	(delete_breakpoint): Rename size for kind.
> 	(find_gdb_breakpoint): Use kind rather than size.
> 	(set_gdb_breakpoint_1): Rename size for kind, call breakpoint_from_kind.
> 	(set_gdb_breakpoint): Rename size for kind.
> 	(delete_gdb_breakpoint_1): Rename size for kind.
> 	(delete_gdb_breakpoint_1): Likewise.
> 	(set_breakpoint_data): Remove.
> 	(validate_inserted_breakpoint): Adjust to use bp->size and bp->data.
> 	(check_mem_read): Adjust to use bp->size.
> 	(check_mem_write): Adjust to use bp->size and bp->data.
> 	(clone_one_breakpoint): Clone new fields, pcfull, data, kind.
> 	* server.c (process_serial_event): Rename len for kind.
> ---
>  gdb/gdbserver/linux-low.c |   6 --
>  gdb/gdbserver/mem-break.c | 154 +++++++++++++++++++++++++++++-----------------
>  gdb/gdbserver/server.c    |   8 +--
>  3 files changed, 100 insertions(+), 68 deletions(-)
> 
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index 5aa2b1d..c410deb 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -7077,16 +7077,10 @@ void
>  initialize_low (void)
>  {
>    struct sigaction sigchld_action;
> -  int breakpoint_len = 0;
> -  const unsigned char *breakpoint = NULL;
>  
>    memset (&sigchld_action, 0, sizeof (sigchld_action));
>    set_target_ops (&linux_target_ops);
>  
> -  breakpoint = the_target->breakpoint_from_pc (NULL, &breakpoint_len);
> -
> -  set_breakpoint_data (breakpoint,
> -		       breakpoint_len);
>    linux_init_signals ();
>    linux_ptrace_init_warnings ();
>  
> diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
> index f077497..4299cfa 100644
> --- a/gdb/gdbserver/mem-break.c
> +++ b/gdb/gdbserver/mem-break.c
> @@ -21,8 +21,6 @@
>  #include "server.h"
>  #include "regcache.h"
>  #include "ax.h"
> -const unsigned char *breakpoint_data;
> -int breakpoint_len;
>  
>  #define MAX_BREAKPOINT_LEN 8
>  
> @@ -100,6 +98,16 @@ struct raw_breakpoint
>       breakpoint for a given PC.  */
>    CORE_ADDR pc;
>  
> +  /* The breakpoint's insertion address, possibly with flags encoded in the pc
> +     (e.g. the instruction mode on ARM).  */
> +  CORE_ADDR pcfull;
> +
> +  /* The breakpoint's data */
> +  const unsigned char *data;
> +
> +  /* The breakpoint's kind.  */
> +  int kind;
> +
>    /* The breakpoint's size.  */
>    int size;

Can't we always find the size from pcfull and kind ?

>  
> @@ -293,6 +301,30 @@ find_raw_breakpoint_at (CORE_ADDR addr, enum raw_bkpt_type type, int size)
>    return NULL;
>  }
>  
> +/* Try to resolve the real breakpoint size from the breakpoint kind  */
> +
> +static int
> +breakpoint_from_kind (int kind,
> +		      const unsigned char **breakpoint_data,
> +		      int *breakpoint_len)
> +{
> +  /* Get the arch dependent breakpoint.  */
> +  if (*the_target->breakpoint_from_kind != NULL)
> +    {
> +      /* Update magic coded size to the right size if needed.  */
> +      *breakpoint_data =
> +       (*the_target->breakpoint_from_kind) (&kind);
> +      *breakpoint_len = kind;
> +    }
> +  else {

Formatting.

> +    if (debug_threads)
> +      debug_printf ("Don't know breakpoints of size %d.\n",
> +                   kind);
> +    return -1;
> +  }
> +  return 0;
> +}
> +
>  /* See mem-break.h.  */
>  
>  int
> @@ -301,24 +333,17 @@ insert_memory_breakpoint (struct raw_breakpoint *bp)
>    unsigned char buf[MAX_BREAKPOINT_LEN];
>    int err;
>  
> -  if (breakpoint_data == NULL)
> -    return 1;
> -
> -  /* If the architecture treats the size field of Z packets as a
> -     'kind' field, then we'll need to be able to know which is the
> -     breakpoint instruction too.  */
> -  if (bp->size != breakpoint_len)
> +  if (bp->data == NULL)
>      {
>        if (debug_threads)
> -	debug_printf ("Don't know how to insert breakpoints of size %d.\n",
> -		      bp->size);
> +	debug_printf ("No breakpoint data present");
>        return -1;
>      }
>  
>    /* Note that there can be fast tracepoint jumps installed in the
>       same memory range, so to get at the original memory, we need to
>       use read_inferior_memory, which masks those out.  */
> -  err = read_inferior_memory (bp->pc, buf, breakpoint_len);
> +  err = read_inferior_memory (bp->pc, buf, bp->size);
>    if (err != 0)
>      {
>        if (debug_threads)
> @@ -328,10 +353,9 @@ insert_memory_breakpoint (struct raw_breakpoint *bp)
>      }
>    else
>      {
> -      memcpy (bp->old_data, buf, breakpoint_len);
> +      memcpy (bp->old_data, buf, bp->size);
>  
> -      err = (*the_target->write_memory) (bp->pc, breakpoint_data,
> -					 breakpoint_len);
> +      err = (*the_target->write_memory) (bp->pc, bp->data, bp->size);
>        if (err != 0)
>  	{
>  	  if (debug_threads)
> @@ -358,8 +382,8 @@ remove_memory_breakpoint (struct raw_breakpoint *bp)
>       note that we need to pass the current shadow contents, because
>       write_inferior_memory updates any shadow memory with what we pass
>       here, and we want that to be a nop.  */
> -  memcpy (buf, bp->old_data, breakpoint_len);
> -  err = write_inferior_memory (bp->pc, buf, breakpoint_len);
> +  memcpy (buf, bp->old_data, bp->size);
> +  err = write_inferior_memory (bp->pc, buf, bp->size);
>    if (err != 0)
>      {
>        if (debug_threads)
> @@ -375,15 +399,16 @@ remove_memory_breakpoint (struct raw_breakpoint *bp)
>     returns NULL and writes the error code to *ERR.  */
>  
>  static struct raw_breakpoint *
> -set_raw_breakpoint_at (enum raw_bkpt_type type, CORE_ADDR where, int size,
> -		       int *err)
> +set_raw_breakpoint_at (enum raw_bkpt_type type, const CORE_ADDR where,
> +		       const CORE_ADDR pc, const unsigned char* data, int kind,
> +		       int size, int *err)

Which is which: "where" vs "pc" | "pc" vs "pcfull" ?  I think the terminology
should be consistent throughout.  Also remember to update intro comments.

>  {
>    struct process_info *proc = current_process ();
>    struct raw_breakpoint *bp;
>  
>    if (type == raw_bkpt_type_sw || type == raw_bkpt_type_hw)
>      {
> -      bp = find_enabled_raw_code_breakpoint_at (where, type);
> +      bp = find_enabled_raw_code_breakpoint_at (pc, type);
>        if (bp != NULL && bp->size != size)
>  	{
>  	  /* A different size than previously seen.  The previous
> @@ -396,7 +421,7 @@ set_raw_breakpoint_at (enum raw_bkpt_type type, CORE_ADDR where, int size,
>  	}
>      }
>    else
> -    bp = find_raw_breakpoint_at (where, type, size);
> +    bp = find_raw_breakpoint_at (pc, type, size);
>  
>    if (bp != NULL)
>      {
> @@ -405,12 +430,15 @@ set_raw_breakpoint_at (enum raw_bkpt_type type, CORE_ADDR where, int size,
>      }
>  
>    bp = XCNEW (struct raw_breakpoint);
> -  bp->pc = where;
> +  bp->pcfull = where;
> +  bp->pc = pc;
> +  bp->data = data;

Why do we need to store "data" per breakpoint?  Can't we just call
the_target->breakpoint_from_pc when necessary?

> +  bp->kind = kind;
>    bp->size = size;
>    bp->refcount = 1;
>    bp->raw_type = type;
>  
> -  *err = the_target->insert_point (bp->raw_type, bp->pc, bp->size, bp);
> +  *err = the_target->insert_point (bp->raw_type, bp->pcfull, kind, bp);
>    if (*err != 0)
>      {
>        if (debug_threads)
> @@ -740,14 +768,14 @@ reinsert_fast_tracepoint_jumps_at (CORE_ADDR where)
>  
>  static struct breakpoint *
>  set_breakpoint (enum bkpt_type type, enum raw_bkpt_type raw_type,
> -		CORE_ADDR where, int size,
> -		int (*handler) (CORE_ADDR), int *err)
> +		CORE_ADDR where, CORE_ADDR pc, const unsigned char *data,
> +		int kind, int size, int (*handler) (CORE_ADDR), int *err)
>  {
>    struct process_info *proc = current_process ();
>    struct breakpoint *bp;
>    struct raw_breakpoint *raw;
>  
> -  raw = set_raw_breakpoint_at (raw_type, where, size, err);
> +  raw = set_raw_breakpoint_at (raw_type, where, pc, data, kind, size, err);
>  
>    if (raw == NULL)
>      {
> @@ -774,9 +802,15 @@ set_breakpoint_at (CORE_ADDR where, int (*handler) (CORE_ADDR))
>  {
>    int err_ignored;
>  
> +  const unsigned char *breakpoint_data;
> +  int breakpoint_len;
> +  CORE_ADDR pc = where;
> +
> +  breakpoint_data = the_target->breakpoint_from_pc (&pc, &breakpoint_len);
> +
>    return set_breakpoint (other_breakpoint, raw_bkpt_type_sw,
> -			 where, breakpoint_len, handler,
> -			 &err_ignored);
> +			 where, pc, breakpoint_data, breakpoint_len,
> +			 breakpoint_len, handler, &err_ignored);
>  }
>  
>  
> @@ -891,12 +925,12 @@ delete_breakpoint (struct breakpoint *todel)
>    return delete_breakpoint_1 (proc, todel);
>  }
>  
> -/* Locate a GDB breakpoint of type Z_TYPE and size SIZE placed at
> -   address ADDR and return a pointer to its structure.  If SIZE is -1,
> +/* Locate a GDB breakpoint of type Z_TYPE and kind KIND placed at
> +   address ADDR and return a pointer to its structure.  If KIND is -1,
>     the breakpoints' sizes are ignored.  */
>  
>  static struct breakpoint *
> -find_gdb_breakpoint (char z_type, CORE_ADDR addr, int size)
> +find_gdb_breakpoint (char z_type, CORE_ADDR addr, int kind)
>  {
>    struct process_info *proc = current_process ();
>    struct breakpoint *bp;
> @@ -904,7 +938,7 @@ find_gdb_breakpoint (char z_type, CORE_ADDR addr, int size)
>  
>    for (bp = proc->breakpoints; bp != NULL; bp = bp->next)
>      if (bp->type == type && bp->raw->pc == addr
> -	&& (size == -1 || bp->raw->size == size))
> +	&& (kind == -1 || bp->raw->kind == kind))
>        return bp;
>  
>    return NULL;
> @@ -918,17 +952,24 @@ z_type_supported (char z_type)
>  	  && the_target->supports_z_point_type (z_type));
>  }
>  
> -/* Create a new GDB breakpoint of type Z_TYPE at ADDR with size SIZE.
> +/* Create a new GDB breakpoint of type Z_TYPE at ADDR with kind KIND.
>     Returns a pointer to the newly created breakpoint on success.  On
>     failure returns NULL and sets *ERR to either -1 for error, or 1 if
>     Z_TYPE breakpoints are not supported on this target.  */
>  
>  static struct breakpoint *
> -set_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int size, int *err)
> +set_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int kind, int *err)
>  {
>    struct breakpoint *bp;
>    enum bkpt_type type;
>    enum raw_bkpt_type raw_type;
> +  const unsigned char *breakpoint_data = NULL;
> +  int breakpoint_len = kind;
> +
> +  if (z_type == Z_PACKET_SW_BP)
> +    {
> +      breakpoint_from_kind (kind, &breakpoint_data, &breakpoint_len);
> +    }

Unnecessary braces.

>  
>    /* If we see GDB inserting a second code breakpoint at the same
>       address, then either: GDB is updating the breakpoint's conditions
> @@ -952,9 +993,9 @@ set_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int size, int *err)
>  
>        if (bp != NULL)
>  	{
> -	  if (bp->raw->size != size)
> +	  if (bp->raw->kind != kind)
>  	    {
> -	      /* A different size than previously seen.  The previous
> +	      /* A different kind than previously seen.  The previous
>  		 breakpoint must be gone then.  */
>  	      bp->raw->inserted = -1;
>  	      delete_breakpoint (bp);
> @@ -978,7 +1019,7 @@ set_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int size, int *err)
>        /* Data breakpoints for the same address but different size are
>  	 expected.  GDB doesn't merge these.  The backend gets to do
>  	 that if it wants/can.  */
> -      bp = find_gdb_breakpoint (z_type, addr, size);
> +      bp = find_gdb_breakpoint (z_type, addr, kind);
>      }
>  
>    if (bp != NULL)
> @@ -993,7 +1034,8 @@ set_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int size, int *err)
>  
>    raw_type = Z_packet_to_raw_bkpt_type (z_type);
>    type = Z_packet_to_bkpt_type (z_type);
> -  return set_breakpoint (type, raw_type, addr, size, NULL, err);
> +  return set_breakpoint (type, raw_type, addr, addr, breakpoint_data, kind,
> +			 breakpoint_len, NULL, err);
>  }
>  
>  static int
> @@ -1024,7 +1066,7 @@ check_gdb_bp_preconditions (char z_type, int *err)
>     knows to prepare to access memory for Z0 breakpoints.  */
>  
>  struct breakpoint *
> -set_gdb_breakpoint (char z_type, CORE_ADDR addr, int size, int *err)
> +set_gdb_breakpoint (char z_type, CORE_ADDR addr, int kind, int *err)
>  {
>    struct breakpoint *bp;
>  
> @@ -1040,7 +1082,7 @@ set_gdb_breakpoint (char z_type, CORE_ADDR addr, int size, int *err)
>  	return NULL;
>      }
>  
> -  bp = set_gdb_breakpoint_1 (z_type, addr, size, err);
> +  bp = set_gdb_breakpoint_1 (z_type, addr, kind, err);
>  
>    if (z_type == Z_PACKET_SW_BP)
>      done_accessing_memory ();
> @@ -1048,18 +1090,18 @@ set_gdb_breakpoint (char z_type, CORE_ADDR addr, int size, int *err)
>    return bp;
>  }
>  
> -/* Delete a GDB breakpoint of type Z_TYPE and size SIZE previously
> +/* Delete a GDB breakpoint of type Z_TYPE and kind KIND previously
>     inserted at ADDR with set_gdb_breakpoint_at.  Returns 0 on success,
>     -1 on error, and 1 if Z_TYPE breakpoints are not supported on this
>     target.  */
>  
>  static int
> -delete_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int size)
> +delete_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int kind)
>  {
>    struct breakpoint *bp;
>    int err;
>  
> -  bp = find_gdb_breakpoint (z_type, addr, size);
> +  bp = find_gdb_breakpoint (z_type, addr, kind);
>    if (bp == NULL)
>      return -1;
>  
> @@ -1077,7 +1119,7 @@ delete_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int size)
>     knows to prepare to access memory for Z0 breakpoints.  */
>  
>  int
> -delete_gdb_breakpoint (char z_type, CORE_ADDR addr, int size)
> +delete_gdb_breakpoint (char z_type, CORE_ADDR addr, int kind)
>  {
>    int ret;
>  
> @@ -1095,7 +1137,7 @@ delete_gdb_breakpoint (char z_type, CORE_ADDR addr, int size)
>  	return -1;
>      }
>  
> -  ret = delete_gdb_breakpoint_1 (z_type, addr, size);
> +  ret = delete_gdb_breakpoint_1 (z_type, addr, kind);
>  
>    if (z_type == Z_PACKET_SW_BP)
>      done_accessing_memory ();
> @@ -1588,13 +1630,6 @@ check_breakpoints (CORE_ADDR stop_pc)
>      }
>  }
>  
> -void
> -set_breakpoint_data (const unsigned char *bp_data, int bp_len)
> -{
> -  breakpoint_data = bp_data;
> -  breakpoint_len = bp_len;
> -}
> -
>  int
>  breakpoint_here (CORE_ADDR addr)
>  {
> @@ -1669,9 +1704,9 @@ validate_inserted_breakpoint (struct raw_breakpoint *bp)
>    gdb_assert (bp->inserted);
>    gdb_assert (bp->raw_type == raw_bkpt_type_sw);
>  
> -  buf = (unsigned char *) alloca (breakpoint_len);
> -  err = (*the_target->read_memory) (bp->pc, buf, breakpoint_len);
> -  if (err || memcmp (buf, breakpoint_data, breakpoint_len) != 0)
> +  buf = (unsigned char *) alloca (bp->size);
> +  err = (*the_target->read_memory) (bp->pc, buf, bp->size);
> +  if (err || memcmp (buf, bp->data, bp->size) != 0)
>      {
>        /* Tag it as gone.  */
>        bp->inserted = -1;
> @@ -1762,7 +1797,7 @@ check_mem_read (CORE_ADDR mem_addr, unsigned char *buf, int mem_len)
>  
>    for (; bp != NULL; bp = bp->next)
>      {
> -      CORE_ADDR bp_end = bp->pc + breakpoint_len;
> +      CORE_ADDR bp_end = bp->pc + bp->size;
>        CORE_ADDR start, end;
>        int copy_offset, copy_len, buf_offset;
>  
> @@ -1851,7 +1886,7 @@ check_mem_write (CORE_ADDR mem_addr, unsigned char *buf,
>  
>    for (; bp != NULL; bp = bp->next)
>      {
> -      CORE_ADDR bp_end = bp->pc + breakpoint_len;
> +      CORE_ADDR bp_end = bp->pc + bp->size;
>        CORE_ADDR start, end;
>        int copy_offset, copy_len, buf_offset;
>  
> @@ -1882,7 +1917,7 @@ check_mem_write (CORE_ADDR mem_addr, unsigned char *buf,
>        if (bp->inserted > 0)
>  	{
>  	  if (validate_inserted_breakpoint (bp))
> -	    memcpy (buf + buf_offset, breakpoint_data + copy_offset, copy_len);
> +	    memcpy (buf + buf_offset, bp->data + copy_offset, copy_len);
>  	  else
>  	    disabled_one = 1;
>  	}
> @@ -1963,6 +1998,9 @@ clone_one_breakpoint (const struct breakpoint *src)
>    dest_raw->raw_type = src->raw->raw_type;
>    dest_raw->refcount = src->raw->refcount;
>    dest_raw->pc = src->raw->pc;
> +  dest_raw->pcfull = src->raw->pcfull;
> +  dest_raw->data = src->raw->data;
> +  dest_raw->kind = src->raw->kind;
>    dest_raw->size = src->raw->size;
>    memcpy (dest_raw->old_data, src->raw->old_data, MAX_BREAKPOINT_LEN);
>    dest_raw->inserted = src->raw->inserted;
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index e25b7c7..ad6626e 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -4069,20 +4069,20 @@ process_serial_event (void)
>        {
>  	char *dataptr;
>  	ULONGEST addr;
> -	int len;
> +	int kind;
>  	char type = own_buf[1];
>  	int res;
>  	const int insert = ch == 'Z';
>  	char *p = &own_buf[3];
>  
>  	p = unpack_varlen_hex (p, &addr);
> -	len = strtol (p + 1, &dataptr, 16);
> +	kind = strtol (p + 1, &dataptr, 16);
>  
>  	if (insert)
>  	  {
>  	    struct breakpoint *bp;
>  
> -	    bp = set_gdb_breakpoint (type, addr, len, &res);
> +	    bp = set_gdb_breakpoint (type, addr, kind, &res);
>  	    if (bp != NULL)
>  	      {
>  		res = 0;
> @@ -4097,7 +4097,7 @@ process_serial_event (void)
>  	      }
>  	  }
>  	else
> -	  res = delete_gdb_breakpoint (type, addr, len);
> +	  res = delete_gdb_breakpoint (type, addr, kind);
>  
>  	if (res == 0)
>  	  write_ok (own_buf);
> 


Thanks,
Pedro Alves