[review] Handle pending stops from the Windows kernel

[review] Handle pending stops from the Windows kernel

[Tom Tromey (Code Review)]

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

Handle pending stops from the Windows kernel

PR gdb/22992 concerns an assertion failure in gdb when debugging a
certain inferior:

    int finish_step_over(execution_control_state*): Assertion `ecs->event_thread->control.trap_expected' failed.

Initially the investigation centered on the discovery that gdb was not
suspending other threads when attempting to single-step.  This
oversight is corrected in this patch: now, when stepping a thread, gdb
will call SuspendThread on all other threads.

However, the bug persisted even after this change.  In particular,
WaitForDebugEvent could see a stop for a thread that was ostensibly
suspended.  Our theory of what is happening here is that there are
actually simultaneous breakpoint hits, and the Windows kernel queues
the events, causing the second stop to be reported on a suspended
thread.

In Windows 10 or later gdb could use the DBG_REPLY_LATER flag to
ContinueDebugEvent to request that such events be re-reported later.
However, relying on that did not seem advisable, so this patch instead
arranges to queue such "pending" stops, and then to report them later,
once the step has completed.

In the PR, Pedro pointed out that it's best in this scenario to
implement the stopped_by_sw_breakpoint method, so this patch does this
as well.

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

	PR gdb/22992
	* windows-nat.c (current_event): Update comment.
	(last_wait_event, desired_stop_thread_id): New globals.
	(struct pending_stop): New.
	(pending_stops, last_stop_was_breakpoint): New globals.
	(windows_nat_target) <stopped_by_sw_breakpoint>
	<supports_stopped_by_sw_breakpoint>: New methods.
	(suspend_thread): New function.
	(thread_rec): Use suspend_thread.
	(windows_continue): Handle pending stops.  Suspend other threads
	when stepping.  Use last_wait_event
	(wait_for_debug_event): New function.
	(get_windows_debug_event): Use wait_for_debug_event.  Handle
	pending stops.  Queue spurious stops.
	(windows_nat_target::wait): Un-bias PC when needed.  Set
	last_stop_was_breakpoint.
	(windows_nat_target::kill): Use wait_for_debug_event.

Change-Id: I5d0dd64c3d2d8220b534d3e02aeaa6f6815264ab
---
M gdb/ChangeLog
M gdb/windows-nat.c
2 files changed, 184 insertions(+), 12 deletions(-)



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 9e7aa7b..ecabb25 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,25 @@
 2019-10-29  Tom Tromey  <tromey@adacore.com>
 
+	PR gdb/22992
+	* windows-nat.c (current_event): Update comment.
+	(last_wait_event, desired_stop_thread_id): New globals.
+	(struct pending_stop): New.
+	(pending_stops, last_stop_was_breakpoint): New globals.
+	(windows_nat_target) <stopped_by_sw_breakpoint>
+	<supports_stopped_by_sw_breakpoint>: New methods.
+	(suspend_thread): New function.
+	(thread_rec): Use suspend_thread.
+	(windows_continue): Handle pending stops.  Suspend other threads
+	when stepping.  Use last_wait_event
+	(wait_for_debug_event): New function.
+	(get_windows_debug_event): Use wait_for_debug_event.  Handle
+	pending stops.  Queue spurious stops.
+	(windows_nat_target::wait): Un-bias PC when needed.  Set
+	last_stop_was_breakpoint.
+	(windows_nat_target::kill): Use wait_for_debug_event.
+
+2019-10-29  Tom Tromey  <tromey@adacore.com>
+
 	* windows-nat.c (enum thread_disposition_type): New.
 	(thread_rec): Replace "get_context" parameter with "disposition";
 	change type.
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index b07da3b..1249778 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -218,8 +218,17 @@
 
 /* The process and thread handles for the above context.  */
 
-static DEBUG_EVENT current_event;	/* The current debug event from
-					   WaitForDebugEvent */
+/* The current debug event from WaitForDebugEvent or from a pending
+   stop.  */
+static DEBUG_EVENT current_event;
+
+/* The most recent event from WaitForDebugEvent.  Unlike
+   current_event, this is guaranteed never to come from a pending
+   stop.  This is important because only data from the most recent
+   event from WaitForDebugEvent can be used when calling
+   ContinueDebugEvent.  */
+static DEBUG_EVENT last_wait_event;
+
 static HANDLE current_process_handle;	/* Currently executing process */
 static windows_thread_info *current_thread;	/* Info on currently selected thread */
 
@@ -289,6 +298,41 @@
 
 #endif /* 0 */
 
+/* The ID of the thread for which we anticipate a stop event.
+   Normally this is -1, meaning we'll accept an event in any
+   thread.  */
+static DWORD desired_stop_thread_id = -1;
+
+/* A single pending stop.  See "pending_stops" for more
+   information.  */
+struct pending_stop
+{
+  /* The thread id.  */
+  DWORD thread_id;
+
+  /* The target waitstatus we computed.  */
+  target_waitstatus status;
+
+  /* The event.  A few fields of this can be referenced after a stop,
+     and it seemed simplest to store the entire event.  */
+  DEBUG_EVENT event;
+};
+
+/* A vector of pending stops.  Sometimes, Windows will report a stop
+   on a thread that has been ostensibly suspended.  We believe what
+   happens here is that two threads hit a breakpoint simultaneously,
+   and the Windows kernel queues the stop events.  However, this can
+   result in the strange effect of trying to single step thread A --
+   leaving all other threads suspended -- and then seeing a stop in
+   thread B.  To handle this scenario, we queue all such "pending"
+   stops here, and then process them once the step has completed.  See
+   PR gdb/22992.  */
+static std::vector<pending_stop> pending_stops;
+
+/* This is true if the most recently reported stop was due to a
+   breakpoint.  */
+static bool last_stop_was_breakpoint = false;
+
 struct windows_nat_target final : public x86_nat_target<inf_child_target>
 {
   void close () override;
@@ -307,6 +351,16 @@
   void fetch_registers (struct regcache *, int) override;
   void store_registers (struct regcache *, int) override;
 
+  bool stopped_by_sw_breakpoint () override
+  {
+    return last_stop_was_breakpoint;
+  }
+
+  bool supports_stopped_by_sw_breakpoint () override
+  {
+    return true;
+  }
+
   enum target_xfer_status xfer_partial (enum target_object object,
 					const char *annex,
 					gdb_byte *readbuf,
@@ -1297,16 +1351,37 @@
 {
   BOOL res;
 
+  desired_stop_thread_id = id;
+
+  /* If there are pending stops, and we might plausibly hit one of
+     them, we don't want to actually continue the inferior -- we just
+     want to report the stop.  In this case, we just pretend to
+     continue.  See the comment by the definition of "pending_stops"
+     for details on why this is needed.  */
+  for (auto &item : pending_stops)
+    {
+      if (desired_stop_thread_id == -1
+	  || desired_stop_thread_id == item.thread_id)
+	{
+	  DEBUG_EVENTS (("windows_continue - pending stop anticipated, "
+			 "desired=0x%x, item=0x%x\n",
+			 desired_stop_thread_id, item.thread_id));
+	  debug_registers_changed = 0;
+	  return TRUE;
+	}
+    }
+
   DEBUG_EVENTS (("ContinueDebugEvent (cpid=%d, ctid=0x%x, %s);\n",
-		  (unsigned) current_event.dwProcessId,
-		  (unsigned) current_event.dwThreadId,
+		  (unsigned) last_wait_event.dwProcessId,
+		  (unsigned) last_wait_event.dwThreadId,
 		  continue_status == DBG_CONTINUE ?
 		  "DBG_CONTINUE" : "DBG_EXCEPTION_NOT_HANDLED"));
 
   for (windows_thread_info *th : thread_list)
-    if ((id == -1 || id == (int) th->tid)
-	&& th->suspended)
+    if (id == -1 || id == (int) th->tid)
       {
+	if (!th->suspended)
+	  continue;
 	if (debug_registers_changed)
 	  {
 	    th->context.ContextFlags |= CONTEXT_DEBUG_REGISTERS;
@@ -1333,9 +1408,15 @@
 	  }
 	th->resume ();
       }
+    else
+      {
+	/* When single-stepping a specific thread, other threads must
+	   be suspended.  */
+	th->suspend ();
+      }
 
-  res = ContinueDebugEvent (current_event.dwProcessId,
-			    current_event.dwThreadId,
+  res = ContinueDebugEvent (last_wait_event.dwProcessId,
+			    last_wait_event.dwThreadId,
 			    continue_status);
 
   if (!res)
@@ -1487,6 +1568,17 @@
   return TRUE;
 }
 
+/* A wrapper for WaitForDebugEvent that sets "last_wait_event"
+   appropriately.  */
+static BOOL
+wait_for_debug_event (DEBUG_EVENT *event, DWORD timeout)
+{
+  BOOL result = WaitForDebugEvent (event, timeout);
+  if (result)
+    last_wait_event = *event;
+  return result;
+}
+
 /* Get the next event from the child.  Returns a non-zero thread id if the event
    requires handling by WFI (or whatever).  */
 static int
@@ -1499,9 +1591,36 @@
   static windows_thread_info dummy_thread_info (0, 0, 0);
   DWORD thread_id = 0;
 
+  /* If there is a relevant pending stop, report it now.  See the
+     comment by the definition of "pending_stops" for details on why
+     this is needed.  */
+  for (auto iter = pending_stops.begin ();
+       iter != pending_stops.end ();
+       ++iter)
+    {
+      if (desired_stop_thread_id == -1
+	  || desired_stop_thread_id == iter->thread_id)
+	{
+	  thread_id = iter->thread_id;
+	  *ourstatus = iter->status;
+	  current_event = iter->event;
+
+	  inferior_ptid = ptid_t (current_event.dwProcessId, thread_id, 0);
+	  current_thread = thread_rec (thread_id, INVALIDATE_CONTEXT);
+	  current_thread->reload_context = 1;
+
+	  DEBUG_EVENTS (("get_windows_debug_event - "
+			 "pending stop found in 0x%x (desired=0x%x)\n",
+			 thread_id, desired_stop_thread_id));
+
+	  pending_stops.erase (iter);
+	  return thread_id;
+	}
+    }
+
   last_sig = GDB_SIGNAL_0;
 
-  if (!(debug_event = WaitForDebugEvent (&current_event, 1000)))
+  if (!(debug_event = wait_for_debug_event (&current_event, 1000)))
     goto out;
 
   event_count++;
@@ -1671,7 +1790,19 @@
 
   if (!thread_id || saw_create != 1)
     {
-      CHECK (windows_continue (continue_status, -1, 0));
+      CHECK (windows_continue (continue_status, desired_stop_thread_id, 0));
+    }
+  else if (desired_stop_thread_id != -1 && desired_stop_thread_id != thread_id)
+    {
+      /* Pending stop.  See the comment by the definition of
+	 "pending_stops" for details on why this is needed.  */
+      DEBUG_EVENTS (("get_windows_debug_event - "
+		     "unexpected stop in 0x%x (expecting 0x%x)\n",
+		     thread_id, desired_stop_thread_id));
+
+      pending_stops.push_back ({thread_id, *ourstatus, current_event});
+      thread_id = 0;
+      CHECK (windows_continue (continue_status, desired_stop_thread_id, 0));
     }
   else
     {
@@ -1733,7 +1864,28 @@
       SetConsoleCtrlHandler (&ctrl_c_handler, FALSE);
 
       if (retval)
-	return ptid_t (current_event.dwProcessId, retval, 0);
+	{
+	  ptid_t result = ptid_t (current_event.dwProcessId, retval, 0);
+
+	  last_stop_was_breakpoint = false;
+	  if (current_event.dwDebugEventCode == EXCEPTION_DEBUG_EVENT
+	      && (current_event.u.Exception.ExceptionRecord.ExceptionCode
+		  == EXCEPTION_BREAKPOINT))
+	    {
+	      struct regcache *regcache = get_thread_regcache (result);
+	      gdbarch *gdbarch = regcache->arch ();
+
+	      CORE_ADDR pc = (regcache_read_pc (regcache)
+			      - gdbarch_decr_pc_after_break (gdbarch));
+	      if (software_breakpoint_inserted_here_p (regcache->aspace (), pc))
+		{
+		  last_stop_was_breakpoint = true;
+		  regcache_write_pc (regcache, pc);
+		}
+	    }
+
+	  return result;
+	}
       else
 	{
 	  int detach = 0;
@@ -2872,7 +3024,7 @@
     {
       if (!windows_continue (DBG_CONTINUE, -1, 1))
 	break;
-      if (!WaitForDebugEvent (&current_event, INFINITE))
+      if (!wait_for_debug_event (&current_event, INFINITE))
 	break;
       if (current_event.dwDebugEventCode == EXIT_PROCESS_DEBUG_EVENT)
 	break;

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

[review] Handle pending stops from the Windows kernel

[Luis Machado (Code Review)]

Luis Machado has posted comments on this change.

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


Patch Set 1: Code-Review+1

Though i'm not too familiar with Windows handling, the change looks sane to me.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I5d0dd64c3d2d8220b534d3e02aeaa6f6815264ab
Gerrit-Change-Number: 414
Gerrit-PatchSet: 1
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Comment-Date: Mon, 04 Nov 2019 14:40:49 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

Re: [review] Handle pending stops from the Windows kernel

[Pedro Alves]

On 10/29/19 5:57 PM, Tom Tromey (Code Review) wrote:

> +/* A single pending stop.  See "pending_stops" for more
> +   information.  */
> +struct pending_stop
> +{
> +  /* The thread id.  */
> +  DWORD thread_id;
> +
> +  /* The target waitstatus we computed.  */
> +  target_waitstatus status;
> +
> +  /* The event.  A few fields of this can be referenced after a stop,
> +     and it seemed simplest to store the entire event.  */
> +  DEBUG_EVENT event;
> +};
> +
> +/* A vector of pending stops.  Sometimes, Windows will report a stop
> +   on a thread that has been ostensibly suspended.  We believe what
> +   happens here is that two threads hit a breakpoint simultaneously,
> +   and the Windows kernel queues the stop events.  

I'm very much suspect that this must be happening also with other events,
not just breakpoints.  I think the comment should be adjusted in
that direction.

> However, this can
> +   result in the strange effect of trying to single step thread A --
> +   leaving all other threads suspended -- and then seeing a stop in
> +   thread B.  To handle this scenario, we queue all such "pending"
> +   stops here, and then process them once the step has completed.  See
> +   PR gdb/22992.  */
> +static std::vector<pending_stop> pending_stops;
> +
> +/* This is true if the most recently reported stop was due to a
> +   breakpoint.  */
> +static bool last_stop_was_breakpoint = false;
> +
>  struct windows_nat_target final : public x86_nat_target<inf_child_target>
>  {
>    void close () override;
> @@ -307,6 +351,16 @@
>    void fetch_registers (struct regcache *, int) override;
>    void store_registers (struct regcache *, int) override;
>  
> +  bool stopped_by_sw_breakpoint () override
> +  {
> +    return last_stop_was_breakpoint;
> +  }
> +
> +  bool supports_stopped_by_sw_breakpoint () override
> +  {
> +    return true;
> +  }
> +
>    enum target_xfer_status xfer_partial (enum target_object object,
>  					const char *annex,
>  					gdb_byte *readbuf,
> @@ -1297,16 +1351,37 @@

I'm disappointed that gerrit doesn't use "diff -up".  :-/

>  {
>    BOOL res;
>  
> +  desired_stop_thread_id = id;
> +
> +  /* If there are pending stops, and we might plausibly hit one of
> +     them, we don't want to actually continue the inferior -- we just
> +     want to report the stop.  In this case, we just pretend to
> +     continue.  See the comment by the definition of "pending_stops"
> +     for details on why this is needed.  */
> +  for (auto &item : pending_stops)

const auto ?

> +    {
> +      if (desired_stop_thread_id == -1
> +	  || desired_stop_thread_id == item.thread_id)
> +	{
> +	  DEBUG_EVENTS (("windows_continue - pending stop anticipated, "
> +			 "desired=0x%x, item=0x%x\n",
> +			 desired_stop_thread_id, item.thread_id));
> +	  debug_registers_changed = 0;

This clearing of debug_registers_changed looks suspicious.
If the debug registers did change, won't we miss propagating them
to the threads, when they're latter actually resumed?

> +	  return TRUE;
> +	}
> +    }
> +
>    DEBUG_EVENTS (("ContinueDebugEvent (cpid=%d, ctid=0x%x, %s);\n",
> -		  (unsigned) current_event.dwProcessId,
> -		  (unsigned) current_event.dwThreadId,
> +		  (unsigned) last_wait_event.dwProcessId,
> +		  (unsigned) last_wait_event.dwThreadId,
>  		  continue_status == DBG_CONTINUE ?
>  		  "DBG_CONTINUE" : "DBG_EXCEPTION_NOT_HANDLED"));
>  
>    for (windows_thread_info *th : thread_list)
> -    if ((id == -1 || id == (int) th->tid)
> -	&& th->suspended)
> +    if (id == -1 || id == (int) th->tid)
>        {
> +	if (!th->suspended)
> +	  continue;
>  	if (debug_registers_changed)
>  	  {
>  	    th->context.ContextFlags |= CONTEXT_DEBUG_REGISTERS;
> @@ -1333,9 +1408,15 @@
>  	  }
>  	th->resume ();
>        }
> +    else
> +      {
> +	/* When single-stepping a specific thread, other threads must
> +	   be suspended.  */
> +	th->suspend ();
> +      }
>  
> -  res = ContinueDebugEvent (current_event.dwProcessId,
> -			    current_event.dwThreadId,
> +  res = ContinueDebugEvent (last_wait_event.dwProcessId,
> +			    last_wait_event.dwThreadId,
>  			    continue_status);
>  
>    if (!res)
> @@ -1487,6 +1568,17 @@
>    return TRUE;
>  }
>  


> +  else if (desired_stop_thread_id != -1 && desired_stop_thread_id != thread_id)
> +    {
> +      /* Pending stop.  See the comment by the definition of
> +	 "pending_stops" for details on why this is needed.  */
> +      DEBUG_EVENTS (("get_windows_debug_event - "
> +		     "unexpected stop in 0x%x (expecting 0x%x)\n",
> +		     thread_id, desired_stop_thread_id));
> +
> +      pending_stops.push_back ({thread_id, *ourstatus, current_event});

I think you should unwind the PC here, not only when returning the pending
event to GDB core.  Consider the case of two threads hitting a breakpoint
at the same time.  When that happens, and do you "info threads", you want to
see the PC of all threads pointing at a valid instruction.  If you don't
unwind the PC of pending breakpoints, then the threads with pending breakpoints
will have their PC offset by one.

> +      thread_id = 0;
> +      CHECK (windows_continue (continue_status, desired_stop_thread_id, 0));
>      }
>    else
>      {
> @@ -1733,7 +1864,28 @@
>        SetConsoleCtrlHandler (&ctrl_c_handler, FALSE);
>  
>        if (retval)
> -	return ptid_t (current_event.dwProcessId, retval, 0);
> +	{
> +	  ptid_t result = ptid_t (current_event.dwProcessId, retval, 0);
> +
> +	  last_stop_was_breakpoint = false;
> +	  if (current_event.dwDebugEventCode == EXCEPTION_DEBUG_EVENT
> +	      && (current_event.u.Exception.ExceptionRecord.ExceptionCode
> +		  == EXCEPTION_BREAKPOINT))
> +	    {
> +	      struct regcache *regcache = get_thread_regcache (result);
> +	      gdbarch *gdbarch = regcache->arch ();
> +
> +	      CORE_ADDR pc = (regcache_read_pc (regcache)
> +			      - gdbarch_decr_pc_after_break (gdbarch));
> +	      if (software_breakpoint_inserted_here_p (regcache->aspace (), pc))

Why is software_breakpoint_inserted_here_p needed?  

> +		{
> +		  last_stop_was_breakpoint = true;
> +		  regcache_write_pc (regcache, pc);
> +		}
> +	    }
> +
> +	  return result;
> +	}
>        else
>  	{
>  	  int detach = 0;
Thanks,
Pedro Alves

[review] Handle pending stops from the Windows kernel

[Joel Brobecker (Code Review)]

Joel Brobecker has posted comments on this change.

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


Patch Set 1: Code-Review+1


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I5d0dd64c3d2d8220b534d3e02aeaa6f6815264ab
Gerrit-Change-Number: 414
Gerrit-PatchSet: 1
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Joel Brobecker <brobecker@adacore.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Comment-Date: Tue, 19 Nov 2019 01:01:21 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

Re: [review] Handle pending stops from the Windows kernel

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

>> +  else if (desired_stop_thread_id != -1 && desired_stop_thread_id != thread_id)
>> +    {
>> +      /* Pending stop.  See the comment by the definition of
>> +	 "pending_stops" for details on why this is needed.  */
>> +      DEBUG_EVENTS (("get_windows_debug_event - "
>> +		     "unexpected stop in 0x%x (expecting 0x%x)\n",
>> +		     thread_id, desired_stop_thread_id));
>> +
>> +      pending_stops.push_back ({thread_id, *ourstatus, current_event});

Pedro> I think you should unwind the PC here, not only when returning the pending
Pedro> event to GDB core.  Consider the case of two threads hitting a breakpoint
Pedro> at the same time.  When that happens, and do you "info threads", you want to
Pedro> see the PC of all threads pointing at a valid instruction.  If you don't
Pedro> unwind the PC of pending breakpoints, then the threads with pending breakpoints
Pedro> will have their PC offset by one.

I think I tried this, but I can try again.

>> +	      if (software_breakpoint_inserted_here_p (regcache->aspace (), pc))

Pedro> Why is software_breakpoint_inserted_here_p needed?  

Offsetting the PC did not work without this.
I tried to document my findings here:

https://sourceware.org/ml/gdb-patches/2019-10/msg00338.html

IIRC what happened is that gdb would sometimes resume the inferior with
wrong PC, causing it to crash.  However, I don't really recall, since it
was a long time ago now.  I guess I'll re-do the experiments.

Tom

Re: [review] Handle pending stops from the Windows kernel

[Pedro Alves]

On 11/19/19 2:20 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

> Pedro> I think you should unwind the PC here, not only when returning the pending
> Pedro> event to GDB core.  Consider the case of two threads hitting a breakpoint
> Pedro> at the same time.  When that happens, and do you "info threads", you want to
> Pedro> see the PC of all threads pointing at a valid instruction.  If you don't
> Pedro> unwind the PC of pending breakpoints, then the threads with pending breakpoints
> Pedro> will have their PC offset by one.
> 
> I think I tried this, but I can try again.

Thanks.

> 
>>> +	      if (software_breakpoint_inserted_here_p (regcache->aspace (), pc))
> 
> Pedro> Why is software_breakpoint_inserted_here_p needed?  
> 
> Offsetting the PC did not work without this.
> I tried to document my findings here:
> 
> https://sourceware.org/ml/gdb-patches/2019-10/msg00338.html
> 

Off hand that doesn't sound right.  Linux doesn't do that.
See linux-nat.c:save_stop_reason, in the USE_SIGTRAP_SIGINFO case
(the #else case is probably and hopefully dead by now).

With the software_breakpoint_inserted_here_p check in place, what I imagine
would happen is:

- thread A and B hit a breakpoint
- the event for thread B is left pending
- event for thread A is reported
- user/GDB removes the breakpoint before the event for thread B is processed
- user continues
- windows-nat.c prepares to return the pending event for B
- software_breakpoint_inserted_here_p returns false, so the PC is left unadjusted
- gdb core reports a spurious SIGTRAP, with the PC left unadjusted
- if the inferior is resumed, it starts execution with a bogus PC

Without the check, what should happen, and is the right behavior, is:

- thread A and B hit a breakpoint
- the event for thread B is left pending
- event for thread A is reported
- user/GDB removes the breakpoint before the event for thread B is processed
- user continues
- windows-nat.c prepares to return the pending event for B, adjusts the PC
- gdb core sees a TARGET_STOPPED_BY_SW_BREAKPOINT event, with the PC
  already adjusted.
- there's no breakpoint at that address, so gdb re-resumes the inferior
  transparently, here, in infrun.c:

	  /* A delayed software breakpoint event.  Ignore the trap.  */
	  if (debug_infrun)
	    fprintf_unfiltered (gdb_stdlog,
				"infrun: delayed software breakpoint "
				"trap, ignoring\n");


Basically, this mechanism replaces the old moribund locations heuristic.

> IIRC what happened is that gdb would sometimes resume the inferior with
> wrong PC, causing it to crash.  However, I don't really recall, since it
> was a long time ago now.  I guess I'll re-do the experiments.

Thanks.  I'm mystified.

Pedro Alves

Re: [review] Handle pending stops from the Windows kernel

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

>>>> +	      if (software_breakpoint_inserted_here_p (regcache->aspace (), pc))

Pedro> Why is software_breakpoint_inserted_here_p needed?  

>> Offsetting the PC did not work without this.

The problem turned out to be that this was the line preventing a
spurious stop at the initial "breakpoint" that occurs when starting an
inferior on Windows.  Checking windows_initialization_done worked as
well.

I've fixed this and also changed the code to adjust the PC for suspended
threads.

Tom

[review v2] Handle pending stops from the Windows kernel

[Tom Tromey (Code Review)]

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

Handle pending stops from the Windows kernel

PR gdb/22992 concerns an assertion failure in gdb when debugging a
certain inferior:

    int finish_step_over(execution_control_state*): Assertion `ecs->event_thread->control.trap_expected' failed.

