Patchwork [review] Implement stopped_by_sw_breakpoint for Windows gdbserver

login
register
mail settings
Submitter Simon Marchi (Code Review)
Date Nov. 26, 2019, 5:11 p.m.
Message ID <gerrit.1574788288000.Ib603fca745bc4ae43b6399f40919b1b399de5d51@gnutoolchain-gerrit.osci.io>
Download mbox | patch
Permalink /patch/36231/
State New
Headers show

Comments

Simon Marchi (Code Review) - Nov. 26, 2019, 5:11 p.m.
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(-)
Simon Marchi (Code Review) - Nov. 27, 2019, 12:55 a.m.
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;
Simon Marchi (Code Review) - Nov. 29, 2019, 9:13 p.m.
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;
Simon Marchi (Code Review) - Dec. 4, 2019, 4:09 p.m.
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;

Patch

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 0002bf7..9008bdc 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,5 +1,17 @@ 
 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.
+
+2019-11-26  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 9d88d1a..c472e0e 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).  */
@@ -1177,6 +1182,31 @@ 
   return false;
 }
 
+/* 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);
+
+  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);
+    }
+}
+
 /* Get the next event from the child.  */
 
 static int
@@ -1388,8 +1418,6 @@ 
 static ptid_t
 win32_wait (ptid_t ptid, struct target_waitstatus *ourstatus, int options)
 {
-  struct regcache *regcache;
-
   if (cached_status.kind != TARGET_WAITKIND_IGNORE)
     {
       /* The core always does a wait after creating the inferior, and
@@ -1416,12 +1444,12 @@ 
 	  return ptid_t (current_event.dwProcessId);
 	case TARGET_WAITKIND_STOPPED:
 	case TARGET_WAITKIND_LOADED:
-	  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 ();
+	    return debug_event_ptid (&current_event);
+	  }
 	default:
 	  OUTMSG (("Ignoring unknown internal event, %d\n", ourstatus->kind));
 	  /* fall-through */
@@ -1585,6 +1613,26 @@ 
   return the_low_target.breakpoint;
 }
 
+/* Implementation of the target_ops method
+   "stopped_by_sw_breakpoint".  */
+
+static int
+win32_stopped_by_sw_breakpoint ()
+{
+  windows_thread_info *th = thread_rec (current_thread_ptid (),
+					DONT_INVALIDATE_CONTEXT);
+  return th == nullptr ? 0 : th->stopped_at_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 +1672,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,