[PATCH 0/5] Software breakpoints support for ARM linux.

[PATCH 0/5] Software breakpoints support for ARM linux.

[Antoine Tremblay]

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

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.

In order to allow that the patches :

"Support multiple breakpoint types per target in GDBServer." and
"Make breakpoint and breakpoint_len local variables in GDBServer."

Allow multiple breakpoints types to be used based on a target specific function
given the current PC.

The ARM implementation of this function is handled by the next patch :
"Add support for ARM breakpoint types in GDBServer."

Also as software breakpoints can have a arch specific encoded length called the
breakpoint kind like it is the case for ARM, GDBServer needs to take this into
account when inserting/removing or reinserting a breakpoint this is handled by :
"Handle breakpoint kinds for software breakpoints in GDBServer."

And finally software breakpoints via Z0 packets are enabled for ARM in :
"Support software breakpoints for ARM on linux."

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 2/5] Make breakpoint and breakpoint_len local variables in GDBServer.

[Antoine Tremblay]

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

It refactors the breakpoint and breakpoint_len global variables to be local
so that multiple types of breakpoints can be used for an arch.

One important implementation detail is the introduction of the pcfull field
in struct raw_breakpoint.

In order to be able to reinsert a breakpoint we need to remember what were the
flags encoded in the PC, however since functions that compare the program pc
to the breakpoint pc expect an unencoded memory address, we can't put the
encoded value directly in the pc field.

So this patch introduces the pcfull field that contains the flags encoded
in the pc so that we can properly reinsert a breakpoint that has its type
information encoded in the pc.  pcfull shall only be used when
inserting/removing/reinserting a breakpoint, all other breakpoint->pc
references can remain the same.

Note this is for software breakpoints only, when using hardware breakpoints
then pcfull is not set or used.

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

gdbserver/ChangeLog:
	* linux-low.c (initialize_low): Remove
        breakpoint_data initialization.
	* mem-break.c (struct raw_breakpoint): Add pcfull.
	(insert_memory_breakpoint): Call breakpoint_from_pc.
	(remove_memory_breakpoint): Likewise.
	(set_raw_breakpoint_at): Likewise.
	(set_breakpoint_at): Set default breakpoint size to 0.
	(set_breakpoint_data): Remove.
	(validate_inserted_breakpoint): Call breakpoint_from_pc.
	(check_mem_read): Call breakpoint_from_pc.
	(check_mem_write): Call breakpoint_from_pc.
	(clone_one_breakpoint): Copy pcfull field.
---
 gdb/gdbserver/linux-low.c |  6 ----
 gdb/gdbserver/mem-break.c | 76 ++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 58 insertions(+), 24 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index bb08761..06387a0 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -7069,16 +7069,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 9356741..68e14c3 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,10 @@ 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 size.  */
   int size;
 
