async patch (no. 4)

async patch (no. 4)

[Nick Roberts]

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


This is just my latest async patch so time that doesn't get wasted on my
previous one.

1) I've moved target specific stuff out of event-loop.c and into linux-nat.c
   and used hooks so e.g it should compile (but not work asynchronously) on
   Windows.
2) It now works with poll as well as select.
3) It's a lot smaller because I've left out changes to gdb-mi.el which
   probably aren't of general interest.
4) The test server-run.exp still fails.

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


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

Re: async patch (no. 4)

[Daniel Jacobowitz]

On Fri, Oct 20, 2006 at 08:02:35PM +1300, Nick Roberts wrote:
Content-Description: message body and .signature
> 
> This is just my latest async patch so time that doesn't get wasted on my
> previous one.
> 
> 1) I've moved target specific stuff out of event-loop.c and into linux-nat.c
>    and used hooks so e.g it should compile (but not work asynchronously) on
>    Windows.
> 2) It now works with poll as well as select.
> 3) It's a lot smaller because I've left out changes to gdb-mi.el which
>    probably aren't of general interest.
> 4) The test server-run.exp still fails.

I've started looking at this.  I suspect I've asked you this before; if
so, I apologize.  But could you give me the big picture on what is
supposed to be different with this patch applied - what's the goal, and
should it be the default eventually?  No matter how I stare at it, I
can't figure out what's going on.

A problem I run into often at work is that when you try to merge
another person's work, you often don't know how every bit of it works.
But in order to review it, you've really got to figure out each
line of it.  I try to read every line of my patch and ask myself
why that line is right / necessary.

There's things in this patch that I definitely don't want to merge
without understanding them.  For instance, the #if 0's in interp_set,
and this comment:

+  /* APPLE LOCAL: I don't think we want to clear the parent interpreter's
+     The parent interpreter may want to be able to snoop on the child
+     interpreter through them.  */

I thought the interpreters were supposed to be independent, which makes
that comment suspicious, and I don't see what it applies to either.

Oh, and I noticed that there's nothing here that needs the new argument
to deprecated_command_loop_hook.

-- 
Daniel Jacobowitz
CodeSourcery

Re: async patch (no. 4)

[Nick Roberts]

 > I've started looking at this.  I suspect I've asked you this before; if
 > so, I apologize.  But could you give me the big picture on what is
 > supposed to be different with this patch applied - what's the goal, and
 > should it be the default eventually?  No matter how I stare at it, I
 > can't figure out what's going on.

To get the async behaviour you need to start GDB with the --async option.
In my branch I have a file called README.async which explains more (see
below)

 > A problem I run into often at work is that when you try to merge
 > another person's work, you often don't know how every bit of it works.
 > But in order to review it, you've really got to figure out each
 > line of it.  I try to read every line of my patch and ask myself
 > why that line is right / necessary.

You're not looking at the last patch, which is smaller, that I sent:

Re: [PATCH] PR mi/2077 "set edit off" breaks MI"
(Tue, 21 Nov 2006 11:20:41 +1300)

 You> We've branched; you have a patch ready; let's get it going!  I'd be
 You> glad to see this merged.  I'm afraid I really haven't looked at what
 You> you have so far; could you post a current patch that I can experiment
 You> with?

 Me> I'm attaching it below....

It's a much smaller change than I started with in the async branch and
I understand more lines than I did then.  Clearly when it comes to
committing the patch I/we need to understand each line but for now I
left in things that I thought were necessary.

 > There's things in this patch that I definitely don't want to merge
 > without understanding them.  For instance, the #if 0's in interp_set,
 > and this comment:
 > 
 > +  /* APPLE LOCAL: I don't think we want to clear the parent interpreter's
 > +     The parent interpreter may want to be able to snoop on the child
 > +     interpreter through them.  */
 > 
 > I thought the interpreters were supposed to be independent, which makes
 > that comment suspicious, and I don't see what it applies to either.

This is still in the latest patch.  I'm unable to currently answer this
question.

 > Oh, and I noticed that there's nothing here that needs the new argument
 > to deprecated_command_loop_hook.

I'll remove it if it's not needed.

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


(edited README.async)

The initial changes are directed at the output of the asynchronous commands
such as run, continue, next, finish etc.  These are implemented through
mi_execute_async_cli_command and for asynchronus operation require
mi_exec_async_cli_cmd_continuation to be called through fetch_inferior event.

Currently -exec-next and next generate different output because the
asynchronous output is faked:

(gdb)
-exec-next
^running
(gdb)
*stopped,reason="end-stepping-range",thread-id="0",frame={addr="0x0804857f",func="main",args=[],file="myprog.c",fullname="/home/nickrob/myprog.c",line="69"}
(gdb)
n
&"n\n"
~"70\t  int n1 = 7, n2 = 8, n3 = 9;\n"
^done
(gdb)

With these changes the output is the same:

-exec-next
^running
(gdb)
*stopped,reason="end-stepping-range",thread-id="0",frame={addr="0x0804857f",func="main",args=[],file="myprog.c",fullname="/home/nickrob/myprog.c",line="69"}
(gdb)
n
&"n\n"
^running
^done
(gdb)
*stopped,reason="end-stepping-range",thread-id="0",frame={addr="0x080485bb",func="main",args=[],file="myprog.c",fullname="/home/nickrob/myprog.c",line="70"}
(gdb)

(actually it generates an extra &"n\n" and ^done but with 
"-interpreter-exec console" it's identical.

To enter a GDB command while the target is running GDB needs a separate
terminal to the inferior.  Currently only a few CLI commands can be entered
e.g pwd see top.c.  Most CLI commands report:

  Cannot execute this command while the target is running.

and all MI commands apart from -exec-interrupt (see mi-main.c) report:

  Cannot execute command interpreter-exec while target running

but this can easily be changed.

I've re-instated the --async option so that --noasync (the default) *should*
run as before.

To run testsuite with asynchronous event loop put the line:

  set GDBFLAGS "--async"

at the bottom of site.exp.

Re: async patch (no. 4)

[Daniel Jacobowitz]

On Sat, Jan 13, 2007 at 11:24:02AM +1300, Nick Roberts wrote:
>  > A problem I run into often at work is that when you try to merge
>  > another person's work, you often don't know how every bit of it works.
>  > But in order to review it, you've really got to figure out each
>  > line of it.  I try to read every line of my patch and ask myself
>  > why that line is right / necessary.
> 
> You're not looking at the last patch, which is smaller, that I sent:
> 
> Re: [PATCH] PR mi/2077 "set edit off" breaks MI"
> (Tue, 21 Nov 2006 11:20:41 +1300)

Whoops!  Thanks.  Well, let this be a reminder to both of us to put
things in their own threads :-)  I'll look at that one instead.  I
thought the other one was newer.

Thanks.

-- 
Daniel Jacobowitz
CodeSourcery

Re: async patch (no. 4)

[Nick Roberts]

Daniel Jacobowitz writes (Fri, 12 Jan 2007 18:03:33 -0500):

 > > You're not looking at the last patch, which is smaller, that I sent:
 > > 
 > > Re: [PATCH] PR mi/2077 "set edit off" breaks MI"
 > > (Tue, 21 Nov 2006 11:20:41 +1300)

This attachment is called async5.diff.gz.

 > Whoops!  Thanks.  Well, let this be a reminder to both of us to put
 > things in their own threads :-)  I'll look at that one instead.  I
 > thought the other one was newer.

I'm not sure if this was the last message in the thread but I'd like to
see if we can get something committed after GDB 6.7 goes out.

As you probably know the GDB mode in Emacs 22.1 uses annotations with some MI
through the "interpreter" command, e.g, variable objects.  Now I want to try to
migrate Emacs to fully use MI, and have some code that I plan to commit shortly
to the Emacs CVS repository.  I think this really needs GDB to work
asynchronously.

With the patch that I posted, when GDB is invoked normally behaviour shouldn't
change.  To work asynchronously, it needs to be invoked with the "--async"
option.  In this way, I hope to insulate others from any problems that it may
have.

I realise that you're probably very busy, but there's no immediate hurry, and
to save time perhaps you could just check that synchronous behaviour is indeed
unaffected.

I can posted an updated patch if needed.

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

Re: async patch (no. 4)

[Daniel Jacobowitz]

On Sun, Jun 17, 2007 at 09:28:16PM +1200, Nick Roberts wrote:
> I'm not sure if this was the last message in the thread but I'd like to
> see if we can get something committed after GDB 6.7 goes out.

Neither of us understands what parts of the patch do; in fact, I
understand almost none of it.  I think it would be a very bad decision
for us to commit structural changes to GDB that we do not understand
(even if they are disabled by default), and I think we have no hope of
keeping the code working and improving it unless it is clearer.  These
are the same requirements we apply to other changes.

I'd be happy to help you merge this, if you can break it down into
well-understood and necessary pieces.  I can help with making the
native wait case asynchronous, since I know how that works.  I also
have a patch lying around that makes target async-remote work a bit
better.

-- 
Daniel Jacobowitz
CodeSourcery

Re: async patch (no. 4)

[Nick Roberts]

 > > I'm not sure if this was the last message in the thread but I'd like to
 > > see if we can get something committed after GDB 6.7 goes out.
 > 
 > Neither of us understands what parts of the patch do; in fact, I
 > understand almost none of it.  I think it would be a very bad decision
 > for us to commit structural changes to GDB that we do not understand
 > (even if they are disabled by default), and I think we have no hope of
 > keeping the code working and improving it unless it is clearer.  These
 > are the same requirements we apply to other changes.

I don't know how you can claim to uderstand almost none of it after you
suggested to me to use SIGCHLD to interrupt the call to select instead of using
threads.  I think some of these comments are based on looking at the previous
(eponymous) patch (no. 4).

 > I'd be happy to help you merge this, if you can break it down into
 > well-understood and necessary pieces.  I can help with making the
 > native wait case asynchronous, since I know how that works.  I also
 > have a patch lying around that makes target async-remote work a bit
 > better.

This is made harder by the fact that some of the event loop is quite obscure
and I suspect has a level of abstraction that is currently unused.  Perhaps a
good starting point would be to simplify this first.

I posted a ChangeLog last time which I divided roughly into my changes and
Apple's which I post below again.  Perhaps you can help me by telling me which
parts you think are unintelligible.  I concede that some of my changes might
fit in that category and don't pretend that understand I them completely.

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


2006-11-21  Nick Roberts  <nickrob@snap.net.nz>

	* defs.h (async_signal_hook): Declare.
	(event_loop_p): New extern.
	(deprecated_command_loop_hook): Make argument type void*.
	
	* main.c (captured_main, print_gdb_help): Add and describe --async
	option conditional on macro ASYNC.
	(event_loop_p): New global variable.
	
	* event-loop.c (async_signal_hook): New function pointer.
	(gdb_wait_for_event): Use it.

	* linux-nat.c: Include inf-loop.h.  Include poll.h conditionally.
	(gdb_post_startup): New extern.
	(gdb_status, gdb_file_event): New variables.
	(async_mask, old_mask, async_action, old_action): New variables.
	(linux_nat_attach, linux_nat_detach): Condition on
	target_can_async_p.
	(linux_nat_resume): Add async file handler if necessary.
	(linux_nat_fetch_event): New function.
	(linux_nat_wait): Use it when asynchronous.
	(async_sigchld_handler, linux_nat_signal_hook): New functions.
	(_initialize_linux_nat): Set up signal handling and pipe for
	asynchronous behaviour.

	* infrun.c: Include event-top.h.
	(sync_execution): Set sync_execution to 1 initially.
	(proceed): Set target_executing to 0 if synchronous.
	(handle_inferior_event): Print process switching for MI(?).
	Don't call context_switch (ecs) or flush_cached_frames ().
	Use local variable was_sync_execution.

	* inf-ptrace.c: Include inf-loop.h, event-loop.h and event-top.h.
	(standard_is_async_p, standard_can_async_p)
	(initialize_sigint_signal_handler): New externs.
	(async_client_context, async_terminal_ours_p): New variables.
	(async_file_handler, inf_ptrace_async): New functions.
	(inf_ptrace_him): Use gdb_post_startup as a flag.
	(inf_ptrace_mourn_inferior): Don't call waitpid if asynchronous.
	(inf_ptrace_attach): Set gdb_post_startup.  Call wait_for_inferior
	if asynchronous.
	(async_terminal_inferior): New function.
	(inf_ptrace_target): Add methods if asynchronous.

	* exec.c (standard_async, standard_is_async_p, standard_can_async_p):
	New functions.
	(init_exec_ops): Initialise above methods if GDB is invoked
	with --async.

	* infcmd.c (jump_command): Call async_disable_stdin if necessary
	before decode_line_spec_1.
	(attach_command): Make attach work asynchronously.

	 (copied verbatim)
	* event-top.h (cli_command_loop):  Make argument type void*.

	 (copied verbatim)
	* event-top.c (cli_command_loop): Make argument void*.
	(display_gdb_prompt, async_enable_stdin)
	(async_disable_stdin, handle_sigint, async_request_quit)
	(gdb_setup_readline, _initialize_event_loop): 
	Changes for asynchronous operation.

	 (copied verbatim)
	* interps.c (interp_set): Make type struct interp *.
	Don't do_all_continuations or call clear_interpreter_hooks. 
	(current_interp_command_loop): Call command_loop with NULL argument.
	(interp_set_quiet): Don't make static.

	 (copied verbatim)
	* interps.h (interp_set, interp_set_quiet): New externs.

	 (copied verbatim)
	* inf-loop.c (inferior_event_handler, complete_execution): 
	Changes for asynchronous operation.

	 (copied verbatim)
	* cli/cli-interp.c (cli_interpreter_resume): Set sync_execution.
	(safe_execute_command): Don't make it static.
	(_initialize_cli_interp): Add cli_command_loop to interp_procs
	structure.

	 (copied verbatim)
	* wrapper.h (safe_execute_command): Declare here.

	 (copied verbatim)
	* remote.c (cleanup_sigint_signal_handler)
	(initialize_sigint_signal_handler): Don't make static.

	 (copied verbatim)
	* top.c (deprecated_command_loop_hook): Make argument type void*.

	 (copied verbatim)
	* mi/mi-interp.c (mi1_command_loop, mi2_command_loop)
	(mi3_command_loop): Make argument type void*.

	* config/i386/nm-linux.h: Add ASYNC macro definition.

	* Makefile.in (inf-ptrace.o, infrun.o, linux-nat.o)
	(cli-interp.o): Add new header dependencies.

 
 ChangeLog              |  102 +++++++++++++++++++++++++++
 Makefile.in            |    9 !!
 cli/cli-interp.c       |   11 -!
 config/i386/nm-linux.h |    3
 defs.h                 |    6 +
 event-loop.c           |   15 ++++
 event-top.c            |   93 ++++++++++++++!!!!!!!!!!!
 event-top.h            |    2
 exec.c                 |   26 ++++++-
 inf-loop.c             |   46 ++++++!!!!!!
 inf-ptrace.c           |  101 ++++++++++++++++++++++++++!
 infcmd.c               |   25 ++--!!
 infrun.c               |   34 ++++---!
 interps.c              |   40 +++!!!!!!!
 interps.h              |    3
 linux-nat.c            |  181 +++++++++++++++++++++++++++++!!!!!!!!!!!!!!!!!!!
 main.c                 |   15 ++++
 mi/mi-interp.c         |   12 !!!
 remote.c               |    8 !!
 top.c                  |    2
 wrapper.h              |    2
 21 files changed, 490 insertions(+), 25 deletions(-), 221 modifications(!)

Re: async patch (no. 4)

[Daniel Jacobowitz]

On Mon, Jun 18, 2007 at 08:50:38AM +1200, Nick Roberts wrote:
> I don't know how you can claim to uderstand almost none of it after you
> suggested to me to use SIGCHLD to interrupt the call to select instead of using
> threads.  I think some of these comments are based on looking at the previous
> (eponymous) patch (no. 4).

That's the only part of the patch I claim to understand at all.  It's
not that it's unintelligible - it's that I don't understand why any
given line is necessary.  Like, what does async_signal_hook do, and
why is it called from those particular places?  Why does async
behavior change where we need to claim the terminal?  Are the
quit_flag changes still necessary now that we've done some work on
QUIT?  Why don't the infrun changes break thread handling all over the
place?

All of these things are the same questions we'd ask for any patch.

> This is made harder by the fact that some of the event loop is quite obscure
> and I suspect has a level of abstraction that is currently unused.  Perhaps a
> good starting point would be to simplify this first.

Sounds fine to me.  We can always re-add it if we want it.

-- 
Daniel Jacobowitz
CodeSourcery

Re: async patch (no. 4)

[Nick Roberts]

 > > I don't know how you can claim to uderstand almost none of it after you
 > > suggested to me to use SIGCHLD to interrupt the call to select instead of
 > > using threads.  I think some of these comments are based on looking at the
 > > previous (eponymous) patch (no. 4).
 > 
 > That's the only part of the patch I claim to understand at all.  It's
 > not that it's unintelligible - it's that I don't understand why any
 > given line is necessary.  Like, what does async_signal_hook do, and
 > why is it called from those particular places?  Why does async
 > behavior change where we need to claim the terminal?  Are the
 > quit_flag changes still necessary now that we've done some work on
 > QUIT?  Why don't the infrun changes break thread handling all over the
 > place?
 > 
 > All of these things are the same questions we'd ask for any patch.

Sure, but to be fair I think this is the first time you have asked them.

1) what does async_signal_hook do

   Firstly this patch only currently works for Linux.  That is because I don't
   know enough about other OSes and ISTR that you said others could extend
   easily it to them later.  For Linux, async_signal_hook is initialised to
   linux_nat_signal_hook in _initialize_linux_nat.  It is called (with
   different arguments) immediately before and after calls to select (or poll,
   if appropriate) and only if gdb is invoked with --async (event_loop_p != 0).

   On the first call, linux_nat_signal_hook sets up a handler,
   async_sigchld_handler, for SIGCHLD, that writes the return value of waitpid
   to the file descriptor that linux_nat_fetch_event reads from.
   linux_nat_fetch_event is called instead of my_waitpid with an asynchronous
   target in linux_nat_wait.  On the second call linux_nat_signal_hook
   restores the old signal mask.

I'll try to answer the other questions in due course, but I'd like to hear if
you think I'm making sense trying to answer this one first.


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

Re: async patch (no. 4)

[Daniel Jacobowitz]

On Mon, Jun 18, 2007 at 05:00:20PM +1200, Nick Roberts wrote:
> Sure, but to be fair I think this is the first time you have asked them.

Sure.  Like I said, the patch is hard to review.  I think that each of
the bits I asked a question about is, logically, a separate change to
GDB.  More or less.

> 1) what does async_signal_hook do
> 
>    Firstly this patch only currently works for Linux.  That is because I don't
>    know enough about other OSes and ISTR that you said others could extend
>    easily it to them later.  For Linux, async_signal_hook is initialised to
>    linux_nat_signal_hook in _initialize_linux_nat.  It is called (with
>    different arguments) immediately before and after calls to select (or poll,
>    if appropriate) and only if gdb is invoked with --async (event_loop_p != 0).
> 
>    On the first call, linux_nat_signal_hook sets up a handler,
>    async_sigchld_handler, for SIGCHLD, that writes the return value of waitpid
>    to the file descriptor that linux_nat_fetch_event reads from.
>    linux_nat_fetch_event is called instead of my_waitpid with an asynchronous
>    target in linux_nat_wait.  On the second call linux_nat_signal_hook
>    restores the old signal mask.
> 
> I'll try to answer the other questions in due course, but I'd like to hear if
> you think I'm making sense trying to answer this one first.

Yes, that makes sense - thank you, and this gives me a sense of making
progress on the async patch :-)

It does explain what the hook is for, although actually the hook is
the wrong approach.  If this belongs to the native target, then it
should be target_async_signal_hook (1) and go through the target
vector.  On the other hand, if the remote target is going to want to
use the same technique on GNU/Linux (i.e. if it depends on the host)
then maybe it belongs somewhere else and not as a hook at all.  Either
utils.c or posix-hdep.c depending how different it is going to be on
multiple platforms.

More broadly, we have a couple of different ways for providing
host-specific and target-specific code so a new global function
pointer should be avoided.

-- 
Daniel Jacobowitz
CodeSourcery

Re: async patch (no. 4)

[Nick Roberts]

 > > I'll try to answer the other questions in due course, but I'd like to hear
 > > if you think I'm making sense trying to answer this one first.
 > 
 > Yes, that makes sense - thank you, and this gives me a sense of making
 > progress on the async patch :-)
 > 
 > It does explain what the hook is for, although actually the hook is
 > the wrong approach.  If this belongs to the native target, then it
 > should be target_async_signal_hook (1) and go through the target
 > vector.  On the other hand, if the remote target is going to want to
 > use the same technique on GNU/Linux (i.e. if it depends on the host)
 > then maybe it belongs somewhere else and not as a hook at all.  Either
 > utils.c or posix-hdep.c depending how different it is going to be on
 > multiple platforms.

Perhaps I'll start with the native target because it looks easier and I
think I understand that.

You mean define a method t->to_async_signal_hook and use it like this:

#define	target_async_signal_hook(args) \
 (*current_target.to_async_signal_hook) (args)

Right?

 > More broadly, we have a couple of different ways for providing
 > host-specific and target-specific code so a new global function
 > pointer should be avoided.

2) Why does async behavior change where we need to claim the terminal?

I don't think it does generally change _where_ we need to claim it but _how_ we
do.  I think the idea is just to add and remove the file handler,
async_file_handler, which invokes inferior_event_handler when gdb take/gives
up the terminal.

3) Are the quit_flag changes still necessary now that we've done some work on
QUIT? 

They have already been added as part of the patch that Fred Fish committed.

4) Why don't the infrun changes break thread handling all over the place?

I'm afraid that the changes in infrun.c are voodoo that I've copied from Apple.
This is one file that I don't think I'll understand for a long time, if at
all.  My approach has been empirical which probably reveals my scientific
background where it is highly rated (but computer scientists just don't
seem to get it!).

However, I can say that thread handling improved when I used the variable
gdb_file_event in async_sigchld_handler:

static void
async_sigchld_handler (int signo)
{
  int msg[2];
  int status, options;

  if (gdb_file_event)
    options = __WCLONE | WNOHANG;
  else
    options = __WALL | WNOHANG;

  msg[0] = waitpid (-1, &status, options);

  if (msg[0] > 0)
    {
      msg[1] = status;
      write (gdb_status[1], msg, 2*sizeof (int));
      gdb_file_event = 1;
    }
}

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