Re: [Patch] Win32 gdbserver new interrupt support, and attach to process fix.

[Lerele <lerele@champenstudios.com>]

Pedro Alves wrote:

>
> Lerele wrote:
>
>>  }
>> -#endif
>> +
>> +/* Expose 'static void input_interrupt (int unused)' function to 
>> enable checking for a +   remote interrupt request. */   
>
>
> Is the 'Expose 'static void input_interrupt (int unused)' function' 
> part really necessary?
>
You mean the comment?
The function?
Or even having to call that function at all to be able to interrupt?

I added a comment just to state that it's a wrapper for that other (with 
a not so significant function name, given its purpose for win32).

I added the function:
1) Not to change static attribute input_interrupt has (I am of the kind 
that I prefer not touch stuff if it's not necessary --less possibility 
for new bugs I think).
2) To give it a rather more significant name (given the context in which 
it is called now), and not expose it as 'input_interrupt'.

Having to call that function is necessary to check if client gdb asked 
gdbserver to stop child process.
Also, can't use async IO signals on win32 the way it's done on 
gdbserver, so I just directly "exposed" that function.


>> +void
>> +check_remote_input_interrupt_request (void)
>> +{
>> +  input_interrupt(0);
>> +}
>>   
>
>
>
>>  
>> +
>> +     It should be safe to continue child given this wait status. 
>> +     See function get_child_debug_event. Default wait status is 
>> spurious,
>> +     and it gets modified if any important debug events get received.
>> +     More specifically, this status gets returned in the wait loop 
>> to +     allow socket pooling/resuming, to allow for remote 
>> interruption. */
>>   
>
>
> Full stop, double space. "...get_child_debug_event.  Default ..."
>
Please clarify.



>>    TARGET_WAITKIND_SPURIOUS,
>>  };
>>  
>> @@ -574,7 +584,7 @@
>>  
>>    FreeLibrary (kernel32);
>>  
>> -  return res;
>> +  return res? 0:-1;
>>   
>
> Space around '?' and ':'.  Personally, I liked the res != FALSE ? 0 : 
> -1 version better.
>

Honestly, I don't really care too much.
You guys decide.


>>  }
>>  
>>  /* Kill all inferiors.  */
>> @@ -840,7 +850,12 @@
>>    last_sig = TARGET_SIGNAL_0;
>>    ourstatus->kind = TARGET_WAITKIND_SPURIOUS;
>>  
>> -  if (!(debug_event = WaitForDebugEvent (&current_event, 1000)))
>> +  /* Check for remote interruption request */
>> +  check_remote_input_interrupt_request();
>>   
>
>
> Missing space: "check_remote_input_interrupt_request ()"
> Full stop double-space: "request.  */", although I guess the comment 
> adds no value, since the
> function name says pretty much the same.
>
>> +/* Send a signal to the inferior process, however is appropriate. */
>> +static void
>> +win32_send_signal (int signo)
>> +{
>> +  if ( signo == TARGET_SIGNAL_INT )
>> +    {
>> +      if ( !GenerateConsoleCtrlEvent(CTRL_BREAK_EVENT, 
>> current_process_id) )
>>   
>
> Missing space: "GenerateConsoleCtrlEvent ("
>
> Also, pedantically speaking, both remote_utils.c:putpkt_binary, and 
> remote_utils.c:input_interrupt
> (the sole users of _send_signal) send SIGINT, not TARGET_SIGNAL_INT.  
> Luckily on
> Windows, both are defined as 2.
>
> I have a local patch that does:
>
>   /* Send a signal to the inferior process, however is appropriate.  */
> -  void (*send_signal) (int);
> +  void (*send_signal) (enum target_signal);
>
> ... and takes care of the conversion of the target side.  I'll post it 
> for review.  In the meantime, you may
> want to change your patch to use SIGINT.
>

You mean line gdb/signals/signal.c 'target_signal_from_host' and 
'do_target_signal_to_host' functions?
I'm not really sure if we should use SIGINT in win32-i386-low.c, given 
that it should only know about target signals.



> Thanks for such a needed feature.
>

Thanks to you.
Nice to see it gets used.
I surely find it useful. It's just great to cross develop on Linux to 
just use Windows as a target. 8-) 
However, I don't know if people out there are using it, otherwise I 
guess the attach bug should have been listed.
It isn't that strange to use the attach feature, is it? Well, I don't 
use it really. hmmm.
Been taking a look ocasionally at gdb buglist, but didn't see any related.

Regards,
Leo.