[PATCH v2 0/7] Software breakpoints support for ARM linux in GDBServer.

[PATCH v2 0/7] Software breakpoints support for ARM linux in GDBServer.

[Antoine Tremblay]

In this v2 :

The main change is that the functions breakpoint_from_pc and
breakpoint_from_kind are now called upstream in the call flow.
As suggested in : https://sourceware.org/ml/gdb-patches/2015-09/msg00597.html

Thus the patchset is restructured for that, patches have been renamed and
reordered.

Patches that have changed have their own v2 comment.

Original comment is also updated :

-----
This patch series adds software breakpoint support for ARM on linux in
GDBServer.

This is a subset of a previous patchset :
https://sourceware.org/ml/gdb-patches/2015-09/msg00221.html

Other patches in that previous patchset will be sent after.

The problematic with ARM software breakpoints is that it can have 3 different
breakpoints one for each instruction set : arm, thumb, thumb2.

So we need to be able to set different kinds of breakpoints when GDBServer sets
it's own breakpoints or when a breakpoint is set using GDB's Z0 packet.

In order to allow that the patches :

[PATCH v2 1/7] Add breakpoint_from_pc target_ops for software breakpoints in
GDBServer.

Adds a new target operation to replace the static breakpoint and breakpoint_len
operation so that the target can return the proper GDBServer breakpoint.

[PATCH v2 2/7] Add breakpoint_from_kind target_ops for software breakpoints in
GDBServer.

Software breakpoints set by GDB can have a arch specific encoded length called
the breakpoint kind like it is the case for ARM.

This patch adds a target operation such that the correct breakpoint can be
returned based on that kind.

[PATCH v2 3/7] Implement breakpoint_from_kind for supported architectures in
GDBServer.

x86 and aarch64 support software breakpoints from GDB and need to implement
breakpoint_from_kind.

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

This is the main patch of the set, it handles the GDBServer's software
breakpoints by calling the breakpoint_from_pc operation and GDB's software
breakpoints by calling breakpoint_from_kind.

[PATCH v2 5/7] Implement breakpoint_from_pc for ARM in GDBServer.

This patch adds the ARM implementation of that target operation, selecting the
thumb, thumb2, or arm breakpoint based on the PC addresss and the flags encoded
in it.

[PATCH v2 6/7] Refactor the breakpoint definitions in linux-arm-low.c.

This patch refactors the breakpoint definitions a bit to be more clear.

[PATCH v2 7/7] Support software breakpoints for ARM linux in GDBServer.

And finally software breakpoints via Z0 packets are enabled for ARM.

This patchset has no regressions, tested on ubuntu 14.04 ARMv7 and x86.
With gdbserver-{native,extended} / { -marm -mthumb }

Note also that while I could not test directly thumbv1 instructions with gcc
-marmv4t , manual testing of the software breakpoints was done for thumv1
instructions.

[PATCH v2 6/7] Refactor the breakpoint definitions in linux-arm-low.c.

[Antoine Tremblay]

Before arm_breakpoint_from_pc would use an #ifdef to return the right
arm_breakpoint from the abi or eabi breakpoint type.

arm_breakpoint_at would also check for the arm_breakpoint ||
arm_eabi_breakpoint.

Thus the selected arm_breakpoint would be what arm_breakpoint_from_pc returned
and arm_breakpoint was arm_abi_breakpoint.

This patch makes it more clear by naming those for what they are : 2 separate
entities: arm_abi_breakpoint and arm_eabi_breakpoint and set the current used
one as arm_breakpoint.

This allows a cleaner arm_breakpoint_from_pc as it just returns arm_breakpoint
rather than having the #ifdef in that function.

Any other reference to the arm_breakpoint can now also be clear of #ifdefs...

No regressions on Ubuntu 14.04 on ARMv7 and x86.
With gdbserver-{native,extended} / { -marm -mthumb }

gdb/gdbserver/ChangeLog:
	* linux-arm-low.c: Refactor breakpoint definitions.
	(arm_breakpoint_at): Adjust for arm_abi_breakpoint.
	(arm_breakpoint_from_pc): Adjust for arm_breakpoint.
---
 gdb/gdbserver/linux-arm-low.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
index 8f420f9..d16ea60 100644
--- a/gdb/gdbserver/linux-arm-low.c
+++ b/gdb/gdbserver/linux-arm-low.c
@@ -236,18 +236,25 @@ arm_set_pc (struct regcache *regcache, CORE_ADDR pc)
 }
 
 /* Correct in either endianness.  */
-static const unsigned long arm_breakpoint = 0xef9f0001;
-#define arm_breakpoint_len 4
-static const unsigned short thumb_breakpoint = 0xde01;
-#define thumb_breakpoint_len 2
-static const unsigned short thumb2_breakpoint[] = { 0xf7f0, 0xa000 };
-#define thumb2_breakpoint_len 4
+#define arm_abi_breakpoint 0xef9f0001UL
 
 /* For new EABI binaries.  We recognize it regardless of which ABI
    is used for gdbserver, so single threaded debugging should work
    OK, but for multi-threaded debugging we only insert the current
    ABI's breakpoint instruction.  For now at least.  */
-static const unsigned long arm_eabi_breakpoint = 0xe7f001f0;
+#define arm_eabi_breakpoint 0xe7f001f0UL
+
+#ifndef __ARM_EABI__
+static const unsigned long arm_breakpoint = arm_abi_breakpoint;
+#else
+static const unsigned long arm_breakpoint = arm_eabi_breakpoint;
+#endif
+
+#define arm_breakpoint_len 4
+static const unsigned short thumb_breakpoint = 0xde01;
+#define thumb_breakpoint_len 2
+static const unsigned short thumb2_breakpoint[] = { 0xf7f0, 0xa000 };
+#define thumb2_breakpoint_len 4
 
 static int
 arm_breakpoint_at (CORE_ADDR where)
@@ -279,7 +286,7 @@ arm_breakpoint_at (CORE_ADDR where)
       unsigned long insn;
 
       (*the_target->read_memory) (where, (unsigned char *) &insn, 4);
-      if (insn == arm_breakpoint)
+      if (insn == arm_abi_breakpoint)
 	return 1;
 
       if (insn == arm_eabi_breakpoint)
@@ -325,11 +332,7 @@ arm_breakpoint_from_pc (CORE_ADDR *pcptr, int *lenptr)
   else
     {
       *lenptr = arm_breakpoint_len;
-#ifndef __ARM_EABI__
       return (const unsigned char *) &arm_breakpoint;
-#else
-      return (const unsigned char *) &arm_eabi_breakpoint;
-#endif
     }
 }
 
-- 
1.9.1

Re: [PATCH v2 6/7] Refactor the breakpoint definitions in linux-arm-low.c.

[Pedro Alves]

On 10/05/2015 05:44 PM, Antoine Tremblay wrote:
> Before arm_breakpoint_from_pc would use an #ifdef to return the right
> arm_breakpoint from the abi or eabi breakpoint type.
> 
> arm_breakpoint_at would also check for the arm_breakpoint ||
> arm_eabi_breakpoint.
> 
> Thus the selected arm_breakpoint would be what arm_breakpoint_from_pc returned
> and arm_breakpoint was arm_abi_breakpoint.
> 
> This patch makes it more clear by naming those for what they are : 2 separate
> entities: arm_abi_breakpoint and arm_eabi_breakpoint and set the current used
> one as arm_breakpoint.
> 
> This allows a cleaner arm_breakpoint_from_pc as it just returns arm_breakpoint
> rather than having the #ifdef in that function.
> 
> Any other reference to the arm_breakpoint can now also be clear of #ifdefs...
> 
> No regressions on Ubuntu 14.04 on ARMv7 and x86.
> With gdbserver-{native,extended} / { -marm -mthumb }
> 
> gdb/gdbserver/ChangeLog:
> 	* linux-arm-low.c: Refactor breakpoint definitions.
> 	(arm_breakpoint_at): Adjust for arm_abi_breakpoint.
> 	(arm_breakpoint_from_pc): Adjust for arm_breakpoint.
> ---
>  gdb/gdbserver/linux-arm-low.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
> index 8f420f9..d16ea60 100644
> --- a/gdb/gdbserver/linux-arm-low.c
> +++ b/gdb/gdbserver/linux-arm-low.c
> @@ -236,18 +236,25 @@ arm_set_pc (struct regcache *regcache, CORE_ADDR pc)
>  }
>  
>  /* Correct in either endianness.  */
> -static const unsigned long arm_breakpoint = 0xef9f0001;
> -#define arm_breakpoint_len 4
> -static const unsigned short thumb_breakpoint = 0xde01;
> -#define thumb_breakpoint_len 2
> -static const unsigned short thumb2_breakpoint[] = { 0xf7f0, 0xa000 };
> -#define thumb2_breakpoint_len 4
> +#define arm_abi_breakpoint 0xef9f0001UL
>  
>  /* For new EABI binaries.  We recognize it regardless of which ABI
>     is used for gdbserver, so single threaded debugging should work
>     OK, but for multi-threaded debugging we only insert the current
>     ABI's breakpoint instruction.  For now at least.  */
> -static const unsigned long arm_eabi_breakpoint = 0xe7f001f0;
> +#define arm_eabi_breakpoint 0xe7f001f0UL
> +
> +#ifndef __ARM_EABI__
> +static const unsigned long arm_breakpoint = arm_abi_breakpoint;
> +#else
> +static const unsigned long arm_breakpoint = arm_eabi_breakpoint;
> +#endif
> +
> +#define arm_breakpoint_len 4
> +static const unsigned short thumb_breakpoint = 0xde01;
> +#define thumb_breakpoint_len 2
> +static const unsigned short thumb2_breakpoint[] = { 0xf7f0, 0xa000 };
> +#define thumb2_breakpoint_len 4
>  
>  static int
>  arm_breakpoint_at (CORE_ADDR where)
> @@ -279,7 +286,7 @@ arm_breakpoint_at (CORE_ADDR where)
>        unsigned long insn;
>  
>        (*the_target->read_memory) (where, (unsigned char *) &insn, 4);
> -      if (insn == arm_breakpoint)
> +      if (insn == arm_abi_breakpoint)
>  	return 1;
>  
>        if (insn == arm_eabi_breakpoint)
> @@ -325,11 +332,7 @@ arm_breakpoint_from_pc (CORE_ADDR *pcptr, int *lenptr)
>    else
>      {
>        *lenptr = arm_breakpoint_len;
> -#ifndef __ARM_EABI__
>        return (const unsigned char *) &arm_breakpoint;
> -#else
> -      return (const unsigned char *) &arm_eabi_breakpoint;
> -#endif
>      }
>  }
>  
> 

LGTM.  (Thought please collect an ack from Yao as well.)

Thanks,
Pedro Alves

Re: [PATCH v2 6/7] Refactor the breakpoint definitions in linux-arm-low.c.

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> LGTM.  (Thought please collect an ack from Yao as well.)

Looks good to me too.

-- 
Yao (齐尧)

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

[Antoine Tremblay]

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

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

[Pedro Alves]

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

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

[Antoine Tremblay]

On 10/15/2015 11:51 AM, Pedro Alves wrote:
> 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?
>

full as opposed to incomplete, meaning it includes all it could include. 
Other then that I would see :

pcencoded ?

pcflaged ?

pcwithflags ?

Not an easy one..

>> @@ -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 ?
>

We could but then we would have to call breakpoint_from_kind in a lot of 
places basically everywhere bp->size is referenced like :

check_mem_read
check_mem_write
insert_memory_breakpoint
remove_memory_breakpoint
set_raw_breakpoint_at
validate_inserted_breakpoint
delete_raw_breakpoint
uninsert_raw_breakpoint
reinsert_raw_breakpoint
find_raw_breakpoint_at

Also since these functions can be called in a stack one would have to be 
careful to call breakpoint_from_kind at the right level and pass it 
down.. and then size/kind becomes confusing.

Also, this is a bit what I did in v1 but changed based on discussions 
with Yao see :

https://sourceware.org/ml/gdb-patches/2015-09/msg00597.html

I think it's more clear to call the function once and set the variable.

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

Done.

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

Yes indeed this is confusing but I hesitated to change it since across 
gdb "where" is used for a location, even before this change where was 
translated to pc in the breakpoint struct.

It felt a bit weird to call set_breakpoint_at(pcfull) compared to like 
find_breakpoint_at (where).

But in this case we have where and pc I think it's necessary indeed.

Done.

>> @@ -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?

For the same reasons as expressed before for ->size I think it's better 
not to call breakpoint_from_pc at the lowest level.

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

Done.

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

[Pedro Alves]

On 10/15/2015 07:02 PM, Antoine Tremblay wrote:
> 
> 
> On 10/15/2015 11:51 AM, Pedro Alves wrote:
>> 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?
>>
> 
> full as opposed to incomplete, meaning it includes all it could include. 
> Other then that I would see :
> 
> pcencoded ?
> 
> pcflaged ?
> 
> pcwithflags ?
> 
> Not an easy one..

GDB calls them "placed address" and "requested address":

struct bp_target_info
{
...
  /* Address at which the breakpoint was placed.  This is normally
     the same as REQUESTED_ADDRESS, except when adjustment happens in
     gdbarch_breakpoint_from_pc.  The most common form of adjustment
     is stripping an alternate ISA marker from the PC which is used
     to determine the type of breakpoint to insert.  */
  CORE_ADDR placed_address;

