Re: [RFC] 07/10 non-stop inferior control

[Pedro Alves <pedro at codesourcery dot com>]

A Thursday 08 May 2008 12:40:00, Eli Zaretskii wrote:
> > From: Pedro Alves <pedro@codesourcery.com>
> > Date: Tue, 6 May 2008 16:49:23 +0100
> >
> >    if (ecs->new_thread_event)
> >      {
> > +      if (non_stop)
> > +	/* Non-stop requires a behaving target.  */
> > +	internal_error (__FILE__, __LINE__,
> > +			"target misreported a new thread in non-stop mode.");
> > +
>
> What does this internal_error mean?  The wording of the error message
> doesn't seem to justify an internal_error.  What am I missing here?

Isn't this new_thread_event handling a bandaid for some
misbehaving target?  My understanding was that targets should handle
new thread events (and call add_thread*) themselves nowadays.

There are two issues with non-stop here.

First, is that I context switched before this code already, which
isn't that great when the new inferior ptid isn't listed yet
(the next context switch after the new thread is added to
the list will store the wrong context).

In non-stop, context switching should happen
before adjust_pc_after_break, because that relies on prev_pc, and
currently_stepping (ecs), which are per-thread, so to make the
new_thread_event handling correct in non-stop, would require
moving this bit:

  /* If it's a new process, add it to the thread database */

  ecs->new_thread_event = (!ptid_equal (ecs->ptid, inferior_ptid)
			   && !ptid_equal (ecs->ptid, minus_one_ptid)
			   && !in_thread_list (ecs->ptid));

  if (ecs->ws.kind != TARGET_WAITKIND_EXITED
      && ecs->ws.kind != TARGET_WAITKIND_SIGNALLED && ecs->new_thread_event)
    add_thread (ecs->ptid);

To the top of handle_inferior_event, and context switch right after it,
if non-stop, instead of context-switching in fetch_inferior_event.

Something like:

void
handle_inferior_event (struct execution_control_state *ecs)
{
  int sw_single_step_trap_p = 0;
  int stopped_by_watchpoint;
  int stepped_after_stopped_by_watchpoint = 0;

  /* If it's a new process, add it to the thread database */

  ecs->new_thread_event = (!ptid_equal (ecs->ptid, inferior_ptid)
			   && !ptid_equal (ecs->ptid, minus_one_ptid)
			   && !in_thread_list (ecs->ptid));

  if (ecs->ws.kind != TARGET_WAITKIND_EXITED
      && ecs->ws.kind != TARGET_WAITKIND_SIGNALLED && ecs->new_thread_event)
    add_thread (ecs->ptid);

  if (non_stop
      && ecs->ws.kind != TARGET_WAITKIND_IGNORE
      && ecs->ws.kind != TARGET_WAITKIND_EXITED
      && ecs->ws.kind != TARGET_WAITKIND_SIGNALLED)
    /* In non-stop mode, each thread is handled individually.  Switch
       early, so the global state is set correctly for this
       thread.  */
    context_switch (ecs->ptid);

  /* Cache the last pid/waitstatus. */
  target_last_wait_ptid = ecs->ptid;
  target_last_waitstatus = ecs->ws;

  /* Always clear state belonging to the previous time we stopped.  */
  stop_stack_dummy = 0;

  adjust_pc_after_break (ecs);
...


Since this is a new mode that will require adaptation
by each target, it sounded reasonable to require that the
the new_thread_event should never be hit, and that the
target handles adding threads to GDB's thread list.

Second is that what follows that code you pointed is
target_resume (RESUME_ALL), which should never happen
in non-stop mode:

  /* At this point, all threads are stopped (happens automatically in
     either the OS or the native code).  Therefore we need to continue
     all threads in order to make progress.  */
  if (ecs->new_thread_event)
    {
      target_resume (RESUME_ALL, 0, TARGET_SIGNAL_0);
      prepare_to_wait (ecs);
      return;
    }

I can certainly change that to:

  if (ecs->new_thread_event)
    {
      /* We may want to consider not doing a resume here in order to
	 give the user a chance to play with the new thread.  It might
	 be good to make that a user-settable option.  */

      if (non_stop)
	/* Only resume the event thread.  */
	target_resume (ecs->ptid, 0, TARGET_SIGNAL_0);
      else
	/* At this point, all threads are stopped (happens
	   automatically in either the OS or the native code).
	   Therefore we need to continue all threads in order to make
	   progress.  */
	target_resume (RESUME_ALL, 0, TARGET_SIGNAL_0);
      prepare_to_wait (ecs);
      return;
    }

But should I ?

-- 
Pedro Alves