[3/4] windows-nat: Consistently use numeric get_context parameter to thread_rec()
Commit Message
thread_rec()'s get_context parameter is not a bool, and has the following
meanings
>0 suspend, retrieve context
0 use already retrieved context
<0 already suspended, retrieve context
Consistently use numeric values throughout windows-nat.c, rather than a mixture
of numeric and bool values.
gdb/ChangeLog:
2015-06-03 Jon Turney <jon.turney@dronecode.org.uk>
* windows-nat.c : Consistently use numeric get_context parameter
to thread_rec() throughout.
Signed-off-by: Jon Turney <jon.turney@dronecode.org.uk>
---
gdb/ChangeLog | 5 +++++
gdb/windows-nat.c | 12 ++++++------
2 files changed, 11 insertions(+), 6 deletions(-)
Comments
> 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.
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
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.
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.
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 :)
@@ -1,5 +1,10 @@
2015-06-03 Jon Turney <jon.turney@dronecode.org.uk>
+ * windows-nat.c : Consistently use numeric get_context parameter
+ to thread_rec() throughout.
+
+2015-06-03 Jon Turney <jon.turney@dronecode.org.uk>
+
* windows-nat.c (do_windows_fetch_inferior_registers)
(handle_output_debug_string): Replace __COPY_CONTEXT_SIZE
conditional with __CYGWIN__.
@@ -341,7 +341,7 @@ windows_add_thread (ptid_t ptid, HANDLE h, void *tlb)
id = ptid_get_tid (ptid);
- if ((th = thread_rec (id, FALSE)))
+ if ((th = thread_rec (id, 0)))
return th;
th = XCNEW (windows_thread_info);
@@ -496,7 +496,7 @@ static void
windows_fetch_inferior_registers (struct target_ops *ops,
struct regcache *regcache, int r)
{
- current_thread = thread_rec (ptid_get_tid (inferior_ptid), TRUE);
+ current_thread = thread_rec (ptid_get_tid (inferior_ptid), 1);
/* Check if current_thread exists. Windows sometimes uses a non-existent
thread id in its events. */
if (current_thread)
@@ -523,7 +523,7 @@ static void
windows_store_inferior_registers (struct target_ops *ops,
struct regcache *regcache, int r)
{
- current_thread = thread_rec (ptid_get_tid (inferior_ptid), TRUE);
+ current_thread = thread_rec (ptid_get_tid (inferior_ptid), 1);
/* Check if current_thread exists. Windows sometimes uses a non-existent
thread id in its events. */
if (current_thread)
@@ -1252,7 +1252,7 @@ windows_resume (struct target_ops *ops,
ptid_get_pid (ptid), ptid_get_tid (ptid), step, sig));
/* Get context for currently selected thread. */
- th = thread_rec (ptid_get_tid (inferior_ptid), FALSE);
+ th = thread_rec (ptid_get_tid (inferior_ptid), 0);
if (th)
{
if (step)
@@ -1513,7 +1513,7 @@ get_windows_debug_event (struct target_ops *ops,
thread_id);
current_thread = th;
if (!current_thread)
- current_thread = thread_rec (thread_id, TRUE);
+ current_thread = thread_rec (thread_id, 1);
}
out:
@@ -2667,7 +2667,7 @@ windows_thread_alive (struct target_ops *ops, ptid_t ptid)
gdb_assert (ptid_get_tid (ptid) != 0);
tid = ptid_get_tid (ptid);
- return WaitForSingleObject (thread_rec (tid, FALSE)->h, 0) == WAIT_OBJECT_0
+ return WaitForSingleObject (thread_rec (tid, 0)->h, 0) == WAIT_OBJECT_0
? FALSE : TRUE;
}