Re: [non-stop] 10/10 split user/internal threads

[Pedro Alves <pedro at codesourcery dot com>]

A Tuesday 01 July 2008 01:51:28, Daniel Jacobowitz wrote:
> On Mon, Jun 30, 2008 at 01:08:48AM +0100, Pedro Alves wrote:
> > - In non-stop mode, the current thread may exit while the
> >   user has it selected.  Switching current thread, and restoring
> >   it with a cleanup is problematic in non-stop mode.
>
> The target interface is async, the inferior program is running - but
> GDB retains a single thread of control.  So unless the inferior runs
> while the cleanup is live, there's no problem here.  I suppose it
> could be trouble if you enter the expression evaluator, though?

Sure it can run while the cleanup is live.  It could already be
running when the cleanup was created.  Remember that the selected
thread may be running at any time.

If the selected thread was not running, but it is set
running while the cleanup is live, and we enter the expression
evaluator, I don't think there's a problem currently, as
returning from infcalls tries to restore the originally
selected frame.

The other case the current thread may be set running is:

01: foo3()
02: {
03: }
04: 
05: foo2()
06: {
07:   foo3();
08: }
09: 
10: foo1()
11: {
12:   foo2();
13: }
14:
15: foo_other_thread()
16: {
17: }

(gdb) info threads
* 1 Thread <foo> at foo2 line 7

(gdb) bt
#0  0xffffe410 in foo2 ()
#1  0xf7e4aff6 in foo1 ()
#2  0xf7e8634c in thread_entry ()

(gdb) up
#1  0xf7e4aff6 in foo1 ()

(gdb) break foo_other_thread
Breakpoint 1 ...
(gdb) commands
>thread 1
>step&
>end

<thread 2 hits break 1>
<steps thread 1>

The user loses the selected frame in thread 1, because when
the cleanup is ran, the thread is running/stepping.
Since the thread stepped into another frame, we could
claim that since the originally selected frame is still live,
it should still be selected.  But, it can also be
claimed that, though luck, the thread was set running,
you lose the selected frame.  This doesn't seem
like an important enough case to care about.  Agree?

> >    - GDB has finished handling the event, so should restore to the
> >      user selected thread and frame.  But, which is it now?
> >      thread 1, or thread 3?  In all-stop mode?  In non-stop mode?
>
> IMO it's thread 3 in all-stop and thread 1 in non-stop.  This is a
> weird user-visible artifact, though.
>

Ok, that's my opinion too.

> > With all the above in mind, I thought of:
> >
> > - calling the { selected thread and frame } thing a "context".
> > - having a context stack
> >   (The top level context being what the user/frontend
> >    considers current.)
> >
> > Instead of temporarilly switching to another thread and frame;
> > execute commands in it, and restoring the selected thread and
> > frame, we do:
> >
> > - push a new context on the context stack
> > - switch the selected thread and frame in this
> >   new now top level context
> > - execute command(s)
> >     which can switch thread and frame at will
> > - pop context
> >
> > - Whenever a thread exit is detected, we go through all
> > contexts and if it was the selected thread in it, invalidate
> > it.
>
> Isn't this exactly the same as using cleanups, except that there is a
> way to invalidate saved cleanups?
>

Yeah, conceptually different, but the end result is close.
In fact, ...

> If so, then it seems to me there is a simpler and less intrusive
> solution.  Put a reference count in the thread structure and increment
> it when you push a cleanup.  I think Vladimir added a hook that will
> let us drop the reference even if the cleanup is discarded.
>

... I like this.

There's one issue that I'd like to point out.  That is, is a
thread exits, and it was inferior_ptid, we can't just switch
inferior_ptid to null_ptid as I could with split user/internal
threads split.  A *lot* of things break.

The issue arises with the OS quickly reusing ptids:

  - inferior_ptid (ptid1) exits, we can't delete it from the
    thread list yet (things break, e.g. context switching..., 
    but many other things break in target code.)
  - We mark it with THREAD_EXITED, but leave it there.
  - target notifies new thread with ptid1.  There's already
    such a thread in the list, and it's also the
    current thread -- inferior_ptid.
  - To add this new thread to the list, we must get rid
    of the old exited thread.  Conceptually, this new thread
    although it has the target identifier, it should have a
    new GDB thread id.  So, we could say that in this case,
    the "non-stop doesn't change threads" premise breaks.  But,
    then again, the user couldn't do anything to the exited
    thread anyway, and it can only exit is it is running, in which
    case the user also couldn't do most things with it.  Maybe we
    can just live with this exception.

> I don't want to proliferate cleanup-like mechanisms if we can avoid
> it, the ones we have are confusing enough already :-)
>
> > > > +/* Only safe to use this cleanup on a stopped thread, and
> > > > +   inferior_ptid isn't ran before running the cleanup.  */
> > > > +
> > > >  struct current_thread_cleanup
> > >
> > > Not sure what you mean by this comment.
> >
> > I've changed the comment to this:
> >
> > /* It is only safe to use this cleanup iff inferior_ptid is alive and
> >    stopped, and, by if by the time it is ran, inferior_ptid represents
> >    the same thread, it is alive and it is stopped.  */
> >
> > Does it make it clearer?
>

> Does it in "by the time it is" refer to the cleanup?  And then the
> "it"s on the last line to "the same thread"?  Pronouns are so
> confusing.

Oh, sorry.  Yes, that's correct.

> I didn't look at the incremental patch, sorry - let's finish talking
> about the problem first.

No problem.  I like the refcounting idea.  Let me work on it, and
see if there's any other problem.

-- 
Pedro Alves