[PATCH] Handle MIPS Linux SIGTRAP siginfo.si_code values

[PATCH] Handle MIPS Linux SIGTRAP siginfo.si_code values

[Pedro Alves]

This unbreaks pending/delayed breakpoints handling, as well as
hardware watchpoints, on MIPS.

Ref: https://sourceware.org/ml/gdb-patches/2016-02/msg00681.html

The MIPS kernel reports SI_KERNEL for all kernel generated traps,
instead of TRAP_BRKPT / TRAP_HWBKPT, but GDB isn't aware of this.

Basically, this commit:

- Folds watchpoints logic into check_stopped_by_breakpoint, and
  renames it to save_stop_reason.

- Adds GDB_ARCH_IS_TRAP_HWBKPT.

- Makes MIPS set both GDB_ARCH_IS_TRAP_BRPT and
  GDB_ARCH_IS_TRAP_HWBKPT to SI_KERNEL.  In save_stop_reason, we
  handle the case of the same si_code returning true for both
  TRAP_BRPT and TRAP_HWBKPT by looking at what the debug registers
  say.

Tested on x86-64 Fedora 20, native and gdbserver.

gdb/ChangeLog:
2016-02-24  Pedro Alves  <palves@redhat.com>

	* linux-nat.c (save_sigtrap) Delete.
	(stop_wait_callback): Call save_stop_reason instead of
	save_sigtrap.
	(check_stopped_by_breakpoint): Rename to ...
	(save_stop_reason): ... this.  Bits of save_sigtrap folded here.
	Use GDB_ARCH_IS_TRAP_HWBKPT and handle ambiguous
	GDB_ARCH_IS_TRAP_BRKPT / GDB_ARCH_IS_TRAP_HWBKPT.  Factor out
	common code between the USE_SIGTRAP_SIGINFO and
	!USE_SIGTRAP_SIGINFO blocks.
	(linux_nat_filter_event): Call save_stop_reason instead of
	save_sigtrap.
	* nat/linux-ptrace.h: Check for both SI_KERNEL and TRAP_BRKPT
	si_code for MIPS.
	* nat/linux-ptrace.h: Fix "TRAP_HWBPT" typo in x86 table.  Add
	comments on MIPS behavior.
	(GDB_ARCH_IS_TRAP_HWBKPT): Define for all archs.

gdb/gdbserver/ChangeLog:
2016-02-24  Pedro Alves  <palves@redhat.com>

	* linux-low.c (check_stopped_by_breakpoint): Rename to ...
	(save_stop_reason): ... this.  Use GDB_ARCH_IS_TRAP_HWBKPT and
	handle ambiguous GDB_ARCH_IS_TRAP_BRKPT / GDB_ARCH_IS_TRAP_HWBKPT.
	Factor out common code between the USE_SIGTRAP_SIGINFO and
	!USE_SIGTRAP_SIGINFO blocks.
	(linux_low_filter_event): Call save_stop_reason instead of
	check_stopped_by_breakpoint and check_stopped_by_watchpoint.
	Update comments.
	(linux_wait_1): Update comments.
---
 gdb/gdbserver/linux-low.c | 170 ++++++++++++++++++++--------------------------
 gdb/linux-nat.c           | 149 ++++++++++++++++++++--------------------
 gdb/nat/linux-ptrace.h    |  38 ++++++++---
 3 files changed, 172 insertions(+), 185 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 8b025bd..b02ad31 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -735,32 +735,16 @@ get_syscall_trapinfo (struct lwp_info *lwp, int *sysno, int *sysret)
   current_thread = saved_thread;
 }
 
-/* This function should only be called if LWP got a SIGTRAP.
-   The SIGTRAP could mean several things.
+static int check_stopped_by_watchpoint (struct lwp_info *child);
 
-   On i386, where decr_pc_after_break is non-zero:
-
-   If we were single-stepping this process using PTRACE_SINGLESTEP, we
-   will get only the one SIGTRAP.  The value of $eip will be the next
-   instruction.  If the instruction we stepped over was a breakpoint,
-   we need to decrement the PC.
-
-   If we continue the process using PTRACE_CONT, we will get a
-   SIGTRAP when we hit a breakpoint.  The value of $eip will be
-   the instruction after the breakpoint (i.e. needs to be
-   decremented).  If we report the SIGTRAP to GDB, we must also
-   report the undecremented PC.  If the breakpoint is removed, we
-   must resume at the decremented PC.
-
-   On a non-decr_pc_after_break machine with hardware or kernel
-   single-step:
-
-   If we either single-step a breakpoint instruction, or continue and
-   hit a breakpoint instruction, our PC will point at the breakpoint
-   instruction.  */
+/* Called when the LWP stopped for a signal/trap.  If it stopped for a
+   trap check what caused it (breakpoint, watchpoint, trace, etc.),
+   and save the result in the LWP's stop_reason field.  If it stopped
+   for a breakpoint, decrement the PC if necessary on the lwp's
+   architecture.  Returns true if we now have the LWP's stop PC.  */
 
 static int
-check_stopped_by_breakpoint (struct lwp_info *lwp)
+save_stop_reason (struct lwp_info *lwp)
 {
   CORE_ADDR pc;
   CORE_ADDR sw_breakpoint_pc;
@@ -785,56 +769,39 @@ check_stopped_by_breakpoint (struct lwp_info *lwp)
     {
       if (siginfo.si_signo == SIGTRAP)
 	{
-	  if (GDB_ARCH_IS_TRAP_BRKPT (siginfo.si_code))
+	  if (GDB_ARCH_IS_TRAP_BRKPT (siginfo.si_code)
+	      && GDB_ARCH_IS_TRAP_HWBKPT (siginfo.si_code))
 	    {
-	      if (debug_threads)
-		{
-		  struct thread_info *thr = get_lwp_thread (lwp);
-
-		  debug_printf ("CSBB: %s stopped by software breakpoint\n",
-				target_pid_to_str (ptid_of (thr)));
-		}
-
-	      /* Back up the PC if necessary.  */
-	      if (pc != sw_breakpoint_pc)
-		{
-		  struct regcache *regcache
-		    = get_thread_regcache (current_thread, 1);
-		  (*the_low_target.set_pc) (regcache, sw_breakpoint_pc);
-		}
-
-	      lwp->stop_pc = sw_breakpoint_pc;
+	      /* The si_code is ambiguous on this arch -- check debug
+		 registers.  */
+	      if (!check_stopped_by_watchpoint (lwp))
+		lwp->stop_reason = TARGET_STOPPED_BY_SW_BREAKPOINT;
+	    }
+	  else if (GDB_ARCH_IS_TRAP_BRKPT (siginfo.si_code))
+	    {
+	      /* If we determine the LWP stopped for a SW breakpoint,
+		 trust it.  Particularly don't check watchpoint
+		 registers, because at least on s390, we'd find
+		 stopped-by-watchpoint as long as there's a watchpoint
+		 set.  */
 	      lwp->stop_reason = TARGET_STOPPED_BY_SW_BREAKPOINT;
-	      current_thread = saved_thread;
-	      return 1;
 	    }
-	  else if (siginfo.si_code == TRAP_HWBKPT)
+	  else if (GDB_ARCH_IS_TRAP_HWBKPT (siginfo.si_code))
 	    {
-	      if (debug_threads)
-		{
-		  struct thread_info *thr = get_lwp_thread (lwp);
-
-		  debug_printf ("CSBB: %s stopped by hardware "
-				"breakpoint/watchpoint\n",
-				target_pid_to_str (ptid_of (thr)));
-		}
-
-	      lwp->stop_pc = pc;
-	      lwp->stop_reason = TARGET_STOPPED_BY_HW_BREAKPOINT;
-	      current_thread = saved_thread;
-	      return 1;
+	      /* This can indicate either a hardware breakpoint or
+		 hardware watchpoint.  Check debug registers.  */
+	      if (!check_stopped_by_watchpoint (lwp))
+		lwp->stop_reason = TARGET_STOPPED_BY_HW_BREAKPOINT;
 	    }
 	  else if (siginfo.si_code == TRAP_TRACE)
 	    {
-	      if (debug_threads)
-		{
-		  struct thread_info *thr = get_lwp_thread (lwp);
-
-		  debug_printf ("CSBB: %s stopped by trace\n",
-				target_pid_to_str (ptid_of (thr)));
-		}
-
-	      lwp->stop_reason = TARGET_STOPPED_BY_SINGLE_STEP;
+	      /* We may have single stepped an instruction that
+		 triggered a watchpoint.  In that case, on some
+		 architectures (such as x86), instead of TRAP_HWBKPT,
+		 si_code indicates TRAP_TRACE, and we need to check
+		 the debug registers separately.  */
+	      if (!check_stopped_by_watchpoint (lwp))
+		lwp->stop_reason = TARGET_STOPPED_BY_SINGLE_STEP;
 	    }
 	}
     }
@@ -845,6 +812,16 @@ check_stopped_by_breakpoint (struct lwp_info *lwp)
      case we need to report the breakpoint PC.  */
   if ((!lwp->stepping || lwp->stop_pc == sw_breakpoint_pc)
       && (*the_low_target.breakpoint_at) (sw_breakpoint_pc))
+    lwp->stop_reason = TARGET_STOPPED_BY_SW_BREAKPOINT;
+
+  if (hardware_breakpoint_inserted_here (pc))
+    lwp->stop_reason = TARGET_STOPPED_BY_HW_BREAKPOINT;
+
+  if (lp->stop_reason == TARGET_STOPPED_BY_NO_REASON)
+    check_stopped_by_watchpoint (lp);
+#endif
+
+  if (lwp->stop_reason == TARGET_STOPPED_BY_SW_BREAKPOINT)
     {
       if (debug_threads)
 	{
@@ -856,19 +833,16 @@ check_stopped_by_breakpoint (struct lwp_info *lwp)
 
       /* Back up the PC if necessary.  */
       if (pc != sw_breakpoint_pc)
-        {
+	{
 	  struct regcache *regcache
 	    = get_thread_regcache (current_thread, 1);
 	  (*the_low_target.set_pc) (regcache, sw_breakpoint_pc);
 	}
 
-      lwp->stop_pc = sw_breakpoint_pc;
-      lwp->stop_reason = TARGET_STOPPED_BY_SW_BREAKPOINT;
-      current_thread = saved_thread;
-      return 1;
+      /* Update this so we record the correct stop PC below.  */
+      pc = sw_breakpoint_pc;
     }
-
-  if (hardware_breakpoint_inserted_here (pc))
+  else if (lwp->stop_reason == TARGET_STOPPED_BY_HW_BREAKPOINT)
     {
       if (debug_threads)
 	{
@@ -877,16 +851,31 @@ check_stopped_by_breakpoint (struct lwp_info *lwp)
 	  debug_printf ("CSBB: %s stopped by hardware breakpoint\n",
 			target_pid_to_str (ptid_of (thr)));
 	}
+    }
+  else if (lwp->stop_reason == TARGET_STOPPED_BY_WATCHPOINT)
+    {
+      if (debug_threads)
+	{
+	  struct thread_info *thr = get_lwp_thread (lwp);
 
-      lwp->stop_pc = pc;
-      lwp->stop_reason = TARGET_STOPPED_BY_HW_BREAKPOINT;
-      current_thread = saved_thread;
-      return 1;
+	  debug_printf ("CSBB: %s stopped by hardware watchpoint\n",
+			target_pid_to_str (ptid_of (thr)));
+	}
     }
-#endif
+  else if (lwp->stop_reason == TARGET_STOPPED_BY_SINGLE_STEP)
+    {
+      if (debug_threads)
+	{
+	  struct thread_info *thr = get_lwp_thread (lwp);
 
+	  debug_printf ("CSBB: %s stopped by trace\n",
+			target_pid_to_str (ptid_of (thr)));
+	}
+    }
+
+  lwp->stop_pc = pc;
   current_thread = saved_thread;
-  return 0;
+  return 1;
 }
 
 static struct lwp_info *
@@ -2434,8 +2423,8 @@ linux_low_filter_event (int lwpid, int wstat)
       child->syscall_state = TARGET_WAITKIND_IGNORE;
     }
 
-  /* Be careful to not overwrite stop_pc until
-     check_stopped_by_breakpoint is called.  */
+  /* Be careful to not overwrite stop_pc until save_stop_reason is
+     called.  */
   if (WIFSTOPPED (wstat) && WSTOPSIG (wstat) == SIGTRAP
       && linux_is_extended_waitstatus (wstat))
     {
@@ -2448,27 +2437,12 @@ linux_low_filter_event (int lwpid, int wstat)
 	}
     }
 
