Re: [win32] Fix suspend count handling

[Lerele <lerele at champenstudios dot com>]

Christopher Faylor escribió:
> On Sat, Nov 24, 2007 at 03:18:42PM +0100, Lerele wrote:
>   
> > > > Christopher Faylor escribi?:
> > > >         
> > > > > I have to ask, however, if the SuspendThread's are even needed at all.
> > > > > When I was writing this code, I wasn't entirely sure that gdb needed to
> > > > > do anything like this but I erred on the side of caution.  So, I'm
> > > > > wondering if things would still work ok if the
> > > > > SuspendThread/ResumeThread stuff was gone.
> > > > >       
> > > > >           
> > > > I think they are needed.  They were anyway with the new gdbserver based
> > > > (vs.  Win32 API based) interrupt code I sent several days ago, and that
> > > > so very kindly Pedro prepared for commitment, but that I still haven't
> > > > found the time to sit down and look at them (however I'm absolutely
> > > > sure they're just fine), I guess his changes must be similar to what I
> > > > sent in the first place.
> > > >     
> > > >         
> > > *Why* did you think you needed them in gdbserver?  Did you actually try
> > > not using them or did you just copy my code from win32-nat.c?
> > >       
> > I thought it natural to implement win32 functionality into gdbserver not 
> > starting from scratch, but from an already working implementation, such as 
> > win32-nat.c, however *truly* one drawback of this is that original code 
> > bugs got inherited into new implementation (such as the subject in 
> > question), and the porting process however was not really as 
> > straightforward as copy/paste; certain amount of work was involved (a lot 
> > of debugging gdbserver and gdb themselves), however I did not stop to think 
> > that SuspendThread was not actually needed, as it was already there.
> >     
>
> So, in answer to my question about whether something was needed or not,
> you asserted that it wasn't needed because you copied the code that I
> wrote which was the source of my question.  Sorry but that doesn't really
> answer my question.
>
>   
Sure I did, however the situation seems a bit more complicated.  I'll 
explain below.




> If Windows already does the SuspendThread on all threads on debug event
> then there is no reason to also do it in gdb.  I was asking if anyone
> had actually checked if that was the case since it has been quite some
> time since I did any experiments.
>
>   


What I said basically is that it is safer to leave the SuspendThread 
there as, besides being safer, it may be required for any future 
enhancement.
Also, upon reception of a Win32 debug event, calling SuspendThread 
should not do any harm, as the threads are already paused.
It is also the basis of the new interrupt method, which is SuspendThread 
based.


In my previous message, in the paragraph I wrote:

"Apart from this, there's also the case where (at least for gdbserver) 
socket data is received asynchronously while the child is running. This 
socket data could indicate gdbserver to set/enable/disable a breakpoint, 
read thread registers, etc., and this kind of things may require to stop 
the child using SuspendThread.
Right?"

I was refering a specific gdbserver functionality that for some reason 
is not currently working for current cvs gdbserver.
In fact right now I do not actually know for sure if this functionality 
ever worked for cvs gdb and got broken, or it is based on local 
modifications I have made to gdbserver.

Please let me clarify:

I have a gdbserver.exe version here that lets you set breakpoints into 
the child while the child is running (without having to interrupt the 
child --if you can, set breakpoint and continue the child).
For this functionality to work socket data availability check must be 
done somewhere in the WaitForDebugEvent loop/call trace.
I think however that what I did was have get_child_debug_event return a 
"spurious" signal periodically to have gdbserver check for this socket 
data and act accordingly to this received data.
The really important thing is that in this case, no Win32 debug event 
was received (and the child was still running), so when gdbserver wanted 
to access the thread, the thread_rec function was ultimately called, 
which itself calls SuspendThread.
So in this case calling SuspendThread *is* necessary.

Now as to the reason why this is not working, I think it's pointless to 
figure why I have a local gdbserver that asynchrounously lets you set 
breakpoints, or try figure out if some older official gdbserver version 
ever had this support.

It is not currently working, and I think it is a very nice feature, not 
only because it actually is useful, but also because people used to 
other debugging systems (such as msvcmon) are used to this.

I think it's best to have it working upstream, and for this, 
SuspendThread is mandatory.
What do you think?

Leo.