Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
* [RFA 2] Debug register support in win32-nat.c
@ 2002-01-08  1:21 Pierre Muller
  2002-01-08  1:26 ` Pierre Muller
  0 siblings, 1 reply; 9+ messages in thread
From: Pierre Muller @ 2002-01-08  1:21 UTC (permalink / raw)
  To: gdb-patches

[-- Attachment #1: Type: text/plain, Size: 1639 bytes --]

   This is a follow up of my first
proposal for win32 debug register support
(which enables hardware watchpoints,
I never tested the hardware breakpoints, as they don't have
much advantages over normal breakpoints on i386 processors).

   This second patch does solve one remaining problem,
that was related to the fact that a thead created after a watchpoint was 
enabled didn't get the address watched from beginning of the thread.

  This makes this win32 watchpoints enabled for all threads
(which seems to be different from the linux case, where watchpoints seem to be
thread specific).

   The unwanted output that is generated at each DLL loading
is still not solved, but this is also the case for each dynamic library loading under
i386 linux.

2002-01-08  Pierre Muller  <muller@ics.u-strasbg.fr>

	* win32-nat.c (CONTEXT_DEBUG_DR macro): Add use of CONTEXT_DEBUG_REGISTERS.
	(dr variable): New static array containing a local copy of debug registers.
	(debug_registers_changed): Non zero whenever the debug registers where changed and
	need to be written to inferior.
	(debug_registers_used): Non zero if any debug register was set, used new new threads are created.
	(cygwin_set_dr, cygwin_set_dr7, cygwin_get_dr6): New functions used by i386-nat code.
	(thread_rec): Set dr array if id is the thread of current_event .
	(child_continue, child_resume): Change the debug registers for all threads 
	if debug_registers_changed.
	(child_add_thread): Change the debug registers if debug_registers_used.
	* config/i386/cygwin.mh: Add use of i386-nat.o file.
	Link nm.h to new nm-cygwin.h file.
	+ config/i386/nm-cygwin.h: New file.


[-- Attachment #2: Type: text/plain, Size: 7953 bytes --]

Index: win32-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/win32-nat.c,v
retrieving revision 1.42
diff -u -r1.42 win32-nat.c
--- win32-nat.c	2002/01/08 08:26:42	1.42
+++ win32-nat.c	2002/01/08 09:12:33
@@ -69,11 +69,15 @@
 #include <psapi.h>
 
 #ifdef HAVE_SSE_REGS
-#define CONTEXT_DEBUGGER_DR CONTEXT_DEBUGGER | CONTEXT_EXTENDED_REGISTERS
+#define CONTEXT_DEBUGGER_DR CONTEXT_DEBUGGER | CONTEXT_DEBUG_REGISTERS \
+	| CONTEXT_EXTENDED_REGISTERS
 #else
-#define CONTEXT_DEBUGGER_DR CONTEXT_DEBUGGER
+#define CONTEXT_DEBUGGER_DR CONTEXT_DEBUGGER | CONTEXT_DEBUG_REGISTERS
 #endif
 
+static unsigned dr[8];
+static int debug_registers_changed = 0;
+static int debug_registers_used = 0;
 
 /* The string sent by cygwin when it processes a signal.
    FIXME: This should be in a cygwin include file. */
@@ -214,6 +218,14 @@
   {EXCEPTION_SINGLE_STEP, TARGET_SIGNAL_TRAP},
   {-1, -1}};
 
+static void
+check (BOOL ok, const char *file, int line)
+{
+  if (!ok)
+    printf_filtered ("error return %s:%d was %lu\n", file, line, GetLastError ());
+}
+
+
 /* Find a thread record given a thread id.
    If get_context then also retrieve the context for this
    thread. */
@@ -234,6 +246,16 @@
 
 	    th->context.ContextFlags = CONTEXT_DEBUGGER_DR;
 	    GetThreadContext (th->h, &th->context);
+	    if (id == current_event.dwThreadId)
+	      {
+		/* Copy dr values from that thread.  */
+		dr[0] = th->context.Dr0;
+		dr[1] = th->context.Dr1;
+		dr[2] = th->context.Dr2;
+		dr[3] = th->context.Dr3;
+		dr[6] = th->context.Dr6;
+		dr[7] = th->context.Dr7;
+	      }
 	  }
 	return th;
       }
@@ -257,6 +279,22 @@
   th->next = thread_head.next;
   thread_head.next = th;
   add_thread (pid_to_ptid (id));
+  /* Set the debug registers for the new thread in they are used.  */ 
+  if (debug_registers_used)
+    {
+      /* Only change the value of the debug registers.  */
+      th->context.ContextFlags = CONTEXT_DEBUG_REGISTERS;
+      CHECK (GetThreadContext (th->h, &th->context));
+      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;
+    }
   return th;
 }
 
@@ -303,13 +341,6 @@
 }
 
 static void
-check (BOOL ok, const char *file, int line)
-{
-  if (!ok)
-    printf_filtered ("error return %s:%d was %lu\n", file, line, GetLastError ());
-}
-
-static void
 do_child_fetch_inferior_registers (int r)
 {
   char *context_offset = ((char *) &current_thread->context) + mappings[r];
@@ -876,11 +907,27 @@
     for (th = &thread_head; (th = th->next) != NULL;)
       if (((id == -1) || (id == (int) th->id)) && th->suspend_count)
 	{
+
 	  for (i = 0; i < th->suspend_count; i++)
 	    (void) ResumeThread (th->h);
 	  th->suspend_count = 0;
+	  if (debug_registers_changed)
+	    {
+	      /* Only change the value of the debug reisters */
+	      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;
 }
 
@@ -1060,10 +1107,15 @@
 do_initial_child_stuff (DWORD pid)
 {
   extern int stop_after_trap;
+  int i;
 
   last_sig = 0;
   event_count = 0;
   exception_count = 0;
+  debug_registers_changed = 0;
+  debug_registers_used = 0;  
+  for (i = 0; i <= 7; i++)
+    dr[i] = 0;
   current_event.dwProcessId = pid;
   memset (&current_event, 0, sizeof (current_event));
   push_target (&child_ops);
@@ -1343,6 +1395,7 @@
 child_mourn_inferior (void)
 {
   (void) child_continue (DBG_CONTINUE, -1);
+  i386_cleanup_dregs();
   unpush_target (&child_ops);
   generic_mourn_inferior ();
 }
@@ -1430,6 +1483,16 @@
 
       if (th->context.ContextFlags)
 	{
+     if (debug_registers_changed)
+       {
+          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;
 	}
@@ -1562,6 +1625,43 @@
 
   add_target (&child_ops);
 }
+
+/* Hardware watchpoint support, adapted from go32-nat.c code.  */
+
+/* Pass the address ADDR to the inferior in the I'th debug register.
+   Here we just store the address in dr array, the registers will be
+   actually set up when child_continue is called.  */
+void
+cygwin_set_dr (int i, CORE_ADDR addr)
+{
+  if (i < 0 || i > 3)
+    internal_error (__FILE__, __LINE__,
+		    "Invalid register %d in cygwin_set_dr.\n", i);
+  dr[i] = (unsigned) addr;
+  debug_registers_changed = 1;
+  debug_registers_used = 1;
+}
+
+/* Pass the value VAL to the inferior in the DR7 debug control
+   register.  Here we just store the address in D_REGS, the watchpoint
+   will be actually set up in child_wait.  */
+void
+cygwin_set_dr7 (unsigned val)
+{
+  dr[7] = val;
+  debug_registers_changed = 1;
+  debug_registers_used = 1;
+}
+
+/* Get the value of the DR6 debug status register from the inferior.
+   Here we just return the value stored in dr[6]
+   by the last call to thread_rec for current_event.dwThreadId id.  */
+unsigned
+cygwin_get_dr6 (void)
+{
+  return dr[6];
+}
+
 
 /* Determine if the thread referenced by "pid" is alive
    by "polling" it.  If WaitForSingleObject returns WAIT_OBJECT_0
Index: config/i386/cygwin.mh
===================================================================
RCS file: /cvs/src/src/gdb/config/i386/cygwin.mh,v
retrieving revision 1.4
diff -u -r1.4 cygwin.mh
--- cygwin.mh	2000/08/27 04:21:35	1.4
+++ cygwin.mh	2002/01/08 09:12:33
@@ -1,6 +1,6 @@
 MH_CFLAGS=
 XM_FILE=xm-cygwin.h
 XDEPFILES=
-NATDEPFILES= win32-nat.o corelow.o
-NAT_FILE=../none/nm-none.h
+NATDEPFILES= i386-nat.o win32-nat.o corelow.o
+NAT_FILE=nm-cygwin.h
 XM_CLIBS=
Index: config/i386/nm-cygwin.h
===================================================================
RCS file: nm-cygwin.h
diff -N nm-cygwin.h
--- /dev/null	Tue May  5 13:32:27 1998
+++ nm-cygwin.h	Tue Jan  8 01:12:33 2002
@@ -0,0 +1,38 @@
+/* Native definitions for Intel x86 running CYGWIN.
+   Copyright (C) 2001 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 2 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, write to the Free Software
+   Foundation, Inc., 59 Temple Place - Suite 330,
+   Boston, MA 02111-1307, USA.  */
+
+#define NO_PTRACE_H
+
+#define I386_USE_GENERIC_WATCHPOINTS
+
+#include "i386/nm-i386.h"
+
+/* Support for hardware-assisted breakpoints and watchpoints.  */
+
+#define I386_DR_LOW_SET_CONTROL(VAL)	cygwin_set_dr7 (VAL)
+extern void cygwin_set_dr7 (unsigned);
+
+#define I386_DR_LOW_SET_ADDR(N,ADDR)	cygwin_set_dr (N,ADDR)
+extern void cygwin_set_dr (int, CORE_ADDR);
+
+#define I386_DR_LOW_RESET_ADDR(N)
+
+#define I386_DR_LOW_GET_STATUS()	cygwin_get_dr6 ()
+extern unsigned cygwin_get_dr6 (void);

[-- Attachment #3: Type: text/plain, Size: 176 bytes --]



Pierre Muller
Institut Charles Sadron
6,rue Boussingault
F 67083 STRASBOURG CEDEX (France)
mailto:muller@ics.u-strasbg.fr
Phone : (33)-3-88-41-40-07  Fax : (33)-3-88-41-40-99

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFA 2] Debug register support in win32-nat.c
  2002-01-08  1:21 [RFA 2] Debug register support in win32-nat.c Pierre Muller
@ 2002-01-08  1:26 ` Pierre Muller
  2002-01-13 10:38   ` Christopher Faylor
  0 siblings, 1 reply; 9+ messages in thread
From: Pierre Muller @ 2002-01-08  1:26 UTC (permalink / raw)
  To: gdb-patches

At 10:20 08/01/2002 , vous avez écrit:
>    This is a follow up of my first
>proposal for win32 debug register support
>(which enables hardware watchpoints,
>I never tested the hardware breakpoints, as they don't have
>much advantages over normal breakpoints on i386 processors).

Sorry, I forgot to add the link
http://sources.redhat.com/ml/gdb-patches/2001-11/msg00537.html
and all the follow-ups.


Pierre Muller
Institut Charles Sadron
6,rue Boussingault
F 67083 STRASBOURG CEDEX (France)
mailto:muller@ics.u-strasbg.fr
Phone : (33)-3-88-41-40-07  Fax : (33)-3-88-41-40-99


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFA 2] Debug register support in win32-nat.c
  2002-01-08  1:26 ` Pierre Muller