  /* Address at which the breakpoint was requested.  */
  CORE_ADDR reqstd_address;


> 
>>> @@ -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 ?
>>
> 
> We could but then we would have to call breakpoint_from_kind in a lot of 
> places basically everywhere bp->size is referenced like :
> 
> check_mem_read
> check_mem_write
> insert_memory_breakpoint
> remove_memory_breakpoint
> set_raw_breakpoint_at
> validate_inserted_breakpoint
> delete_raw_breakpoint
> uninsert_raw_breakpoint
> reinsert_raw_breakpoint
> find_raw_breakpoint_at

See below.

> 
> Also since these functions can be called in a stack one would have to be 
> careful to call breakpoint_from_kind at the right level and pass it 
> down.. and then size/kind becomes confusing.
> 
> Also, this is a bit what I did in v1 but changed based on discussions 
> with Yao see :
> 
> https://sourceware.org/ml/gdb-patches/2015-09/msg00597.html
> 
> I think it's more clear to call the function once and set the variable.

I don't see why my comment conflicts with Yao's.  But I think we
could simplify the interfaces and entry points, and get rid of the
duplication, like this:

Replace the breakpoint_from_pc method with a breakpoint_kind_from_pc
method.  This adjusts the PC (if necessary) and returns the
breakpoint _kind_ instead of the breakpoint opcode / data.

enum arm_breakpoint_kinds
{
   ARM_BP_KIND_THUMB = 2,
   ARM_BP_KIND_THUMB2 = 3,
   ARM_BP_KIND_ARM = 4,
};

static int
arm_breakpoint_kind_from_pc (CORE_ADDR *pcptr, int len)
{
  if (IS_THUMB_ADDR (*pcptr))
    {
      gdb_byte buf[2];

      *pcptr = UNMAKE_THUMB_ADDR (*pcptr);

      /* Check whether we are replacing a thumb2 32-bit instruction.  */
      if ((*the_target->read_memory) (*pcptr, buf, 2) == 0)
	{
	  unsigned short inst1 = 0;

	  (*the_target->read_memory) (*pcptr, (gdb_byte *) &inst1, 2);
	  if (thumb_insn_size (inst1) == 4)
            return ARM_BP_KIND_THUMB2;
	}

      return ARM_BP_KIND_THUMB;
    }
  else
    return ARM_BP_KIND_ARM;
}

Then the breakpoints functions and structures always work
with the already-adjusted PC, and with a breakpoint-kind.

for internal breakpoints, we have:

  set_breakpoint_at (breakpoint_kind_from_pc, to find bp kind,
                     rest the same as today)
  set_gdb_breakpoint_1 (same as today)
     |
     `--> set_breakpoint (address, kind)
             |
             `-->set_raw_breakpoint_at (address, kind)
                    |
                    `--> the_target->insert_point (address, kind)

Everything thinks in terms of breakpoint kind.  Then the only
places that need to know the real breakpoint instruction opcode
and opcode size can query the breakpoint_from_kind target method
you already added.

About:

> We could but then we would have to call breakpoint_from_kind in a lot of
> places basically everywhere bp->size is referenced like :
>
> check_mem_read
> check_mem_write
> insert_memory_breakpoint
> remove_memory_breakpoint
> set_raw_breakpoint_at
> validate_inserted_breakpoint
> delete_raw_breakpoint
> uninsert_raw_breakpoint
> reinsert_raw_breakpoint
> find_raw_breakpoint_at

Minimizing the patch size is less important than making sure the
resulting code is clear

Sounds like that's manageable with a trivial replace of bp->size
with a call to something like:

static int
bp_size (struct raw_breakpoint *bp)
{
   int size = bp->kind;

   breakpoint_from_kind (&size);
   return size;
}

Likewise for the opcode data:

static const gdb_byte *
bp_opcode (struct raw_breakpoint *bp)
{
   int size = bp->kind;

   return breakpoint_from_kind (&size);
}

Doesn't seem to me like the end result would be any less clear.

Thanks,
Pedro Alves

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

[Antoine Tremblay]

On 10/16/2015 12:06 PM, Pedro Alves wrote:
> GDB calls them "placed address" and "requested address":
>
> struct bp_target_info
> {
> ...
>    /* Address at which the breakpoint was placed.  This is normally
>       the same as REQUESTED_ADDRESS, except when adjustment happens in
>       gdbarch_breakpoint_from_pc.  The most common form of adjustment
>       is stripping an alternate ISA marker from the PC which is used
>       to determine the type of breakpoint to insert.  */
>    CORE_ADDR placed_address;
>
>    /* Address at which the breakpoint was requested.  */
>    CORE_ADDR reqstd_address;
>
>

Nice I had not seen that !

I'll change pcfull for reqstd_address;

But leave pc as pc for now, we can change it in a later patch.

> I don't see why my comment conflicts with Yao's.  But I think we
> could simplify the interfaces and entry points, and get rid of the
> duplication, like this:
>
> Replace the breakpoint_from_pc method with a breakpoint_kind_from_pc
> method.  This adjusts the PC (if necessary) and returns the
> breakpoint _kind_ instead of the breakpoint opcode / data.
>
> enum arm_breakpoint_kinds
> {
>     ARM_BP_KIND_THUMB = 2,
>     ARM_BP_KIND_THUMB2 = 3,
>     ARM_BP_KIND_ARM = 4,
> };
>
> static int
> arm_breakpoint_kind_from_pc (CORE_ADDR *pcptr, int len)
> {
>    if (IS_THUMB_ADDR (*pcptr))
>      {
>        gdb_byte buf[2];
>
>        *pcptr = UNMAKE_THUMB_ADDR (*pcptr);
>
>        /* Check whether we are replacing a thumb2 32-bit instruction.  */
>        if ((*the_target->read_memory) (*pcptr, buf, 2) == 0)
> 	{
> 	  unsigned short inst1 = 0;
>
> 	  (*the_target->read_memory) (*pcptr, (gdb_byte *) &inst1, 2);
> 	  if (thumb_insn_size (inst1) == 4)
>              return ARM_BP_KIND_THUMB2;
> 	}
>
>        return ARM_BP_KIND_THUMB;
>      }
>    else
>      return ARM_BP_KIND_ARM;
> }
>
> Then the breakpoints functions and structures always work
> with the already-adjusted PC, and with a breakpoint-kind.
>
> for internal breakpoints, we have:
>
>    set_breakpoint_at (breakpoint_kind_from_pc, to find bp kind,
>                       rest the same as today)
>    set_gdb_breakpoint_1 (same as today)
>       |
>       `--> set_breakpoint (address, kind)
>               |
>               `-->set_raw_breakpoint_at (address, kind)
>                      |
>                      `--> the_target->insert_point (address, kind)
>
> Everything thinks in terms of breakpoint kind.  Then the only
> places that need to know the real breakpoint instruction opcode
> and opcode size can query the breakpoint_from_kind target method
> you already added.
>
> About:
>
>> We could but then we would have to call breakpoint_from_kind in a lot of
>> places basically everywhere bp->size is referenced like :
>>
>> check_mem_read
>> check_mem_write
>> insert_memory_breakpoint
>> remove_memory_breakpoint
>> set_raw_breakpoint_at
>> validate_inserted_breakpoint
>> delete_raw_breakpoint
>> uninsert_raw_breakpoint
>> reinsert_raw_breakpoint
>> find_raw_breakpoint_at
>
> Minimizing the patch size is less important than making sure the
> resulting code is clear
>

I was thinking about making the code clear.

> Sounds like that's manageable with a trivial replace of bp->size
> with a call to something like:
>
> static int
> bp_size (struct raw_breakpoint *bp)
> {
>     int size = bp->kind;
>
>     breakpoint_from_kind (&size);
>     return size;
> }
>
> Likewise for the opcode data:
>
> static const gdb_byte *
> bp_opcode (struct raw_breakpoint *bp)
> {
>     int size = bp->kind;
>
>     return breakpoint_from_kind (&size);
> }
>
> Doesn't seem to me like the end result would be any less clear.
>

I see what you mean more clearly now.

I like the idea to treat only in kinds but I'm not sure about the 
advantage in terms of clarity.

I would say it's debatable like you said in the end result is not any 
less clear but the current implementation doesn't seem less clear to me 
either.

I do not like the detour we need to make when we do not have a real 
reason to have a kind, adding a "fake" unique kind and having to add 
breakpoint_from_kind implementations on all architectures, not just the 
ones that support software breakpoints. (Regardless of the patch size).

Also even if we can call functions to get the size and opcode when 
needed, it still seems like since those values do not change for the 
life of the breakpoint and that they can be set only once from a 
meaningful context it's perfectly acceptable and more clear to set them 
once and avoid this level of abstraction to access these values.

At this point I could do either but to avoid redoing the patch set 
multiple times, I would like to ask for Yao's opinion and I will work on 
the consensual option.

Regards,
Antoine


	

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

[Pedro Alves]

On 10/16/2015 07:08 PM, Antoine Tremblay wrote:

> I see what you mean more clearly now.
> 
> I like the idea to treat only in kinds but I'm not sure about the 
> advantage in terms of clarity.

The main advantage is that it eliminates the redundant info
in the breakpoint structures fields.  I really see no reason for every
breakpoint to have to carry around a pointer to the opcode to use,
since that's a property that can be inferred from other fields.

Less redundancy means fewer chances of things getting out of sync.

> 
> I would say it's debatable like you said in the end result is not any 
> less clear but the current implementation doesn't seem less clear to me 
> either.
> 
> I do not like the detour we need to make when we do not have a real 
> reason to have a kind, adding a "fake" unique kind and having to add 
> breakpoint_from_kind implementations on all architectures, not just the 
> ones that support software breakpoints. (Regardless of the patch size).
> 

The default implementation of breakpoint_from_pc can simply be:

int
default_breakpoint_kind_from_pc (CORE_ADDR *pcptr)
{
  /* Most architectures have only one kind of software breakpoint.  */
  return 0;
}

and then most architectures would just implement the
breakpoint_from_kind method simply as:

static gdb_byte *
arch_breakpoint_from_kind (int kind /* ignored, there's only one */)
{
  return arch_breakpoint;
}

which is quite like what you already wrote.

That is, most architectures only have one 'kind' of breakpoint,
so they can ignore the parameter.  Just like in your breakpoint_from_pc
hook, most archs would ignore the length.

Note that there's always a need to implement _one_ hook on all
architecture.  In your version, it's the breakpoint_from_pc hook.
In my suggestion, it's breakpoint_from_kind.  But it's the same
number of "hooks x architectures implementations".

> Also even if we can call functions to get the size and opcode when 
> needed, it still seems like since those values do not change for the 
> life of the breakpoint and that they can be set only once from a 
> meaningful context it's perfectly acceptable and more clear to set them 
> once and avoid this level of abstraction to access these values.

I don't buy that.  The opcode to use for a particular mode can be
determined from that info alone ("which mode"), and as such doesn't
need to be stored on every breakpoint.  If you look at it from the
other angle, you can see it as factorization.

> At this point I could do either but to avoid redoing the patch set 
> multiple times, I would like to ask for Yao's opinion and I will work on 
> the consensual option.

Fair enough.

Thanks,
Pedro Alves

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

[Antoine Tremblay]

On 10/16/2015 03:04 PM, Pedro Alves wrote:
>
> Note that there's always a need to implement _one_ hook on all
> architecture.  In your version, it's the breakpoint_from_pc hook.
> In my suggestion, it's breakpoint_from_kind.  But it's the same
> number of "hooks x architectures implementations".

Good point it would only transfer the operation to archs that can 
software single step basically, I withdraw this concern.

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

[Antoine Tremblay]

On 10/16/2015 03:23 PM, Antoine Tremblay wrote:
>
>
> On 10/16/2015 03:04 PM, Pedro Alves wrote:
>>
>> Note that there's always a need to implement _one_ hook on all
>> architecture.  In your version, it's the breakpoint_from_pc hook.
>> In my suggestion, it's breakpoint_from_kind.  But it's the same
>> number of "hooks x architectures implementations".
>
> Good point it would only transfer the operation to archs that can
> software single step basically, I withdraw this concern.

Humm thinking more about it however if we were to apply the same logic 
to pc and pcfull.

Removing the pc from the struct would cause a call to 
breakpoint_kind_from_pc to be mandatory.

Would you see too pc to be removed ?

And replaced by a call to :

static CORE_ADDR
bp_pc (CORE_ADDR *pcptr)
{
   return breakpoint_kind_from_pc (pcptr, 0)
}

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

[Antoine Tremblay]

On 10/16/2015 03:44 PM, Antoine Tremblay wrote:
>
>
> On 10/16/2015 03:23 PM, Antoine Tremblay wrote:
>>
>>
>> On 10/16/2015 03:04 PM, Pedro Alves wrote:
>>>
>>> Note that there's always a need to implement _one_ hook on all
>>> architecture.  In your version, it's the breakpoint_from_pc hook.
>>> In my suggestion, it's breakpoint_from_kind.  But it's the same
>>> number of "hooks x architectures implementations".
>>
>> Good point it would only transfer the operation to archs that can
>> software single step basically, I withdraw this concern.
>
> Humm thinking more about it however if we were to apply the same logic
> to pc and pcfull.
>
> Removing the pc from the struct would cause a call to
> breakpoint_kind_from_pc to be mandatory.
>
> Would you see too pc to be removed ?
>
> And replaced by a call to :
>
> static CORE_ADDR
> bp_pc (CORE_ADDR *pcptr)
> {
>    return breakpoint_kind_from_pc (pcptr, 0)
> }
>
Oops I mean

static CORE_ADDR
bp_pc (struct raw_breakpoint *bp)
{
CORE_ADDR pc = bp->pcfull;
breakpoint_kind_from_pc (&pc, 0)
return pc;
}

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

[Pedro Alves]

On 10/16/2015 08:44 PM, Antoine Tremblay wrote:

> Humm thinking more about it however if we were to apply the same logic 
> to pc and pcfull.
> 
> Removing the pc from the struct would cause a call to 
> breakpoint_kind_from_pc to be mandatory.
> 
> Would you see too pc to be removed ?
> 

I was seeing pcfull being removed, actually.

Z0 breakpoints always have their address already adjusted by GDB, right?

You had:

@@ -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);
 }


But I think you should be able to instead do:

@@ -774,9 +802,15 @@ set_breakpoint_at (CORE_ADDR where, int (*handler) (CORE_ADDR))
 {
   int err_ignored;

+  CORE_ADDR adjusted_pc = where;
+  int bp_kind;
+
+  bp_kind = the_target->breakpoint_kind_from_pc (&adjusted_pc);
+
   return set_breakpoint (other_breakpoint, raw_bkpt_type_sw,
-			 where, breakpoint_len, handler,
+			 adjusted_pc, bp_kind, handler,
			 &err_ignored);
 }

Thanks,
Pedro Alves

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

[Antoine Tremblay]

On 10/19/2015 05:35 AM, Pedro Alves wrote:

> I was seeing pcfull being removed, actually.

Yes good point, in fact, I don't even need it in the current implementation.

Your solution has grown on me over the weekend, so I will post a v3 with 
this implemented unless Yao has an objection.

[PATCH v2 3/7] Implement breakpoint_from_kind for supported architectures in GDBServer.

[Antoine Tremblay]

This patch implements the breakpoint_from_kind linux_target_ops for
architectures that support software breakpoints, namely : x86 and aarch64.

No regressions, tested on Ubuntu 14.04 x86.
With gdbserver-{native,extended}.

Compilation was also tested on aarch64.
---
 gdb/gdbserver/linux-aarch64-low.c | 9 +++++++++
 gdb/gdbserver/linux-x86-low.c     | 9 +++++++++
 2 files changed, 18 insertions(+)

diff --git a/gdb/gdbserver/linux-aarch64-low.c b/gdb/gdbserver/linux-aarch64-low.c
index 1e7a0ff..46b79a5 100644
--- a/gdb/gdbserver/linux-aarch64-low.c
+++ b/gdb/gdbserver/linux-aarch64-low.c
@@ -3238,6 +3238,14 @@ aarch64_supports_range_stepping (void)
   return 1;
 }
 
+/* Implementation of linux_target_ops method "breakpoint_from_kind".  */
+
+static const unsigned char *
+aarch64_breakpoint_from_kind (int *kind)
+{
+    return (const unsigned char *) &aarch64_breakpoint;
+}
+
 struct linux_target_ops the_low_target =
 {
   aarch64_arch_setup,
@@ -3270,6 +3278,7 @@ struct linux_target_ops the_low_target =
   aarch64_emit_ops,
   aarch64_get_min_fast_tracepoint_insn_len,
   aarch64_supports_range_stepping,
+  aarch64_breakpoint_from_kind,
 };
 
 void
diff --git a/gdb/gdbserver/linux-x86-low.c b/gdb/gdbserver/linux-x86-low.c
index 41803c4..e64b498 100644
--- a/gdb/gdbserver/linux-x86-low.c
+++ b/gdb/gdbserver/linux-x86-low.c
@@ -3258,6 +3258,14 @@ x86_supports_range_stepping (void)
   return 1;
 }
 
+/* Implementation of linux_target_ops method "breakpoint_from_kind".  */
+
+static const unsigned char *
+x86_breakpoint_from_kind (int *kind)
+{
+  return x86_breakpoint;
+}
+
 /* This is initialized assuming an amd64 target.
    x86_arch_setup will correct it for i386 or amd64 targets.  */
 
@@ -3297,6 +3305,7 @@ struct linux_target_ops the_low_target =
   x86_emit_ops,
   x86_get_min_fast_tracepoint_insn_len,
   x86_supports_range_stepping,
+  x86_breakpoint_from_kind,
 };
 
 void
-- 
1.9.1

Re: [PATCH v2 3/7] Implement breakpoint_from_kind for supported architectures in GDBServer.

[Yao Qi]

Antoine Tremblay <antoine.tremblay@ericsson.com> writes:

There is no changelog entry.

> +/* Implementation of linux_target_ops method "breakpoint_from_kind".  */
> +
> +static const unsigned char *
> +aarch64_breakpoint_from_kind (int *kind)
> +{
> +    return (const unsigned char *) &aarch64_breakpoint;

Indentation looks odd, and do we really need the cast?

Note that this function is correct because we restrict the usage of Z0
packet.  Z0 packet is only used with non-extended protocol and inferior
is 64bit.  See aarch64_supports_z_point_type.  Once we remove the
restriction, we need to update this function to return different
breakpoint instructions (aarch64, arm, thumb, and thumb2) according to
*KIND and other information.

Otherwise, patch is OK to me.

-- 
Yao (齐尧)

Re: [PATCH v2 3/7] Implement breakpoint_from_kind for supported architectures in GDBServer.

[Antoine Tremblay]

On 10/15/2015 05:19 AM, Yao Qi wrote:
> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
> There is no changelog entry.
>
Wow sorry adding it.

>> +/* Implementation of linux_target_ops method "breakpoint_from_kind".  */
>> +
>> +static const unsigned char *
>> +aarch64_breakpoint_from_kind (int *kind)
>> +{
>> +    return (const unsigned char *) &aarch64_breakpoint;
>
> Indentation looks odd, and do we really need the cast?
>

Fixed.

> Note that this function is correct because we restrict the usage of Z0
> packet.  Z0 packet is only used with non-extended protocol and inferior
> is 64bit.  See aarch64_supports_z_point_type.  Once we remove the
> restriction, we need to update this function to return different
> breakpoint instructions (aarch64, arm, thumb, and thumb2) according to
> *KIND and other information.

Yes indeed.

>
> Otherwise, patch is OK to me.
>

Re: [PATCH v2 3/7] Implement breakpoint_from_kind for supported architectures in GDBServer.

[Antoine Tremblay]

On 10/15/2015 06:57 AM, Antoine Tremblay wrote:
>
>
> On 10/15/2015 05:19 AM, Yao Qi wrote:
>> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>>
>> There is no changelog entry.
>>
> Wow sorry adding it.
>
>>> +/* Implementation of linux_target_ops method
>>> "breakpoint_from_kind".  */
>>> +
>>> +static const unsigned char *
>>> +aarch64_breakpoint_from_kind (int *kind)
>>> +{
>>> +    return (const unsigned char *) &aarch64_breakpoint;
>>
>> Indentation looks odd, and do we really need the cast?
>>
>
> Fixed.

Note this was changed to gdb_byte the cast is removed and 
aarch64_breakpoint is returned not &aarch64_breakpoint...

>
>> Note that this function is correct because we restrict the usage of Z0
>> packet.  Z0 packet is only used with non-extended protocol and inferior
>> is 64bit.  See aarch64_supports_z_point_type.  Once we remove the
>> restriction, we need to update this function to return different
>> breakpoint instructions (aarch64, arm, thumb, and thumb2) according to
>> *KIND and other information.
>
> Yes indeed.
>
>>
>> Otherwise, patch is OK to me.
>>

[PATCH v2 2/7] Add breakpoint_from_kind target_ops for software breakpoints in GDBServer.

[Antoine Tremblay]

This patch is in preparation for software breakpoints on ARM in
GDBServer.

This patch introduces a new target_ops and linux_target_ops called
breakpoint_from_kind that will be used to ask the target for the right
breakpoint for a kind as set in a Z0 packet.

No regressions, tested on ubuntu 14.04 ARMv7 and x86.
With gdbserver-{native,extended} / { -marm -mthumb }

gdb/gdbserver/ChangeLog:
	* linux-low.c (linux_breakpoint_from_kind): New function.
	(struct target_ops) <breakpoint_from_kind>: Initialize field.
	* linux-low.h (struct linux_target_ops)
	<breakpoint_from_kind>: New field.
	* target.h (struct target_ops) <breakpoint_from_kind>: New field.
---
 gdb/gdbserver/linux-low.c | 10 ++++++++++
 gdb/gdbserver/linux-low.h |  4 ++++
 gdb/gdbserver/target.h    |  4 ++++
 3 files changed, 18 insertions(+)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index dc16fe0..5aa2b1d 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -5965,6 +5965,15 @@ linux_supports_range_stepping (void)
   return (*the_low_target.supports_range_stepping) ();
 }
 
+static const unsigned char*
+linux_breakpoint_from_kind (int *kind)
+{
+  if (*the_low_target.breakpoint_from_kind != NULL)
+    return (*the_low_target.breakpoint_from_kind) (kind);
+  else
+    return NULL;
+}
+
 /* Enumerate spufs IDs for process PID.  */
 static int
 spu_enumerate_spu_ids (long pid, unsigned char *buf, CORE_ADDR offset, int len)
@@ -7040,6 +7049,7 @@ static struct target_ops linux_target_ops = {
   linux_mntns_unlink,
   linux_mntns_readlink,
   linux_breakpoint_from_pc,
+  linux_breakpoint_from_kind
 };
 
 static void
diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
index a9964ac..cc19b88 100644
--- a/gdb/gdbserver/linux-low.h
+++ b/gdb/gdbserver/linux-low.h
@@ -227,6 +227,10 @@ struct linux_target_ops
 
   /* Returns true if the low target supports range stepping.  */
   int (*supports_range_stepping) (void);
+
+  /* Returns the proper breakpoint from size, the kind can have target
+     specific meaning like the z0 kind parameter */
+  const unsigned char *(*breakpoint_from_kind) (int *kind);
 };
 
 extern struct linux_target_ops the_low_target;
diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
index 603819e..74fb36d 100644
--- a/gdb/gdbserver/target.h
+++ b/gdb/gdbserver/target.h
@@ -446,6 +446,10 @@ struct target_ops
      can be NULL, the default breakpoint for the target should be returned in
      this case.  */
   const unsigned char *(*breakpoint_from_pc) (CORE_ADDR *pcptr, int *lenptr);
+
+  /* Returns a breakpoint from a kind, a kind is a length can have target
+     specific meaning like the z0 kind parameter.  */
+  const unsigned char *(*breakpoint_from_kind) (int *kind);
 };
 
 extern struct target_ops *the_target;
-- 
1.9.1

Re: [PATCH v2 2/7] Add breakpoint_from_kind target_ops for software breakpoints in GDBServer.

[Yao Qi]

Antoine Tremblay <antoine.tremblay@ericsson.com> writes:

> diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
> index 603819e..74fb36d 100644
> --- a/gdb/gdbserver/target.h
> +++ b/gdb/gdbserver/target.h
> @@ -446,6 +446,10 @@ struct target_ops
>       can be NULL, the default breakpoint for the target should be returned in
>       this case.  */
>    const unsigned char *(*breakpoint_from_pc) (CORE_ADDR *pcptr, int *lenptr);
> +
> +  /* Returns a breakpoint from a kind, a kind is a length can have target
> +     specific meaning like the z0 kind parameter.  */
> +  const unsigned char *(*breakpoint_from_kind) (int *kind);
>  };

Since this function is used for software breakpoint, how about renaming
it to software_breakpoint_from_kind?  Sorry that I didn't raise this up
in the review lats time.

Otherwise it is OK.

-- 
Yao (齐尧)

Re: [PATCH v2 2/7] Add breakpoint_from_kind target_ops for software breakpoints in GDBServer.

[Antoine Tremblay]

On 10/15/2015 05:04 AM, Yao Qi wrote:
> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>> diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
>> index 603819e..74fb36d 100644
>> --- a/gdb/gdbserver/target.h
>> +++ b/gdb/gdbserver/target.h
>> @@ -446,6 +446,10 @@ struct target_ops
>>        can be NULL, the default breakpoint for the target should be returned in
>>        this case.  */
>>     const unsigned char *(*breakpoint_from_pc) (CORE_ADDR *pcptr, int *lenptr);
>> +
>> +  /* Returns a breakpoint from a kind, a kind is a length can have target
>> +     specific meaning like the z0 kind parameter.  */
>> +  const unsigned char *(*breakpoint_from_kind) (int *kind);
>>   };
>
> Since this function is used for software breakpoint, how about renaming
> it to software_breakpoint_from_kind?  Sorry that I didn't raise this up
> in the review lats time.
>
Humm I would be for it for it if it did not translate in long names like 
linux_software_breakpoint_from_kind (36 chars almost half a line) and I 
can't see any confusion with hardware breakpoints since we return a byte 
array.

I would also need to change breakpoint_from_pc from the previous patch 
to software_breakpoint_from_pc ?

Also, the old names were too "breakpoint and breakpoint_len rather then 
sofwware_breakpoint software_breakpoint_len"...

Overall I feel while it adds some meaning it removes no confusion in the 
context it is used.

Would it be ok to leave it as is ?

> Otherwise it is OK.
>

Re: [PATCH v2 2/7] Add breakpoint_from_kind target_ops for software breakpoints in GDBServer.

[Yao Qi]

Antoine Tremblay <antoine.tremblay@ericsson.com> writes:

> +static const unsigned char*
> +linux_breakpoint_from_kind (int *kind)
> +{
> +  if (*the_low_target.breakpoint_from_kind != NULL)
> +    return (*the_low_target.breakpoint_from_kind) (kind);
> +  else
> +    return NULL;
> +}

What does returned NULL mean?  We need to assert
the_low_target.breakpoint_from_kind isn't NULL, as a sanity check?

-- 
Yao (齐尧)

Re: [PATCH v2 2/7] Add breakpoint_from_kind target_ops for software breakpoints in GDBServer.

[Antoine Tremblay]

On 10/15/2015 05:09 AM, Yao Qi wrote:
> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>> +static const unsigned char*
>> +linux_breakpoint_from_kind (int *kind)
>> +{
>> +  if (*the_low_target.breakpoint_from_kind != NULL)
>> +    return (*the_low_target.breakpoint_from_kind) (kind);
>> +  else
>> +    return NULL;
>> +}
>
> What does returned NULL mean?  We need to assert
> the_low_target.breakpoint_from_kind isn't NULL, as a sanity check?
>

Yes that's a left over from previous implementations, it should assert 
now as breakpoint_from_kind is mandatory.

I'll fix it.

Re: [PATCH v2 2/7] Add breakpoint_from_kind target_ops for software breakpoints in GDBServer.

[Pedro Alves]

