Re: [RFC] New gdbserver Win32 interrupt code

[Pedro Alves <pedro_alves@portugalmail.pt>]

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