-  /* Check first whether this was a SW/HW breakpoint before checking
-     watchpoints, because at least s390 can't tell the data address of
-     hardware watchpoint hits, and returns stopped-by-watchpoint as
-     long as there's a watchpoint set.  */
   if (WIFSTOPPED (wstat) && linux_wstatus_maybe_breakpoint (wstat))
     {
-      if (check_stopped_by_breakpoint (child))
+      if (save_stop_reason (child))
 	have_stop_pc = 1;
     }
 
-  /* Note that TRAP_HWBKPT can indicate either a hardware breakpoint
-     or hardware watchpoint.  Check which is which if we got
-     TARGET_STOPPED_BY_HW_BREAKPOINT.  Likewise, we may have single
-     stepped an instruction that triggered a watchpoint.  In that
-     case, on some architectures (such as x86), instead of
-     TRAP_HWBKPT, si_code indicates TRAP_TRACE, and we need to check
-     the debug registers separately.  */
-  if (WIFSTOPPED (wstat) && WSTOPSIG (wstat) == SIGTRAP
-      && child->stop_reason != TARGET_STOPPED_BY_SW_BREAKPOINT)
-    check_stopped_by_watchpoint (child);
-
   if (!have_stop_pc)
     child->stop_pc = get_pc (child);
 
@@ -3209,7 +3183,7 @@ linux_wait_1 (ptid_t ptid,
      hardware single step it means a gdb/gdbserver breakpoint had been
      planted on top of a permanent breakpoint, in the case of a software
      single step it may just mean that gdbserver hit the reinsert breakpoint.
-     The PC has been adjusted by check_stopped_by_breakpoint to point at
+     The PC has been adjusted by save_stop_reason to point at
      the breakpoint address.
      So in the case of the hardware single step advance the PC manually
      past the breakpoint and in the case of software single step advance only
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 6ded38d..0829bcb 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -303,10 +303,11 @@ static struct lwp_info *find_lwp_pid (ptid_t ptid);
 
 static int lwp_status_pending_p (struct lwp_info *lp);
 
-static int check_stopped_by_breakpoint (struct lwp_info *lp);
 static int sigtrap_is_event (int status);
 static int (*linux_nat_status_is_event) (int status) = sigtrap_is_event;
 
+static void save_stop_reason (struct lwp_info *lp);
+
 \f
 /* LWP accessors.  */
 
@@ -2321,30 +2322,6 @@ check_stopped_by_watchpoint (struct lwp_info *lp)
   return lp->stop_reason == TARGET_STOPPED_BY_WATCHPOINT;
 }
 
-/* Called when the LWP stopped for a trap that could be explained by a
-   watchpoint or a breakpoint.  */
-
-static void
-save_sigtrap (struct lwp_info *lp)
-{
-  gdb_assert (lp->stop_reason == TARGET_STOPPED_BY_NO_REASON);
-  gdb_assert (lp->status != 0);
-
-  /* Check first if this was a SW/HW breakpoint before checking
-     watchpoints, because at least s390 can't tell the data address of
-     hardware watchpoint hits, and the kernel returns
-     stopped-by-watchpoint as long as there's a watchpoint set.  */
-  if (linux_nat_status_is_event (lp->status))
-    check_stopped_by_breakpoint (lp);
-
-  /* Note that TRAP_HWBKPT can indicate either a hardware breakpoint
-     or hardware watchpoint.  Check which is which if we got
-     TARGET_STOPPED_BY_HW_BREAKPOINT.  */
-  if (lp->stop_reason == TARGET_STOPPED_BY_NO_REASON
-      || lp->stop_reason == TARGET_STOPPED_BY_HW_BREAKPOINT)
-    check_stopped_by_watchpoint (lp);
-}
-
 /* Returns true if the LWP had stopped for a watchpoint.  */
 
 static int
@@ -2441,7 +2418,7 @@ stop_wait_callback (struct lwp_info *lp, void *data)
 	  /* Save the sigtrap event.  */
 	  lp->status = status;
 	  gdb_assert (lp->signalled);
-	  save_sigtrap (lp);
+	  save_stop_reason (lp);
 	}
       else
 	{
@@ -2583,29 +2560,32 @@ select_event_lwp_callback (struct lwp_info *lp, void *data)
   return 0;
 }
 
-/* Called when the LWP got a signal/trap that could be explained by a
-   software or hardware breakpoint.  */
+/* Called when the LWP stopped for a signal/trap.  If it stopped for a
+   trap check what caused it (breakpoint, watchpoint, trace, etc.),
+   and save the result in the LWP's stop_reason field.  If it stopped
+   for a breakpoint, decrement the PC if necessary on the lwp's
+   architecture.  */
 