Initially the investigation centered on the discovery that gdb was not
suspending other threads when attempting to single-step.  This
oversight is corrected in this patch: now, when stepping a thread, gdb
will call SuspendThread on all other threads.

However, the bug persisted even after this change.  In particular,
WaitForDebugEvent could see a stop for a thread that was ostensibly
suspended.  Our theory of what is happening here is that there are
actually simultaneous breakpoint hits, and the Windows kernel queues
the events, causing the second stop to be reported on a suspended
thread.

In Windows 10 or later gdb could use the DBG_REPLY_LATER flag to
ContinueDebugEvent to request that such events be re-reported later.
However, relying on that did not seem advisable, so this patch instead
arranges to queue such "pending" stops, and then to report them later,
once the step has completed.

In the PR, Pedro pointed out that it's best in this scenario to
implement the stopped_by_sw_breakpoint method, so this patch does this
as well.

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

	PR gdb/22992
	* windows-nat.c (current_event): Update comment.
	(last_wait_event, desired_stop_thread_id): New globals.
	(struct pending_stop): New.
	(pending_stops): New global.
	(windows_nat_target) <stopped_by_sw_breakpoint>
	<supports_stopped_by_sw_breakpoint>: New methods.
	(windows_fetch_one_register): Add assertions.  Adjust PC.
	(windows_continue): Handle pending stops.  Suspend other threads
	when stepping.  Use last_wait_event
	(wait_for_debug_event): New function.
	(get_windows_debug_event): Use wait_for_debug_event.  Handle
	pending stops.  Queue spurious stops.
	(windows_nat_target::wait): Set stopped_at_breakpoint.
	(windows_nat_target::kill): Use wait_for_debug_event.
	* nat/windows-nat.h (struct windows_thread_info)
	<stopped_at_breakpoint>: New field.
	* nat/windows-nat.c (windows_thread_info::resume): Clear
	stopped_at_breakpoint.

