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

[Pedro Alves <pedro@codesourcery.com>]

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