[review] Share Windows thread-suspend and -resume code

Message ID gerrit.1572371871000.I4e2be5d13faae083d9a83e65425c6c05d5d0e353@gnutoolchain-gerrit.osci.io
State New, archived
Headers

Commit Message

Simon Marchi (Code Review) Oct. 29, 2019, 5:57 p.m. UTC
  Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/412
......................................................................

Share Windows thread-suspend and -resume code

This adds "suspend" and "resume" methods to windows_thread_info, and
changes gdb and gdbserver to share this code.

gdb/ChangeLog
2019-10-29  Tom Tromey  <tromey@adacore.com>

	* windows-nat.c (thread_rec): Use windows_thread_info::suspend.
	(windows_continue): Use windows_continue::resume.
	* nat/windows-nat.h (struct windows_thread_info) <suspend,
	resume>: Declare new methods.
	* nat/windows-nat.c: New file.
	* configure.nat (NATDEPFILES): Add nat/windows-nat.o when needed.

gdb/gdbserver/ChangeLog
2019-10-29  Tom Tromey  <tromey@adacore.com>

	* win32-low.c (win32_require_context, suspend_one_thread): Use
	windows_thread_info::suspend.
	(continue_one_thread): Use windows_thread_info::resume.
	* configure.srv (srv_tgtobj): Add windows-nat.o when needed.

Change-Id: I4e2be5d13faae083d9a83e65425c6c05d5d0e353
---
M gdb/ChangeLog
M gdb/configure.nat
M gdb/gdbserver/ChangeLog
M gdb/gdbserver/configure.srv
M gdb/gdbserver/win32-low.c
A gdb/nat/windows-nat.c
M gdb/nat/windows-nat.h
M gdb/windows-nat.c
8 files changed, 98 insertions(+), 62 deletions(-)
  

Comments

Simon Marchi (Code Review) Nov. 4, 2019, 2:30 p.m. UTC | #1
Luis Machado has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/412
......................................................................


Patch Set 1: Code-Review+1

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/412/1/gdb/nat/windows-nat.h 
File gdb/nat/windows-nat.h:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/412/1/gdb/nat/windows-nat.h@43 
PS1, Line 43: 
26 | struct windows_thread_info
   | ...
38 |   void suspend ();
39 | 
40 |   /* Resume the thread if it has been suspended.  */
41 |   void resume ();
42 | 
43 > 
44 |   /* The Win32 thread identifier.  */
45 |   DWORD tid;
46 | 
47 |   /* The handle to the thread.  */
48 |   HANDLE h;

Spurious new line?
  
Simon Marchi (Code Review) Nov. 6, 2019, 7:14 p.m. UTC | #2
Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/412
......................................................................


Patch Set 1:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/412/1/gdb/nat/windows-nat.h 
File gdb/nat/windows-nat.h:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/412/1/gdb/nat/windows-nat.h@43 
PS1, Line 43: 
26 | struct windows_thread_info
   | ...
38 |   void suspend ();
39 | 
40 |   /* Resume the thread if it has been suspended.  */
41 |   void resume ();
42 | 
43 > 
44 |   /* The Win32 thread identifier.  */
45 |   DWORD tid;
46 | 
47 |   /* The handle to the thread.  */
48 |   HANDLE h;

> Spurious new line?

I fixed this locally.
  
Simon Marchi (Code Review) Nov. 29, 2019, 6:08 p.m. UTC | #3
Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/412
......................................................................


Patch Set 2: Code-Review+2

(1 comment)

| --- gdb/nat/windows-nat.h
| +++ gdb/nat/windows-nat.h
| @@ -13,17 +13,19 @@ /* Internal interfaces for the Windows code
|     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
|     GNU General Public License for more details.
|  
|     You should have received a copy of the GNU General Public License
|     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
|  
|  #ifndef NAT_WINDOWS_NAT_H
|  #define NAT_WINDOWS_NAT_H
|  
| +#include <windows.h>

PS2, Line 22:

Nothing in this patch is adding code that requires this, so I wondered
why add it here.  After looking at the file, I realize that I think
this should have been added right when the nat/windows-nat.h was
created, due to use of DWORD, HANDLE, etc.

| +
|  /* Thread information structure used to track extra information about
|     each thread.  */
|  struct windows_thread_info
|  {
|    windows_thread_info (DWORD tid_, HANDLE h_, CORE_ADDR tlb)
|      : tid (tid_),
|        h (h_),
|        thread_local_base (tlb)
  

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index dd9983b..904a322 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,14 @@ 
 2019-10-29  Tom Tromey  <tromey@adacore.com>
 
+	* windows-nat.c (thread_rec): Use windows_thread_info::suspend.
+	(windows_continue): Use windows_continue::resume.
+	* nat/windows-nat.h (struct windows_thread_info) <suspend,
+	resume>: Declare new methods.
+	* nat/windows-nat.c: New file.
+	* configure.nat (NATDEPFILES): Add nat/windows-nat.o when needed.
+
+2019-10-29  Tom Tromey  <tromey@adacore.com>
+
 	* windows-nat.c (windows_add_thread, windows_delete_thread)
 	(windows_nat_target::fetch_registers)
 	(windows_nat_target::store_registers, fake_create_process)
diff --git a/gdb/configure.nat b/gdb/configure.nat
index 77a2ee8..15390e3 100644
--- a/gdb/configure.nat
+++ b/gdb/configure.nat
@@ -75,10 +75,10 @@ 
 	NATDEPFILES='fork-child.o nat/fork-inferior.o inf-ptrace.o'
 	;;
     cygwin*)
