[review] Add pending stop support to gdbserver's Windows port

[review] Add pending stop support to gdbserver's Windows port

[Tom Tromey (Code Review)]

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

Add pending stop support to gdbserver's Windows port

This changes gdbserver to also handle pending stops, the same way that
gdb does.  This is PR gdb/22992.

2019-11-26  Tom Tromey  <tromey@adacore.com>

	PR gdb/22992
	* win32-low.c (child_continue): Call matching_pending_stop.
	(get_child_debug_event): Call fetch_pending_stop.  Push pending
	stop when needed.

Change-Id: I8d2d24a55dc081887deebdd952dd6512ae23fa71
---
M gdb/gdbserver/ChangeLog
M gdb/gdbserver/win32-low.c
2 files changed, 38 insertions(+), 3 deletions(-)



diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 9008bdc..a076a40 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,5 +1,12 @@
 2019-11-26  Tom Tromey  <tromey@adacore.com>
 
+	PR gdb/22992
+	* win32-low.c (child_continue): Call matching_pending_stop.
+	(get_child_debug_event): Call fetch_pending_stop.  Push pending
+	stop when needed.
+
+2019-11-26  Tom Tromey  <tromey@adacore.com>
+
 	* win32-low.c (win32_supports_z_point_type): Always handle
 	Z_PACKET_SW_BP.
 	(win32_insert_point): Call insert_memory_breakpoint when needed.
diff --git a/gdb/gdbserver/win32-low.c b/gdb/gdbserver/win32-low.c
index c472e0e..f85fa23 100644
--- a/gdb/gdbserver/win32-low.c
+++ b/gdb/gdbserver/win32-low.c
@@ -430,6 +430,10 @@
 static BOOL
 child_continue (DWORD continue_status, int thread_id)
 {
+  desired_stop_thread_id = thread_id;
+  if (matching_pending_stop (debug_threads))
+    return TRUE;
+
   /* The inferior will only continue after the ContinueDebugEvent
      call.  */
   for_each_thread ([&] (thread_info *thread)
@@ -1261,6 +1265,16 @@
   else
 #endif
     {
+      gdb::optional<pending_stop> stop = fetch_pending_stop (debug_threads);
+      if (stop.has_value ())
+	{
+	  *ourstatus = stop->status;
+	  current_event = stop->event;
+	  ptid = debug_event_ptid (&current_event);
+	  current_thread = find_thread_ptid (ptid);
+	  return 1;
+	}
+
       /* Keep the wait time low enough for comfortable remote
 	 interruption, but high enough so gdbserver doesn't become a
 	 bottleneck.  */
@@ -1348,7 +1362,7 @@
 		(unsigned) current_event.dwThreadId));
       ourstatus->kind = TARGET_WAITKIND_EXITED;
       ourstatus->value.integer = current_event.u.ExitProcess.dwExitCode;
-      child_continue (DBG_CONTINUE, -1);
+      child_continue (DBG_CONTINUE, desired_stop_thread_id);
       CloseHandle (current_process_handle);
       current_process_handle = NULL;
       break;
@@ -1408,7 +1422,21 @@
     }
 
   ptid = debug_event_ptid (&current_event);
-  current_thread = find_thread_ptid (ptid);
+
+  if (desired_stop_thread_id != -1 && desired_stop_thread_id != ptid.lwp ())
+    {
+      /* Pending stop.  See the comment by the definition of
+	 "pending_stops" for details on why this is needed.  */
+      OUTMSG2 (("get_windows_debug_event - "
+		"unexpected stop in 0x%x (expecting 0x%x)\n",
+		ptid.lwp (), desired_stop_thread_id));
+      maybe_adjust_pc ();
+      pending_stops.push_back ({(DWORD) ptid.lwp (), *ourstatus, current_event});
+      ourstatus->kind = TARGET_WAITKIND_SPURIOUS;
+    }
+  else
+    current_thread = find_thread_ptid (ptid);
+
   return 1;
 }
 
