Merge of nickrob-async-20060513 to mainline?

Merge of nickrob-async-20060513 to mainline?

[Nick Roberts]

I have recently updated/merged the asynchronous branch (nickrob-async-20060513)
that I am working on.  The overall task is too great for me to complete alone
and it has attracted no interest as a branch.  For these reasons (and the
coming release of Emacs 22) I would like to merge it with mainline GDB.  I have
tried to arrange that GDB on my branch will only build with the "--async"
option for GNU/Linux on i386.  This should mean that it should build and run as
before on any other platform, and that itwill only run differently on
i386-linux-gnu if the "--async" option is specified.

If it passes the current testsuite and I add some new testcases for the
asynchronous operation (probably in the MI part of the testsuite), may I make
such a merge?

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: Merge of nickrob-async-20060513 to mainline?

[Daniel Jacobowitz]

On Wed, Aug 30, 2006 at 02:25:01PM +1200, Nick Roberts wrote:
> 
> I have recently updated/merged the asynchronous branch (nickrob-async-20060513)
> that I am working on.  The overall task is too great for me to complete alone
> and it has attracted no interest as a branch.  For these reasons (and the
> coming release of Emacs 22) I would like to merge it with mainline GDB.  I have
> tried to arrange that GDB on my branch will only build with the "--async"
> option for GNU/Linux on i386.  This should mean that it should build and run as
> before on any other platform, and that itwill only run differently on
> i386-linux-gnu if the "--async" option is specified.
> 
> If it passes the current testsuite and I add some new testcases for the
> asynchronous operation (probably in the MI part of the testsuite), may I make
> such a merge?

Could you remind me what this has to do with the release of emacs 22?

I'm not sure what interest you expected to generate on a branch, which
you didn't discuss on any of the mailing lists; as a rule, no one has
time to check out and poke at projects that no one tells them about. 
The last time I heard status from you on the branch, it wasn't working.
I gather it's better off now.  That makes it much more interesting.

I think that merging it in without review would be at the least unwise.
How big are the changes relative to mainline (diffstat)?  Is there an
angle from which we can start looking at them?

-- 
Daniel Jacobowitz
CodeSourcery

Re: Merge of nickrob-async-20060513 to mainline?

[Nick Roberts]

Daniel Jacobowitz writes:
 > On Wed, Aug 30, 2006 at 02:25:01PM +1200, Nick Roberts wrote:
 > > 
 > > I have recently updated/merged the asynchronous branch (nickrob-async-20060513)
 > > that I am working on.  The overall task is too great for me to complete alone
 > > and it has attracted no interest as a branch.  For these reasons (and the
 > > coming release of Emacs 22) I would like to merge it with mainline GDB.  I have
 > > tried to arrange that GDB on my branch will only build with the "--async"
 > > option for GNU/Linux on i386.  This should mean that it should build and run as
 > > before on any other platform, and that itwill only run differently on
 > > i386-linux-gnu if the "--async" option is specified.
 > > 
 > > If it passes the current testsuite and I add some new testcases for the
 > > asynchronous operation (probably in the MI part of the testsuite), may I make
 > > such a merge?
 > 
 > Could you remind me what this has to do with the release of emacs 22?