-	NATDEPFILES='x86-nat.o nat/x86-dregs.o windows-nat.o'
+	NATDEPFILES='x86-nat.o nat/x86-dregs.o windows-nat.o nat/windows-nat.o'
 	;;
     mingw*)
-	NATDEPFILES='x86-nat.o nat/x86-dregs.o windows-nat.o'
+	NATDEPFILES='x86-nat.o nat/x86-dregs.o windows-nat.o nat/windows-nat.o'
 	;;
     aix)
 	NATDEPFILES='nat/fork-inferior.o fork-child.o inf-ptrace.o'
diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 1358db3..2f31755 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,5 +1,12 @@ 
 2019-10-29  Tom Tromey  <tromey@adacore.com>
 
+	* win32-low.c (win32_require_context, suspend_one_thread): Use
+	windows_thread_info::suspend.
+	(continue_one_thread): Use windows_thread_info::resume.
+	* configure.srv (srv_tgtobj): Add windows-nat.o when needed.
+
+2019-10-29  Tom Tromey  <tromey@adacore.com>
+
 	* win32-i386-low.c (update_debug_registers)
 	(i386_prepare_to_resume, i386_thread_added): Update.
 
diff --git a/gdb/gdbserver/configure.srv b/gdb/gdbserver/configure.srv
index 1a4ab8e..404ed11 100644
--- a/gdb/gdbserver/configure.srv
+++ b/gdb/gdbserver/configure.srv
@@ -63,7 +63,7 @@ 
 			srv_linux_thread_db=yes
 			;;
   arm*-*-mingw32ce*)	srv_regobj=reg-arm.o
-			srv_tgtobj="win32-low.o win32-arm-low.o"
+			srv_tgtobj="win32-low.o windows-nat.o win32-arm-low.o"
 			srv_tgtobj="${srv_tgtobj} wincecompat.o"
 			# hostio_last_error implementation is in win32-low.c
 			srv_hostio_err_objs=""
@@ -86,7 +86,7 @@ 
 			srv_linux_thread_db=yes
 			;;
   i[34567]86-*-cygwin*)	srv_regobj=""
-			srv_tgtobj="x86-low.o x86-dregs.o win32-low.o win32-i386-low.o"
+			srv_tgtobj="x86-low.o x86-dregs.o win32-low.o windows-nat.o win32-i386-low.o"
 			srv_tgtobj="${srv_tgtobj} arch/i386.o"
 			;;
   i[34567]86-*-linux*)	srv_tgtobj="${srv_tgtobj} arch/i386.o"
@@ -108,7 +108,7 @@ 
 			;;
   i[34567]86-*-mingw32ce*)
 			srv_regobj=""
-			srv_tgtobj="x86-low.o x86-dregs.o win32-low.o win32-i386-low.o"
+			srv_tgtobj="x86-low.o x86-dregs.o win32-low.o windows-nat.o win32-i386-low.o"
 			srv_tgtobj="${srv_tgtobj} arch/i386.o"
 			srv_tgtobj="${srv_tgtobj} wincecompat.o"
 			# hostio_last_error implementation is in win32-low.c
@@ -117,7 +117,7 @@ 
 			srv_mingwce=yes
 			;;
   i[34567]86-*-mingw*)	srv_regobj=""
-			srv_tgtobj="x86-low.o x86-dregs.o win32-low.o win32-i386-low.o"
+			srv_tgtobj="x86-low.o x86-dregs.o win32-low.o windows-nat.o win32-i386-low.o"
 			srv_tgtobj="${srv_tgtobj} arch/i386.o"
 			srv_mingw=yes
 			;;
@@ -364,12 +364,12 @@ 
 			ipa_obj="${ipa_obj} arch/amd64-ipa.o"
 			;;
   x86_64-*-mingw*)	srv_regobj=""
-			srv_tgtobj="x86-low.o x86-dregs.o i387-fp.o win32-low.o win32-i386-low.o"
+			srv_tgtobj="x86-low.o x86-dregs.o i387-fp.o win32-low.o windows-nat.o win32-i386-low.o"
 			srv_tgtobj="${srv_tgtobj} arch/amd64.o"
 			srv_mingw=yes
 			;;
   x86_64-*-cygwin*)	srv_regobj=""
-			srv_tgtobj="x86-low.o x86-dregs.o i387-fp.o win32-low.o win32-i386-low.o"
+			srv_tgtobj="x86-low.o x86-dregs.o i387-fp.o win32-low.o windows-nat.o win32-i386-low.o"
 			srv_tgtobj="${srv_tgtobj} arch/amd64.o"
 			;;
 