-static int
-check_stopped_by_breakpoint (struct lwp_info *lp)
+static void
+save_stop_reason (struct lwp_info *lp)
 {
-  /* Arrange for a breakpoint to be hit again later.  We don't keep
-     the SIGTRAP status and don't forward the SIGTRAP signal to the
-     LWP.  We will handle the current event, eventually we will resume
-     this LWP, and this breakpoint will trap again.
-
-     If we do not do this, then we run the risk that the user will
-     delete or disable the breakpoint, but the LWP will have already
-     tripped on it.  */
-
-  struct regcache *regcache = get_thread_regcache (lp->ptid);
-  struct gdbarch *gdbarch = get_regcache_arch (regcache);
+  struct regcache *regcache;
+  struct gdbarch *gdbarch;
   CORE_ADDR pc;
   CORE_ADDR sw_bp_pc;
 #if USE_SIGTRAP_SIGINFO
   siginfo_t siginfo;
 #endif
 
+  gdb_assert (lp->stop_reason == TARGET_STOPPED_BY_NO_REASON);
+  gdb_assert (lp->status != 0);
+
+  if (!linux_nat_status_is_event (lp->status))
+    return;
+
+  regcache = get_thread_regcache (lp->ptid);
+  gdbarch = get_regcache_arch (regcache);
+
   pc = regcache_read_pc (regcache);
   sw_bp_pc = pc - gdbarch_decr_pc_after_break (gdbarch);
 
@@ -2614,33 +2594,29 @@ check_stopped_by_breakpoint (struct lwp_info *lp)
     {
       if (siginfo.si_signo == SIGTRAP)
 	{
-	  if (GDB_ARCH_IS_TRAP_BRKPT (siginfo.si_code))
+	  if (GDB_ARCH_IS_TRAP_BRKPT (siginfo.si_code)
+	      && GDB_ARCH_IS_TRAP_HWBKPT (siginfo.si_code))
 	    {
-	      if (debug_linux_nat)
-		fprintf_unfiltered (gdb_stdlog,
-				    "CSBB: %s stopped by software "
-				    "breakpoint\n",
-				    target_pid_to_str (lp->ptid));
-
-	      /* Back up the PC if necessary.  */
-	      if (pc != sw_bp_pc)
-		regcache_write_pc (regcache, sw_bp_pc);
-
-	      lp->stop_pc = sw_bp_pc;
+	      /* The si_code is ambiguous on this arch -- check debug
+		 registers.  */
+	      if (!check_stopped_by_watchpoint (lp))
+		lp->stop_reason = TARGET_STOPPED_BY_SW_BREAKPOINT;
+	    }
+	  else if (GDB_ARCH_IS_TRAP_BRKPT (siginfo.si_code))
+	    {
+	      /* If we determine the LWP stopped for a SW breakpoint,
+		 trust it.  Particularly don't check watchpoint
+		 registers, because at least on s390, we'd find
+		 stopped-by-watchpoint as long as there's a watchpoint
+		 set.  */
 	      lp->stop_reason = TARGET_STOPPED_BY_SW_BREAKPOINT;
-	      return 1;
 	    }
-	  else if (siginfo.si_code == TRAP_HWBKPT)
+	  else if (GDB_ARCH_IS_TRAP_HWBKPT (siginfo.si_code))
 	    {
-	      if (debug_linux_nat)
-		fprintf_unfiltered (gdb_stdlog,
-				    "CSBB: %s stopped by hardware "
-				    "breakpoint/watchpoint\n",
-				    target_pid_to_str (lp->ptid));
-
-	      lp->stop_pc = pc;
-	      lp->stop_reason = TARGET_STOPPED_BY_HW_BREAKPOINT;
-	      return 1;
+	      /* This can indicate either a hardware breakpoint or
+		 hardware watchpoint.  Check debug registers.  */
+	      if (!check_stopped_by_watchpoint (lp))
+		lp->stop_reason = TARGET_STOPPED_BY_HW_BREAKPOINT;
 	    }
 	  else if (siginfo.si_code == TRAP_TRACE)
 	    {
@@ -2648,6 +2624,13 @@ check_stopped_by_breakpoint (struct lwp_info *lp)
 		fprintf_unfiltered (gdb_stdlog,
 				    "CSBB: %s stopped by trace\n",
 				    target_pid_to_str (lp->ptid));
+
+	      /* We may have single stepped an instruction that
+		 triggered a watchpoint.  In that case, on some
+		 architectures (such as x86), instead of TRAP_HWBKPT,
+		 si_code indicates TRAP_TRACE, and we need to check
+		 the debug registers separately.  */
+	      check_stopped_by_watchpoint (lp);
 	    }
 	}
     }
@@ -2658,6 +2641,18 @@ check_stopped_by_breakpoint (struct lwp_info *lp)
     {
       /* The LWP was either continued, or stepped a software
 	 breakpoint instruction.  */
+      lp->stop_reason = TARGET_STOPPED_BY_SW_BREAKPOINT;
+    }
+
+  if (hardware_breakpoint_inserted_here_p (get_regcache_aspace (regcache), pc))
+    lp->stop_reason = TARGET_STOPPED_BY_HW_BREAKPOINT;
+
+  if (lp->stop_reason == TARGET_STOPPED_BY_NO_REASON)
+    check_stopped_by_watchpoint (lp);
+#endif
+
+  if (lp->stop_reason == TARGET_STOPPED_BY_SW_BREAKPOINT)
+    {
       if (debug_linux_nat)
 	fprintf_unfiltered (gdb_stdlog,
 			    "CSBB: %s stopped by software breakpoint\n",
@@ -2667,25 +2662,25 @@ check_stopped_by_breakpoint (struct lwp_info *lp)
       if (pc != sw_bp_pc)
 	regcache_write_pc (regcache, sw_bp_pc);
 
-      lp->stop_pc = sw_bp_pc;
-      lp->stop_reason = TARGET_STOPPED_BY_SW_BREAKPOINT;
-      return 1;
+      /* Update this so we record the correct stop PC below.  */
+      pc = sw_bp_pc;
     }
-
-  if (hardware_breakpoint_inserted_here_p (get_regcache_aspace (regcache), pc))
+  else if (lp->stop_reason == TARGET_STOPPED_BY_HW_BREAKPOINT)
     {
       if (debug_linux_nat)
 	fprintf_unfiltered (gdb_stdlog,
-			    "CSBB: stopped by hardware breakpoint %s\n",
+			    "CSBB: %s stopped by hardware breakpoint\n",
+			    target_pid_to_str (lp->ptid));
+    }
+  else if (lp->stop_reason == TARGET_STOPPED_BY_WATCHPOINT)
+    {
+      if (debug_linux_nat)
+	fprintf_unfiltered (gdb_stdlog,
+			    "CSBB: %s stopped by hardware watchpoint\n",
 			    target_pid_to_str (lp->ptid));
-
-      lp->stop_pc = pc;
-      lp->stop_reason = TARGET_STOPPED_BY_HW_BREAKPOINT;
-      return 1;
     }
-#endif
 
-  return 0;
+  lp->stop_pc = pc;
 }
 
 
@@ -3057,7 +3052,7 @@ linux_nat_filter_event (int lwpid, int status)
   /* An interesting event.  */
   gdb_assert (lp);
   lp->status = status;
-  save_sigtrap (lp);
+  save_stop_reason (lp);
   return lp;
 }
 
diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h
index ba58717..1d137e8 100644
--- a/gdb/nat/linux-ptrace.h
+++ b/gdb/nat/linux-ptrace.h
@@ -119,14 +119,14 @@ struct buffer;
 /* The x86 kernel gets some of the si_code values backwards, like
    this:
 
-   | what                                     | si_code    |
-   |------------------------------------------+------------|
-   | software breakpoints (int3)              | SI_KERNEL  |
-   | single-steps                             | TRAP_TRACE |
-   | single-stepping a syscall                | TRAP_BRKPT |
-   | user sent SIGTRAP                        | 0          |
-   | exec SIGTRAP (when no PTRACE_EVENT_EXEC) | 0          |
-   | hardware breakpoints/watchpoints         | TRAP_HWBPT |
+   | what                                     | si_code     |
+   |------------------------------------------+-------------|
+   | software breakpoints (int3)              | SI_KERNEL   |
+   | single-steps                             | TRAP_TRACE  |
+   | single-stepping a syscall                | TRAP_BRKPT  |
+   | user sent SIGTRAP                        | 0           |
+   | exec SIGTRAP (when no PTRACE_EVENT_EXEC) | 0           |
+   | hardware breakpoints/watchpoints         | TRAP_HWBKPT |
 
    That is, it reports SI_KERNEL for software breakpoints (and only
    for those), and TRAP_BRKPT for single-stepping a syscall...  If the
@@ -140,14 +140,32 @@ struct buffer;
    in SPU code on a Cell/B.E.  However, SI_KERNEL is never seen
    on a SIGTRAP for any other reason.
 
-   The generic Linux target code should use GDB_ARCH_IS_TRAP_BRKPT
-   instead of TRAP_BRKPT to abstract out these peculiarities.  */
+   The MIPS kernel uses SI_KERNEL for all kernel generated traps.
+   Since:
+
+     - MIPS doesn't do hardware single-step
+     - We don't need to care about exec SIGTRAPs, since we assume
+       PTRACE_EVENT_EXEC.
+     - The MIPS kernel doesn't support hardware breakpoints.
+
+   on MIPS, all we need to care about is distinguishing between
+   software breakpoints and hardware watchpoints, which can be done by
+   peeking the debug registers.
+
+   The generic Linux target code should use GDB_ARCH_IS_TRAP_* instead
+   of TRAP_* to abstract out these peculiarities.  */
 #if defined __i386__ || defined __x86_64__
 # define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL)
+# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == TRAP_HWBKPT)
 #elif defined __powerpc__
 # define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL || (X) == TRAP_BRKPT)
+# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == TRAP_HWBKPT)
+#elif defined __mips__
+# define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL)
+# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == SI_KERNEL)
 #else
 # define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == TRAP_BRKPT)
+# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == TRAP_HWBKPT)
 #endif
 
 #ifndef TRAP_HWBKPT
-- 
1.9.3

Re: [PATCH] Handle MIPS Linux SIGTRAP siginfo.si_code values

[Maciej W. Rozycki]

On Wed, 24 Feb 2016, Pedro Alves wrote:

> @@ -140,14 +140,32 @@ struct buffer;
>     in SPU code on a Cell/B.E.  However, SI_KERNEL is never seen
>     on a SIGTRAP for any other reason.
>  
> -   The generic Linux target code should use GDB_ARCH_IS_TRAP_BRKPT
> -   instead of TRAP_BRKPT to abstract out these peculiarities.  */
> +   The MIPS kernel uses SI_KERNEL for all kernel generated traps.
> +   Since:
> +
> +     - MIPS doesn't do hardware single-step
> +     - We don't need to care about exec SIGTRAPs, since we assume
> +       PTRACE_EVENT_EXEC.
> +     - The MIPS kernel doesn't support hardware breakpoints.
> +
> +   on MIPS, all we need to care about is distinguishing between
> +   software breakpoints and hardware watchpoints, which can be done by
> +   peeking the debug registers.

 I'm assuming spurious traps such as from trap instructions will still be 
delivered as such, right?

> +
> +   The generic Linux target code should use GDB_ARCH_IS_TRAP_* instead
> +   of TRAP_* to abstract out these peculiarities.  */
>  #if defined __i386__ || defined __x86_64__
>  # define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL)
> +# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == TRAP_HWBKPT)
>  #elif defined __powerpc__
>  # define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL || (X) == TRAP_BRKPT)
> +# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == TRAP_HWBKPT)
> +#elif defined __mips__
> +# define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL)
> +# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == SI_KERNEL)

 Shall I add the TRAP_BRKPT and TRAP_HWBKPT codes to the MIPS Linux kernel 
then or not?  If anything, this looks like a performance win to me, at no 
significant cost (any possible kernel overhead will be in the range of a 
couple processor instructions, which is nothing compared to an extra 
ptrace(2) call and all the associated processing).

  Maciej

Re: [PATCH] Handle MIPS Linux SIGTRAP siginfo.si_code values

[Pedro Alves]

On 02/24/2016 06:29 PM, Maciej W. Rozycki wrote:
> On Wed, 24 Feb 2016, Pedro Alves wrote:
> 
>> @@ -140,14 +140,32 @@ struct buffer;
>>     in SPU code on a Cell/B.E.  However, SI_KERNEL is never seen
>>     on a SIGTRAP for any other reason.
>>  
>> -   The generic Linux target code should use GDB_ARCH_IS_TRAP_BRKPT
>> -   instead of TRAP_BRKPT to abstract out these peculiarities.  */
>> +   The MIPS kernel uses SI_KERNEL for all kernel generated traps.
>> +   Since:
>> +
>> +     - MIPS doesn't do hardware single-step
>> +     - We don't need to care about exec SIGTRAPs, since we assume
>> +       PTRACE_EVENT_EXEC.
>> +     - The MIPS kernel doesn't support hardware breakpoints.
>> +
>> +   on MIPS, all we need to care about is distinguishing between
>> +   software breakpoints and hardware watchpoints, which can be done by
>> +   peeking the debug registers.
> 
>  I'm assuming spurious traps such as from trap instructions will still be 
> delivered as such, right?

Yes, they should.

