[rfc][1/2] Signal delivery + software single-step is broken

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

Hello,

we're seeing failing testcases related to signal delivery on ARM,
which turn out to be related to software single-stepping.

If GDB wants to pass a signal to the inferior, it calls ptrace
with non-zero "data" argument.  If at this point, the inferior
was stopped on a breakpoint location, GDB will in addition need
to single-step off that breakpoint.

Now, if the platform supports hardware single-stepping, this will
end up as a PTRACE_SINGLESTEP ptrace call with non-zero "data".
If delivery of that signal results in invocation of a handler,
GDB will expect the OS to trap back to the debugger when the
inferior is at the first instruction of the handler.  (At this
point, GDB usually skip the handler by placing a step-resume
breakpoint at its return address and the single-stepping
through the signal return trampoline.)

However, if the platform does *not* support hardware single-
stepping, things seem broken.  GDB will attempt to fall back
to inserting software single-step breakpoints.  Now this works
only if GDB is able to determine the set of potential next
instruction addresses.  If a signal is about to be delivered,
however, the start of the signal handler is one of these
potential addresses -- but the software single-step code does
not know about those.  (And in fact, there does not seem to be
an easy way to safely and efficiently determine them.)

So what happens is that we'll set single-step breakpoints only
on the original successors to the current instruction, and then
deliver the signal.  This means execution runs through the
signal handler, while GDB has removed all other breakpoints
(since we're stepping off a breakpoint!).  If there was a
breakpoint somewhere in the signal handler, we will therefore
miss it.


I'm not completely sure how to best fix this problem.  One option
could be to change GDB's method of handling signal delivery while
stopped on a breakpoint location: we could just deliver the signal
first, while a step-resume breakpoint on the current location is
in effect; and once that breakpoint is hit, we *then* single-step
off the location as usual.  This is actually already implemented
in handle_inferior_event when delivering a signal that originated
in the inferior itself.  The patch below moves that mechanism to
resume, so that it gets also used for delivering signals that
originate in GDB (e.g. via the "signal" command).

This fixes the following set of test cases on ARM:
FAIL: gdb.base/annota1.exp: send SIGUSR1 (timeout)
FAIL: gdb.base/annota1.exp: backtrace @ signal handler (timeout)
FAIL: gdb.base/annota3.exp: send SIGUSR1 (pattern 4)
FAIL: gdb.base/annota3.exp: backtrace @ signal handler (pattern 1)

and in conjunction with the follow-on patch, it also fixes:
FAIL: gdb.base/sigstep.exp: step on breakpoint, to handler; performing step
FAIL: gdb.base/sigstep.exp: next on breakpoint, to handler; performing next
FAIL: gdb.base/sigstep.exp: continue on breakpoint, to handler; performing continue
FAIL: gdb.base/sigstep.exp: step on breakpoint, to handler entry; performing step
FAIL: gdb.base/sigstep.exp: next on breakpoint, to handler entry; performing next
FAIL: gdb.base/sigstep.exp: continue on breakpoint, to handler entry; performing continue

I've tested the patch on i386-linux, s390-linux, and powerpc-linux
(including Cell/B.E.) with no regressions.


One potential problem that this patch still does not resolve is if we
at the same time are also doing software-emulated watchpoints (which
are implemented by single-stepping all the time).  With the patch as
below, software single-stepping would be disabled while a signal
handler is executing, so we'd notice changes to a watches location
only after the handler has completed ...

I'd appreciate any comments -- is there a better way of doing this?

Bye,
Ulrich


ChangeLog:

	* infrun.c (resume): Do not single-step into signal delivery
	when stepping off a breakpoint location.
	(handle_inferior_event): Update to new resume behaviour.
	(insert_step_resume_breakpoint_at_frame): Move prototype earlier.
	(insert_step_resume_breakpoint_at_caller): Likewise.
	(insert_step_resume_breakpoint_at_sal): Likewise.

testsuite/ChangeLog:

	* gdb.base/annota1.exp: Accept breakpoints-invalid annotation
	while delivering signal.


Index: gdb/infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.467
diff -u -p -r1.467 infrun.c
--- gdb/infrun.c	9 Jan 2011 03:08:56 -0000	1.467
+++ gdb/infrun.c	17 Jan 2011 19:15:05 -0000
@@ -99,6 +97,14 @@ void _initialize_infrun (void);
 
 void nullify_last_target_wait_ptid (void);
 
+static void insert_step_resume_breakpoint_at_frame (struct frame_info *);
+
+static void insert_step_resume_breakpoint_at_caller (struct frame_info *);
+
+static void insert_step_resume_breakpoint_at_sal (struct gdbarch *,
+						  struct symtab_and_line ,
+						  struct frame_id);
+
 /* When set, stop the 'step' command if we enter a function which has
    no line number information.  The normal behavior is that we step
    over such function.  */
@@ -1666,6 +1672,27 @@ a command like `return' or `jump' to con
 						   displaced->step_closure);
     }
 
