[RFC] New gdbserver Win32 interrupt code

[RFC] New gdbserver Win32 interrupt code

[Lerele]

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

Hello,

Sorry for the delay, didn't get the time to continue with gdbserver 
Win32 coding.

Some time ago I suggested a method to better support remote interrupt of 
debugged process, in this thread:
http://sourceware.org/ml/gdb-patches/2007-03/msg00026.html

The attached patch does just what I described there, with all the 
benefits I discussed back then.
I have tried to code the bits with WinCE in mind by copying how it's 
already done for WinCE (LoadLibrary basically), but I have not compiled 
nor tested it under Windows CE. However, it does work fine with Cygwin.

Also I fixed what I think is a slight bug in thread suspend/resume in 
win32-low.c, and something that is also related with this new interrupt 
thread pause/resume functionality.
The thread_rec function was setting the gdbserver internal suspend_count 
to the number of suspends a thread actually has, as reported by Windows. 
However I think this is erroneous to keep this way because a thread may 
already be paused by the application being debugged, and as such when 
gdbserver pauses/resumes threads, it could wake up a child thread that 
should actually stay paused.

I have also kept old functionality with the macro NEW_INT in case 
someone needs/want to fallback to the old code. This can also be removed 
of course.
Any thoughts about this new functionality?

Hope patch is Ok.

Regards,
Leo.


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

ChangeLog:

2007-11-04	Leo Zayas

	* win32-low.c [NEW_INT]:
	New macro to allow conditional compilation with new or old interrupt code.
	
	* win32-low.c [NEW_INT] (interrupt_requested):
	New variable to indicate remote interrupt request.
	
	* win32-low.c [NEW_INT] (interrupt_thread_event):
	New variable to store interrupt code secondary gdbserver thread event.
	
	* win32-low.c [NEW_INT] (winapi_SetProcessPriorityBoost): New API typedef 
	[NEW_INT] (winapi_GetProcessPriorityBoost): New API typedef 
	[NEW_INT] (winapi_SetProcessAffinityMask): New API typedef
	[!NEW_INT] (winapi_DebugBreakProcess): Old typedef being replaced 
	[!NEW_INT] (winapi_GenerateConsoleCtrlEvent): Old typedef being replaced 

	* win32-low.c (thread_rec): set suspend_count to our internal suspends only.
	
	* win32-low.c (continue_one_thread): fix indentation.
	
	* win32-low.c [NEW_INT] (consume_cpu_interrupt_thread):
	Thread function that consumes cpu, and exits upon signal.
	
	* win32-low.c [NEW_INT] (pause_child_inferior_thread): 
	Pause a single thread.
	(resume_child_inferior_thread): Resume a single thread.
	
	* win32-low.c [NEW_INT] (synthetic_child_interrupt):
	New function to pause or resume all child threads.

	* win32-low.c (child_continue) [NEW_INT]: Resume the child process threads.
	
	* win32-low.c (get_child_debug_event) [NEW_INT]:
	Pause child process threads upon interrupt request, and return signal.
	
	* win32-low.c (win32_request_interrupt) [NEW_INT]:
	Set interrupt request flag.
	(win32_request_interrupt) [!NEW_INT]: Keep old code around.

diff -u src_orig/gdb/gdbserver/win32-low.c src/gdb/gdbserver/win32-low.c
--- src_orig/gdb/gdbserver/win32-low.c	2007-09-04 00:17:27.000000000 +0200
+++ src/gdb/gdbserver/win32-low.c	2007-11-04 00:54:53.218750000 +0100
@@ -37,6 +37,8 @@
 #include <sys/cygwin.h>
 #endif
 
+#define NEW_INT 1
+
 #define LOG 0
 
 #define OUTMSG(X) do { printf X; fflush (stdout); } while (0)
@@ -64,6 +66,11 @@
 
 int using_threads = 1;
 
+#if NEW_INT
+static int interrupt_requested = 0;
+static HANDLE interrupt_thread_event = NULL;
+#endif
+
 /* Globals.  */
 static HANDLE current_process_handle = NULL;
 static DWORD current_process_id = 0;
@@ -76,8 +83,14 @@
 
 typedef BOOL WINAPI (*winapi_DebugActiveProcessStop) (DWORD dwProcessId);
 typedef BOOL WINAPI (*winapi_DebugSetProcessKillOnExit) (BOOL KillOnExit);