On 10/05/2015 05:44 PM, Antoine Tremblay wrote:
> +static const unsigned char*
> +linux_breakpoint_from_kind (int *kind)
> +{

gdb_byte?

Thanks,
Pedro Alves

Re: [PATCH v2 2/7] Add breakpoint_from_kind target_ops for software breakpoints in GDBServer.

[Antoine Tremblay]

On 10/15/2015 11:33 AM, Pedro Alves wrote:
> On 10/05/2015 05:44 PM, Antoine Tremblay wrote:
>> +static const unsigned char*
>> +linux_breakpoint_from_kind (int *kind)
>> +{
>
> gdb_byte?
>
Done.

[PATCH v2 5/7] Implement breakpoint_from_pc for ARM in GDBServer.

[Antoine Tremblay]

In this v2:

 - ChangeLog fixes.
 - Copyright notices fixes.
 - Makefiles / configure.tgt now include arm.o at the proper places.
 - Indentation fixes.
 - common/arm-common.c/h moved to arch/arm.c/h
 - Refactor the breakpoints in another patch.
 - Modified commit log.

---
Implement breakpoint_from_pc for ARM in GDBServer.

This patch is in preparation for software breakpoints on ARM in
GDBServer.

ARM can have multiple breakpoint types based on the instruction set
it's currently in: arm, thumb or thumb2.

GDBServer needs to know what breakpoint is to be inserted at location
when inserting a breakpoint.

This is handled by the breakpoint_from_pc target ops introduced in a
previous patch, this patch adds the arm_breakpoint_from_pc
implementation so that the proper breakpoint type is returned based on
the current pc.

Also in order to share some code with GDB a new file called
arm.c have been introduced in arch/.

While this file does not contain much yet future patches will add more
to it thus the inclusion at this stage.

No regressions on Ubuntu 14.04 on ARMv7 and x86.
With gdbserver-{native,extended} / { -marm -mthumb }

gdb/ChangeLog:
	* Makefile.in: Add arm.o.
	* arch/arm.c: New file.
	* arch/arm.h: (IS_THUMB_ADDR): Move macro from arm-tdep.c.
	(MAKE_THUMB_ADDR): Likewise.
	(UNMAKE_THUMB_ADDR): Likewise.
	* arm-tdep.c (int thumb_insn_size): Move to arm.c.
	(IS_THUMB_ADDR): Remove.
	(MAKE_THUMB_ADDR): Remove.
	(UNMAKE_THUMB_ADDR): Remove.
	* configure.tgt (aarch64*-*-elf)): Add arm.o to all arm	configs.

gdb/gdbserver/ChangeLog:
	* Makefile.in: Add arm.o.
	* configure.srv: Likewise.
	(arm_breakpoint_from_pc): Return the proper breakpoint types.
---
 gdb/Makefile.in               | 13 ++++++++-
 gdb/arch/arm.c                | 32 ++++++++++++++++++++++
 gdb/arch/arm.h                | 12 ++++++++-
 gdb/arm-tdep.c                | 21 +--------------
 gdb/configure.tgt             | 16 +++++------
 gdb/gdbserver/Makefile.in     |  9 ++++++-
 gdb/gdbserver/configure.srv   |  1 +
 gdb/gdbserver/linux-arm-low.c | 63 ++++++++++++++++++++++++++++++++-----------
 8 files changed, 121 insertions(+), 46 deletions(-)
 create mode 100644 gdb/arch/arm.c

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index d5ca2ee..14ad405 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -657,7 +657,7 @@ ALL_64_TARGET_OBS = \
 
 # All other target-dependent objects files (used with --enable-targets=all).
 ALL_TARGET_OBS = \
-	armbsd-tdep.o arm-linux-tdep.o arm-symbian-tdep.o \
+	armbsd-tdep.o arm.o arm-linux-tdep.o arm-symbian-tdep.o \
 	armnbsd-tdep.o armobsd-tdep.o \
 	arm-tdep.o arm-wince-tdep.o \
 	avr-tdep.o \
@@ -1660,6 +1660,7 @@ ALLDEPFILES = \
 	amd64-dicos-tdep.c \
 	amd64-linux-nat.c amd64-linux-tdep.c \
 	amd64-sol2-tdep.c \
+	arm.c \
 	arm-linux-nat.c arm-linux-tdep.c arm-symbian-tdep.c arm-tdep.c \
 	armnbsd-nat.c armbsd-tdep.c armnbsd-tdep.c armobsd-tdep.c \
 	avr-tdep.c \
@@ -2275,6 +2276,16 @@ waitstatus.o: ${srcdir}/target/waitstatus.c
 	$(COMPILE) $(srcdir)/target/waitstatus.c
 	$(POSTCOMPILE)
 
+#
+# gdb/arch/ dependencies
+#
+# Need to explicitly specify the compile rule as make will do nothing
+# or try to compile the object file into the sub-directory.
+
+arm.o: ${srcdir}/arch/arm.c
+	$(COMPILE) $(srcdir)/arch/arm.c
+	$(POSTCOMPILE)
+
 # gdb/nat/ dependencies
 #
 # Need to explicitly specify the compile rule as make will do nothing
diff --git a/gdb/arch/arm.c b/gdb/arch/arm.c
new file mode 100644
index 0000000..a83175a
--- /dev/null
+++ b/gdb/arch/arm.c
@@ -0,0 +1,32 @@
+/* Common target dependent code for GDB on ARM systems.
+
+   Copyright (C) 1988-2015 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "arm.h"
+
+/* Return the size in bytes of the complete Thumb instruction whose
+   first halfword is INST1.  */
+
+int
+thumb_insn_size (unsigned short inst1)
+{
+  if ((inst1 & 0xe000) == 0xe000 && (inst1 & 0x1800) != 0)
+    return 4;
+  else
+    return 2;
+}
diff --git a/gdb/arch/arm.h b/gdb/arch/arm.h
index e0eed60..a054776 100644
--- a/gdb/arch/arm.h
+++ b/gdb/arch/arm.h
@@ -1,5 +1,5 @@
 /* Common target dependent code for GDB on ARM systems.
-   Copyright (C) 2002-2015 Free Software Foundation, Inc.
+   Copyright (C) 1988-2015 Free Software Foundation, Inc.
 
    This file is part of GDB.
 
@@ -58,4 +58,14 @@ enum gdb_regnum {
   ARM_LAST_FP_ARG_REGNUM = ARM_F3_REGNUM
 };
 
+/* Addresses for calling Thumb functions have the bit 0 set.
+   Here are some macros to test, set, or clear bit 0 of addresses.  */
+#define IS_THUMB_ADDR(addr)    ((addr) & 1)
+#define MAKE_THUMB_ADDR(addr)  ((addr) | 1)
+#define UNMAKE_THUMB_ADDR(addr) ((addr) & ~1)
+
+/* Return the size in bytes of the complete Thumb instruction whose
+   first halfword is INST1.  */
+int thumb_insn_size (unsigned short inst1);
+
 #endif
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 4c99ddf..aad2ce6 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -45,6 +45,7 @@
 #include "user-regs.h"
 #include "observer.h"
 
+#include "arch/arm.h"
 #include "arm-tdep.h"
 #include "gdb/sim-arm.h"
 
@@ -235,8 +236,6 @@ static void arm_neon_quad_write (struct gdbarch *gdbarch,
 				 struct regcache *regcache,
 				 int regnum, const gdb_byte *buf);
 
-static int thumb_insn_size (unsigned short inst1);
-
 struct arm_prologue_cache
 {
   /* The stack pointer at the time this frame was created; i.e. the
@@ -267,12 +266,6 @@ static CORE_ADDR arm_analyze_prologue (struct gdbarch *gdbarch,
 
 #define DISPLACED_STEPPING_ARCH_VERSION		5
 
-/* Addresses for calling Thumb functions have the bit 0 set.
-   Here are some macros to test, set, or clear bit 0 of addresses.  */
-#define IS_THUMB_ADDR(addr)	((addr) & 1)
-#define MAKE_THUMB_ADDR(addr)	((addr) | 1)
-#define UNMAKE_THUMB_ADDR(addr) ((addr) & ~1)
-
 /* Set to true if the 32-bit mode is in use.  */
 
 int arm_apcs_32 = 1;
@@ -4364,18 +4357,6 @@ bitcount (unsigned long val)
   return nbits;
 }
 
-/* Return the size in bytes of the complete Thumb instruction whose
-   first halfword is INST1.  */
-
-static int
-thumb_insn_size (unsigned short inst1)
-{
-  if ((inst1 & 0xe000) == 0xe000 && (inst1 & 0x1800) != 0)
-    return 4;
-  else
-    return 2;
-}
-
 static int
 thumb_advance_itstate (unsigned int itstate)
 {
diff --git a/gdb/configure.tgt b/gdb/configure.tgt
index 33d4cfc..2e824ad 100644
--- a/gdb/configure.tgt
+++ b/gdb/configure.tgt
@@ -44,7 +44,7 @@ aarch64*-*-elf)
 aarch64*-*-linux*)
 	# Target: AArch64 linux
 	gdb_target_obs="aarch64-tdep.o aarch64-linux-tdep.o aarch64-insn.o \
-			arm-tdep.o arm-linux-tdep.o \
+			arm.o arm-tdep.o arm-linux-tdep.o \
 			glibc-tdep.o linux-tdep.o solib-svr4.o \
 			symfile-mem.o linux-record.o"
 	build_gdbserver=yes
@@ -84,31 +84,31 @@ am33_2.0*-*-linux*)
 
 arm*-wince-pe | arm*-*-mingw32ce*)
 	# Target: ARM based machine running Windows CE (win32)
-	gdb_target_obs="arm-tdep.o arm-wince-tdep.o windows-tdep.o"
+	gdb_target_obs="arm.o arm-tdep.o arm-wince-tdep.o windows-tdep.o"
 	build_gdbserver=yes
 	;;
 arm*-*-linux*)
 	# Target: ARM based machine running GNU/Linux
-	gdb_target_obs="arm-tdep.o arm-linux-tdep.o glibc-tdep.o \
+	gdb_target_obs="arm.o arm-tdep.o arm-linux-tdep.o glibc-tdep.o \
 			solib-svr4.o symfile-mem.o linux-tdep.o linux-record.o"
 	build_gdbserver=yes
 	;;
 arm*-*-netbsd* | arm*-*-knetbsd*-gnu)
 	# Target: NetBSD/arm
-	gdb_target_obs="arm-tdep.o armnbsd-tdep.o solib-svr4.o"
+	gdb_target_obs="arm.o arm-tdep.o armnbsd-tdep.o solib-svr4.o"
 	;;
 arm*-*-openbsd*)
 	# Target: OpenBSD/arm
-	gdb_target_obs="arm-tdep.o armbsd-tdep.o armobsd-tdep.o obsd-tdep.o \
-			solib-svr4.o"
+	gdb_target_obs="arm.o arm-tdep.o armbsd-tdep.o armobsd-tdep.o \
+			obsd-tdep.o solib-svr4.o"
 	;;
 arm*-*-symbianelf*)
 	# Target: SymbianOS/arm
-	gdb_target_obs="arm-tdep.o arm-symbian-tdep.o"
+	gdb_target_obs="arm.o arm-tdep.o arm-symbian-tdep.o"
 	;;
 arm*-*-*)
 	# Target: ARM embedded system
-	gdb_target_obs="arm-tdep.o"
+	gdb_target_obs="arm.o arm-tdep.o"
 	gdb_sim=../sim/arm/libsim.a
 	;;
 
diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
index cd146f4..97e1e62 100644
--- a/gdb/gdbserver/Makefile.in
+++ b/gdb/gdbserver/Makefile.in
@@ -180,7 +180,8 @@ SFILES=	$(srcdir)/gdbreplay.c $(srcdir)/inferiors.c $(srcdir)/dll.c \
 	$(srcdir)/common/common-debug.c $(srcdir)/common/cleanups.c \
 	$(srcdir)/common/common-exceptions.c $(srcdir)/symbol.c \
 	$(srcdir)/common/btrace-common.c \
-	$(srcdir)/common/fileio.c $(srcdir)/nat/linux-namespaces.c
+	$(srcdir)/common/fileio.c $(srcdir)/nat/linux-namespaces.c \
+	$(srcdir)/arch/arm.c
 
 DEPFILES = @GDBSERVER_DEPFILES@
 
@@ -583,6 +584,12 @@ fileio.o: ../common/fileio.c
 	$(COMPILE) $<
 	$(POSTCOMPILE)
 
+# Arch object files rules form ../arch
+
+arm.o: ../arch/arm.c
+	$(COMPILE) $<
+	$(POSTCOMPILE)
+
 # Native object files rules from ../nat
 
 x86-dregs.o: ../nat/x86-dregs.c
diff --git a/gdb/gdbserver/configure.srv b/gdb/gdbserver/configure.srv
index f187c9d..e854110 100644
--- a/gdb/gdbserver/configure.srv
+++ b/gdb/gdbserver/configure.srv
@@ -70,6 +70,7 @@ case "${target}" in
 			srv_regobj="${srv_regobj} arm-with-neon.o"
 			srv_tgtobj="$srv_linux_obj linux-arm-low.o"
 			srv_tgtobj="$srv_tgtobj linux-aarch32-low.o"
+			srv_tgtobj="${srv_tgtobj} arm.o"
 			srv_xmlfiles="arm-with-iwmmxt.xml"
 			srv_xmlfiles="${srv_xmlfiles} arm-with-vfpv2.xml"
 			srv_xmlfiles="${srv_xmlfiles} arm-with-vfpv3.xml"
diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
index 367c704..8f420f9 100644
--- a/gdb/gdbserver/linux-arm-low.c
+++ b/gdb/gdbserver/linux-arm-low.c
@@ -30,6 +30,8 @@
 #include "nat/gdb_ptrace.h"
 #include <signal.h>
 
+#include "arch/arm.h"
+
 /* Defined in auto-generated files.  */
 void init_registers_arm (void);
 extern const struct target_desc *tdesc_arm;
@@ -237,7 +239,9 @@ arm_set_pc (struct regcache *regcache, CORE_ADDR pc)
 static const unsigned long arm_breakpoint = 0xef9f0001;
 #define arm_breakpoint_len 4
 static const unsigned short thumb_breakpoint = 0xde01;
