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

[Lerele <lerele@champenstudios.com>]

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