@ 2002-01-13 10:38   ` Christopher Faylor
  2002-01-13 15:21     ` muller
  0 siblings, 1 reply; 9+ messages in thread
From: Christopher Faylor @ 2002-01-13 10:38 UTC (permalink / raw)
  To: gdb-patches

On Tue, Jan 08, 2002 at 10:26:12AM +0100, Pierre Muller wrote:
>At 10:20 08/01/2002 , vous avez ?crit:
>>    This is a follow up of my first
>>proposal for win32 debug register support
>>(which enables hardware watchpoints,
>>I never tested the hardware breakpoints, as they don't have
>>much advantages over normal breakpoints on i386 processors).
>
>Sorry, I forgot to add the link
>http://sources.redhat.com/ml/gdb-patches/2001-11/msg00537.html
>and all the follow-ups.

I applied this patch but it doesn't seem to build.  I get a:
libgdb.a(win32-nat.o): In function `child_mourn_inferior':
/cygnus/src/uberbaum/gdb/win32-nat.c:1398: undefined reference to `_i386_cleanup_dregs'

I assume that this is related to your other patch.

In the meantime, I noticed a couple of things:

- ChangeLog needs to be wrapped to 80 columns.

- ChangeLog wording needs more verbs and more description.  For instance:
        (debug_registers_changed): Non zero whenever the debug registers where changed and
        need to be written to inferior.
   You need to mention that this is a new variable:
        (debug_registers_changed): New variable.  Reflects when debug registers are changed and
        need to be written to inferior.

