Re: [PATCH 3/4] windows-nat: Consistently use numeric get_context parameter to thread_rec()

[Jon TURNEY <jon dot turney at dronecode dot org dot uk>]

On 09/06/2015 20:14, Joel Brobecker wrote:
> > 2015-06-03  Jon Turney  <jon.turney@dronecode.org.uk>
> >
> > 	* windows-nat.c : Consistently use numeric get_context parameter
> > 	to thread_rec() throughout.
>
> I am wondering what others think of this patch.
>
> On the one hand, it seems clearly correct. But on the other hand,
> this is making me think that perhaps thread_rec's "get_context"
> parameter is not very clear. What I would probably do, instead,
> is split that parameter in two, one being a boolean "get_context",
> and the other being a "suspend_thread" boolean.

Thanks for taking the time to review these patches.

Yes, this interface is a bit ad-hoc and far from clear.

> I was looking at the code in gdbserver/win32-low.c, and the code
> is similar, but not quite. There, thread_rec does not have the ability
> to suspend threads. I am not sure whether whether it is an oversight

It seems be be done in win32_require_context()?

> in gdbserver's code or not, but the bottom line with the current
> code is that we wouldn't want to make the same change in gdbserver's
> code as well. As a result and getting back to GDB's windows-nat.c,
> keeping get_context as a boolean, and adding an extra one for
> suspend_thread would allow more similarity between both implementations.
> And given that we are hoping that, one day, we'll be able to merge
> gdbserver's -low.c code with GDB's -nat code, I think similarity is
> important.

It's also probably worth confirming if SuspendThread() is actually 
needed at all, since the debugee is, I think, halted when 
WaitForDebugEvent() returns.

> That's why I would probably suggest the 2-parameters approach over
> the one you've taken here. But I'd like to have other people's opinion
> as well.

It seems like a good idea to me, but somebody has to do it :)