@@ -300,6 +302,12 @@ insert_memory_breakpoint (struct raw_breakpoint *bp)
 {
   unsigned char buf[MAX_BREAKPOINT_LEN];
   int err;
+  const unsigned char *breakpoint_data;
+  int breakpoint_len;
+  CORE_ADDR pc;
+
+  pc = bp->pcfull;
+  breakpoint_data = the_target->breakpoint_from_pc (&pc, &breakpoint_len);
 
   if (breakpoint_data == NULL)
     return 1;
@@ -349,6 +357,11 @@ remove_memory_breakpoint (struct raw_breakpoint *bp)
 {
   unsigned char buf[MAX_BREAKPOINT_LEN];
   int err;
+  int breakpoint_len;
+  CORE_ADDR pc;
+
+  pc = bp->pcfull;
+  the_target->breakpoint_from_pc (&pc, &breakpoint_len);
 
   /* Since there can be trap breakpoints inserted in the same address
      range, we use `write_inferior_memory', which takes care of
@@ -375,15 +388,27 @@ 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,
+set_raw_breakpoint_at (enum raw_bkpt_type type, const CORE_ADDR where, int size,
 		       int *err)
 {
   struct process_info *proc = current_process ();
   struct raw_breakpoint *bp;
+  CORE_ADDR pc;
+  int breakpoint_len;
+
+  /* pc could be modified by breakpoint_from_pc, use the modified
+     version to find breakpoints and use the full where pc for
+     insert_point so that arch specific data can be passed.  */
+  pc = where;
+
+  the_target->breakpoint_from_pc (&pc, &breakpoint_len);
+
+  if (size == 0)
+    size = breakpoint_len;
 
   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,7 +430,8 @@ set_raw_breakpoint_at (enum raw_bkpt_type type, CORE_ADDR where, int size,
     }
 
   bp = XCNEW (struct raw_breakpoint);
-  bp->pc = where;
+  bp->pc = pc;
+  bp->pcfull = where;
   bp->size = size;
   bp->refcount = 1;
   bp->raw_type = type;
@@ -774,8 +800,9 @@ set_breakpoint_at (CORE_ADDR where, int (*handler) (CORE_ADDR))
 {
   int err_ignored;
 
+  /* default breakpoint_len will be initialized downstream.  */
   return set_breakpoint (other_breakpoint, raw_bkpt_type_sw,
-			 where, breakpoint_len, handler,
+			 where, 0, handler,
 			 &err_ignored);
 }
 
@@ -1588,13 +1615,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)
 {
@@ -1665,6 +1685,13 @@ validate_inserted_breakpoint (struct raw_breakpoint *bp)
 {
   unsigned char *buf;
   int err;
+  const unsigned char *breakpoint_data;
+  int breakpoint_len;
+  CORE_ADDR raw_pc;
+
+  raw_pc = bp->pcfull;
+
+  breakpoint_data = the_target->breakpoint_from_pc (&raw_pc, &breakpoint_len);
 
   gdb_assert (bp->inserted);
   gdb_assert (bp->raw_type == raw_bkpt_type_sw);
@@ -1762,10 +1789,15 @@ 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 start, end;
+      int breakpoint_len;
+      CORE_ADDR raw_pc;
+      CORE_ADDR bp_end, start, end;
       int copy_offset, copy_len, buf_offset;
 
+      raw_pc = bp->pcfull;
+      the_target->breakpoint_from_pc (&raw_pc, &breakpoint_len);
+      bp_end = bp->pc + breakpoint_len;
+
       if (bp->raw_type != raw_bkpt_type_sw)
 	continue;
 
@@ -1851,10 +1883,17 @@ 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 start, end;
+      int breakpoint_len;
+      const unsigned char *breakpoint_data;
+      CORE_ADDR raw_pc;
+      CORE_ADDR bp_end, start, end;
       int copy_offset, copy_len, buf_offset;
 
+      raw_pc = bp->pcfull;
+      breakpoint_data =
+	the_target->breakpoint_from_pc (&raw_pc, &breakpoint_len);
+      bp_end = bp->pc + breakpoint_len;
+
       if (bp->raw_type != raw_bkpt_type_sw)
 	continue;
 
@@ -1963,6 +2002,7 @@ 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->size = src->raw->size;
   memcpy (dest_raw->old_data, src->raw->old_data, MAX_BREAKPOINT_LEN);
   dest_raw->inserted = src->raw->inserted;
-- 
1.9.1

[PATCH 1/5] Support multiple breakpoint types per target in GDBServer.

[Antoine Tremblay]

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, xtensa.

gdbserver/ChangeLog:
	* linux-aarch64-low.c
        (aarch64_breakpoint_from_pc): New function.
	* linux-arm-low.c (arm_breakpoint_from_pc): Likewise.
	* linux-bfin-low.c (bfin_breakpoint_from_pc): Likewise.
	* linux-cris-low.c (cris_breakpoint_from_pc): Likewise.
	* linux-crisv32-low.c: (cris_breakpoint_from_pc): Likewise.
	* linux-low.c (linux_wait_1): Add call to breakpoint_from_pc.
	(linux_breakpoint_from_pc): New function.
	(initialize_low): Add call to breakpoint_from_pc.
	* linux-low.h: Add breakpoint_from_pc operation.
	* linux-m32r-low.c (m32r_breakpoint_from_pc): New function.
	* linux-m68k-low.c (m68k_breakpoint_from_pc): Likewise.
	* linux-mips-low.c (mips_breakpoint_from_pc): Likewise.
	* linux-nios2-low.c (nios2_breakpoint_from_pc): Likewise.
	* linux-ppc-low.c (ppc_breakpoint_from_pc): Likewise.
	* linux-s390-low.c (s390_breakpoint_from_pc): Likewise.
	* linux-sh-low.c (sh_breakpoint_from_pc): Likewise.
	* linux-sparc-low.c (sparc_breakpoint_from_pc): Likewise.
	* linux-tic6x-low.c (tic6x_breakpoint_from_pc): Likewise.
	* linux-tile-low.c (tile_breakpoint_from_pc): Likewise.
	* linux-x86-low.c (x86_breakpoint_from_pc): Likewise.
	* linux-xtensa-low.c(xtensa_breakpoint_from_pc): Likewise.
	* target.h (struct target_ops): Add breakpoint_from_pc operation.
	* win32-arm-low.c (arm_wince_breakpoint_from_pc): New Function.
	* win32-i386-low.c(i386_win32_breakpoint_from_pc): Likewise.
---
 gdb/gdbserver/linux-aarch64-low.c | 10 ++++++++--
 gdb/gdbserver/linux-arm-low.c     | 28 ++++++++++++++++------------
 gdb/gdbserver/linux-bfin-low.c    | 10 ++++++++--
 gdb/gdbserver/linux-cris-low.c    | 12 +++++++++---
 gdb/gdbserver/linux-crisv32-low.c | 10 ++++++++--
 gdb/gdbserver/linux-low.c         | 28 +++++++++++++++++++++++++---
 gdb/gdbserver/linux-low.h         |  9 +++++++--
 gdb/gdbserver/linux-m32r-low.c    | 10 ++++++++--
 gdb/gdbserver/linux-m68k-low.c    | 10 ++++++++--
 gdb/gdbserver/linux-mips-low.c    | 10 ++++++++--
 gdb/gdbserver/linux-nios2-low.c   | 22 +++++++++++++---------
 gdb/gdbserver/linux-ppc-low.c     | 10 ++++++++--
 gdb/gdbserver/linux-s390-low.c    | 10 ++++++++--
 gdb/gdbserver/linux-sh-low.c      | 10 ++++++++--
 gdb/gdbserver/linux-sparc-low.c   |  9 +++++++--
 gdb/gdbserver/linux-tic6x-low.c   | 13 +++++++++----
 gdb/gdbserver/linux-tile-low.c    | 10 ++++++++--
 gdb/gdbserver/linux-x86-low.c     | 10 ++++++++--
 gdb/gdbserver/linux-xtensa-low.c  | 10 ++++++++--
 gdb/gdbserver/target.h            |  5 +++++
 gdb/gdbserver/win32-arm-low.c     | 10 ++++++++--
 gdb/gdbserver/win32-i386-low.c    | 10 ++++++++--
 22 files changed, 203 insertions(+), 63 deletions(-)

diff --git a/gdb/gdbserver/linux-aarch64-low.c b/gdb/gdbserver/linux-aarch64-low.c
index 73b248c..c4f9ffe 100644
--- a/gdb/gdbserver/linux-aarch64-low.c
+++ b/gdb/gdbserver/linux-aarch64-low.c
@@ -200,6 +200,13 @@ aarch64_set_pc (struct regcache *regcache, CORE_ADDR pc)
    (aarch64_default_breakpoint).  */
 static const gdb_byte aarch64_breakpoint[] = {0x00, 0x00, 0x20, 0xd4};
 
+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
@@ -590,8 +597,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..1c0e1e9 100644
--- a/gdb/gdbserver/linux-bfin-low.c
+++ b/gdb/gdbserver/linux-bfin-low.c
@@ -75,6 +75,13 @@ 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};
 
+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 +129,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..da5876d 100644
--- a/gdb/gdbserver/linux-cris-low.c
+++ b/gdb/gdbserver/linux-cris-low.c
@@ -62,7 +62,7 @@ cris_cannot_fetch_register (int regno)
 extern int debug_threads;
 
 static CORE_ADDR
-cris_get_pc (struct regcache *regcache, void)
+cris_get_pc (struct regcache *regcache)
 {
   unsigned long pc;
   collect_register_by_name (regcache, "pc", &pc);
@@ -81,6 +81,13 @@ cris_set_pc (struct regcache *regcache, CORE_ADDR pc)
 static const unsigned short cris_breakpoint = 0xe938;
 #define cris_breakpoint_len 2
 
+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 +147,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..d2dba91 100644
--- a/gdb/gdbserver/linux-crisv32-low.c
+++ b/gdb/gdbserver/linux-crisv32-low.c
@@ -77,6 +77,13 @@ cris_set_pc (struct regcache *regcache, CORE_ADDR pc)
 static const unsigned short cris_breakpoint = 0xe938;
 #define cris_breakpoint_len 2
 
+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 +427,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 f5b64ab..bb08761 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,17 @@ 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 +7041,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 +7069,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..c623150 100644
--- a/gdb/gdbserver/linux-low.h
+++ b/gdb/gdbserver/linux-low.h
@@ -141,8 +141,13 @@ 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.  Note that the PC
+     can be NULL, the default breakpoint for the target should be returned in
+     this case. The PC 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..4712a32 100644
--- a/gdb/gdbserver/linux-m32r-low.c
+++ b/gdb/gdbserver/linux-m32r-low.c
@@ -73,6 +73,13 @@ m32r_set_pc (struct regcache *regcache, CORE_ADDR pc)
 static const unsigned short m32r_breakpoint = 0x10f1;
 #define m32r_breakpoint_len 2
 
+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 +127,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..39a9753 100644
--- a/gdb/gdbserver/linux-m68k-low.c
+++ b/gdb/gdbserver/linux-m68k-low.c
@@ -125,6 +125,13 @@ static struct regset_info m68k_regsets[] = {
 static const unsigned char m68k_breakpoint[] = { 0x4E, 0x4F };
 #define m68k_breakpoint_len 2
 
+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 +222,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..d5333ab 100644
--- a/gdb/gdbserver/linux-mips-low.c
+++ b/gdb/gdbserver/linux-mips-low.c
@@ -266,6 +266,13 @@ mips_set_pc (struct regcache *regcache, CORE_ADDR pc)
 static const unsigned int mips_breakpoint = 0x0005000d;
 #define mips_breakpoint_len 4
 
+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 +888,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..bf9ecc2 100644
--- a/gdb/gdbserver/linux-nios2-low.c
+++ b/gdb/gdbserver/linux-nios2-low.c
@@ -119,7 +119,6 @@ nios2_set_pc (struct regcache *regcache, CORE_ADDR pc)
 
 /* Breakpoint support.  Also see comments on nios2_breakpoint_from_pc
    in nios2-tdep.c.  */
-
 #if defined(__nios2_arch__) && __nios2_arch__ == 2
 #define NIOS2_BREAKPOINT 0xb7fd0020
 #define CDX_BREAKPOINT 0xd7c9
@@ -127,9 +126,21 @@ 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
 
+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 +274,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..4c71cd9 100644
--- a/gdb/gdbserver/linux-ppc-low.c
+++ b/gdb/gdbserver/linux-ppc-low.c
@@ -486,6 +486,13 @@ ppc_arch_setup (void)
 static const unsigned int ppc_breakpoint = 0x7d821008;
 #define ppc_breakpoint_len 4
 
+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 +692,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..f76f867 100644
--- a/gdb/gdbserver/linux-s390-low.c
+++ b/gdb/gdbserver/linux-s390-low.c
@@ -397,6 +397,13 @@ static struct regset_info s390_regsets[] = {
 static const unsigned char s390_breakpoint[] = { 0, 1 };
 #define s390_breakpoint_len 2
 
+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 +672,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..5c11fd7 100644
--- a/gdb/gdbserver/linux-sh-low.c
+++ b/gdb/gdbserver/linux-sh-low.c
@@ -77,6 +77,13 @@ sh_set_pc (struct regcache *regcache, CORE_ADDR pc)
 static const unsigned short sh_breakpoint = 0xc3c3;
 #define sh_breakpoint_len 2
 
+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 +155,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..35820fb 100644
--- a/gdb/gdbserver/linux-sparc-low.c
+++ b/gdb/gdbserver/linux-sparc-low.c
@@ -240,6 +240,12 @@ static const unsigned char sparc_breakpoint[INSN_SIZE] = {
 };
 #define sparc_breakpoint_len INSN_SIZE
 
+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 +329,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..86b433c 100644
--- a/gdb/gdbserver/linux-tic6x-low.c
+++ b/gdb/gdbserver/linux-tic6x-low.c
@@ -171,6 +171,14 @@ extern struct linux_target_ops the_low_target;
 
 static int *tic6x_regmap;
 static unsigned int tic6x_breakpoint;
+#define tic6x_breakpoint_len 4
+
+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 +255,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 +373,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..802812b 100644
--- a/gdb/gdbserver/linux-tile-low.c
+++ b/gdb/gdbserver/linux-tile-low.c
@@ -88,6 +88,13 @@ tile_set_pc (struct regcache *regcache, CORE_ADDR pc)
 static uint64_t tile_breakpoint = 0x400b3cae70166000ULL;
 #define tile_breakpoint_len 8
 
+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 +189,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..699eb4d 100644
--- a/gdb/gdbserver/linux-x86-low.c
+++ b/gdb/gdbserver/linux-x86-low.c
@@ -3243,6 +3243,13 @@ x86_emit_ops (void)
     return &i386_emit_ops;
 }
 
+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 +3268,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..1e2cf94 100644
--- a/gdb/gdbserver/linux-xtensa-low.c
+++ b/gdb/gdbserver/linux-xtensa-low.c
@@ -154,6 +154,13 @@ static struct regset_info xtensa_regsets[] = {
 static const unsigned char xtensa_breakpoint[] = XTENSA_BREAKPOINT;
 #define xtensa_breakpoint_len 2
 
+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 +241,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;
diff --git a/gdb/gdbserver/win32-arm-low.c b/gdb/gdbserver/win32-arm-low.c
index d4b2c6f..1b469a3 100644
--- a/gdb/gdbserver/win32-arm-low.c
+++ b/gdb/gdbserver/win32-arm-low.c
@@ -113,6 +113,13 @@ arm_arch_setup (void)
 static const unsigned long arm_wince_breakpoint = 0xe6000010;
 #define arm_wince_breakpoint_len 4
 
+static const unsigned char *
+arm_wince_breakpoint_from_pc (CORE_ADDR *pcptr, int *len)
+{
+  *len = arm_wince_breakpoint_len;
+  return (const unsigned char *) &arm_wince_breakpoint;
+}
+
 struct win32_target_ops the_low_target = {
   arm_arch_setup,
   sizeof (mappings) / sizeof (mappings[0]),
@@ -123,8 +130,7 @@ struct win32_target_ops the_low_target = {
   arm_fetch_inferior_register,
   arm_store_inferior_register,
   NULL, /* single_step */
-  (const unsigned char *) &arm_wince_breakpoint,
-  arm_wince_breakpoint_len,
+  arm_wince_breakpoint_from_pc,
   /* Watchpoint related functions.  See target.h for comments.  */
   NULL, /* supports_z_point_type */
   NULL, /* insert_point */
diff --git a/gdb/gdbserver/win32-i386-low.c b/gdb/gdbserver/win32-i386-low.c
index 7c22f05..b9d60a7 100644
--- a/gdb/gdbserver/win32-i386-low.c
+++ b/gdb/gdbserver/win32-i386-low.c
@@ -444,6 +444,13 @@ i386_store_inferior_register (struct regcache *regcache,
 static const unsigned char i386_win32_breakpoint = 0xcc;
 #define i386_win32_breakpoint_len 1
 
+static const unsigned char *
+i386_win32_breakpoint_from_pc (CORE_ADDR *pcptr, int *len)
+{
+  *len = i386_win32_breakpoint_len;
+  return (const unsigned char *) &i386_win32_breakpoint;
+}
+
 static void
 i386_arch_setup (void)
 {
@@ -466,8 +473,7 @@ struct win32_target_ops the_low_target = {
   i386_fetch_inferior_register,
   i386_store_inferior_register,
   i386_single_step,
-  &i386_win32_breakpoint,
-  i386_win32_breakpoint_len,
+  i386_win32_breakpoint_from_pc,
   i386_supports_z_point_type,
   i386_insert_point,
   i386_remove_point,
-- 
1.9.1

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

[Antoine Tremblay]

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

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

It also enables 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_length): New function.
	(arm_supports_z_point_type): Add software breakpoint support.
	(struct linux_target_ops): Add breakpoint_from_length operation.
---
 gdb/NEWS                      |  2 ++
 gdb/gdbserver/linux-arm-low.c | 31 +++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/gdb/NEWS b/gdb/NEWS
index f563b8c..85f234e 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.
+
 * Support for tracepoints on aarch64-linux was added in GDBserver.
 
 * The 'record instruction-history' command now indicates speculative execution
diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
index 15ecb70..9685db5 100644
--- a/gdb/gdbserver/linux-arm-low.c
+++ b/gdb/gdbserver/linux-arm-low.c
@@ -354,6 +354,28 @@ arm_breakpoint_from_pc (CORE_ADDR *pcptr, int *lenptr)
     }
 }
 
+/* Get the breakpoint from the remote length
+   2 is thumb-16
+   3 is thumb2-32
+   4 is arm
+*/
+static const unsigned char *
+arm_breakpoint_from_length (int *len)
+{
+  switch (*len) {
+  case 2:
+    return (unsigned char *) &thumb_breakpoint;
+  case 3:
+    *len = 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.  */
@@ -595,6 +617,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:
@@ -1006,6 +1029,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_length,
 };
 
 void
-- 
1.9.1

[PATCH 3/5] Add support for ARM breakpoint types in GDBServer.

[Antoine Tremblay]

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 and register values.

Also in order to share some code with GDB new files called
arm-common.h/c have been introduced in common/.

While these files do not contain much yet future patches will add more
to them 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-common.o.
	* arm-tdep.c (thumb_insn_size): Move to arm-common.h/c.
	* common/arm-common.c: New file.
	* common/arm-common.h: New file.
	* configure.tgt: Add arm-common.o.
	* gdbserver/Makefile.in: Add arm-common.c/o.
	* gdbserver/configure.srv: Add arm-common.o.
	* gdbserver/linux-arm-low.c (arm_breakpoint_at): Adjust for
        breakpoint types.
	(arm_breakpoint_from_pc): New function.
---
 gdb/Makefile.in               |  8 +++-
 gdb/arm-tdep.c                | 21 +---------
 gdb/common/arm-common.c       | 32 ++++++++++++++
 gdb/common/arm-common.h       | 33 +++++++++++++++
 gdb/configure.tgt             |  2 +-
 gdb/gdbserver/Makefile.in     |  6 ++-
 gdb/gdbserver/configure.srv   |  1 +
 gdb/gdbserver/linux-arm-low.c | 98 +++++++++++++++++++++++++++++++++----------
 8 files changed, 156 insertions(+), 45 deletions(-)
 create mode 100644 gdb/common/arm-common.c
 create mode 100644 gdb/common/arm-common.h

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 5659116..331c20b 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-common.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-common.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 \
@@ -2265,6 +2266,11 @@ btrace-common.o: ${srcdir}/common/btrace-common.c
 fileio.o: ${srcdir}/common/fileio.c
 	$(COMPILE) $(srcdir)/common/fileio.c
 	$(POSTCOMPILE)
+
+arm-common.o: ${srcdir}/common/arm-common.c
+	$(COMPILE) $(srcdir)/common/arm-common.c
+	$(POSTCOMPILE)
+
 #
 # gdb/target/ dependencies
 #
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index bcee29c..fcbd3ff 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -45,6 +45,7 @@
 #include "user-regs.h"
 #include "observer.h"
 
+#include "common/arm-common.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;
@@ -4361,18 +4354,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/common/arm-common.c b/gdb/common/arm-common.c
new file mode 100644
index 0000000..861b249
--- /dev/null
+++ b/gdb/common/arm-common.c
@@ -0,0 +1,32 @@
+/* Common code for generic ARM support.
+
+   Copyright (C) 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-common.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/common/arm-common.h b/gdb/common/arm-common.h
new file mode 100644
index 0000000..a6d44af
--- /dev/null
+++ b/gdb/common/arm-common.h
@@ -0,0 +1,33 @@
+/* Common code for generic ARM support.
+
+   Copyright (C) 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/>.  */
+
+#ifndef ARM_COMMON_H
+#define ARM_COMMON_H 1
+
+/* 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 /* ARM_COMMON_H */
diff --git a/gdb/configure.tgt b/gdb/configure.tgt
index c42b4df..e831f59 100644
--- a/gdb/configure.tgt
+++ b/gdb/configure.tgt
@@ -89,7 +89,7 @@ arm*-wince-pe | arm*-*-mingw32ce*)
 	;;
 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-common.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
 	;;
diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
index b715a32..6e51ccc 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)/common/arm-common.c
 
 DEPFILES = @GDBSERVER_DEPFILES@
 
@@ -576,6 +577,9 @@ waitstatus.o: ../target/waitstatus.c
 fileio.o: ../common/fileio.c
 	$(COMPILE) $<
 	$(POSTCOMPILE)
+arm-common.o: ../common/arm-common.c
+	$(COMPILE) $<
+	$(POSTCOMPILE)
 
 # Native object files rules from ../nat
 
diff --git a/gdb/gdbserver/configure.srv b/gdb/gdbserver/configure.srv
index aa232f8..1d10266 100644
--- a/gdb/gdbserver/configure.srv
+++ b/gdb/gdbserver/configure.srv
@@ -68,6 +68,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-common.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..15ecb70 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 "common/arm-common.h"
+
 /* Defined in auto-generated files.  */
 void init_registers_arm (void);
 extern const struct target_desc *tdesc_arm;
@@ -234,19 +236,28 @@ 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;
-static const unsigned short thumb2_breakpoint[] = { 0xf7f0, 0xa000 };
+#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)
+arm_is_thumb_mode (void)
 {
   struct regcache *regcache = get_thread_regcache (current_thread, 1);
   unsigned long cpsr;
@@ -254,6 +265,17 @@ arm_breakpoint_at (CORE_ADDR where)
   collect_register_by_name (regcache, "cpsr", &cpsr);
 
   if (cpsr & 0x20)
+      return 1;
+  else
+      return 0;
+}
+
+/* Returns 1 if there is a software breakpoint at location.  */
+
+static int
+arm_breakpoint_at (CORE_ADDR where)
+{
+  if (arm_is_thumb_mode ())
     {
       /* Thumb mode.  */
       unsigned short insn;
@@ -275,7 +297,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)
@@ -285,6 +307,53 @@ 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)
+{
+  /* Default if no pc is set to arm breakpoint.  */
+  if (pcptr == NULL)
+    {
+      *lenptr = arm_breakpoint_len;
+      return (unsigned char *) &arm_breakpoint;
+    }
+
+  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;
+
+	  (*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;
+      return (unsigned char *) &arm_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.  */
@@ -913,21 +982,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

[PATCH 4/5] Handle breakpoint kinds for software breakpoints in GDBServer.

[Antoine Tremblay]

In order to enable software breakpoints kinds, GDBServer needs to make use
of the kind parameter sent by the Z0 packet.

Using this kind parameter, GDBServer is able to write the right breakpoint
by asking the new operation called breakpoint_from_length to return
the breakpoint data for a specific kind.

Also, GDBServer always needs to handle that the software breakpoint size
as it can be coded as a kind field and thus always decode this to the real
length of the breakpoint when doing memory operations on the breakpoint
using check_breakpoint_from_length.

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_length): New function.
	* linux-low.h: Add breakpoint_from_length operation.
	* mem-break.c (check_breakpoint_from_length): New function.
	(insert_memory_breakpoint): Call check_breakpoint_from_length.
	(remove_memory_breakpoint): Likewise.
	(validate_inserted_breakpoint): Call check_breakpoint_from_length.
	(check_mem_read): Likewise.
	(check_mem_write): Likewise.
	* target.h (struct target_ops): Add breakpoint_from_length operation.
---
 gdb/gdbserver/linux-low.c | 12 +++++++++
 gdb/gdbserver/linux-low.h |  4 +++
 gdb/gdbserver/mem-break.c | 68 ++++++++++++++++++++++++++++++++++++++++-------
 gdb/gdbserver/target.h    |  4 +++
 4 files changed, 78 insertions(+), 10 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 06387a0..8e8b7c0 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -5965,6 +5965,17 @@ linux_supports_range_stepping (void)
   return (*the_low_target.supports_range_stepping) ();
 }
 
+static const unsigned char*
+linux_breakpoint_from_length (int *len)
+{
+  if (*the_low_target.breakpoint_from_length != NULL)
+    {
+      return (*the_low_target.breakpoint_from_length) (len);
+    }
+  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)
@@ -7042,6 +7053,7 @@ static struct target_ops linux_target_ops = {
   linux_mntns_unlink,
   linux_mntns_readlink,
   linux_breakpoint_from_pc,
+  linux_breakpoint_from_length
 };
 
 static void
diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
index c623150..bb56584 100644
--- a/gdb/gdbserver/linux-low.h
+++ b/gdb/gdbserver/linux-low.h
@@ -228,6 +228,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 length can have target
+     specific meaning like the z0 kind parameter */
+  const unsigned char *(*breakpoint_from_length) (int *length);
 };
 
 extern struct linux_target_ops the_low_target;
diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index 68e14c3..dca228c 100644
--- a/gdb/gdbserver/mem-break.c
+++ b/gdb/gdbserver/mem-break.c
@@ -295,6 +295,38 @@ find_raw_breakpoint_at (CORE_ADDR addr, enum raw_bkpt_type type, int size)
   return NULL;
 }
 
+/* Check if if we can insert the breakpoint of size, try to resolve the real
+   breakpoint size from an architecture specific encoded size if needed.  */
+
+static int
+check_breakpoint_from_length (struct raw_breakpoint *bp,
+			      const unsigned char **breakpoint_data,
+			      int *breakpoint_len)
+{
+  /* 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)
+    {
+      /* Get the arch dependent breakpoint */
+      if (*the_target->breakpoint_from_length != NULL)
+	{
+	  /* Update magic coded size to the right size if needed.  */
+	  int size = bp->size;
+	  *breakpoint_data =
+	    (*the_target->breakpoint_from_length) (&size);
+	  *breakpoint_len = size;
+	}
+      else {
+	if (debug_threads)
+	  debug_printf ("Don't know breakpoints of size %d.\n",
+                     bp->size);
+	return -1;
+      }
+    }
+
+  return 1;
+}
 /* See mem-break.h.  */
 
 int
@@ -312,16 +344,9 @@ insert_memory_breakpoint (struct raw_breakpoint *bp)
   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 (debug_threads)
-	debug_printf ("Don't know how to insert breakpoints of size %d.\n",
-		      bp->size);
-      return -1;
-    }
+  if ((err = check_breakpoint_from_length (bp, &breakpoint_data,
+					   &breakpoint_len)) < 0)
+    return err;
 
   /* Note that there can be fast tracepoint jumps installed in the
      same memory range, so to get at the original memory, we need to
@@ -356,6 +381,7 @@ int
 remove_memory_breakpoint (struct raw_breakpoint *bp)
 {
   unsigned char buf[MAX_BREAKPOINT_LEN];
+  const unsigned char* breakpoint_data;
   int err;
   int breakpoint_len;
   CORE_ADDR pc;
@@ -363,6 +389,10 @@ remove_memory_breakpoint (struct raw_breakpoint *bp)
   pc = bp->pcfull;
   the_target->breakpoint_from_pc (&pc, &breakpoint_len);
 
+  if ((err = check_breakpoint_from_length (bp, &breakpoint_data,
+					   &breakpoint_len)) < 0)
+    return err;
+
   /* Since there can be trap breakpoints inserted in the same address
      range, we use `write_inferior_memory', which takes care of
      layering breakpoints on top of fast tracepoints, and on top of
@@ -1693,6 +1723,14 @@ validate_inserted_breakpoint (struct raw_breakpoint *bp)
 
   breakpoint_data = the_target->breakpoint_from_pc (&raw_pc, &breakpoint_len);
 
+  if ((err = check_breakpoint_from_length (bp, &breakpoint_data,
+				    &breakpoint_len)) < 0)
+    {
+      /* Tag it as gone.  */
+      bp->inserted = -1;
+      return 0;
+    }
+
   gdb_assert (bp->inserted);
   gdb_assert (bp->raw_type == raw_bkpt_type_sw);
 
@@ -1791,11 +1829,16 @@ check_mem_read (CORE_ADDR mem_addr, unsigned char *buf, int mem_len)
     {
       int breakpoint_len;
       CORE_ADDR raw_pc;
+      const unsigned char* breakpoint_data;
       CORE_ADDR bp_end, start, end;
       int copy_offset, copy_len, buf_offset;
 
       raw_pc = bp->pcfull;
+
       the_target->breakpoint_from_pc (&raw_pc, &breakpoint_len);
+      check_breakpoint_from_length (bp, &breakpoint_data,
+				    &breakpoint_len);
+
       bp_end = bp->pc + breakpoint_len;
 
       if (bp->raw_type != raw_bkpt_type_sw)
@@ -1890,8 +1933,13 @@ check_mem_write (CORE_ADDR mem_addr, unsigned char *buf,
       int copy_offset, copy_len, buf_offset;
 
       raw_pc = bp->pcfull;
+
       breakpoint_data =
 	the_target->breakpoint_from_pc (&raw_pc, &breakpoint_len);
+
+      check_breakpoint_from_length (bp, &breakpoint_data,
+				    &breakpoint_len);
+
       bp_end = bp->pc + breakpoint_len;
 
       if (bp->raw_type != raw_bkpt_type_sw)
diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
index 603819e..bf4c3c7 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 length, the length can have target specific
+     meaning like the z0 kind parameter.  */
+  const unsigned char *(*breakpoint_from_length) (int *len);
 };
 
 extern struct target_ops *the_target;
-- 
1.9.1

Re: [PATCH 1/5] Support multiple breakpoint types per target in GDBServer.

[Yao Qi]

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

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

Please add comment like this to all these $ARCH_breakpoint_from_pc functions.

/* Implementation of linux_target_ops method "breakpoint_from_pc".  */

> diff --git a/gdb/gdbserver/linux-cris-low.c b/gdb/gdbserver/linux-cris-low.c
> index e0bfa1a..da5876d 100644
> --- a/gdb/gdbserver/linux-cris-low.c
> +++ b/gdb/gdbserver/linux-cris-low.c
> @@ -62,7 +62,7 @@ cris_cannot_fetch_register (int regno)
>  extern int debug_threads;
>  
>  static CORE_ADDR
> -cris_get_pc (struct regcache *regcache, void)
> +cris_get_pc (struct regcache *regcache)
>  {
>    unsigned long pc;
>    collect_register_by_name (regcache, "pc", &pc);

This is a unrelated change.  Please move it out this patch, and submit
it separately.

> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index f5b64ab..bb08761 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);
>  

There was a problem here and your patch doesn't fix it.  I want to raise
it here first.  It is incorrect to get increment_pc by
the_low_target.breakpoint_len or (*the_low_target.breakpoint_from_pc)
for arm/thumb, because given the stop_pc, we can't tell the breakpoint
size (2-byte or 4-byte).  We need a new target hook, say
breakpoint_from_current_state, and its default implementation is
breakpoint_from_pc.  However, its arm implementation checks whether
the program is in thumb mode by CPSR and return the right breakpoint size.

IIUC, the code here is used for step-over GDBserver breakpoint, so it is
not used for arm target until we support conditional breakpoint or
tracepoint, but we should fix it before supporting conditional
breakpoint and tracepoint for arm target.

>        if (debug_threads)
>  	{
> @@ -6932,6 +6936,17 @@ 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);
> +    }

"{" and "}" is not needed.

> diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
> index f8f6e78..c623150 100644
> --- a/gdb/gdbserver/linux-low.h
> +++ b/gdb/gdbserver/linux-low.h
> @@ -141,8 +141,13 @@ 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.  Note that the PC

s/PC/*PCPTR/

> +     can be NULL, the default breakpoint for the target should be returned in

PC can't be NULL after your patch #2.  You can remove the second
sentence in this patch or patch #2.

> +     this case. The PC 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);
> +

-- 
Yao (齐尧)

Re: [PATCH 1/5] Support multiple breakpoint types per target in GDBServer.

[Antoine Tremblay]

On 09/23/2015 06:51 AM, Yao Qi wrote:
> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>> +static const unsigned char *
>> +aarch64_breakpoint_from_pc (CORE_ADDR *pcptr, int *len)
>
> Please add comment like this to all these $ARCH_breakpoint_from_pc functions.
>
> /* Implementation of linux_target_ops method "breakpoint_from_pc".  */
>

Done

>> diff --git a/gdb/gdbserver/linux-cris-low.c b/gdb/gdbserver/linux-cris-low.c
>> index e0bfa1a..da5876d 100644
>> --- a/gdb/gdbserver/linux-cris-low.c
>> +++ b/gdb/gdbserver/linux-cris-low.c
>> @@ -62,7 +62,7 @@ cris_cannot_fetch_register (int regno)
>>   extern int debug_threads;
>>
>>   static CORE_ADDR
>> -cris_get_pc (struct regcache *regcache, void)
>> +cris_get_pc (struct regcache *regcache)
>>   {
>>     unsigned long pc;
>>     collect_register_by_name (regcache, "pc", &pc);
>
> This is a unrelated change.  Please move it out this patch, and submit
> it separately.
>

Done

>> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
>> index f5b64ab..bb08761 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);
>>
>
> There was a problem here and your patch doesn't fix it.  I want to raise
> it here first.  It is incorrect to get increment_pc by
> the_low_target.breakpoint_len or (*the_low_target.breakpoint_from_pc)
> for arm/thumb, because given the stop_pc, we can't tell the breakpoint
> size (2-byte or 4-byte).We need a new target hook, say
> breakpoint_from_current_state, and its default implementation is
> breakpoint_from_pc.  However, its arm implementation checks whether
> the program is in thumb mode by CPSR and return the right breakpoint size.
>
> IIUC, the code here is used for step-over GDBserver breakpoint, so it is
> not used for arm target until we support conditional breakpoint or
> tracepoint, but we should fix it before supporting conditional
> breakpoint and tracepoint for arm target.
>

Indeed good point. It's too bad another target hook needs to be there 
for this but there's no choice I think.

I will fix it in my next patchset introducing conditional breakpoints.

>>         if (debug_threads)
>>   	{
>> @@ -6932,6 +6936,17 @@ 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);
>> +    }
>
> "{" and "}" is not needed.

