[review] Implement stopped_by_sw_breakpoint for Windows gdbserver

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

Implement stopped_by_sw_breakpoint for Windows gdbserver

This changes the Windows gdbserver port to implement the
stopped_by_sw_breakpoint target method.  This is needed to support
pending stops.

This is a separate patch now, because Pedro suggested splitting it out
for simpler bisecting, in the case that it introduces a bug.

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.
	(win32_remove_point): Call remove_memory_breakpoint when needed.
	(win32_stopped_by_sw_breakpoint)
	(win32_supports_stopped_by_sw_breakpoint): New functions.
	(win32_target_ops): Update.
	(maybe_adjust_pc): New function.
	(win32_wait): Call maybe_adjust_pc.

Change-Id: Ib603fca745bc4ae43b6399f40919b1b399de5d51
---
M gdb/gdbserver/ChangeLog
M gdb/gdbserver/win32-low.c
2 files changed, 74 insertions(+), 14 deletions(-)
  

Luis Machado has posted comments on this change.

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


Patch Set 1:

(2 comments)

| --- gdb/gdbserver/win32-low.c
| +++ gdb/gdbserver/win32-low.c
| @@ -1178,2 +1183,19 @@ windows_nat::handle_ms_vc_exception (const EXCEPTION_RECORD *rec)
|  }
|  
| +/* A helper function that will, if needed, set 'stopped_at_breakpoint'
| +   on the thread and adjust the PC.  */
| +
| +static void
| +maybe_adjust_pc ()
| +{
| +  struct regcache *regcache = get_thread_regcache (current_thread, 1);
| +  child_fetch_inferior_registers (regcache, -1);

PS1, Line 1192:

How about moving this into the conditional block so that we only fetch
registers when we know we need them?

Or is it the case that ....

| +
| +  windows_thread_info *th = thread_rec (current_thread_ptid (),
| +					INVALIDATE_CONTEXT);
| +  th->stopped_at_breakpoint = false;
| +
| +  if (current_event.dwDebugEventCode == EXCEPTION_DEBUG_EVENT
| +      && (current_event.u.Exception.ExceptionRecord.ExceptionCode
| +	  == EXCEPTION_BREAKPOINT)
| +      && child_initialization_done)

 ...

| @@ -1419,13 +1447,13 @@ win32_wait (ptid_t ptid, struct target_waitstatus *ourstatus, int options)
| -	  OUTMSG2 (("Child Stopped with signal = %d \n",
| -		    ourstatus->value.sig));
| -
| -	  regcache = get_thread_regcache (current_thread, 1);
| -	  child_fetch_inferior_registers (regcache, -1);
| -	  return debug_event_ptid (&current_event);
| +	  {
| +	    OUTMSG2 (("Child Stopped with signal = %d \n",
| +		      ourstatus->value.sig));
| +	    maybe_adjust_pc ();

PS1, Line 1450:

... we always need to fetch registers here? If so, maybe a comment in
maybe_adjust_pc would make it more clear.

| +	    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);
|  	  break;
  

Pedro Alves has posted comments on this change.

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


Patch Set 1:

(2 comments)

| --- gdb/gdbserver/win32-low.c
| +++ gdb/gdbserver/win32-low.c
| @@ -1178,9 +1183,34 @@ windows_nat::handle_ms_vc_exception (const EXCEPTION_RECORD *rec)
|  }
|  
| +/* A helper function that will, if needed, set 'stopped_at_breakpoint'
| +   on the thread and adjust the PC.  */
| +
| +static void
| +maybe_adjust_pc ()
| +{
| +  struct regcache *regcache = get_thread_regcache (current_thread, 1);
| +  child_fetch_inferior_registers (regcache, -1);

PS1, Line 1192:

> How about moving this into the conditional block so that we only fetch registers when we know we need them?

Also note that as is, the code reads a little odd, for doing
thread_rec (...INVALIDATE_CONTEXT) right after
child_fetch_inferior_registers.  I mean,
child_fetch_inferior_registers will pull in CONTEXT, and then right
after we invalidate & fetch it again.

| +
| +  windows_thread_info *th = thread_rec (current_thread_ptid (),
| +					INVALIDATE_CONTEXT);
| +  th->stopped_at_breakpoint = false;
| +
| +  if (current_event.dwDebugEventCode == EXCEPTION_DEBUG_EVENT
| +      && (current_event.u.Exception.ExceptionRecord.ExceptionCode
| +	  == EXCEPTION_BREAKPOINT)
| +      && child_initialization_done)
| +    {
| +      th->stopped_at_breakpoint = true;
| +      CORE_ADDR pc = regcache_read_pc (regcache);
| +      CORE_ADDR sw_breakpoint_pc = pc - the_low_target.decr_pc_after_break;
| +      regcache_write_pc (regcache, sw_breakpoint_pc);
| +    }

PS1, Line 1207:

I'm also curious about why this decr_pc handling isn't done within
child_fetch_inferior_registers to mirror how it's handled on the gdb
side.

| +}
| +
|  /* Get the next event from the child.  */
|  
|  static int
|  get_child_debug_event (DWORD *continue_status,
|  		       struct target_waitstatus *ourstatus)
|  {
|    ptid_t ptid;
  

Tom Tromey has posted comments on this change.

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


Patch Set 1:

(2 comments)

| --- gdb/gdbserver/win32-low.c
| +++ gdb/gdbserver/win32-low.c
| @@ -1178,9 +1183,34 @@ windows_nat::handle_ms_vc_exception (const EXCEPTION_RECORD *rec)
|  }
|  
| +/* A helper function that will, if needed, set 'stopped_at_breakpoint'
| +   on the thread and adjust the PC.  */
| +
| +static void
| +maybe_adjust_pc ()
| +{
| +  struct regcache *regcache = get_thread_regcache (current_thread, 1);
| +  child_fetch_inferior_registers (regcache, -1);

PS1, Line 1192:

> > How about moving this into the conditional block so that we only fetch registers when we know we need them?

We want to clear stopped_at_breakpoint unconditionally here.

| +
| +  windows_thread_info *th = thread_rec (current_thread_ptid (),
| +					INVALIDATE_CONTEXT);
| +  th->stopped_at_breakpoint = false;
| +
| +  if (current_event.dwDebugEventCode == EXCEPTION_DEBUG_EVENT
| +      && (current_event.u.Exception.ExceptionRecord.ExceptionCode
| +	  == EXCEPTION_BREAKPOINT)
| +      && child_initialization_done)
| +    {
| +      th->stopped_at_breakpoint = true;
| +      CORE_ADDR pc = regcache_read_pc (regcache);
| +      CORE_ADDR sw_breakpoint_pc = pc - the_low_target.decr_pc_after_break;
| +      regcache_write_pc (regcache, sw_breakpoint_pc);
| +    }

PS1, Line 1207:

> I'm also curious about why this decr_pc handling isn't done within  child_fetch_inferior_registers to mirror how it's handled on the gdb side.

It was just more convenient to do it this way.
I'll try making the change.

| +}
| +
|  /* Get the next event from the child.  */
|  
|  static int
|  get_child_debug_event (DWORD *continue_status,
|  		       struct target_waitstatus *ourstatus)
|  {
|    ptid_t ptid;