+#define thumb_breakpoint_len 2
 static const unsigned short thumb2_breakpoint[] = { 0xf7f0, 0xa000 };
+#define thumb2_breakpoint_len 4
 
 /* For new EABI binaries.  We recognize it regardless of which ABI
    is used for gdbserver, so single threaded debugging should work
@@ -285,6 +289,50 @@ arm_breakpoint_at (CORE_ADDR where)
   return 0;
 }
 
+/* Determine the type and size of breakpoint to insert at PCPTR.  Uses
+   the program counter value to determine whether a 16-bit or 32-bit
+   breakpoint should be used.  It returns a pointer to a string of
+   bytes that encode a breakpoint instruction, stores the length of
+   the string to *lenptr, and adjusts the program counter (if
+   necessary) to point to the actual memory location where the
+   breakpoint should be inserted.  */
+
+static const unsigned char *
+arm_breakpoint_from_pc (CORE_ADDR *pcptr, int *lenptr)
+{
+  if (IS_THUMB_ADDR (*pcptr))
+    {
+      gdb_byte buf[2];
+
+      *pcptr = UNMAKE_THUMB_ADDR (*pcptr);
+
+      /* Check whether we are replacing a thumb2 32-bit instruction.  */
+      if ((*the_target->read_memory) (*pcptr, buf, 2) == 0)
+	{
+	  unsigned short inst1 = 0;
+
+	  (*the_target->read_memory) (*pcptr, (gdb_byte *) &inst1, 2);
+	  if (thumb_insn_size (inst1) == 4)
+	    {
+	      *lenptr = thumb2_breakpoint_len;
+	      return (unsigned char *) &thumb2_breakpoint;
+	    }
+	}
+
+      *lenptr = thumb_breakpoint_len;
+      return (unsigned char *) &thumb_breakpoint;
+    }
+  else
+    {
+      *lenptr = arm_breakpoint_len;
+#ifndef __ARM_EABI__
+      return (const unsigned char *) &arm_breakpoint;
+#else
+      return (const unsigned char *) &arm_eabi_breakpoint;
+#endif
+    }
+}
+
 /* We only place breakpoints in empty marker functions, and thread locking
    is outside of the function.  So rather than importing software single-step,
    we can just run until exit.  */
@@ -913,21 +961,6 @@ arm_regs_info (void)
     return &regs_info_arm;
 }
 
-static const unsigned char *
-arm_breakpoint_from_pc (CORE_ADDR *pcptr, int *len)
-{
-  *len = arm_breakpoint_len;
-   /* Define an ARM-mode breakpoint; we only set breakpoints in the C
-     library, which is most likely to be ARM.  If the kernel supports
-     clone events, we will never insert a breakpoint, so even a Thumb
-     C library will work; so will mixing EABI/non-EABI gdbserver and
-     application.  */
-#ifndef __ARM_EABI__
-  return (const unsigned char *) &arm_breakpoint;
-#else
-  return (const unsigned char *) &arm_eabi_breakpoint;
-#endif
-}
 struct linux_target_ops the_low_target = {
   arm_arch_setup,
   arm_regs_info,
-- 
1.9.1

Re: [PATCH v2 5/7] Implement breakpoint_from_pc for ARM in GDBServer.

[Pedro Alves]

On 10/05/2015 05:44 PM, Antoine Tremblay wrote:

> index 0000000..a83175a
> --- /dev/null
> +++ b/gdb/arch/arm.c
> @@ -0,0 +1,32 @@

> +#include "arm.h"

Should include "common-defs.h" first.

Thanks,
Pedro Alves

Re: [PATCH v2 5/7] Implement breakpoint_from_pc for ARM in GDBServer.

[Antoine Tremblay]

On 10/15/2015 12:07 PM, Pedro Alves wrote:
> On 10/05/2015 05:44 PM, Antoine Tremblay wrote:
>
>> index 0000000..a83175a
>> --- /dev/null
>> +++ b/gdb/arch/arm.c
>> @@ -0,0 +1,32 @@
>
>> +#include "arm.h"
>
> Should include "common-defs.h" first.
>
Done.

[PATCH v2 7/7] Support software breakpoints for ARM linux in GDBServer.

[Antoine Tremblay]

This patch implements the breakpoint_from_kind operation introduced
in a previous patch.

The proper breakpoint can then be returned to be inserted in memory.

It enables software breakpoints via GDB's Z0 packets on ARM.

No regressions, tested on ubuntu 14.04 ARMv7 and x86.
With gdbserver-{native,extended} / { -marm -mthumb }

gdb/ChangeLog:
	* NEWS: Add news for software breakpoints.

gdb/gdbserver/ChangeLog:
	* linux-arm-low.c (arm_breakpoint_from_kind): New function.
	(arm_supports_z_point_type): Add software breakpoint support.
	(struct linux_target_ops) <breakpoint_from_kind>: Initialize field.
---
 gdb/NEWS                      |  2 ++
 gdb/gdbserver/linux-arm-low.c | 31 +++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/gdb/NEWS b/gdb/NEWS
index 2e38d9a..17f1c05 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,8 @@
 
 *** Changes since GDB 7.10
 
+* Support for software breakpoints on ARM linux was added in GDBServer.
+
 * Record btrace now supports non-stop mode.
 
 * Support for tracepoints on aarch64-linux was added in GDBserver.
diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
index d16ea60..bd499f8 100644
--- a/gdb/gdbserver/linux-arm-low.c
+++ b/gdb/gdbserver/linux-arm-low.c
@@ -336,6 +336,28 @@ arm_breakpoint_from_pc (CORE_ADDR *pcptr, int *lenptr)
     }
 }
 
+/* Get the breakpoint from the remote kind
+   2 is thumb-16
+   3 is thumb2-32
+   4 is arm
+*/
+static const unsigned char *
+arm_breakpoint_from_kind (int *kind)
+{
+  switch (*kind) {
+  case 2:
+    return (unsigned char *) &thumb_breakpoint;
+  case 3:
+    *kind = 4;
+    return (unsigned char *) &thumb2_breakpoint;
+  case 4:
+    return (unsigned char *) &arm_breakpoint;
+  default:
+    return NULL;
+  }
+  return NULL;
+}
+
 /* We only place breakpoints in empty marker functions, and thread locking
    is outside of the function.  So rather than importing software single-step,
    we can just run until exit.  */