- In do_initial_child_stuff, I'd prefer that you either use sizeof to
  derive the size of the dr array for zeroing or use a defined constant,
  rather than just a raw "7".

- I'm wondering if your implementation is thread safe?  You're storing
  debug registers in a global array and copying them into a structure
  as needed.  Couldn't they just be stored in the per-thread structure?
  You could add a debug_registers_used value to the structure, if necessary.

cgf


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFA 2] Debug register support in win32-nat.c
  2002-01-13 10:38   ` Christopher Faylor
@ 2002-01-13 15:21     ` muller
  2002-01-13 17:58       ` [RFA 2] Debug register support in win32-nat.c (need opinions) Christopher Faylor
  0 siblings, 1 reply; 9+ messages in thread
From: muller @ 2002-01-13 15:21 UTC (permalink / raw)
  To: gdb-patches

At 13:39 13/01/02 -0500, Christopher Faylor wrote:
>On Tue, Jan 08, 2002 at 10:26:12AM +0100, Pierre Muller wrote:
>>At 10:20 08/01/2002 , vous avez ?crit:
>>>    This is a follow up of my first
>>>proposal for win32 debug register support
>>>(which enables hardware watchpoints,
>>>I never tested the hardware breakpoints, as they don't have
>>>much advantages over normal breakpoints on i386 processors).
>>
>>Sorry, I forgot to add the link
>>http://sources.redhat.com/ml/gdb-patches/2001-11/msg00537.html
>>and all the follow-ups.
>
>I applied this patch but it doesn't seem to build.  I get a:
>libgdb.a(win32-nat.o): In function `child_mourn_inferior':
>/cygnus/src/uberbaum/gdb/win32-nat.c:1398: undefined reference to
`_i386_cleanup_dregs'

Did I forget to incorporate the change in config/i386/cygwin.mh ?
No, I rechecked the reference patch and it is included.

Did you rerun configure in gdb build directory?
This is necessary as the nm.h link
is changed by my patch to cygwin.mh,
but might be missed by the Makefile itself.
Run ../../src/gdb/configure in build/gdb
if src and build dirs are at same dir level should suppress
that error.

>I assume that this is related to your other patch.
>
>In the meantime, I noticed a couple of things:
>
>- ChangeLog needs to be wrapped to 80 columns.
>
>- ChangeLog wording needs more verbs and more description.  For instance:
>        (debug_registers_changed): Non zero whenever the debug registers
where changed and
>        need to be written to inferior.
>   You need to mention that this is a new variable:
>        (debug_registers_changed): New variable.  Reflects when debug
registers are changed and
>        need to be written to inferior.
>
>- In do_initial_child_stuff, I'd prefer that you either use sizeof to
>  derive the size of the dr array for zeroing or use a defined constant,
>  rather than just a raw "7".

OK, so I should rewrite it to 
+  for (i = 0; i < sizeof (dr) / sizeof (dr[0]); i++)
+    dr[i] = 0;
   
Is that correct?
(Please excuse me to still ask so basic C questions,
but I never used C outside GDB code itself...)

>- I'm wondering if your implementation is thread safe?  You're storing
>  debug registers in a global array and copying them into a structure
>  as needed.  Couldn't they just be stored in the per-thread structure?
>  You could add a debug_registers_used value to the structure, if necessary.

The basic idea of my implementation is that 
watchpoints should be triggered by an expression that is modifed by
any thread, and thus the same value should be written to 
all threads. 
This means that:
 - we only need one copy of the debug registers.
 - we need to write their value to all threads
(including newly created ones, which is the major difference
between this patch respective to the first patch).
 - this of course assumes that the debuggee does not itself 
change its debug registers, in that case the patch
will not work correctly, but I think that this 
is a reasonable limitation.

I agree that the linux implementation 
does not set the debug registers for all threads but this
means that if a watched expression is modified by another
thread than the current thread at the time of setting
the watchpoint will not be caught and that is much
worse...

 If Christopher is able to build and test my patch
after a rerun of the configure scriptand if
he agrees with my remarks above, I will be happy 
to resubmit a third patch taking his remarks into account.
Thanks for reviewing my patch, Christopher.




^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFA 2] Debug register support in win32-nat.c  (need opinions)
  2002-01-13 15:21     ` muller
