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

[Pedro Alves <palves at redhat dot com>]

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