This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.

Index Nav:	[Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav:	[Date Prev] [Date Next]	[Thread Prev] [Thread Next]
Other format:	[Raw text]

[win32] Fix watchpoint support

From: Pedro Alves <pedro_alves at portugalmail dot pt>
To: gdb-patches at sourceware dot org
Date: Wed, 21 Nov 2007 00:35:21 +0000
Subject: [win32] Fix watchpoint support

Hi,

The current watchpoint support for native debugging on win32 systems
doesn't work reliably.  Quite often, we'd miss that the inferior
stopped due to a watchpoint triggering, because Windows wouldn't
report in DR6 which watchpoint triggered.  Take a look at this
"set debug infrun 1; maint show-debug-regs 1" snippet [1]:

infrun: TARGET_WAITKIND_STOPPED
infrun: stop_pc = 0x40108e
stopped_data_addr:
           CONTROL (DR7): 000d0101          STATUS (DR6): 00000000
                                                          ^^^^^^^^
           DR0: addr=0x00403010, ref.count=1  DR1: addr=0x00000000, ref.count=0
           DR2: addr=0x00000000, ref.count=0  DR3: addr=0x00000000, ref.count=0
infrun: random signal 5
^^^^^^^^^^^^^^^^^^^^^^^

Program received signal SIGTRAP, Trace/breakpoint trap.
infrun: stop_stepping
remove_watchpoint (addr=403010, len=4, type=data-write):
           CONTROL (DR7): 000d0100          STATUS (DR6): 00000000
           DR0: addr=0x00000000, ref.count=0  DR1: addr=0x00000000, ref.count=0
           DR2: addr=0x00000000, ref.count=0  DR3: addr=0x00000000, ref.count=0
main () at watch.c:11
11        printf ("count %d\n", count);

0x00403010 in that example is the address of a variable that
was just written to.

[1] Full example here:
http://cygwin.com/ml/cygwin/2007-10/msg00057.html

I ended up tracing the problem to win32_continue (gdb/win32-nat.c).
Currently, it looks somewhat like this:

1 win32_continue(TID)
2       ContinueDebugEvent(current_TID)
3       foreach thread in threads do
4              if thread == TID
5                     ResumeThread(TID)
6                     SetThreadContext(TID, DEBUG_REGS)
7              fi
8       hcaerof

The first problem is that we shoudn't be calling
SetThreadContext after ResumeThread (5,6) -- it should
be the other way around.

Second, when TID represents the thread of the
current debug event, TID will already be resumed
after the ContinueDebugEvent call, because we don't
call SuspendThread on the current debug event thread.
This is what caused the weird watchpoint behaviour.

I can see more than one way to fix this :

1 - Only call ContinueDebugEvent after resuming and setting
the context on every other thread:

     1 win32_continue(TID)
     2       foreach thread in threads do
     3              if thread == TID
     4                     ResumeThread(TID)
     5                     SetThreadContext(TID, DEBUG_REGS)
     6              fi
     7       hcaerof
     8       ContinueDebugEvent(current_TID)

2 - Don't ResumeThread/SetThreadContext on the thread of
the current debug event, since it is already set in
win32_resume:

     1 win32_continue(TID)
     2       foreach thread in threads do
     3              if thread == TID && thread != current_TID
     4                     ResumeThread(TID)
     5                     SetThreadContext(TID, DEBUG_REGS)
     6              fi
     7       hcaerof
     8       ContinueDebugEvent(current_TID)

I tend to prefer the first, because win32_continue isn't
always called from win32_resume, and because until
ContinueDebugEvent is called, all threads are stopped.  The
second choice isn't as atomic.

The attached patch implements option 1.  It fixes a
bunch of watchpoint related failures on Cygwin, and has
no regressions:

PASS: gdb.base/commands.exp: add print command to watch PASS: gdb.base/commands.exp: add continue command to watch PASS: gdb.base/commands.exp: end commands on watch -FAIL: gdb.base/commands.exp: continue with watch +PASS: gdb.base/commands.exp: continue with watch PASS: gdb.base/commands.exp: break factorial #3 PASS: gdb.base/commands.exp: set value to 5 in test_command_prompt_position PASS: gdb.base/commands.exp: if test in test_command_prompt_position PASS: gdb.base/display.exp: display &k PASS: gdb.base/display.exp: display/f f PASS: gdb.base/display.exp: display/s &sum -FAIL: gdb.base/display.exp: first disp -FAIL: gdb.base/display.exp: second disp +PASS: gdb.base/display.exp: first disp +PASS: gdb.base/display.exp: second disp PASS: gdb.base/display.exp: catch err PASS: gdb.base/display.exp: disab disp 1 PASS: gdb.base/display.exp: disab disp 2 PASS: gdb.base/display.exp: re-enab of enab PASS: gdb.base/display.exp: undisp PASS: gdb.base/display.exp: info disp -FAIL: gdb.base/display.exp: next hit +PASS: gdb.base/display.exp: next hit PASS: gdb.base/display.exp: undisp all PASS: gdb.base/display.exp: disab 3 PASS: gdb.base/display.exp: watch off Running ../../../gdb-server_submit/src/gdb/testsuite/gdb.base/recurse.exp ... PASS: gdb.base/recurse.exp: next over b = 0 in first instance PASS: gdb.base/recurse.exp: set first instance watchpoint -FAIL: gdb.base/recurse.exp: continue to first instance watchpoint, first time +PASS: gdb.base/recurse.exp: continue to first instance watchpoint, first time PASS: gdb.base/recurse.exp: continue to recurse (a = 9) PASS: gdb.base/recurse.exp: continue to recurse (a = 8) PASS: gdb.base/recurse.exp: continue to recurse (a = 7) PASS: gdb.base/recurse.exp: continue to recurse (a = 5) PASS: gdb.base/recurse.exp: next over b = 0 in second instance PASS: gdb.base/recurse.exp: set second instance watchpoint -FAIL: gdb.base/recurse.exp: continue to second instance watchpoint, first time +PASS: gdb.base/recurse.exp: continue to second instance watchpoint, first time PASS: gdb.base/recurse.exp: continue to recurse (a = 4) PASS: gdb.base/recurse.exp: continue to recurse (a = 3) PASS: gdb.base/recurse.exp: continue to recurse (a = 2) PASS: gdb.base/recurse.exp: continue to recurse (a = 1) -FAIL: gdb.base/recurse.exp: continue to second instance watchpoint, second time +PASS: gdb.base/recurse.exp: continue to second instance watchpoint, second time PASS: gdb.base/recurse.exp: second instance watchpoint deleted when leaving scope -FAIL: gdb.base/recurse.exp: continue to first instance watchpoint, second time +PASS: gdb.base/recurse.exp: continue to first instance watchpoint, second time PASS: gdb.base/recurse.exp: first instance watchpoint deleted when leaving scope PASS: gdb.base/watchpoint.exp: break func1 PASS: gdb.base/watchpoint.exp: set $func1_breakpoint_number = $bpnum PASS: gdb.base/watchpoint.exp: continue to breakpoint at func1 -FAIL: gdb.base/watchpoint.exp: watchpoint hit, first time -FAIL: gdb.base/watchpoint.exp: watchpoints found in watchpoint/breakpoint table +PASS: gdb.base/watchpoint.exp: watchpoint hit, first time +PASS: gdb.base/watchpoint.exp: Watchpoint hit count is 1 +PASS: gdb.base/watchpoint.exp: delete $func1_breakpoint_number +PASS: gdb.base/watchpoint.exp: watchpoint hit, second time +PASS: gdb.base/watchpoint.exp: Watchpoint hit count is 2 +PASS: gdb.base/watchpoint.exp: watchpoint hit, third time +PASS: gdb.base/watchpoint.exp: Watchpoint hit count is 3 +PASS: gdb.base/watchpoint.exp: watchpoint hit, fourth time +PASS: gdb.base/watchpoint.exp: Watchpoint hit count is 4 +PASS: gdb.base/watchpoint.exp: watchpoint hit, fifth time +PASS: gdb.base/watchpoint.exp: Watchpoint hit count is 5 +PASS: gdb.base/watchpoint.exp: continue to marker2 +PASS: gdb.base/watchpoint.exp: watchpoint disabled +PASS: gdb.base/watchpoint.exp: continue until exit at continue to exit in test_simple_watchpoint +PASS: gdb.base/watchpoint.exp: watchpoints found in watchpoint/breakpoint table PASS: gdb.base/watchpoint.exp: disable watchpoint in test_disabling_watchpoints PASS: gdb.base/watchpoint.exp: run to marker1 in test_disabling_watchpoints PASS: gdb.base/watchpoint.exp: watchpoint enabled -FAIL: gdb.base/watchpoint.exp: watchpoint hit in test_disabling_watchpoints, first time -FAIL: gdb.base/watchpoint.exp: watchpoint hit in test_disabling_watchpoints, second time +PASS: gdb.base/watchpoint.exp: watchpoint hit in test_disabling_watchpoints, first time +PASS: gdb.base/watchpoint.exp: watchpoint hit in test_disabling_watchpoints, second time PASS: gdb.base/watchpoint.exp: disable watchpoint #2 in test_disabling_watchpoints PASS: gdb.base/watchpoint.exp: watchpoint disabled in table PASS: gdb.base/watchpoint.exp: disabled watchpoint skipped PASS: gdb.mi/mi-watch.exp: hw: mi runto callee4 PASS: gdb.mi/mi-watch.exp: hw: break-watch operation PASS: gdb.mi/mi-watch.exp: hw: list of watchpoints -FAIL: gdb.mi/mi-watch.exp: hw: watchpoint trigger (2) +PASS: gdb.mi/mi-watch.exp: hw: watchpoint trigger PASS: gdb.mi/mi-watch.exp: hw: wp out of scope PASS: gdb.mi/mi2-watch.exp: hw: break-watch operation PASS: gdb.mi/mi2-watch.exp: hw: list of watchpoints -FAIL: gdb.mi/mi2-watch.exp: hw: watchpoint trigger (2) +PASS: gdb.mi/mi2-watch.exp: hw: watchpoint trigger PASS: gdb.mi/mi2-watch.exp: hw: wp out of scope PASS: gdb.threads/watchthreads.exp: successfully compiled posix threads test case PASS: gdb.threads/watchthreads.exp: watch args[0] PASS: gdb.threads/watchthreads.exp: watch args[1] -FAIL: gdb.threads/watchthreads.exp: threaded watch loop -FAIL: gdb.threads/watchthreads.exp: first watchpoint on args[0] hit -FAIL: gdb.threads/watchthreads.exp: first watchpoint on args[1] hit -FAIL: gdb.threads/watchthreads.exp: watchpoint on args[0] hit in thread -FAIL: gdb.threads/watchthreads.exp: watchpoint on args[1] hit in thread -FAIL: gdb.threads/watchthreads.exp: combination of threaded watchpoints = 30 +PASS: gdb.threads/watchthreads.exp: threaded watch loop +PASS: gdb.threads/watchthreads.exp: first watchpoint on args[0] hit +PASS: gdb.threads/watchthreads.exp: first watchpoint on args[1] hit +PASS: gdb.threads/watchthreads.exp: watchpoint on args[0] hit in thread +PASS: gdb.threads/watchthreads.exp: watchpoint on args[1] hit in thread +PASS: gdb.threads/watchthreads.exp: combination of threaded watchpoints = 30

OK ?

--
Pedro Alves

2007-11-21  Pedro Alves  <pedro_alves@portugalmail.pt>

	* win32-nat.c (win32_add_thread): Set Dr6 to 0xffff0ff0.
	(win32_continue): Resume threads and set the debug registers
	before calling ContinueDebugEvent.

---
 gdb/win32-nat.c |   51 +++++++++++++++++++++++++--------------------------
 1 file changed, 25 insertions(+), 26 deletions(-)

Index: src/gdb/win32-nat.c
===================================================================
--- src.orig/gdb/win32-nat.c	2007-11-20 03:19:22.000000000 +0000
+++ src/gdb/win32-nat.c	2007-11-20 23:27:58.000000000 +0000
@@ -304,8 +304,7 @@ win32_add_thread (DWORD id, HANDLE h)
       th->context.Dr1 = dr[1];
       th->context.Dr2 = dr[2];
       th->context.Dr3 = dr[3];
-      /* th->context.Dr6 = dr[6];
-      FIXME: should we set dr6 also ?? */
+      th->context.Dr6 = 0xffff0ff0;
       th->context.Dr7 = dr[7];
       CHECK (SetThreadContext (th->h, &th->context));
       th->context.ContextFlags = 0;
@@ -1132,31 +1131,32 @@ win32_continue (DWORD continue_status, i
 		  current_event.dwProcessId, current_event.dwThreadId,
 		  continue_status == DBG_CONTINUE ?
 		  "DBG_CONTINUE" : "DBG_EXCEPTION_NOT_HANDLED"));
