RFC: Updates support for breakpoints that generate SIGILL

RFC: Updates support for breakpoints that generate SIGILL

[Daniel Jacobowitz]

GDB has several bits of code which claim to handle breakpoints that
generate SIGILL, SIGEMT (?), or SIGSEGV.  I needed to use this code
for testing; I have a patch coming up which requires a new breakpoint
instruction on ARM targets, and existing kernels report SIGILL
on that instruction.

I've submitted a kernel patch to add the new breakpoint to the list
that generate SIGTRAP.  So, eventually, I won't care about this patch
any more.  But I think it's worthwhile anyway, unless there's a
problem with it I haven't found.

I didn't touch linux-nat.  It will probably have similar problems to
gdbserver for multi-threaded programs, but I can't test it.  I claim
that this patch is strictly an improvement over the status quo.

Any thoughts about this patch, or shall I check it in?  Tested on
arm-linux-gnueabi using gdbserver, both with and without SIGILLs.

-- 
Daniel Jacobowitz
CodeSourcery

2010-01-28  Daniel Jacobowitz  <dan@codesourcery.com>

	gdb/
	* infrun.c (prepare_to_proceed): Handle other signals which might
	match a breakpoint.
	(handle_inferior_event): Move the check for unusual breakpoint
	signals earlier.

	gdb/gdbserver/
	* linux-low.c (get_stop_pc): Check for SIGTRAP.
	(linux_wait_for_event_1): Handle SIGILL and SIGSEGV as possible
	breakpoints.

---
 gdb/gdbserver/linux-low.c |   21 ++++++++++++++-----
 gdb/infrun.c              |   50 ++++++++++++++++++++++++++--------------------
 2 files changed, 45 insertions(+), 26 deletions(-)

Index: gdb-mainline/gdb/infrun.c
===================================================================
--- gdb-mainline.orig/gdb/infrun.c	2010-01-28 13:30:18.000000000 -0800
+++ gdb-mainline/gdb/infrun.c	2010-01-28 13:31:22.000000000 -0800
@@ -1642,7 +1642,10 @@ prepare_to_proceed (int step)
 
   /* Make sure we were stopped at a breakpoint.  */
   if (wait_status.kind != TARGET_WAITKIND_STOPPED
-      || wait_status.value.sig != TARGET_SIGNAL_TRAP)
+      || (wait_status.value.sig != TARGET_SIGNAL_TRAP
+	  && wait_status.value.sig != TARGET_SIGNAL_ILL
+	  && wait_status.value.sig != TARGET_SIGNAL_SEGV
+	  && wait_status.value.sig != TARGET_SIGNAL_EMT))
     {
       return 0;
     }
