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

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

[Jon Turney]

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

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;
 }
 
-- 
2.1.4

[PATCH 4/4] windows-nat: Also ignore ERROR_INVALID_HANDLE from SuspendThread()

[Jon Turney]

Discussed somewhat in the thread at
https://cygwin.com/ml/gdb-patches/2013-06/msg00680.html

This is pretty straightforward to demonstrate on Cygwin currently:

$ cat main.c

int main()
{
  return 0;
}

$ gcc -g -O0 main.c -o main

$ ./gdb ./main
[...]
(gdb) r
Starting program: /wip/binutils-gdb/build.x86_64/gdb/main
warning: SuspendThread (tid=0x1cf0) failed. (winerr 6)
[Inferior 1 (process 976) exited normally]

with this patch applied:

$ ./gdb ./main
[...]
(gdb) r
Starting program: /wip/binutils-gdb/build.x86_64/gdb/main
[Inferior 1 (process 4852) exited normally]

gdb/ChangeLog:

2015-06-03  Jon Turney  <jon.turney@dronecode.org.uk>

	* windows-nat.c (thread_rec): Also ignore ERROR_INVALID_HANDLE
	from SuspendThread().

Signed-off-by: Jon Turney <jon.turney@dronecode.org.uk>
---
 gdb/ChangeLog     | 5 +++++
 gdb/windows-nat.c | 7 +++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 996dffe..eddcf4d 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2015-06-03  Jon Turney  <jon.turney@dronecode.org.uk>
 
+	* windows-nat.c (thread_rec): Also ignore ERROR_INVALID_HANDLE
+	from SuspendThread().
+
+2015-06-03  Jon Turney  <jon.turney@dronecode.org.uk>
+
 	* windows-nat.c : Consistently use numeric get_context parameter
 	to thread_rec() throughout.
 
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index ce1513f..75d9414 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -310,8 +310,11 @@ thread_rec (DWORD id, int get_context)
 		    /* We get Access Denied (5) when trying to suspend
 		       threads that Windows started on behalf of the
 		       debuggee, usually when those threads are just
-		       about to exit.  */
-		    if (err != ERROR_ACCESS_DENIED)
+		       about to exit.
+		       We can get Invalid Handle (6) if the main thread
+		       has exited. */
+		    if (err != ERROR_INVALID_HANDLE
+			&& err != ERROR_ACCESS_DENIED)
 		      warning (_("SuspendThread (tid=0x%x) failed."
 				 " (winerr %u)"),
 			       (unsigned) id, (unsigned) err);
-- 
2.1.4

[PATCH 0/4] Various Cygwin patches

[Jon Turney]

Various patches, originally by cgf, which have been carried in Cygwin's gdb
package for a few years.  I've cleaned them up a bit and revised them for
master.

Tested on x86_64-pc-cygwin.

Jon Turney (4):
  windows-nat: Trim a trailing '\n' from OutputDebugString before
    echoing it
  windows-nat: Replace __COPY_CONTEXT_SIZE conditional with __CYGWIN__
  windows-nat: Consistently use numeric get_context parameter to
    thread_rec()
  windows-nat: Also ignore ERROR_INVALID_HANDLE from SuspendThread()

 gdb/ChangeLog     | 21 +++++++++++++++++++++
 gdb/windows-nat.c | 30 +++++++++++++++++++-----------
 2 files changed, 40 insertions(+), 11 deletions(-)

-- 
2.1.4

[PATCH 1/4] windows-nat: Trim a trailing '\n' from OutputDebugString before echoing it

[Jon Turney]

For cosmetic purposes, trim a trailing '\n' from OutputDebugString before
echoing it, as warning() will add a '\n', anyhow.

gdb/ChangeLog:

2015-06-03  Jon Turney  <jon.turney@dronecode.org.uk>

	* windows-nat.c (handle_output_debug_string): Trim trailing '\n'
	from OutputDebugString.

Signed-off-by: Jon Turney <jon.turney@dronecode.org.uk>
---
 gdb/ChangeLog     | 5 +++++
 gdb/windows-nat.c | 7 ++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 37d619b..ee9f1df 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2015-06-03  Jon Turney  <jon.turney@dronecode.org.uk>