Done

>
>> diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
>> index f8f6e78..c623150 100644
>> --- a/gdb/gdbserver/linux-low.h
>> +++ b/gdb/gdbserver/linux-low.h
>> @@ -141,8 +141,13 @@ 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.  Note that the PC
>
> s/PC/*PCPTR/
>
>> +     can be NULL, the default breakpoint for the target should be returned in
>
> PC can't be NULL after your patch #2.  You can remove the second
> sentence in this patch or patch #2.
>
I think you mean after patch #3 ?

But it can still be NULL see in #3 :

/* Default if no pc is set to arm breakpoint.  */
+  if (pcptr == NULL)
+    {
+      *lenptr = arm_breakpoint_len;
+      return (unsigned char *) &arm_breakpoint;
+    }

>> +     this case. The PC 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);
>> +
>

Note also that I removed the win32 changes, they were not needed and 
broken, I can't test win32 properly anyway.

Re: [PATCH 1/5] Support multiple breakpoint types per target in GDBServer.

[Yao Qi]

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

>> PC can't be NULL after your patch #2.  You can remove the second
>> sentence in this patch or patch #2.
>>
> I think you mean after patch #3 ?
>
> But it can still be NULL see in #3 :
>
> /* Default if no pc is set to arm breakpoint.  */
> +  if (pcptr == NULL)
> +    {
> +      *lenptr = arm_breakpoint_len;
> +      return (unsigned char *) &arm_breakpoint;
> +    }

