Re: linux native async mode support

[Daniel Jacobowitz <drow at false dot org>]

Nick, would you like to comment on this before it goes in?

On Fri, Mar 14, 2008 at 08:10:22AM +0000, Pedro Alves wrote:
> To enable/disable async mode, there's a new set/show command
> "set linux-async 1" enables it.  I left it on in this patch, but
> if/when this goes in, it should be turned off by default until
> we fix at least the defines.exp problems.

Could you move this to "maint set"?  Users shouldn't go twiddling it.
When everything is together, I hope we can remove it again.

Until then, it has to go in the manual; Eli went to a lot of trouble
to fix all the undocumented commands :-)  The debug command needs
to also, unless you want to lump it in with debug_linux_nat.

> (remote.c could take the same treatment and get rid
> of "target async").

Yeah.  If you want to do that, we could call it "maint set async".

> @@ -516,6 +707,12 @@ linux_child_follow_fork (struct target_o
>  	 target_detach (which does other necessary cleanup).  */
>  
>        push_target (ops);
> +
> +      if (async_was_enabled && !linux_nat_async_enabled)
> +	/* target_detach may disable async depending on multi-threaded
> +	   enabled or not.  Reenable it if needed.  */
> +	linux_nat_enable_async ();
> +
>        linux_nat_switch_fork (inferior_ptid);
>        check_for_thread_db ();
>  

This will drain the queue, losing any pending events.  In non-stop
mode I can imagine that causing trouble.  Maybe we need to avoid
target_detach now.

> @@ -966,6 +1173,17 @@ linux_nat_attach (char *args, int from_t
>    pid_t pid;
>    int status;
>    int cloned = 0;
> +  sigset_t mask, prev_mask;
> +
> +  /* Make sure SIGCHLD is blocked.  The sync mode SIGCHLD handler
> +     accesses the current_target, which is pushed by
> +     linux_ops->to_attach below.  This stops the race.  It also
> +     prevents the async mode to lose any SIGCHLD events, as the async
> +     handler is only registered at function exit.  */
> +  sigemptyset (&mask);
> +  sigaddset (&mask, SIGCHLD);
> +  sigprocmask (SIG_SETMASK, NULL, &prev_mask);
> +  sigprocmask (SIG_BLOCK, &mask, NULL);

That's the same as sigprocmask (SIG_BLOCK, &mask, &prev_mask).

We can get a SIGCHLD pretty much any time, even when we think
the target is stopped (e.g. if we detached from something
that ran on the same terminal, and it finally exited).  Do we
reach common code that might change the target stack while
SIGCHLD is unblocked?  If it's just for the new assert, adding
the assert may not be worth the trouble.

>  
>    /* FIXME: We should probably accept a list of process id's, and
>       attach all of them.  */
> @@ -994,9 +1212,36 @@ linux_nat_attach (char *args, int from_t
>  
>    lp->stopped = 1;
>  
> -  /* Fake the SIGSTOP that core GDB expects.  */
> -  lp->status = W_STOPCODE (SIGSTOP);
> -  lp->resumed = 1;
> +  if (target_can_async_p ())
> +    /* This needs to before the above, so the current target is
> +       already set when the user didn't specify an exec file, and the
> +       async SIGCHLD handler doesn't mess with the waitpid calls
> +       above.  */
> +    linux_nat_enable_async ();

"This needs to before the above" has wrong grammar, and I don't know
what it means; do you mean after?

> @@ -1199,6 +1456,13 @@ linux_nat_resume (ptid_t ptid, int step,
>  			    "LLR: Short circuiting for status 0x%x\n",
>  			    lp->status);
>  
> +      if (target_can_async_p () && linux_nat_async_enabled)
> +	{
> +	  target_async (inferior_event_handler, 0);
> +
> +	  /* Wake event loop with special token, to get to WFI.  */
> +	  linux_nat_event_pipe_push (-1, -1, -1);
> +	}
>        return;
>      }
>  

If target_can_async_p, doesn't that imply linux_nat_async_enabled?

> +      /* We're in sync mode.  Make sure SIGCHLD isn't handled by
> +	 async_sigchld_handler, which would call waitpid and put the
> +	 results in the event pipe.  When we wake up from sigsuspend
> +	 below, we wan't to call waitpid ourselves.  It is just easier
> +	 and more efficient to toggle the handler.  */

wan't -> want

>  	  if (options & __WCLONE)
> -	    sigsuspend (&suspend_mask);
> +	    {
> +	      if (target_can_async_p () && linux_nat_async_enabled)
> +		{
> +		  /* No interesting event.  */
> +		  ourstatus->kind = TARGET_WAITKIND_IGNORE;
> +
> +		  /* Get ready for the next event.  */
> +		  target_async (inferior_event_handler, 0);
> +
> +		  if (debug_linux_nat_async)
> +		    fprintf_unfiltered (gdb_stdlog, "LLW: exit (ignore)\n");
> +
> +		  return minus_one_ptid;

If we just return from here, we miss clear_sigio_trap and
clear_sigint_trap.  But we keep calling set_sigint_trap on each
entrance.  I think we'll eventually end up with them saving and
restoring the wrong signal handlers.

> +/* target_can_async_p implementation.  */
> +
> +static int
> +linux_nat_can_async_p (void)
> +{
> +  /* NOTE: palves 2008-03-11: We're only async when the user requests
> +     it explicitly with the "set linux-async" command.  Someday, linux
> +     will always be async.  */
> +  if (!linux_nat_async_permitted)
> +    return 0;
> +
> +  /* See target.h/target_async_mask.  */
> +  return linux_nat_async_mask_value;
> +}

I hope we can get rid of target_async_mask someday.  Would just a
wait call suffice?  if (target_executing) reenter the event
loop or something like that?

> +/* Disable async mode.  */
> +
> +static void
> +linux_nat_disable_async (void)
> +{
> +  /* Unregister from event loop.  Don't go through the target stack
> +     (target_async), as linux-nat may have been poped off already.  */

popped

> +  linux_nat_async (NULL, 0);
> +
> +  linux_nat_async_enabled = 0;
> +
> +  sigaction (SIGCHLD, &async_old_action, NULL);
> +
> +  /* Restore the original signal mask.  */
> +  sigprocmask (SIG_SETMASK, &normal_mask, NULL);
> +  sigemptyset (&blocked_mask);
> +
> +  clear_cached_waitpid_queue ();
> +  linux_nat_async_enabled = 0;
> +  linux_nat_num_queued_events = 0;

Sets linux_nat_async_enabled twice.

Other than that, I don't see any problems.

-- 
Daniel Jacobowitz
CodeSourcery