@ 2002-01-13 17:58       ` Christopher Faylor
  2002-01-14  0:15         ` Eli Zaretskii
  0 siblings, 1 reply; 9+ messages in thread
From: Christopher Faylor @ 2002-01-13 17:58 UTC (permalink / raw)
  To: gdb-patches

On Mon, Jan 14, 2002 at 12:33:26AM +0100, muller@cerbere.u-strasbg.fr wrote:
>At 13:39 13/01/02 -0500, Christopher Faylor wrote:
>>I applied this patch but it doesn't seem to build.  I get a:
>>libgdb.a(win32-nat.o): In function `child_mourn_inferior':
>>/cygnus/src/uberbaum/gdb/win32-nat.c:1398: undefined reference to `_i386_cleanup_dregs'
>
>Did I forget to incorporate the change in config/i386/cygwin.mh ?
>No, I rechecked the reference patch and it is included.
>
>Did you rerun configure in gdb build directory?

I did reconfigure, yes.  However, I just wiped out my build directory
and reconfigured.  It built now.  Sorry for the false alarm.

>>- In do_initial_child_stuff, I'd prefer that you either use sizeof to
>>  derive the size of the dr array for zeroing or use a defined constant,
>>  rather than just a raw "7".
>
>OK, so I should rewrite it to 
>+  for (i = 0; i < sizeof (dr) / sizeof (dr[0]); i++)
>+    dr[i] = 0;
>   
>Is that correct?  (Please excuse me to still ask so basic C questions,
>but I never used C outside GDB code itself...)