In Emacs 22, interaction with gdb is done primarily with annotations (although
MI is accessed through "interprter-exec mi" for some features such as
variable objects.  After Emacs 22, the plan is to switch to MI but this
really requires asynchronous behaviour.

 > I'm not sure what interest you expected to generate on a branch, which
 > you didn't discuss on any of the mailing lists; as a rule, no one has
 > time to check out and poke at projects that no one tells them about.

I didn't really expect to generate much interest.  I thought I had discussed
it, although I haven't evangelised as it doesn't seem appropriate.
 
 > The last time I heard status from you on the branch, it wasn't working.
 > I gather it's better off now.  That makes it much more interesting.

I hadn't committed all the changes I had made until recently, so although
it wouldn't have worked, I had thought it would (at least in a limited way).

 > I think that merging it in without review would be at the least unwise.

If people are willing to review it then that is clearly preferable.  If
not, then after passing the testuite, putting it in the repository would
seem to be a good way to test it.  If it caused too much disruption then
it could be reverted fairy easily and there is plenty of time before the
next release.

 > How big are the changes relative to mainline (diffstat)?

A few extra files and a sprinkling of extra lines/if clauses (to test
if asynchronous) in twenty or so files.  I could run diffstat but I'm
not familiar with it.

 >                                                            Is there an
 > angle from which we can start looking at them?

Well, it should be invisible for anything but i386-linux-gnu and work as before
on i386-linux-gnu unless "--async" is specified.  To that extent it shouldn't
interfere with anyone not interested in using it.  However, I may used the
wrong files to try to achieve this.  There is also a file called README.async
in the branch.

Also it still uses pthreads.  You said previously that it should be done
in one process and, following a remark from Jim Ingham, that ptrace is
non-blocking.  AFAICS the problem is that wait *is* blocking and I can't
see of a way to deal with that without using another thread.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: Merge of nickrob-async-20060513 to mainline?

[Daniel Jacobowitz]

On Wed, Aug 30, 2006 at 03:18:58PM +1200, Nick Roberts wrote:
> If people are willing to review it then that is clearly preferable.  If
> not, then after passing the testuite, putting it in the repository would
> seem to be a good way to test it.  If it caused too much disruption then
> it could be reverted fairy easily and there is plenty of time before the
> next release.

I'll wait to see what other developers have to say, but I think we
should at least make an effort to review it.

>  > How big are the changes relative to mainline (diffstat)?
> 
> A few extra files and a sprinkling of extra lines/if clauses (to test
> if asynchronous) in twenty or so files.  I could run diffstat but I'm
> not familiar with it.

Just generate the diff between your mergepoint and branch, and then
pipe it to diffstat, assuming you've got it installed.

> Well, it should be invisible for anything but i386-linux-gnu and work as before
> on i386-linux-gnu unless "--async" is specified.  To that extent it shouldn't
> interfere with anyone not interested in using it.  However, I may used the
> wrong files to try to achieve this.  There is also a file called README.async
> in the branch.

Well, I meant from an internals perspective - like, a patch just
summarizing the interface changes, if the whole patch is too big to
post at once.

> Also it still uses pthreads.  You said previously that it should be done
> in one process and, following a remark from Jim Ingham, that ptrace is
> non-blocking.  AFAICS the problem is that wait *is* blocking and I can't
> see of a way to deal with that without using another thread.

Ah, but wait is non-blocking.  You have to (A) use WNOHANG, and (B) be
careful to handle asynchronous SIGCHLD events.  It's not especially
difficult, I don't think, but the async event loop in gdb has always
confused me; we'll have to figure out how to gather up sigchld/wait
events.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Merge of nickrob-async-20060513 to mainline?

[Eli Zaretskii]

> Date: Wed, 30 Aug 2006 00:01:13 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: gdb@sources.redhat.com
> 
> On Wed, Aug 30, 2006 at 03:18:58PM +1200, Nick Roberts wrote:
> > If people are willing to review it then that is clearly preferable.  If
> > not, then after passing the testuite, putting it in the repository would
> > seem to be a good way to test it.  If it caused too much disruption then
> > it could be reverted fairy easily and there is plenty of time before the
> > next release.
> 
> I'll wait to see what other developers have to say, but I think we
> should at least make an effort to review it.

I definitely agree that we should review the patch before committing
it.  That is what we do with any other patch.  Saying it passed the
test suite is not enough, since no one can prove that the test suite
has sufficiently complete coverage to give such trust to it.  In fact,
we _know_ that the test suite cannot be trusted to be that good,
because we continue adding tests to it all the time.

Re: Merge of nickrob-async-20060513 to mainline?

[Nick Roberts]

 > Just generate the diff between your mergepoint and branch, and then
 > pipe it to diffstat, assuming you've got it installed.

Doing:

cvs diff -r nickrob-async-20060828-mergepoint gdb | diffstat

from the src directory gives the output below.

 > Well, I meant from an internals perspective - like, a patch just
 > summarizing the interface changes, if the whole patch is too big to
 > post at once.

Instead of GDB waiting for the enferior to stop directly, it creates a signal
thread to do that.  This means that the main thread can continue to process the
event loop.  When the inferior does stop, the signal thread writes to a file
which GDB detects through handle_file_event in the event loop. It then calls
inferior_event_handler.

 > > Also it still uses pthreads.  You said previously that it should be done
 > > in one process and, following a remark from Jim Ingham, that ptrace is
 > > non-blocking.  AFAICS the problem is that wait *is* blocking and I can't
 > > see of a way to deal with that without using another thread.
 > 
 > Ah, but wait is non-blocking.  You have to (A) use WNOHANG, and (B) be
 > careful to handle asynchronous SIGCHLD events.  It's not especially
 > difficult, I don't think, but the async event loop in gdb has always
 > confused me; we'll have to figure out how to gather up sigchld/wait
 > events.

The manpage just says:

       WNOHANG
              return immediately if no child has exited.

but what value does it return with?  Does GDB ignore SIGCHLD events so it
can detect state changes through wait?

-- 
Nick                                           http://www.inet.net.nz/~nickrob

New files:

gdb/PROBLEMS.async
gdb/README.async
gdb/TODO.async
gdb/async-nat-inferior.c
gdb/async-nat-inferior.h
gdb/async-nat-sigthread.c
gdb/async-nat-sigthread.h

Makefile.in                |   13 
 gdb-mi.el                  | 2129 ++++++++++++++++++++++++--
 gdb/ChangeLog              | 3645 +--------------------------------------------
 gdb/cli-out.c              |   12 
 gdb/cli-out.h              |    1 
 gdb/cli/cli-interp.c       |   83 -
 gdb/config/i386/nm-linux.h |    3 
 gdb/defs.h                 |   11 
 gdb/event-loop.c           |   72 
 gdb/event-loop.h           |    9 
 gdb/event-top.c            |   93 +
 gdb/event-top.h            |    2 
 gdb/exec.c                 |   52 
 gdb/i386-linux-nat.c       |   21 
 gdb/inf-child.c            |    1 
 gdb/inf-loop.c             |   46 
 gdb/inf-ptrace.c           |   38 
 gdb/infcmd.c               |    9 
 gdb/infrun.c               |   26 
 gdb/interps.c              |   40 
 gdb/interps.h              |    3 
 gdb/linux-nat.c            |  747 ++++-----
 gdb/linux-thread-db.c      |    4 
 gdb/main.c                 |   13 
 gdb/mi/mi-interp.c         |   82 -
 gdb/mi/mi-main.c           |  371 ++++
 gdb/mi/mi-main.h           |   11 
 gdb/remote.c               |   12 
 gdb/target.c               |   31 
 gdb/target.h               |   16 
 gdb/top.c                  |   25 
 gdb/tui/tui-hooks.c        |    4 
 gdb/version.in             |    2 
 gdb/wrapper.h              |    2 
 linux.mh                   |    7 
 35 files changed, 3494 insertions(+), 4142 deletions(-)

Re: Merge of nickrob-async-20060513 to mainline?

[Daniel Jacobowitz]

On Thu, Aug 31, 2006 at 09:32:18AM +1200, Nick Roberts wrote:
>  > Just generate the diff between your mergepoint and branch, and then
>  > pipe it to diffstat, assuming you've got it installed.
> 
> Doing:
> 
> cvs diff -r nickrob-async-20060828-mergepoint gdb | diffstat
> 
> from the src directory gives the output below.

Not too bad.

>  > Well, I meant from an internals perspective - like, a patch just
>  > summarizing the interface changes, if the whole patch is too big to
>  > post at once.
> 
> Instead of GDB waiting for the enferior to stop directly, it creates a signal
> thread to do that.  This means that the main thread can continue to process the
> event loop.  When the inferior does stop, the signal thread writes to a file
> which GDB detects through handle_file_event in the event loop. It then calls
> inferior_event_handler.

OK, this is easily mimicable without threads, at least on GNU/Linux.

On Cygwin/Windows I imagine we'd want to use threads, but the tradeoffs
there are somewhat different :-)

>  > > Also it still uses pthreads.  You said previously that it should be done
>  > > in one process and, following a remark from Jim Ingham, that ptrace is
>  > > non-blocking.  AFAICS the problem is that wait *is* blocking and I can't
>  > > see of a way to deal with that without using another thread.
>  > 
>  > Ah, but wait is non-blocking.  You have to (A) use WNOHANG, and (B) be
>  > careful to handle asynchronous SIGCHLD events.  It's not especially
>  > difficult, I don't think, but the async event loop in gdb has always
>  > confused me; we'll have to figure out how to gather up sigchld/wait
>  > events.
> 
> The manpage just says:
> 
>        WNOHANG
>               return immediately if no child has exited.
> 
> but what value does it return with?  Does GDB ignore SIGCHLD events so it
> can detect state changes through wait?

If that's all it says I recommend a better man page :-)

       waitpid(): on success, returns the process ID of the child whose
       state has changed; on error, -1 is returned; if WNOHANG was
       specified and no child(ren) specified by pid has yet changed
       state, then 0 is returned.

Different ports of GDB treat SIGCHLD differently.  linux-nat.c, for
instance, blocks it so that it can use sigsuspend.  Instead, we would
have to register handlers for sigchld events somehow (probably using
the same infrastructure used for SIGINT to quit operations).

I can work on this, though not right away.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Merge of nickrob-async-20060513 to mainline?

[Nick Roberts]

 > > Instead of GDB waiting for the enferior to stop directly, it creates a
 > > signal thread to do that.  This means that the main thread can continue to
 > > process the event loop.  When the inferior does stop, the signal thread
 > > writes to a file which GDB detects through handle_file_event in the event
 > > loop. It then calls inferior_event_handler.
 > 
 > OK, this is easily mimicable without threads, at least on GNU/Linux.

Easy being in the eye of the beholder!

 > On Cygwin/Windows I imagine we'd want to use threads, but the tradeoffs
 > there are somewhat different :-)

As there are platform specific changes e.g to linux_nat_attach, linux_nat_wait,
i386_linux_resume etc, I guess they need to be added on a case by case basis.
Presumably this could be done incrementally i.e we could merge asynchronous
operation on GNU/Linux into GDB without waiting for it to work on
Cygwin/Windows.

 > > The manpage just says:
 > > 
 > >        WNOHANG
 > >               return immediately if no child has exited.
 > > 
 > > but what value does it return with?  Does GDB ignore SIGCHLD events so it
 > > can detect state changes through wait?
 > 
 > If that's all it says I recommend a better man page :-)
 > 
 >        waitpid(): on success, returns the process ID of the child whose
 >        state has changed; on error, -1 is returned; if WNOHANG was
 >        specified and no child(ren) specified by pid has yet changed
 >        state, then 0 is returned.

Doh!

 > Different ports of GDB treat SIGCHLD differently.  linux-nat.c, for
 > instance, blocks it so that it can use sigsuspend.  Instead, we would
 > have to register handlers for sigchld events somehow (probably using
 > the same infrastructure used for SIGINT to quit operations).
 > 
 > I can work on this, though not right away.

Thanks.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: Merge of nickrob-async-20060513 to mainline?

[Nick Roberts]

 > > Instead of GDB waiting for the enferior to stop directly, it creates a
 > > signal thread to do that.  This means that the main thread can continue to
 > > process the event loop.  When the inferior does stop, the signal thread
 > > writes to a file which GDB detects through handle_file_event in the event
 > > loop. It then calls inferior_event_handler.
 > 
 > OK, this is easily mimicable without threads, at least on GNU/Linux.

