[PATCH 0/6] S390: Watchpoint enhancements and hardware breakpoints

[PATCH 0/6] S390: Watchpoint enhancements and hardware breakpoints

[Andreas Arnez]

This series adds some enhancements to the current s390-linux native
support for watchpoints.  It also adds hardware breakpoint support.

While this is mostly an s390-specific series, it includes one
common-code patch: patch #5, "linux-nat: Add function lwp_is_stepping".
I'm looking for feedback on that.  Comments to the s390-specific patches
are certainly welcome as well.

Andreas Arnez (6):
  S390: Avoid direct access to lwp_info structure
  S390: Migrate watch areas from list to VEC type
  S390: Multi-inferior watchpoint support
  S390: Enable "maint set show-debug-regs"
  linux-nat: Add function lwp_is_stepping
  S390: Hardware breakpoint support

 gdb/gdbserver/linux-low.c |   8 +
 gdb/linux-nat.c           |   8 +
 gdb/nat/linux-nat.h       |   4 +
 gdb/s390-linux-nat.c      | 442 ++++++++++++++++++++++++++++++++++++++--------
 gdb/testsuite/lib/gdb.exp |   3 +-
 5 files changed, 394 insertions(+), 71 deletions(-)

-- 
2.5.0

[PATCH] Fix order of inferiors in "thread apply all"

[Andreas Arnez]

This inserts missing parentheses in the calculation of the comparison
result between two different inferior numbers.  The problem was found by
Philipp Rudo.

gdb/ChangeLog:

	* thread.c (tp_array_compar): Insert missing parentheses.

gdb/testsuite/ChangeLog:

	* gdb.multi/tids.exp: Test "thread apply all".
---
 gdb/testsuite/gdb.multi/tids.exp | 6 ++++++
 gdb/thread.c                     | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.multi/tids.exp b/gdb/testsuite/gdb.multi/tids.exp