That's ok, yes.  You could write a simple test case to convince yourself
of this, if you wanted.

>>- I'm wondering if your implementation is thread safe?  You're storing
>>  debug registers in a global array and copying them into a structure
>>  as needed.  Couldn't they just be stored in the per-thread structure?
>>  You could add a debug_registers_used value to the structure, if necessary.
>
>The basic idea of my implementation is that 
>watchpoints should be triggered by an expression that is modifed by
>any thread, and thus the same value should be written to 
>all threads. 
>This means that:
> - we only need one copy of the debug registers.
> - we need to write their value to all threads
>(including newly created ones, which is the major difference between
>this patch respective to the first patch).  - this of course assumes
>that the debuggee does not itself change its debug registers, in that
>case the patch will not work correctly, but I think that this is a
>reasonable limitation.
>
>I agree that the linux implementation does not set the debug registers
>for all threads but this means that if a watched expression is modified
>by another thread than the current thread at the time of setting the
>watchpoint will not be caught and that is much worse...

You described this in your original email.  I should have responded to
it.

I don't think it makes sense to make gratuitous changes to the way gdb
works.  If you're implementing an improvement for gdb for Windows then
I think it should probably work the same way for Windows as it does
for linux.

I guess I need a ruling from more experienced maintainers about this.

How should gdb behave in this scenario?

