Re: [PATCH] Fixes to Cygwin-specific signal handling

[Joel Brobecker <brobecker at adacore dot com>]

Jon,

On Tue, Apr 14, 2015 at 12:03:02PM +0100, Jon Turney wrote:
> Originally by cgf, this patch has been carried in Cygwin's gdb package
> for a few years.  I've cleaned it up a bit and revised it for master.
> 
> Without this patch, it's impossible to usefully run the testsuite on
> Cygwin.
> 
> gdb/ChangeLog:
> 
> 2015-04-11  Jon Turney  <jon.turney@dronecode.org.uk>
> 
> 	* windows-nat.c: Replace have_saved_context with signal_thread_id
> 	throughout.
> 	(thread_rec): Don't retrieve context if we have a saved one.
> 	Ignore 'Invalid Handle' errors.
> 	(handle_output_debug_string): Mark signal context as not to be
> 	written to inferior by windows_continue() or windows_resume().
> 	(get_windows_debug_event): Replace retval with thread_id
> 	throughout.  Don't clear any saved context.

Overall, the patch looks reasonable to me. But I think there are
at least 3 independent changes, and it would be nice to split those
two out, for a couple of reasons:
  1. It allows you to explain the nature of the problem, from the user's
     standpoint, that the patch is fixing (ie, what user-visible
     symptoms it fixes);
  2. it allows us to see how each problem is fixed, and to deal with
     each issue separately.

The three issues I view as independent:
  a. ignoring "invalid handle" errors;
  b. unsetting saved_context.ContextFlags
  c. the renaming of have_saved_context into signal_thread_id
     so you can compare the current thread id with the saved
     signal_thread_id.

A few minor comments below.

> @@ -849,8 +853,12 @@ handle_output_debug_string (struct target_waitstatus *ourstatus)
>  					 &saved_context,
>  					 __COPY_CONTEXT_SIZE, &n)
>  		   && n == __COPY_CONTEXT_SIZE)
> -	    have_saved_context = 1;
> -	  current_event.dwThreadId = retval;
> +	    {
> +	      signal_thread_id = retval;
> +	      saved_context.ContextFlags = 0;  /* Don't attempt to call SetThreadContext */

Can you move the comment just above the statement so as to avoid
exceeding the maximum line length, and also explain why we should
not call SetThreadContext?

> @@ -1330,7 +1338,6 @@ get_windows_debug_event (struct target_ops *ops,
>    event_code = current_event.dwDebugEventCode;
>    ourstatus->kind = TARGET_WAITKIND_SPURIOUS;
>    th = NULL;
> -  have_saved_context = 0;

Normally, there is a corresponding addition that sets signal_thread_id
to zero.  This leads me to be believe that there might be an additional
user-visible symptom that this patch fixes?

> @@ -1501,12 +1508,12 @@ get_windows_debug_event (struct target_ops *ops,
>    else
>      {
>        inferior_ptid = ptid_build (current_event.dwProcessId, 0,
> -				  retval);
> -      current_thread = th ?: thread_rec (current_event.dwThreadId, TRUE);
> +				  thread_id);
> +      current_thread = th ?: thread_rec (thread_id, TRUE);

I know you are just repeating what was there before, but I don't think
this is the clearest way to do things. GDB Coding Standards also do not
allow assignments within conditions. I suggest instead:

        current_thread = th;
        if (!current_thread)
          thread_rec (thread_id, TRUE);

>  out:
> -  return retval;
> +  return (int) thread_id;

I'm wondering whether the cast is actually necessary?

Also, it looks like the function's comment might be stale:

    /* Get the next event from the child.  Return 1 if the event requires
       handling by WFI (or whatever).  */
    static int
    get_windows_debug_event (struct target_ops *ops,
                             int pid, struct target_waitstatus *ourstatus)

Would you mind fixing that?

Thanks,
-- 
Joel