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

[Pedro Alves <pedro_alves at portugalmail dot pt>]

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?

> +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 ..."

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

>  }
>  
>  /* 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.

Thanks for such a needed feature.

Cheers,
Pedro Alves