+  /* Stepping over a breakpoint while at the same time delivering a signal
+     has a problem: we cannot use displaced stepping (see above), but we
+     also cannot use software single-stepping, because we do not know
+     where execution will continue if a signal handler is installed.
+
+     On the other hand, if there is a signal handler we'd have to step
+     over it anyway.  So what we do instead is to install a step-resume
+     handler at the current address right away, deliver the signal without
+     stepping (which means we may have to re-insert breakpoints), and once
+     we arrive back at the step-resume breakpoint, step once more over the
+     original breakpoint we wanted to step over.  */
+  else if (step && tp->control.trap_expected && sig != TARGET_SIGNAL_0
+	   && execution_direction == EXEC_FORWARD)
+    {
+      insert_step_resume_breakpoint_at_frame (get_current_frame ());
+      insert_breakpoints ();
+      tp->step_after_step_resume_breakpoint = 1;
+      tp->control.trap_expected = 0;
+      step = 0;
+    }
+
   /* Do we need to do it the hard way, w/temp breakpoints?  */
   else if (step)
     step = maybe_software_singlestep (gdbarch, pc);
@@ -2232,11 +2259,6 @@ static void handle_step_into_function (s
 				       struct execution_control_state *ecs);
 static void handle_step_into_function_backward (struct gdbarch *gdbarch,
 						struct execution_control_state *ecs);
-static void insert_step_resume_breakpoint_at_frame (struct frame_info *);
-static void insert_step_resume_breakpoint_at_caller (struct frame_info *);
-static void insert_step_resume_breakpoint_at_sal (struct gdbarch *,
-						  struct symtab_and_line ,
-						  struct frame_id);
 static void insert_longjmp_resume_breakpoint (struct gdbarch *, CORE_ADDR);
 static void check_exception_resume (struct execution_control_state *,
 				    struct frame_info *, struct symbol *);
@@ -4060,7 +4082,8 @@ process_event_stop_test:
 	     Instead this signal arrives.  This signal will take us out
 	     of the stepping range so GDB needs to remember to, when
 	     the signal handler returns, resume stepping off that