@@ -1455,7 +1483,7 @@
 	  /* fall-through */
 	case TARGET_WAITKIND_SPURIOUS:
 	  /* do nothing, just continue */
-	  child_continue (continue_status, -1);
+	  child_continue (continue_status, desired_stop_thread_id);
 	  break;
 	}
     }

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I8d2d24a55dc081887deebdd952dd6512ae23fa71
Gerrit-Change-Number: 724
Gerrit-PatchSet: 1
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: newchange

[review] Add pending stop support to gdbserver's Windows port

[Pedro Alves (Code Review)]

Pedro Alves has posted comments on this change.

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


Patch Set 1: Code-Review-1

(1 comment)

The comments to the gdb equivalent bits apply here as well, so I'll skip repeating them.

Maybe adding software breakpoints support could be split to a separate patch, for bisecting reasons, since that's a change that could plausibly uncover issues on its own.

| --- /dev/null
| +++ /COMMIT_MSG
| @@ -1,0 +4,19 @@ AuthorDate: 2019-10-28 10:54:34 -0600
| +Commit:     Tom Tromey <tromey@adacore.com>
| +CommitDate: 2019-10-29 10:08:42 -0600
| +
| +Add pending stop support to gdbserver's Windows port
| +
| +This changes gdbserver to also handle pending stops, the same way that
| +gdb does.  This is PR gdb/22992.
| +
| +Doing this required changing win32-low.c to use memory breakpoints and
| +to be implement the stopped_by_sw_breakpoint method.

PS1, Line 13:

"to be implement" -> "to implement"

| +
| +Change-Id: I8d2d24a55dc081887deebdd952dd6512ae23fa71
| +
| +gdb/gdbserver/ChangeLog
| +2019-10-29  Tom Tromey  <tromey@adacore.com>
| +
| +	PR gdb/22992
| +	* win32-low.c (win32_supports_z_point_type): Always handle
| +	Z_PACKET_SW_BP.

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I74edd5a8dddb98429262599a8c3d4f9e16e7f770
Gerrit-Change-Number: 432
Gerrit-PatchSet: 1
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-CC: Joel Brobecker <brobecker@adacore.com>
Gerrit-Comment-Date: Thu, 21 Nov 2019 18:44:45 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[review] Add pending stop support to gdbserver's Windows port

[Joel Brobecker (Code Review)]

Joel Brobecker has posted comments on this change.

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


Patch Set 1:

Hey Pedro; is that something you would mind reviewing?

Thanks!


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I74edd5a8dddb98429262599a8c3d4f9e16e7f770
Gerrit-Change-Number: 432
Gerrit-PatchSet: 1
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-CC: Joel Brobecker <brobecker@adacore.com>
Gerrit-Comment-Date: Mon, 11 Nov 2019 17:35:59 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review] Add pending stop support to gdbserver's Windows port

[Tom Tromey (Code Review)]

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

Add pending stop support to gdbserver's Windows port

This changes gdbserver to also handle pending stops, the same way that
gdb does.  This is PR gdb/22992.

Doing this required changing win32-low.c to use memory breakpoints and
to be implement the stopped_by_sw_breakpoint method.

Change-Id: I8d2d24a55dc081887deebdd952dd6512ae23fa71

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

	PR gdb/22992
	* win32-low.c (win32_supports_z_point_type): Always handle
	Z_PACKET_SW_BP.
	(win32_insert_point): Call insert_memory_breakpoint when needed.
	(win32_remove_point): Call remove_memory_breakpoint when needed.
	(child_continue): Call matching_pending_stop.
	(get_child_debug_event): Call fetch_pending_stop.  Push pending
	stop when needed.
	(win32_wait): Set last_stop_was_breakpoint.
	(win32_stopped_by_sw_breakpoint)
	(win32_supports_stopped_by_sw_breakpoint): New functions.
	(win32_target_ops): Update.