+
+  for (th = &thread_head; (th = th->next) != NULL;)
+    if ((id == -1 || id == (int) th->id)
+	&& th->suspended)
+      {
+	if (debug_registers_changed)
+	  {
+	    th->context.ContextFlags |= CONTEXT_DEBUG_REGISTERS;
+	    th->context.Dr0 = dr[0];
+	    th->context.Dr1 = dr[1];
+	    th->context.Dr2 = dr[2];
+	    th->context.Dr3 = dr[3];
+	    th->context.Dr6 = 0xffff0ff0;
+	    th->context.Dr7 = dr[7];
+	  }
+	if (th->context.ContextFlags)
+	  CHECK (SetThreadContext (th->h, &th->context));
+	th->context.ContextFlags = 0;
+	if (th->suspended > 0)
+	  (void) ResumeThread (th->h);
+	th->suspended = 0;
+      }
+
   res = ContinueDebugEvent (current_event.dwProcessId,
 			    current_event.dwThreadId,
 			    continue_status);
-  if (res)
-    for (th = &thread_head; (th = th->next) != NULL;)
-      if (((id == -1) || (id == (int) th->id)) && th->suspended)
-	{
-	  if (th->suspended > 0)
-	    (void) ResumeThread (th->h);
-	  th->suspended = 0;
-	  if (debug_registers_changed)
-	    {
-	      /* Only change the value of the debug registers */
-	      th->context.ContextFlags = CONTEXT_DEBUG_REGISTERS;
-	      th->context.Dr0 = dr[0];
-	      th->context.Dr1 = dr[1];
-	      th->context.Dr2 = dr[2];
-	      th->context.Dr3 = dr[3];
-	      /* th->context.Dr6 = dr[6];
-		 FIXME: should we set dr6 also ?? */
-	      th->context.Dr7 = dr[7];
-	      CHECK (SetThreadContext (th->h, &th->context));
-	      th->context.ContextFlags = 0;
-	    }
-	}
 
   debug_registers_changed = 0;
   return res;
@@ -1242,8 +1242,7 @@ win32_resume (ptid_t ptid, int step, enu
 	      th->context.Dr1 = dr[1];
 	      th->context.Dr2 = dr[2];
 	      th->context.Dr3 = dr[3];
-	      /* th->context.Dr6 = dr[6];
-	       FIXME: should we set dr6 also ?? */
+	      th->context.Dr6 = 0xffff0ff0;
 	      th->context.Dr7 = dr[7];
 	    }
 	  CHECK (SetThreadContext (th->h, &th->context));

Follow-Ups:
- RE: [win32] Fix watchpoint support
  - From: Pierre Muller
- Re: [win32] Fix watchpoint support
  - From: Christopher Faylor

Index Nav:	[Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav:	[Date Prev] [Date Next]	[Thread Prev] [Thread Next]