-	     breakpoint.  */
+	     breakpoint.  This happens in resume, so we simply need
+	     to keep stepping here.  */
 	  /* To simplify things, "continue" is forced to use the same
 	     code paths as single-step - set a breakpoint at the
 	     signal return address and then, once hit, step off that
@@ -4070,8 +4093,7 @@ process_event_stop_test:
                                 "infrun: signal arrived while stepping over "
                                 "breakpoint\n");
 
-	  insert_step_resume_breakpoint_at_frame (frame);
-	  ecs->event_thread->step_after_step_resume_breakpoint = 1;
+	  ecs->event_thread->stepping_over_breakpoint = 1;
 	  keep_going (ecs);
 	  return;
 	}
Index: gdb/testsuite/gdb.base/annota1.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/annota1.exp,v
retrieving revision 1.40
diff -u -p -r1.40 annota1.exp
--- gdb/testsuite/gdb.base/annota1.exp	1 Jan 2011 15:33:40 -0000	1.40
+++ gdb/testsuite/gdb.base/annota1.exp	17 Jan 2011 19:15:13 -0000
@@ -266,10 +266,10 @@ if [target_info exists gdb,nosignals] {
     unsupported "backtrace @ signal handler"
 } else {
     gdb_test_multiple "signal SIGUSR1" "send SIGUSR1" {
-	-re "\r\n\032\032post-prompt\r\nContinuing with signal SIGUSR1.\r\n\r\n\032\032starting\(\r\n\r\n\032\032frames-invalid\)+\r\n\r\n\032\032breakpoint 2\r\n\r\nBreakpoint 2, \r\n\032\032frame-begin 0 $hex\r\n\r\n\032\032frame-function-name\r\nhandle_USR1\r\n\032\032frame-args\r\n \\(\r\n\032\032arg-begin\r\nsig\r\n\032\032arg-name-end\r\n=\r\n\032\032arg-value -\r\n$decimal\r\n\032\032arg-end\r\n\\)\r\n\032\032frame-source-begin\r\n at \r\n\032\032frame-source-file\r\n${escapedsrcfile}\r\n\032\032frame-source-file-end\r\n:\r\n\032\032frame-source-line\r\n.*\r\n\032\032frame-source-end\r\n\r\n\r\n\032\032source.*annota1.c:.*:.*:beg:$hex\r\n\r\n\032\032frame-end\r\n\r\n\032\032stopped\r\n$gdb_prompt$" {
+	-re "\r\n\032\032post-prompt\r\nContinuing with signal SIGUSR1.\r\n\r\n\032\032starting\(\(\r\n\r\n\032\032frames-invalid\)|\(\r\n\r\n\032\032breakpoints-invalid\)\)+\r\n\r\n\032\032breakpoint 2\r\n\r\nBreakpoint 2, \r\n\032\032frame-begin 0 $hex\r\n\r\n\032\032frame-function-name\r\nhandle_USR1\r\n\032\032frame-args\r\n \\(\r\n\032\032arg-begin\r\nsig\r\n\032\032arg-name-end\r\n=\r\n\032\032arg-value -\r\n$decimal\r\n\032\032arg-end\r\n\\)\r\n\032\032frame-source-begin\r\n at \r\n\032\032frame-source-file\r\n${escapedsrcfile}\r\n\032\032frame-source-file-end\r\n:\r\n\032\032frame-source-line\r\n.*\r\n\032\032frame-source-end\r\n\r\n\r\n\032\032source.*annota1.c:.*:.*:beg:$hex\r\n\r\n\032\032frame-end\r\n\r\n\032\032stopped\r\n$gdb_prompt$" {
 	    pass "send SIGUSR1"
 	}
-	-re "\r\n\032\032post-prompt\r\nContinuing with signal SIGUSR1.\r\n\r\n\032\032starting\(\r\n\r\n\032\032frames-invalid\)+\r\n\r\n\032\032breakpoint 2\r\n\r\nBreakpoint 2, \r\n\032\032frame-begin 0 $hex\r\n\r\n\032\032frame-function-name\r\nhandle_USR1\r\n\032\032frame-args\r\n \\(\r\n\032\032arg-begin\r\nsig\r\n\032\032arg-name-end\r\n=\r\n\032\032arg-value -\r\n$decimal\r\n\032\032arg-end\r\n\\)\r\n\032\032frame-source-begin\r\n at \r\n\032\032frame-source-file\r\n.*${srcfile}\r\n\032\032frame-source-file-end\r\n:\r\n\032\032frame-source-line\r\n.*\r\n\032\032frame-source-end\r\n\r\n\r\n\032\032source.*annota1.c:.*:.*:beg:$hex\r\n\r\n\032\032frame-end\r\n\r\n\032\032stopped\r\n$gdb_prompt$" {
+	-re "\r\n\032\032post-prompt\r\nContinuing with signal SIGUSR1.\r\n\r\n\032\032starting\(\(\r\n\r\n\032\032frames-invalid\)|\(\r\n\r\n\032\032breakpoints-invalid\)\)+\r\n\r\n\032\032breakpoint 2\r\n\r\nBreakpoint 2, \r\n\032\032frame-begin 0 $hex\r\n\r\n\032\032frame-function-name\r\nhandle_USR1\r\n\032\032frame-args\r\n \\(\r\n\032\032arg-begin\r\nsig\r\n\032\032arg-name-end\r\n=\r\n\032\032arg-value -\r\n$decimal\r\n\032\032arg-end\r\n\\)\r\n\032\032frame-source-begin\r\n at \r\n\032\032frame-source-file\r\n.*${srcfile}\r\n\032\032frame-source-file-end\r\n:\r\n\032\032frame-source-line\r\n.*\r\n\032\032frame-source-end\r\n\r\n\r\n\032\032source.*annota1.c:.*:.*:beg:$hex\r\n\r\n\032\032frame-end\r\n\r\n\032\032stopped\r\n$gdb_prompt$" {
 	    setup_xfail "*-*-*" 1270
 	    fail "send SIGUSR1"
 	}
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com