Change-Id: I74edd5a8dddb98429262599a8c3d4f9e16e7f770
---
M gdb/gdbserver/ChangeLog
M gdb/gdbserver/win32-low.c
2 files changed, 91 insertions(+), 9 deletions(-)



diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index a6896d6..d4c68ad 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,5 +1,20 @@
 2019-10-29  Tom Tromey  <tromey@adacore.com>
 
+	PR gdb/22992
+	* win32-low.c (win32_supports_z_point_type): Always handle
+	Z_PACKET_SW_BP.
+	(win32_insert_point): Call insert_memory_breakpoint when needed.
+	(win32_remove_point): Call remove_memory_breakpoint when needed.
+	(child_continue): Call matching_pending_stop.
+	(get_child_debug_event): Call fetch_pending_stop.  Push pending
+	stop when needed.
+	(win32_wait): Set last_stop_was_breakpoint.
+	(win32_stopped_by_sw_breakpoint)
+	(win32_supports_stopped_by_sw_breakpoint): New functions.
+	(win32_target_ops): Update.
+
+2019-10-29  Tom Tromey  <tromey@adacore.com>
+
 	* win32-low.h (struct win32_target_ops) <decr_pc_after_break>: New
 	field.
 	* win32-i386-low.c (the_low_target): Update.
diff --git a/gdb/gdbserver/win32-low.c b/gdb/gdbserver/win32-low.c
index 14bd79f..76724d8 100644
--- a/gdb/gdbserver/win32-low.c
+++ b/gdb/gdbserver/win32-low.c
@@ -236,15 +236,18 @@
 static int
 win32_supports_z_point_type (char z_type)
 {
-  return (the_low_target.supports_z_point_type != NULL
-	  && the_low_target.supports_z_point_type (z_type));
+  return (z_type == Z_PACKET_SW_BP
+	  || (the_low_target.supports_z_point_type != NULL
+	      && the_low_target.supports_z_point_type (z_type)));
 }
 
 static int
 win32_insert_point (enum raw_bkpt_type type, CORE_ADDR addr,
 		    int size, struct raw_breakpoint *bp)
 {
-  if (the_low_target.insert_point != NULL)
+  if (type == raw_bkpt_type_sw)
+    return insert_memory_breakpoint (bp);
+  else if (the_low_target.insert_point != NULL)
     return the_low_target.insert_point (type, addr, size, bp);
   else
     /* Unsupported (see target.h).  */
@@ -255,7 +258,9 @@
 win32_remove_point (enum raw_bkpt_type type, CORE_ADDR addr,
 		    int size, struct raw_breakpoint *bp)
 {
-  if (the_low_target.remove_point != NULL)
+  if (type == raw_bkpt_type_sw)
+    return remove_memory_breakpoint (bp);
+  else if (the_low_target.remove_point != NULL)
     return the_low_target.remove_point (type, addr, size, bp);
   else
     /* Unsupported (see target.h).  */
@@ -425,6 +430,10 @@
 static BOOL
 child_continue (DWORD continue_status, int thread_id)
 {
+  desired_stop_thread_id = thread_id;
+  if (matching_pending_stop (debug_threads))
+    return TRUE;
+
   /* The inferior will only continue after the ContinueDebugEvent
      call.  */
   for_each_thread ([&] (thread_info *thread)
@@ -1231,6 +1240,16 @@
   else
 #endif
     {
+      pending_stop stop;
+      if (fetch_pending_stop (debug_threads, &stop))
+	{
+	  *ourstatus = stop.status;
+	  current_event = stop.event;
+	  ptid = debug_event_ptid (&current_event);
+	  current_thread = find_thread_ptid (ptid);
+	  return 1;
+	}
+
       /* Keep the wait time low enough for comfortable remote
 	 interruption, but high enough so gdbserver doesn't become a
 	 bottleneck.  */
@@ -1318,7 +1337,7 @@
 		(unsigned) current_event.dwThreadId));
       ourstatus->kind = TARGET_WAITKIND_EXITED;
       ourstatus->value.integer = current_event.u.ExitProcess.dwExitCode;
-      child_continue (DBG_CONTINUE, -1);
+      child_continue (DBG_CONTINUE, desired_stop_thread_id);
       CloseHandle (current_process_handle);
       current_process_handle = NULL;
       break;
@@ -1378,7 +1397,21 @@
     }
 
   ptid = debug_event_ptid (&current_event);
-  current_thread = find_thread_ptid (ptid);
+
+  if (desired_stop_thread_id != -1 && desired_stop_thread_id != ptid.lwp ())
+    {
+      /* Pending stop.  See the comment by the definition of
+	 "pending_stops" for details on why this is needed.  */
+      OUTMSG2 (("get_windows_debug_event - "
+		"unexpected stop in 0x%x (expecting 0x%x)\n",
+		ptid.lwp (), desired_stop_thread_id));
+
+      pending_stops.push_back ({(DWORD) ptid.lwp (), *ourstatus, current_event});
+      ourstatus->kind = TARGET_WAITKIND_SPURIOUS;
+    }
+  else
+    current_thread = find_thread_ptid (ptid);
+
   return 1;
 }
 
@@ -1421,13 +1454,29 @@
 
 	  regcache = get_thread_regcache (current_thread, 1);
 	  child_fetch_inferior_registers (regcache, -1);
+
+	  last_stop_was_breakpoint = false;
+	  if (current_event.dwDebugEventCode == EXCEPTION_DEBUG_EVENT
+	      && (current_event.u.Exception.ExceptionRecord.ExceptionCode
+		  == EXCEPTION_BREAKPOINT))
+	    {
+	      CORE_ADDR pc = regcache_read_pc (regcache);
+	      CORE_ADDR sw_breakpoint_pc
+		= pc - the_low_target.decr_pc_after_break;
+	      if (gdb_breakpoint_here (sw_breakpoint_pc))
+		{
+		  last_stop_was_breakpoint = true;
+		  regcache_write_pc (regcache, sw_breakpoint_pc);
+		}
+	    }
+
 	  return debug_event_ptid (&current_event);
 	default:
 	  OUTMSG (("Ignoring unknown internal event, %d\n", ourstatus->kind));
 	  /* fall-through */
 	case TARGET_WAITKIND_SPURIOUS:
 	  /* do nothing, just continue */
-	  child_continue (continue_status, -1);
+	  child_continue (continue_status, desired_stop_thread_id);
 	  break;
 	}
     }