diff --git a/gdb/gdbserver/win32-low.c b/gdb/gdbserver/win32-low.c
index 7e3afff..59749f0 100644
--- a/gdb/gdbserver/win32-low.c
+++ b/gdb/gdbserver/win32-low.c
@@ -172,18 +172,7 @@ 
 {
   if (th->context.ContextFlags == 0)
     {
-      if (!th->suspended)
-	{
-	  if (SuspendThread (th->h) == (DWORD) -1)
-	    {
-	      DWORD err = GetLastError ();
-	      OUTMSG (("warning: SuspendThread failed in thread_rec, "
-		       "(error %d): %s\n", (int) err, strwinerror (err)));
-	    }
-	  else
-	    th->suspended = 1;
-	}
-
+      th->suspend ();
       win32_get_thread_context (th);
     }
 }
@@ -436,13 +425,7 @@ 
 	      th->context.ContextFlags = 0;
 	    }
 
-	  if (ResumeThread (th->h) == (DWORD) -1)
-	    {
-	      DWORD err = GetLastError ();
-	      OUTMSG (("warning: ResumeThread failed in continue_one_thread, "
-		       "(error %d): %s\n", (int) err, strwinerror (err)));
-	    }
-	  th->suspended = 0;
+	  th->resume ();
 	}
     }
 }
@@ -1334,17 +1317,7 @@ 
 {
   windows_thread_info *th = (windows_thread_info *) thread_target_data (thread);
 
-  if (!th->suspended)
-    {
-      if (SuspendThread (th->h) == (DWORD) -1)
-	{
-	  DWORD err = GetLastError ();
-	  OUTMSG (("warning: SuspendThread failed in suspend_one_thread, "
-		   "(error %d): %s\n", (int) err, strwinerror (err)));
-	}
-      else
-	th->suspended = 1;
-    }
+  th->suspend ();
 }
 
 static void
diff --git a/gdb/nat/windows-nat.c b/gdb/nat/windows-nat.c
new file mode 100644
index 0000000..a98ff42
--- /dev/null
+++ b/gdb/nat/windows-nat.c
@@ -0,0 +1,60 @@ 
+/* Internal interfaces for the Windows code
+   Copyright (C) 1995-2019 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "gdbsupport/common-defs.h"
+#include "nat/windows-nat.h"
+
+void
+windows_thread_info::suspend ()
+{
+  if (suspended != 0)
+    return;
+
+  if (SuspendThread (h) == (DWORD) -1)
+    {
+      DWORD err = GetLastError ();
+
+      /* 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.
+	 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) tid, (unsigned) err);
+      suspended = -1;
+    }
+  else
+    suspended = 1;
+}
+
+void
+windows_thread_info::resume ()
+{
+  if (suspended > 0)
+    {
+      if (ResumeThread (h) == (DWORD) -1)
+	{
+	  DWORD err = GetLastError ();
+	  warning (_("warning: ResumeThread (tid=0x%x) failed. (winerr %u)"),
+		   (unsigned) tid, (unsigned) err);
+	}
+    }
+  suspended = 0;
+}
diff --git a/gdb/nat/windows-nat.h b/gdb/nat/windows-nat.h
index 06c6486..b51279b 100644
--- a/gdb/nat/windows-nat.h
+++ b/gdb/nat/windows-nat.h
@@ -19,6 +19,8 @@ 
 #ifndef NAT_WINDOWS_NAT_H
 #define NAT_WINDOWS_NAT_H
 
+#include <windows.h>
+
 /* Thread information structure used to track extra information about
    each thread.  */
 struct windows_thread_info
@@ -32,6 +34,13 @@ 
 
   DISABLE_COPY_AND_ASSIGN (windows_thread_info);
 
+  /* Ensure that this thread has been suspended.  */
+  void suspend ();
+
+  /* Resume the thread if it has been suspended.  */
+  void resume ();
+
+
   /* The Win32 thread identifier.  */
   DWORD tid;
 
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index b7c8ae5..26e5343 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -377,27 +377,7 @@ 
 	if (!th->suspended && get_context)
 	  {
 	    if (get_context > 0 && id != current_event.dwThreadId)
-	      {
-		if (SuspendThread (th->h) == (DWORD) -1)
-		  {
-		    DWORD err = GetLastError ();
-
-		    /* 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.
-		       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);
-		    th->suspended = -1;
-		  }
-		else
-		  th->suspended = 1;
-	      }
+	      th->suspend ();
 	    else if (get_context < 0)
 	      th->suspended = -1;
 	    th->reload_context = true;
@@ -1329,9 +1309,7 @@ 
 	      }
 	    th->context.ContextFlags = 0;
 	  }
-	if (th->suspended > 0)
-	  (void) ResumeThread (th->h);
-	th->suspended = 0;
+	th->resume ();
       }
 
   res = ContinueDebugEvent (current_event.dwProcessId,