@@ -577,6 +599,7 @@ arm_supports_z_point_type (char z_type)
 {
   switch (z_type)
     {
+    case Z_PACKET_SW_BP:
     case Z_PACKET_HW_BP:
     case Z_PACKET_WRITE_WP:
     case Z_PACKET_READ_WP:
@@ -988,6 +1011,14 @@ struct linux_target_ops the_low_target = {
   arm_new_thread,
   arm_new_fork,
   arm_prepare_to_resume,
+  NULL, /* process_qsupported */
+  NULL, /* supports_tracepoints */
+  NULL, /* get_thread_area */
+  NULL, /* install_fast_tracepoint_jump_pad */
+  NULL, /* emit_ops */
+  NULL, /* get_min_fast_tracepoint_insn_len */
+  NULL, /* supports_range_stepping */
+  arm_breakpoint_from_kind,
 };
 
 void
-- 
1.9.1

Re: [PATCH v2 7/7] Support software breakpoints for ARM linux in GDBServer.

[Eli Zaretskii]

> From: Antoine Tremblay <antoine.tremblay@ericsson.com>
> CC: Antoine Tremblay <antoine.tremblay@ericsson.com>
> Date: Mon, 5 Oct 2015 12:44:15 -0400
> 
> This patch implements the breakpoint_from_kind operation introduced
> in a previous patch.
> 
> The proper breakpoint can then be returned to be inserted in memory.
> 
> It enables software breakpoints via GDB's Z0 packets on ARM.
> 
> No regressions, tested on ubuntu 14.04 ARMv7 and x86.
> With gdbserver-{native,extended} / { -marm -mthumb }
> 
> gdb/ChangeLog:
> 	* NEWS: Add news for software breakpoints.
> 
> gdb/gdbserver/ChangeLog:
> 	* linux-arm-low.c (arm_breakpoint_from_kind): New function.
> 	(arm_supports_z_point_type): Add software breakpoint support.
> 	(struct linux_target_ops) <breakpoint_from_kind>: Initialize field.

OK for the NEWS part.

Re: [PATCH v2 7/7] Support software breakpoints for ARM linux in GDBServer.

[Pedro Alves]

On 10/05/2015 05:44 PM, Antoine Tremblay wrote:

> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,8 @@
>  
>  *** Changes since GDB 7.10
>  
> +* Support for software breakpoints on ARM linux was added in GDBServer.

Putting a user hat on, what does this mean?  Why is it news worthy?

> +
>  * Record btrace now supports non-stop mode.
>  
>  * Support for tracepoints on aarch64-linux was added in GDBserver.
> diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
> index d16ea60..bd499f8 100644
> --- a/gdb/gdbserver/linux-arm-low.c
> +++ b/gdb/gdbserver/linux-arm-low.c
> @@ -336,6 +336,28 @@ arm_breakpoint_from_pc (CORE_ADDR *pcptr, int *lenptr)
>      }
>  }
>  
> +/* Get the breakpoint from the remote kind
> +   2 is thumb-16
> +   3 is thumb2-32
> +   4 is arm
> +*/
> +static const unsigned char *
> +arm_breakpoint_from_kind (int *kind)
> +{
> +  switch (*kind) {
> +  case 2:

Formatting, break line before {, indent case.

> +    return (unsigned char *) &thumb_breakpoint;
> +  case 3:
> +    *kind = 4;
> +    return (unsigned char *) &thumb2_breakpoint;
> +  case 4:
> +    return (unsigned char *) &arm_breakpoint;
> +  default:
> +    return NULL;
> +  }
> +  return NULL;
> +}
> +

Thanks,
Pedro Alves

Re: [PATCH v2 7/7] Support software breakpoints for ARM linux in GDBServer.

[Antoine Tremblay]

On 10/15/2015 12:07 PM, Pedro Alves wrote:
> On 10/05/2015 05:44 PM, Antoine Tremblay wrote:
>
>> --- a/gdb/NEWS
>> +++ b/gdb/NEWS
>> @@ -3,6 +3,8 @@
>>
>>   *** Changes since GDB 7.10
>>
>> +* Support for software breakpoints on ARM linux was added in GDBServer.
>
> Putting a user hat on, what does this mean?  Why is it news worthy?
>

Humm I had not thought about it this way would saying something like :

* Support for software breakpoints on ARM linux was added in GDBServer. 
Users can now call break to set a breakpoint with GDBServer on ARM linux.

Be better ?

>> +
>>   * Record btrace now supports non-stop mode.
>>
>>   * Support for tracepoints on aarch64-linux was added in GDBserver.
>> diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
>> index d16ea60..bd499f8 100644
>> --- a/gdb/gdbserver/linux-arm-low.c
>> +++ b/gdb/gdbserver/linux-arm-low.c
>> @@ -336,6 +336,28 @@ arm_breakpoint_from_pc (CORE_ADDR *pcptr, int *lenptr)
>>       }
>>   }
>>
>> +/* Get the breakpoint from the remote kind
>> +   2 is thumb-16
>> +   3 is thumb2-32
>> +   4 is arm
>> +*/
>> +static const unsigned char *
>> +arm_breakpoint_from_kind (int *kind)
>> +{
>> +  switch (*kind) {
>> +  case 2:
>
> Formatting, break line before {, indent case.
>
Indeed seems like the switch case indent was not in the Emacs GNU 
default config

Done.

Re: [PATCH v2 7/7] Support software breakpoints for ARM linux in GDBServer.

[Pedro Alves]

On 10/15/2015 07:23 PM, Antoine Tremblay wrote:
> 
> 
> On 10/15/2015 12:07 PM, Pedro Alves wrote:
>> On 10/05/2015 05:44 PM, Antoine Tremblay wrote:
>>
>>> --- a/gdb/NEWS
>>> +++ b/gdb/NEWS
>>> @@ -3,6 +3,8 @@
>>>
>>>   *** Changes since GDB 7.10
>>>
>>> +* Support for software breakpoints on ARM linux was added in GDBServer.
>>
>> Putting a user hat on, what does this mean?  Why is it news worthy?
>>
> 
> Humm I had not thought about it this way would saying something like :
> 
> * Support for software breakpoints on ARM linux was added in GDBServer. 
> Users can now call break to set a breakpoint with GDBServer on ARM linux.

When I read this I'm left confused and wonder what's new, because
it suggests users couldn't already call break and set breakpoints before,
but, they obviously could.

Thanks,
Pedro Alves

Re: [PATCH v2 7/7] Support software breakpoints for ARM linux in GDBServer.

[Antoine Tremblay]

On 10/15/2015 02:33 PM, Pedro Alves wrote:
> On 10/15/2015 07:23 PM, Antoine Tremblay wrote:
>>
>>
>> On 10/15/2015 12:07 PM, Pedro Alves wrote:
>>> On 10/05/2015 05:44 PM, Antoine Tremblay wrote:
>>>
>>>> --- a/gdb/NEWS
>>>> +++ b/gdb/NEWS
>>>> @@ -3,6 +3,8 @@
>>>>
>>>>    *** Changes since GDB 7.10
>>>>
>>>> +* Support for software breakpoints on ARM linux was added in GDBServer.
>>>
>>> Putting a user hat on, what does this mean?  Why is it news worthy?
>>>
>>
>> Humm I had not thought about it this way would saying something like :
>>
>> * Support for software breakpoints on ARM linux was added in GDBServer.
>> Users can now call break to set a breakpoint with GDBServer on ARM linux.
>
> When I read this I'm left confused and wonder what's new, because
> it suggests users couldn't already call break and set breakpoints before,
> but, they obviously could.
>

Right indeed, I can't explain what would this add from a user point of 
view so I will remove the NEWS entry.

Re: [PATCH v2 7/7] Support software breakpoints for ARM linux in GDBServer.

[Yao Qi]

On 15/10/15 17:07, Pedro Alves wrote:
>> +* Support for software breakpoints on ARM linux was added in GDBServer.
> Putting a user hat on, what does this mean?  Why is it news worthy?
>

Supporting software breakpoint on ARM Linux GDBserver isn't user
visible, but isn't it a user-visible change that ARM Linux GDBserver 
supports Z0 packet?

-- 
Yao (齐尧)

Re: [PATCH v2 7/7] Support software breakpoints for ARM linux in GDBServer.

[Pedro Alves]

On 10/16/2015 10:30 AM, Yao Qi wrote:
> On 15/10/15 17:07, Pedro Alves wrote:
>>> +* Support for software breakpoints on ARM linux was added in GDBServer.
>> Putting a user hat on, what does this mean?  Why is it news worthy?
>>
> 
> Supporting software breakpoint on ARM Linux GDBserver isn't user
> visible, but isn't it a user-visible change that ARM Linux GDBserver 
> supports Z0 packet?

How can the user tell?  Is there any user-visible functionality or feature
that this enables?  Something the user couldn't do before but now can?
It just looks like an implementation detail to me.

If we're going to mention it, then I think it should be described
in terms of the packet, otherwise we're back to "but I could always
set software breakpoints before, what's new?".

BTW, I think we should move all the new gdbserver features
to a new section, like in previous releases.  Then the end result
would be something like:

* New features in the GDB remote stub, GDBserver

   ** Support for software breakpoint packets (Z0) on ARM Linux.

   ** Support for tracepoints on Aarch64 Linux.

   ** Support for fast tracepoints on Aarch64 Linux, including JIT compiling
     fast tracepoint's conditional expression bytecode into native code.

Thanks,
Pedro Alves

Re: [PATCH v2 7/7] Support software breakpoints for ARM linux in GDBServer.

[Yao Qi]

Antoine Tremblay <antoine.tremblay@ericsson.com> writes:

> +/* Get the breakpoint from the remote kind
> +   2 is thumb-16
> +   3 is thumb2-32
> +   4 is arm
> +*/

Need comment /* Implementation of .... */

> +static const unsigned char *
> +arm_breakpoint_from_kind (int *kind)
> +{
> +  switch (*kind) {

Wrong format.  "{" should be put in the next line.

> +  case 2:
> +    return (unsigned char *) &thumb_breakpoint;
> +  case 3:
> +    *kind = 4;
> +    return (unsigned char *) &thumb2_breakpoint;
> +  case 4:
> +    return (unsigned char *) &arm_breakpoint;
> +  default:
> +    return NULL;
> +  }
> +  return NULL;
> +}

Otherwise, patch is good to me.

-- 
Yao (齐尧)

Re: [PATCH v2 7/7] Support software breakpoints for ARM linux in GDBServer.

[Yao Qi]

On 16/10/15 12:52, Yao Qi wrote:
> Otherwise, patch is good to me.

FAOD， I meant non-NEWS part of patch is good to me.

-- 
Yao (齐尧)

[PATCH v2 1/7] Add breakpoint_from_pc target_ops for software breakpoints in GDBServer.

[Antoine Tremblay]

In this v2 :
 - Comments added for linux_target_ops method implementation
 - Removed cris_get_pc (void) change
 - Removed unneeded {}
 - Fixed PC/PCPTR comments
 - Comments adjusted for breakpoint_from_pc
 - Improved ChangeLog
---

Add breakpoint_from_pc target_ops for software breakpoints in GDBServer.

This patch is in preparation for software breakpoints on ARM
linux.  It refactors breakpoint and breakpoint_len into
breakpoint_from_pc to prepare the case where we have multiple types of
breakpoints.

breakpoint_from_pc returns the breakpoint for this PC as a string of bytes,
the length of the breakpoint and ajusts the PC to the real memory location in
case a flag was present in the PC.

No regressions, tested on Ubuntu 14.04 on ARMv7 and x86
With gdbserver-{native,extended} / { -marm -mthumb }

Also since the target_ops have been changed compilation was tested on
all affected archs namely : aarch64, arm, bfin, cris, crisv32, m32r,
m68k, mips, nios2, ppc, s390, sh, sparc, tic6x, tile, x86, steins.

gdb/gdbserver/ChangeLog:
	* linux-aarch64-low.c (aarch64_breakpoint_from_pc): New function.
	(struct linux_target_ops) <breakpoint>: Remove.
	(struct linux_target_ops) <breakpoint_len>: Remove.
	(struct linux_target_ops) <breakpoint_from_pc>: Initialize field.
	* linux-arm-low.c (arm_breakpoint_from_pc): New function.
	(struct linux_target_ops) <breakpoint>: Remove.
	(struct linux_target_ops) <breakpoint_len>: Remove.
	(struct linux_target_ops) <breakpoint_from_pc>: Initialize field.
	* linux-bfin-low.c (bfin_breakpoint_from_pc): New function.
	(struct linux_target_ops) <breakpoint>: Remove.
	(struct linux_target_ops) <breakpoint_len>: Remove.
	(struct linux_target_ops) <breakpoint_from_pc>: Initialize field.
	* linux-cris-low.c (cris_breakpoint_from_pc): New function.
	(struct linux_target_ops) <breakpoint>: Remove.
	(struct linux_target_ops) <breakpoint_len>: Remove.
	(struct linux_target_ops) <breakpoint_from_pc>: Initialize field.
	* linux-crisv32-low.c (cris_breakpoint_from_pc): New function.
	(struct linux_target_ops) <breakpoint>: Remove.
	(struct linux_target_ops) <breakpoint_len>: Remove.
	(struct linux_target_ops) <breakpoint_from_pc>: Initialize field.
	* linux-low.c (linux_wait_1): Add call to breakpoint_from_pc.
	(linux_breakpoint_from_pc): New function.
        (static struct target_ops) <breakpoint_from_pc>: Initialize field.
	(initialize_low): Add call to breakpoint_from_pc.
	* linux-low.h (struct linux_target_ops) <breakpoint_from_pc>: New field.
	* linux-m32r-low.c (m32r_breakpoint_from_pc): New function.
	(struct linux_target_ops) <breakpoint>: Remove.
	(struct linux_target_ops) <breakpoint_len>: Remove.
	(struct linux_target_ops) <breakpoint_from_pc>: Initialize field.
	* linux-m68k-low.c (m68k_breakpoint_from_pc): New function.
	(struct linux_target_ops) <breakpoint>: Remove.
	(struct linux_target_ops) <breakpoint_len>: Remove.
	(struct linux_target_ops) <breakpoint_from_pc>: Initialize field.
	* linux-mips-low.c (mips_breakpoint_from_pc): New function.
	(struct linux_target_ops) <breakpoint>: Remove.
	(struct linux_target_ops) <breakpoint_len>: Remove.
	(struct linux_target_ops) <breakpoint_from_pc>: Initialize field.
	* linux-nios2-low.c (nios2_breakpoint_from_pc): New function.
	(struct linux_target_ops) <breakpoint>: Remove.
	(struct linux_target_ops) <breakpoint_len>: Remove.
	(struct linux_target_ops) <breakpoint_from_pc>: Initialize field.
	* linux-ppc-low.c (ppc_breakpoint_from_pc): New function.
	(struct linux_target_ops) <breakpoint>: Remove.
	(struct linux_target_ops) <breakpoint_len>: Remove.
	(struct linux_target_ops) <breakpoint_from_pc>: Initialize field.
	* linux-s390-low.c (s390_breakpoint_from_pc): Likewise.
	(struct linux_target_ops) <breakpoint>: Remove.
	(struct linux_target_ops) <breakpoint_len>: Remove.
	(struct linux_target_ops) <breakpoint_from_pc>: Initialize field.
	* linux-sh-low.c (sh_breakpoint_from_pc): New function.
	(struct linux_target_ops) <breakpoint>: Remove.
	(struct linux_target_ops) <breakpoint_len>: Remove.
	(struct linux_target_ops) <breakpoint_from_pc>: Initialize field.
	* linux-sparc-low.c (sparc_breakpoint_from_pc): New function.
	(struct linux_target_ops) <breakpoint>: Remove.
	(struct linux_target_ops) <breakpoint_len>: Remove.
	(struct linux_target_ops) <breakpoint_from_pc>: Initialize field.
	* linux-tic6x-low.c (tic6x_breakpoint_from_pc): New function.
	(struct linux_target_ops) <breakpoint>: Remove.
	(struct linux_target_ops) <breakpoint_len>: Remove.
	(struct linux_target_ops) <breakpoint_from_pc>: Initialize field.
	* linux-tile-low.c (tile_breakpoint_from_pc): New function.
	(struct linux_target_ops) <breakpoint>: Remove.
	(struct linux_target_ops) <breakpoint_len>: Remove.
	(struct linux_target_ops) <breakpoint_from_pc>: Initialize field.
	* linux-x86-low.c (x86_breakpoint_from_pc): New function.
	(struct linux_target_ops) <breakpoint>: Remove.
	(struct linux_target_ops) <breakpoint_len>: Remove.
	(struct linux_target_ops) <breakpoint_from_pc>: Initialize field.
	* linux-xtensa-low.c (xtensa_breakpoint_from_pc): New function.
	(struct linux_target_ops) <breakpoint>: Remove.
	(struct linux_target_ops) <breakpoint_len>: Remove.
	(struct linux_target_ops) <breakpoint_from_pc>: Initialize field.
	* target.h (struct target_ops) <breakpoint_from_pc>: New field.
---
 gdb/gdbserver/linux-aarch64-low.c | 12 ++++++++++--
 gdb/gdbserver/linux-arm-low.c     | 28 ++++++++++++++++------------
 gdb/gdbserver/linux-bfin-low.c    | 12 ++++++++++--
 gdb/gdbserver/linux-cris-low.c    | 12 ++++++++++--
 gdb/gdbserver/linux-crisv32-low.c | 12 ++++++++++--
 gdb/gdbserver/linux-low.c         | 26 +++++++++++++++++++++++---
 gdb/gdbserver/linux-low.h         |  8 ++++++--
 gdb/gdbserver/linux-m32r-low.c    | 12 ++++++++++--
 gdb/gdbserver/linux-m68k-low.c    | 12 ++++++++++--
 gdb/gdbserver/linux-mips-low.c    | 12 ++++++++++--
 gdb/gdbserver/linux-nios2-low.c   | 23 +++++++++++++++--------
 gdb/gdbserver/linux-ppc-low.c     | 12 ++++++++++--
 gdb/gdbserver/linux-s390-low.c    | 12 ++++++++++--
 gdb/gdbserver/linux-sh-low.c      | 12 ++++++++++--
 gdb/gdbserver/linux-sparc-low.c   | 11 +++++++++--
 gdb/gdbserver/linux-tic6x-low.c   | 15 +++++++++++----
 gdb/gdbserver/linux-tile-low.c    | 12 ++++++++++--
 gdb/gdbserver/linux-x86-low.c     | 12 ++++++++++--
 gdb/gdbserver/linux-xtensa-low.c  | 12 ++++++++++--
 gdb/gdbserver/target.h            |  5 +++++
 20 files changed, 215 insertions(+), 57 deletions(-)

diff --git a/gdb/gdbserver/linux-aarch64-low.c b/gdb/gdbserver/linux-aarch64-low.c
index 5592e61..1e7a0ff 100644
--- a/gdb/gdbserver/linux-aarch64-low.c
+++ b/gdb/gdbserver/linux-aarch64-low.c
@@ -205,6 +205,15 @@ aarch64_set_pc (struct regcache *regcache, CORE_ADDR pc)
    (aarch64_default_breakpoint).  */
 static const gdb_byte aarch64_breakpoint[] = {0x00, 0x00, 0x20, 0xd4};
 
+/* Implementation of linux_target_ops method "breakpoint_from_pc".  */
+
+static const unsigned char *
+aarch64_breakpoint_from_pc (CORE_ADDR *pcptr, int *len)
+{
+  *len = aarch64_breakpoint_len;
+  return (const unsigned char *) &aarch64_breakpoint;
+}
+
 /* Implementation of linux_target_ops method "breakpoint_at".  */
 
 static int
@@ -3238,8 +3247,7 @@ struct linux_target_ops the_low_target =
   NULL, /* fetch_register */
   aarch64_get_pc,
   aarch64_set_pc,
-  (const unsigned char *) &aarch64_breakpoint,
-  aarch64_breakpoint_len,
+  aarch64_breakpoint_from_pc,
   NULL, /* breakpoint_reinsert_addr */
   0,    /* decr_pc_after_break */
   aarch64_breakpoint_at,
diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
index a277bb6..367c704 100644
--- a/gdb/gdbserver/linux-arm-low.c
+++ b/gdb/gdbserver/linux-arm-low.c
@@ -913,6 +913,21 @@ arm_regs_info (void)
     return &regs_info_arm;
 }
 
+static const unsigned char *
+arm_breakpoint_from_pc (CORE_ADDR *pcptr, int *len)
+{
+  *len = arm_breakpoint_len;
+   /* Define an ARM-mode breakpoint; we only set breakpoints in the C
+     library, which is most likely to be ARM.  If the kernel supports
+     clone events, we will never insert a breakpoint, so even a Thumb
+     C library will work; so will mixing EABI/non-EABI gdbserver and
+     application.  */
+#ifndef __ARM_EABI__
+  return (const unsigned char *) &arm_breakpoint;
+#else
+  return (const unsigned char *) &arm_eabi_breakpoint;
+#endif
+}
 struct linux_target_ops the_low_target = {
   arm_arch_setup,
   arm_regs_info,
@@ -921,18 +936,7 @@ struct linux_target_ops the_low_target = {
   NULL, /* fetch_register */
   arm_get_pc,
   arm_set_pc,
-
-  /* Define an ARM-mode breakpoint; we only set breakpoints in the C
-     library, which is most likely to be ARM.  If the kernel supports
-     clone events, we will never insert a breakpoint, so even a Thumb
-     C library will work; so will mixing EABI/non-EABI gdbserver and
-     application.  */
-#ifndef __ARM_EABI__
-  (const unsigned char *) &arm_breakpoint,
-#else
-  (const unsigned char *) &arm_eabi_breakpoint,
-#endif
-  arm_breakpoint_len,
+  arm_breakpoint_from_pc,
   arm_reinsert_addr,
   0,
   arm_breakpoint_at,
diff --git a/gdb/gdbserver/linux-bfin-low.c b/gdb/gdbserver/linux-bfin-low.c
index 4002f22..2dc57b4 100644
--- a/gdb/gdbserver/linux-bfin-low.c
+++ b/gdb/gdbserver/linux-bfin-low.c
@@ -75,6 +75,15 @@ bfin_set_pc (struct regcache *regcache, CORE_ADDR pc)
 #define bfin_breakpoint_len 2
 static const unsigned char bfin_breakpoint[bfin_breakpoint_len] = {0xa1, 0x00};
 
+/* Implementation of linux_target_ops method "breakpoint_from_pc".  */
+
+static const unsigned char *
+bfin_breakpoint_from_pc (CORE_ADDR *pcptr, int *len)
+{
+  *len = bfin_breakpoint_len;
+  return (const unsigned char *) &bfin_breakpoint;
+}
+
 static int
 bfin_breakpoint_at (CORE_ADDR where)
 {
@@ -122,8 +131,7 @@ struct linux_target_ops the_low_target = {
   NULL, /* fetch_register */
   bfin_get_pc,
   bfin_set_pc,
-  bfin_breakpoint,
-  bfin_breakpoint_len,
+  bfin_breakpoint_from_pc,
   NULL, /* breakpoint_reinsert_addr */
   2,
   bfin_breakpoint_at,
diff --git a/gdb/gdbserver/linux-cris-low.c b/gdb/gdbserver/linux-cris-low.c
index e0bfa1a..64413f8 100644
--- a/gdb/gdbserver/linux-cris-low.c
+++ b/gdb/gdbserver/linux-cris-low.c
@@ -81,6 +81,15 @@ cris_set_pc (struct regcache *regcache, CORE_ADDR pc)
 static const unsigned short cris_breakpoint = 0xe938;
 #define cris_breakpoint_len 2
 
+/* Implementation of linux_target_ops method "breakpoint_from_pc".  */
+
+static const unsigned char *
+cris_breakpoint_from_pc (CORE_ADDR *pcptr, int *len)
+{
+  *len = cris_breakpoint_len;
+  return (const unsigned char *) &cris_breakpoint;
+}
+
 static int
 cris_breakpoint_at (CORE_ADDR where)
 {
@@ -140,8 +149,7 @@ struct linux_target_ops the_low_target = {
   NULL, /* fetch_register */
   cris_get_pc,
   cris_set_pc,
-  (const unsigned char *) &cris_breakpoint,
-  cris_breakpoint_len,
+  cris_breakpoint_from_pc,
   cris_reinsert_addr,
   0,
   cris_breakpoint_at,
diff --git a/gdb/gdbserver/linux-crisv32-low.c b/gdb/gdbserver/linux-crisv32-low.c
index 5120863..7e499f2 100644
--- a/gdb/gdbserver/linux-crisv32-low.c
+++ b/gdb/gdbserver/linux-crisv32-low.c
@@ -77,6 +77,15 @@ cris_set_pc (struct regcache *regcache, CORE_ADDR pc)
 static const unsigned short cris_breakpoint = 0xe938;
 #define cris_breakpoint_len 2
 
+/* Implementation of linux_target_ops method "breakpoint_from_pc".  */
+
+static const unsigned char *
+cris_breakpoint_from_pc (CORE_ADDR *pcptr, int *len)
+{
+  *len = cris_breakpoint_len;
+  return (const unsigned char *) &cris_breakpoint;
+}
+
 static int
 cris_breakpoint_at (CORE_ADDR where)
 {
@@ -420,8 +429,7 @@ struct linux_target_ops the_low_target = {
   NULL, /* fetch_register */
   cris_get_pc,
   cris_set_pc,
-  (const unsigned char *) &cris_breakpoint,
-  cris_breakpoint_len,
+  cris_breakpoint_from_pc,
   cris_reinsert_addr,
   0,
   cris_breakpoint_at,
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 3a1a6ae..dc16fe0 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -3012,7 +3012,11 @@ linux_wait_1 (ptid_t ptid,
   if (!ptid_equal (step_over_bkpt, null_ptid)
       && event_child->stop_reason == TARGET_STOPPED_BY_SW_BREAKPOINT)
     {
-      unsigned int increment_pc = the_low_target.breakpoint_len;
+      int increment_pc = 0;
+      CORE_ADDR stop_pc = event_child->stop_pc;
+
+      (*the_low_target.breakpoint_from_pc)
+	(&stop_pc, &increment_pc);
 
       if (debug_threads)
 	{
@@ -6932,6 +6936,15 @@ current_lwp_ptid (void)
   return ptid_of (current_thread);
 }
 
+const unsigned char *
+linux_breakpoint_from_pc (CORE_ADDR *pcptr, int *lenptr)
+{
+  if (the_low_target.breakpoint_from_pc != NULL)
+    return (*the_low_target.breakpoint_from_pc) (pcptr, lenptr);
+  else
+    return NULL;
+}
+
 static struct target_ops linux_target_ops = {
   linux_create_inferior,
   linux_arch_setup,
@@ -7026,6 +7039,7 @@ static struct target_ops linux_target_ops = {
   linux_mntns_open_cloexec,
   linux_mntns_unlink,
   linux_mntns_readlink,
+  linux_breakpoint_from_pc,
 };
 
 static void
@@ -7053,10 +7067,16 @@ 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);
-  set_breakpoint_data (the_low_target.breakpoint,
-		       the_low_target.breakpoint_len);
+
+  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/linux-low.h b/gdb/gdbserver/linux-low.h
index f8f6e78..a9964ac 100644
--- a/gdb/gdbserver/linux-low.h
+++ b/gdb/gdbserver/linux-low.h
@@ -141,8 +141,12 @@ 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 raw breakpoint for this target based on PC.  The PCPTR is
+     ajusted to the real memory location in case a flag was present in the
+     PC.  */
+  const unsigned char *(*breakpoint_from_pc) (CORE_ADDR *pcptr, int *lenptr);
+
   CORE_ADDR (*breakpoint_reinsert_addr) (void);
 
   int decr_pc_after_break;
diff --git a/gdb/gdbserver/linux-m32r-low.c b/gdb/gdbserver/linux-m32r-low.c
index 8ffeda2..41c8252 100644
--- a/gdb/gdbserver/linux-m32r-low.c
+++ b/gdb/gdbserver/linux-m32r-low.c
@@ -73,6 +73,15 @@ m32r_set_pc (struct regcache *regcache, CORE_ADDR pc)
 static const unsigned short m32r_breakpoint = 0x10f1;
 #define m32r_breakpoint_len 2
 
+/* Implementation of linux_target_ops method "breakpoint_from_pc".  */
+
+static const unsigned char *
+m32r_breakpoint_from_pc (CORE_ADDR *pcptr, int *len)
+{
+  *len = m32r_breakpoint_len;
+  return (const unsigned char *) &m32r_breakpoint;
+}
+
 static int
 m32r_breakpoint_at (CORE_ADDR where)
 {
@@ -120,8 +129,7 @@ struct linux_target_ops the_low_target = {
   NULL, /* fetch_register */
   m32r_get_pc,
   m32r_set_pc,
-  (const unsigned char *) &m32r_breakpoint,
-  m32r_breakpoint_len,
+  m32r_breakpoint_from_pc,
   NULL,
   0,
   m32r_breakpoint_at,
diff --git a/gdb/gdbserver/linux-m68k-low.c b/gdb/gdbserver/linux-m68k-low.c
index 39c9cc5..2f0c6a0 100644
--- a/gdb/gdbserver/linux-m68k-low.c
+++ b/gdb/gdbserver/linux-m68k-low.c
@@ -125,6 +125,15 @@ static struct regset_info m68k_regsets[] = {
 static const unsigned char m68k_breakpoint[] = { 0x4E, 0x4F };
 #define m68k_breakpoint_len 2
 
+/* Implementation of linux_target_ops method "breakpoint_from_pc".  */
+
+static const unsigned char *
+m68k_breakpoint_from_pc (CORE_ADDR *pcptr, int *len)
+{
+  *len = m68k_breakpoint_len;
+  return (unsigned char*) &m68k_breakpoint;
+}
+
 static CORE_ADDR
 m68k_get_pc (struct regcache *regcache)
 {
@@ -215,8 +224,7 @@ struct linux_target_ops the_low_target = {
   NULL, /* fetch_register */
   m68k_get_pc,
   m68k_set_pc,
-  m68k_breakpoint,
-  m68k_breakpoint_len,
+  m68k_breakpoint_from_pc,
   NULL,
   2,
   m68k_breakpoint_at,
diff --git a/gdb/gdbserver/linux-mips-low.c b/gdb/gdbserver/linux-mips-low.c
index d1181b6..96cfc1c 100644
--- a/gdb/gdbserver/linux-mips-low.c
+++ b/gdb/gdbserver/linux-mips-low.c
@@ -266,6 +266,15 @@ mips_set_pc (struct regcache *regcache, CORE_ADDR pc)
 static const unsigned int mips_breakpoint = 0x0005000d;
 #define mips_breakpoint_len 4
 
+/* Implementation of linux_target_ops method "breakpoint_from_pc".  */
+
+static const unsigned char *
+mips_breakpoint_from_pc (CORE_ADDR *pcptr, int *len)
+{
+  *len = mips_breakpoint_len;
+  return (const unsigned char *) &mips_breakpoint;
+}
+
 /* We only place breakpoints in empty marker functions, and thread locking
    is outside of the function.  So rather than importing software single-step,
    we can just run until exit.  */
@@ -881,8 +890,7 @@ struct linux_target_ops the_low_target = {
   NULL, /* fetch_register */
   mips_get_pc,
   mips_set_pc,
-  (const unsigned char *) &mips_breakpoint,
-  mips_breakpoint_len,
+  mips_breakpoint_from_pc,
   mips_reinsert_addr,
   0,
   mips_breakpoint_at,
diff --git a/gdb/gdbserver/linux-nios2-low.c b/gdb/gdbserver/linux-nios2-low.c
index 71542b4..d04dbe0 100644
--- a/gdb/gdbserver/linux-nios2-low.c
+++ b/gdb/gdbserver/linux-nios2-low.c
@@ -127,9 +127,23 @@ nios2_set_pc (struct regcache *regcache, CORE_ADDR pc)
 #define NIOS2_BREAKPOINT 0x003b6ffa
 #endif
 
+/* We only register the 4-byte breakpoint, even on R2 targets which also
+   support 2-byte breakpoints.  Since there is no supports_z_point_type
+   function provided, gdbserver never inserts software breakpoints itself
+   and instead relies on GDB to insert the breakpoint of the correct length
+   via a memory write.  */
 static const unsigned int nios2_breakpoint = NIOS2_BREAKPOINT;
 #define nios2_breakpoint_len 4
 
+/* Implementation of linux_target_ops method "breakpoint_from_pc".  */
+
+static const unsigned char *
+nios2_breakpoint_from_pc (CORE_ADDR *pcptr, int *len)
+{
+  *len = nios2_breakpoint_len;
+  return (const unsigned char *) &nios2_breakpoint;
+}
+
 /* Implement the breakpoint_reinsert_addr linux_target_ops method.  */
 
 static CORE_ADDR
@@ -263,14 +277,7 @@ struct linux_target_ops the_low_target =
   NULL,
   nios2_get_pc,
   nios2_set_pc,
-
-  /* We only register the 4-byte breakpoint, even on R2 targets which also
-     support 2-byte breakpoints.  Since there is no supports_z_point_type
-     function provided, gdbserver never inserts software breakpoints itself
-     and instead relies on GDB to insert the breakpoint of the correct length
-     via a memory write.  */
-  (const unsigned char *) &nios2_breakpoint,
-  nios2_breakpoint_len,
+  nios2_breakpoint_from_pc,
   nios2_reinsert_addr,
   0,
   nios2_breakpoint_at,
diff --git a/gdb/gdbserver/linux-ppc-low.c b/gdb/gdbserver/linux-ppc-low.c
index 188fac0..92ebb03 100644
--- a/gdb/gdbserver/linux-ppc-low.c
+++ b/gdb/gdbserver/linux-ppc-low.c
@@ -486,6 +486,15 @@ ppc_arch_setup (void)
 static const unsigned int ppc_breakpoint = 0x7d821008;
 #define ppc_breakpoint_len 4
 
+/* Implementation of linux_target_ops method "breakpoint_from_pc".  */
+
+static const unsigned char *
+ppc_breakpoint_from_pc (CORE_ADDR *pcptr, int *len)
+{
+  *len = ppc_breakpoint_len;
+  return (const unsigned char *) &ppc_breakpoint;
+}
+
 static int
 ppc_breakpoint_at (CORE_ADDR where)
 {
@@ -685,8 +694,7 @@ struct linux_target_ops the_low_target = {
   NULL, /* fetch_register */
   ppc_get_pc,
   ppc_set_pc,
-  (const unsigned char *) &ppc_breakpoint,
-  ppc_breakpoint_len,
+  ppc_breakpoint_from_pc,
   NULL,
   0,
   ppc_breakpoint_at,
diff --git a/gdb/gdbserver/linux-s390-low.c b/gdb/gdbserver/linux-s390-low.c
index 8a0a689..d973f96 100644
--- a/gdb/gdbserver/linux-s390-low.c
+++ b/gdb/gdbserver/linux-s390-low.c
@@ -397,6 +397,15 @@ static struct regset_info s390_regsets[] = {
 static const unsigned char s390_breakpoint[] = { 0, 1 };
 #define s390_breakpoint_len 2
 
+/* Implementation of linux_target_ops method "breakpoint_from_pc".  */
+
+static const unsigned char *
+s390_breakpoint_from_pc (CORE_ADDR *pcptr, int *len)
+{
+  *len = s390_breakpoint_len;
+  return (const unsigned char *) &s390_breakpoint;
+}
+
 static CORE_ADDR
 s390_get_pc (struct regcache *regcache)
 {
@@ -665,8 +674,7 @@ struct linux_target_ops the_low_target = {
   NULL, /* fetch_register */
   s390_get_pc,
   s390_set_pc,
-  s390_breakpoint,
-  s390_breakpoint_len,
+  s390_breakpoint_from_pc,
   NULL,
   s390_breakpoint_len,
   s390_breakpoint_at,
diff --git a/gdb/gdbserver/linux-sh-low.c b/gdb/gdbserver/linux-sh-low.c
index 218d4d3..2b5cb15 100644
--- a/gdb/gdbserver/linux-sh-low.c
+++ b/gdb/gdbserver/linux-sh-low.c
@@ -77,6 +77,15 @@ sh_set_pc (struct regcache *regcache, CORE_ADDR pc)
 static const unsigned short sh_breakpoint = 0xc3c3;
 #define sh_breakpoint_len 2
 
+/* Implementation of linux_target_ops method "breakpoint_from_pc".  */
+
+static const unsigned char *
+sh_breakpoint_from_pc (CORE_ADDR *pcptr, int *len)
+{
+  *len = sh_breakpoint_len;
+  return (const unsigned char *) &sh_breakpoint;
+}
+
 static int
 sh_breakpoint_at (CORE_ADDR where)
 {
@@ -148,8 +157,7 @@ struct linux_target_ops the_low_target = {
   NULL, /* fetch_register */
   sh_get_pc,
   sh_set_pc,
-  (const unsigned char *) &sh_breakpoint,
-  sh_breakpoint_len,
+  sh_breakpoint_from_pc,
   NULL,
   0,
   sh_breakpoint_at,
diff --git a/gdb/gdbserver/linux-sparc-low.c b/gdb/gdbserver/linux-sparc-low.c
index 796af8a..8e1febf 100644
--- a/gdb/gdbserver/linux-sparc-low.c
+++ b/gdb/gdbserver/linux-sparc-low.c
@@ -240,6 +240,14 @@ static const unsigned char sparc_breakpoint[INSN_SIZE] = {
 };
 #define sparc_breakpoint_len INSN_SIZE
 
+/* Implementation of linux_target_ops method "breakpoint_from_pc".  */
+
+static const unsigned char *
+sparc_breakpoint_from_pc (CORE_ADDR *pcptr, int *len)
+{
+  *len = sparc_breakpoint_len;
+  return (const unsigned char *) &sparc_breakpoint;
+}
 
 static int
 sparc_breakpoint_at (CORE_ADDR where)
@@ -323,8 +331,7 @@ struct linux_target_ops the_low_target = {
   sparc_get_pc,
   /* No sparc_set_pc is needed.  */
   NULL,
-  (const unsigned char *) sparc_breakpoint,
-  sparc_breakpoint_len,
+  sparc_breakpoint_from_pc,
   sparc_reinsert_addr,
   0,
   sparc_breakpoint_at,
diff --git a/gdb/gdbserver/linux-tic6x-low.c b/gdb/gdbserver/linux-tic6x-low.c
index a2ac3ee..a259e72 100644
--- a/gdb/gdbserver/linux-tic6x-low.c
+++ b/gdb/gdbserver/linux-tic6x-low.c
@@ -171,6 +171,16 @@ extern struct linux_target_ops the_low_target;
 
 static int *tic6x_regmap;
 static unsigned int tic6x_breakpoint;
+#define tic6x_breakpoint_len 4
+
+/* Implementation of linux_target_ops method "breakpoint_from_pc".  */
+
+static const unsigned char *
+tic6x_breakpoint_from_pc (CORE_ADDR *pcptr, int *len)
+{
+  *len = tic6x_breakpoint_len;
+  return (const unsigned char *) &tic6x_breakpoint;
+}
 
 /* Forward definition.  */
 static struct usrregs_info tic6x_usrregs_info;
@@ -247,8 +257,6 @@ tic6x_set_pc (struct regcache *regcache, CORE_ADDR pc)
   supply_register_by_name (regcache, "PC", newpc.buf);
 }
 
-#define tic6x_breakpoint_len 4
-
 static int
 tic6x_breakpoint_at (CORE_ADDR where)
 {
@@ -367,8 +375,7 @@ struct linux_target_ops the_low_target = {
   NULL, /* fetch_register */
   tic6x_get_pc,
   tic6x_set_pc,
-  (const unsigned char *) &tic6x_breakpoint,
-  tic6x_breakpoint_len,
+  tic6x_breakpoint_from_pc,
   NULL,
   0,
   tic6x_breakpoint_at,
diff --git a/gdb/gdbserver/linux-tile-low.c b/gdb/gdbserver/linux-tile-low.c
index 6aaea6a..04c7cd0 100644
--- a/gdb/gdbserver/linux-tile-low.c
+++ b/gdb/gdbserver/linux-tile-low.c
@@ -88,6 +88,15 @@ tile_set_pc (struct regcache *regcache, CORE_ADDR pc)
 static uint64_t tile_breakpoint = 0x400b3cae70166000ULL;
 #define tile_breakpoint_len 8
 
+/* Implementation of linux_target_ops method "breakpoint_from_pc".  */
+
+static const unsigned char *
+tile_breakpoint_from_pc (CORE_ADDR *pcptr, int *len)
+{
+  *len = tile_breakpoint_len;
+  return (const unsigned char *) &tile_breakpoint;
+}
+
 static int
 tile_breakpoint_at (CORE_ADDR where)
 {
@@ -182,8 +191,7 @@ struct linux_target_ops the_low_target =
   NULL,
   tile_get_pc,
   tile_set_pc,
-  (const unsigned char *) &tile_breakpoint,
-  tile_breakpoint_len,
+  tile_breakpoint_from_pc,
   NULL,
   0,
   tile_breakpoint_at,
diff --git a/gdb/gdbserver/linux-x86-low.c b/gdb/gdbserver/linux-x86-low.c
index 20d4257..41803c4 100644
--- a/gdb/gdbserver/linux-x86-low.c
+++ b/gdb/gdbserver/linux-x86-low.c
@@ -3243,6 +3243,15 @@ x86_emit_ops (void)
     return &i386_emit_ops;
 }
 
+/* Implementation of linux_target_ops method "breakpoint_from_pc".  */
+
+static const unsigned char *
+x86_breakpoint_from_pc (CORE_ADDR *pcptr, int *len)
+{
+  *len = x86_breakpoint_len;
+  return x86_breakpoint;
+}
+
 static int
 x86_supports_range_stepping (void)
 {
@@ -3261,8 +3270,7 @@ struct linux_target_ops the_low_target =
   NULL, /* fetch_register */
   x86_get_pc,
   x86_set_pc,
-  x86_breakpoint,
-  x86_breakpoint_len,
+  x86_breakpoint_from_pc,
   NULL,
   1,
   x86_breakpoint_at,
diff --git a/gdb/gdbserver/linux-xtensa-low.c b/gdb/gdbserver/linux-xtensa-low.c
index debe467..eb7c5bb 100644
--- a/gdb/gdbserver/linux-xtensa-low.c
+++ b/gdb/gdbserver/linux-xtensa-low.c
@@ -154,6 +154,15 @@ static struct regset_info xtensa_regsets[] = {
 static const unsigned char xtensa_breakpoint[] = XTENSA_BREAKPOINT;
 #define xtensa_breakpoint_len 2
 
+/* Implementation of linux_target_ops method "breakpoint_from_pc".  */
+
+static const unsigned char *
+xtensa_breakpoint_from_pc (CORE_ADDR *pcptr, int *len)
+{
+  *len = xtensa_breakpoint_len;
+  return xtensa_breakpoint;
+}
+
 static CORE_ADDR
 xtensa_get_pc (struct regcache *regcache)
 {
@@ -234,8 +243,7 @@ struct linux_target_ops the_low_target = {
   NULL, /* fetch_register */
   xtensa_get_pc,
   xtensa_set_pc,
-  xtensa_breakpoint,
-  xtensa_breakpoint_len,
+  xtensa_breakpoint_from_pc,
   NULL,
   0,
   xtensa_breakpoint_at,
diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
index a2842b4..603819e 100644
--- a/gdb/gdbserver/target.h
+++ b/gdb/gdbserver/target.h
@@ -441,6 +441,11 @@ struct target_ops
      readlink(2).  */
   ssize_t (*multifs_readlink) (int pid, const char *filename,
 			       char *buf, size_t bufsiz);
+
+  /* Return the raw breakpoint for this target based on PC.  Note that the PC
+     can be NULL, the default breakpoint for the target should be returned in
+     this case.  */
+  const unsigned char *(*breakpoint_from_pc) (CORE_ADDR *pcptr, int *lenptr);
 };
 
 extern struct target_ops *the_target;
-- 
1.9.1

Re: [PATCH v2 1/7] Add breakpoint_from_pc target_ops for software breakpoints in GDBServer.

[Yao Qi]

Antoine Tremblay <antoine.tremblay@ericsson.com> writes:

> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index 3a1a6ae..dc16fe0 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -3012,7 +3012,11 @@ linux_wait_1 (ptid_t ptid,
>    if (!ptid_equal (step_over_bkpt, null_ptid)
>        && event_child->stop_reason == TARGET_STOPPED_BY_SW_BREAKPOINT)
>      {
> -      unsigned int increment_pc = the_low_target.breakpoint_len;
> +      int increment_pc = 0;
> +      CORE_ADDR stop_pc = event_child->stop_pc;
> +
> +      (*the_low_target.breakpoint_from_pc)
> +	(&stop_pc, &increment_pc);
>  

They can be in the same line.

>        if (debug_threads)
>  	{
> @@ -6932,6 +6936,15 @@ current_lwp_ptid (void)
>    return ptid_of (current_thread);
>  }
>  
> +const unsigned char *
> +linux_breakpoint_from_pc (CORE_ADDR *pcptr, int *lenptr)
> +{
> +  if (the_low_target.breakpoint_from_pc != NULL)
> +    return (*the_low_target.breakpoint_from_pc) (pcptr, lenptr);
> +  else
> +    return NULL;
> +}

If the breakpoint_from_pc is NULL, this GDBserver port should be
broken.  We can use gdb_assert to assert
the_low_target.breakpoint_from_pc isn't NULL.

Otherwise the patch looks good to me.

-- 
Yao (齐尧)

Re: [PATCH v2 1/7] Add breakpoint_from_pc target_ops for software breakpoints in GDBServer.

[Pedro Alves]

On 10/05/2015 05:44 PM, Antoine Tremblay wrote:
> +static const unsigned char *

gdb_byte?

> +aarch64_breakpoint_from_pc (CORE_ADDR *pcptr, int *len)
> +{
> +  *len = aarch64_breakpoint_len;
> +  return (const unsigned char *) &aarch64_breakpoint;

... and then this cast goes away.

> +}
> +

> +static const unsigned char *
> +arm_breakpoint_from_pc (CORE_ADDR *pcptr, int *len)
> +{

Missing '/* Implementation of ... */' comment.

> +}
>  struct linux_target_ops the_low_target = {

Missing empty line.


> +
> +  /* Return the raw breakpoint for this target based on PC.  The PCPTR is
> +     ajusted to the real memory location in case a flag was present in the

"adjusted".  Suggest an example, like "a flag (e.g., the Thumb bit on ARM) was"

> +     PC.  */
> +  const unsigned char *(*breakpoint_from_pc) (CORE_ADDR *pcptr, int *lenptr);

Thanks,
Pedro Alves

Re: [PATCH v2 1/7] Add breakpoint_from_pc target_ops for software breakpoints in GDBServer.

[Antoine Tremblay]

On 10/15/2015 11:33 AM, Pedro Alves wrote:
> On 10/05/2015 05:44 PM, Antoine Tremblay wrote:
>> +static const unsigned char *
>
> gdb_byte?
>
>> +aarch64_breakpoint_from_pc (CORE_ADDR *pcptr, int *len)
>> +{
>> +  *len = aarch64_breakpoint_len;
>> +  return (const unsigned char *) &aarch64_breakpoint;
>
> ... and then this cast goes away.
>
>> +}
>> +
>
Ouch indeed that was very wrong , I also fixed bfin, m68k, s390 and 
sparc that had the same kind of issue.

>> +static const unsigned char *
>> +arm_breakpoint_from_pc (CORE_ADDR *pcptr, int *len)
>> +{
>
> Missing '/* Implementation of ... */' comment.
>
Done.

>> +}
>>   struct linux_target_ops the_low_target = {
>
> Missing empty line.
>
Done.
>
>> +
>> +  /* Return the raw breakpoint for this target based on PC.  The PCPTR is
>> +     ajusted to the real memory location in case a flag was present in the
>
> "adjusted".  Suggest an example, like "a flag (e.g., the Thumb bit on ARM) was"
>
Done.
>> +     PC.  */
>> +  const unsigned char *(*breakpoint_from_pc) (CORE_ADDR *pcptr, int *lenptr);
>
> Thanks,
> Pedro Alves
>

Re: [PATCH v2 1/7] Add breakpoint_from_pc target_ops for software breakpoints in GDBServer.

[Antoine Tremblay]

On 10/15/2015 11:58 AM, Antoine Tremblay wrote:
>
>
> On 10/15/2015 11:33 AM, Pedro Alves wrote:
>> On 10/05/2015 05:44 PM, Antoine Tremblay wrote:
>>> +static const unsigned char *
>>
>> gdb_byte?

Note I ended up changing the breakpoint_from_pc signature to return 
gdb_byte * and changed all targets to match this in their breakpoint 
definitions and return from breakpoint_from_pc.