> 
>> +
>> +   The generic Linux target code should use GDB_ARCH_IS_TRAP_* instead
>> +   of TRAP_* to abstract out these peculiarities.  */
>>  #if defined __i386__ || defined __x86_64__
>>  # define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL)
>> +# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == TRAP_HWBKPT)
>>  #elif defined __powerpc__
>>  # define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL || (X) == TRAP_BRKPT)
>> +# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == TRAP_HWBKPT)
>> +#elif defined __mips__
>> +# define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL)
>> +# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == SI_KERNEL)
> 
>  Shall I add the TRAP_BRKPT and TRAP_HWBKPT codes to the MIPS Linux kernel 
> then or not?  

The higher order issue was having a way to distinguish the possible
traps, for correctness.  Since we found a way, it's no longer a
pressing issue and we could leave it be.

> If anything, this looks like a performance win to me, at no 
> significant cost (any possible kernel overhead will be in the range of a 
> couple processor instructions, which is nothing compared to an extra 
> ptrace(2) call and all the associated processing).

Yeah.  We usually need several other ptrace calls so it may not
be noticeable.  But if you do teach the kernel about TRAP_BRKPT, and want to
avoid the watchpoints check, I think gdb/gdbserver could be made to auto detect
at run time what does the kernel report.  E.g,. have gdb fork itself, set a
sw bp at the current PC in the child and continue it.  That triggers the bp
immediately.  Then the parent can check what is in the child's si_code.  We
do similar things already in linux_check_ptrace_features (gdb/nat/linux-ptrace.c).

Thanks,
Pedro Alves

Re: [PATCH] Handle MIPS Linux SIGTRAP siginfo.si_code values

[Luis Machado]