@@ -2673,6 +2676,7 @@ handle_inferior_event (struct execution_
 {
   struct frame_info *frame;
   struct gdbarch *gdbarch;
+  struct regcache *regcache;
   int sw_single_step_trap_p = 0;
   int stopped_by_watchpoint;
   int stepped_after_stopped_by_watchpoint = 0;
@@ -2733,6 +2737,30 @@ handle_inferior_event (struct execution_
 
   breakpoint_retire_moribund ();
 
+  /* First, distinguish signals caused by the debugger from signals
+     that have to do with the program's own actions.  Note that
+     breakpoint insns may cause SIGTRAP or SIGILL or SIGEMT, depending
+     on the operating system version.  Here we detect when a SIGILL or
+     SIGEMT is really a breakpoint and change it to SIGTRAP.  We do
+     something similar for SIGSEGV, since a SIGSEGV will be generated
+     when we're trying to execute a breakpoint instruction on a
+     non-executable stack.  This happens for call dummy breakpoints
+     for architectures like SPARC that place call dummies on the
+     stack.  */
+  regcache = get_thread_regcache (ecs->ptid);
+  if (ecs->ws.kind == TARGET_WAITKIND_STOPPED
+      && (ecs->ws.value.sig == TARGET_SIGNAL_ILL
+	  || ecs->ws.value.sig == TARGET_SIGNAL_SEGV
+	  || ecs->ws.value.sig == TARGET_SIGNAL_EMT)
+      && breakpoint_inserted_here_p (get_regcache_aspace (regcache),
+				     regcache_read_pc (regcache)))
+    {
+      if (debug_infrun)
+	fprintf_unfiltered (gdb_stdlog,
+			    "infrun: Treating signal as SIGTRAP\n");
+      ecs->ws.value.sig = TARGET_SIGNAL_TRAP;
+    }
+
   /* Mark the non-executing threads accordingly.  In all-stop, all
      threads of all processes are stopped when we get any event
      reported.  In non-stop mode, only the event thread stops.  If
@@ -3512,27 +3540,7 @@ targets should add new threads to the th
      3) set ecs->random_signal to 1, and the decision between 1 and 2
      will be made according to the signal handling tables.  */
 
-  /* First, distinguish signals caused by the debugger from signals
-     that have to do with the program's own actions.  Note that
-     breakpoint insns may cause SIGTRAP or SIGILL or SIGEMT, depending
-     on the operating system version.  Here we detect when a SIGILL or
-     SIGEMT is really a breakpoint and change it to SIGTRAP.  We do
-     something similar for SIGSEGV, since a SIGSEGV will be generated
-     when we're trying to execute a breakpoint instruction on a
-     non-executable stack.  This happens for call dummy breakpoints
-     for architectures like SPARC that place call dummies on the
-     stack.
-
-     If we're doing a displaced step past a breakpoint, then the
-     breakpoint is always inserted at the original instruction;
-     non-standard signals can't be explained by the breakpoint.  */
   if (ecs->event_thread->stop_signal == TARGET_SIGNAL_TRAP
-      || (! ecs->event_thread->trap_expected
-          && breakpoint_inserted_here_p (get_regcache_aspace (get_current_regcache ()),
-					 stop_pc)
-	  && (ecs->event_thread->stop_signal == TARGET_SIGNAL_ILL
-	      || ecs->event_thread->stop_signal == TARGET_SIGNAL_SEGV
-	      || ecs->event_thread->stop_signal == TARGET_SIGNAL_EMT))
       || stop_soon == STOP_QUIETLY || stop_soon == STOP_QUIETLY_NO_SIGSTOP
       || stop_soon == STOP_QUIETLY_REMOTE)
     {
Index: gdb-mainline/gdb/gdbserver/linux-low.c
===================================================================
--- gdb-mainline.orig/gdb/gdbserver/linux-low.c	2010-01-28 12:21:54.000000000 -0800
+++ gdb-mainline/gdb/gdbserver/linux-low.c	2010-01-28 13:31:39.000000000 -0800
@@ -448,7 +448,8 @@ get_stop_pc (void)
 {
   CORE_ADDR stop_pc = (*the_low_target.get_pc) ();
 
-  if (! get_thread_lwp (current_inferior)->stepping)
+  if (! get_thread_lwp (current_inferior)->stepping
+      && WSTOPSIG (get_thread_lwp (current_inferior)->last_status) == SIGTRAP)
     stop_pc -= the_low_target.decr_pc_after_break;
 
   if (debug_threads)
@@ -1233,18 +1234,28 @@ linux_wait_for_event_1 (ptid_t ptid, int
 	  continue;
 	}
 
-      /* If this event was not handled above, and is not a SIGTRAP, report
-	 it.  */
-      if (!WIFSTOPPED (*wstat) || WSTOPSIG (*wstat) != SIGTRAP)
+      /* If this event was not handled above, and is not a SIGTRAP,
+	 report it.  SIGILL and SIGSEGV are also treated as traps in case
+	 a breakpoint is inserted at the current PC.  */
+      if (!WIFSTOPPED (*wstat)
+	  || (WSTOPSIG (*wstat) != SIGTRAP && WSTOPSIG (*wstat) != SIGILL
+	      && WSTOPSIG (*wstat) != SIGSEGV))
 	return lwpid_of (event_child);
 
       /* If this target does not support breakpoints, we simply report the
-	 SIGTRAP; it's of no concern to us.  */
+	 signal; it's of no concern to us.  */
       if (the_low_target.get_pc == NULL)
 	return lwpid_of (event_child);
 
       stop_pc = get_stop_pc ();
 
+      /* Only handle SIGILL or SIGSEGV if we've hit a recognized
+	 breakpoint.  */
+      if (WSTOPSIG (*wstat) != SIGTRAP
+	  && (event_child->stepping
+	      || ! (*the_low_target.breakpoint_at) (stop_pc)))
+	return lwpid_of (event_child);
+
       /* bp_reinsert will only be set if we were single-stepping.
 	 Notice that we will resume the process after hitting
 	 a gdbserver breakpoint; single-stepping to/over one

Re: RFC: Updates support for breakpoints that generate SIGILL

[Joel Brobecker]

> Any thoughts about this patch, or shall I check it in?  Tested on
> arm-linux-gnueabi using gdbserver, both with and without SIGILLs.

I agree that the patch is useful - someone else might need the same
functionality later on too.

> 2010-01-28  Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	gdb/
> 	* infrun.c (prepare_to_proceed): Handle other signals which might
> 	match a breakpoint.
> 	(handle_inferior_event): Move the check for unusual breakpoint
> 	signals earlier.
> 
> 	gdb/gdbserver/
> 	* linux-low.c (get_stop_pc): Check for SIGTRAP.
> 	(linux_wait_for_event_1): Handle SIGILL and SIGSEGV as possible
> 	breakpoints.

-- 
Joel

Re: RFC: Updates support for breakpoints that generate SIGILL

[Daniel Jacobowitz]

On Fri, Jan 29, 2010 at 08:09:33AM +0400, Joel Brobecker wrote:
> > Any thoughts about this patch, or shall I check it in?  Tested on
> > arm-linux-gnueabi using gdbserver, both with and without SIGILLs.
> 
> I agree that the patch is useful - someone else might need the same
> functionality later on too.

Thanks, I've checked it in.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: Updates support for breakpoints that generate SIGILL

[Ulrich Weigand]

Daniel Jacobowitz wrote:

> 2010-01-28  Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	* infrun.c (prepare_to_proceed): Handle other signals which might
> 	match a breakpoint.
> 	(handle_inferior_event): Move the check for unusual breakpoint
> 	signals earlier.


Unfortunately this change broke Cell/B.E. debugging.

The problem is that with the change, handle_inferior_event will now
*always* look up a regcache, even when the process has exited.

Before the patch, the code has always taken care to avoid that; while
some targets don't mind, others will break (in particular, the Cell/B.E.
multi-arch target does).

The patch below changes this to only look up the regcache when needed
(in particular, this implies the process is still there).

Tested on Cell/B.E. (ppc32 and ppc64) with no regressions.

Does this look OK to you?

Bye,
Ulricu


ChangeLog:

	* infrun.c (handle_inferior_event): Do not look up regcache
	for exited processes.

Index: gdb/infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.428
diff -u -p -r1.428 infrun.c
--- gdb/infrun.c	29 Jan 2010 15:40:21 -0000	1.428
+++ gdb/infrun.c	8 Feb 2010 18:03:29 -0000
@@ -2664,7 +2664,6 @@ handle_inferior_event (struct execution_
 {
   struct frame_info *frame;
   struct gdbarch *gdbarch;
-  struct regcache *regcache;
   int sw_single_step_trap_p = 0;
   int stopped_by_watchpoint;
   int stepped_after_stopped_by_watchpoint = 0;
@@ -2735,18 +2734,21 @@ handle_inferior_event (struct execution_
      non-executable stack.  This happens for call dummy breakpoints
      for architectures like SPARC that place call dummies on the
      stack.  */
-  regcache = get_thread_regcache (ecs->ptid);
   if (ecs->ws.kind == TARGET_WAITKIND_STOPPED
       && (ecs->ws.value.sig == TARGET_SIGNAL_ILL
 	  || ecs->ws.value.sig == TARGET_SIGNAL_SEGV
-	  || ecs->ws.value.sig == TARGET_SIGNAL_EMT)
-      && breakpoint_inserted_here_p (get_regcache_aspace (regcache),
-				     regcache_read_pc (regcache)))
+	  || ecs->ws.value.sig == TARGET_SIGNAL_EMT))
     {
-      if (debug_infrun)
-	fprintf_unfiltered (gdb_stdlog,
-			    "infrun: Treating signal as SIGTRAP\n");
-      ecs->ws.value.sig = TARGET_SIGNAL_TRAP;
+      struct regcache *regcache = get_thread_regcache (ecs->ptid);
+
+      if (breakpoint_inserted_here_p (get_regcache_aspace (regcache),
+				      regcache_read_pc (regcache)))
+	{
+	  if (debug_infrun)
+	    fprintf_unfiltered (gdb_stdlog,
+				"infrun: Treating signal as SIGTRAP\n");
+	  ecs->ws.value.sig = TARGET_SIGNAL_TRAP;
+	}
     }
 
   /* Mark the non-executing threads accordingly.  In all-stop, all

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: RFC: Updates support for breakpoints that generate SIGILL

[Daniel Jacobowitz]

On Mon, Feb 08, 2010 at 08:35:17PM +0100, Ulrich Weigand wrote:
> Unfortunately this change broke Cell/B.E. debugging.
> 
> The problem is that with the change, handle_inferior_event will now
> *always* look up a regcache, even when the process has exited.
> 
> Before the patch, the code has always taken care to avoid that; while
> some targets don't mind, others will break (in particular, the Cell/B.E.
> multi-arch target does).
> 
> The patch below changes this to only look up the regcache when needed
> (in particular, this implies the process is still there).
> 
> Tested on Cell/B.E. (ppc32 and ppc64) with no regressions.
> 
> Does this look OK to you?

Sorry for the breakage.  Yes, your fix looks good to me.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: Updates support for breakpoints that generate SIGILL

[Ulrich Weigand]

Daniel Jacobowitz wrote:

> Sorry for the breakage.  Yes, your fix looks good to me.

Thanks for the review!  I've checked this in now.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com