I meant this change below in patch #2,

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index bb08761..06387a0 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -7069,16 +7069,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 ();

We only pass NULL to breakpoint_from_pc here, and we remove it from
patch #2.  That is why I suggest that PCPTR can't be NULL.

[I am still reviewing this patch series.  I get something I can't
explain after I enable thread event breakpoint in order to exercise
your patches.  I'll send out my comments once I understand the them
fully.]

-- 
Yao (齐尧)

Re: [PATCH 1/5] Support multiple breakpoint types per target in GDBServer.

[Antoine Tremblay]

On 09/23/2015 10:46 AM, Yao Qi wrote:
> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>>> PC can't be NULL after your patch #2.  You can remove the second
>>> sentence in this patch or patch #2.
>>>
>> I think you mean after patch #3 ?
>>
>> But it can still be NULL see in #3 :
>>
>> /* Default if no pc is set to arm breakpoint.  */
>> +  if (pcptr == NULL)
>> +    {
>> +      *lenptr = arm_breakpoint_len;
>> +      return (unsigned char *) &arm_breakpoint;
>> +    }
>
> I meant this change below in patch #2,
>
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index bb08761..06387a0 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -7069,16 +7069,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 ();
>
> We only pass NULL to breakpoint_from_pc here, and we remove it from
> patch #2.  That is why I suggest that PCPTR can't be NULL.
>