index 5d8701e..12ce98a 100644
--- a/gdb/testsuite/gdb.multi/tids.exp
+++ b/gdb/testsuite/gdb.multi/tids.exp
@@ -224,6 +224,12 @@ with_test_prefix "two inferiors" {
     thr_apply_info_thr "1.1-2 2.2-3" \
 	"1.1 1.2 2.2 2.3"
 
+    # All threads.
+    thread_apply "all" \
+	"2.3 2.2 2.1 1.3 1.2 1.1"
+    thread_apply "all -ascending" \
+	"1.1 1.2 1.3 2.1 2.2 2.3"
+
     # Now test using GDB convenience variables.
 
     gdb_test "p \$inf = 1" " = 1"
diff --git a/gdb/thread.c b/gdb/thread.c
index ab98777..a66a2b5 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1725,7 +1725,7 @@ tp_array_compar (const void *ap_voidp, const void *bp_voidp)
 
   if (a->inf->num != b->inf->num)
     {
-      return ((a->inf->num > b->inf->num) - (a->inf->num < b->inf->num)
+      return (((a->inf->num > b->inf->num) - (a->inf->num < b->inf->num))
 	      * (tp_array_compar_ascending ? +1 : -1));
     }
 
-- 
2.5.0

Re: [PATCH] Fix order of inferiors in "thread apply all"

[Andreas Arnez]

On Thu, Sep 15 2016, Andreas Arnez wrote:

> This inserts missing parentheses in the calculation of the comparison
> result between two different inferior numbers.  The problem was found by
> Philipp Rudo.

Oops, this is an inadvertant re-send.  Please ignore.

--
Andreas

[PATCH 1/6] S390: Avoid direct access to lwp_info structure

[Andreas Arnez]

When using the lwp_info structure, avoid accessing its members directly,
and use the advertised function interfaces instead.  This is according
to the instructions in linux-nat.h and prepares for making some of the
code common between gdb and gdbserver.

gdb/ChangeLog:

	* s390-linux-nat.c (s390_prepare_to_resume): Use advertised lwp
	functions instead of accessing lwp_info structure members.
	(s390_mark_per_info_changed): New function.
	(s390_new_thread): Use it.
	(s390_refresh_per_info_cb): New function.
	(s390_refresh_per_info): Remove parameter.  Refresh all lwps of
	the current process.
	(s390_insert_watchpoint): Adjust call to s390_refresh_per_info.
	(s390_remove_watchpoint): Likewise.
---
 gdb/s390-linux-nat.c | 60 +++++++++++++++++++++++++++++++---------------------
 1 file changed, 36 insertions(+), 24 deletions(-)

diff --git a/gdb/s390-linux-nat.c b/gdb/s390-linux-nat.c
index e91297b..1e937f9 100644
--- a/gdb/s390-linux-nat.c
+++ b/gdb/s390-linux-nat.c
@@ -488,16 +488,16 @@ s390_prepare_to_resume (struct lwp_info *lp)
 
   CORE_ADDR watch_lo_addr = (CORE_ADDR)-1, watch_hi_addr = 0;
   struct watch_area *area;
+  struct arch_lwp_info *lp_priv = lwp_arch_private_info (lp);
 
-  if (lp->arch_private == NULL
-      || !lp->arch_private->per_info_changed)
+  if (lp_priv == NULL || !lp_priv->per_info_changed)
     return;
 
-  lp->arch_private->per_info_changed = 0;
+  lp_priv->per_info_changed = 0;
 
-  tid = ptid_get_lwp (lp->ptid);
+  tid = ptid_get_lwp (ptid_of_lwp (lp));
   if (tid == 0)
-    tid = ptid_get_pid (lp->ptid);
+    tid = ptid_get_pid (ptid_of_lwp (lp));
 
   for (area = watch_base; area; area = area->next)
     {
@@ -528,19 +528,15 @@ s390_prepare_to_resume (struct lwp_info *lp)
     perror_with_name (_("Couldn't modify watchpoint status"));
 }
 
-/* Make sure that LP is stopped and mark its PER info as changed, so
-   the next resume will update it.  */
+/* Mark the PER info as changed, so the next resume will update it.  */
 
 static void
-s390_refresh_per_info (struct lwp_info *lp)
+s390_mark_per_info_changed (struct lwp_info *lp)
 {
-  if (lp->arch_private == NULL)
-    lp->arch_private = XCNEW (struct arch_lwp_info);
+  if (lwp_arch_private_info (lp) == NULL)
+    lwp_set_arch_private_info (lp, XCNEW (struct arch_lwp_info));
 
-  lp->arch_private->per_info_changed = 1;
-
-  if (!lp->stopped)
-    linux_stop_lwp (lp);
+  lwp_arch_private_info (lp)->per_info_changed = 1;
 }
 
 /* When attaching to a new thread, mark its PER info as changed.  */
@@ -548,8 +544,30 @@ s390_refresh_per_info (struct lwp_info *lp)
 static void
 s390_new_thread (struct lwp_info *lp)
 {
-  lp->arch_private = XCNEW (struct arch_lwp_info);
-  lp->arch_private->per_info_changed = 1;
+  s390_mark_per_info_changed (lp);
+}
+
+/* Iterator callback for s390_refresh_per_info.  */
+
+static int
+s390_refresh_per_info_cb (struct lwp_info *lp, void *arg)
+{
+  s390_mark_per_info_changed (lp);
+
+  if (!lwp_is_stopped (lp))
+    linux_stop_lwp (lp);
+  return 0;
+}
+
+/* Make sure that threads are stopped and mark PER info as changed.  */
+
+static int
+s390_refresh_per_info (void)
+{
+  ptid_t pid_ptid = pid_to_ptid (ptid_get_pid (current_lwp_ptid ()));
+
+  iterate_over_lwps (pid_ptid, s390_refresh_per_info_cb, NULL);
+  return 0;
 }
 
 static int
@@ -557,7 +575,6 @@ s390_insert_watchpoint (struct target_ops *self,
 			CORE_ADDR addr, int len, enum target_hw_bp_type type,
 			struct expression *cond)
 {
-  struct lwp_info *lp;
   struct watch_area *area = XNEW (struct watch_area);
 
   if (!area)
@@ -569,9 +586,7 @@ s390_insert_watchpoint (struct target_ops *self,
   area->next = watch_base;
   watch_base = area;
 
-  ALL_LWPS (lp)
-    s390_refresh_per_info (lp);
-  return 0;
+  return s390_refresh_per_info ();
 }
 
 static int
@@ -579,7 +594,6 @@ s390_remove_watchpoint (struct target_ops *self,
 			CORE_ADDR addr, int len, enum target_hw_bp_type type,
 			struct expression *cond)
 {
-  struct lwp_info *lp;
   struct watch_area *area, **parea;
 
   for (parea = &watch_base; *parea; parea = &(*parea)->next)
@@ -598,9 +612,7 @@ s390_remove_watchpoint (struct target_ops *self,
   *parea = area->next;
   xfree (area);
 
-  ALL_LWPS (lp)
-    s390_refresh_per_info (lp);
-  return 0;
+  return s390_refresh_per_info ();
 }
 
 static int
-- 
2.5.0

[PATCH 3/6] S390: Multi-inferior watchpoint support

[Andreas Arnez]

Support different sets of watchpoints in multiple inferiors.

gdb/ChangeLog:

	* s390-linux-nat.c (watch_areas): Remove variable.  Replace by a
	member of...
	(struct s390_debug_reg_state): ...this.  New struct.
	(struct s390_process_info): New struct.
	(s390_process_list): New variable.
	(s390_find_process_pid, s390_add_process, s390_process_info_get)
	(s390_get_debug_reg_state): New functions.
	(s390_stopped_by_watchpoint): Now access the watch_areas VEC via
	s390_get_debug_reg_state.
	(s390_prepare_to_resume): Likewise.
	(s390_insert_watchpoint): Likewise.
	(s390_remove_watchpoint): Likewise.
	(s390_forget_process, s390_linux_new_fork): New linux_nat target
	methods.
	(_initialize_s390_nat): Register them.
---
 gdb/s390-linux-nat.c | 149 +++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 139 insertions(+), 10 deletions(-)

diff --git a/gdb/s390-linux-nat.c b/gdb/s390-linux-nat.c
index 13bf7fd..b549246 100644
--- a/gdb/s390-linux-nat.c
+++ b/gdb/s390-linux-nat.c
@@ -430,8 +430,8 @@ s390_linux_store_inferior_registers (struct target_ops *ops,
 
 /* Hardware-assisted watchpoint handling.  */
 
-/* We maintain a list of all currently active watchpoints in order
-   to properly handle watchpoint removal.
+/* For each process we maintain a list of all currently active
+   watchpoints, in order to properly handle watchpoint removal.
 
    The only thing we actually need is the total address space area
    spanned by the watchpoints.  */
@@ -444,17 +444,138 @@ typedef struct watch_area
 
 DEF_VEC_O (s390_watch_area);
 
-VEC_s390_watch_area *watch_areas = NULL;
+/* Hardware debug state.  */
+
+struct s390_debug_reg_state
+{
+  VEC_s390_watch_area *watch_areas;
+};
+
+/* Per-process data.  */
+
+struct s390_process_info
+{
+  struct s390_process_info *next;
+  pid_t pid;
+  struct s390_debug_reg_state state;
+};
+
+static struct s390_process_info *s390_process_list = NULL;
+
+/* Find process data for process PID.  */
+
+static struct s390_process_info *
+s390_find_process_pid (pid_t pid)
+{
+  struct s390_process_info *proc;
+
+  for (proc = s390_process_list; proc; proc = proc->next)
+    if (proc->pid == pid)
+      return proc;
+
+  return NULL;
+}
+
+/* Add process data for process PID.  Returns newly allocated info
+   object.  */
+
+static struct s390_process_info *
+s390_add_process (pid_t pid)
+{
+  struct s390_process_info *proc = XCNEW (struct s390_process_info);
+
+  proc->pid = pid;
+  proc->next = s390_process_list;
+  s390_process_list = proc;
+
+  return proc;
+}
+
+/* Get data specific info for process PID, creating it if necessary.
+   Never returns NULL.  */
+
+static struct s390_process_info *
+s390_process_info_get (pid_t pid)
+{
+  struct s390_process_info *proc;
+
+  proc = s390_find_process_pid (pid);
+  if (proc == NULL)
+    proc = s390_add_process (pid);
+
+  return proc;
+}
+
+/* Get hardware debug state for process PID.  */
+
+static struct s390_debug_reg_state *
+s390_get_debug_reg_state (pid_t pid)
+{
+  return &s390_process_info_get (pid)->state;
+}
+
+/* Called whenever GDB is no longer debugging process PID.  It deletes
+   data structures that keep track of hardware debug state.  */
+
+static void
+s390_forget_process (pid_t pid)
+{
+  struct s390_process_info *proc, **proc_link;
+
+  proc = s390_process_list;
+  proc_link = &s390_process_list;
+
+  while (proc != NULL)
+    {
+      if (proc->pid == pid)
+	{
+	  VEC_free (s390_watch_area, proc->state.watch_areas);
+	  *proc_link = proc->next;
+	  xfree (proc);
+	  return;
+	}
+
+      proc_link = &proc->next;
+      proc = *proc_link;
+    }
+}
+
+/* linux_nat_new_fork hook.   */
+
+static void
+s390_linux_new_fork (struct lwp_info *parent, pid_t child_pid)
+{
+  pid_t parent_pid;
+  struct s390_debug_reg_state *parent_state;
+  struct s390_debug_reg_state *child_state;
+
+  /* NULL means no watchpoint has ever been set in the parent.  In
+     that case, there's nothing to do.  */
+  if (lwp_arch_private_info (parent) == NULL)
+    return;
+
+  /* GDB core assumes the child inherits the watchpoints/hw breakpoints of
+     the parent.  So copy the debug state from parent to child.  */
+
+  parent_pid = ptid_get_pid (parent->ptid);
+  parent_state = s390_get_debug_reg_state (parent_pid);
+  child_state = s390_get_debug_reg_state (child_pid);
+
+  child_state->watch_areas = VEC_copy (s390_watch_area,
+				       parent_state->watch_areas);
+}
 
 static int
 s390_stopped_by_watchpoint (struct target_ops *ops)
 {
+  struct s390_debug_reg_state *state
+    = s390_get_debug_reg_state (ptid_get_pid (inferior_ptid));
   per_lowcore_bits per_lowcore;
   ptrace_area parea;
   int result;
 
   /* Speed up common case.  */
-  if (VEC_empty (s390_watch_area, watch_areas))
+  if (VEC_empty (s390_watch_area, state->watch_areas))
     return 0;
 
   parea.len = sizeof (per_lowcore);
@@ -483,6 +604,7 @@ static void
 s390_prepare_to_resume (struct lwp_info *lp)
 {
   int tid;
+  pid_t pid = ptid_get_pid (ptid_of_lwp (lp));
 
   per_struct per_info;
   ptrace_area parea;
@@ -491,6 +613,7 @@ s390_prepare_to_resume (struct lwp_info *lp)
   unsigned ix;
   s390_watch_area *area;
   struct arch_lwp_info *lp_priv = lwp_arch_private_info (lp);
+  struct s390_debug_reg_state *state = s390_get_debug_reg_state (pid);
 
   if (lp_priv == NULL || !lp_priv->per_info_changed)
     return;
@@ -499,7 +622,7 @@ s390_prepare_to_resume (struct lwp_info *lp)
 
   tid = ptid_get_lwp (ptid_of_lwp (lp));
   if (tid == 0)
-    tid = ptid_get_pid (ptid_of_lwp (lp));
+    tid = pid;
 
   parea.len = sizeof (per_info);
   parea.process_addr = (addr_t) & per_info;
@@ -507,10 +630,10 @@ s390_prepare_to_resume (struct lwp_info *lp)
   if (ptrace (PTRACE_PEEKUSR_AREA, tid, &parea, 0) < 0)
     perror_with_name (_("Couldn't retrieve watchpoint status"));
 
-  if (!VEC_empty (s390_watch_area, watch_areas))
+  if (!VEC_empty (s390_watch_area, state->watch_areas))
     {
       for (ix = 0;
-	   VEC_iterate (s390_watch_area, watch_areas, ix, area);
+	   VEC_iterate (s390_watch_area, state->watch_areas, ix, area);
 	   ix++)
 	{
 	  watch_lo_addr = min (watch_lo_addr, area->lo_addr);
@@ -580,10 +703,12 @@ s390_insert_watchpoint (struct target_ops *self,
 			struct expression *cond)
 {
   s390_watch_area area;
+  struct s390_debug_reg_state *state
+    = s390_get_debug_reg_state (ptid_get_pid (inferior_ptid));
 
   area.lo_addr = addr;
   area.hi_addr = addr + len - 1;
-  VEC_safe_push (s390_watch_area, watch_areas, &area);
+  VEC_safe_push (s390_watch_area, state->watch_areas, &area);
 
   return s390_refresh_per_info ();
 }
@@ -595,14 +720,16 @@ s390_remove_watchpoint (struct target_ops *self,
 {
   unsigned ix;
   s390_watch_area *area;
+  struct s390_debug_reg_state *state
+    = s390_get_debug_reg_state (ptid_get_pid (inferior_ptid));
 
   for (ix = 0;
-       VEC_iterate (s390_watch_area, watch_areas, ix, area);
+       VEC_iterate (s390_watch_area, state->watch_areas, ix, area);
        ix++)
     {
       if (area->lo_addr == addr && area->hi_addr == addr + len - 1)
 	{
-	  VEC_unordered_remove (s390_watch_area, watch_areas, ix);
+	  VEC_unordered_remove (s390_watch_area, state->watch_areas, ix);
 	  return s390_refresh_per_info ();
 	}
     }
@@ -753,4 +880,6 @@ _initialize_s390_nat (void)
   linux_nat_add_target (t);
   linux_nat_set_new_thread (t, s390_new_thread);
   linux_nat_set_prepare_to_resume (t, s390_prepare_to_resume);
+  linux_nat_set_forget_process (t, s390_forget_process);
+  linux_nat_set_new_fork (t, s390_linux_new_fork);
 }
-- 
2.5.0

[PATCH 4/6] S390: Enable "maint set show-debug-regs"

[Andreas Arnez]

Implement a new function for dumping the S390 "debug
registers" (actually, the PER info) and invoke it at appropriate places.
Respect the variable show_debug_regs and make it settable by the user.

gdb/ChangeLog:

	* s390-linux-nat.c (gdbcmd.h): New include.
	(s390_show_debug_regs): New function.
	(s390_stopped_by_watchpoint): Call it, if show_debug_regs is set.
	(s390_prepare_to_resume): Likewise.
	(_initialize_s390_nat): Register the command "maint set
	show-debug-regs".
---
 gdb/s390-linux-nat.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/gdb/s390-linux-nat.c b/gdb/s390-linux-nat.c
index b549246..c5d9bd3 100644
--- a/gdb/s390-linux-nat.c
+++ b/gdb/s390-linux-nat.c
@@ -28,6 +28,7 @@
 #include "gregset.h"
 #include "regset.h"
 #include "nat/linux-ptrace.h"
+#include "gdbcmd.h"
 
 #include "s390-linux-tdep.h"
 #include "elf/common.h"
@@ -565,6 +566,37 @@ s390_linux_new_fork (struct lwp_info *parent, pid_t child_pid)
 				       parent_state->watch_areas);
 }
 
+/* Dump PER state.  */
+
+static void
+s390_show_debug_regs (int tid, const char *where)
+{
+  per_struct per_info;
+  ptrace_area parea;
+
+  parea.len = sizeof (per_info);
+  parea.process_addr = (addr_t) &per_info;
+  parea.kernel_addr = offsetof (struct user_regs_struct, per_info);
+
+  if (ptrace (PTRACE_PEEKUSR_AREA, tid, &parea, 0) < 0)
+    perror_with_name (_("Couldn't retrieve debug regs"));
+
+  debug_printf ("PER (debug) state for %d -- %s\n"
+		"  cr9-11: %lx %lx %lx\n"
+		"  start, end: %lx %lx\n"
+		"  code/ATMID: %x  address: %lx  PAID: %x\n",
+		tid,
+		where,
+		per_info.control_regs.words.cr[0],
+		per_info.control_regs.words.cr[1],
+		per_info.control_regs.words.cr[2],
+		per_info.starting_addr,
+		per_info.ending_addr,
+		per_info.lowcore.words.perc_atmid,
+		per_info.lowcore.words.address,
+		per_info.lowcore.words.access_id);
+}
+
 static int
 s390_stopped_by_watchpoint (struct target_ops *ops)
 {
@@ -574,6 +606,9 @@ s390_stopped_by_watchpoint (struct target_ops *ops)
   ptrace_area parea;
   int result;
 
+  if (show_debug_regs)
+    s390_show_debug_regs (s390_inferior_tid (), "stop");
+
   /* Speed up common case.  */
   if (VEC_empty (s390_watch_area, state->watch_areas))
     return 0;
@@ -653,6 +688,9 @@ s390_prepare_to_resume (struct lwp_info *lp)
 
   if (ptrace (PTRACE_POKEUSR_AREA, tid, &parea, 0) < 0)
     perror_with_name (_("Couldn't modify watchpoint status"));
+
+  if (show_debug_regs)
+    s390_show_debug_regs (tid, "resume");
 }
 
 /* Mark the PER info as changed, so the next resume will update it.  */
@@ -882,4 +920,17 @@ _initialize_s390_nat (void)
   linux_nat_set_prepare_to_resume (t, s390_prepare_to_resume);
   linux_nat_set_forget_process (t, s390_forget_process);
   linux_nat_set_new_fork (t, s390_linux_new_fork);
+
+  /* A maintenance command to enable showing the PER state.  */
+  add_setshow_boolean_cmd ("show-debug-regs", class_maintenance,
+			   &show_debug_regs, _("\
+Set whether to show the PER (debug) hardware state."), _("\
+Show whether to show the PER (debug) hardware state."), _("\
+Use \"on\" to enable, \"off\" to disable.\n\
+If enabled, the PER state is shown after it is changed by GDB,\n\
+and when the inferior triggers a breakpoint or watchpoint."),
+			   NULL,
+			   NULL,
+			   &maintenance_set_cmdlist,
+			   &maintenance_show_cmdlist);
 }
-- 
2.5.0

[PATCH 2/6] S390: Migrate watch areas from list to VEC type

[Andreas Arnez]

For S390, the list of active watchpoints is maintained in a list based
at "watch_base".  This refactors the list to a vector "watch_areas".

gdb/ChangeLog:

	* s390-linux-nat.c (s390_watch_area): New typedef.  Define a VEC.
	(watch_base): Remove variable.
	(watch_areas): New variable.
	(s390_stopped_by_watchpoint): Transform operations on the
	watch_base list to equivalent operations on the watch_areas VEC.
	(s390_prepare_to_resume): Likewise.
	(s390_insert_watchpoint): Likewise.
	(s390_remove_watchpoint): Likewise.
---
 gdb/s390-linux-nat.c | 71 +++++++++++++++++++++++++---------------------------
 1 file changed, 34 insertions(+), 37 deletions(-)

diff --git a/gdb/s390-linux-nat.c b/gdb/s390-linux-nat.c
index 1e937f9..13bf7fd 100644
--- a/gdb/s390-linux-nat.c
+++ b/gdb/s390-linux-nat.c
@@ -436,14 +436,15 @@ s390_linux_store_inferior_registers (struct target_ops *ops,
    The only thing we actually need is the total address space area
    spanned by the watchpoints.  */
 
-struct watch_area
+typedef struct watch_area
 {
-  struct watch_area *next;
   CORE_ADDR lo_addr;
   CORE_ADDR hi_addr;
-};
+} s390_watch_area;
+
+DEF_VEC_O (s390_watch_area);
 
-static struct watch_area *watch_base = NULL;
+VEC_s390_watch_area *watch_areas = NULL;
 
 static int
 s390_stopped_by_watchpoint (struct target_ops *ops)
@@ -453,7 +454,7 @@ s390_stopped_by_watchpoint (struct target_ops *ops)
   int result;
 
   /* Speed up common case.  */
-  if (!watch_base)
+  if (VEC_empty (s390_watch_area, watch_areas))
     return 0;
 
   parea.len = sizeof (per_lowcore);
@@ -487,7 +488,8 @@ s390_prepare_to_resume (struct lwp_info *lp)
   ptrace_area parea;
 
   CORE_ADDR watch_lo_addr = (CORE_ADDR)-1, watch_hi_addr = 0;
-  struct watch_area *area;
+  unsigned ix;
+  s390_watch_area *area;
   struct arch_lwp_info *lp_priv = lwp_arch_private_info (lp);
 
   if (lp_priv == NULL || !lp_priv->per_info_changed)
@@ -499,20 +501,22 @@ s390_prepare_to_resume (struct lwp_info *lp)
   if (tid == 0)
     tid = ptid_get_pid (ptid_of_lwp (lp));
 
-  for (area = watch_base; area; area = area->next)
-    {
-      watch_lo_addr = min (watch_lo_addr, area->lo_addr);
-      watch_hi_addr = max (watch_hi_addr, area->hi_addr);
-    }
-
   parea.len = sizeof (per_info);
   parea.process_addr = (addr_t) & per_info;
   parea.kernel_addr = offsetof (struct user_regs_struct, per_info);
   if (ptrace (PTRACE_PEEKUSR_AREA, tid, &parea, 0) < 0)
     perror_with_name (_("Couldn't retrieve watchpoint status"));
 
-  if (watch_base)
+  if (!VEC_empty (s390_watch_area, watch_areas))
     {
+      for (ix = 0;
+	   VEC_iterate (s390_watch_area, watch_areas, ix, area);
+	   ix++)
+	{
+	  watch_lo_addr = min (watch_lo_addr, area->lo_addr);
+	  watch_hi_addr = max (watch_hi_addr, area->hi_addr);
+	}
+
       per_info.control_regs.bits.em_storage_alteration = 1;
       per_info.control_regs.bits.storage_alt_space_ctl = 1;
     }
@@ -575,16 +579,11 @@ s390_insert_watchpoint (struct target_ops *self,
 			CORE_ADDR addr, int len, enum target_hw_bp_type type,
 			struct expression *cond)
 {
-  struct watch_area *area = XNEW (struct watch_area);
+  s390_watch_area area;
 
-  if (!area)
-    return -1;
-
-  area->lo_addr = addr;
-  area->hi_addr = addr + len - 1;
-
-  area->next = watch_base;
-  watch_base = area;
+  area.lo_addr = addr;
+  area.hi_addr = addr + len - 1;
+  VEC_safe_push (s390_watch_area, watch_areas, &area);
 
   return s390_refresh_per_info ();
 }
@@ -594,25 +593,23 @@ s390_remove_watchpoint (struct target_ops *self,
 			CORE_ADDR addr, int len, enum target_hw_bp_type type,
 			struct expression *cond)
 {
-  struct watch_area *area, **parea;
-
-  for (parea = &watch_base; *parea; parea = &(*parea)->next)
-    if ((*parea)->lo_addr == addr
-	&& (*parea)->hi_addr == addr + len - 1)
-      break;
+  unsigned ix;
+  s390_watch_area *area;
 
-  if (!*parea)
+  for (ix = 0;
+       VEC_iterate (s390_watch_area, watch_areas, ix, area);
+       ix++)
     {
-      fprintf_unfiltered (gdb_stderr,
-			  "Attempt to remove nonexistent watchpoint.\n");
-      return -1;
+      if (area->lo_addr == addr && area->hi_addr == addr + len - 1)
+	{
+	  VEC_unordered_remove (s390_watch_area, watch_areas, ix);
+	  return s390_refresh_per_info ();
+	}
     }
 
-  area = *parea;
-  *parea = area->next;
-  xfree (area);
-
-  return s390_refresh_per_info ();
+  fprintf_unfiltered (gdb_stderr,
+		      "Attempt to remove nonexistent watchpoint.\n");
+  return -1;
 }
 
 static int
-- 
2.5.0

[PATCH 6/6] S390: Hardware breakpoint support

[Andreas Arnez]

Add hardware breakpoint support for S390 targets.

gdb/ChangeLog:

	* s390-linux-nat.c (PER_BIT, PER_EVENT_BRANCH, PER_EVENT_IFETCH)
	(PER_EVENT_STORE, PER_EVENT_NULLIFICATION)
	(PER_CONTROL_BRANCH_ADDRESS, PER_CONTROL_SUSPENSION)
	(PER_CONTROL_ALTERATION): New macros.
	(struct s390_debug_reg_state) <break_areas>: New member.
	(s390_forget_process): Free break_areas as well.
	(s390_linux_new_fork): Copy break_areas as well.
	(s390_prepare_to_resume): Install hardware breakpoints.
	(s390_can_use_hw_breakpoint): Indicate support for hardware
	breakpoints.
	(s390_insert_hw_breakpoint, s390_remove_hw_breakpoint): New
	linux_nat target methods.
	(_initialize_s390_nat): Register them.

gdb/testsuite/ChangeLog:

	* lib/gdb.exp: No longer skip hardware breakpoint tests on s390.
---
 gdb/s390-linux-nat.c      | 131 ++++++++++++++++++++++++++++++++++++++++++----
 gdb/testsuite/lib/gdb.exp |   3 +-
 2 files changed, 124 insertions(+), 10 deletions(-)

diff --git a/gdb/s390-linux-nat.c b/gdb/s390-linux-nat.c
index c5d9bd3..26c56f8 100644
--- a/gdb/s390-linux-nat.c
+++ b/gdb/s390-linux-nat.c
@@ -93,6 +93,18 @@ static const struct regset s390_64_gregset =
 #define S390_PSWA_OFFSET 8
 #endif
 
+/* PER-event mask bits and PER control bits (CR9).  */
+
+#define PER_BIT(n)			(1UL << (63 - (n)))
+#define PER_EVENT_BRANCH		PER_BIT (32)
+#define PER_EVENT_IFETCH		PER_BIT (33)
+#define PER_EVENT_STORE			PER_BIT (34)
+#define PER_EVENT_NULLIFICATION		PER_BIT (39)
+#define PER_CONTROL_BRANCH_ADDRESS	PER_BIT (40)
+#define PER_CONTROL_SUSPENSION		PER_BIT (41)
+#define PER_CONTROL_ALTERATION		PER_BIT (42)
+
+
 /* Fill GDB's register array with the general-purpose register values
    in *REGP.
 
@@ -450,6 +462,7 @@ DEF_VEC_O (s390_watch_area);
 struct s390_debug_reg_state
 {
   VEC_s390_watch_area *watch_areas;
+  VEC_s390_watch_area *break_areas;
 };
 
 /* Per-process data.  */
@@ -531,6 +544,7 @@ s390_forget_process (pid_t pid)
       if (proc->pid == pid)
 	{
 	  VEC_free (s390_watch_area, proc->state.watch_areas);
+	  VEC_free (s390_watch_area, proc->state.break_areas);
 	  *proc_link = proc->next;
 	  xfree (proc);
 	  return;
@@ -564,6 +578,8 @@ s390_linux_new_fork (struct lwp_info *parent, pid_t child_pid)
 
   child_state->watch_areas = VEC_copy (s390_watch_area,
 				       parent_state->watch_areas);
+  child_state->break_areas = VEC_copy (s390_watch_area,
+				       parent_state->break_areas);
 }
 
 /* Dump PER state.  */
@@ -649,10 +665,20 @@ s390_prepare_to_resume (struct lwp_info *lp)
   s390_watch_area *area;
   struct arch_lwp_info *lp_priv = lwp_arch_private_info (lp);
   struct s390_debug_reg_state *state = s390_get_debug_reg_state (pid);
+  int step = lwp_is_stepping (lp);
 
-  if (lp_priv == NULL || !lp_priv->per_info_changed)
+  /* Nothing to do if there was never any PER info for this thread.  */
+  if (lp_priv == NULL)
     return;
 
+  /* If PER info has changed, update it.  When single-stepping, disable
+     hardware breakpoints (if any).  Otherwise we're done.  */
+  if (!lp_priv->per_info_changed)
+    {
+      if (!step || VEC_empty (s390_watch_area, state->break_areas))
+	return;
+    }
+
   lp_priv->per_info_changed = 0;
 
   tid = ptid_get_lwp (ptid_of_lwp (lp));
@@ -662,8 +688,11 @@ s390_prepare_to_resume (struct lwp_info *lp)
   parea.len = sizeof (per_info);
   parea.process_addr = (addr_t) & per_info;
   parea.kernel_addr = offsetof (struct user_regs_struct, per_info);
-  if (ptrace (PTRACE_PEEKUSR_AREA, tid, &parea, 0) < 0)
-    perror_with_name (_("Couldn't retrieve watchpoint status"));
+
+  /* Clear PER info, but adjust the single_step field (used by older
+     kernels only).  */
+  memset (&per_info, 0, sizeof (per_info));
+  per_info.single_step = (step != 0);
 
   if (!VEC_empty (s390_watch_area, state->watch_areas))
     {
@@ -675,13 +704,45 @@ s390_prepare_to_resume (struct lwp_info *lp)
 	  watch_hi_addr = max (watch_hi_addr, area->hi_addr);
 	}
 
-      per_info.control_regs.bits.em_storage_alteration = 1;
-      per_info.control_regs.bits.storage_alt_space_ctl = 1;
+      /* Enable storage-alteration events.  */
+      per_info.control_regs.words.cr[0] |= (PER_EVENT_STORE
+					    | PER_CONTROL_ALTERATION);
     }
-  else
+
+  if (!VEC_empty (s390_watch_area, state->break_areas))
     {
-      per_info.control_regs.bits.em_storage_alteration = 0;
-      per_info.control_regs.bits.storage_alt_space_ctl = 0;
+      /* Don't install hardware breakpoints while single-stepping.  But
+	 re-install them afterwards.  */
+      if (step)
+	lp_priv->per_info_changed = 1;
+      else
+	{
+	  for (ix = 0;
+	       VEC_iterate (s390_watch_area, state->break_areas, ix, area);
+	       ix++)
+	    {
+	      watch_lo_addr = min (watch_lo_addr, area->lo_addr);
+	      watch_hi_addr = max (watch_hi_addr, area->hi_addr);
+	    }
+
+	  /* If there's just one breakpoint, enable instruction-fetching
+	     nullification events for the breakpoint address (fast).
+	     Otherwise stop after any instruction within the PER area and
+	     after any branch into it (slow).  */
+	  if (watch_hi_addr == watch_lo_addr)
+	    per_info.control_regs.words.cr[0] |= (PER_EVENT_NULLIFICATION
+						  | PER_EVENT_IFETCH);
+	  else
+	    {
+	      /* The PER area must include the instruction before the
+		 first breakpoint address.  */
+	      watch_lo_addr = watch_lo_addr > 6 ? watch_lo_addr - 6 : 0;
+	      per_info.control_regs.words.cr[0]
+		|= (PER_EVENT_BRANCH
+		    | PER_EVENT_IFETCH
+		    | PER_CONTROL_BRANCH_ADDRESS);
+	    }
+	}
     }
   per_info.starting_addr = watch_lo_addr;
   per_info.ending_addr = watch_hi_addr;
@@ -777,11 +838,61 @@ s390_remove_watchpoint (struct target_ops *self,
   return -1;
 }
 
+/* Implement the "can_use_hw_breakpoint" target_ops method. */
+
 static int
 s390_can_use_hw_breakpoint (struct target_ops *self,
 			    enum bptype type, int cnt, int othertype)
 {
-  return type == bp_hardware_watchpoint;
+  if (type == bp_hardware_watchpoint || type == bp_hardware_breakpoint)
+    return 1;
+  return 0;
+}
+
+/* Implement the "insert_hw_breakpoint" target_ops method.  */
+
+static int
+s390_insert_hw_breakpoint (struct target_ops *self,
+			   struct gdbarch *gdbarch,
+			   struct bp_target_info *bp_tgt)
+{
+  s390_watch_area area;
+  struct s390_debug_reg_state *state;
+
+  area.lo_addr = bp_tgt->placed_address = bp_tgt->reqstd_address;
+  area.hi_addr = area.lo_addr;
+  state = s390_get_debug_reg_state (ptid_get_pid (inferior_ptid));
+  VEC_safe_push (s390_watch_area, state->break_areas, &area);
+
+  return s390_refresh_per_info ();
+}
+
+/* Implement the "remove_hw_breakpoint" target_ops method.  */
+
+static int
+s390_remove_hw_breakpoint (struct target_ops *self,
+			   struct gdbarch *gdbarch,
+			   struct bp_target_info *bp_tgt)
+{
+  unsigned ix;
+  struct watch_area *area;
+  struct s390_debug_reg_state *state;
+
+  state = s390_get_debug_reg_state (ptid_get_pid (inferior_ptid));
+  for (ix = 0;
+       VEC_iterate (s390_watch_area, state->break_areas, ix, area);
+       ix++)
+    {
+      if (area->lo_addr == bp_tgt->placed_address)
+	{
+	  VEC_unordered_remove (s390_watch_area, state->break_areas, ix);
+	  return s390_refresh_per_info ();
+	}
+    }
+
+  fprintf_unfiltered (gdb_stderr,
+		      "Attempt to remove nonexistent breakpoint.\n");
+  return -1;
 }
 
 static int
@@ -904,6 +1015,8 @@ _initialize_s390_nat (void)
 
   /* Add our watchpoint methods.  */
   t->to_can_use_hw_breakpoint = s390_can_use_hw_breakpoint;
+  t->to_insert_hw_breakpoint = s390_insert_hw_breakpoint;
+  t->to_remove_hw_breakpoint = s390_remove_hw_breakpoint;
   t->to_region_ok_for_hw_watchpoint = s390_region_ok_for_hw_watchpoint;
   t->to_have_continuable_watchpoint = 1;
   t->to_stopped_by_watchpoint = s390_stopped_by_watchpoint;
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index e538812..758db46 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -2953,7 +2953,8 @@ proc skip_hw_breakpoint_tests {} {
 	 || [istarget "x86_64-*-*"]
 	 || [istarget "ia64-*-*"] 
 	 || [istarget "arm*-*-*"]
-	 || [istarget "aarch64*-*-*"]} {
+	 || [istarget "aarch64*-*-*"]
+	 || [istarget "s390*-*-*"] } {
 	return 0
     }
 
-- 
2.5.0

[PATCH 5/6] linux-nat: Add function lwp_is_stepping

[Andreas Arnez]

Add the function lwp_is_stepping which indicates whether the given LWP
is currently single-stepping.  This is a common interface, usable from
native GDB as well as from gdbserver.

gdb/gdbserver/ChangeLog:

	* linux-low.c (lwp_is_stepping): New function.

gdb/ChangeLog:

	* nat/linux-nat.h (lwp_is_stepping): New declaration.
	* linux-nat.c (lwp_is_stepping): New function.
---
 gdb/gdbserver/linux-low.c | 8 ++++++++
 gdb/linux-nat.c           | 8 ++++++++
 gdb/nat/linux-nat.h       | 4 ++++
 3 files changed, 20 insertions(+)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index fb70715..4203b92 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -176,6 +176,14 @@ lwp_stop_reason (struct lwp_info *lwp)
   return lwp->stop_reason;
 }
 
+/* See nat/linux-nat.h.  */
+
+int
+lwp_is_stepping (struct lwp_info *lwp)
+{
+  return lwp->stepping;
+}
+
 /* A list of all unknown processes which receive stop signals.  Some
    other process will presumably claim each of these as forked
    children momentarily.  */
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 7410f8e..8e9eb0b 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -355,6 +355,14 @@ lwp_stop_reason (struct lwp_info *lwp)
   return lwp->stop_reason;
 }
 
+/* See nat/linux-nat.h.  */
+
+int
+lwp_is_stepping (struct lwp_info *lwp)
+{
+  return lwp->step;
+}
+
 \f
 /* Trivial list manipulation functions to keep track of a list of
    new stopped processes.  */
diff --git a/gdb/nat/linux-nat.h b/gdb/nat/linux-nat.h
index 2b485db..f7d1c63 100644
--- a/gdb/nat/linux-nat.h
+++ b/gdb/nat/linux-nat.h
@@ -85,4 +85,8 @@ extern enum target_stop_reason lwp_stop_reason (struct lwp_info *lwp);
 
 extern void linux_stop_lwp (struct lwp_info *lwp);
 
+/* Return nonzero if we are single-stepping this LWP.  */
+
+extern int lwp_is_stepping (struct lwp_info *lwp);
+
 #endif /* LINUX_NAT_H */
-- 
2.5.0

Re: [PATCH 5/6] linux-nat: Add function lwp_is_stepping

[Yao Qi]

Hi Andreas,
Patch is good to me.  One nit below ...

On Thu, Sep 15, 2016 at 1:01 PM, Andreas Arnez <arnez@linux.vnet.ibm.com> wrote:
> Add the function lwp_is_stepping which indicates whether the given LWP
> is currently single-stepping.  This is a common interface, usable from
> native GDB as well as from gdbserver.

"single-stepping" in hardware or software?  I know it is hardware single
step.

>
> +/* Return nonzero if we are single-stepping this LWP.  */
> +
> +extern int lwp_is_stepping (struct lwp_info *lwp);
> +
>  #endif /* LINUX_NAT_H */

in gdbserver,
struct lwp_info
{
 ....
  /* If this flag is set, the last continue operation at the ptrace
     level on this process was a single-step.  */
  int stepping;

in gdb,
struct lwp_info
{
...
  /* Non-zero if we were stepping this LWP.  */
  int step;

Looks the comments in gdbservers is better than the comments in
gdb.  lwp_is_stepping, as an api, should be documented clearly.
Something like,

/* Return nonzero if we are single-stepping this LWP at the
    ptrace level.  */

-- 
Yao (齐尧)

Re: [PATCH 0/6] S390: Watchpoint enhancements and hardware breakpoints

[Pedro Alves]

On 09/15/2016 12:52 PM, Andreas Arnez wrote:
> This series adds some enhancements to the current s390-linux native
> support for watchpoints.  It also adds hardware breakpoint support.
> 
> While this is mostly an s390-specific series, it includes one
> common-code patch: patch #5, "linux-nat: Add function lwp_is_stepping".
> I'm looking for feedback on that.  Comments to the s390-specific patches
> are certainly welcome as well.

I'm curious on the need to disable hw breakpoints when single-stepping.

It's of course more efficient if we manage to disable the breakpoint just
for the thread that is stepping, without disturbing other threads, but
OTOH, infrun's handling of stepping over breakpoints should already be 
making sure that threads can step past breakpoints somehow?  I.e.,
without displaced stepping, infrun should be removing breakpoints from
all threads, and with displaced stepping, we'll be stepping the thread
at a different PC so the hardware breakpoint at the original PC should
not trigger.

Thanks,
Pedro Alves

Re: [PATCH 0/6] S390: Watchpoint enhancements and hardware breakpoints

[Ulrich Weigand]

Pedro Alves wrote:
> On 09/15/2016 12:52 PM, Andreas Arnez wrote:
> > This series adds some enhancements to the current s390-linux native
> > support for watchpoints.  It also adds hardware breakpoint support.
> > 
> > While this is mostly an s390-specific series, it includes one
> > common-code patch: patch #5, "linux-nat: Add function lwp_is_stepping".
> > I'm looking for feedback on that.  Comments to the s390-specific patches
> > are certainly welcome as well.
> 
> I'm curious on the need to disable hw breakpoints when single-stepping.

This is more of a hardware restriction than a GDB issue.  The problem is
that s390 hardware doesn't really have a separate hardware single-step
feature; to implement ptrace single-stepping, the kernel uses the s390
hardware breakpoint feature (in effect, enabling hardware breakpoints
on the full address space, which will hit on the next instruction that
is executed).

This use of the feature by kernel single-stepping can conflict with
explicit use of the same feature by GDB; that's why it is better to
not attempt such explicit use while at the same time also requesting
kernel single-stepping.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH 0/6] S390: Watchpoint enhancements and hardware breakpoints

[Pedro Alves]

On 09/16/2016 01:43 PM, Ulrich Weigand wrote:
> Pedro Alves wrote:

>> I'm curious on the need to disable hw breakpoints when single-stepping.
> 
> This is more of a hardware restriction than a GDB issue.  The problem is
> that s390 hardware doesn't really have a separate hardware single-step
> feature; to implement ptrace single-stepping, the kernel uses the s390
> hardware breakpoint feature (in effect, enabling hardware breakpoints
> on the full address space, which will hit on the next instruction that
> is executed).
> 
> This use of the feature by kernel single-stepping can conflict with
> explicit use of the same feature by GDB; that's why it is better to
> not attempt such explicit use while at the same time also requesting
> kernel single-stepping.

I see.  Thanks for the clarification.

A comment to the effect in the code would be nice.  Maybe it's
there and I missed it.

I have no further comments on the series.  LGTM.

Thanks,
Pedro Alves

Re: [PATCH 0/6] S390: Watchpoint enhancements and hardware breakpoints

[Andreas Arnez]

On Fri, Sep 16 2016, Pedro Alves wrote:

> I see.  Thanks for the clarification.
>
> A comment to the effect in the code would be nice.  Maybe it's
> there and I missed it.

OK, changed the relevant comment to this:

  /* Don't install hardware breakpoints while single-stepping, since
     our PER settings (e.g. the nullification bit) might then conflict
     with the kernel's.  But re-install them afterwards.  */

Also changed the documentation of lwp_is_stepping as suggested by Yao:

  /* Return nonzero if we are single-stepping this LWP at the ptrace
     level.  */

> I have no further comments on the series.  LGTM.

Thanks.  Pushed with the changes above.

--
Andreas

Re: [PATCH 0/6] S390: Watchpoint enhancements and hardware breakpoints

[Yao Qi]

On Thu, Sep 15, 2016 at 12:52 PM, Andreas Arnez
<arnez@linux.vnet.ibm.com> wrote:
> This series adds some enhancements to the current s390-linux native
> support for watchpoints.  It also adds hardware breakpoint support.
>

If you also plan to support watchpoint in GDBserver, It would be nice
if you can move all HW watchpoint control code into nat/ directory,
like aarch64-linux-hw-point.c and mips-linux-watch.c, so that
GDBserver can use them easily.

To be clear, I am not against your approach.

-- 
Yao (齐尧)

Re: [PATCH 0/6] S390: Watchpoint enhancements and hardware breakpoints

[Andreas Arnez]

On Thu, Sep 15 2016, Yao Qi wrote:

> On Thu, Sep 15, 2016 at 12:52 PM, Andreas Arnez
> <arnez@linux.vnet.ibm.com> wrote:
>> This series adds some enhancements to the current s390-linux native
>> support for watchpoints.  It also adds hardware breakpoint support.
>>
>
> If you also plan to support watchpoint in GDBserver, It would be nice
> if you can move all HW watchpoint control code into nat/ directory,
> like aarch64-linux-hw-point.c and mips-linux-watch.c, so that
> GDBserver can use them easily.

Agreed.  I'm planning to perform this move when actually adding
gdbserver support.

--
Andreas