Change-Id: I5d0dd64c3d2d8220b534d3e02aeaa6f6815264ab
---
M gdb/ChangeLog
M gdb/nat/windows-nat.c
M gdb/nat/windows-nat.h
M gdb/windows-nat.c
4 files changed, 214 insertions(+), 13 deletions(-)



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 14cb238..bb441a1 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,27 @@
 2019-11-26  Tom Tromey  <tromey@adacore.com>
 
+	PR gdb/22992
+	* windows-nat.c (current_event): Update comment.
+	(last_wait_event, desired_stop_thread_id): New globals.
+	(struct pending_stop): New.
+	(pending_stops): New global.
+	(windows_nat_target) <stopped_by_sw_breakpoint>
+	<supports_stopped_by_sw_breakpoint>: New methods.
+	(windows_fetch_one_register): Add assertions.  Adjust PC.
+	(windows_continue): Handle pending stops.  Suspend other threads
+	when stepping.  Use last_wait_event
+	(wait_for_debug_event): New function.
+	(get_windows_debug_event): Use wait_for_debug_event.  Handle
+	pending stops.  Queue spurious stops.
+	(windows_nat_target::wait): Set stopped_at_breakpoint.
+	(windows_nat_target::kill): Use wait_for_debug_event.
+	* nat/windows-nat.h (struct windows_thread_info)
+	<stopped_at_breakpoint>: New field.
+	* nat/windows-nat.c (windows_thread_info::resume): Clear
+	stopped_at_breakpoint.
+
+2019-11-26  Tom Tromey  <tromey@adacore.com>
+
 	* windows-nat.c (enum thread_disposition_type): New.
 	(thread_rec): Replace "get_context" parameter with "disposition";
 	change type.