On 02/24/2016 01:43 PM, Pedro Alves wrote:
> This unbreaks pending/delayed breakpoints handling, as well as
> hardware watchpoints, on MIPS.
>
> Ref: https://sourceware.org/ml/gdb-patches/2016-02/msg00681.html
>
> The MIPS kernel reports SI_KERNEL for all kernel generated traps,
> instead of TRAP_BRKPT / TRAP_HWBKPT, but GDB isn't aware of this.
>
> Basically, this commit:
>
> - Folds watchpoints logic into check_stopped_by_breakpoint, and
>    renames it to save_stop_reason.
>
> - Adds GDB_ARCH_IS_TRAP_HWBKPT.
>
> - Makes MIPS set both GDB_ARCH_IS_TRAP_BRPT and
>    GDB_ARCH_IS_TRAP_HWBKPT to SI_KERNEL.  In save_stop_reason, we
>    handle the case of the same si_code returning true for both
>    TRAP_BRPT and TRAP_HWBKPT by looking at what the debug registers
>    say.
>
> Tested on x86-64 Fedora 20, native and gdbserver.
>
> gdb/ChangeLog:
> 2016-02-24  Pedro Alves  <palves@redhat.com>
>
> 	* linux-nat.c (save_sigtrap) Delete.
> 	(stop_wait_callback): Call save_stop_reason instead of
> 	save_sigtrap.
> 	(check_stopped_by_breakpoint): Rename to ...
> 	(save_stop_reason): ... this.  Bits of save_sigtrap folded here.
> 	Use GDB_ARCH_IS_TRAP_HWBKPT and handle ambiguous
> 	GDB_ARCH_IS_TRAP_BRKPT / GDB_ARCH_IS_TRAP_HWBKPT.  Factor out
> 	common code between the USE_SIGTRAP_SIGINFO and
> 	!USE_SIGTRAP_SIGINFO blocks.
> 	(linux_nat_filter_event): Call save_stop_reason instead of
> 	save_sigtrap.
> 	* nat/linux-ptrace.h: Check for both SI_KERNEL and TRAP_BRKPT
> 	si_code for MIPS.
> 	* nat/linux-ptrace.h: Fix "TRAP_HWBPT" typo in x86 table.  Add
> 	comments on MIPS behavior.
> 	(GDB_ARCH_IS_TRAP_HWBKPT): Define for all archs.
>
> gdb/gdbserver/ChangeLog:
> 2016-02-24  Pedro Alves  <palves@redhat.com>
>
> 	* linux-low.c (check_stopped_by_breakpoint): Rename to ...
> 	(save_stop_reason): ... this.  Use GDB_ARCH_IS_TRAP_HWBKPT and
> 	handle ambiguous GDB_ARCH_IS_TRAP_BRKPT / GDB_ARCH_IS_TRAP_HWBKPT.
> 	Factor out common code between the USE_SIGTRAP_SIGINFO and
> 	!USE_SIGTRAP_SIGINFO blocks.
> 	(linux_low_filter_event): Call save_stop_reason instead of
> 	check_stopped_by_breakpoint and check_stopped_by_watchpoint.
> 	Update comments.
> 	(linux_wait_1): Update comments.
> ---
>   gdb/gdbserver/linux-low.c | 170 ++++++++++++++++++++--------------------------
>   gdb/linux-nat.c           | 149 ++++++++++++++++++++--------------------
>   gdb/nat/linux-ptrace.h    |  38 ++++++++---
>   3 files changed, 172 insertions(+), 185 deletions(-)
>
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index 8b025bd..b02ad31 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -735,32 +735,16 @@ get_syscall_trapinfo (struct lwp_info *lwp, int *sysno, int *sysret)
>     current_thread = saved_thread;
>   }
>
> -/* This function should only be called if LWP got a SIGTRAP.
> -   The SIGTRAP could mean several things.
> +static int check_stopped_by_watchpoint (struct lwp_info *child);
>
> -   On i386, where decr_pc_after_break is non-zero:
> -
> -   If we were single-stepping this process using PTRACE_SINGLESTEP, we
> -   will get only the one SIGTRAP.  The value of $eip will be the next
> -   instruction.  If the instruction we stepped over was a breakpoint,
> -   we need to decrement the PC.
> -
> -   If we continue the process using PTRACE_CONT, we will get a
> -   SIGTRAP when we hit a breakpoint.  The value of $eip will be
> -   the instruction after the breakpoint (i.e. needs to be
> -   decremented).  If we report the SIGTRAP to GDB, we must also
> -   report the undecremented PC.  If the breakpoint is removed, we
> -   must resume at the decremented PC.
> -
> -   On a non-decr_pc_after_break machine with hardware or kernel
> -   single-step:
> -
> -   If we either single-step a breakpoint instruction, or continue and
> -   hit a breakpoint instruction, our PC will point at the breakpoint
> -   instruction.  */
> +/* Called when the LWP stopped for a signal/trap.  If it stopped for a
> +   trap check what caused it (breakpoint, watchpoint, trace, etc.),
> +   and save the result in the LWP's stop_reason field.  If it stopped
> +   for a breakpoint, decrement the PC if necessary on the lwp's
> +   architecture.  Returns true if we now have the LWP's stop PC.  */
>
>   static int
> -check_stopped_by_breakpoint (struct lwp_info *lwp)
> +save_stop_reason (struct lwp_info *lwp)
>   {
>     CORE_ADDR pc;
>     CORE_ADDR sw_breakpoint_pc;
> @@ -785,56 +769,39 @@ check_stopped_by_breakpoint (struct lwp_info *lwp)
>       {
>         if (siginfo.si_signo == SIGTRAP)
>   	{
> -	  if (GDB_ARCH_IS_TRAP_BRKPT (siginfo.si_code))
> +	  if (GDB_ARCH_IS_TRAP_BRKPT (siginfo.si_code)
> +	      && GDB_ARCH_IS_TRAP_HWBKPT (siginfo.si_code))
>   	    {
> -	      if (debug_threads)
> -		{
> -		  struct thread_info *thr = get_lwp_thread (lwp);
> -
> -		  debug_printf ("CSBB: %s stopped by software breakpoint\n",
> -				target_pid_to_str (ptid_of (thr)));
> -		}
> -
> -	      /* Back up the PC if necessary.  */
> -	      if (pc != sw_breakpoint_pc)
> -		{
> -		  struct regcache *regcache
> -		    = get_thread_regcache (current_thread, 1);
> -		  (*the_low_target.set_pc) (regcache, sw_breakpoint_pc);
> -		}
> -
> -	      lwp->stop_pc = sw_breakpoint_pc;
> +	      /* The si_code is ambiguous on this arch -- check debug
> +		 registers.  */
> +	      if (!check_stopped_by_watchpoint (lwp))
> +		lwp->stop_reason = TARGET_STOPPED_BY_SW_BREAKPOINT;
> +	    }
> +	  else if (GDB_ARCH_IS_TRAP_BRKPT (siginfo.si_code))
> +	    {
> +	      /* If we determine the LWP stopped for a SW breakpoint,
> +		 trust it.  Particularly don't check watchpoint
> +		 registers, because at least on s390, we'd find
> +		 stopped-by-watchpoint as long as there's a watchpoint
> +		 set.  */
>   	      lwp->stop_reason = TARGET_STOPPED_BY_SW_BREAKPOINT;
> -	      current_thread = saved_thread;
> -	      return 1;
>   	    }
> -	  else if (siginfo.si_code == TRAP_HWBKPT)
> +	  else if (GDB_ARCH_IS_TRAP_HWBKPT (siginfo.si_code))
>   	    {
> -	      if (debug_threads)
> -		{
> -		  struct thread_info *thr = get_lwp_thread (lwp);
> -
> -		  debug_printf ("CSBB: %s stopped by hardware "
> -				"breakpoint/watchpoint\n",
> -				target_pid_to_str (ptid_of (thr)));
> -		}
> -
> -	      lwp->stop_pc = pc;
> -	      lwp->stop_reason = TARGET_STOPPED_BY_HW_BREAKPOINT;
> -	      current_thread = saved_thread;
> -	      return 1;
> +	      /* This can indicate either a hardware breakpoint or
> +		 hardware watchpoint.  Check debug registers.  */
> +	      if (!check_stopped_by_watchpoint (lwp))
> +		lwp->stop_reason = TARGET_STOPPED_BY_HW_BREAKPOINT;
>   	    }
>   	  else if (siginfo.si_code == TRAP_TRACE)
>   	    {
> -	      if (debug_threads)
> -		{
> -		  struct thread_info *thr = get_lwp_thread (lwp);
> -
> -		  debug_printf ("CSBB: %s stopped by trace\n",
> -				target_pid_to_str (ptid_of (thr)));
> -		}
> -
> -	      lwp->stop_reason = TARGET_STOPPED_BY_SINGLE_STEP;
> +	      /* We may have single stepped an instruction that
> +		 triggered a watchpoint.  In that case, on some
> +		 architectures (such as x86), instead of TRAP_HWBKPT,
> +		 si_code indicates TRAP_TRACE, and we need to check
> +		 the debug registers separately.  */
> +	      if (!check_stopped_by_watchpoint (lwp))
> +		lwp->stop_reason = TARGET_STOPPED_BY_SINGLE_STEP;
>   	    }
>   	}
>       }
> @@ -845,6 +812,16 @@ check_stopped_by_breakpoint (struct lwp_info *lwp)
>        case we need to report the breakpoint PC.  */
>     if ((!lwp->stepping || lwp->stop_pc == sw_breakpoint_pc)
>         && (*the_low_target.breakpoint_at) (sw_breakpoint_pc))
> +    lwp->stop_reason = TARGET_STOPPED_BY_SW_BREAKPOINT;
> +
> +  if (hardware_breakpoint_inserted_here (pc))
> +    lwp->stop_reason = TARGET_STOPPED_BY_HW_BREAKPOINT;
> +
> +  if (lp->stop_reason == TARGET_STOPPED_BY_NO_REASON)
> +    check_stopped_by_watchpoint (lp);
> +#endif
> +
> +  if (lwp->stop_reason == TARGET_STOPPED_BY_SW_BREAKPOINT)
>       {
>         if (debug_threads)
>   	{
> @@ -856,19 +833,16 @@ check_stopped_by_breakpoint (struct lwp_info *lwp)
>
>         /* Back up the PC if necessary.  */
>         if (pc != sw_breakpoint_pc)
> -        {
> +	{
>   	  struct regcache *regcache
>   	    = get_thread_regcache (current_thread, 1);
>   	  (*the_low_target.set_pc) (regcache, sw_breakpoint_pc);
>   	}
>
> -      lwp->stop_pc = sw_breakpoint_pc;
> -      lwp->stop_reason = TARGET_STOPPED_BY_SW_BREAKPOINT;
> -      current_thread = saved_thread;
> -      return 1;
> +      /* Update this so we record the correct stop PC below.  */
> +      pc = sw_breakpoint_pc;
>       }
> -
> -  if (hardware_breakpoint_inserted_here (pc))
> +  else if (lwp->stop_reason == TARGET_STOPPED_BY_HW_BREAKPOINT)
>       {
>         if (debug_threads)
>   	{
> @@ -877,16 +851,31 @@ check_stopped_by_breakpoint (struct lwp_info *lwp)
>   	  debug_printf ("CSBB: %s stopped by hardware breakpoint\n",
>   			target_pid_to_str (ptid_of (thr)));
>   	}
> +    }
> +  else if (lwp->stop_reason == TARGET_STOPPED_BY_WATCHPOINT)
> +    {
> +      if (debug_threads)
> +	{
> +	  struct thread_info *thr = get_lwp_thread (lwp);
>
> -      lwp->stop_pc = pc;
> -      lwp->stop_reason = TARGET_STOPPED_BY_HW_BREAKPOINT;
> -      current_thread = saved_thread;
> -      return 1;
> +	  debug_printf ("CSBB: %s stopped by hardware watchpoint\n",
> +			target_pid_to_str (ptid_of (thr)));
> +	}
>       }
> -#endif
> +  else if (lwp->stop_reason == TARGET_STOPPED_BY_SINGLE_STEP)
> +    {
> +      if (debug_threads)
> +	{
> +	  struct thread_info *thr = get_lwp_thread (lwp);
>
> +	  debug_printf ("CSBB: %s stopped by trace\n",
> +			target_pid_to_str (ptid_of (thr)));
> +	}
> +    }
> +
> +  lwp->stop_pc = pc;
>     current_thread = saved_thread;
> -  return 0;
> +  return 1;
>   }
>
>   static struct lwp_info *
> @@ -2434,8 +2423,8 @@ linux_low_filter_event (int lwpid, int wstat)
>         child->syscall_state = TARGET_WAITKIND_IGNORE;
>       }
>
> -  /* Be careful to not overwrite stop_pc until
> -     check_stopped_by_breakpoint is called.  */
> +  /* Be careful to not overwrite stop_pc until save_stop_reason is
> +     called.  */
>     if (WIFSTOPPED (wstat) && WSTOPSIG (wstat) == SIGTRAP
>         && linux_is_extended_waitstatus (wstat))
>       {
> @@ -2448,27 +2437,12 @@ linux_low_filter_event (int lwpid, int wstat)
>   	}
>       }
>
> -  /* Check first whether this was a SW/HW breakpoint before checking
> -     watchpoints, because at least s390 can't tell the data address of
> -     hardware watchpoint hits, and returns stopped-by-watchpoint as
> -     long as there's a watchpoint set.  */
>     if (WIFSTOPPED (wstat) && linux_wstatus_maybe_breakpoint (wstat))
>       {
> -      if (check_stopped_by_breakpoint (child))
> +      if (save_stop_reason (child))
>   	have_stop_pc = 1;
>       }
>
> -  /* Note that TRAP_HWBKPT can indicate either a hardware breakpoint
> -     or hardware watchpoint.  Check which is which if we got
> -     TARGET_STOPPED_BY_HW_BREAKPOINT.  Likewise, we may have single
> -     stepped an instruction that triggered a watchpoint.  In that
> -     case, on some architectures (such as x86), instead of
> -     TRAP_HWBKPT, si_code indicates TRAP_TRACE, and we need to check
> -     the debug registers separately.  */
> -  if (WIFSTOPPED (wstat) && WSTOPSIG (wstat) == SIGTRAP
> -      && child->stop_reason != TARGET_STOPPED_BY_SW_BREAKPOINT)
> -    check_stopped_by_watchpoint (child);
> -
>     if (!have_stop_pc)
>       child->stop_pc = get_pc (child);
>
> @@ -3209,7 +3183,7 @@ linux_wait_1 (ptid_t ptid,
>        hardware single step it means a gdb/gdbserver breakpoint had been
>        planted on top of a permanent breakpoint, in the case of a software
>        single step it may just mean that gdbserver hit the reinsert breakpoint.
> -     The PC has been adjusted by check_stopped_by_breakpoint to point at
> +     The PC has been adjusted by save_stop_reason to point at
>        the breakpoint address.
>        So in the case of the hardware single step advance the PC manually
>        past the breakpoint and in the case of software single step advance only
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 6ded38d..0829bcb 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -303,10 +303,11 @@ static struct lwp_info *find_lwp_pid (ptid_t ptid);
>
>   static int lwp_status_pending_p (struct lwp_info *lp);
>
> -static int check_stopped_by_breakpoint (struct lwp_info *lp);
>   static int sigtrap_is_event (int status);
>   static int (*linux_nat_status_is_event) (int status) = sigtrap_is_event;
>
> +static void save_stop_reason (struct lwp_info *lp);
> +
>   \f
>   /* LWP accessors.  */
>
> @@ -2321,30 +2322,6 @@ check_stopped_by_watchpoint (struct lwp_info *lp)
>     return lp->stop_reason == TARGET_STOPPED_BY_WATCHPOINT;
>   }
>
> -/* Called when the LWP stopped for a trap that could be explained by a
> -   watchpoint or a breakpoint.  */
> -
> -static void
> -save_sigtrap (struct lwp_info *lp)
> -{
> -  gdb_assert (lp->stop_reason == TARGET_STOPPED_BY_NO_REASON);
> -  gdb_assert (lp->status != 0);
> -
> -  /* Check first if this was a SW/HW breakpoint before checking
> -     watchpoints, because at least s390 can't tell the data address of
> -     hardware watchpoint hits, and the kernel returns
> -     stopped-by-watchpoint as long as there's a watchpoint set.  */
> -  if (linux_nat_status_is_event (lp->status))
> -    check_stopped_by_breakpoint (lp);
> -
> -  /* Note that TRAP_HWBKPT can indicate either a hardware breakpoint
> -     or hardware watchpoint.  Check which is which if we got
> -     TARGET_STOPPED_BY_HW_BREAKPOINT.  */
> -  if (lp->stop_reason == TARGET_STOPPED_BY_NO_REASON
> -      || lp->stop_reason == TARGET_STOPPED_BY_HW_BREAKPOINT)
> -    check_stopped_by_watchpoint (lp);
> -}
> -
>   /* Returns true if the LWP had stopped for a watchpoint.  */
>
>   static int
> @@ -2441,7 +2418,7 @@ stop_wait_callback (struct lwp_info *lp, void *data)
>   	  /* Save the sigtrap event.  */
>   	  lp->status = status;
>   	  gdb_assert (lp->signalled);
> -	  save_sigtrap (lp);
> +	  save_stop_reason (lp);
>   	}
>         else
>   	{
> @@ -2583,29 +2560,32 @@ select_event_lwp_callback (struct lwp_info *lp, void *data)
>     return 0;
>   }
>
> -/* Called when the LWP got a signal/trap that could be explained by a
> -   software or hardware breakpoint.  */
> +/* Called when the LWP stopped for a signal/trap.  If it stopped for a
> +   trap check what caused it (breakpoint, watchpoint, trace, etc.),
> +   and save the result in the LWP's stop_reason field.  If it stopped
> +   for a breakpoint, decrement the PC if necessary on the lwp's
> +   architecture.  */
>
> -static int
> -check_stopped_by_breakpoint (struct lwp_info *lp)
> +static void
> +save_stop_reason (struct lwp_info *lp)
>   {
> -  /* Arrange for a breakpoint to be hit again later.  We don't keep
> -     the SIGTRAP status and don't forward the SIGTRAP signal to the
> -     LWP.  We will handle the current event, eventually we will resume
> -     this LWP, and this breakpoint will trap again.
> -
> -     If we do not do this, then we run the risk that the user will
> -     delete or disable the breakpoint, but the LWP will have already
> -     tripped on it.  */
> -
> -  struct regcache *regcache = get_thread_regcache (lp->ptid);
> -  struct gdbarch *gdbarch = get_regcache_arch (regcache);
> +  struct regcache *regcache;
> +  struct gdbarch *gdbarch;
>     CORE_ADDR pc;
>     CORE_ADDR sw_bp_pc;
>   #if USE_SIGTRAP_SIGINFO
>     siginfo_t siginfo;
>   #endif
>
> +  gdb_assert (lp->stop_reason == TARGET_STOPPED_BY_NO_REASON);
> +  gdb_assert (lp->status != 0);
> +
> +  if (!linux_nat_status_is_event (lp->status))
> +    return;
> +
> +  regcache = get_thread_regcache (lp->ptid);
> +  gdbarch = get_regcache_arch (regcache);
> +
>     pc = regcache_read_pc (regcache);
>     sw_bp_pc = pc - gdbarch_decr_pc_after_break (gdbarch);
>
> @@ -2614,33 +2594,29 @@ check_stopped_by_breakpoint (struct lwp_info *lp)
>       {
>         if (siginfo.si_signo == SIGTRAP)
>   	{
> -	  if (GDB_ARCH_IS_TRAP_BRKPT (siginfo.si_code))
> +	  if (GDB_ARCH_IS_TRAP_BRKPT (siginfo.si_code)
> +	      && GDB_ARCH_IS_TRAP_HWBKPT (siginfo.si_code))
>   	    {
> -	      if (debug_linux_nat)
> -		fprintf_unfiltered (gdb_stdlog,
> -				    "CSBB: %s stopped by software "
> -				    "breakpoint\n",
> -				    target_pid_to_str (lp->ptid));
> -
> -	      /* Back up the PC if necessary.  */
> -	      if (pc != sw_bp_pc)
> -		regcache_write_pc (regcache, sw_bp_pc);
> -
> -	      lp->stop_pc = sw_bp_pc;
> +	      /* The si_code is ambiguous on this arch -- check debug
> +		 registers.  */
> +	      if (!check_stopped_by_watchpoint (lp))
> +		lp->stop_reason = TARGET_STOPPED_BY_SW_BREAKPOINT;
> +	    }
> +	  else if (GDB_ARCH_IS_TRAP_BRKPT (siginfo.si_code))
> +	    {
> +	      /* If we determine the LWP stopped for a SW breakpoint,
> +		 trust it.  Particularly don't check watchpoint
> +		 registers, because at least on s390, we'd find
> +		 stopped-by-watchpoint as long as there's a watchpoint
> +		 set.  */
>   	      lp->stop_reason = TARGET_STOPPED_BY_SW_BREAKPOINT;
> -	      return 1;
>   	    }
> -	  else if (siginfo.si_code == TRAP_HWBKPT)
> +	  else if (GDB_ARCH_IS_TRAP_HWBKPT (siginfo.si_code))
>   	    {
> -	      if (debug_linux_nat)
> -		fprintf_unfiltered (gdb_stdlog,
> -				    "CSBB: %s stopped by hardware "
> -				    "breakpoint/watchpoint\n",
> -				    target_pid_to_str (lp->ptid));
> -
> -	      lp->stop_pc = pc;
> -	      lp->stop_reason = TARGET_STOPPED_BY_HW_BREAKPOINT;
> -	      return 1;
> +	      /* This can indicate either a hardware breakpoint or
> +		 hardware watchpoint.  Check debug registers.  */
> +	      if (!check_stopped_by_watchpoint (lp))
> +		lp->stop_reason = TARGET_STOPPED_BY_HW_BREAKPOINT;
>   	    }
>   	  else if (siginfo.si_code == TRAP_TRACE)
>   	    {
> @@ -2648,6 +2624,13 @@ check_stopped_by_breakpoint (struct lwp_info *lp)
>   		fprintf_unfiltered (gdb_stdlog,
>   				    "CSBB: %s stopped by trace\n",
>   				    target_pid_to_str (lp->ptid));
> +
> +	      /* We may have single stepped an instruction that
> +		 triggered a watchpoint.  In that case, on some
> +		 architectures (such as x86), instead of TRAP_HWBKPT,
> +		 si_code indicates TRAP_TRACE, and we need to check
> +		 the debug registers separately.  */
> +	      check_stopped_by_watchpoint (lp);
>   	    }
>   	}
>       }
> @@ -2658,6 +2641,18 @@ check_stopped_by_breakpoint (struct lwp_info *lp)
>       {
>         /* The LWP was either continued, or stepped a software
>   	 breakpoint instruction.  */
> +      lp->stop_reason = TARGET_STOPPED_BY_SW_BREAKPOINT;
> +    }
> +
> +  if (hardware_breakpoint_inserted_here_p (get_regcache_aspace (regcache), pc))
> +    lp->stop_reason = TARGET_STOPPED_BY_HW_BREAKPOINT;
> +
> +  if (lp->stop_reason == TARGET_STOPPED_BY_NO_REASON)
> +    check_stopped_by_watchpoint (lp);
> +#endif
> +
> +  if (lp->stop_reason == TARGET_STOPPED_BY_SW_BREAKPOINT)
> +    {
>         if (debug_linux_nat)
>   	fprintf_unfiltered (gdb_stdlog,
>   			    "CSBB: %s stopped by software breakpoint\n",
> @@ -2667,25 +2662,25 @@ check_stopped_by_breakpoint (struct lwp_info *lp)
>         if (pc != sw_bp_pc)
>   	regcache_write_pc (regcache, sw_bp_pc);
>
> -      lp->stop_pc = sw_bp_pc;
> -      lp->stop_reason = TARGET_STOPPED_BY_SW_BREAKPOINT;
> -      return 1;
> +      /* Update this so we record the correct stop PC below.  */
> +      pc = sw_bp_pc;
>       }
> -
> -  if (hardware_breakpoint_inserted_here_p (get_regcache_aspace (regcache), pc))
> +  else if (lp->stop_reason == TARGET_STOPPED_BY_HW_BREAKPOINT)
>       {
>         if (debug_linux_nat)
>   	fprintf_unfiltered (gdb_stdlog,
> -			    "CSBB: stopped by hardware breakpoint %s\n",
> +			    "CSBB: %s stopped by hardware breakpoint\n",
> +			    target_pid_to_str (lp->ptid));
> +    }
> +  else if (lp->stop_reason == TARGET_STOPPED_BY_WATCHPOINT)
> +    {
> +      if (debug_linux_nat)
> +	fprintf_unfiltered (gdb_stdlog,
> +			    "CSBB: %s stopped by hardware watchpoint\n",
>   			    target_pid_to_str (lp->ptid));
> -
> -      lp->stop_pc = pc;
> -      lp->stop_reason = TARGET_STOPPED_BY_HW_BREAKPOINT;
> -      return 1;
>       }
> -#endif
>
> -  return 0;
> +  lp->stop_pc = pc;
>   }
>
>
> @@ -3057,7 +3052,7 @@ linux_nat_filter_event (int lwpid, int status)
>     /* An interesting event.  */
>     gdb_assert (lp);
>     lp->status = status;
> -  save_sigtrap (lp);
> +  save_stop_reason (lp);
>     return lp;
>   }
>
> diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h
> index ba58717..1d137e8 100644
> --- a/gdb/nat/linux-ptrace.h
> +++ b/gdb/nat/linux-ptrace.h
> @@ -119,14 +119,14 @@ struct buffer;
>   /* The x86 kernel gets some of the si_code values backwards, like
>      this:
>
> -   | what                                     | si_code    |
> -   |------------------------------------------+------------|
> -   | software breakpoints (int3)              | SI_KERNEL  |
> -   | single-steps                             | TRAP_TRACE |
> -   | single-stepping a syscall                | TRAP_BRKPT |
> -   | user sent SIGTRAP                        | 0          |
> -   | exec SIGTRAP (when no PTRACE_EVENT_EXEC) | 0          |
> -   | hardware breakpoints/watchpoints         | TRAP_HWBPT |
> +   | what                                     | si_code     |
> +   |------------------------------------------+-------------|
> +   | software breakpoints (int3)              | SI_KERNEL   |
> +   | single-steps                             | TRAP_TRACE  |
> +   | single-stepping a syscall                | TRAP_BRKPT  |
> +   | user sent SIGTRAP                        | 0           |
> +   | exec SIGTRAP (when no PTRACE_EVENT_EXEC) | 0           |
> +   | hardware breakpoints/watchpoints         | TRAP_HWBKPT |
>
>      That is, it reports SI_KERNEL for software breakpoints (and only
>      for those), and TRAP_BRKPT for single-stepping a syscall...  If the
> @@ -140,14 +140,32 @@ struct buffer;
>      in SPU code on a Cell/B.E.  However, SI_KERNEL is never seen
>      on a SIGTRAP for any other reason.
>
> -   The generic Linux target code should use GDB_ARCH_IS_TRAP_BRKPT
> -   instead of TRAP_BRKPT to abstract out these peculiarities.  */
> +   The MIPS kernel uses SI_KERNEL for all kernel generated traps.
> +   Since:
> +
> +     - MIPS doesn't do hardware single-step
> +     - We don't need to care about exec SIGTRAPs, since we assume
> +       PTRACE_EVENT_EXEC.
> +     - The MIPS kernel doesn't support hardware breakpoints.
> +
> +   on MIPS, all we need to care about is distinguishing between
> +   software breakpoints and hardware watchpoints, which can be done by
> +   peeking the debug registers.
> +
> +   The generic Linux target code should use GDB_ARCH_IS_TRAP_* instead
> +   of TRAP_* to abstract out these peculiarities.  */
>   #if defined __i386__ || defined __x86_64__
>   # define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL)
> +# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == TRAP_HWBKPT)
>   #elif defined __powerpc__
>   # define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL || (X) == TRAP_BRKPT)
> +# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == TRAP_HWBKPT)
> +#elif defined __mips__
> +# define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL)
> +# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == SI_KERNEL)
>   #else
>   # define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == TRAP_BRKPT)
> +# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == TRAP_HWBKPT)
>   #endif
>
>   #ifndef TRAP_HWBKPT
>