Ok I see, indeed it is never called again with NULL, good point I'll 
remove that and also remove the default NULL handling in the arm 
implementation.


> [I am still reviewing this patch series.  I get something I can't
> explain after I enable thread event breakpoint in order to exercise
> your patches.  I'll send out my comments once I understand the them
> fully.]
>

Ok thank you, note I'm on IRC as hexa if you ever need some quick answer.

Re: [PATCH 2/5] Make breakpoint and breakpoint_len local variables in GDBServer.

[Yao Qi]

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

> @@ -774,8 +800,9 @@ set_breakpoint_at (CORE_ADDR where, int (*handler) (CORE_ADDR))
>  {
>    int err_ignored;
>  
> +  /* default breakpoint_len will be initialized downstream.  */
>    return set_breakpoint (other_breakpoint, raw_bkpt_type_sw,
> -			 where, breakpoint_len, handler,
> +			 where, 0, handler,
>  			 &err_ignored);
>  }

Why do you set breakpoint length to zero?  I know you'll set the length
downstream properly, but we should compute the breakpoint length here.

After thinking about it a bit more, I think this reveals some design
issues in GDBserver brekapoint, nowadays, GDBserver inserts its own
breakpoints and breakpoints requested by GDB.  After this patch series,
GDBserver should be able to:

  1) choose the right breakpoint instruction for its own breakpoints,
  according to the breakpoint address, status register flag, etc,
  through API set_breakpoint_at,
  2) choose the right breakpoint instruction for breakpoints requested
  by GDB, according to the information in Z packets, through API
  set_gdb_breakpoint

there should be two paths for them, and each path needs different target
hook to choose breakpoint instructions.  breakpoint_from_pc is needed for
#1, and breakpoint_from_length is needed for #2.  In your current patch
set (in patch 4/5), two things are mixed together, which doesn't look
good to me.  The current functions calls in GDBserver to create
breakpoint is like

  set_breakpoint_at
  set_gdb_breakpoint_1
     |
     `--> set_breakpoint
             |
             `-->set_raw_breakpoint_at
                    |
                    `--> the_target->insert_point

we are handling the breakpoint length at the lowest level, in
insert_memory_breakpoint, and we use breakpoint_from_pc and
breakpoint_from_length together there, which looks unclean.  Ideally, we
can move these code handling breakpoint length (breakpoint_from_pc and
breakpoint_from_length) to upper levels, like this,

  set_breakpoint_at (call breakpoint_from_pc)
  set_gdb_breakpoint_1 (call breakpoint_from_length)
     |
     `--> set_breakpoint
             |
             `-->set_raw_breakpoint_at
                    |
                    `--> the_target->insert_point

all needed information is saved in struct breakpoint or struct
raw_breakpoint, and function set_breakpoint and it callees can use
breakpoint or raw_breakpoint directly.  It'll be cleaner in this way,
let me know what do you think?

-- 
Yao (齐尧)

Re: [PATCH 2/5] Make breakpoint and breakpoint_len local variables in GDBServer.

[Eli Zaretskii]

> From: Yao Qi <qiyaoltc@gmail.com>
> Cc: <gdb-patches@sourceware.org>
> Date: Mon, 28 Sep 2015 10:54:48 +0100
> 
> After thinking about it a bit more, I think this reveals some design
> issues in GDBserver brekapoint, nowadays, GDBserver inserts its own
> breakpoints and breakpoints requested by GDB.  After this patch series,
> GDBserver should be able to:
> 
>   1) choose the right breakpoint instruction for its own breakpoints,
>   according to the breakpoint address, status register flag, etc,
>   through API set_breakpoint_at,
>   2) choose the right breakpoint instruction for breakpoints requested
>   by GDB, according to the information in Z packets, through API
>   set_gdb_breakpoint
> 
> there should be two paths for them, and each path needs different target
> hook to choose breakpoint instructions.  breakpoint_from_pc is needed for
> #1, and breakpoint_from_length is needed for #2.  In your current patch
> set (in patch 4/5), two things are mixed together, which doesn't look
> good to me.  The current functions calls in GDBserver to create
> breakpoint is like
> 
>   set_breakpoint_at
>   set_gdb_breakpoint_1
>      |
>      `--> set_breakpoint
>              |
>              `-->set_raw_breakpoint_at
>                     |
>                     `--> the_target->insert_point
> 
> we are handling the breakpoint length at the lowest level, in
> insert_memory_breakpoint, and we use breakpoint_from_pc and
> breakpoint_from_length together there, which looks unclean.  Ideally, we
> can move these code handling breakpoint length (breakpoint_from_pc and
> breakpoint_from_length) to upper levels, like this,
> 
>   set_breakpoint_at (call breakpoint_from_pc)
>   set_gdb_breakpoint_1 (call breakpoint_from_length)
>      |
>      `--> set_breakpoint
>              |
>              `-->set_raw_breakpoint_at
>                     |
>                     `--> the_target->insert_point
> 
> all needed information is saved in struct breakpoint or struct
> raw_breakpoint, and function set_breakpoint and it callees can use
> breakpoint or raw_breakpoint directly.  It'll be cleaner in this way,
> let me know what do you think?