cgf


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFA 2] Debug register support in win32-nat.c (need opinions)
  2002-01-13 17:58       ` [RFA 2] Debug register support in win32-nat.c (need opinions) Christopher Faylor
@ 2002-01-14  0:15         ` Eli Zaretskii
  2002-01-14  0:33           ` Pierre Muller
  0 siblings, 1 reply; 9+ messages in thread
From: Eli Zaretskii @ 2002-01-14  0:15 UTC (permalink / raw)
  To: Christopher Faylor; +Cc: gdb-patches


On Sun, 13 Jan 2002, Christopher Faylor wrote:

> >I agree that the linux implementation does not set the debug registers
> >for all threads but this means that if a watched expression is modified
> >by another thread than the current thread at the time of setting the
> >watchpoint will not be caught and that is much worse...
> 
> You described this in your original email.  I should have responded to
> it.
> 
> I don't think it makes sense to make gratuitous changes to the way gdb
> works.  If you're implementing an improvement for gdb for Windows then
> I think it should probably work the same way for Windows as it does
> for linux.
> 
> I guess I need a ruling from more experienced maintainers about this.
> 
> How should gdb behave in this scenario?

I'm not an expert, so this is FWIW:

The basic assumption behind the generic x86 watchpoint code in
i386-nat.c is that the watchpoints are not thread-specific.  That's
why i386-nat.c stores the info in a single array that doesn't have
thread information, and that's why the I386_DR_LOW_* macros don't
accept a thread id as an argument.

IIRC, this issue was discussed back when I published the first draft
of the watchpoint API, and the general consensus was that I shouldn't
bother about thread-specific watchpoints.  You may wish to reread that
discussion (I can dig out a pointer to it if you cannot find it.)

I also agree with Pierre that global watchpoints are much better than
thread-local ones.  For starters, you can always write a condition for
a global watchpoint that lets the debuggee continue if the thread id
is not what you want; but pulling the reverse trick with thread-local
watchpoints is impossible.

So if indeed GNU/Linux versions of GDB set watchpoints on a per thread
basis (I'm surprised they do), I think that's a misfeature, to say the
least.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFA 2] Debug register support in win32-nat.c (need opinions)
  2002-01-14  0:15         ` Eli Zaretskii
@ 2002-01-14  0:33           ` Pierre Muller
  2002-01-30  9:20             ` Christopher Faylor
  0 siblings, 1 reply; 9+ messages in thread
From: Pierre Muller @ 2002-01-14  0:33 UTC (permalink / raw)
  To: gdb-patches

At 09:14 14/01/2002 , Eli Zaretskii a écrit:

>On Sun, 13 Jan 2002, Christopher Faylor wrote:
>
> > >I agree that the linux implementation does not set the debug registers
> > >for all threads but this means that if a watched expression is modified
> > >by another thread than the current thread at the time of setting the
> > >watchpoint will not be caught and that is much worse...
> > 
> > You described this in your original email.  I should have responded to
> > it.
> > 
> > I don't think it makes sense to make gratuitous changes to the way gdb
> > works.  If you're implementing an improvement for gdb for Windows then
> > I think it should probably work the same way for Windows as it does
> > for linux.
> > 
> > I guess I need a ruling from more experienced maintainers about this.
> > 
> > How should gdb behave in this scenario?
>
>I'm not an expert, so this is FWIW:
>
>The basic assumption behind the generic x86 watchpoint code in
>i386-nat.c is that the watchpoints are not thread-specific.  That's
>why i386-nat.c stores the info in a single array that doesn't have
>thread information, and that's why the I386_DR_LOW_* macros don't
>accept a thread id as an argument.

Anyway, it is not even possible yet to specify a thread index for
a watchpoint, the command pareser only accepts a thread argument
for normal breakpoints.

>IIRC, this issue was discussed back when I published the first draft
>of the watchpoint API, and the general consensus was that I shouldn't
>bother about thread-specific watchpoints.  You may wish to reread that
>discussion (I can dig out a pointer to it if you cannot find it.)
>
>I also agree with Pierre that global watchpoints are much better than
>thread-local ones.  For starters, you can always write a condition for
>a global watchpoint that lets the debuggee continue if the thread id
>is not what you want; but pulling the reverse trick with thread-local
>watchpoints is impossible.

Thanks for your comments, Eli.

