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

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

[Ulrich Weigand]

Hello,

even with the previous patch, some sigstep.exp tests are still failing.
This is due to a peculiarity of the linux-nat target, which sometimes
decides to *not* report a signal event to GDB's main event loop, but
simply re-dispatch the inferior directly.

This is broken if GDB actually needs to handle signal delivery specially,
which is in particular the case if we're currently single-stepping.
Therefore, linux-nat refrains from such by-passing of the main event
loop if single-stepping is in progress.  However, it does so by simply
checking the "step" flag that was passed to its resume implementation.
But this flag will always be zero on targets that do software single-
stepping -- but those also must not bypass signal delivery ...

The patch below changes linux_nat_wait_1 to actually look at the stepping
status of the thread directly, instead of relying on the "step" flag.
This means the "currently_stepping" routine has to be exported from
infrun.c so it can be called here.

Thoughts?

Patch tested on arm-linux, i386-linux, s390-linux, powerpc-linux.

Bye,
Ulrich



ChangeLog:

	* linux-nat.c (linux_nat_wait_1): Do not short-cut signal delivery
	while software single-step in progress.
	* inferior.h (struct thread_info): Add forward declaration.
	(currently_stepping): Add prototype.
	* infrun.c (currently_stepping): Remove prototype.  Make global.

Index: gdb/inferior.h
===================================================================
RCS file: /cvs/src/src/gdb/inferior.h,v
retrieving revision 1.149
diff -u -p -r1.149 inferior.h
--- gdb/inferior.h	9 Jan 2011 03:08:56 -0000	1.149
+++ gdb/inferior.h	17 Jan 2011 19:15:00 -0000
@@ -31,6 +31,7 @@ struct gdbarch;
 struct regcache;
 struct ui_out;
 struct terminal_info;
+struct thread_info;
 
 /* For bpstat.  */
 #include "breakpoint.h"
@@ -253,6 +254,8 @@ extern void ensure_not_running (void);
 
 void set_step_info (struct frame_info *frame, struct symtab_and_line sal);
 
+extern int currently_stepping (struct thread_info *tp);
+
 /* From infcmd.c */
 
 extern void post_create_inferior (struct target_ops *, int);
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
@@ -76,8 +76,6 @@ static int follow_fork (void);
 static void set_schedlock_func (char *args, int from_tty,
 				struct cmd_list_element *c);
 
-static int currently_stepping (struct thread_info *tp);
-
 static int currently_stepping_or_nexting_callback (struct thread_info *tp,
 						   void *data);
 
@@ -4957,7 +4979,7 @@ process_event_stop_test:
 
 /* Is thread TP in the middle of single-stepping?  */
 
-static int
+int
 currently_stepping (struct thread_info *tp)
 {
   return ((tp->control.step_range_end
Index: gdb/linux-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/linux-nat.c,v
retrieving revision 1.191
diff -u -p -r1.191 linux-nat.c
--- gdb/linux-nat.c	9 Jan 2011 03:08:57 -0000	1.191
+++ gdb/linux-nat.c	17 Jan 2011 19:15:08 -0000
@@ -3604,7 +3604,7 @@ retry:
 	 single-stepping, since that may need special care, e.g. to
 	 skip the signal handler, or, if we're gaining control of the
 	 inferior.  */
-      if (!lp->step
+      if (!currently_stepping (find_thread_ptid (lp->ptid))
 	  && inf->control.stop_soon == NO_STOP_QUIETLY
 	  && signal_stop_state (signo) == 0
 	  && signal_print_state (signo) == 0
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

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

[Pedro Alves]

On Wednesday 19 January 2011 09:42:49, Ulrich Weigand wrote:
> even with the previous patch, some sigstep.exp tests are still failing.
> This is due to a peculiarity of the linux-nat target, which sometimes
> decides to *not* report a signal event to GDB's main event loop, but
> simply re-dispatch the inferior directly.
> 
> This is broken if GDB actually needs to handle signal delivery specially,
> which is in particular the case if we're currently single-stepping.
> Therefore, linux-nat refrains from such by-passing of the main event
> loop if single-stepping is in progress.  However, it does so by simply
> checking the "step" flag that was passed to its resume implementation.
> But this flag will always be zero on targets that do software single-
> stepping -- but those also must not bypass signal delivery ...
> 
> The patch below changes linux_nat_wait_1 to actually look at the stepping
> status of the thread directly, instead of relying on the "step" flag.
> This means the "currently_stepping" routine has to be exported from
> infrun.c so it can be called here.
> 
> Thoughts?

I'm not objecting, but I'm curious on whether you've thought about how
the same problem would be solved in gdbserver/linux-low.c, which
can't call currently_stepping, since it's running in a different process?

One way to do it would be to do:

 QPassSignals:
 vCont;c
 QPassSignals:foo;bar

but that is a lot of extra roundtrips, and not really (inferior) threadsafe in 
non-stop mode.

It sounds like we'd need to tweak the target resume interface to be
able to say "continue, but I'm interested in signals and everything else",
or, "I'm telling you to continue, but you're really single-stepping",
like a new vCont;cs or some such?

If we end up needing something like this, then we'd use the same
interface for linux nat.

-- 
Pedro Alves

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

[Ulrich Weigand]

Pedro Alves wrote:
> On Wednesday 19 January 2011 09:42:49, Ulrich Weigand wrote:
> > The patch below changes linux_nat_wait_1 to actually look at the stepping
> > status of the thread directly, instead of relying on the "step" flag.
> > This means the "currently_stepping" routine has to be exported from
> > infrun.c so it can be called here.
> 
> I'm not objecting, but I'm curious on whether you've thought about how
> the same problem would be solved in gdbserver/linux-low.c, which
> can't call currently_stepping, since it's running in a different process?

Good point, however this test already must use information only available
in GDB itself, namely whether or not the signal is to be passed to the
inferior or intercepted by GDB:

     if (!lp->step
          && inf->control.stop_soon == NO_STOP_QUIETLY
          && signal_stop_state (signo) == 0
          && signal_print_state (signo) == 0
          && signal_pass_state (signo) == 1)

(The "stop_soon" state likewise is GDB private data.)

Presumably because of this, gdbserver today does not appear to be
implementing this particular optimization at all, but always reports
all signals back to GDB to decide what to do with them.

> One way to do it would be to do:
> 
>  QPassSignals:
>  vCont;c
>  QPassSignals:foo;bar
> 
> but that is a lot of extra roundtrips, and not really (inferior) threadsafe in 
> non-stop mode.

I agree that this would probably be the way to go about it.  I'm not sure
thread safety is really a concern here, given that we're talking about an
optimization.  If the implementation is conservative in the right direction,
the worst thing that could happen is that a signal is reported that might
have gotten short-circuited ..

Similarly, the number of roundtrips could probably be reduced by only
sending a QPassSignals when the list of interesting signal changes.
For example, once we start single-stepping, we'd once send the 
  QPassSignals:
and then not send and further QPassSignals until we go back to letting
the inferior continue freely.  (And even then, we might further delay
sending of the QPassSignals until such time as we notice we're actually
seeing many to-be-passed signals being delivered to GDB, at which point
switching on the optimization is actually worthwhile.)

> It sounds like we'd need to tweak the target resume interface to be
> able to say "continue, but I'm interested in signals and everything else",
> or, "I'm telling you to continue, but you're really single-stepping",
> like a new vCont;cs or some such?

I'm not so sure I like this, as it introduces somewhat less well-
defined semantics: what does "*really* single-stepping" mean, other
than in terms of doing whatever it is GDB does now ...

Bye,
Ulrich

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

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

[Pedro Alves]

On Wednesday 19 January 2011 11:48:20, Ulrich Weigand wrote:
> Pedro Alves wrote:
> > On Wednesday 19 January 2011 09:42:49, Ulrich Weigand wrote:
> > > The patch below changes linux_nat_wait_1 to actually look at the
> > > stepping status of the thread directly, instead of relying on the
> > > "step" flag. This means the "currently_stepping" routine has to be
> > > exported from infrun.c so it can be called here.
> >
> > 
> >
> > I'm not objecting, but I'm curious on whether you've thought about how
> > the same problem would be solved in gdbserver/linux-low.c, which
> > can't call currently_stepping, since it's running in a different process?
> 
> Good point, however this test already must use information only available
> in GDB itself, namely whether or not the signal is to be passed to the
> inferior or intercepted by GDB:
> 
>      if (!lp->step
>           && inf->control.stop_soon == NO_STOP_QUIETLY
>           && signal_stop_state (signo) == 0
>           && signal_print_state (signo) == 0
>           && signal_pass_state (signo) == 1)
> 
> (The "stop_soon" state likewise is GDB private data.)
> 
> Presumably because of this, gdbserver today does not appear to be
> implementing this particular optimization at all, but always reports
> all signals back to GDB to decide what to do with them.

It does implement it.  It's done in gdbserver/linux-low.c:linux_wait_1.
Look for:

	  (pass_signals[target_signal_from_host (WSTOPSIG (w))]
 
In linux-nat.c's case, stop_soon used to be set to STOP_QUIETLY
while doing the startup_inferior dance, but it no longer does ever
since you've rewriten startup_inferior to not use wait_for_inferior,
but use target_resume/target_wait directly, I think.  I guess it
may be a bug that stop_soon is no longer set in that case?

In any case, gdbserver does it's own startup_inferior equivalent 
(gdbserver/server.c:start_inferior), so it does not need to peek
into gdb's inf->control.stop_soon for this case.  It can use a flag
in its own inferior/process structure.

The flag is also set to STOP_QUIETLY in svr4_solib_create_inferior_hook,
but that's in code that only gets built for SCO.  In this case,
if we cared for SCO, I'd say we'd wrap the loop in
svr4_solib_create_inferior_hook with the same
QPassSignals dance (the target method in question
is target_notice_signals, btw).

> 
> > One way to do it would be to do:
> > 
> >  QPassSignals:
> >  vCont;c
> >  QPassSignals:foo;bar
> > 
> >
> > but that is a lot of extra roundtrips, and not really (inferior)
> > threadsafe in  non-stop mode.
> 
> I agree that this would probably be the way to go about it.  I'm not sure
> thread safety is really a concern here, given that we're talking about an
> optimization.  If the implementation is conservative in the right
> direction, the worst thing that could happen is that a signal is reported
> that might have gotten short-circuited ..

Right.

> 
> Similarly, the number of roundtrips could probably be reduced by only
> sending a QPassSignals when the list of interesting signal changes.
> For example, once we start single-stepping, we'd once send the 
>   QPassSignals:
> and then not send and further QPassSignals until we go back to letting
> the inferior continue freely.  

Yeah.  remote.c:remote_pass_signals  already avoids sending the
QPassSignals packet to the target if the list of interesting
signals didn't change.

> 
> > It sounds like we'd need to tweak the target resume interface to be
> > able to say "continue, but I'm interested in signals and everything
> > else", or, "I'm telling you to continue, but you're really
> > single-stepping", like a new vCont;cs or some such?
> 
> I'm not so sure I like this, as it introduces somewhat less well-
> defined semantics: what does "*really* single-stepping" mean, other
> than in terms of doing whatever it is GDB does now ...

Agreed.

-- 
Pedro Alves

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

[Ulrich Weigand]

Pedro Alves wrote:
> On Wednesday 19 January 2011 11:48:20, Ulrich Weigand wrote:
> > Presumably because of this, gdbserver today does not appear to be
> > implementing this particular optimization at all, but always reports
> > all signals back to GDB to decide what to do with them.
> 
> It does implement it.  It's done in gdbserver/linux-low.c:linux_wait_1.
> Look for:
> 
> 	  (pass_signals[target_signal_from_host (WSTOPSIG (w))]

D'oh, I somehow completely missed that.  Sorry.

> In linux-nat.c's case, stop_soon used to be set to STOP_QUIETLY
> while doing the startup_inferior dance, but it no longer does ever
> since you've rewriten startup_inferior to not use wait_for_inferior,
> but use target_resume/target_wait directly, I think.  I guess it
> may be a bug that stop_soon is no longer set in that case?

Actually, IIRC one of the reasons for the startup_inferior was to
*get away* from using the magic stop_soon flag to influence the
behaviour in somewhat under-defined ways, but instead just directly
handle whatever events we expect here.  I probably wasn't aware that
the *target* itself directly uses the flag too ...

In any case, if we move to a solution where we toggle the signal
pass flags as needed, we should simply set all signals to non-pass
during startup too.

> The flag is also set to STOP_QUIETLY in svr4_solib_create_inferior_hook,
> but that's in code that only gets built for SCO.  In this case,
> if we cared for SCO, I'd say we'd wrap the loop in
> svr4_solib_create_inferior_hook with the same
> QPassSignals dance (the target method in question
> is target_notice_signals, btw).

Right.

I'll see if I can come up with a patch ...

Bye,
Ulrich

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