I've tried it with a few MIPS/Linux boards and it works fine for 
hardware watchpoints and software breakpoints again.

Thanks,
Luis

Re: [PATCH] Handle MIPS Linux SIGTRAP siginfo.si_code values

[Luis Machado]

On 02/24/2016 03:51 PM, Pedro Alves wrote:
> On 02/24/2016 06:29 PM, Maciej W. Rozycki wrote:
>> On Wed, 24 Feb 2016, Pedro Alves wrote:
>>
>>> @@ -140,14 +140,32 @@ struct buffer;
>>>      in SPU code on a Cell/B.E.  However, SI_KERNEL is never seen
>>>      on a SIGTRAP for any other reason.
>>>
>>> -   The generic Linux target code should use GDB_ARCH_IS_TRAP_BRKPT
>>> -   instead of TRAP_BRKPT to abstract out these peculiarities.  */
>>> +   The MIPS kernel uses SI_KERNEL for all kernel generated traps.
>>> +   Since:
>>> +
>>> +     - MIPS doesn't do hardware single-step
>>> +     - We don't need to care about exec SIGTRAPs, since we assume
>>> +       PTRACE_EVENT_EXEC.
>>> +     - The MIPS kernel doesn't support hardware breakpoints.
>>> +
>>> +   on MIPS, all we need to care about is distinguishing between
>>> +   software breakpoints and hardware watchpoints, which can be done by
>>> +   peeking the debug registers.
>>
>>   I'm assuming spurious traps such as from trap instructions will still be
>> delivered as such, right?
>
> Yes, they should.
>
>>
>>> +
>>> +   The generic Linux target code should use GDB_ARCH_IS_TRAP_* instead
>>> +   of TRAP_* to abstract out these peculiarities.  */
>>>   #if defined __i386__ || defined __x86_64__
>>>   # define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL)
>>> +# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == TRAP_HWBKPT)
>>>   #elif defined __powerpc__
>>>   # define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL || (X) == TRAP_BRKPT)
>>> +# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == TRAP_HWBKPT)
>>> +#elif defined __mips__
>>> +# define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL)
>>> +# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == SI_KERNEL)
>>
>>   Shall I add the TRAP_BRKPT and TRAP_HWBKPT codes to the MIPS Linux kernel
>> then or not?
>
> The higher order issue was having a way to distinguish the possible
> traps, for correctness.  Since we found a way, it's no longer a
> pressing issue and we could leave it be.