diff --git a/gdb/nat/windows-nat.c b/gdb/nat/windows-nat.c
index a98ff42..59b0fac 100644
--- a/gdb/nat/windows-nat.c
+++ b/gdb/nat/windows-nat.c
@@ -49,6 +49,8 @@
 {
   if (suspended > 0)
     {
+      stopped_at_breakpoint = false;
+
       if (ResumeThread (h) == (DWORD) -1)
 	{
 	  DWORD err = GetLastError ();
diff --git a/gdb/nat/windows-nat.h b/gdb/nat/windows-nat.h
index 488c9fb..d64ef8e 100644
--- a/gdb/nat/windows-nat.h
+++ b/gdb/nat/windows-nat.h
@@ -71,6 +71,10 @@
      inferior thread.  */
   bool reload_context = false;
 
+  /* True if this thread is currently stopped at a breakpoint.  This
+     is used to offset the PC when needed.  */
+  bool stopped_at_breakpoint = false;
+
   /* The name of the thread, allocated by xmalloc.  */
   gdb::unique_xmalloc_ptr<char> name;
 };
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index abd9d55..8448d1c 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -218,8 +218,17 @@
 
 /* The process and thread handles for the above context.  */
 
-static DEBUG_EVENT current_event;	/* The current debug event from
-					   WaitForDebugEvent */
+/* The current debug event from WaitForDebugEvent or from a pending
+   stop.  */
+static DEBUG_EVENT current_event;
+
+/* The most recent event from WaitForDebugEvent.  Unlike
+   current_event, this is guaranteed never to come from a pending
+   stop.  This is important because only data from the most recent
+   event from WaitForDebugEvent can be used when calling
+   ContinueDebugEvent.  */
+static DEBUG_EVENT last_wait_event;
+
 static HANDLE current_process_handle;	/* Currently executing process */
 static windows_thread_info *current_thread;	/* Info on currently selected thread */
 
@@ -289,6 +298,37 @@
 
 #endif /* 0 */
 
+/* The ID of the thread for which we anticipate a stop event.
+   Normally this is -1, meaning we'll accept an event in any
+   thread.  */
+static DWORD desired_stop_thread_id = -1;
+
+/* A single pending stop.  See "pending_stops" for more
+   information.  */
+struct pending_stop
+{
+  /* The thread id.  */
+  DWORD thread_id;
+
+  /* The target waitstatus we computed.  */
+  target_waitstatus status;
+
+  /* The event.  A few fields of this can be referenced after a stop,
+     and it seemed simplest to store the entire event.  */
+  DEBUG_EVENT event;
+};
+
+/* A vector of pending stops.  Sometimes, Windows will report a stop
+   on a thread that has been ostensibly suspended.  We believe what
+   happens here is that two threads hit a breakpoint simultaneously,
+   and the Windows kernel queues the stop events.  However, this can
+   result in the strange effect of trying to single step thread A --
+   leaving all other threads suspended -- and then seeing a stop in
+   thread B.  To handle this scenario, we queue all such "pending"
+   stops here, and then process them once the step has completed.  See
+   PR gdb/22992.  */
+static std::vector<pending_stop> pending_stops;
+
 struct windows_nat_target final : public x86_nat_target<inf_child_target>
 {
   void close () override;
@@ -307,6 +347,16 @@
   void fetch_registers (struct regcache *, int) override;
   void store_registers (struct regcache *, int) override;
 
+  bool stopped_by_sw_breakpoint () override
+  {
+    return current_thread->stopped_at_breakpoint;
+  }
+
+  bool supports_stopped_by_sw_breakpoint () override
+  {
+    return true;
+  }
+
   enum target_xfer_status xfer_partial (enum target_object object,
 					const char *annex,
 					gdb_byte *readbuf,
@@ -542,6 +592,10 @@
   struct gdbarch *gdbarch = regcache->arch ();
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
 
+  gdb_assert (!gdbarch_read_pc_p (gdbarch));
+  gdb_assert (gdbarch_pc_regnum (gdbarch) >= 0);
+  gdb_assert (!gdbarch_write_pc_p (gdbarch));
+
   if (r == I387_FISEG_REGNUM (tdep))
     {
       long l = *((long *) context_offset) & 0xffff;
@@ -561,7 +615,28 @@
       regcache->raw_supply (r, (char *) &l);
     }
   else
-    regcache->raw_supply (r, context_offset);
+    {
+      if (th->stopped_at_breakpoint && r == gdbarch_pc_regnum (gdbarch))
+	{
+	  int size = register_size (gdbarch, r);
+	  if (size == 4)
+	    {
+	      uint32_t value;
+	      memcpy (&value, context_offset, size);
+	      value -= gdbarch_decr_pc_after_break (gdbarch);
+	      memcpy (context_offset, &value, size);
+	    }
+	  else
+	    {
+	      gdb_assert (size == 8);
+	      uint64_t value;
+	      memcpy (&value, context_offset, size);
+	      value -= gdbarch_decr_pc_after_break (gdbarch);
+	      memcpy (context_offset, &value, size);
+	    }
+	}
+      regcache->raw_supply (r, context_offset);
+    }
 }
 
 void
@@ -1297,16 +1372,36 @@
 {
   BOOL res;
 
+  desired_stop_thread_id = id;
+
+  /* If there are pending stops, and we might plausibly hit one of
+     them, we don't want to actually continue the inferior -- we just
+     want to report the stop.  In this case, we just pretend to
+     continue.  See the comment by the definition of "pending_stops"
+     for details on why this is needed.  */
+  for (const auto &item : pending_stops)
+    {
+      if (desired_stop_thread_id == -1
+	  || desired_stop_thread_id == item.thread_id)
+	{
+	  DEBUG_EVENTS (("windows_continue - pending stop anticipated, "
+			 "desired=0x%x, item=0x%x\n",
+			 desired_stop_thread_id, item.thread_id));
+	  return TRUE;
+	}
+    }
+
   DEBUG_EVENTS (("ContinueDebugEvent (cpid=%d, ctid=0x%x, %s);\n",
-		  (unsigned) current_event.dwProcessId,
-		  (unsigned) current_event.dwThreadId,
+		  (unsigned) last_wait_event.dwProcessId,
+		  (unsigned) last_wait_event.dwThreadId,
 		  continue_status == DBG_CONTINUE ?
 		  "DBG_CONTINUE" : "DBG_EXCEPTION_NOT_HANDLED"));
 
   for (windows_thread_info *th : thread_list)
-    if ((id == -1 || id == (int) th->tid)
-	&& th->suspended)
+    if (id == -1 || id == (int) th->tid)
       {
+	if (!th->suspended)
+	  continue;
 	if (debug_registers_changed)
 	  {
 	    th->context.ContextFlags |= CONTEXT_DEBUG_REGISTERS;
@@ -1333,9 +1428,15 @@
 	  }
 	th->resume ();
       }
+    else
+      {
+	/* When single-stepping a specific thread, other threads must
+	   be suspended.  */
+	th->suspend ();
+      }
 
-  res = ContinueDebugEvent (current_event.dwProcessId,
-			    current_event.dwThreadId,
+  res = ContinueDebugEvent (last_wait_event.dwProcessId,
+			    last_wait_event.dwThreadId,
 			    continue_status);
 
   if (!res)
@@ -1487,6 +1588,17 @@
   return TRUE;
 }
 
+/* A wrapper for WaitForDebugEvent that sets "last_wait_event"
+   appropriately.  */
+static BOOL
+wait_for_debug_event (DEBUG_EVENT *event, DWORD timeout)
+{
+  BOOL result = WaitForDebugEvent (event, timeout);
+  if (result)
+    last_wait_event = *event;
+  return result;
+}
+
 /* Get the next event from the child.  Returns a non-zero thread id if the event
    requires handling by WFI (or whatever).  */
 static int
@@ -1499,9 +1611,36 @@
   static windows_thread_info dummy_thread_info (0, 0, 0);
   DWORD thread_id = 0;
 
+  /* If there is a relevant pending stop, report it now.  See the
+     comment by the definition of "pending_stops" for details on why
+     this is needed.  */
+  for (auto iter = pending_stops.begin ();
+       iter != pending_stops.end ();
+       ++iter)
+    {
+      if (desired_stop_thread_id == -1
+	  || desired_stop_thread_id == iter->thread_id)
+	{
+	  thread_id = iter->thread_id;
+	  *ourstatus = iter->status;
+	  current_event = iter->event;
+
+	  inferior_ptid = ptid_t (current_event.dwProcessId, thread_id, 0);
+	  current_thread = thread_rec (thread_id, INVALIDATE_CONTEXT);
+	  current_thread->reload_context = 1;
+
+	  DEBUG_EVENTS (("get_windows_debug_event - "
+			 "pending stop found in 0x%x (desired=0x%x)\n",
+			 thread_id, desired_stop_thread_id));
+
+	  pending_stops.erase (iter);
+	  return thread_id;
+	}
+    }
+
   last_sig = GDB_SIGNAL_0;
 
-  if (!(debug_event = WaitForDebugEvent (&current_event, 1000)))
+  if (!(debug_event = wait_for_debug_event (&current_event, 1000)))
     goto out;
 
   event_count++;
@@ -1671,7 +1810,27 @@
 
   if (!thread_id || saw_create != 1)
     {
-      CHECK (windows_continue (continue_status, -1, 0));
+      CHECK (windows_continue (continue_status, desired_stop_thread_id, 0));
+    }
+  else if (desired_stop_thread_id != -1 && desired_stop_thread_id != thread_id)
+    {
+      /* Pending stop.  See the comment by the definition of
+	 "pending_stops" for details on why this is needed.  */
+      DEBUG_EVENTS (("get_windows_debug_event - "
+		     "unexpected stop in 0x%x (expecting 0x%x)\n",
+		     thread_id, desired_stop_thread_id));
+
+      if (current_event.dwDebugEventCode == EXCEPTION_DEBUG_EVENT
+	  && (current_event.u.Exception.ExceptionRecord.ExceptionCode
+	      == EXCEPTION_BREAKPOINT)
+	  && windows_initialization_done)
+	{
+	  th = thread_rec (thread_id, INVALIDATE_CONTEXT);
+	  th->stopped_at_breakpoint = true;
+	}
+      pending_stops.push_back ({thread_id, *ourstatus, current_event});
+      thread_id = 0;
+      CHECK (windows_continue (continue_status, desired_stop_thread_id, 0));
     }
   else
     {
@@ -1733,7 +1892,21 @@
       SetConsoleCtrlHandler (&ctrl_c_handler, FALSE);
 
       if (retval)
-	return ptid_t (current_event.dwProcessId, retval, 0);
+	{
+	  ptid_t result = ptid_t (current_event.dwProcessId, retval, 0);
+
+	  if (current_thread != nullptr)
+	    {
+	      current_thread->stopped_at_breakpoint = false;
+	      if (current_event.dwDebugEventCode == EXCEPTION_DEBUG_EVENT
+		  && (current_event.u.Exception.ExceptionRecord.ExceptionCode
+		      == EXCEPTION_BREAKPOINT)
+		  && windows_initialization_done)
+		current_thread->stopped_at_breakpoint = true;
+	    }
+
+	  return result;
+	}
       else
 	{
 	  int detach = 0;
@@ -2873,7 +3046,7 @@
     {
       if (!windows_continue (DBG_CONTINUE, -1, 1))
 	break;
-      if (!WaitForDebugEvent (&current_event, INFINITE))
+      if (!wait_for_debug_event (&current_event, INFINITE))
 	break;
       if (current_event.dwDebugEventCode == EXIT_PROCESS_DEBUG_EVENT)
 	break;

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I5d0dd64c3d2d8220b534d3e02aeaa6f6815264ab
Gerrit-Change-Number: 414
Gerrit-PatchSet: 2
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Joel Brobecker <brobecker@adacore.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-MessageType: newpatchset

[review v2] Handle pending stops from the Windows kernel

[Pedro Alves (Code Review)]

Pedro Alves has posted comments on this change.

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


Patch Set 2: Code-Review+2

(2 comments)

Minor nit below.  Otherwise this version LGTM.

| --- gdb/nat/windows-nat.h
| +++ gdb/nat/windows-nat.h
| @@ -64,15 +64,19 @@ #endif
|    CONTEXT context {};
|  
|    /* Whether debug registers changed since we last set CONTEXT back to
|       the thread.  */
|    bool debug_registers_changed = false;
|  
|    /* Nonzero if CONTEXT is invalidated and must be re-read from the
|       inferior thread.  */
|    bool reload_context = false;
|  
| +  /* True if this thread is currently stopped at a breakpoint.  This
| +     is used to offset the PC when needed.  */
| +  bool stopped_at_breakpoint = false;

PS2, Line 76:

I think it would be better if this said "stopped at a software
breakpoint", and the field was called stopped_at_sw_breakpoint (for
example).  This is because you won't do the offsetting with hardware
breakpoints.

| +
|    /* The name of the thread, allocated by xmalloc.  */
|    gdb::unique_xmalloc_ptr<char> name;
|  };
|  
|  #endif
| --- gdb/windows-nat.c
| +++ gdb/windows-nat.c
| @@ -556,16 +610,37 @@ windows_fetch_one_register (struct regcache *regcache,
|      {
|        /* GDB treats segment registers as 32bit registers, but they are
|  	 in fact only 16 bits long.  Make sure we do not read extra
|  	 bits from our source buffer.  */
|        long l = *((long *) context_offset) & 0xffff;
|        regcache->raw_supply (r, (char *) &l);
|      }
|    else
| -    regcache->raw_supply (r, context_offset);
| +    {
| +      if (th->stopped_at_breakpoint && r == gdbarch_pc_regnum (gdbarch))
| +	{
| +	  int size = register_size (gdbarch, r);
| +	  if (size == 4)
| +	    {
| +	      uint32_t value;
| +	      memcpy (&value, context_offset, size);
| +	      value -= gdbarch_decr_pc_after_break (gdbarch);
| +	      memcpy (context_offset, &value, size);
| +	    }
| +	  else
| +	    {
| +	      gdb_assert (size == 8);
| +	      uint64_t value;
| +	      memcpy (&value, context_offset, size);
| +	      value -= gdbarch_decr_pc_after_break (gdbarch);
| +	      memcpy (context_offset, &value, size);
| +	    }
| +	}

PS2, Line 637:

Clever to do this here.

| +      regcache->raw_supply (r, context_offset);
| +    }
|  }
|  
|  void
|  windows_nat_target::fetch_registers (struct regcache *regcache, int r)
|  {
|    DWORD tid = regcache->ptid ().lwp ();
|    windows_thread_info *th = thread_rec (tid, INVALIDATE_CONTEXT);

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I5d0dd64c3d2d8220b534d3e02aeaa6f6815264ab
Gerrit-Change-Number: 414
Gerrit-PatchSet: 2
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Joel Brobecker <brobecker@adacore.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Comment-Date: Fri, 29 Nov 2019 19:08:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment