[RFC] Catch exception after stepped over watchpoint.

[Yao Qi <yao at codesourcery dot com>]

Hi,
When I test mips h/w watchpoint gdbserver patches V2, I get an internal
error in gdb.  Here are the steps to reproduce it with my patches,

(gdb) target remote mips:1234
(gdb) watch *global_ptr
Hardware watchpoint 1: *global_ptr
(gdb) watch **global_ptr
Hardware watchpoint 2: **global_ptr
(gdb) c
Continuing.
Warning:
Could not insert hardware watchpoint 1.
Could not insert hardware breakpoints:
You may have requested too many hardware breakpoints/watchpoints.

(gdb) kill
Kill the program being debugged? (y or n) y
(gdb) target remote mips:1234
Remote debugging using mips:1234
gdb/infrun.c:3889:
internal-error: handle_inferior_event: Assertion `ptid_equal
(inferior_ptid, singlestep_ptid)' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n)

I got this internal error seldom when the V1 patches were tested.  I
bypass it as it is unrelated.  Nowadays, I got this internal error
every time, even with V1 patches, so I have to fix it at first.

Note that my mips board only has one watch register, and we request
two here.  After some analysis, this internal error is caused by
exception is thrown when stepping over watchpoint (by software single
step).

When the inferior hit a watchpoint, GDB gets a stop and steps over the
watchpoint.  GDB will remove all the breakpoints, perform single step,
wait, and insert breakpoints again.

Let us look at the infrun debug on how GDB behaves with one
watchpoint,

....
nfrun: stopped by watchpoint
infrun: stopped data address = 0x120011bd0

 // stepping over a watchpoint

infrun: prepare_to_wait
infrun: target_wait (7512 [Thread 7512], status) =
infrun:   7512 [Thread 7512],
infrun:   status->kind = stopped, signal = SIGTRAP
infrun: infwait_nonstep_watch_state

 // watchpoints are inserted here, and single step

infrun: TARGET_WAITKIND_STOPPED
infrun: stop_pc = 0x1200012f8
infrun: software single step trap for Thread 7512
infrun: BPSTAT_WHAT_STOP_NOISY
infrun: stop_stepping

 // single step is done

Hardware watchpoint 1: *global_ptr

and here is how GDB behaves with two watchpoints,

infrun: stopped by watchpoint
infrun: stopped data address = 0x120011bd0

 // stepping over a watchpoint

infrun: prepare_to_wait
infrun: target_wait (7516 [Thread 7516], status) =
infrun:   7516 [Thread 7516],
infrun:   status->kind = stopped, signal = SIGTRAP
infrun: infwait_nonstep_watch_state

 // watchpoints are being inserted, but an exception is thrown

Warning:
Could not insert hardware watchpoint 1.
Could not insert hardware breakpoints:
You may have requested too many hardware breakpoints/watchpoints.

As we can see, exception is thrown when insert breakpoints (because
GDB requests two watchpoint registers while the board only has one),
but the state on software single step is not cleared
(singlestep_breakpoints_inserted_p is still true, which is wrong).
When GDB connects to the remote again, it goes to the path triggers
the assert.  This problem may exist on other targets which 1) use
software single step 2) support hardware watchpoint.  Probably, we can
reproduce it on some arm boards.  I didn't do that because I don't
have such board on hand.

This patch catches the exception around insert_breakpoints and clear the
state on software single step.

Note that I tried to call stop_stepping (ecs) and return after print
exception, but the watchpoint hit is not processed in this way.  The
output of GDB is:

(gdb) c
Continuing.
Warning:
Could not insert hardware watchpoint 1.
Could not insert hardware breakpoints:
You may have requested too many hardware breakpoints/watchpoints.

func4 () at /scratch/yqi/mips-linux/src/gdb-2013.05/gdb/testsuite/gdb.base/watchpoint.c:135
135       buf[0] = 7;

users are confused that why program stops here.  In my current
approach, the output looks more clear,

(gdb) c
Continuing.
Warning:
Could not insert hardware watchpoint 1.
Could not insert hardware breakpoints:
You may have requested too many hardware breakpoints/watchpoints.

Hardware watchpoint 1: *global_ptr

Old value = <unreadable>
New value = 3 '\003'
func4 () at /scratch/yqi/mips-linux/src/gdb-2013.05/gdb/testsuite/gdb.base/watchpoint.c:135
135       buf[0] = 7;

Regression tested on x86_64 and mips (with my h/w watchpoint patches).
Comments?

gdb:

2013-06-28  Yao Qi  <yao@codesourcery.com>

	* infrun.c (handle_inferior_event): Catch exception of
	'insert_breakpoints'.  Pull the single step breakpoints if
	exception is thrown.
---
 gdb/infrun.c |   36 +++++++++++++++++++++++++++---------
 1 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 8fb219a..67752c7 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3278,17 +3278,35 @@ handle_inferior_event (struct execution_control_state *ecs)
       break;
 
     case infwait_nonstep_watch_state:
-      if (debug_infrun)
-        fprintf_unfiltered (gdb_stdlog,
-			    "infrun: infwait_nonstep_watch_state\n");
-      insert_breakpoints ();
+      {
+	volatile struct gdb_exception e;
 
-      /* FIXME-maybe: is this cleaner than setting a flag?  Does it
-         handle things like signals arriving and other things happening
-         in combination correctly?  */
-      stepped_after_stopped_by_watchpoint = 1;
-      break;
+	if (debug_infrun)
+	  fprintf_unfiltered (gdb_stdlog,
+			      "infrun: infwait_nonstep_watch_state\n");
 
+	TRY_CATCH (e, RETURN_MASK_ERROR)
+	  {
+	    insert_breakpoints ();
+	  }
+	if (e.reason < 0)
+	  {
+	    if (singlestep_breakpoints_inserted_p)
+	      {
+		/* Pull the single step breakpoints out of the target.  */
+		remove_single_step_breakpoints ();
+		singlestep_breakpoints_inserted_p = 0;
+	      }
+
+	    exception_print (gdb_stderr, e);
+	  }
+
+	/* FIXME-maybe: is this cleaner than setting a flag?  Does it
+	   handle things like signals arriving and other things happening
+	   in combination correctly?  */
+	stepped_after_stopped_by_watchpoint = 1;
+      }
+      break;
     default:
       internal_error (__FILE__, __LINE__, _("bad switch"));
     }
-- 
1.7.7.6