I'm starting to do this now.  I still use a pipe to write to a file descriptor
but instead of a separate thread I use a while loop in gdb_wait_for_event.
I've only done it for select because that's all I know but I guess a similar
thing could be done for poll.  It seems to work reasonably well for single
threaded inferiors but get's stuck again with multi-threaded ones.  The
multi-threaded code in linux-nat.c just seems so convoluted (maybe threads
are more easily handled with a Mach kernel).

Below is a (pseudo) diff fragment that should (hopefully) give the general
idea.  Do you mean something along these lines?  If yes, I'll try to get it
to work more generally.

-- 
Nick                                           http://www.inet.net.nz/~nickrob


*** linux-nat.c	18 Sep 2006 16:44:40 +1200	1.50

+ static int gdb_fetch_event (int* ptr_status)
+ {
+   struct timeval tv;
+   fd_set fds;
+   int msg[2], ret;
+ 
+   FD_ZERO (&fds);
+   FD_SET (gdb_status[0], &fds);
+   tv.tv_sec = 0;
+   tv.tv_usec = 0;
+ 
+   msg[0] = 0;
+   ret = select (FD_SETSIZE, &fds, NULL, NULL, NULL);
+   if (ret)
+     {
+       read (gdb_status[0], msg, 2*sizeof (int));
+       *ptr_status = msg[1];
+     }
+   
+   return msg[0];
+ }
+ 
  static ptid_t
  linux_nat_wait (ptid_t ptid, struct target_waitstatus *ourstatus)
  {
***************
*** 1956,1962 **** (linux_nat_wait)
      {
        pid_t lwpid;
  
!       lwpid = my_waitpid (pid, &status, options);
        if (lwpid > 0)
  	{
  	  gdb_assert (pid == -1 || lwpid == pid);
--- 2094,2106 ----
      {
        pid_t lwpid;
  
!       if (target_can_async_p () && gdb_post_startup)
! 	{
! 	  gdb_file_event = 0;
! 	  lwpid = gdb_fetch_event (&status);
! 	}
!       else
! 	lwpid = my_waitpid (pid, &status, options);
        if (lwpid > 0)
  	{
  	  gdb_assert (pid == -1 || lwpid == pid);


*** event-loop.c (gdb_wait_for_event) 11 Feb 2006 11:01:43 +1300	1.29

        gdb_notifier.ready_masks[0] = gdb_notifier.check_masks[0];
        gdb_notifier.ready_masks[1] = gdb_notifier.check_masks[1];
        gdb_notifier.ready_masks[2] = gdb_notifier.check_masks[2];
!       num_found = gdb_select (gdb_notifier.num_fds,
! 			      &gdb_notifier.ready_masks[0],
! 			      &gdb_notifier.ready_masks[1],
! 			      &gdb_notifier.ready_masks[2],
! 			      gdb_notifier.timeout_valid
! 			      ? &gdb_notifier.select_timeout : NULL);
  
        /* Clear the masks after an error from select. */
        if (num_found == -1)
--- 765,806 ----
        gdb_notifier.ready_masks[0] = gdb_notifier.check_masks[0];
        gdb_notifier.ready_masks[1] = gdb_notifier.check_masks[1];
        gdb_notifier.ready_masks[2] = gdb_notifier.check_masks[2];
! 
!       if (event_loop_p)
! 	{
! 	  struct timeval tv;
! 
! 	  while (num_found == 0)
! 	    {
! 	      msg[0] = waitpid (-1, ptr_status, WNOHANG);
!       
! 	      if (msg[0] > 0)
! 		{
! 		  msg[1] = *ptr_status;
! 		  write (gdb_status[1], msg, 2*sizeof (int));
! 		  gdb_file_event = 1;
! 		}
! 
! 	      tv.tv_sec = 0;
! 	      tv.tv_usec = 1000;
! 
! 	      gdb_notifier.ready_masks[0] = gdb_notifier.check_masks[0];
! 	      gdb_notifier.ready_masks[1] = gdb_notifier.check_masks[1];
! 	      gdb_notifier.ready_masks[2] = gdb_notifier.check_masks[2];
! 
! 	      num_found = gdb_select (gdb_notifier.num_fds,
! 				      &gdb_notifier.ready_masks[0],
! 				      &gdb_notifier.ready_masks[1],
! 				      &gdb_notifier.ready_masks[2],
! 				      &tv);
! 	    }
! 	} else
! 	num_found = gdb_select (gdb_notifier.num_fds,
! 				&gdb_notifier.ready_masks[0],
! 				&gdb_notifier.ready_masks[1],
! 				&gdb_notifier.ready_masks[2],
! 				gdb_notifier.timeout_valid
! 				? &gdb_notifier.select_timeout : NULL);

Re: Merge of nickrob-async-20060513 to mainline?

[Daniel Jacobowitz]

On Tue, Sep 26, 2006 at 08:38:23PM +1200, Nick Roberts wrote:
> Below is a (pseudo) diff fragment that should (hopefully) give the general
> idea.  Do you mean something along these lines?  If yes, I'll try to get it
> to work more generally.

Yes, this should work, though it seems cumbersome.  When you've got
a single thread, you don't need to pass actual data around an fd,
just a marker "yes i'm ready now".

Moving the call to waitpid out of linux-nat.c, and to a target
independent file, is a mistake - presumably that's just how you got it
to work quickly?  That's related to why it doesn't work for threads.
The vital line is "options ^= __WCLONE" in the loop in linux_nat_wait.
Without __WCLONE, you'll never see a wait status from a thread; with
it you'll never see a wait status from the main program.

Ideally you'd be able to reuse the signal handler logic (see the calls
to sigprocmask and sigsuspend) and thus not have a millisecond latency
and excessive spinning.  That's actually a pretty important feature,
because context switching to gdb every millisecond or so is going to
really hurt performance of the debuggee.


-- 
Daniel Jacobowitz
CodeSourcery

Re: Merge of nickrob-async-20060513 to mainline?

[Nick Roberts]

 > Yes, this should work, though it seems cumbersome.  When you've got
 > a single thread, you don't need to pass actual data around an fd,
 > just a marker "yes i'm ready now".

I don't quite follow, that's how the event loop works.  Even user input
requires an fd through stdin_event_handler.  There is a provision for other
events but currently only file events are used.

 > Moving the call to waitpid out of linux-nat.c, and to a target
 > independent file, is a mistake - presumably that's just how you got it
 > to work quickly?  That's related to why it doesn't work for threads.
 > The vital line is "options ^= __WCLONE" in the loop in linux_nat_wait.
 > Without __WCLONE, you'll never see a wait status from a thread; with
 > it you'll never see a wait status from the main program.

Perhaps I can use __WALL as a catch all.

 > Ideally you'd be able to reuse the signal handler logic (see the calls
 > to sigprocmask and sigsuspend) and thus not have a millisecond latency
 > and excessive spinning.  That's actually a pretty important feature,
 > because context switching to gdb every millisecond or so is going to
 > really hurt performance of the debuggee.

OK, a bit more homework then.

Thanks

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: Merge of nickrob-async-20060513 to mainline?

[Daniel Jacobowitz]

On Wed, Sep 27, 2006 at 10:09:38AM +1200, Nick Roberts wrote:
>  > Yes, this should work, though it seems cumbersome.  When you've got
>  > a single thread, you don't need to pass actual data around an fd,
>  > just a marker "yes i'm ready now".
> 
> I don't quite follow, that's how the event loop works.  Even user input
> requires an fd through stdin_event_handler.  There is a provision for other
> events but currently only file events are used.

Right - but you don't need to put the actual data (e.g. the PID and
status) through the fd.  I guess it doesn't hurt, but it does make
things harder to debug.  Instead, you can save them in memory, and
just push a byte through the fd - that will make it show up in select.

>  > Moving the call to waitpid out of linux-nat.c, and to a target
>  > independent file, is a mistake - presumably that's just how you got it
>  > to work quickly?  That's related to why it doesn't work for threads.
>  > The vital line is "options ^= __WCLONE" in the loop in linux_nat_wait.
>  > Without __WCLONE, you'll never see a wait status from a thread; with
>  > it you'll never see a wait status from the main program.
> 
> Perhaps I can use __WALL as a catch all.

There's supposedly some supported kernel versions left where that
didn't work.  Why can't you use the same code the existing support
uses?

I'm sorry I haven't been able to pitch in and help with this.  I
checked it out last week and started looking at it, but I'm buried
in work past my eyeballs right now.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Merge of nickrob-async-20060513 to mainline?

[Nick Roberts]

 > Right - but you don't need to put the actual data (e.g. the PID and
 > status) through the fd.  I guess it doesn't hurt, but it does make
 > things harder to debug.  Instead, you can save them in memory, and
 > just push a byte through the fd - that will make it show up in select.

OK, if that's simpler.

 > >  > Moving the call to waitpid out of linux-nat.c, and to a target
 > >  > independent file, is a mistake - presumably that's just how you got it
 > >  > to work quickly?  That's related to why it doesn't work for threads.
 > >  > The vital line is "options ^= __WCLONE" in the loop in linux_nat_wait.
 > >  > Without __WCLONE, you'll never see a wait status from a thread; with
 > >  > it you'll never see a wait status from the main program.
 > > 
 > > Perhaps I can use __WALL as a catch all.
 > 
 > There's supposedly some supported kernel versions left where that
 > didn't work.  Why can't you use the same code the existing support
 > uses?

AFAICS the problem is one of timing.  GDB uses the status information in
linux_nat_wait but must detect that the status has changed in the event loop
(gdb_wait_for_event).  However linux_nat_wait makes many calls to waitpid (at
least in the multi-threaded case).  I am/was hoping that, for asynchronous
operation, I only need to replace the first call to waitpid with another one,
which I've called gdb_fetch_event, to read the status information which has
already been waited on.  That's probably a bit naive but I can't see how
I can dart back and forth between gdb_wait_for_event and a single call
to linux_nat_wait.

 > I'm sorry I haven't been able to pitch in and help with this.  I
 > checked it out last week and started looking at it, but I'm buried
 > in work past my eyeballs right now.

I'm just grateful for any help you can give me.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: Merge of nickrob-async-20060513 to mainline?

[Nick Roberts]

[-- Attachment #1: message body and .signature --]
[-- Type: text/plain, Size: 1409 bytes --]

 > > Perhaps I can use __WALL as a catch all.
 > 
 > There's supposedly some supported kernel versions left where that
 > didn't work.

OK, but this approach (using waitpid (-1, &status, WNOHANG | __WALL); in
gdb_wait_for_event) looks more promising than nickrob-async-20060513.  It
didn't work with pthreads because the thread is _always_ waiting and receives
status information that other parts of the code waits on. Multi-thread tests
now work. Ignoring sigstep.exp (which doesn't work on mainline for me now
anyway), I get:

		=== gdb Summary ===

# of expected passes		10707
# of unexpected failures	55
# of unexpected successes	1
# of expected failures		42
# of unknown successes		3
# of known failures		70
# of unresolved testcases	103
# of untested testcases		5
# of unsupported tests		4

with fails (where mainline doesn't) that include:

1) checkpoint.exp, mult-forks.exp which fail with 
   "ERROR: internal buffer is full." because the message
   "select: Bad file descriptor" is repeatedly generated.

2) A couple of fails in attach.exp, define.exp

3) Extra failures in staticthreads.exp