+#if NEW_INT
+typedef BOOL WINAPI (*winapi_SetProcessPriorityBoost) (HANDLE, BOOL);
+typedef BOOL WINAPI (*winapi_GetProcessPriorityBoost) (HANDLE, PBOOL);
+typedef BOOL WINAPI (*winapi_SetProcessAffinityMask) (HANDLE, DWORD_PTR);
+#else
 typedef BOOL WINAPI (*winapi_DebugBreakProcess) (HANDLE);
 typedef BOOL WINAPI (*winapi_GenerateConsoleCtrlEvent) (DWORD, DWORD);
+#endif
 
 static DWORD main_thread_id = 0;
 
@@ -92,7 +105,7 @@
   return th->tid;
 }
 
-/* Find a thread record given a thread id.  If GET_CONTEXT is set then
+/* Find a thread record given a thread id.  If get_context is set then
    also retrieve the context for this thread.  */
 static win32_thread_info *
 thread_rec (DWORD id, int get_context)
@@ -108,7 +121,13 @@
   if (!th->suspend_count && get_context)
     {
       if (id != current_event.dwThreadId)
-	th->suspend_count = SuspendThread (th->h) + 1;
+        {
+          /* Set suspend count to *our* internal thread suspends, so that we 
+             do not interfere with already thread suspend state.  */
+          th->suspend_count++;
+          SuspendThread (th->h);
+          /* th->suspend_count = SuspendThread (th->h) + 1; */
+        }
 
       (*the_low_target.get_thread_context) (th, &current_event);
     }
@@ -265,24 +284,178 @@
       && th->suspend_count)
     {
       if (th->context.ContextFlags)
-	{
-	  (*the_low_target.set_thread_context) (th, &current_event);
-	  th->context.ContextFlags = 0;
-	}
+        {
+          (*the_low_target.set_thread_context) (th, &current_event);
+          th->context.ContextFlags = 0;
+        }
 
       for (i = 0; i < th->suspend_count; i++)
-	(void) ResumeThread (th->h);
+        (void) ResumeThread (th->h);
       th->suspend_count = 0;
     }
 
   return 0;
 }
 
