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

[Pedro Alves <pedro_alves@portugalmail.pt>]

Lerele escreveu:
> 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?

Yep.  Sorry I wasn't clear.


>> Full stop, double space. "...get_child_debug_event.  Default ..."
>>
> Please clarify.
> 

The GNU coding convention states that you use a full stop followed by
two spaces to end a sentence.


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

I don't care much either, but I think the spaces around '?' and ':'
are mandatory.

+  return res ? 0 : -1;

>>
>>> +/* 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) )
>>>   
>>
>> 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.
> 

The only send_signal calls I could find, are in remote_utils like so:
(*the_target->send_signal) (SIGINT);

So, *pedantically*, it should be:

+static void
+win32_send_signal (int signo)
+{
+  if (signo == SIGINT)


And I agree that it doesn't look right.  With my patch
installed, it turns to:

+static void
+win32_send_signal (enum target_signal signo)
+{
+  if (signo == TARGET_SIGNAL_INT)

Anyway, don't let this bother you.  If it gets OKed as is,
I'll take care of it later.

Cheers,
Pedro Alves