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

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

[Lerele]

Hello,

I've added remote interrupt support for gdbserver on win32.
Also fixed gdbserver attach functionality, it wasn't even working at all.

Daniel, adding this support was much simpler than what I proposed some 
time ago.
You can also see I have added in server.h a new function so that win32 
code can call 'input_interrupt' function from remote-utils.c. I don't 
know if you're going to like that (having win32-i386-low.c call server 
code), but it needs to check for socket data availability, to be able to 
interrupt the child process.

Also, interrupt support is done by having the gdbserver constantly check 
(4 times a second) for debug events from child, and also check for 
socket data to interrupt it. This was how win32 gdbserver was working 
before anyway, except it was doing it once a second, and without 
checking socket data.
The alternative I think would be to, as Eli Zaretskii pointed out some 
days ago in gdb list, to have a separate thread check for socket 
availability and have it send the signal to the child, however this 
option would need some kind of thread synchronization to access 
gdbserver data from the thread (mainly read socket data from several 
threads).
It works fine as it is, without making it too complex. Only problem is 
that gdbserver may consume *some* cpu.

Hope patch is Ok.

Regards,
Leo.


PD: Patch follows:

2007-02-24    Leo Zayas
    * server.h, remote-utils.c: Expose input_interrupt through
    check_remote_input_interrupt_request for gdbserver.
    * win32-i386-low.c: Fix gdbserver attach support on win32.
    * win32-i386-low.c: Add remote interrupt support.
===================================================================
diff -u src_orig/gdb/gdbserver/remote-utils.c 
src/gdb/gdbserver/remote-utils.c
--- src_orig/gdb/gdbserver/remote-utils.c    2007-02-16 
21:01:14.000000000 +0100
+++ src/gdb/gdbserver/remote-utils.c    2007-02-23 23:53:44.796875000 +0100
@@ -78,7 +78,11 @@
 int remote_debug = 0;
 struct ui_file *gdb_stdlog;
 
-static int remote_desc;
+#ifdef USE_WIN32API
+static int remote_desc=INVALID_SOCKET;
+#else
+static int remote_desc=-1;
+#endif
 
 /* FIXME headerize? */
 extern int using_threads;
@@ -567,8 +571,6 @@
   return putpkt_binary (buf, strlen (buf));
 }
 
-#ifndef USE_WIN32API
-
 /* Come here when we get an input interrupt from the remote side.  This
    interrupt should only be active while we are waiting for the child to do
    something.  About the only thing that should come through is a ^C, which
@@ -580,16 +582,30 @@
   fd_set readset;
   struct timeval immediate = { 0, 0 };
 
+  /* We will be calling input_interrupt on win32 to check for remote 
interrupt,
+     via exposed function 'check_remote_input_interrupt_request'.
+     This function may be called before establishing communications;
+     need to validate socket object. */
+
+#ifdef USE_WIN32API
+  if (remote_desc==INVALID_SOCKET)
+    return;
+#else
+  if (remote_desc==-1)
+    return;
+#endif
+
   /* Protect against spurious interrupts.  This has been observed to
      be a problem under NetBSD 1.4 and 1.5.  */
-
+ 
   FD_ZERO (&readset);
   FD_SET (remote_desc, &readset);
+
   if (select (remote_desc + 1, &readset, 0, 0, &immediate) > 0)
     {
       int cc;
       char c = 0;
-     
+
       cc = read (remote_desc, &c, 1);
 
       if (cc != 1 || c != '\003')
@@ -602,7 +618,13 @@
       (*the_target->send_signal) (SIGINT);
     }
 }
-#endif
+
+/* Expose 'static void input_interrupt (int unused)' function to enable 
checking for a
+   remote interrupt request. */
+void check_remote_input_interrupt_request (void)
+{
+  input_interrupt(0);
+}
 
 /* Asynchronous I/O support.  SIGIO must be enabled when waiting, in 
order to
    accept Control-C from the client, and must be disabled when talking to
diff -u src_orig/gdb/gdbserver/server.h src/gdb/gdbserver/server.h
--- src_orig/gdb/gdbserver/server.h    2007-01-09 18:59:08.000000000 +0100
+++ src/gdb/gdbserver/server.h    2007-02-23 12:53:56.859375000 +0100
@@ -147,6 +147,7 @@
 void disable_async_io (void);
 void unblock_async_io (void);
 void block_async_io (void);
+void check_remote_input_interrupt_request (void);
 void convert_ascii_to_int (char *from, unsigned char *to, int n);
 void convert_int_to_ascii (unsigned char *from, char *to, int n);
 void new_thread_notify (int id);
diff -u src_orig/gdb/gdbserver/win32-i386-low.c 
src/gdb/gdbserver/win32-i386-low.c
--- src_orig/gdb/gdbserver/win32-i386-low.c    2007-01-09 
18:59:08.000000000 +0100
+++ src/gdb/gdbserver/win32-i386-low.c    2007-02-24 00:26:26.171875000 
+0100
@@ -37,6 +37,10 @@
 
 #define LOG 0
 
+#if LOG
+#include "../gdb_wait.h"
+#endif
+
 #define OUTMSG(X) do { printf X; fflush (stdout); } while (0)
 #if LOG
 #define OUTMSG2(X) do { printf X; fflush (stdout); } while (0)
@@ -238,7 +242,13 @@
 
   /* Nothing happened, but we stopped anyway.  This perhaps should be 
handled
      within target_wait, but I'm not sure target_wait should be 
resuming the
-     inferior.  */
+     inferior.
+
+     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. */
   TARGET_WAITKIND_SPURIOUS,
 };
 
@@ -574,7 +584,7 @@
 
   FreeLibrary (kernel32);
 
-  return res;
+  return res? 0:-1;
 }
 
 /* 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();
+
+  /* 250 ms wait time for debug event to allow for more precise
+     remote interruption. */
+  if (!(debug_event = WaitForDebugEvent (&current_event, 250)))
     goto out;
 
   current_inferior =