@@ -1585,6 +1634,24 @@
   return the_low_target.breakpoint;
 }
 
+/* Implementation of the target_ops method
+   "stopped_by_sw_breakpoint".  */
+
+static int
+win32_stopped_by_sw_breakpoint ()
+{
+  return last_stop_was_breakpoint;
+}
+
+/* Implementation of the target_ops method
+   "supports_stopped_by_sw_breakpoint".  */
+
+static int
+win32_supports_stopped_by_sw_breakpoint ()
+{
+  return 1;
+}
+
 /* Implementation of the target_ops method "read_pc".  */
 
 static CORE_ADDR
@@ -1624,8 +1691,8 @@
   win32_supports_z_point_type,
   win32_insert_point,
   win32_remove_point,
-  NULL, /* stopped_by_sw_breakpoint */
-  NULL, /* supports_stopped_by_sw_breakpoint */
+  win32_stopped_by_sw_breakpoint,
+  win32_supports_stopped_by_sw_breakpoint,
   NULL, /* stopped_by_hw_breakpoint */
   NULL, /* supports_stopped_by_hw_breakpoint */
   target_can_do_hardware_single_step,

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I74edd5a8dddb98429262599a8c3d4f9e16e7f770
Gerrit-Change-Number: 432
Gerrit-PatchSet: 1
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: newchange