Sometimes only the target layer knows how to choose the length
correctly.  Are we sure this isn't one of those cases, and can never
be?

Re: [PATCH 3/5] Add support for ARM breakpoint types in GDBServer.

[Yao Qi]

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

> gdb/ChangeLog:
> 	* Makefile.in: Add arm-common.o.
> 	* arm-tdep.c (thumb_insn_size): Move to arm-common.h/c.

"Move to arm-common.c".  Some macros are moved, we should mention the
move here as well.

> 	* common/arm-common.c: New file.
> 	* common/arm-common.h: New file.

arm-common.c is not a good file name to me, how about arm-insn.c?
Please move this file to arch/ directory rather than common/

> 	* configure.tgt: Add arm-common.o.
> 	* gdbserver/Makefile.in: Add arm-common.c/o.

ChangeLog entries for GDBserver should be written separately.

> 	* gdbserver/configure.srv: Add arm-common.o.
> 	* gdbserver/linux-arm-low.c (arm_breakpoint_at): Adjust for
>         breakpoint types.
> 	(arm_breakpoint_from_pc): New function.

arm_breakpoint_from_pc is not a new function, but arm_is_thumb_mode is.

> diff --git a/gdb/common/arm-common.c b/gdb/common/arm-common.c
> new file mode 100644
> index 0000000..861b249
> --- /dev/null
> +++ b/gdb/common/arm-common.c
> @@ -0,0 +1,32 @@
> +/* Common code for generic ARM support.
> +
> +   Copyright (C) 2015 Free Software Foundation, Inc.
> +

Contents in file are moved from existing file, so we should still keep
the year range of the original file.  It should be

     Copyright (C) 1988-2015 Free Software Foundation, Inc.


> diff --git a/gdb/configure.tgt b/gdb/configure.tgt
> index c42b4df..e831f59 100644
> --- a/gdb/configure.tgt
> +++ b/gdb/configure.tgt
> @@ -89,7 +89,7 @@ arm*-wince-pe | arm*-*-mingw32ce*)
>  	;;
>  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-common.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

since arm-common.o is moved out of arm-tdep.o, so it should be added to
every target which uses arm-tdep.o, such as aarch64*-*-linux*,
arm*-wince-pe, etc.

> diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
> index 367c704..15ecb70 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 "common/arm-common.h"
> +
>  /* Defined in auto-generated files.  */
>  void init_registers_arm (void);
>  extern const struct target_desc *tdesc_arm;
> @@ -234,19 +236,28 @@ 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;
> -static const unsigned short thumb2_breakpoint[] = { 0xf7f0, 0xa000 };
> +#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

I am confused by your changes here.  Why do you change them?

>  
>  static int
> -arm_breakpoint_at (CORE_ADDR where)
> +arm_is_thumb_mode (void)
>  {
>    struct regcache *regcache = get_thread_regcache (current_thread, 1);
>    unsigned long cpsr;
> @@ -254,6 +265,17 @@ arm_breakpoint_at (CORE_ADDR where)
>    collect_register_by_name (regcache, "cpsr", &cpsr);
>  
>    if (cpsr & 0x20)
> +      return 1;
> +  else
> +      return 0;

Indentation looks odd.

> +}
> +
> +/* Returns 1 if there is a software breakpoint at location.  */
> +
> +static int
> +arm_breakpoint_at (CORE_ADDR where)
> +{
> +  if (arm_is_thumb_mode ())
>      {
>        /* Thumb mode.  */
>        unsigned short insn;
> @@ -275,7 +297,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)
> @@ -285,6 +307,53 @@ 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)
> +{
> +  /* Default if no pc is set to arm breakpoint.  */
> +  if (pcptr == NULL)

Such NULL checking is no longer needed, right?

-- 
Yao (齐尧)

Re: [PATCH 4/5] Handle breakpoint kinds 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..bf4c3c7 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 length, the length can have target specific
> +     meaning like the z0 kind parameter.  */
> +  const unsigned char *(*breakpoint_from_length) (int *len);
>  };

If you agree on my comments to patch 2/5, we can rename this hook to
breakpoint_from_z0_kind.

-- 
Yao (齐尧)

Re: [PATCH 2/5] Make breakpoint and breakpoint_len local variables in GDBServer.

[Antoine Tremblay]

On 09/28/2015 05:54 AM, Yao Qi wrote:
> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>> @@ -774,8 +800,9 @@ set_breakpoint_at (CORE_ADDR where, int (*handler) (CORE_ADDR))
>>   {
>>     int err_ignored;
>>
>> +  /* default breakpoint_len will be initialized downstream.  */
>>     return set_breakpoint (other_breakpoint, raw_bkpt_type_sw,
>> -			 where, breakpoint_len, handler,
>> +			 where, 0, handler,
>>   			 &err_ignored);
>>   }
>
> Why do you set breakpoint length to zero?  I know you'll set the length
> downstream properly, but we should compute the breakpoint length here.
>
That's what I thought too initially but somehow that intention got lost 
a long the way.

> After thinking about it a bit more, I think this reveals some design
> issues in GDBserver brekapoint, nowadays, GDBserver inserts its own
> breakpoints and breakpoints requested by GDB.  After this patch series,
> GDBserver should be able to:
>
>    1) choose the right breakpoint instruction for its own breakpoints,
>    according to the breakpoint address, status register flag, etc,
>    through API set_breakpoint_at,
>    2) choose the right breakpoint instruction for breakpoints requested
>    by GDB, according to the information in Z packets, through API
>    set_gdb_breakpoint
>

Indeed, you need to consider also the case of tracepoints as they will 
set a third case where you set a GDBServer breakpoint with a z0 like 
kind field for tracepoints. But it could be done with another call like 
set_breakpoint_at_with_length or with_kind.

Also the HW breakpoint case is already is using where and size as pcfull 
and kind in insert_point so we need to keep the meaning of those 
arguments from set_breakpoint_at and gdb_set_breakpoint.

> there should be two paths for them, and each path needs different target
> hook to choose breakpoint instructions.  breakpoint_from_pc is needed for
> #1, and breakpoint_from_length is needed for #2.  In your current patch
> set (in patch 4/5), two things are mixed together, which doesn't look
> good to me.

I do agree that seems weird.

> breakpoint is like
>
>    set_breakpoint_at
>    set_gdb_breakpoint_1
>       |
>       `--> set_breakpoint
>               |
>               `-->set_raw_breakpoint_at
>                      |
>                      `--> the_target->insert_point
>
> we are handling the breakpoint length at the lowest level, in
> insert_memory_breakpoint, and we use breakpoint_from_pc and
> breakpoint_from_length together there, which looks unclean.  Ideally, we
> can move these code handling breakpoint length (breakpoint_from_pc and
> breakpoint_from_length) to upper levels, like this,
>
>    set_breakpoint_at (call breakpoint_from_pc)
>    set_gdb_breakpoint_1 (call breakpoint_from_length)
>       |
>       `--> set_breakpoint
>               |
>               `-->set_raw_breakpoint_at
>                      |
>                      `--> the_target->insert_point
>
> all needed information is saved in struct breakpoint or struct
> raw_breakpoint, and function set_breakpoint and it callees can use
> breakpoint or raw_breakpoint directly.  It'll be cleaner in this way,
> let me know what do you think?
>
Indeed this is a very good point, seems obvious now.

I've redone the whole patch sets up to tracepoints with this with no 
issues..

So look forward to this change in v2 ..

Here's what this patch will look like with the change :

Does that look better ?

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 200ea94..cc42f36 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -7067,16 +7067,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 9356741..5e5128e 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;

@@ -301,24 +309,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 +329,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 +358,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 +375,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 +397,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 +406,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 +744,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 +778,16 @@ 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);
+
+  /* default breakpoint_len will be initialized downstream.  */
    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);
  }


@@ -929,6 +940,9 @@ set_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, 
int size, int *err)
    struct breakpoint *bp;
    enum bkpt_type type;
    enum raw_bkpt_type raw_type;
+  const unsigned char *breakpoint_data;
+  int breakpoint_len = size;
+  CORE_ADDR pc = addr;

    /* If we see GDB inserting a second code breakpoint at the same
       address, then either: GDB is updating the breakpoint's conditions
@@ -993,7 +1007,22 @@ 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);
+
+  if (z_type == Z_PACKET_SW_BP)
+    {
+      breakpoint_data = the_target->breakpoint_from_pc (&pc, 
&breakpoint_len);
+
+      if (breakpoint_len != size)
+	{
+	  if (debug_threads)
+	    debug_printf ("Don't know how to insert breakpoint of size %d.\n",
+		      size);
+	  return NULL;
+	}
+    }
+
+  return set_breakpoint (type, raw_type, addr, pc, breakpoint_data, size,
+			 breakpoint_len, NULL, err);
  }

  static int
@@ -1588,13 +1617,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 +1691,9 @@ validate_inserted_breakpoint (struct 
raw_breakpoint *bp)
    gdb_assert (bp->inserted);
    gdb_assert (bp->raw_type == raw_bkpt_type_sw);

-  buf = alloca (breakpoint_len);
-  err = (*the_target->read_memory) (bp->pc, buf, breakpoint_len);
-  if (err || memcmp (buf, breakpoint_data, breakpoint_len) != 0)
+  buf = 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,10 +1784,11 @@ 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 start, end;
+      CORE_ADDR bp_end, start, end;
        int copy_offset, copy_len, buf_offset;

+      bp_end = bp->pc + bp->size;
+
        if (bp->raw_type != raw_bkpt_type_sw)
  	continue;

@@ -1851,10 +1874,11 @@ 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 start, end;
+      CORE_ADDR bp_end, start, end;
        int copy_offset, copy_len, buf_offset;

+      bp_end = bp->pc + bp->size;
+
        if (bp->raw_type != raw_bkpt_type_sw)
  	continue;

@@ -1882,7 +1906,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 +1987,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;

Re: [PATCH 2/5] Make breakpoint and breakpoint_len local variables in GDBServer.

[Antoine Tremblay]

On 09/28/2015 06:05 AM, Eli Zaretskii wrote:
>> From: Yao Qi <qiyaoltc@gmail.com>
>> Cc: <gdb-patches@sourceware.org>
>> Date: Mon, 28 Sep 2015 10:54:48 +0100
>>
>> After thinking about it a bit more, I think this reveals some design
>> issues in GDBserver brekapoint, nowadays, GDBserver inserts its own
>> breakpoints and breakpoints requested by GDB.  After this patch series,
>> GDBserver should be able to:
>>
>>    1) choose the right breakpoint instruction for its own breakpoints,
>>    according to the breakpoint address, status register flag, etc,
>>    through API set_breakpoint_at,
>>    2) choose the right breakpoint instruction for breakpoints requested
>>    by GDB, according to the information in Z packets, through API
>>    set_gdb_breakpoint
>>
>> there should be two paths for them, and each path needs different target
>> hook to choose breakpoint instructions.  breakpoint_from_pc is needed for
>> #1, and breakpoint_from_length is needed for #2.  In your current patch
>> set (in patch 4/5), two things are mixed together, which doesn't look
>> good to me.  The current functions calls in GDBserver to create
>> breakpoint is like
>>
>>    set_breakpoint_at
>>    set_gdb_breakpoint_1
>>       |
>>       `--> set_breakpoint
>>               |
>>               `-->set_raw_breakpoint_at
>>                      |
>>                      `--> the_target->insert_point
>>
>> we are handling the breakpoint length at the lowest level, in
>> insert_memory_breakpoint, and we use breakpoint_from_pc and
>> breakpoint_from_length together there, which looks unclean.  Ideally, we
>> can move these code handling breakpoint length (breakpoint_from_pc and
>> breakpoint_from_length) to upper levels, like this,
>>
>>    set_breakpoint_at (call breakpoint_from_pc)
>>    set_gdb_breakpoint_1 (call breakpoint_from_length)
>>       |
>>       `--> set_breakpoint
>>               |
>>               `-->set_raw_breakpoint_at
>>                      |
>>                      `--> the_target->insert_point
>>
>> all needed information is saved in struct breakpoint or struct
>> raw_breakpoint, and function set_breakpoint and it callees can use
>> breakpoint or raw_breakpoint directly.  It'll be cleaner in this way,
>> let me know what do you think?
>
> Sometimes only the target layer knows how to choose the length
> correctly.  Are we sure this isn't one of those cases, and can never
> be?
>

breakpoint_from_pc and breakpoint_from_length are target ops so indeed 
only the target knows but it's ok we can query the target properly.

Re: [PATCH 3/5] Add support for ARM breakpoint types in GDBServer.

[Antoine Tremblay]

On 09/28/2015 06:29 AM, Yao Qi wrote:
> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>> gdb/ChangeLog:
>> 	* Makefile.in: Add arm-common.o.
>> 	* arm-tdep.c (thumb_insn_size): Move to arm-common.h/c.
>
> "Move to arm-common.c".  Some macros are moved, we should mention the
> move here as well.

Done.

>
>> 	* common/arm-common.c: New file.
>> 	* common/arm-common.h: New file.
>
> arm-common.c is not a good file name to me, how about arm-insn.c?

This will contain more stuff as I post the next patch sets for single 
stepping etc.. I would like to keep a more general name.

It's basically all coming from arm-tdep.c but I don't want to name it 
common/arm-tdep.c to avoid confusion and Makefile problems.

arm-ctdep.c ?

> Please move this file to arch/ directory rather than common/

It seems weird to me to have common objects somewhere else then in 
common/ , if common becomes more a lib we don't want it all over the 
place no ?

Would creating a common/arch be acceptable ? I guess we would have to 
move things like x86-xstate.h etc.. there too ?

>
>> 	* configure.tgt: Add arm-common.o.
>> 	* gdbserver/Makefile.in: Add arm-common.c/o.
>
> ChangeLog entries for GDBserver should be written separately.

Oops my bad sorry about that.

>
>> 	* gdbserver/configure.srv: Add arm-common.o.
>> 	* gdbserver/linux-arm-low.c (arm_breakpoint_at): Adjust for
>>          breakpoint types.
>> 	(arm_breakpoint_from_pc): New function.
>
> arm_breakpoint_from_pc is not a new function, but arm_is_thumb_mode is.
>

Done.

>> diff --git a/gdb/common/arm-common.c b/gdb/common/arm-common.c
>> new file mode 100644
>> index 0000000..861b249
>> --- /dev/null
>> +++ b/gdb/common/arm-common.c
>> @@ -0,0 +1,32 @@
>> +/* Common code for generic ARM support.
>> +
>> +   Copyright (C) 2015 Free Software Foundation, Inc.
>> +
>
> Contents in file are moved from existing file, so we should still keep
> the year range of the original file.  It should be
>
>       Copyright (C) 1988-2015 Free Software Foundation, Inc.
>
>
OK np

>> diff --git a/gdb/configure.tgt b/gdb/configure.tgt
>> index c42b4df..e831f59 100644
>> --- a/gdb/configure.tgt
>> +++ b/gdb/configure.tgt
>> @@ -89,7 +89,7 @@ arm*-wince-pe | arm*-*-mingw32ce*)
>>   	;;
>>   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-common.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
>
> since arm-common.o is moved out of arm-tdep.o, so it should be added to
> every target which uses arm-tdep.o, such as aarch64*-*-linux*,
> arm*-wince-pe, etc.

Even if they don't use it ? But ok.

>
>> diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
>> index 367c704..15ecb70 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 "common/arm-common.h"
>> +
>>   /* Defined in auto-generated files.  */
>>   void init_registers_arm (void);
>>   extern const struct target_desc *tdesc_arm;
>> @@ -234,19 +236,28 @@ 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;
>> -static const unsigned short thumb2_breakpoint[] = { 0xf7f0, 0xa000 };
>> +#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
>
> I am confused by your changes here.  Why do you change them?
>

Before arm_breakpoint_from_pc would be :

#ifndef __ARM_EABI__
   return (const unsigned char *) &arm_breakpoint;
#else
-  return (const unsigned char *) &arm_eabi_breakpoint;
#endif

And arm_breakpoint_at would check for the arm_breakpoint || 
arm_eabi_breakpoint.

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

It felt more clear to me to name 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 breakpoint_from_pc as it just returns 
arm_breakpoint rather than having the #ifdef in that function.

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


>>
>>   static int
>> -arm_breakpoint_at (CORE_ADDR where)
>> +arm_is_thumb_mode (void)
>>   {
>>     struct regcache *regcache = get_thread_regcache (current_thread, 1);
>>     unsigned long cpsr;
>> @@ -254,6 +265,17 @@ arm_breakpoint_at (CORE_ADDR where)
>>     collect_register_by_name (regcache, "cpsr", &cpsr);
>>
>>     if (cpsr & 0x20)
>> +      return 1;
>> +  else
>> +      return 0;
>
> Indentation looks odd.

Done.

>
>> +}
>> +
>> +/* Returns 1 if there is a software breakpoint at location.  */
>> +
>> +static int
>> +arm_breakpoint_at (CORE_ADDR where)
>> +{
>> +  if (arm_is_thumb_mode ())
>>       {
>>         /* Thumb mode.  */
>>         unsigned short insn;
>> @@ -275,7 +297,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)
>> @@ -285,6 +307,53 @@ 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)
>> +{
>> +  /* Default if no pc is set to arm breakpoint.  */
>> +  if (pcptr == NULL)
>
> Such NULL checking is no longer needed, right?
>

Indeed this is gone based on comments in patch 1/5.

Re: [PATCH 3/5] Add support for ARM breakpoint types in GDBServer.

[Yao Qi]

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

> This will contain more stuff as I post the next patch sets for single
> stepping etc.. I would like to keep a more general name.
>
> It's basically all coming from arm-tdep.c but I don't want to name it
> common/arm-tdep.c to avoid confusion and Makefile problems.
>
> arm-ctdep.c ?
>

How about arm.c?

>> Please move this file to arch/ directory rather than common/
>
> It seems weird to me to have common objects somewhere else then in
> common/ , if common becomes more a lib we don't want it all over the
> place no ?
>
> Would creating a common/arch be acceptable ? I guess we would have to
> move things like x86-xstate.h etc.. there too ?

common/ was created in order to share code between GDB and GDBserver, see
https://sourceware.org/gdb/wiki/Common After that, we find that we'll
end up with moving most of gdb files into common/ and we'd better move
different common things into different common directory.  Nowadays, we
have nat/ common/ and arch/ directories for common things.  arm.c should
be put in arch/ directory.

>>> diff --git a/gdb/configure.tgt b/gdb/configure.tgt
>>> index c42b4df..e831f59 100644
>>> --- a/gdb/configure.tgt
>>> +++ b/gdb/configure.tgt
>>> @@ -89,7 +89,7 @@ arm*-wince-pe | arm*-*-mingw32ce*)
>>>   	;;
>>>   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-common.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
>>
>> since arm-common.o is moved out of arm-tdep.o, so it should be added to
>> every target which uses arm-tdep.o, such as aarch64*-*-linux*,
>> arm*-wince-pe, etc.
>
> Even if they don't use it ? But ok.
>