I think we should converge to a standard solution across all 
architectures in the future rather than potentially perpetuate old 
non-standard ways. So the movement towards returning well defined 
si_code values in the MIPS Linux Kernel is a plus, even though we might 
not benefit from it right now. I'm in favor of having the change to the 
kernel made.

The later we change this, the longer we need to support the old ways and 
their one-off mechanisms for each architecture, no?

Re: [PATCH] Handle MIPS Linux SIGTRAP siginfo.si_code values

[Maciej W. Rozycki]

On Wed, 24 Feb 2016, Pedro Alves wrote:

> >> +   The generic Linux target code should use GDB_ARCH_IS_TRAP_* instead
> >> +   of TRAP_* to abstract out these peculiarities.  */
> >>  #if defined __i386__ || defined __x86_64__
> >>  # define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL)
> >> +# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == TRAP_HWBKPT)
> >>  #elif defined __powerpc__
> >>  # define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL || (X) == TRAP_BRKPT)
> >> +# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == TRAP_HWBKPT)
> >> +#elif defined __mips__
> >> +# define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL)
> >> +# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == SI_KERNEL)
> > 
> >  Shall I add the TRAP_BRKPT and TRAP_HWBKPT codes to the MIPS Linux kernel 
> > then or not?  
> 
> The higher order issue was having a way to distinguish the possible
> traps, for correctness.  Since we found a way, it's no longer a
> pressing issue and we could leave it be.
> 
> > If anything, this looks like a performance win to me, at no 
> > significant cost (any possible kernel overhead will be in the range of a 
> > couple processor instructions, which is nothing compared to an extra 
> > ptrace(2) call and all the associated processing).
> 
> Yeah.  We usually need several other ptrace calls so it may not
> be noticeable.  But if you do teach the kernel about TRAP_BRKPT, and want to
> avoid the watchpoints check, I think gdb/gdbserver could be made to auto detect
> at run time what does the kernel report.  E.g,. have gdb fork itself, set a
> sw bp at the current PC in the child and continue it.  That triggers the bp
> immediately.  Then the parent can check what is in the child's si_code.  We
> do similar things already in linux_check_ptrace_features (gdb/nat/linux-ptrace.c).

 That would be the big-hammer approach.  However from my understanding of 
your code the benefit from such probing would only matter for speeding up 
random SIGTRAP reporting, a corner case not worth it IMO.

 Whereas for real breakpoint hits if we report TRAP_BRKPT for software 
breakpoints and TRAP_HWBKPT for hardware data or instruction breakpoints 
(the latters once implemented), then the:

	  if (GDB_ARCH_IS_TRAP_BRKPT (siginfo.si_code)
	      && GDB_ARCH_IS_TRAP_HWBKPT (siginfo.si_code))

condition will never be true for legitimate SIGTRAP events and the slow 
path won't trigger.  But then the MIPS variant will need:

# define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL || (X) == TRAP_BRKPT)
# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == SI_KERNEL || (X) == TRAP_HWBKPT)

to actually recognise these events at all in the first place.  So we 
better have it right away or updated kernels will break GDB for a change.  
But you haven't put it in your proposal despite my earlier suggestion, 
hence my question whether you want this kernel API correction and the 
resulting optimisation or not (I do).

 Given my observations above it looks to me that we only really need the 
two macros updated with my proposal on the GDB side and a corresponding 
rather trivial kernel update to have the codes set in the first place, and 
we can get away without complicating code with an extra run-time probe.

 Have I missed anything?

  Maciej

Re: [PATCH] Handle MIPS Linux SIGTRAP siginfo.si_code values

[Maciej W. Rozycki]

On Wed, 24 Feb 2016, Luis Machado wrote:

> I think we should converge to a standard solution across all architectures in
> the future rather than potentially perpetuate old non-standard ways. So the
> movement towards returning well defined si_code values in the MIPS Linux
> Kernel is a plus, even though we might not benefit from it right now. I'm in
> favor of having the change to the kernel made.

 Not right now as in: "not earlier than in 2-3 months' time needed for a 
proper kernel release", which is when 4.6.0 is likely to happen?  Well, I 
think it's not much waiting really unless you don't care, in which case 
you'll get it with your next regular kernel upgrade scheduled.

  Maciej

Re: [PATCH] Handle MIPS Linux SIGTRAP siginfo.si_code values

[Luis Machado]

On 02/24/2016 05:49 PM, Maciej W. Rozycki wrote:
> On Wed, 24 Feb 2016, Luis Machado wrote:
>
>> I think we should converge to a standard solution across all architectures in
>> the future rather than potentially perpetuate old non-standard ways. So the
>> movement towards returning well defined si_code values in the MIPS Linux
>> Kernel is a plus, even though we might not benefit from it right now. I'm in
>> favor of having the change to the kernel made.
>
>   Not right now as in: "not earlier than in 2-3 months' time needed for a
> proper kernel release", which is when 4.6.0 is likely to happen?  Well, I
> think it's not much waiting really unless you don't care, in which case
> you'll get it with your next regular kernel upgrade scheduled.
>
>    Maciej
>

The benefit (long term one) i'm looking at is mostly from GDB's 
perspective, not having to special-case things to accomodate old 
mechanisms that tend to grow code that wouldn't be needed otherwise. 
We'll still have to support the old mechanism for years, but having this 
change sooner (be it 3 months) rather than later will get us on the 
right path for this matter.

Looking at gdb/nat/linux-ptrace.h, we already have a few variations on 
the theme, with architectures doing different things. :-)

Re: [PATCH] Handle MIPS Linux SIGTRAP siginfo.si_code values

[Pedro Alves]

On 02/24/2016 08:39 PM, Maciej W. Rozycki wrote:

>  Whereas for real breakpoint hits if we report TRAP_BRKPT for software 
> breakpoints and TRAP_HWBKPT for hardware data or instruction breakpoints 
> (the latters once implemented), then the:
> 
> 	  if (GDB_ARCH_IS_TRAP_BRKPT (siginfo.si_code)
> 	      && GDB_ARCH_IS_TRAP_HWBKPT (siginfo.si_code))
> 
> condition will never be true for legitimate SIGTRAP events and the slow 
> path won't trigger.  But then the MIPS variant will need:
> 
> # define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL || (X) == TRAP_BRKPT)
> # define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == SI_KERNEL || (X) == TRAP_HWBKPT)

Exactly.

> 
> to actually recognise these events at all in the first place.  So we 
> better have it right away or updated kernels will break GDB for a change.  

OK, if you're still willing to change the kernel, let's do it.

I had somehow imagined (and reading back, I have no idea why) that you
_didn't_ want to change the si_code, if possible, and was going by that.

(There's always risk associated with such a change, as it's effectively
an ABI break and some tool out there may be relying on SI_KERNEL and may
thus stop working correctly.  Usually ABI stability trumps "cleanliness",
in kernel circles.)


> But you haven't put it in your proposal despite my earlier suggestion, 
> hence my question whether you want this kernel API correction and the 
> resulting optimisation or not (I do).

I was just keeping matters separate, by keeping the patch strictly
about fixing the regression with existing kernels.

>  Given my observations above it looks to me that we only really need the 
> two macros updated with my proposal on the GDB side and a corresponding 
> rather trivial kernel update to have the codes set in the first place, and 
> we can get away without complicating code with an extra run-time probe.
> 
>  Have I missed anything?

I think not.

I've just finished testing it on s390 -- no regressions.  I've
pushed it in now, as is.  I'll follow up with a new patch that makes
gdb accept the anticipated new si_codes too.

Thanks,
Pedro Alves

[PATCH] MIPS/Linux: Also recognize TRAP_BRKPT and TRAP_HWBKPT

[Pedro Alves]

Ref: https://sourceware.org/ml/gdb-patches/2016-02/msg00756.html

This makes the MIPS Linux backends recognize TRAP_BRKPT and
TRAP_HWBKPT in siginfo.si_code too, in addition to SI_KERNEL.  This is
in anticipation of a future kernel change.

On kernels that reports SI_KERNEL, we'll enter the "ambiguous" path of
save_stop_reason:

	  if (GDB_ARCH_IS_TRAP_BRKPT (siginfo.si_code)
	      && GDB_ARCH_IS_TRAP_HWBKPT (siginfo.si_code))
	    {
	      /* The si_code is ambiguous on this arch -- check debug
		 registers.  */
	      if (!check_stopped_by_watchpoint (lp))
		lp->stop_reason = TARGET_STOPPED_BY_SW_BREAKPOINT;
	    }