@@ -1004,6 +1019,10 @@
 
       return our_status.value.sig;
     }
+      else if (our_status.kind == TARGET_WAITKIND_SPURIOUS)
+    {
+      /* do nothing, just continue */
+    }
       else
     OUTMSG (("Ignoring unknown internal event, %d\n", our_status.kind));
 
@@ -1054,6 +1073,35 @@
   return child_xfer_memory (memaddr, (char *) myaddr, len, 1, 0) != len;
 }
 
+/* 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))
+    {
+      /* fallback to XP/Vista 'DebugBreakProcess'.
+         GenerateConsoleCtrlEvent can fail if process id being debugged is
+         not a process group id.
+         Note if the above function fails, the code bellow will
+         generate a breakpoint exception to stop. */
+
+      typedef BOOL winapi_DebugBreakProcess(HANDLE);
+      HMODULE kernel32 = LoadLibrary ("KERNEL32.DLL");
+      winapi_DebugBreakProcess *DebugBreakProcess = NULL;
+      DebugBreakProcess =
+        (winapi_DebugBreakProcess *) GetProcAddress (kernel32,
+                             "DebugBreakProcess");
+      if (DebugBreakProcess==NULL || !DebugBreakProcess 
(current_process_handle))
+        {
+          OUTMSG ( ("Could not interrupt process.\n") );
+        }
+      FreeLibrary(kernel32);
+    }
+    }
+}
+
 static struct target_ops win32_target_ops = {
   win32_create_inferior,
   win32_attach,
@@ -1067,7 +1115,7 @@
   win32_read_inferior_memory,
   win32_write_inferior_memory,
   0,
-  0
+  win32_send_signal
 };
 
 /* Initialize the Win32 backend.  */

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

[Eli Zaretskii]

> Date: Sat, 24 Feb 2007 00:52:07 +0100
> From: Lerele <lerele@champenstudios.com>
> 
> I've added remote interrupt support for gdbserver on win32.
> Also fixed gdbserver attach functionality, it wasn't even working at all.

Thanks!

> 2007-02-24    Leo Zayas
>     * server.h, remote-utils.c: Expose input_interrupt through
>     check_remote_input_interrupt_request for gdbserver.
>     * win32-i386-low.c: Fix gdbserver attach support on win32.
>     * win32-i386-low.c: Add remote interrupt support.

Please reformat the ChangeLog entries according to the GNU coding
standards (http://www.gnu.org/prep/standards/).

> +#ifdef USE_WIN32API
> +static int remote_desc=INVALID_SOCKET;
> +#else
> +static int remote_desc=-1;
> +#endif

I don't like using OS-dependent #define's where a functionality-based
#define can do the job.  How about

    +#ifndef INVALID_SOCKET
    +#define INVALID_SOCKET -1
    +#endif

and then use INVALID_SOCKET everywhere?

Also, this:

    +static int remote_desc=INVALID_SOCKET;

is not according to GNU standards: you need spaces around `='.

> +  /* We will be calling input_interrupt on win32 to check for remote 
> interrupt,

Your mailer wraps lines, which will cause the Patch utility to fail to
apply the diffs.  Please find a way to not wrap lines in the patch; if
nothing else helps, send it as a binary attachment.

> +#ifdef USE_WIN32API
> +  if (remote_desc==INVALID_SOCKET)
> +    return;
> +#else
> +  if (remote_desc==-1)
> +    return;
> +#endif

See above.

> -
> + 

Please don't add excess whitespace.  (There's a single blank on the
line you modified.)

> @@ -574,7 +584,7 @@
>  
>    FreeLibrary (kernel32);
>  
> -  return res;
> +  return res? 0:-1;

I don't understand the need for this change.  Can you explain?
child_continue does not promise to return exactly 1 when it fails,
only non-zero.

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

[Lerele]

Oops. This was my first patch submission directly to the list (last one 
I sent directly to gdbserver maintainer), something had to go wrong, sorry.

>>+#ifdef USE_WIN32API
>>+static int remote_desc=INVALID_SOCKET;
>>+#else
>>+static int remote_desc=-1;
>>+#endif
>>    
>>
>
>I don't like using OS-dependent #define's where a functionality-based
>#define can do the job.  How about
>
>    +#ifndef INVALID_SOCKET
>    +#define INVALID_SOCKET -1
>    +#endif
>
>and then use INVALID_SOCKET everywhere?
>  
>
Isn't INVALID_SOCKET just an OS specific define?
Since this define will be in non OS specific files, wouldn't it "hurt" 
to be reading that portable code with OS specific embedded  words?

Personally I'd prefer something like:
#if USE_WIN32API
#define BADSOCKET INVALID_SOCKET
#else
#define BADSOCKET -1
#endif

However I believe it's natural to use -1 directly to check for bad 
socket in non-win32 code, so maybe using INVALID_SOCKET or BADSOCKET 
everywhere would make code uglier.

Tell me what you prefer.




>  
>
>>@@ -574,7 +584,7 @@
>> 
>>   FreeLibrary (kernel32);
>> 
>>-  return res;
>>+  return res? 0:-1;
>>    
>>
>
>I don't understand the need for this change.  Can you explain?
>child_continue does not promise to return exactly 1 when it fails,
>only non-zero.
>
>
>  
>

That should actually be the simple fix for attach to process (function 
win32_attach). Do you see that line in child_continue function? Strange. 
It should be the last line in win32_attach.

'res' as it is indicates success if it's != 0 (it's a Win32 BOOL), 
however the attach to process function should indicate success returning 
0, or -1 if it cannot attach (see server.c 'attach_inferior' function). 
That's what this line does.
Is it necessary to add a source code comment here to explain this?

Or change the line; maybe this line would be clearer:

return res != FALSE? 0:-1;

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

[Eli Zaretskii]

> Date: Sat, 24 Feb 2007 14:02:37 +0100
> From: Lerele <lerele@champenstudios.com>
> CC: gdb-patches@sourceware.org
> 
> >>+#ifdef USE_WIN32API
> >>+static int remote_desc=INVALID_SOCKET;
> >>+#else
> >>+static int remote_desc=-1;
> >>+#endif
> >>    
> >>
> >
> >I don't like using OS-dependent #define's where a functionality-based
> >#define can do the job.  How about
> >
> >    +#ifndef INVALID_SOCKET
> >    +#define INVALID_SOCKET -1
> >    +#endif
> >
> >and then use INVALID_SOCKET everywhere?
> >  
> >
> Isn't INVALID_SOCKET just an OS specific define?

It is defined on some systems, but not on others.  However, it is
(IMO) cleaner to use the defined symbol than to use the name of the OS
or an OS-specific API, because if tomorrow some other supported
platform will define INVALID_SOCKET, the code I suggested will work
without any changes, while yours will require to add that other
platform's name to the #ifdef.

> >>@@ -574,7 +584,7 @@
> >> 
> >>   FreeLibrary (kernel32);
> >> 
> >>-  return res;
> >>+  return res? 0:-1;
> >>    
> >>
> >
> >I don't understand the need for this change.  Can you explain?
> >child_continue does not promise to return exactly 1 when it fails,
> >only non-zero.
> >
> >
> >  
> >
> 
> That should actually be the simple fix for attach to process (function 
> win32_attach). Do you see that line in child_continue function? Strange. 
> It should be the last line in win32_attach.

Sorry, you are right, I was looking at the wrong function.

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

[Lerele]

[-- Attachment #1: Type: text/plain, Size: 1678 bytes --]


New patch attached.
Hope didn't miss anything.

Regards,
Leo.



Eli Zaretskii wrote:

>>Date: Sat, 24 Feb 2007 14:02:37 +0100
>>From: Lerele <lerele@champenstudios.com>
>>CC: gdb-patches@sourceware.org
>>
>>    
>>
>>>>+#ifdef USE_WIN32API
>>>>+static int remote_desc=INVALID_SOCKET;
>>>>+#else
>>>>+static int remote_desc=-1;
>>>>+#endif
>>>>   
>>>>
>>>>        
>>>>
>>>I don't like using OS-dependent #define's where a functionality-based
>>>#define can do the job.  How about
>>>
>>>   +#ifndef INVALID_SOCKET
>>>   +#define INVALID_SOCKET -1
>>>   +#endif
>>>
>>>and then use INVALID_SOCKET everywhere?
>>> 
>>>
>>>      
>>>
>>Isn't INVALID_SOCKET just an OS specific define?
>>    
>>
>
>It is defined on some systems, but not on others.  However, it is
>(IMO) cleaner to use the defined symbol than to use the name of the OS
>or an OS-specific API, because if tomorrow some other supported
>platform will define INVALID_SOCKET, the code I suggested will work
>without any changes, while yours will require to add that other
>platform's name to the #ifdef.
>
>  
>
>>>>@@ -574,7 +584,7 @@
>>>>
>>>>  FreeLibrary (kernel32);
>>>>
>>>>-  return res;
>>>>+  return res? 0:-1;
>>>>   
>>>>
>>>>        
>>>>
>>>I don't understand the need for this change.  Can you explain?
>>>child_continue does not promise to return exactly 1 when it fails,
>>>only non-zero.
>>>
>>>
>>> 
>>>
>>>      
>>>
>>That should actually be the simple fix for attach to process (function 
>>win32_attach). Do you see that line in child_continue function? Strange. 
>>It should be the last line in win32_attach.
>>    
>>
>
>Sorry, you are right, I was looking at the wrong function.
>
>
>  
>


[-- Attachment #2: diff --]
[-- Type: text/plain, Size: 6846 bytes --]

2007-02-24	Leo Zayas

	* server.h (check_remote_input_interrupt_request): 
	New function.
	* remote_utils.c (check_remote_input_interrupt_request):
	New function to expose input_interrupt.

	* remote_utils.c [INVALID_SOCKET]: 
	Define for Win32 API code.

	* remote_utils.c (input_interrupt): 
	Validate socket object to be able to call the function
	without having initialized communication.

	* win32-i386-low.c [LOG]: Include "../gdb_wait.h".
	When using LOG define, need header for WSTOPSIG.

	* win32-i386-low.c (win32-attach): Fix attach to process. 
	Return value was incorrect, making attach fail. 

	* win32-i386-low.c (get_child_debug_event): 
	Check for remote interrupt request.
	Lower Win32 debug event pollling from 1 sec to 250 ms.

	* win32-i386-low.c (win32_wait): 
	Skip unknown event message for TARGET_WAITKIND_SPURIOUS.

	* win32-i386-low.c (win32_send_signal): New function.
	Called by server to send interrupt signal to child.

=================================================================== 
diff -u src_orig/gdb/gdbserver/remote-utils.c src/gdb/gdbserver/remote-utils.c
--- src_orig/gdb/gdbserver/remote-utils.c	2007-02-16 21:01:14.000000000 +0100
+++ src/gdb/gdbserver/remote-utils.c	2007-02-24 16:06:37.640625000 +0100
@@ -60,6 +60,10 @@
 typedef int socklen_t;
 #endif
 
+#ifndef INVALID_SOCKET
+# define INVALID_SOCKET -1
+#endif
+
 /* A cache entry for a successfully looked-up symbol.  */
 struct sym_cache
 {
@@ -78,7 +82,7 @@
 int remote_debug = 0;
 struct ui_file *gdb_stdlog;
 
-static int remote_desc;
+static int remote_desc = INVALID_SOCKET;
 
 /* FIXME headerize? */
 extern int using_threads;
@@ -567,8 +571,6 @@
   return putpkt_binary (buf, strlen (buf));
 }
 
-#ifndef USE_WIN32API
-
 /* Come here when we get an input interrupt from the remote side.  This
    interrupt should only be active while we are waiting for the child to do
    something.  About the only thing that should come through is a ^C, which
@@ -580,6 +582,14 @@
   fd_set readset;
   struct timeval immediate = { 0, 0 };
 
+  /* We will be calling input_interrupt on win32 to check for remote interrupt, 
+     via exposed function 'check_remote_input_interrupt_request'.
+     This function may be called before establishing communications;
+     need to validate socket object. */
+
+  if (remote_desc == INVALID_SOCKET)
+    return;
+
   /* Protect against spurious interrupts.  This has been observed to
      be a problem under NetBSD 1.4 and 1.5.  */
 
@@ -589,7 +599,7 @@
     {
       int cc;
       char c = 0;
-      
+
       cc = read (remote_desc, &c, 1);
 
       if (cc != 1 || c != '\003')
@@ -602,7 +612,14 @@
       (*the_target->send_signal) (SIGINT);
     }
 }
-#endif
+
+/* Expose 'static void input_interrupt (int unused)' function to enable checking for a 
+   remote interrupt request. */ 
+void
+check_remote_input_interrupt_request (void)
+{
+  input_interrupt(0);
+}
 
 /* Asynchronous I/O support.  SIGIO must be enabled when waiting, in order to
    accept Control-C from the client, and must be disabled when talking to
diff -u src_orig/gdb/gdbserver/server.h src/gdb/gdbserver/server.h
--- src_orig/gdb/gdbserver/server.h	2007-01-09 18:59:08.000000000 +0100
+++ src/gdb/gdbserver/server.h	2007-02-23 12:53:56.859375000 +0100
@@ -147,6 +147,7 @@
 void disable_async_io (void);
 void unblock_async_io (void);
 void block_async_io (void);
+void check_remote_input_interrupt_request (void);
 void convert_ascii_to_int (char *from, unsigned char *to, int n);
 void convert_int_to_ascii (unsigned char *from, char *to, int n);
 void new_thread_notify (int id);
diff -u src_orig/gdb/gdbserver/win32-i386-low.c src/gdb/gdbserver/win32-i386-low.c
--- src_orig/gdb/gdbserver/win32-i386-low.c	2007-01-09 18:59:08.000000000 +0100
+++ src/gdb/gdbserver/win32-i386-low.c	2007-02-24 15:13:14.750000000 +0100
@@ -37,6 +37,10 @@
 
 #define LOG 0
 
+#if LOG
+#include "../gdb_wait.h"
+#endif
+
 #define OUTMSG(X) do { printf X; fflush (stdout); } while (0)
 #if LOG
 #define OUTMSG2(X) do { printf X; fflush (stdout); } while (0)
@@ -238,7 +242,13 @@
 
   /* Nothing happened, but we stopped anyway.  This perhaps should be handled
      within target_wait, but I'm not sure target_wait should be resuming the
-     inferior.  */
+     inferior.
+
+     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. */
   TARGET_WAITKIND_SPURIOUS,
 };
 
@@ -574,7 +584,7 @@
 
   FreeLibrary (kernel32);
 
-  return res;
+  return res? 0:-1;
 }
 
 /* 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();
+
+  /* 250 ms wait time for debug event to allow for more precise 
+     remote interruption. */
+  if (!(debug_event = WaitForDebugEvent (&current_event, 250))) 
     goto out;
 
   current_inferior =
@@ -1004,6 +1019,10 @@
 
 	  return our_status.value.sig;
 	}
+      else if (our_status.kind == TARGET_WAITKIND_SPURIOUS)
+	{
+	  /* do nothing, just continue */
+	}
       else
 	OUTMSG (("Ignoring unknown internal event, %d\n", our_status.kind));
 
@@ -1054,6 +1073,35 @@
   return child_xfer_memory (memaddr, (char *) myaddr, len, 1, 0) != len;
 }
 
+/* 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) )
+	{
+	  /* fallback to XP/Vista 'DebugBreakProcess'.
+	     GenerateConsoleCtrlEvent can fail if process id being debugged is 
+	     not a process group id.
+	     Note if the above function fails, the code bellow will 
+	     generate a breakpoint exception to stop. */
+
+	  typedef BOOL winapi_DebugBreakProcess(HANDLE);
+	  HMODULE kernel32 = LoadLibrary ("KERNEL32.DLL");
+	  winapi_DebugBreakProcess *DebugBreakProcess = NULL;
+	  DebugBreakProcess = 
+	    (winapi_DebugBreakProcess *) GetProcAddress (kernel32, 
+							 "DebugBreakProcess");
+	  if ( DebugBreakProcess == NULL || !DebugBreakProcess (current_process_handle) )
+	    {
+	      OUTMSG ( ("Could not interrupt process.\n") );
+	    }
+	  FreeLibrary(kernel32);
+	}
+    }
+}
+
 static struct target_ops win32_target_ops = {
   win32_create_inferior,
   win32_attach,
@@ -1067,7 +1115,7 @@
   win32_read_inferior_memory,
   win32_write_inferior_memory,
   0,
-  0
+  win32_send_signal
 };
 
 /* Initialize the Win32 backend.  */

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

[Eli Zaretskii]

> Date: Sat, 24 Feb 2007 16:23:30 +0100
> From: Lerele <lerele@champenstudios.com>
> CC: gdb-patches@sourceware.org
> 
> New patch attached.
> Hope didn't miss anything.

Thanks, I'm happy now.  But I'd like to hear from Daniel as well,
before you commit the changes.

Also, a few small gotchas:

> 2007-02-24	Leo Zayas

This should state your email address as well.

> 	* remote_utils.c [INVALID_SOCKET]: 
> 	Define for Win32 API code.

You meant "Define for non-Win32 code", right?

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

[Pedro Alves]

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

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

[Andreas Schwab]

Pedro Alves <pedro_alves@portugalmail.pt> writes:

> Lerele wrote:
>> +/* 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 ("

And too many spaces.

+  if (signo == TARGET_SIGNAL_INT)
+    {
+      if (!GenerateConsoleCtrlEvent (CTRL_BREAK_EVENT, current_process_id))

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

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

[Lerele]

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.

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

[Daniel Jacobowitz]

On Sun, Feb 25, 2007 at 12:29:30AM +0100, Lerele wrote:
> 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-) 

Indeed!  Sorry I haven't gotten back to you about the patch yet - I
definitely will soon.

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

I'm not surprised no one's noticed yet.  For one thing, it's only been
in a single release of GDB so far - very few people even know about it
yet.  And I use attach with gdbserver probably only 2% as often as I
use it to start programs directly.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Pedro Alves]

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

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

[Pedro Alves]

Lerele wrote:

> It works fine as it is, without making it too complex.
> 

Just tested it on Cygwin + XP Pro, and I could successfully stop a
running process.  Nice.

I plan on eventually adding a third option to GenerateConsoleCtrlEvent
and DebugBreakProcess, that injects a thread into the debuggee, since
WinCE doesn't have any of the above.  That should make every
stoppable process stoppable, in all Windows versions.

Cheers,
Pedro Alves

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

[Lerele]

Pedro Alves wrote:

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


I was wrong, res is not a Win32 BOOL, it's plainly a 0 or a 1.
Already fixed spacing, while you were uploading your patches.
I'll send a new version for interrupt patch when I get some time to do 
it, I'll need get latest cvs and merge my changes, then resend patch.



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

I changed it to:

/* Send a signal to the inferior process, however is appropriate.  */
static void
win32_send_signal (int signo)
{
  if (target_signal_from_host (signo) == TARGET_SIGNAL_INT)
    {


I guess that would have been sufficient. But since you already changed 
"target->send_signal" to "target->send_interrupt" I guess that is no 
longer necessary.


> Hi all,
>
> This patch adds support for sending OUTPUT_DEBUG_STRING_EVENT
> messages to the controller gdb.  Compared to the version found in
> gdb/win32-nat.c, this handles Unicode messages, needed for
> WinCE, but will not handle the special cygwin signal markers. 


That was the next thing I was going to do for win32 low. Nicely done. 
Thanks. :-)


> Lerele wrote:
>
>> It works fine as it is, without making it too complex.
>>
>
> Just tested it on Cygwin + XP Pro, and I could successfully stop a
> running process.  Nice.
>
> I plan on eventually adding a third option to GenerateConsoleCtrlEvent
> and DebugBreakProcess, that injects a thread into the debuggee, since
> WinCE doesn't have any of the above.  That should make every
> stoppable process stoppable, in all Windows versions.
>

Do you find appropriate to use this method?
Isn't there any other way to do it on WinCE?

I say this because creating a single thread, either if you do it at 
child process creation, or at interrupt request, may make the child 
behave differently than when running it as a standalone (not being 
debugged). I mean, I don't know if this is really desired for gdbserver 
because doing this, memory layout for child may change, and when I debug 
a process that is buggy, say it crashes or whatever, I expect that when 
I debug the process, it will behave exactly the same way as when I run 
it without it being debugged, or at least as close as possible. A single 
memory allocation *may* affect a buggy child behavior and bypass the 
looked-for bug.

As an alternative, may I suggest maybe doing a simple PauseThread for 
all threads in child?
Win32 low already has synthetic thread pausing support and maybe this 
could be a fair solution.
This would involve tricking gdb to think a real Win32 debug exception 
has occured, and control that situation within win32 low exclusively.
The only problems I see with this is that 1) a child may suspend/resume 
threads (even though win32 docs say those functions are for debugger 
mainly), and this could interfere with stop procedure. 2) May affect 
timing, but anyway this already is affected by just running the child in 
multiprocess environment.

2) I guess is not really a problem.
1) May be solved by doing:

1. Create in gdbserver process as much threads as number of cpus -1 the 
computer has. These threads should consume all scheduled cpu for them.
2. SetPriorityClass on gdbserver process with real-time priority.
3. GetPriorityClass on child and store it to restore later on.
4. SetPriorityClass on child with below normal or idle.
5. Suspend all child threads.
Child should be stopped here.

Only problem I see with this can be if child changes its own priority 
class between steps 3 and 4 above, however this is a very remote 
possibility, because if this happens it is because child is already 
running in real-time priority, and in this case gdbserver possibly may 
not even work at all (unless you run gdbserver itself with this priority).

What do you think about this?
Does WinCE API have these required functions?
It may seem a quite odd way of doing it, but if it works, that's what 
counts I think.

Leo.

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

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 4508 bytes --]

Lerele wrote:
> Pedro Alves wrote:
> 
>> 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;
> 
> 
> I was wrong, res is not a Win32 BOOL, it's plainly a 0 or a 1.

Then make it a BOOL, it makes the code clearer anyway.  See attachment.


> Already fixed spacing, while you were uploading your patches.
> I'll send a new version for interrupt patch when I get some time to do 
> it, I'll need get latest cvs and merge my changes, then resend patch.
> 

> I guess that would have been sufficient. But since you already changed 
> "target->send_signal" to "target->send_interrupt" I guess that is no 
> longer necessary.
> 

Yep, just change it to win32_send_interrupt (void), and drop the
if (sig == SIGINT) test.

>> I plan on eventually adding a third option to GenerateConsoleCtrlEvent
>> and DebugBreakProcess, that injects a thread into the debuggee, since
>> WinCE doesn't have any of the above.  That should make every
>> stoppable process stoppable, in all Windows versions.
>>
> 
> Do you find appropriate to use this method?

It is exactly what DebugBreakProcess does internally, so if you
have a problem with the approach, you shouldn't be using it :)
The thread is short-lived.  It is something like:

//inferior's address space
void helper_thread (void*)
{
   DebugBreak ();  // stops here.

   // and dies on resume.
}


// debugger's address space
DebugBreakProcess (pid)
{
   if (debugger_present) // we are sure of this
	                // but this call can come from
	                // a 3rd process.
     {
       create_remote_thread (helper_thread, pid);
     }
}


I stumbled on the fact that on WinCE there is no
CreateRemoteThread.  There is an undocumented but widely used
PerformCallback4 that does a pretty good hack and can be used
to simulate CreateRemoteThread (haven't tested yet), but it
gone in WinCE >= 5, and I don't know the a replacement, that
doesn't need messing with the registry to inject a
dll into the inferior's address space, and start a thread
on DllMain.  Still searching.  Maybe it is possible
to stop one thread, and manually manipulate the stack to
insert a call into CreateThread with DebugBreak as
entry point.  Although the prototype for DebugBreak is
different from what CreateThread expects, there should be
no problem, since WinCE is __cdecl.  It may be problematic
to do this is a thread blocked inside a Win32 or kernel
call, or if the inferior thread chosen to do it is in a stack
corrupted state.  The advantage of these approaches is that there
is a real breakpoint exception being thrown.  Oh, you can't just
insert a breakpoint at the current PC, since it may be a ROM
address (real Flash/ROM, not locked pages).

> Isn't there any other way to do it on WinCE?
> 

Probably.  I would like to know how other debuggers do it.

> 1) May be solved by doing:
> 
> 1. Create in gdbserver process as much threads as number of cpus -1 the 
> computer has. These threads should consume all scheduled cpu for them.
> 2. SetPriorityClass on gdbserver process with real-time priority.
> 3. GetPriorityClass on child and store it to restore later on.
> 4. SetPriorityClass on child with below normal or idle.
> 5. Suspend all child threads.
> Child should be stopped here.
> 
> Only problem I see with this can be if child changes its own priority 
> class between steps 3 and 4 above, however this is a very remote 
> possibility, because if this happens it is because child is already 
> running in real-time priority, and in this case gdbserver possibly may 
> not even work at all (unless you run gdbserver itself with this priority).
> 
> What do you think about this?

I was leaving something like this as a last resort, but should be possible,
I guess.  Would need extra logic to handle the fact that there isn't
any real breakpoint exception live (in resume, for instance).

> Does WinCE API have these required functions?
> It may seem a quite odd way of doing it, but if it works, that's what 
> counts I think.
> 

Don't know about WinCE > 5.  The whole security model + address space model
has changed.  I'm still looking at a portable way across all WinCEs from
Mobile 2003 to 6, and if I can simulate a DebugBreakProcess, it would
all be nicely encapsulated.

Thanks for the suggestions!

Cheers,
Pedro Alves

[-- Attachment #2: win32_attach --]
[-- Type: text/plain, Size: 1338 bytes --]

Index: src/gdb/gdbserver/win32-i386-low.c
===================================================================
--- src.orig/gdb/gdbserver/win32-i386-low.c	2007-03-03 19:52:56.000000000 +0000
+++ src/gdb/gdbserver/win32-i386-low.c	2007-03-05 00:01:52.000000000 +0000
@@ -673,7 +673,7 @@ win32_create_inferior (char *program, ch
 static int
 win32_attach (unsigned long pid)
 {
-  int res = 0;
+  BOOL res = FALSE;
   winapi_DebugActiveProcessStop *DebugActiveProcessStop = NULL;
 
   winapi_DebugSetProcessKillOnExit *DebugSetProcessKillOnExit = NULL;
@@ -683,7 +683,7 @@ win32_attach (unsigned long pid)
   DebugSetProcessKillOnExit = (winapi_DebugSetProcessKillOnExit *)
     GetProcAddress (kernel32, _T("DebugSetProcessKillOnExit"));
 
-  res = DebugActiveProcess (pid) ? 1 : 0;
+  res = DebugActiveProcess (pid);
 
   if (!res)
     error ("Attach to process failed.");
@@ -696,7 +696,7 @@ win32_attach (unsigned long pid)
 
   if (current_process_handle == NULL)
     {
-      res = 0;
+      res = FALSE;
       if (DebugActiveProcessStop != NULL)
 	DebugActiveProcessStop (current_process_id);
     }
@@ -707,7 +707,7 @@ win32_attach (unsigned long pid)
   if (kernel32 != NULL)
     FreeLibrary (kernel32);
 
-  return res? 0:-1;
+  return (res != FALSE) ? 0 : -1;
 }
 
 /* Handle OUTPUT_DEBUG_STRING_EVENT from child process.  */

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

[Pedro Alves]

Pedro Alves wrote:

> Lerele wrote:
>> Pedro Alves wrote:
>> I was wrong, res is not a Win32 BOOL, it's plainly a 0 or a 1.
> 
> Then make it a BOOL, it makes the code clearer anyway.  See attachment.
> 
> 

Or leave it an int, and fix the constants, which looks
like what was intended in the first place.


-  res = DebugActiveProcess (pid) ? 1 : 0;
+  res = DebugActiveProcess (pid) ? -1 : 0;

     if (current_process_handle == NULL)
       {
-      res = 0;
+      res = -1;
        if (DebugActiveProcessStop != NULL)
  	DebugActiveProcessStop (current_process_id);
      }

(...)

    if (kernel32 != NULL)
      FreeLibrary (kernel32);

-  return res? 0:-1;
+  return res;
}

Anyway, what I really wanted to say, that I had forgotten, was that
if you posted this patch isolated from the rest of the interrupt
support, it would be probably be classified as an "obvious" fix,
and could go in much faster.

Cheers,
Pedro Alves

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

[Daniel Jacobowitz]

On Sun, Mar 04, 2007 at 11:51:06PM +0100, Lerele wrote:
> I'll send a new version for interrupt patch when I get some time to do 
> it, I'll need get latest cvs and merge my changes, then resend patch.

Thanks.  I'm sorry I didn't get a chance to look at this last week;
I'll try to take care of the new patch more promptly.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Lerele]

Pedro Alves wrote:

>
>>> I plan on eventually adding a third option to GenerateConsoleCtrlEvent
>>> and DebugBreakProcess, that injects a thread into the debuggee, since
>>> WinCE doesn't have any of the above.  That should make every
>>> stoppable process stoppable, in all Windows versions.
>>>
>>
>> Do you find appropriate to use this method?
>
>
> It is exactly what DebugBreakProcess does internally, so if you
> have a problem with the approach, you shouldn't be using it :)
> The thread is short-lived.  It is something like:
>
>

Well so it seems. :-)
GenerateConsoleCtrlEvent seems to create a remote thread too.

It's not only that I'm concerned about it, personally I just find it 
unacceptable for a debugger to do that kind of thing (or Windows 
anyway); user code (or standard windows dlls) can do plainly anything in 
DllEntry for instance, that can make debugged code behave very differently.

It seems after all the solution proposed may be the best resort, instead 
of the last: more compatible, less interfereable with child, however a 
bit more difficult to write.
In fact, the first version of interrupt code I wrote several months ago 
worked like that (I still keep a copy somewhere). It worked, but it was 
buggy, so I dropped it and wrote that other simpler new version.
This other solution may be the one-for-all solution, because it uses 
rather more standard Win32 calls.
Another problem besides one commented in my last message, is that signal 
handlers will not be called in child, but is this really an issue when 
doing a remote interrupt request?


>
>> 1) May be solved by doing:
>>
>> 1. Create in gdbserver process as much threads as number of cpus -1 
>> the computer has. These threads should consume all scheduled cpu for 
>> them.
>> 2. SetPriorityClass on gdbserver process with real-time priority.
>> 3. GetPriorityClass on child and store it to restore later on.
>> 4. SetPriorityClass on child with below normal or idle.
>> 5. Suspend all child threads.
>> Child should be stopped here.
>>
>> Only problem I see with this can be if child changes its own priority 
>> class between steps 3 and 4 above, however this is a very remote 
>> possibility, because if this happens it is because child is already 
>> running in real-time priority, and in this case gdbserver possibly 
>> may not even work at all (unless you run gdbserver itself with this 
>> priority).
>>
>> What do you think about this?
>
>
> I was leaving something like this as a last resort, but should be 
> possible,
> I guess.  Would need extra logic to handle the fact that there isn't
> any real breakpoint exception live (in resume, for instance).
>

Yes, but it would be something just internal to win32 low, gdbserver nor 
gdb should notice.

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

[Lerele]

Daniel Jacobowitz wrote:

>On Sun, Mar 04, 2007 at 11:51:06PM +0100, Lerele wrote:
>  
>
>>I'll send a new version for interrupt patch when I get some time to do 
>>it, I'll need get latest cvs and merge my changes, then resend patch.
>>    
>>
>
>Thanks.  I'm sorry I didn't get a chance to look at this last week;
>I'll try to take care of the new patch more promptly.
>
>  
>

No problem.
I'll try to get some time this week, leave it for me if you like.
Thanks.

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

[Lerele]

Lerele wrote:

>
> It's not only that I'm concerned about it, personally I just find it 
> unacceptable for a debugger to do that kind of thing (or Windows 
> anyway); user code (or standard windows dlls) can do plainly anything 
> in DllEntry for instance, that can make debugged code behave very 
> differently.


Well, that "unacceptable" sounded a bit hard.

I meant that when I debug a program I expect the debugger not to fail in 
catching the same bugs I get when running it in standalone, and creating 
threads for interrupting the program, I think may (or may not, 
possibility is there) make it worse for the debugger to be able to catch 
those bugs (am I wrong?). If it fails in doing so, it's useless, and 
makes me search for other strategies, tools, or whatever to catch the bug.

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

[Pedro Alves]

Lerele wrote:

> It seems after all the solution proposed may be the best resort, instead 
> of the last: more compatible, less interfereable with child, however a 
> bit more difficult to write.
> In fact, the first version of interrupt code I wrote several months ago 
> worked like that (I still keep a copy somewhere). It worked, but it was 
> buggy, so I dropped it and wrote that other simpler new version.
> This other solution may be the one-for-all solution, because it uses 
> rather more standard Win32 calls.

The more I search for a *clean* way to do it in WinCE, the more I'm
inclined to drop the create_a_remote_thread idea.  A bit of
googling and reading old MSFT docs indicates that older debuggers either
suspend all threads with SuspendThread (like you suggest, or get the
main thread's PC (EIP) and set a breakpoint there, which should be
simpler.  (I would still have to do a little more work for WinCE
because the address might not be physically writable.)  I think I
read somewhere that recent MSVC uses DebugBreakProcess, and hides the fact
that the break was inside ntdll.dll, or wherever, by switching threads...

What do you think of just suspending of thread, and setting a breakpoint
at the current PC, and resuming?

> Another problem besides one commented in my last message, is that signal 
> handlers will not be called in child, but is this really an issue when 
> doing a remote interrupt request?
> 

You mean the ctrl-c event/SIGINT?  I don't have much of an opinion
here.  I never needed it except for stopping the inferior, and
WinCE doesn't have signals, 'ctrl-c events', or a proper console, for the
matter.


> 
>>
>>> 1) May be solved by doing:
>>>
>>> 1. Create in gdbserver process as much threads as number of cpus -1 
>>> the computer has. These threads should consume all scheduled cpu for 
>>> them.
>>> 2. SetPriorityClass on gdbserver process with real-time priority.
>>> 3. GetPriorityClass on child and store it to restore later on.
>>> 4. SetPriorityClass on child with below normal or idle.
>>> 5. Suspend all child threads.
>>> Child should be stopped here.
>>>
>>> Only problem I see with this can be if child changes its own priority 
>>> class between steps 3 and 4 above, however this is a very remote 
>>> possibility, because if this happens it is because child is already 
>>> running in real-time priority, and in this case gdbserver possibly 
>>> may not even work at all (unless you run gdbserver itself with this 
>>> priority).
>>>
>>> What do you think about this?
>>

What about using the version you sent (if approved), and then work on
this new version on top?  IMHO, it is better to have something working
first.  (I don't believe the extra thread makes a difference 99.999999999%
of the times.)

Cheers,
Pedro Alves

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

[Pedro Alves]

Pedro Alves escreveu:
> Lerele wrote:
>
>> It seems after all the solution proposed may be the best resort, 
>> instead of the last: more compatible, less interfereable with child, 
>> however a bit more difficult to write.
>> In fact, the first version of interrupt code I wrote several months 
>> ago worked like that (I still keep a copy somewhere). It worked, but 
>> it was buggy, so I dropped it and wrote that other simpler new version.
>> This other solution may be the one-for-all solution, because it uses 
>> rather more standard Win32 calls.
>
> The more I search for a *clean* way to do it in WinCE, the more I'm
> inclined to drop the create_a_remote_thread idea.  A bit of
> googling and reading old MSFT docs indicates that older debuggers either
> suspend all threads with SuspendThread (like you suggest, or get the
> main thread's PC (EIP) and set a breakpoint there, which should be
> simpler.  (I would still have to do a little more work for WinCE
> because the address might not be physically writable.)  I think I
> read somewhere that recent MSVC uses DebugBreakProcess, and hides the 
> fact
> that the break was inside ntdll.dll, or wherever, by switching threads...
>
> What do you think of just suspending of thread, and setting a breakpoint
> at the current PC, and resuming?
>

The GoVest debugger (*) uses something even simpler:
- Suspend main thread,
- Set single-step / trace flag (i386)
- Resume thread

(*) http://www.geocities.com/GoVest/

Cheers,
Pedro Alves

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

[Pedro Alves]

Pedro Alves wrote:
> What do you think of just suspending of thread, and setting a breakpoint
> at the current PC, and resuming?
>

Errrh, don't even bother to answer this question.  This method is of 
course very
fallible. The thread may be blocked on IO or on a synchronization 
object, which is
common in windows code (waiting for events).  If the thread doesn't get 
scheduled,
the breakpoint won't be hit.  There may not be any thread that is a got 
candidate
for the breakpoint - like if you have all your threads either deadlock 
or blocked.
The beauty of injecting a remote thread, is that it stops all the 
inferior threads atomically,
with minimum interference.  That leaves:

- ctrl-c event, which in some cases doesn't get through.
- DebugBreakProcess, on XP and 2003 Server and emulating it on NT < 5 / 
Win9x/ME / CE
  using code injection. One case where it could disturb the inferior 
would be
  when you can't debug a DllMain because of the CREATE_THREAD_EVENT that
  this generates, or is there a way to inhibit its propagation?
- 'suspend all threads manually, but no breakpoint' method.

> What about using the version you sent (if approved), and then work on
> this new version on top?  IMHO, it is better to have something working
> first.  (I don't believe the extra thread makes a difference 
> 99.999999999%
> of the times.)
This is still my opinion.

Cheers,
Pedro Alves

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

[Lerele]

Pedro Alves wrote:

> Pedro Alves wrote:
>
>> What do you think of just suspending of thread, and setting a breakpoint
>> at the current PC, and resuming?
>>
>
> Errrh, don't even bother to answer this question.  This method is of 
> course very
> fallible. The thread may be blocked on IO or on a synchronization 
> object, which is
> common in windows code (waiting for events).  If the thread doesn't 
> get scheduled,
> the breakpoint won't be hit.  There may not be any thread that is a 
> got candidate
> for the breakpoint - like if you have all your threads either deadlock 
> or blocked.
> The beauty of injecting a remote thread, is that it stops all the 
> inferior threads atomically,
> with minimum interference.  That leaves:
>
Sorry for delay.
Also there's the problem you said on WinCE about not being able to write 
memory (flash/rom...)

> - ctrl-c event, which in some cases doesn't get through.
> - DebugBreakProcess, on XP and 2003 Server and emulating it on NT < 5 
> / Win9x/ME / CE
>  using code injection. One case where it could disturb the inferior 
> would be
>  when you can't debug a DllMain because of the CREATE_THREAD_EVENT that
>  this generates, or is there a way to inhibit its propagation?
> - 'suspend all threads manually, but no breakpoint' method.
>
I'd just leave the last option.
That's according with my personal preferences, for all advantages I 
wroite in previous messages.
Does WinCe have available Set/GetPriorityClass, SetProcessAffinityMask, 
SuspendThread, ResumeThread?
First two only work on some cases/win versions anyway.
However, gdb win32-nat is already done with first option if I remember 
right, so maybe there's a preference to keep it that way.
Also, about DllMain problem you say, I'd guess last option should also 
let you seamlessly debug DllMain without doing anything else (without 
having to disable breakpoints when doing an interrupt for instance 
--this would have to be done on client gdb side-- and even would 
introduce problems such as hitting a disabled-for-interrupt breakpoint 
when interrupt has not yet reached child).
I find last option better almost however you look at it, unless there's 
some other problem I'm not seeing... maybe having a trillion threads 
running or something?
Maybe writing down a pros/cons list could decide for itself.


>> What about using the version you sent (if approved), and then work on
>> this new version on top?  IMHO, it is better to have something working
>> first.  (I don't believe the extra thread makes a difference 
>> 99.999999999%
>> of the times.)
>
> This is still my opinion.
>
I agree. Keep working code, and work on other option(s).
Haven't had time to prepare the new patch. I hope I do will this week(end).

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

[Pedro Alves]

Lerele wrote:
> I'd just leave the last option.
> That's according with my personal preferences, for all advantages I 
> wroite in previous messages.

I tend to agree.

> Does WinCe have available Set/GetPriorityClass, SetProcessAffinityMask,
WinCE <= 5 - There are other functions that would have the same effect. 
Dunno about 6.
But don't worry, I'll take care of that.  Oh, and in case you haven't 
noticed, I haven't
submitted the WinCE support yet.

> SuspendThread, ResumeThread?

Yes.  Same debug API as the big brothers.


> I find last option better almost however you look at it, unless 
> there's some other problem I'm not seeing...maybe having a trillion 
> threads running or something?

I'd be surprised is SuspendThread was noticeably slow for a reasonably 
large number
of threads, but you never know ...

Cheers,
Pedro Alves