Re: [PATCH] Displaced stepping (non-stop debugging) support for ARM Linux

[Pedro Alves <pedro at codesourcery dot com>]

On Tuesday 09 June 2009 18:37:09, Daniel Jacobowitz wrote:
> On Sat, May 16, 2009 at 07:19:10PM +0100, Julian Brown wrote:

> > I'm not sure what the status is here now. For testing purposes, I've
> > (still) been using a local patch which uses displaced stepping for all
> > single-step operations.

> We still can't use software single-stepping simultaneously in multiple
> threads.  Pedro, should we fix that or always use displaced stepping
> for now?

It would be nice to have that fixed, for sure, so yes to the
we should fix that question.  However, it seems to me that this
is something that can be worked on mostly independently of the ARM
bits as it's a general software single-step issue, not really ARM
specific.  Unless someone wants to (and has time to) tackle it
right now, I'd say go with the always displace-step version.  If
nothing else, helps in stressing the displaced stepping
implementation.  :-)

> 
> > Daniel Jacobowitz <drow@false.org> wrote:
> > 
> > > * What's the point of executing mov<cond> on the target for BL<cond>?
> > > At that point it seems like we ought to skip the target step entirely;
> > > just simulate the instruction.  We've already got a function to check
> > > conditions (condition_true).
> > 
> > I'm now using NOP instructions and condition_true, because the current
> > displaced stepping support wants to execute "something" rather than
> > nothing.
> 
> From infrun.c:
> 
>    One way to work around [software single stepping]...
>    would be to have gdbarch_displaced_step_copy_insn fully
>    simulate the effect of PC-relative instructions (and return NULL)
>    on architectures that use software single-stepping.
> 
> So the interface you need is there; it's just not implemented yet:
> 
>   /* We don't support the fully-simulated case at present.  */
>   gdb_assert (closure);
> 
> I think the implementation strategy will look like:
> 
>   * Add another non-zero return value from displaced_step_prepare.

The thread should still be marked as running in the frontend/cli's
perspective, as it would be stuck doing internal things, so that
the user can't try to "continue" it again ...

> 
>   * Update should_resume after the call, in resume (currently unused).

... so this bit in resume applies:

    {
      if (!displaced_step_prepare (inferior_ptid))
	{
	  /* Got placed in displaced stepping queue.  Will be resumed
	     later when all the currently queued displaced stepping
	     requests finish.  The thread is not executing at this point,
	     and the call to set_executing will be made later.  But we
	     need to call set_running here, since from frontend point of view,
	     the thread is running.  */
	  set_running (inferior_ptid, 1);
	  discard_cleanups (old_cleanups);
	  return;
	}
    }

You need that, since "set_running" is usually called from target_resume,
which you'd bypass in the fully similated case.

> 
>   * Ask Pedro how to pretend that the inferior resumed and stopped,
>   for higher levels.  

See below.

>   I think this will entail a new queue.   

Indeed.  Every time we required something similar, we got
away with doing that shortcut inside target code, keeping infrun
agnostic of the trick.  But this is gdbarch code, independent of which
target_ops is playing (e.g., could be native linux-nat.c, could be remote.c).

>   Bonus 
>   points if prepare_to_wait and wait_for_inferior do not invalidate
>   the perfectly good register cache at this point.
> 
> Pedro, thoughts - easy or should we stick with the NOP workaround for
> now?

In sync mode, I can picture bypassing the target_wait call in
wait_for_inferior, by peeking a local event queue first.  In
async (and non-stop) modes, that would happen in fetch_inferior_event,
which is an asynchronous event loop callback.  This requires registering
a new event source from within infrun.c, and, marking it whenever
we have events to handle (that is, when the queue isn't empty).  In
principle, it shouldn't be hard.  Keep a local queue of events in
infrun.c (as first approximation, each event consisting of a ptid
and a target_waitstatus), and add an async_event_handler to infrun.c (look
around in remote.c for event queue and remote_async_inferior_event_token
for copy&paste sources).  Then, in displaced_step_prepare, if you just
detected a fully simulated case, push a new stop
event (TARGET_WAITKIND_STOP/SIGTRAP) in this queue, and mark the
event source as having something to process.  The event source
callback would be something like:

static void
infrun_async_inferior_event_handler (gdb_client_data data)
{
  inferior_event_handler (INF_REG_EVENT, NULL);
}

Then, just do nothing else, returning back to the event loop, which
will eventually call inferior_event_handler->fetch_inferior_event, and
that would collect the event from the local event queue, and pass it
to handle_inferior_event as usual.

The event queue would expose a similar interface to target_wait,
so that wait_for_inferior can request a queued event from a
specific ptid (and things look neat).

Care must be taken to keep the event queue in sync with target
reality --- e.g., if the thread that was doing the short-circuit (and so
has a pending event in a new infrun.c local event queue) exits (because
e.g., we lost connection to the target in between, or the whole
process exits due to another thread doing  an _exit call or something
like that), then we need to remove the event from the queue,
otherwise, when you go to process it, things break while referencing
a thread that doesn't exist anymore.  Should be mostly a matter of
taking care of the event queue from within infrun.c:infrun_thread_thread_exit.

Another source of care is if there's code out there that tries to resume
all threads and the target happens to try to resume such a thread behind
infrun event queue's back.  This shouldn't be a problem in non-stop
mode, though, so, probably not something to worry much about much
for now.

-- 
Pedro Alves