I attach the compressed diff (28 Sep 2006) below.  I put the line:

set GDBFLAGS "--async"

at the end of site.exp for the tests.  Asynchronous output from MI requires
new tests but I haven't included these in the diff.

-- 
Nick                                           http://www.inet.net.nz/~nickrob



[-- Attachment #2: Asynchronous GDB --]
[-- Type: application/octet-stream, Size: 43097 bytes --]

Re: Merge of nickrob-async-20060513 to mainline?

[Nick Roberts]

 > Ideally you'd be able to reuse the signal handler logic (see the calls
 > to sigprocmask and sigsuspend) and thus not have a millisecond latency
 > and excessive spinning.  That's actually a pretty important feature,
 > because context switching to gdb every millisecond or so is going to
 > really hurt performance of the debuggee.

From the event loop, GDB needs to detect that a process/thread has changed
status and check for user input.  If it does the first with sigsuspend, the
only way I can see how it can do the second is by using threads. This would
almost take us back to where we started.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: Merge of nickrob-async-20060513 to mainline?

[Daniel Jacobowitz]

On Fri, Oct 06, 2006 at 01:50:34PM +1300, Nick Roberts wrote:
>  > Ideally you'd be able to reuse the signal handler logic (see the calls
>  > to sigprocmask and sigsuspend) and thus not have a millisecond latency
>  > and excessive spinning.  That's actually a pretty important feature,
>  > because context switching to gdb every millisecond or so is going to
>  > really hurt performance of the debuggee.
> 
> From the event loop, GDB needs to detect that a process/thread has changed
> status and check for user input.  If it does the first with sigsuspend, the
> only way I can see how it can do the second is by using threads. This would
> almost take us back to where we started.

