About handle_inferior_event new_thread_event

About handle_inferior_event new_thread_event

[Pedro Alves]

This bit of code in handle_inferior_event:

  /* 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->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.  */

      /* 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;
    }


seems to me that this is intended to have targets report new
threads to the core by reporting e.g., TARGET_WAITKIND_STOP
with any fake signal.  If the stop was due to a new thread event
in the target side (as oposed to a signal that should really cause
a stop), then the resume really lets the thread go free on the target
side.  If otherwise, the stop was due to a real signal (a SIGTRAP, a
SIGSEGV, etc.), then the resume causes the target to report the signal
again (that's what happens on linux, for example), and so, handle_inferior
event is again called to handle the same signal, only the second
time, the thread is already listed, so the event goes on to be
handled as usual.

I've always been curious as to which target relies on this, since
the remote target always adds threads to the thread list
before reporting events to the core (possibly due to the fact that
there are targets where resuming with TARGET_SIGNAL_0 when stopped
at a signal doesn't retrigger the pending signal).  Maybe this was
something that was intended to be documented?  Anyone knows the
history behind this?

-- 
Pedro Alves

Re: About handle_inferior_event new_thread_event

[Daniel Jacobowitz]

On Mon, Jun 29, 2009 at 12:29:12PM +0100, Pedro Alves wrote:
> seems to me that this is intended to have targets report new
> threads to the core by reporting e.g., TARGET_WAITKIND_STOP
> with any fake signal.  If the stop was due to a new thread event
> in the target side (as oposed to a signal that should really cause
> a stop), then the resume really lets the thread go free on the target
> side.  If otherwise, the stop was due to a real signal (a SIGTRAP, a
> SIGSEGV, etc.), then the resume causes the target to report the signal
> again (that's what happens on linux, for example), and so, handle_inferior
> event is again called to handle the same signal, only the second
> time, the thread is already listed, so the event goes on to be
> handled as usual.
> 
> I've always been curious as to which target relies on this, since
> the remote target always adds threads to the thread list
> before reporting events to the core (possibly due to the fact that
> there are targets where resuming with TARGET_SIGNAL_0 when stopped
> at a signal doesn't retrigger the pending signal).  Maybe this was
> something that was intended to be documented?  Anyone knows the
> history behind this?

I don't know - but I do know that this code has never worked reliably
for Linux; we used to get crashes or internal errors from unset
LWP-private state any time we went through here.  I'd rather all
targets were required to do thread accounting on their own.

-- 
Daniel Jacobowitz
CodeSourcery

Re: About handle_inferior_event new_thread_event

[Michael Snyder]

Pedro Alves wrote:
> This bit of code in handle_inferior_event:
> 
>   /* 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->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.  */
> 
>       /* 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;
>     }
> 
> 
> seems to me that this is intended to have targets report new
> threads to the core by reporting e.g., TARGET_WAITKIND_STOP
> with any fake signal.  If the stop was due to a new thread event
> in the target side (as oposed to a signal that should really cause
> a stop), then the resume really lets the thread go free on the target
> side.  If otherwise, the stop was due to a real signal (a SIGTRAP, a
> SIGSEGV, etc.), then the resume causes the target to report the signal
> again (that's what happens on linux, for example), and so, handle_inferior
> event is again called to handle the same signal, only the second
> time, the thread is already listed, so the event goes on to be
> handled as usual.
> 
> I've always been curious as to which target relies on this, since
> the remote target always adds threads to the thread list
> before reporting events to the core (possibly due to the fact that
> there are targets where resuming with TARGET_SIGNAL_0 when stopped
> at a signal doesn't retrigger the pending signal).  Maybe this was
> something that was intended to be documented?  Anyone knows the
> history behind this?

It's very old; it probably predates the code in remote.c that
you're referring to.  Can't really say which if any targets
still rely on it.

There's been a lot of evolution as far as thread list
accounting since that code was put in place.  In the
very early days, this was the only way we had of discovering
threads.  If a thread didn't get a stop event, gdb would
not know that thread existed.

Re: About handle_inferior_event new_thread_event

[Pedro Alves]

On Monday 29 June 2009 19:03:54, Michael Snyder wrote:
> There's been a lot of evolution as far as thread list
> accounting since that code was put in place.  In the
> very early days, this was the only way we had of discovering
> threads.  If a thread didn't get a stop event, gdb would
> not know that thread existed.

That's understandable, and still true on many targets (remote.c included,
but new threads found discovered by stop events are handled by
remote.c:remote_wait itself), but, you don't need to resume the
target immediately for that.  You just need to add the thread
to the list, and continue handling the event.  It's the resume-once
bit that I find surprising, and which makes me believe that
someone was using this in place of a ficticious
TARGET_WAITKIND_NEW_THREAD_EVENT (add new thread to list,
otherwise treat as TARGET_WAIKIND_SPURIOUS).

-- 
Pedro Alves

Re: About handle_inferior_event new_thread_event

[Pedro Alves]

On Monday 29 June 2009 18:56:28, Daniel Jacobowitz wrote:
> I don't know - but I do know that this code has never worked reliably
> for Linux; we used to get crashes or internal errors from unset
> LWP-private state any time we went through here.  I'd rather all
> targets were required to do thread accounting on their own.

Yeah, although I don't think those should be happening currently.
linux-nat.c just ignores events from lwps not in the lwp_list...  In non-stop
mode, it's already an internal error to reach this bit.  I do
notice that nto-procfs.c is one target that defers adding threads to the
list to handle_inferior_event ... but I honestly doubt the new thread
needs a first-chance resume.

-- 
Pedro Alves