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

[Antoine Tremblay <antoine dot tremblay at ericsson dot com>]

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.
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;
 
@@ -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 {
+    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)
 {
   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;
+  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);
+    }
 
   /* 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);
-- 
1.9.1