+
+	* windows-nat.c (handle_output_debug_string): Trim trailing '\n'
+	from OutputDebugString.
+
 2015-06-02  Simon Marchi  <simon.marchi@ericsson.com>
 
 	PR gdb/15564
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 701d2c5..b56b916 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -818,7 +818,12 @@ handle_output_debug_string (struct target_waitstatus *ourstatus)
 #ifdef __CYGWIN__
       if (!startswith (s, "cYg"))
 #endif
-	warning (("%s"), s);
+	{
+	  char *p = strchr (s, '\0');
+	  if (p > s && *--p == '\n')
+	    *p = '\0';
+	  warning (("%s"), s);
+	}
     }
 #ifdef __COPY_CONTEXT_SIZE
   else
-- 
2.1.4

[PATCH 1/4] windows-nat: Trim a trailing '\n's from OutputDebugString before echoing it

[Jon Turney]

For cosmetic purposes, trim a trailing '\n' from OutputDebugString before
echoing it, as warning() will add a '\n', anyhow.

gdb/ChangeLog:

2015-06-03  Jon Turney  <jon.turney@dronecode.org.uk>

	* windows-nat.c (handle_output_debug_string): Trim trailing '\n'
	from OutputDebugString.

Signed-off-by: Jon Turney <jon.turney@dronecode.org.uk>
---
 gdb/ChangeLog     | 5 +++++
 gdb/windows-nat.c | 7 ++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 37d619b..ee9f1df 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2015-06-03  Jon Turney  <jon.turney@dronecode.org.uk>
+
+	* windows-nat.c (handle_output_debug_string): Trim trailing '\n'
+	from OutputDebugString.
+
 2015-06-02  Simon Marchi  <simon.marchi@ericsson.com>
 
 	PR gdb/15564
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 701d2c5..b56b916 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -818,7 +818,12 @@ handle_output_debug_string (struct target_waitstatus *ourstatus)
 #ifdef __CYGWIN__
       if (!startswith (s, "cYg"))
 #endif
-	warning (("%s"), s);
+	{
+	  char *p = strchr (s, '\0');
+	  if (p > s && *--p == '\n')
+	    *p = '\0';
+	  warning (("%s"), s);
+	}
     }
 #ifdef __COPY_CONTEXT_SIZE
   else
-- 
2.1.4

[PATCH 2/4] windows-nat: Replace __COPY_CONTEXT_SIZE conditional with __CYGWIN__

[Jon Turney]

Replace __COPY_CONTEXT_SIZE conditional with __CYGWIN__

__COPY_CONTEXT_SIZE was added to Cygwin's headers in 2006.

Versions of Cygwin which don't define __COPY_CONTEXT_SIZE are long obsolete.

Also see the thread starting at
https://sourceware.org/ml/gdb-patches/2015-03/msg00989.html for some discussion

Note that __COPY_CONTEXT_SIZE should just be sizeof(CONTEXT) (which is a
platform constant), but isn't due to historical mistakes in Cygwin headers.

gdb/ChangeLog:

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

Signed-off-by: Jon Turney <jon.turney@dronecode.org.uk>
---
 gdb/ChangeLog     | 6 ++++++
 gdb/windows-nat.c | 4 ++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index ee9f1df..d170f7c 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,11 @@
 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__.