while on kernels that report the finer-grained si_code values, we'll
enter the corresponding branches:

	  else if (GDB_ARCH_IS_TRAP_BRKPT (siginfo.si_code))
	    {
	    }
	  else if (GDB_ARCH_IS_TRAP_HWBKPT (siginfo.si_code))
	    {
	      ...

gdb/ChangeLog:
2016-02-24  Pedro Alves  <palves@redhat.com>

	* nat/linux-ptrace.h [__mips__] (GDB_ARCH_IS_TRAP_BRKPT): Also
	accept TRAP_BRKPT.
	 [__mips__] (GDB_ARCH_IS_TRAP_HWBKPT): Also accept TRAP_HWBKPT.
---
 gdb/nat/linux-ptrace.h | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h
index b9123c9..86b1f29 100644
--- a/gdb/nat/linux-ptrace.h
+++ b/gdb/nat/linux-ptrace.h
@@ -140,8 +140,8 @@ struct buffer;
    in SPU code on a Cell/B.E.  However, SI_KERNEL is never seen
    on a SIGTRAP for any other reason.
 
-   The MIPS kernel uses SI_KERNEL for all kernel generated traps.
-   Since:
+   The MIPS kernel (up until 4.5 at least) uses SI_KERNEL for all
+   kernel generated traps.  Since:
 
      - MIPS doesn't do hardware single-step.
      - We don't need to care about exec SIGTRAPs --- we assume
@@ -152,6 +152,9 @@ struct buffer;
    software breakpoints and hardware watchpoints, which can be done by
    peeking the debug registers.
 
+   However, it is anticipated that later MIPS kernels will report
+   proper TRAP_BRKPT and TRAP_HWBKPT codes, so we also match them.
+
    The generic Linux target code should use GDB_ARCH_IS_TRAP_* instead
    of TRAP_* to abstract out these peculiarities.  */
 #if defined __i386__ || defined __x86_64__
@@ -161,8 +164,8 @@ struct buffer;
 # define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL || (X) == TRAP_BRKPT)
 # define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == TRAP_HWBKPT)
 #elif defined __mips__
-# define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL)
-# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == SI_KERNEL)
+# define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL || (X) == TRAP_BRKPT)
+# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == SI_KERNEL || (X) == TRAP_HWBKPT)
 #else
 # define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == TRAP_BRKPT)
 # define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == TRAP_HWBKPT)
-- 
1.9.3

Re: [PATCH] Handle MIPS Linux SIGTRAP siginfo.si_code values

[Maciej W. Rozycki]

On Wed, 24 Feb 2016, Pedro Alves wrote:

> > to actually recognise these events at all in the first place.  So we 
> > better have it right away or updated kernels will break GDB for a change.  
> 
> OK, if you're still willing to change the kernel, let's do it.
> 
> I had somehow imagined (and reading back, I have no idea why) that you
> _didn't_ want to change the si_code, if possible, and was going by that.
> 
> (There's always risk associated with such a change, as it's effectively
> an ABI break and some tool out there may be relying on SI_KERNEL and may
> thus stop working correctly.  Usually ABI stability trumps "cleanliness",
> in kernel circles.)

 I'll post a proposal, cc-ing you (and Luis), and see if anything pops up.  
Given that the only codes for SIGTRAP on MIPS/Linux have so far been 
SI_USER (i.e. 0) or SI_KERNEL, I really doubt that any software bothered 
checking it.  I've never thought of trying to fool a debugger by sending 
it SIGTRAP signals with kill(2) and now that I think of it, then honestly 
I fail to see a reason to actually special-case such fooling and prevent 
the user from doing so -- if they do it, then certainly they must have had 
a reason.

 Did GDB itself check for SI_KERNEL before your recent rewrite?

> I've just finished testing it on s390 -- no regressions.  I've
> pushed it in now, as is.  I'll follow up with a new patch that makes
> gdb accept the anticipated new si_codes too.

 I'm fine with waiting for any outcome from a discussion with kernel 
people before pushing such a change.

  Maciej

Re: [PATCH] Handle MIPS Linux SIGTRAP siginfo.si_code values

[Pedro Alves]

On 02/25/2016 01:39 AM, Maciej W. Rozycki wrote:

>  Did GDB itself check for SI_KERNEL before your recent rewrite?

No, it did not.

> 
>> I've just finished testing it on s390 -- no regressions.  I've
>> pushed it in now, as is.  I'll follow up with a new patch that makes
>> gdb accept the anticipated new si_codes too.
> 
>  I'm fine with waiting for any outcome from a discussion with kernel 
> people before pushing such a change.

Ack.

Thanks,
Pedro Alves

Re: [PATCH] Handle MIPS Linux SIGTRAP siginfo.si_code values

[Maciej W. Rozycki]

Hi Pedro,

> >> I've just finished testing it on s390 -- no regressions.  I've
> >> pushed it in now, as is.  I'll follow up with a new patch that makes
> >> gdb accept the anticipated new si_codes too.
> > 
> >  I'm fine with waiting for any outcome from a discussion with kernel 
> > people before pushing such a change.
> 
> Ack.

 FYI, the kernel change has now made it upstream:

commit 3b143cca6e1397188f507a6c727f4108861ceb8b
Author: Maciej W. Rozycki <macro@imgtec.com>
Date:   Fri Mar 4 01:44:28 2016 +0000

    MIPS: traps: Correct the SIGTRAP debug ABI in `do_watch' and `do_trap_or_bp'

and will be present starting from Linux 4.6.0 release.

  Maciej

Re: [PATCH] Handle MIPS Linux SIGTRAP siginfo.si_code values

[Pedro Alves]

On 04/05/2016 05:57 PM, Maciej W. Rozycki wrote:

>  FYI, the kernel change has now made it upstream:
> 
> commit 3b143cca6e1397188f507a6c727f4108861ceb8b
> Author: Maciej W. Rozycki <macro@imgtec.com>
> Date:   Fri Mar 4 01:44:28 2016 +0000
> 
>     MIPS: traps: Correct the SIGTRAP debug ABI in `do_watch' and `do_trap_or_bp'
> 
> and will be present starting from Linux 4.6.0 release.
> 

Thanks Maciej,

Here's the same patch with updated comments.  I've pushed this to both master
and the 7.11 branch.

I've filed https://sourceware.org/bugzilla/show_bug.cgi?id=19958
(Breakpoints/watchpoints broken on MIPS Linux <= 4.5), as necessary
according to our branch backporting procedure.

From f25c3723b2251e33e725afa52280fde679f3e600 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 15 Apr 2016 23:52:00 +0100
Subject: [PATCH] MIPS/Linux: Also recognize TRAP_BRKPT and TRAP_HWBKPT

This makes the MIPS Linux backends recognize TRAP_BRKPT and
TRAP_HWBKPT in siginfo.si_code in addition to SI_KERNEL, since Linux
4.6 now reports the finer-grained si_code values too.

Refs:
 https://sourceware.org/ml/gdb-patches/2016-02/msg00756.html
 https://sourceware.org/ml/gdb-patches/2016-04/msg00090.html

On kernels that report SI_KERNEL (<= 4.5), we'll enter the "ambiguous"
path of save_stop_reason:

	  if (GDB_ARCH_IS_TRAP_BRKPT (siginfo.si_code)
	      && GDB_ARCH_IS_TRAP_HWBKPT (siginfo.si_code))
	    {
	      /* The si_code is ambiguous on this arch -- check debug
		 registers.  */
	      if (!check_stopped_by_watchpoint (lp))
		lp->stop_reason = TARGET_STOPPED_BY_SW_BREAKPOINT;
	    }

while on kernels that report the finer-grained si_code values (>= 4.6),
we'll enter the corresponding branches:

	  else if (GDB_ARCH_IS_TRAP_BRKPT (siginfo.si_code))
	    {
	    }
	  else if (GDB_ARCH_IS_TRAP_HWBKPT (siginfo.si_code))
	    {
	      ...

gdb/ChangeLog:
2016-04-15  Pedro Alves  <palves@redhat.com>

	* nat/linux-ptrace.h [__mips__] (GDB_ARCH_IS_TRAP_BRKPT): Also
	accept TRAP_BRKPT.
	 [__mips__] (GDB_ARCH_IS_TRAP_HWBKPT): Also accept TRAP_HWBKPT.
---
 gdb/nat/linux-ptrace.h | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h
index b9123c9..0a23bcb 100644
--- a/gdb/nat/linux-ptrace.h
+++ b/gdb/nat/linux-ptrace.h
@@ -140,8 +140,8 @@ struct buffer;
    in SPU code on a Cell/B.E.  However, SI_KERNEL is never seen
    on a SIGTRAP for any other reason.
 
-   The MIPS kernel uses SI_KERNEL for all kernel generated traps.
-   Since:
+   The MIPS kernel up until 4.5 used SI_KERNEL for all kernel
+   generated traps.  Since:
 
      - MIPS doesn't do hardware single-step.
      - We don't need to care about exec SIGTRAPs --- we assume
@@ -152,6 +152,9 @@ struct buffer;
    software breakpoints and hardware watchpoints, which can be done by
    peeking the debug registers.
 
+   Beginning with Linux 4.6, the MIPS port reports proper TRAP_BRKPT and
+   TRAP_HWBKPT codes, so we also match them.
+
    The generic Linux target code should use GDB_ARCH_IS_TRAP_* instead
    of TRAP_* to abstract out these peculiarities.  */
 #if defined __i386__ || defined __x86_64__
@@ -161,8 +164,8 @@ struct buffer;
 # define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL || (X) == TRAP_BRKPT)
 # define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == TRAP_HWBKPT)
 #elif defined __mips__
-# define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL)
-# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == SI_KERNEL)
+# define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL || (X) == TRAP_BRKPT)
+# define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == SI_KERNEL || (X) == TRAP_HWBKPT)
 #else
 # define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == TRAP_BRKPT)
 # define GDB_ARCH_IS_TRAP_HWBKPT(X) ((X) == TRAP_HWBKPT)
-- 
2.5.5