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

[Pedro Alves <pedro_alves at portugalmail dot 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