+
+2015-06-03  Jon Turney  <jon.turney@dronecode.org.uk>
+
 	* windows-nat.c (handle_output_debug_string): Trim trailing '\n'
 	from OutputDebugString.
 
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index b56b916..9d2e3c2 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -432,7 +432,7 @@ do_windows_fetch_inferior_registers (struct regcache *regcache, int r)
 
   if (current_thread->reload_context)
     {
-#ifdef __COPY_CONTEXT_SIZE
+#ifdef __CYGWIN__
       if (have_saved_context)
 	{
 	  /* Lie about where the program actually is stopped since
@@ -825,7 +825,7 @@ handle_output_debug_string (struct target_waitstatus *ourstatus)
 	  warning (("%s"), s);
 	}
     }
-#ifdef __COPY_CONTEXT_SIZE
+#ifdef __CYGWIN__
   else
     {
       /* Got a cygwin signal marker.  A cygwin signal is followed by
-- 
2.1.4

Re: [PATCH 1/4] windows-nat: Trim a trailing '\n' from OutputDebugString before echoing it

[Joel Brobecker]

> 2015-06-03  Jon Turney  <jon.turney@dronecode.org.uk>
> 
> 	* windows-nat.c (handle_output_debug_string): Trim trailing '\n'
> 	from OutputDebugString.

Pre-approved with one formatting nit.

> -	warning (("%s"), s);
> +	{
> +	  char *p = strchr (s, '\0');
> +	  if (p > s && *--p == '\n')
> +	    *p = '\0';
> +	  warning (("%s"), s);
> +	}

Empty line between local variable declarations and the first statement
after that. So empty line after "char *p = [...]".

Thanks,
-- 
Joel

Re: [PATCH 2/4] windows-nat: Replace __COPY_CONTEXT_SIZE conditional with __CYGWIN__

[Joel Brobecker]

> Replace __COPY_CONTEXT_SIZE conditional with __CYGWIN__
> 
> __COPY_CONTEXT_SIZE was added to Cygwin's headers in 2006.
> 
> Versions of Cygwin which don't define __COPY_CONTEXT_SIZE are long obsolete.
> 
> Also see the thread starting at
> https://sourceware.org/ml/gdb-patches/2015-03/msg00989.html for some discussion
> 
> Note that __COPY_CONTEXT_SIZE should just be sizeof(CONTEXT) (which is a
> platform constant), but isn't due to historical mistakes in Cygwin headers.
> 
> gdb/ChangeLog:
> 
> 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__.

FWIW, I don't mind this patch, but I don't necessarily see what
we are gaining from it. Can you explain?

Thanks,
-- 
Joel

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

[Joel Brobecker]

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

-- 
Joel

Re: [PATCH 4/4] windows-nat: Also ignore ERROR_INVALID_HANDLE from SuspendThread()

[Joel Brobecker]

> 2015-06-03  Jon Turney  <jon.turney@dronecode.org.uk>
> 
> 	* windows-nat.c (thread_rec): Also ignore ERROR_INVALID_HANDLE
> 	from SuspendThread().

OK to push, after one slight formatting nit gets fixed.

> -		    if (err != ERROR_ACCESS_DENIED)
> +		       about to exit.
> +		       We can get Invalid Handle (6) if the main thread
> +		       has exited. */

GNU Coding Style: 2 spacces after a period.

Thank you,
-- 
Joel

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

[Jon TURNEY]

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

Re: [PATCH 2/4] windows-nat: Replace __COPY_CONTEXT_SIZE conditional with __CYGWIN__

[Jon TURNEY]

On 09/06/2015 19:55, Joel Brobecker wrote:
>> Replace __COPY_CONTEXT_SIZE conditional with __CYGWIN__
>>
>> __COPY_CONTEXT_SIZE was added to Cygwin's headers in 2006.
>>
>> Versions of Cygwin which don't define __COPY_CONTEXT_SIZE are long obsolete.
>>
>> Also see the thread starting at
>> https://sourceware.org/ml/gdb-patches/2015-03/msg00989.html for some discussion
>>
>> Note that __COPY_CONTEXT_SIZE should just be sizeof(CONTEXT) (which is a
>> platform constant), but isn't due to historical mistakes in Cygwin headers.
>>
>> gdb/ChangeLog:
>>
>> 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__.
>
> FWIW, I don't mind this patch, but I don't necessarily see what
> we are gaining from it. Can you explain?

There are still some problems with Cygwin signal handling [1], which I 
hope to fix, and this is a bit of clean-up preparatory to that.

This also makes it clear that the code which uses __COPY_CONTEXT_SIZE is 
Cygwin specific.

[1] https://sourceware.org/ml/gdb-patches/2015-06/msg00180.html

Re: [PATCH 2/4] windows-nat: Replace __COPY_CONTEXT_SIZE conditional with __CYGWIN__

[Joel Brobecker]

> >>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__.
> >
> >FWIW, I don't mind this patch, but I don't necessarily see what
> >we are gaining from it. Can you explain?
> 
> There are still some problems with Cygwin signal handling [1], which I hope
> to fix, and this is a bit of clean-up preparatory to that.
> 
> This also makes it clear that the code which uses __COPY_CONTEXT_SIZE is
> Cygwin specific.
> 
> [1] https://sourceware.org/ml/gdb-patches/2015-06/msg00180.html

OK, patch is approved.

Thank you,
-- 
Joel