+/* thread function that consumes cpu while interrupt code pauses 
+   child threads.  */
+#if NEW_INT
+static DWORD WINAPI
+consume_cpu_interrupt_thread (LPVOID data)
+{
+  HANDLE *event = (HANDLE *) data;
+  while (WaitForSingleObject (interrupt_thread_event, 0) != WAIT_OBJECT_0)
+    {
+    }
+  if (event != 0)
+    SetEvent (*event);
+  return 0;
+}
+#endif
+
+/* Pause child thread.  */
+#if NEW_INT
+static void pause_child_inferior_thread (struct inferior_list_entry *inf)
+{
+  struct thread_info *thr = (struct thread_info *) inf;
+
+  if (thr != NULL)
+    {
+      win32_thread_info *th = inferior_target_data (thr);
+      
+      /* Pause and get context from thread if necessary.  */
+      int tid = th->tid;
+      thread_rec (tid, 1);
+    }
+}
+#endif
+
+/* Resume child thread.  */
+#if NEW_INT
+static void resume_child_inferior_thread (struct inferior_list_entry *inf)
+{
+  struct thread_info *thr = (struct thread_info *) inf;
+
+  if (thr != NULL)
+    {
+      win32_thread_info *th = inferior_target_data (thr);
+      
+      /* Resume thread and set context if necessary.  */
+      int tid = th->tid;
+      continue_one_thread (inf, &tid);
+    }
+}
+#endif
+
+/* Pause/resume all child threads.  This function attempts to perform the 
+   required operation in a more portable manner across Windows versions and also
+   bypass some problems with GenerateConcoleCtrlEvent and DebugBreakProcess
+   functions.  */
+#if NEW_INT
+static void
+synthetic_child_interrupt (int pause)
+{
+  DWORD old_process_priority_class;
+  DWORD old_process_affinity_mask;
+  BOOL old_process_priority_boost;
+  DWORD system_affinity_mask;
+  HANDLE current_process_handle;
+  SYSTEM_INFO sys_info;
+  int thr;
+
+  winapi_SetProcessPriorityBoost SetProcessPriorityBoost;
+  winapi_GetProcessPriorityBoost GetProcessPriorityBoost;
+  winapi_SetProcessAffinityMask SetProcessAffinityMask;
+  
+#ifdef _WIN32_WCE
+  HMODULE dll = GetModuleHandle (_T("COREDLL.DLL"));
+#else
+  HMODULE dll = GetModuleHandle (_T("KERNEL32.DLL"));
+#endif
+
+  SetProcessPriorityBoost = GETPROCADDRESS (dll, SetProcessPriorityBoost);
+  GetProcessPriorityBoost = GETPROCADDRESS (dll, GetProcessPriorityBoost);
+  SetProcessAffinityMask = GETPROCADDRESS (dll, SetProcessAffinityMask);
+  
+  /* As we are going to play around with gdbserver scheduling, use system 
+     reported current process, in case gdbserver own data gets corrupted.  */
+  current_process_handle = OpenProcess (PROCESS_ALL_ACCESS, FALSE, 
+                                        GetCurrentProcessId ());
+
+  old_process_priority_class = GetPriorityClass (current_process_handle);
+
+  /* Go real-time so we can pause child as atomically as possible.  */
+  SetPriorityClass (current_process_handle, REALTIME_PRIORITY_CLASS);
+
+  if (GetProcessPriorityBoost != NULL)
+    GetProcessPriorityBoost (current_process_handle, 
+                             &old_process_priority_boost);
+  if (SetProcessPriorityBoost != NULL)
+    SetProcessPriorityBoost (current_process_handle, FALSE);
+  
+  GetProcessAffinityMask (current_process_handle, 
+                          &old_process_affinity_mask,
+                          &system_affinity_mask);
+
+  /* Set gdbserver to run on all CPUs.  */
+  if (SetProcessAffinityMask != NULL)
+    SetProcessAffinityMask (current_process_handle, system_affinity_mask);
+  
+  GetSystemInfo (&sys_info);
+
+  interrupt_thread_event = CreateEvent(NULL, TRUE, FALSE, NULL);
+  
+  HANDLE *events;
+  events = malloc (sizeof (HANDLE) * (sys_info.dwNumberOfProcessors - 1));
+  for (thr = 0; thr < sys_info.dwNumberOfProcessors - 1; thr++)
+    {
+      events[thr] = CreateEvent (NULL, TRUE, FALSE, NULL);
+      CreateThread(NULL, 0, consume_cpu_interrupt_thread, events+thr, 0, NULL);
+    }
+  
+  /* Now pause/resume child threads one by one.  */
+  if (pause)
+    for_each_inferior (&all_threads, pause_child_inferior_thread);
+  else
+    for_each_inferior (&all_threads, resume_child_inferior_thread);
+  
+  /* Once paused, signal gdbserver secondary cpu-consumption threads 
+     to terminate, wait for them to do so, and gracefully
+     free all synchronization objects.  */
+  SetEvent (interrupt_thread_event);
+  WaitForMultipleObjects (sys_info.dwNumberOfProcessors - 1, 
+                          events, TRUE, INFINITE);
+  for (thr = 0; thr < sys_info.dwNumberOfProcessors - 1; thr++)
+    CloseHandle (events[thr]);
+  free (events);
+  CloseHandle (interrupt_thread_event);
+  
+  /* Restore gdbserver Windows scheduling state.  */
+  if (SetProcessPriorityBoost != NULL)
+    SetProcessPriorityBoost (current_process_handle, 
+                             old_process_priority_boost);
+  if (SetProcessAffinityMask != NULL)
+    SetProcessAffinityMask (current_process_handle, old_process_affinity_mask);
+  SetPriorityClass (current_process_handle, old_process_priority_class);
+  
+  FreeLibrary (dll);
+}
+#endif
+
 static BOOL
 child_continue (DWORD continue_status, int thread_id)
 {
   BOOL res;
 
+#if NEW_INT
+  if (interrupt_requested)
+    {
+      interrupt_requested = 0;
+      synthetic_child_interrupt(0);
+      return TRUE;
+    }
+#endif
+  
   res = ContinueDebugEvent (current_event.dwProcessId,
 			    current_event.dwThreadId, continue_status);
   if (res)
@@ -1243,6 +1416,33 @@
 
   last_sig = TARGET_SIGNAL_0;
   ourstatus->kind = TARGET_WAITKIND_SPURIOUS;
+  
+#if NEW_INT
+  /* Remote interrupt has been requested.  In this case return a signal 
+   * the client gdb can understand as the child being stopped.  
+   * As we are checking this first, any pending OS exceptions arised
+   * between interrupt request and next call to WaitForDebugEvent should
+   * get stacked and pending.  */
+  if (interrupt_requested)
+    {
+      current_inferior =
+        (struct thread_info *) find_inferior_id (&all_threads,
+                             main_thread_id);
+      ourstatus->kind = TARGET_WAITKIND_STOPPED;
+      ourstatus->value.sig = TARGET_SIGNAL_INT;
+      
+      /* Clear current event data, as this could interfere with thread_rec.  */
+      current_event.dwThreadId = 0;
+      current_event.dwProcessId = current_process_id;
+      
+      /* Simulate child stop.  Basically give gdbserver process maximum priority
+       * so that child doesn't get a chance to run, then pause one by one all
+       * child threads, and finally return new signaled state.  */
+      synthetic_child_interrupt(1);
+      
+      return; 
+    }
+#endif
 
   /* Keep the wait time low enough for confortable remote interruption,
      but high enough so gdbserver doesn't become a bottleneck.  */
@@ -1445,7 +1645,8 @@
    called through read_inferior_memory, which handles breakpoint shadowing.
    Read LEN bytes at MEMADDR into a buffer at MYADDR.  */
 static int
-win32_read_inferior_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
+win32_read_inferior_memory (CORE_ADDR memaddr, unsigned char *myaddr, 
+                               int len)
 {
   return child_xfer_memory (memaddr, (char *) myaddr, len, 0, 0) != len;
 }
@@ -1455,8 +1656,9 @@
    Write LEN bytes from the buffer at MYADDR to MEMADDR.
    Returns 0 on success and errno on failure.  */
 static int
-win32_write_inferior_memory (CORE_ADDR memaddr, const unsigned char *myaddr,
-			     int len)
+win32_write_inferior_memory (CORE_ADDR memaddr, 
+                                 const unsigned char *myaddr,
+                                 int len)
 {
   return child_xfer_memory (memaddr, (char *) myaddr, len, 1, 0) != len;
 }
@@ -1465,6 +1667,12 @@
 static void
 win32_request_interrupt (void)
 {
+#if NEW_INT
+  
+  interrupt_requested = 1;
+  
+#else
+  
   winapi_DebugBreakProcess DebugBreakProcess;
   winapi_GenerateConsoleCtrlEvent GenerateConsoleCtrlEvent;
 
@@ -1492,6 +1700,8 @@
     return;
 
   OUTMSG (("Could not interrupt process.\n"));
+  
+#endif
 }
 
 static const char *

Re: [RFC] New gdbserver Win32 interrupt code

[Pedro Alves]

Lerele wrote:
> Hello,
> 
> Sorry for the delay, didn't get the time to continue with gdbserver 
> Win32 coding.
> 

Welcome back!

> I have tried to code the bits with WinCE in mind by copying how it's 
> already done for WinCE (LoadLibrary basically), but I have not compiled 
> nor tested it under Windows CE. However, it does work fine with Cygwin.
> 

I also implemented the synthetic suspending here, because soon
after that thread, I had a need for it in WinCE.  Unfortunatelly, I got
distracted with other things and never submitted.  I hate it when
work gets duplicated  :(

At a quick glance, I think your patch should be at least split
in 3.

1 - the synthetic suspending

   On WinCE, the synthetic suspending is also needed when
attaching, since in opposition to 9xNT, the OS doesn't stop
the inferior at all (neither when attaching nor running).
If you're OK with it, I'll clean up my patch a bit, and merge
with your bits so 1 can go in first.  It will be easier for me
this way, since I've already taken care of the function
reusing and ifdefing minimising.  I'll then volunteer to
clean up the rest of your patch for you, as a reward for
you patience  ;)

2 - the suspend count handling

   This is also a problem in native debugging
   (gdb/win32-nat.c).  I also saw this when doing my version
   of 1.  I got a chance to look at the logs of the native
   WinCE debugger, and could infer that it also takes care of
   this correctly.  The way they do it, is to read the current
   suspend count by always doing a SuspendThread + ResumeThread
   sequence on a debug event (ResumeThread return the current
   suspend count).

3 - the elevation

Cheers,
Pedro Alves

Re: [RFC] New gdbserver Win32 interrupt code

[Lerele]

> Welcome back!
>
Thanks!

> I also implemented the synthetic suspending here, because soon
> after that thread, I had a need for it in WinCE.  Unfortunatelly, I got
> distracted with other things and never submitted.  I hate it when
> work gets duplicated  :(
Yep, me too. :(
There's one benefit though, which is that things that slip through one's 
mind, may not through the other's. Merging may give us the super-patch! ;)


>
> At a quick glance, I think your patch should be at least split
> in 3.
You mean three different patches? Submitted individually?
I didn't submit thread pause counting separately, as it was a really 
simple fix (one line).
I'll write that down for next time.


>
> 1 - the synthetic suspending
>
>   On WinCE, the synthetic suspending is also needed when
> attaching, since in opposition to 9xNT, the OS doesn't stop
> the inferior at all (neither when attaching nor running).
> If you're OK with it, I'll clean up my patch a bit, and merge
> with your bits so 1 can go in first.  It will be easier for me
> this way, since I've already taken care of the function
> reusing and ifdefing minimising.  I'll then volunteer to
> clean up the rest of your patch for you, as a reward for
> you patience  ;)
>
Sure, no problem with that.
When you do so, please note that in the patch main pausing function 
synthetic_child_interrupt, which just pauses or resumes the child 
process, every single line is important (at least for Win32) and is 
there for a reason, as is also the order of calls, according to the 
conversation in the old thread.

I'd like to know what you are referring to when you say clean up the 
rest of my patch. I took care of properly formatting code, full stops, 
and even fixed some previous erroneous code indentation and formatting. 
I'd like to know, what is it I missed that you are talking about?
I'd also like to know what ifdefing in the patch I submitted that you 
don't like. The NEW_INT macro is the only one I sent, and I left it 
there just in case this new interrupt code isn't suitable for all, but 
can be removed easily.

Also, note that I did not include this synthetic_child_interrupt into 
separate architecture files (i386/arm) because of one of the benefits of 
this pausing method, which is portability among Windows versions, and 
this new code is compatible from Win95 and newer, however I don't know 
about WinCE.


> 2 - the suspend count handling
>
>   This is also a problem in native debugging
>   (gdb/win32-nat.c).  I also saw this when doing my version

Yes, actually when I wrote win32 gdbserver port, I started from 
modifying gdb/win32-nat.c, and the thread_rec function if I remember 
right is the same one, apart from the arch (i386/arm) modifications you 
made, so truly the problem must be there in native gdb too.
By the way, maybe this interrupt method could be of interest for native 
Win32 gdb.
If so, should we handle the native win32 patch, or should that be done 
by their respective maintainers?

Also note, that this problem is not a problem I have seen to fail in 
practice, so maybe it isn't a problem at all (for some reason). This fix 
is just something I noticed when I wrote the new interrupt function, 
which ultimately relies on thread_rec function.


>   of 1.  I got a chance to look at the logs of the native
>   WinCE debugger, and could infer that it also takes care of
>   this correctly.  The way they do it, is to read the current
>   suspend count by always doing a SuspendThread + ResumeThread
>   sequence on a debug event (ResumeThread return the current
>   suspend count).
But isn't this wrong too, according to my explanation in my previous 
message?
The problem actually is that when resuming a thread in win32-nat.c, in 
function continue_one_thread, which resumes a thread suspend_count 
times, which itself was being set to the number of suspends of the 
thread as reported by Windows, so for example if the client app had a 
thread, say suspended 2 times, and then gdbserver suspends the thread 
again, the internal gdbserver count would be 3, and when resuming it, 
gdbserver would wake it up three times, which is wrong because the child 
wanted it to be suspended 2 times.


>
> 3 - the elevation
>
What do you mean by this?


Regards,
Leo.

Re: [RFC] New gdbserver Win32 interrupt code

[Pedro Alves]

> 
>> At a quick glance, I think your patch should be at least split
>> in 3.
> You mean three different patches? Submitted individually?
> I didn't submit thread pause counting separately, as it was a really 
> simple fix (one line).
> I'll write that down for next time.
> 

Yes, please.  Independent fixes should be split into different
patches.  It is easier to review, and if it turns out
something brakes, it should be possible to easilly revert
the independant changes.  There are tools that make the
management of patch series easier.  I use quilt myself
for that.

> 
>>
>> 1 - the synthetic suspending
>>
> Sure, no problem with that.
> When you do so, please note that in the patch main pausing function 
> synthetic_child_interrupt, which just pauses or resumes the child 
> process, every single line is important (at least for Win32) and is 
> there for a reason, as is also the order of calls, according to the 
> conversation in the old thread.
> 

I'd like to split that playing with the priorities out of the
first patch, because it will be different in CE.  Focusing a
patch on just that will be clearer.

> I'd like to know what you are referring to when you say clean up the 
> rest of my patch. I took care of properly formatting code, full stops, 
> and even fixed some previous erroneous code indentation and formatting. 
> I'd like to know, what is it I missed that you are talking about?

Oh, nothing really serious, don't worry.  A few spaces around operators,
and before '(' here and there, and usage of /* * * * for comment
blocks.  After a while of staring at GNU code, it starts to really
stand out.  What's not conforming to the standards will just
look weird.   :-)

> I'd also like to know what ifdefing in the patch I submitted that you 
> don't like. The NEW_INT macro is the only one I sent, and I left it
> there just in case this new interrupt code isn't suitable for all, but 
> can be removed easily.
> 

There is no need for this macro.  We either entirely switch to this
mechanism, or we add it as a fallback if the current ones fail.

> Also, note that I did not include this synthetic_child_interrupt into 
> separate architecture files (i386/arm) because of one of the benefits of 
> this pausing method, which is portability among Windows versions, and 
> this new code is compatible from Win95 and newer, however I don't know 
> about WinCE.

Well, WinCE runs on i386 too, so splitting by machine isn't
exactly right.  (although mingw32ce only supports ARM currently).

> 
> 
>> 2 - the suspend count handling
>>
>>   This is also a problem in native debugging
>>   (gdb/win32-nat.c).  I also saw this when doing my version
> 
> Yes, actually when I wrote win32 gdbserver port, I started from 
> modifying gdb/win32-nat.c, and the thread_rec function if I remember 
> right is the same one, apart from the arch (i386/arm) modifications you 
> made, so truly the problem must be there in native gdb too.
> By the way, maybe this interrupt method could be of interest for native 
> Win32 gdb.
> If so, should we handle the native win32 patch, or should that be done 
> by their respective maintainers?
> 

I'm interested in this for WinCE.  Whoever needs this for
native debugging should submit a patch and it will be
reviewed.  We can't expect the maintainers to scratch our
own itches :-)
I think that if you look back a long way in the history of
win32-nat.c you'll see that once a similar method was used.

> 
>>   of 1.  I got a chance to look at the logs of the native
>>   WinCE debugger, and could infer that it also takes care of
>>   this correctly.  The way they do it, is to read the current
>>   suspend count by always doing a SuspendThread + ResumeThread
>>   sequence on a debug event (ResumeThread return the current
>>   suspend count).
> But isn't this wrong too, according to my explanation in my previous 
> message?
> The problem actually is that when resuming a thread in win32-nat.c, in 
> function continue_one_thread, which resumes a thread suspend_count 
> times, which itself was being set to the number of suspends of the 
> thread as reported by Windows, so for example if the client app had a 
> thread, say suspended 2 times, and then gdbserver suspends the thread 
> again, the internal gdbserver count would be 3, and when resuming it, 
> gdbserver would wake it up three times, which is wrong because the child 
> wanted it to be suspended 2 times.
> 

They must be storing the current suspend count, and not resuming
past it.  What's important is that they also though about it, or
so it seemed from the logs.

> 
>>
>> 3 - the elevation
>>
> What do you mean by this?
> 

I mean the elevating the priority of gdbserver.

Cheers,
Pedro Alves

Re: [RFC] New gdbserver Win32 interrupt code

[Lerele]

> Yes, please.  Independent fixes should be split into different
> patches.  It is easier to review, and if it turns out
> something brakes, it should be possible to easilly revert
> the independant changes.  There are tools that make the
> management of patch series easier.  I use quilt myself
> for that.
>
Ok, acknowledged. :)

> I'd like to split that playing with the priorities out of the
> first patch, because it will be different in CE.  Focusing a
> patch on just that will be clearer.
>
Okay, but it's important to have that priority stuff in Win32 final 
version to avoid gdbserver interfere with child, if it for example 
pauses/resumes its own threads while gdbserver tries to interrupt.
I think it's also important to have gdbserver to go real-time the least 
possible amount of time. Go real-time, pause child and restore priority, 
as quick as possible.

>> I'd like to know what you are referring to when you say clean up the 
>> rest of my patch. I took care of properly formatting code, full 
>> stops, and even fixed some previous erroneous code indentation and 
>> formatting. I'd like to know, what is it I missed that you are 
>> talking about?
>
> Oh, nothing really serious, don't worry.  A few spaces around operators,
> and before '(' here and there, and usage of /* * * * for comment
> blocks.  After a while of staring at GNU code, it starts to really
> stand out.  What's not conforming to the standards will just
> look weird.   :-)
>
Oops.
I did just see some of those, even though I checked it before sending patch.
Truth is I don't usually code in GNU style, just for this. At work use 
one style and my personal coding another.

>> I'd also like to know what ifdefing in the patch I submitted that you 
>> don't like. The NEW_INT macro is the only one I sent, and I left it
>> there just in case this new interrupt code isn't suitable for all, 
>> but can be removed easily.
>>
>
> There is no need for this macro.  We either entirely switch to this
> mechanism, or we add it as a fallback if the current ones fail.
>
Ok, then just it's up to removing the old code, if necessary.
One thing I'd like to comment that I have noticed about this new method 
is that when I interrupt now it seems that child threads when paused in 
system calls seem to be at different locations than with old method.
If you get the chance please take a look at the Win32 versio, as I am 
not completely sure, but it seems that this method is like more precise 
in where exactly the child gets interrupted, like if it works better in 
the sense that child gets interrupted precisely where child is running, 
and not where Windows wants to interrupt it. Not that this is usefull 
for anything, but it's curious.


>> Also, note that I did not include this synthetic_child_interrupt into 
>> separate architecture files (i386/arm) because of one of the benefits 
>> of this pausing method, which is portability among Windows versions, 
>> and this new code is compatible from Win95 and newer, however I don't 
>> know about WinCE.
>
> Well, WinCE runs on i386 too, so splitting by machine isn't
> exactly right.  (although mingw32ce only supports ARM currently).
>
So, the version I wrote is not so straightforward to use for WinCE.
I thought it would be better to interrupt WinCE also this way.


> I'm interested in this for WinCE.  Whoever needs this for
> native debugging should submit a patch and it will be
> reviewed.  We can't expect the maintainers to scratch our
> own itches :-)
> I think that if you look back a long way in the history of
> win32-nat.c you'll see that once a similar method was used.
>
...and maybe it got dumped for some reason??
That would be nice to know.
Maybe it has to do with the fact that the child being interrupted this 
way is not actually interrupted but paused, so that any other app 
running in the system could resume the child even if gdbserver has 
interrupted it.

Anyway, do your WinCE stuff, because no matter what I do here, you'll 
have to come with a solution that'll work for both Win32 and WinCE.
Once it's done on gdbserver, then maybe we can think about making a 
patch for native gdb.



>>
>>>   of 1.  I got a chance to look at the logs of the native
>>>   WinCE debugger, and could infer that it also takes care of
>>>   this correctly.  The way they do it, is to read the current
>>>   suspend count by always doing a SuspendThread + ResumeThread
>>>   sequence on a debug event (ResumeThread return the current
>>>   suspend count).
>> But isn't this wrong too, according to my explanation in my previous 
>> message?
>> The problem actually is that when resuming a thread in win32-nat.c, 
>> in function continue_one_thread, which resumes a thread suspend_count 
>> times, which itself was being set to the number of suspends of the 
>> thread as reported by Windows, so for example if the client app had a 
>> thread, say suspended 2 times, and then gdbserver suspends the thread 
>> again, the internal gdbserver count would be 3, and when resuming it, 
>> gdbserver would wake it up three times, which is wrong because the 
>> child wanted it to be suspended 2 times.
>>
>
> They must be storing the current suspend count, and not resuming
> past it.  What's important is that they also though about it, or
> so it seemed from the logs.
>
But there's no doubt in that when resuming a child thread, suspend_count 
is the number of times it gets resumed. This suspend_count as I see it 
is the number of child suspends, plus the number of gdb suspends, which 
is wrong, unless there's something I missed in the code.

Regards,
Leo.