Correct.  You can't do it quite the same way.  The key is not
sigsuspend itself, but the use of signals, and blocking them when you
don't need them.

The easy way to combine the two would be with pselect, but it's
inadequately portable.  Here's a bit from the man page of that function
on my system, which explains a useful alternative:

       Since version 2.1, glibc has provided an emulation of pselect()
       that is implemented using sig- procmask(2) and select().  This
       implementation remains vulnerable to the very race condition
       that pselect() was designed to prevent.  On systems that lack
       pselect() reliable (and more portable) signal trapping can be
       achieved using the self-pipe trick (where a signal handler
       writes a byte to a pipe whose other end is monitored by select()
       in the main program.)

So when you want to wait, you make sure such a signal handler is
installed for SIGCHLD, unblock SIGCHLD, and call select on a set of fds
that includes the one written to by the signal handler.  If you get a
byte on that fd, you know there's a child ready to be waited for. When
you're done waiting, you re-block the signal.  It's possible to get
spurious wakeups this way (for instance if a signal is received between
the return of select and the re-blocking), but with a little care
everything works out OK.

This is actually pretty similar to the way you do it with threads.

Does that make sense?

-- 
Daniel Jacobowitz
CodeSourcery

Re: Merge of nickrob-async-20060513 to mainline?

[Nick Roberts]

 > So when you want to wait, you make sure such a signal handler is
 > installed for SIGCHLD, unblock SIGCHLD, and call select on a set of fds
 > that includes the one written to by the signal handler.  If you get a
 > byte on that fd, you know there's a child ready to be waited for. When
 > you're done waiting, you re-block the signal.  It's possible to get
 > spurious wakeups this way (for instance if a signal is received between
 > the return of select and the re-blocking), but with a little care
 > everything works out OK.
 > 
 > This is actually pretty similar to the way you do it with threads.
 > 
 > Does that make sense?

Yes I think so.  I don't quite follow the explanation of pselect in the manpage
but I think you're saying that the signal handler for SIGCHLD will run even
when GDB is already in select, and can get GDB's attention by writing to a
pipe with a file descriptor that select is watching.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: Merge of nickrob-async-20060513 to mainline?

[Daniel Jacobowitz]

On Fri, Oct 06, 2006 at 03:10:37PM +1300, Nick Roberts wrote:
> Yes I think so.  I don't quite follow the explanation of pselect in the manpage
> but I think you're saying that the signal handler for SIGCHLD will run even
> when GDB is already in select, and can get GDB's attention by writing to a
> pipe with a file descriptor that select is watching.

Exactly.  Actually, getting the signal may also knock GDB out of the
select syscall; that depends on whether the "SA_RESTART" flag is used.
But you want to rely on the pipe, as there are fewer corner cases.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Merge of nickrob-async-20060513 to mainline?

[Nick Roberts]

[-- Attachment #1: message body and .signature --]
[-- Type: text/plain, Size: 1807 bytes --]

 > > Yes I think so.  I don't quite follow the explanation of pselect in the
 > > manpage but I think you're saying that the signal handler for SIGCHLD will
 > > run even when GDB is already in select, and can get GDB's attention by
 > > writing to a pipe with a file descriptor that select is watching.
 > 
 > Exactly.  Actually, getting the signal may also knock GDB out of the
 > select syscall; that depends on whether the "SA_RESTART" flag is used.
 > But you want to rely on the pipe, as there are fewer corner cases.

I've tried to do this now. This approach has the added benefit that the tests
checkpoint.exp, multi-forks.exp work now.  I've also resolved fails in
attach.exp and jump.exp. 

Ignoring sigstep.exp again, I now get:

		=== gdb Summary ===

# of expected passes		10789
# of unexpected failures	58
# of unexpected successes	1
# of expected failures		42
# of unknown successes		3
# of known failures		70
# of unresolved testcases	2
# of untested testcases		5
# of unsupported tests		4
/home/nickrob/src4/gdb/testsuite/../../gdb/gdb version  6.5.50.20061006-cvs --async

Fails still exist for server-run.exp, print-threads.exp and staticthreads.exp.

define.exp fails with the script nextwhere.  Asynchronous operation means that
it tries to do `where' before 'next' has finished.  Perhaps scripts should be
forced to run synchronously.

It's almost there now although I still need to tidy it up e.g move waitpid out
of event-loop.c and put declarations in header files rather than use externs.
I would like to commit these changes shortly after the next release so that
there is a full six months to iron out any problems they might cause.

I attach the compressed diff below for general inspection.

-- 
Nick                                           http://www.inet.net.nz/~nickrob



[-- Attachment #2: Asynchronous operation using signal handler --]
[-- Type: application/octet-stream, Size: 43519 bytes --]

async implies sync, was Re: Merge of nickrob-async-20060513 to mainline?

[Michael Snyder]

On Sun, 2006-10-08 at 16:43 +1300, Nick Roberts wrote:

> define.exp fails with the script nextwhere.  Asynchronous operation means that
> it tries to do `where' before 'next' has finished.  Perhaps scripts should be
> forced to run synchronously.

Hmm, boy, that's a good point.  In fact, you know what?
I think we need some sort of synchronization primatives, 
since the target and gdb can now run in parallel.

Every time we do a step/next/continue/until, and come
back to the gdb prompt before the target has stopped, 
we've effectively done a "fork".  At an absolute minimum, 
I think we need a "join" or a "wait".

Re: async implies sync

[Nick Roberts]

 > > define.exp fails with the script nextwhere.  Asynchronous operation means
 > > that it tries to do `where' before 'next' has finished.  Perhaps scripts
 > > should be forced to run synchronously.
 > 
 > Hmm, boy, that's a good point.  In fact, you know what?
 > I think we need some sort of synchronization primatives, 
 > since the target and gdb can now run in parallel.
 > 
 > Every time we do a step/next/continue/until, and come
 > back to the gdb prompt before the target has stopped, 
 > we've effectively done a "fork".  At an absolute minimum, 
 > I think we need a "join" or a "wait".

Some commands can sensibly be executed while the target is running e.g pwd,
"show height", etc.  Currently GDB only allows a few CLI commands (see top.c).
Most CLI commands report:

  Cannot execute this command while the target is running.

GDB allows no MI commands apart from -exec-interrupt (see mi-main.c) and
reports:

  Cannot execute command interpreter-exec while target running

At some stage more commands should be allowed but the above messages make
sense in interactive use.

Currently GDB reports when execution has stopped by outputting "*stopped,..".
Isn't this equivalent to your "join"?

Perhaps Apple can say what they do for scripts.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: Merge of nickrob-async-20060513 to mainline?

[Mark Kettenis]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Wed, 30 Aug 2006 15:18:58 +1200
> 
> Also it still uses pthreads.  You said previously that it should be done
> in one process and, following a remark from Jim Ingham, that ptrace is
> non-blocking.  AFAICS the problem is that wait *is* blocking and I can't
> see of a way to deal with that without using another thread.

I've stated before that I think that making gdb multi-threaded is a
seriously bad idea.

Mark

Re: Merge of nickrob-async-20060513 to mainline?

[Nick Roberts]

 > > Also it still uses pthreads.  You said previously that it should be done
 > > in one process and, following a remark from Jim Ingham, that ptrace is
 > > non-blocking.  AFAICS the problem is that wait *is* blocking and I can't
 > > see of a way to deal with that without using another thread.
 > 
 > I've stated before that I think that making gdb multi-threaded is a
 > seriously bad idea.

I don't know enough about threads to argue a case for them, but I'd like to
note that:

1) Daniel thinks that he can get the same functionality (on GNU/Linux at least)
   without using them.

2) Apple's GDB is multi-threaded and this doesn't seem to cause them problems.

3) It would only be multi-threaded, at most, for the specific platforms that
   asynchronous behaviour was implemented and only when invoked that way.  It
   could even be added as a compile option.

4) GDB appears to already to be a threaded application (at least, for me, it
   uses the libthread_db library).


-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: Merge of nickrob-async-20060513 to mainline?

[Daniel Jacobowitz]

On Fri, Sep 01, 2006 at 09:47:09AM +1200, Nick Roberts wrote:
> 2) Apple's GDB is multi-threaded and this doesn't seem to cause them problems.

That's their choice.

> 4) GDB appears to already to be a threaded application (at least, for me, it
>    uses the libthread_db library).

That's for debugging threads and has nothing to do with using them
itself.  As I've said in the past, I totally agree with Mark on this
point.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Merge of nickrob-async-20060513 to mainline?

[Nick Roberts]

 > > 2) Apple's GDB is multi-threaded and this doesn't seem to cause them
 > >    problems.
 > 
 > That's their choice.

It's their choice to make GDB multi-threaded.  I'm just saying it's an
example of where it seems to work in practice.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: Merge of nickrob-async-20060513 to mainline?

[Michael Snyder]

On Fri, 2006-09-01 at 10:38 +1200, Nick Roberts wrote:
>  > > 2) Apple's GDB is multi-threaded and this doesn't seem to cause them
>  > >    problems.
>  > 
>  > That's their choice.
> 
> It's their choice to make GDB multi-threaded.  I'm just saying it's an
> example of where it seems to work in practice.

Nick, what you're up against is that this is an old discussion.
We've tossed around the idea of making gdb a multi-threaded app
for years, and for various reasons, we've decided against it.
Therefore there is some inertia involved here, and you're not 
gonna sell the idea casually.  

I suspect that some of us are reluctant to jump into the
discussion anew, with someone who doesn't have all of the 
history and context.

Re: Merge of nickrob-async-20060513 to mainline?

[Nick Roberts]

 > > It's their choice to make GDB multi-threaded.  I'm just saying it's an
 > > example of where it seems to work in practice.
 > 
 > Nick, what you're up against is that this is an old discussion.
 > We've tossed around the idea of making gdb a multi-threaded app
 > for years, and for various reasons, we've decided against it.

If it's an old discussion, it was presumably before NPTL, when LinuxThreads
was used.

 > Therefore there is some inertia involved here, and you're not 
 > gonna sell the idea casually.  

I'm not trying to sell the idea of threads (I wouldn't know how to) but
the idea of an asynchronous event loop (which might involve the use of
a single extra thread).

 > I suspect that some of us are reluctant to jump into the
 > discussion anew, with someone who doesn't have all of the 
 > history and context.

Sure, I think the focus of my original message has somehow shifted anyway.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: Merge of nickrob-async-20060513 to mainline?

[Daniel Jacobowitz]

On Fri, Sep 01, 2006 at 11:31:14AM +1200, Nick Roberts wrote:
>  > > It's their choice to make GDB multi-threaded.  I'm just saying it's an
>  > > example of where it seems to work in practice.
>  > 
>  > Nick, what you're up against is that this is an old discussion.
>  > We've tossed around the idea of making gdb a multi-threaded app
>  > for years, and for various reasons, we've decided against it.
> 
> If it's an old discussion, it was presumably before NPTL, when LinuxThreads
> was used.

More likely it was before Linux was a relevant platform at all.  GDB
isn't all about GNU/Linux.

Anyway, I don't see the point in worrying about this.  We'll try to
come up with an interface that lets an extra thread, if necessary,
be contained to native files on platforms that really need it due to
quirks of their debug interface; I vaguely recall hearing that Darwin
was such a platform.

I don't think any of the other native debug interfaces I'm familiar
with qualify.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Merge of nickrob-async-20060513 to mainline?

[Jim Ingham]

Optionally adding another thread if a given platform needs it is  
actually pretty straight-forward to do as things stand.  The event  
stuff is designed to be able to accept an arbitrary number of event  
sources.  If you need an extra thread, you just hook up a pipe between  
it an the main thread, and add the pipe to the select sources.  The  
ordinary event stuff handles waking up and calling the handler for  
that source.

We do that for macosx - creating & hooking up the thread in  
create_inferior and tearing it down in mourn_inferior.  That part of  
it is fairly simple.

And yes, we really do have to have the extra thread.  The only way to  
get "mach exceptions" - which are the ur-signals on Mac OS X and which  
we need to capture - is a blocking API.  So if we want to run gdb  
asynchronously, we have to use an extra thread for the mach exceptions.

Jim

On Aug 31, 2006, at 4:37 PM, Daniel Jacobowitz wrote:

> On Fri, Sep 01, 2006 at 11:31:14AM +1200, Nick Roberts wrote:
>>>> It's their choice to make GDB multi-threaded.  I'm just saying  
>>>> it's an
>>>> example of where it seems to work in practice.
>>>
>>> Nick, what you're up against is that this is an old discussion.
>>> We've tossed around the idea of making gdb a multi-threaded app
>>> for years, and for various reasons, we've decided against it.
>>
>> If it's an old discussion, it was presumably before NPTL, when  
>> LinuxThreads
>> was used.
>
> More likely it was before Linux was a relevant platform at all.  GDB
> isn't all about GNU/Linux.
>
> Anyway, I don't see the point in worrying about this.  We'll try to
> come up with an interface that lets an extra thread, if necessary,
> be contained to native files on platforms that really need it due to
> quirks of their debug interface; I vaguely recall hearing that Darwin
> was such a platform.
>
> I don't think any of the other native debug interfaces I'm familiar
> with qualify.
>
> -- 
> Daniel Jacobowitz
> CodeSourcery