[PATCH v2] cygthread: suspend thread before terminating.

[Corinna Vinschen]

Hi Jeremy,


thanks for this patch, it looks pretty nice.  A few minor points...

On Nov 14 08:24, Jeremy Drake via Cygwin-patches wrote:
> @@ -302,6 +302,20 @@ cygthread::terminate_thread ()
>    if (!inuse)
>      goto force_notterminated;
> 
> +  if (_my_tls._ctinfo != this)
> +    {
> +      CONTEXT context;
> +      context.ContextFlags = CONTEXT_CONTROL;
> +      /* SuspendThread makes sure a thread is "booted" from emulation before
> +	 it is suspended.  As such, the emulator hopefully won't be in a bad
> +	 state (aka, holding any locks) when the thread is terminated. */
> +      SuspendThread (h);
> +      /* We need to call GetThreadContext, even though we don't care about the
> +	 context, because SuspendThread is asynchronous and GetThreadContext
> +	 will make sure the thread is *really* suspended before returning */
> +      GetThreadContext (h, &context);
> +    }
> +

Neat, but if this only affects the ARM64 emulation, shouldn't this only
be called under wincap.cpu_arch() == PROCESSOR_ARCHITECTURE_AMD64?

>    TerminateThread (h, 0);
>    WaitForSingleObject (h, INFINITE);
>    CloseHandle (h);
> diff --git a/winsup/cygwin/pinfo.cc b/winsup/cygwin/pinfo.cc
> index e31a67d8f4..2395c36665 100644
> --- a/winsup/cygwin/pinfo.cc
> +++ b/winsup/cygwin/pinfo.cc
> @@ -1252,13 +1252,14 @@ proc_waiter (void *arg)
> 
>    for (;;)
>      {
> -      DWORD nb;
> +      DWORD nb, err;
>        char buf = '\0';
> 
>        if (!ReadFile (vchild.rd_proc_pipe, &buf, 1, &nb, NULL)
> -	  && GetLastError () != ERROR_BROKEN_PIPE)
> +	  && (err = GetLastError ()) != ERROR_BROKEN_PIPE)
>  	{
> -	  system_printf ("error on read of child wait pipe %p, %E", vchild.rd_proc_pipe);

A one-line comment explain why ERROR_OPERATION_ABORTED is exempt from
the debug message might be helpful here.

> +	  if (err != ERROR_OPERATION_ABORTED)
> +	    system_printf ("error on read of child wait pipe %p, %E", vchild.rd_proc_pipe);
>  	  break;
>  	}
> 
> diff --git a/winsup/cygwin/sigproc.cc b/winsup/cygwin/sigproc.cc
> index 81b6c31695..360bdac232 100644
> --- a/winsup/cygwin/sigproc.cc
> +++ b/winsup/cygwin/sigproc.cc
> @@ -410,7 +410,8 @@ proc_terminate ()
>  	  if (!have_execed || !have_execed_cygwin)
>  	    chld_procs[i]->ppid = 1;
>  	  if (chld_procs[i].wait_thread)
> -	    chld_procs[i].wait_thread->terminate_thread ();
> +	    if (!CancelSynchronousIo (chld_procs[i].wait_thread->thread_handle ()))

This expression should be bracketed.  But actually, can you just change
this to

   if (chld_procs[i].wait_thread
       && CancelSynchronousIo())


please?  And another comment might be helpful here, too.


Thanks,
Corinna