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

[Jon TURNEY <jon.turney@dronecode.org.uk>]

Thanks for taking the time to look at this.

On 14/04/2015 14:16, Joel Brobecker wrote:
>
> 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.

Ok.  Unfortunately I don't have access to the original problems which 
inspired these patches, so I have reduced the scope to a problem I can 
easily demonstrate and split things up further.

>
> The three issues I view as independent:
>    a. ignoring "invalid handle" errors;

Yes, this is an unrelated change I should have excluded.

This was discussed somewhat in the thread starting at 
https://cygwin.com/ml/gdb-patches/2013-06/msg00237.html

>    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?

I've dropped this change for the moment.

>> @@ -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?

I'm sorry I don't follow you.

have_saved_context is reset in do_windows_fetch_inferior_registers() 
when the context is actually retrieved.

It seems odd to reset it in get_windows_debug_event() as it seems that 
any other Windows debug event between the Cygwin signal message and the 
do_windows_fetch_inferior_registers() will prevent the saved context 
being used, if it's possible for that to happen.

>> @@ -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);

I've done this (with the last line as an assignment), but you'll have to 
help me find where the Coding Standard says that.

[1] mentions not using assignments inside if conditions, but I don't see 
any mention of ?:

[1] http://www.gnu.org/prep/standards/standards.html#Syntactic-Conventions

>>   out:
>> -  return retval;
>> +  return (int) thread_id;
>
> I'm wondering whether the cast is actually necessary?

On third thoughts, I think not.

> 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?

Done.