No, they use it.  This patch moves thumb_insn_size to arm-common.o, but
thumb_insn_size is used by arm-tdep.c.  If we don't add arm-common.o to
every target which uses arm-tdep.o, the build will fail on these targets

build/gdb/aarch64/gdb/../../../../binutils-gdb/gdb/arm-tdep.c:846: undefined reference to `thumb_insn_size'
build/gdb/aarch64/gdb/../../../../binutils-gdb/gdb/arm-tdep.c:846: undefined reference to `thumb_insn_size'
arm-tdep.o: In function `arm_adjust_breakpoint_address':
build/gdb/aarch64/gdb/../../../../binutils-gdb/gdb/arm-tdep.c:5417: undefined reference to `thumb_insn_size'
build/gdb/aarch64/gdb/../../../../binutils-gdb/gdb/arm-tdep.c:5467: undefined reference to `thumb_insn_size'
arm-tdep.o: In function `arm_breakpoint_from_pc':
build/gdb/aarch64/gdb/../../../../binutils-gdb/gdb/arm-tdep.c:8858: undefined reference to `thumb_insn_size'

>>
>>> diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
>>> index 367c704..15ecb70 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 "common/arm-common.h"
>>> +
>>>   /* Defined in auto-generated files.  */
>>>   void init_registers_arm (void);
>>>   extern const struct target_desc *tdesc_arm;
>>> @@ -234,19 +236,28 @@ 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;
>>> -static const unsigned short thumb2_breakpoint[] = { 0xf7f0, 0xa000 };
>>> +#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
>>
>> I am confused by your changes here.  Why do you change them?
>>
>
> Before arm_breakpoint_from_pc would be :
>
> #ifndef __ARM_EABI__
>   return (const unsigned char *) &arm_breakpoint;
> #else
> -  return (const unsigned char *) &arm_eabi_breakpoint;
> #endif
>
> And arm_breakpoint_at would check for the arm_breakpoint ||
> arm_eabi_breakpoint.
>
> Thus the selected arm_breakpoint would be what that function returned
> and arm_breakpoint was arm_abi_breakpoint.
>
> It felt more clear to me to name 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 breakpoint_from_pc as it just returns
> arm_breakpoint rather than having the #ifdef in that function.
>
> Any other reference to the arm_breakpoint would also be clear of #ifdefs...

Please don't combine movement and refactoring.  This patch is about
movement, so let us do movement only.  Refactor can be done separately.

-- 
Yao (齐尧)

Re: [PATCH 3/5] Add support for ARM breakpoint types in GDBServer.

[Antoine Tremblay]

On 09/29/2015 04:32 AM, Yao Qi wrote:
> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
>> This will contain more stuff as I post the next patch sets for single
>> stepping etc.. I would like to keep a more general name.
>>
>> It's basically all coming from arm-tdep.c but I don't want to name it
>> common/arm-tdep.c to avoid confusion and Makefile problems.
>>
>> arm-ctdep.c ?
>>
>
> How about arm.c?
>

There's already arm.h in there I think I can actually merge what is in 
there with what I have, and create arm.c.

>>> Please move this file to arch/ directory rather than common/
>>
>> It seems weird to me to have common objects somewhere else then in
>> common/ , if common becomes more a lib we don't want it all over the
>> place no ?
>>
>> Would creating a common/arch be acceptable ? I guess we would have to
>> move things like x86-xstate.h etc.. there too ?
>
> common/ was created in order to share code between GDB and GDBserver, see
> https://sourceware.org/gdb/wiki/Common After that, we find that we'll
> end up with moving most of gdb files into common/ and we'd better move
> different common things into different common directory.  Nowadays, we
> have nat/ common/ and arch/ directories for common things.  arm.c should
> be put in arch/ directory.

Ok I understand thanks for explaining.

>
>>>> diff --git a/gdb/configure.tgt b/gdb/configure.tgt
>>>> index c42b4df..e831f59 100644
>>>> --- a/gdb/configure.tgt
>>>> +++ b/gdb/configure.tgt
>>>> @@ -89,7 +89,7 @@ arm*-wince-pe | arm*-*-mingw32ce*)
>>>>    	;;
>>>>    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-common.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
>>>
>>> since arm-common.o is moved out of arm-tdep.o, so it should be added to
>>> every target which uses arm-tdep.o, such as aarch64*-*-linux*,
>>> arm*-wince-pe, etc.
>>
>> Even if they don't use it ? But ok.
>>
>
> No, they use it.  This patch moves thumb_insn_size to arm-common.o, but
> thumb_insn_size is used by arm-tdep.c.  If we don't add arm-common.o to
> every target which uses arm-tdep.o, the build will fail on these targets
>
> build/gdb/aarch64/gdb/../../../../binutils-gdb/gdb/arm-tdep.c:846: undefined reference to `thumb_insn_size'
> build/gdb/aarch64/gdb/../../../../binutils-gdb/gdb/arm-tdep.c:846: undefined reference to `thumb_insn_size'
> arm-tdep.o: In function `arm_adjust_breakpoint_address':
> build/gdb/aarch64/gdb/../../../../binutils-gdb/gdb/arm-tdep.c:5417: undefined reference to `thumb_insn_size'
> build/gdb/aarch64/gdb/../../../../binutils-gdb/gdb/arm-tdep.c:5467: undefined reference to `thumb_insn_size'
> arm-tdep.o: In function `arm_breakpoint_from_pc':
> build/gdb/aarch64/gdb/../../../../binutils-gdb/gdb/arm-tdep.c:8858: undefined reference to `thumb_insn_size'
>

Indeed, thanks

>>>
>>>> diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
>>>> index 367c704..15ecb70 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 "common/arm-common.h"
>>>> +
>>>>    /* Defined in auto-generated files.  */
>>>>    void init_registers_arm (void);
>>>>    extern const struct target_desc *tdesc_arm;
>>>> @@ -234,19 +236,28 @@ 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;
>>>> -static const unsigned short thumb2_breakpoint[] = { 0xf7f0, 0xa000 };
>>>> +#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
>>>
>>> I am confused by your changes here.  Why do you change them?
>>>
>>
>> Before arm_breakpoint_from_pc would be :
>>
>> #ifndef __ARM_EABI__
>>    return (const unsigned char *) &arm_breakpoint;
>> #else
>> -  return (const unsigned char *) &arm_eabi_breakpoint;
>> #endif
>>
>> And arm_breakpoint_at would check for the arm_breakpoint ||
>> arm_eabi_breakpoint.
>>
>> Thus the selected arm_breakpoint would be what that function returned
>> and arm_breakpoint was arm_abi_breakpoint.
>>
>> It felt more clear to me to name 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 breakpoint_from_pc as it just returns
>> arm_breakpoint rather than having the #ifdef in that function.
>>
>> Any other reference to the arm_breakpoint would also be clear of #ifdefs...
>
> Please don't combine movement and refactoring.  This patch is about
> movement, so let us do movement only.  Refactor can be done separately.
>

Ok , would you find it acceptable still to include the refactoring in 
the series as the next patch or should I create it out of series ?

Re: [PATCH 3/5] Add support for ARM breakpoint types in GDBServer.

[Yao Qi]

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

> Ok , would you find it acceptable still to include the refactoring in
> the series as the next patch or should I create it out of series ?

Either is OK to me.  My point is don't combine movement and refactoring
in a single patch.

-- 
Yao (齐尧)

Re: [PATCH 4/5] Handle breakpoint kinds for software breakpoints in GDBServer.

[Antoine Tremblay]

On 09/28/2015 06:33 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..bf4c3c7 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 length, the length can have target specific
>> +     meaning like the z0 kind parameter.  */
>> +  const unsigned char *(*breakpoint_from_length) (int *len);
>>   };
>
> If you agree on my comments to patch 2/5, we can rename this hook to
> breakpoint_from_z0_kind.
>

I've renamed this to breakpoint_from_kind since I want to be able to 
reuse the same operation for future tracepoint kinds and don't want the 
confusion with z0.

This follows the terminology introduced in patch 2/5 in general too.