>So if indeed GNU/Linux versions of GDB set watchpoints on a per thread
>basis (I'm surprised they do), I think that's a misfeature, to say the
>least.

  You can see this is a comment added by Mark Kettenis
at line 707 of i386-linux-nat.c file:
   /* FIXME: kettenis/2001-01-29: It's not clear what we should do with
      multi-threaded processes here.  For now, pretend there is just
      one thread.  */

Mark, wouldn't it be just enough to loop over the the thread chain 
and call ptrace for each pid?

In any case, this comment clearly shows that the coreect behavior of a watchpoint 
should be that it is triggered in all threads.

Christopher, does this answer your concerns?





Pierre Muller
Institut Charles Sadron
6,rue Boussingault
F 67083 STRASBOURG CEDEX (France)
mailto:muller@ics.u-strasbg.fr
Phone : (33)-3-88-41-40-07  Fax : (33)-3-88-41-40-99


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFA 2] Debug register support in win32-nat.c (need opinions)
  2002-01-14  0:33           ` Pierre Muller
@ 2002-01-30  9:20             ` Christopher Faylor
  2002-02-04  3:04               ` Pierre Muller
  0 siblings, 1 reply; 9+ messages in thread
From: Christopher Faylor @ 2002-01-30  9:20 UTC (permalink / raw)
  To: gdb-patches

On Mon, Jan 14, 2002 at 09:33:27AM +0100, Pierre Muller wrote:
>Christopher, does this answer your concerns?

I don't think I saw any dissenting opinions, so go ahead and check
your changes in, Pierre.

Thanks!

cgf


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFA 2] Debug register support in win32-nat.c (need opinions)
  2002-01-30  9:20             ` Christopher Faylor
@ 2002-02-04  3:04               ` Pierre Muller
  0 siblings, 0 replies; 9+ messages in thread
From: Pierre Muller @ 2002-02-04  3:04 UTC (permalink / raw)
  To: gdb-patches

At 18:20 30/01/2002 , vous avez écrit:
>On Mon, Jan 14, 2002 at 09:33:27AM +0100, Pierre Muller wrote:
> >Christopher, does this answer your concerns?
>
>I don't think I saw any dissenting opinions, so go ahead and check
>your changes in, Pierre.

OK, I checked this in with the minor modifications in the code discussed and with this
modified log entry.

PS:
   I must confess that I was unable to recheck my patch as there seems to be a 
configure problem for cygwin lately.


2002-02-04  Pierre Muller  <muller@ics.u-strasbg.fr>
         Add support for hardware watchpoints for win32 native.
         * win32-nat.c (CONTEXT_DEBUG_DR macro): Add use of 
         CONTEXT_DEBUG_REGISTERS.
         (dr variable): New variable. Static array containing a local copy 
         of debug registers.
         (debug_registers_changed): New variable.  Reflects when debug registers
         are changed and need to be written to inferior.
         (debug_registers_used): New variable. Reflects when any debug register 
         was set, used when new threads are created.
         (cygwin_set_dr, cygwin_set_dr7, cygwin_get_dr6): New functions used by
         i386-nat code.
         (thread_rec): Set dr array if id is the thread of current_event .
         (child_continue, child_resume): Change the debug registers for all
         threads if debug_registers_changed.
         (child_add_thread): Change the debug registers if debug_registers_used.
         * config/i386/cygwin.mh: Add use of i386-nat.o file.
         Link nm.h to new nm-cygwin.h file.
         + config/i386/nm-cygwin.h: New file. Contains the macros used for use
         of hardware registers.



Pierre Muller
Institut Charles Sadron
6,rue Boussingault
F 67083 STRASBOURG CEDEX (France)
mailto:muller@ics.u-strasbg.fr
Phone : (33)-3-88-41-40-07  Fax : (33)-3-88-41-40-99


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2002-02-04 11:04 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2002-01-08  1:21 [RFA 2] Debug register support in win32-nat.c Pierre Muller
2002-01-08  1:26 ` Pierre Muller
2002-01-13 10:38   ` Christopher Faylor
2002-01-13 15:21     ` muller
2002-01-13 17:58       ` [RFA 2] Debug register support in win32-nat.c (need opinions) Christopher Faylor
2002-01-14  0:15         ` Eli Zaretskii
2002-01-14  0:33           ` Pierre Muller
2002-01-30  9:20             ` Christopher Faylor
2002-02-04  3:04               ` Pierre Muller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox