diff mbox

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

Message ID 1433352592-9728-5-git-send-email-jon.turney@dronecode.org.uk
State New
Headers show

Commit Message

Jon TURNEY June 3, 2015, 5:29 p.m. UTC
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

Joel Brobecker June 9, 2015, 7:14 p.m. UTC | #1
> 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.
Jon TURNEY June 10, 2015, 1:13 p.m. UTC | #2
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 :)
diff mbox

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index d170f7c..996dffe 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -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__.
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 9d2e3c2..ce1513f 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -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;
 }