[RFC/RFA] Add hardware watchpoint support for cygwin target.

[RFC/RFA] Add hardware watchpoint support for cygwin target.

[Pierre Muller]

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


    Following DJ Delorie's suggestion to use
CONTEXT_DEBUG_REGISTERS, I tested the
use of i386 debug registers to set hardware watchpoints.

   It works successfully on my Win2000 box here.

But te are some annoying things,
the most annoying is that an exception seems to be generated
on read access of the watched area even if you only set a normal
watchpoint (which should use a write-only debug feature).

    This results in lost of garbage written on output due
to the fact that mention function is called for watchpoints
even if the value remains unchanged and thus no stop is done...
    It is unclear to me if this is a bug in the breakpoint code or
if i386 code should take care of this issue.

   To Eli:
    If you test this patch, that contains a call to i386_cleanup_dregs(),
you can test the failure if you comment the call out...

PS:  the patch should be a Dos style text file,
Eudora seems completely unable to send Unix style text file....
Hopefully this will not create other problems.

2001-11-28 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.
	(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.
	* 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.





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 

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

Index: win32-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/win32-nat.c,v
retrieving revision 1.40
diff -r1.40 win32-nat.c
71c71,72
< #define CONTEXT_DEBUGGER_DR CONTEXT_DEBUGGER | CONTEXT_EXTENDED_REGISTERS
---
> #define CONTEXT_DEBUGGER_DR CONTEXT_DEBUGGER | CONTEXT_DEBUG_REGISTERS \
> 	| CONTEXT_EXTENDED_REGISTERS
73c74
< #define CONTEXT_DEBUGGER_DR CONTEXT_DEBUGGER
---
> #define CONTEXT_DEBUGGER_DR CONTEXT_DEBUGGER | CONTEXT_DEBUG_REGISTERS
75a77,79
> static unsigned dr[8];
> static int debug_registers_changed = 0;
> 
235a240,249
> 	    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;
> 	      }
877a892,904
> 	  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];
>               th->context.Dr7 = dr[7];
> 	      CHECK (SetThreadContext (th->h, &th->context));
>               th->context.ContextFlags = 0;
> 	    }
882a910
>   debug_registers_changed = 0;
1061a1090
>   int i;
1065a1095,1097
>   debug_registers_changed = 0;
>   for (i = 0; i <= 7; i++)
>     dr[i] = 0;
1344a1377
>   i386_cleanup_dregs();
1411a1445
> 
1431a1466,1474
> 	  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];
> 	      th->context.Dr7 = dr[7];
> 	    }
1563a1607,1641
> 
> /* 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;
> }
> 
> /* 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;
> }
> 
> /* Get the value of the DR6 debug status register from the inferior.
>    Here we just return the value stored in D_REGS, as we've got it
>    from the last go32_wait call.  */
> unsigned
> cygwin_get_dr6 (void)
> {
>   return dr[6];
> }
> 
Index: config/i386/cygwin.mh
===================================================================
RCS file: /cvs/src/src/gdb/config/i386/cygwin.mh,v
retrieving revision 1.4
diff -r1.4 cygwin.mh
4,5c4,5
< 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
/* 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

[RFC/RFA] Add hardware watchpoint support for cygwin target.

[Pierre Muller]

    Following DJ Delorie's suggestion to use
CONTEXT_DEBUG_REGISTERS, I tested the
use of i386 debug registers to set hardware watchpoints.

   It works successfully on my Win2000 box here.

But te are some annoying things,
the most annoying is that an exception seems to be generated
on read access of the watched area even if you only set a normal
watchpoint (which should use a write-only debug feature).

    This results in lost of garbage written on output due
to the fact that mention function is called for watchpoints
even if the value remains unchanged and thus no stop is done...
    It is unclear to me if this is a bug in the breakpoint code or
if i386 code should take care of this issue.

   To Eli:
    If you test this patch, that contains a call to i386_cleanup_dregs(),
you can test the failure if you comment the call out...

PS:  the patch should be a Dos style text file,
Eudora seems completely unable to send Unix style text file....
Hopefully this will not create other problems.

2001-11-28 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.
	(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.
	* 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.





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 
Index: win32-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/win32-nat.c,v
retrieving revision 1.40
diff -r1.40 win32-nat.c
71c71,72
< #define CONTEXT_DEBUGGER_DR CONTEXT_DEBUGGER | CONTEXT_EXTENDED_REGISTERS
---
> #define CONTEXT_DEBUGGER_DR CONTEXT_DEBUGGER | CONTEXT_DEBUG_REGISTERS \
> 	| CONTEXT_EXTENDED_REGISTERS
73c74
< #define CONTEXT_DEBUGGER_DR CONTEXT_DEBUGGER
---
> #define CONTEXT_DEBUGGER_DR CONTEXT_DEBUGGER | CONTEXT_DEBUG_REGISTERS
75a77,79
> static unsigned dr[8];
> static int debug_registers_changed = 0;
> 
235a240,249
> 	    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;
> 	      }
877a892,904
> 	  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];
>               th->context.Dr7 = dr[7];
> 	      CHECK (SetThreadContext (th->h, &th->context));
>               th->context.ContextFlags = 0;
> 	    }
882a910
>   debug_registers_changed = 0;
1061a1090
>   int i;
1065a1095,1097
>   debug_registers_changed = 0;
>   for (i = 0; i <= 7; i++)
>     dr[i] = 0;
1344a1377
>   i386_cleanup_dregs();
1411a1445
> 
1431a1466,1474
> 	  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];
> 	      th->context.Dr7 = dr[7];
> 	    }
1563a1607,1641
> 
> /* 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;
> }
> 
> /* 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;
> }
> 
> /* Get the value of the DR6 debug status register from the inferior.
>    Here we just return the value stored in D_REGS, as we've got it
>    from the last go32_wait call.  */
> unsigned
> cygwin_get_dr6 (void)
> {
>   return dr[6];
> }
> 
Index: config/i386/cygwin.mh
===================================================================
RCS file: /cvs/src/src/gdb/config/i386/cygwin.mh,v
retrieving revision 1.4
diff -r1.4 cygwin.mh
4,5c4,5
< 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
/* 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);
From eliz@is.elta.co.il Wed Nov 28 10:05:00 2001
From: "Eli Zaretskii" <eliz@is.elta.co.il>
To: orjan.friberg@axis.com
Cc: gdb-patches@sources.redhat.com
Subject: Re: [RFC]: Solib search (Was: Re: Cross solib support; continued)
Date: Wed, 28 Nov 2001 10:05:00 -0000
Message-id: <2950-Wed28Nov2001200425+0200-eliz@is.elta.co.il>
References: <3BEAA3A0.586B3046@axis.com> <20011108110955.A12240@nevyn.them.org> <3C03AB51.DB27B3D4@axis.com> <3405-Tue27Nov2001175932+0200-eliz@is.elta.co.il> <3C04B5A9.1F054A9F@axis.com> <7458-Wed28Nov2001183554+0200-eliz@is.elta.co.il> <3C052245.8C02668E@axis.com>
X-SW-Source: 2001-11/msg00538.html
Content-length: 884

> Date: Wed, 28 Nov 2001 18:43:33 +0100
> From: Orjan Friberg <orjan.friberg@axis.com>
> 
> I was trying to address the situation where in_pathname contains several
> leading slashes; then your suggested code (and my original code also)
> would terminate too early.  I guess we don't normally care about
> multiple leading slashes since it's a valid path, but in this case we
> need to get rid of all of them to make it a relative path.
> 
> This code should cut it:
> 
>   if (found_file < 0 && IS_ABSOLUTE_PATH (in_pathname))
>     {
>       /* First, get rid of any drive letters etc.  */
>       while (!IS_DIR_SEPARATOR (*in_pathname))
>         in_pathname++;
> 
>       /* Next, get rid of all leading dir separators.  */
>       while (IS_DIR_SEPARATOR (*in_pathname))
>         in_pathname++;
>     }

Yes, this does what you want, AFAICS.

Thanks for explaining the issue.

Re: [RFC/RFA] Add hardware watchpoint support for cygwin target.

[Eli Zaretskii]

> Date: Wed, 28 Nov 2001 18:44:44 +0100
> From: Pierre Muller <muller@cerbere.u-strasbg.fr>
> 
> But te are some annoying things,
> the most annoying is that an exception seems to be generated
> on read access of the watched area even if you only set a normal
> watchpoint (which should use a write-only debug feature).

So you are saying that watch, rwatch, and awatch all yield the same
behavior?

Are you sure that you pass the watchpoint information correctly to
the OS?  For example, is the format of DR7 as the OS wants it
identical to what GDB uses?  The layout of bits in dr_control_mirror
follows Intel documentation, but the OS might want those bits in a
different format (that's what the corresponding DPMI call does, for
example).  I don't have Windows docs, so I cannot check this.

> > /* Get the value of the DR6 debug status register from the inferior.
> >    Here we just return the value stored in D_REGS, as we've got it
> >    from the last go32_wait call.  */

I believe you didn't really mean ``go32_wait'' here ;-)

Re: [RFC/RFA] Add hardware watchpoint support for cygwin target.

[Eli Zaretskii]

> Date: Wed, 28 Nov 2001 18:44:44 +0100
> From: Pierre Muller <muller@cerbere.u-strasbg.fr>
> 
> But te are some annoying things,
> the most annoying is that an exception seems to be generated
> on read access of the watched area even if you only set a normal
> watchpoint (which should use a write-only debug feature).

So you are saying that watch, rwatch, and awatch all yield the same
behavior?

Are you sure that you pass the watchpoint information correctly to
the OS?  For example, is the format of DR7 as the OS wants it
identical to what GDB uses?  The layout of bits in dr_control_mirror
follows Intel documentation, but the OS might want those bits in a
different format (that's what the corresponding DPMI call does, for
example).  I don't have Windows docs, so I cannot check this.

> > /* Get the value of the DR6 debug status register from the inferior.
> >    Here we just return the value stored in D_REGS, as we've got it
> >    from the last go32_wait call.  */

I believe you didn't really mean ``go32_wait'' here ;-)

Re: [RFC/RFA] Add hardware watchpoint support for cygwin target.

[Christopher Faylor]

On Wed, Nov 28, 2001 at 08:13:12PM +0200, Eli Zaretskii wrote:
>> Date: Wed, 28 Nov 2001 18:44:44 +0100
>> From: Pierre Muller <muller@cerbere.u-strasbg.fr>
>> 
>> But te are some annoying things,
>> the most annoying is that an exception seems to be generated
>> on read access of the watched area even if you only set a normal
>> watchpoint (which should use a write-only debug feature).
>
>So you are saying that watch, rwatch, and awatch all yield the same
>behavior?
>
>Are you sure that you pass the watchpoint information correctly to
>the OS?  For example, is the format of DR7 as the OS wants it
>identical to what GDB uses?  The layout of bits in dr_control_mirror
>follows Intel documentation, but the OS might want those bits in a
>different format (that's what the corresponding DPMI call does, for
>example).  I don't have Windows docs, so I cannot check this.
>
>> > /* Get the value of the DR6 debug status register from the inferior.
>> >    Here we just return the value stored in D_REGS, as we've got it
>> >    from the last go32_wait call.  */
>
>I believe you didn't really mean ``go32_wait'' here ;-)

I'd like some clarification on this before I can accept the patch.  It
seems like the described behavior would be annoying indeed.  It would
be nice to fix this.

Re: [RFC/RFA] Add hardware watchpoint support for cygwin target.

[muller]

At 14:30 28/11/01 -0500, Christopher Faylor wrote:
>On Wed, Nov 28, 2001 at 08:13:12PM +0200, Eli Zaretskii wrote:
>>> Date: Wed, 28 Nov 2001 18:44:44 +0100
>>> From: Pierre Muller <muller@cerbere.u-strasbg.fr>
>>> 
>>> But te are some annoying things,
>>> the most annoying is that an exception seems to be generated
>>> on read access of the watched area even if you only set a normal
>>> watchpoint (which should use a write-only debug feature).
>>
>>So you are saying that watch, rwatch, and awatch all yield the same
>>behavior?
>>
>>Are you sure that you pass the watchpoint information correctly to
>>the OS?  For example, is the format of DR7 as the OS wants it
>>identical to what GDB uses?  The layout of bits in dr_control_mirror
>>follows Intel documentation, but the OS might want those bits in a
>>different format (that's what the corresponding DPMI call does, for
>>example).  I don't have Windows docs, so I cannot check this.
>>
>>> > /* Get the value of the DR6 debug status register from the inferior.
>>> >    Here we just return the value stored in D_REGS, as we've got it
>>> >    from the last go32_wait call.  */
>>
>>I believe you didn't really mean ``go32_wait'' here ;-)
>
>I'd like some clarification on this before I can accept the patch.  It
>seems like the described behavior would be annoying indeed.  It would
>be nice to fix this.

As tested and explained in my previous mail, 
the unwanted messages are generated by DLL loading events,
and not by wrong debug register generated stops...

  The behavior of the i386 debug register seems to 
be "normal" (i.e. as the intel docs or Ralph Brwon interrupt list
specifies them).

  The problem is only that the messages should probably be 
removed by disaling output while loading new DLLs.
Does this answer your worries?

Re: [RFC/RFA] Add hardware watchpoint support for cygwin target.

[Christopher Faylor]

On Wed, Nov 28, 2001 at 11:40:55PM +0100, muller@cerbere.u-strasbg.fr wrote:
>At 14:30 28/11/01 -0500, Christopher Faylor wrote:
>The problem is only that the messages should probably be removed by
>disaling output while loading new DLLs.  Does this answer your worries?

Sorry, no.  I am still not clear on what is happening?  Are you talking
about a regression or not?

cgf

Re: [RFC/RFA] Add hardware watchpoint support for cygwin target.

[Christopher Faylor]

On Wed, Nov 28, 2001 at 11:40:55PM +0100, muller@cerbere.u-strasbg.fr wrote:
>At 14:30 28/11/01 -0500, Christopher Faylor wrote:
>The problem is only that the messages should probably be removed by
>disaling output while loading new DLLs.  Does this answer your worries?

Sorry, no.  I am still not clear on what is happening?  Are you talking
about a regression or not?

cgf

Re: [RFC/RFA] Add hardware watchpoint support for cygwin target.

[muller]

At 14:30 28/11/01 -0500, Christopher Faylor wrote:
>On Wed, Nov 28, 2001 at 08:13:12PM +0200, Eli Zaretskii wrote:
>>> Date: Wed, 28 Nov 2001 18:44:44 +0100
>>> From: Pierre Muller <muller@cerbere.u-strasbg.fr>
>>> 
>>> But te are some annoying things,
>>> the most annoying is that an exception seems to be generated
>>> on read access of the watched area even if you only set a normal
>>> watchpoint (which should use a write-only debug feature).
>>
>>So you are saying that watch, rwatch, and awatch all yield the same
>>behavior?
>>
>>Are you sure that you pass the watchpoint information correctly to
>>the OS?  For example, is the format of DR7 as the OS wants it
>>identical to what GDB uses?  The layout of bits in dr_control_mirror
>>follows Intel documentation, but the OS might want those bits in a
>>different format (that's what the corresponding DPMI call does, for
>>example).  I don't have Windows docs, so I cannot check this.
>>
>>> > /* Get the value of the DR6 debug status register from the inferior.
>>> >    Here we just return the value stored in D_REGS, as we've got it
>>> >    from the last go32_wait call.  */
>>
>>I believe you didn't really mean ``go32_wait'' here ;-)
>
>I'd like some clarification on this before I can accept the patch.  It
>seems like the described behavior would be annoying indeed.  It would
>be nice to fix this.

As tested and explained in my previous mail, 
the unwanted messages are generated by DLL loading events,
and not by wrong debug register generated stops...

  The behavior of the i386 debug register seems to 
be "normal" (i.e. as the intel docs or Ralph Brwon interrupt list
specifies them).

  The problem is only that the messages should probably be 
removed by disaling output while loading new DLLs.
Does this answer your worries?

Re: [RFC/RFA] Add hardware watchpoint support for cygwin target.

[Eli Zaretskii]

On Wed, 28 Nov 2001, Christopher Faylor wrote:

> It seems like the described behavior would be annoying indeed.  It
> would be nice to fix this.

I second that.

In my experience, having watchpoints fire when they shouldn't renders
them almost unusable.  A watchpoint is a kind of a ``silver bullet'':
it is supposed to reveal bugs that cannot be reasonably caught by any
other means, mainly when some code overwrites locations it shouldn't.
They are also very useful when studying complex programs, when you
want to find out which code modifies some variable.

In both of these situations, if a watchpoint triggers when the
variable isn't written, you cannot trust such a watchpoint, because
you have no way of verifying independently whether the watchpoint
should have triggered or not.  (Comparing the old and new values is
not a reliable way to find out whether the address was or wasn't
written to.)

So I'd suggest some more R&D here.  For example, can anyone see if the
debugger which comes with the Visual Studio does TRT with watchpoints?
If it does, it means there is a way, albeit undocumented (so what else
is new in Redmond-land?) to set a watchpoint and have it break on
writes only.

Re: [RFC/RFA] Add hardware watchpoint support for cygwin target.

[Pierre Muller]

At 09:12 29/11/2001 , vous avez écrit:

>On Wed, 28 Nov 2001, Christopher Faylor wrote:
>
> > It seems like the described behavior would be annoying indeed.  It
> > would be nice to fix this.
>
>I second that.

More info:

  The unwanted output comes from an ui_out_text call
inside mention when a new DLL is loaded

   After some debugging, I think that the problem
arrises at some hidden stops (causes by events like DLL loading)
that do call to new_symfile_objfile,
that does call breakpoint_re_set
that finally calls breakpoint_re_set_one for each enabled
break or watch point.

What is strange is that this new_symfile_objfile call
is encapsulated in  safe_symbol_file_add
that does temporarily disable all output by
change gdb_stdout and gdb_stderr into 
dummy versions.

   I suspect that the problem comes from the fact that uiout 
still remains associated to the normal gdb_stdout.
   This is the only way I can understand why most messages are suppressed
but not these.

    I am completely uncompetent in the UI area,
so someone else can probably better confirm or infirm 
my deductions.




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

Re: [RFC/RFA] Add hardware watchpoint support for cygwin target.

[Christopher Faylor]

On Fri, Nov 30, 2001 at 04:10:42PM +0100, Pierre Muller wrote:
>I suspect that the problem comes from the fact that uiout still remains
>associated to the normal gdb_stdout.  This is the only way I can
>understand why most messages are suppressed but not these.
>
>I am completely uncompetent in the UI area, so someone else can
>probably better confirm or infirm my deductions.

Just in case, you're waiting for feedback from me, I have to tell you
that I don't know much about the ui stuff either.

cgf

Re: [RFC/RFA] Add hardware watchpoint support for cygwin target.

[Christopher Faylor]

On Fri, Nov 30, 2001 at 04:10:42PM +0100, Pierre Muller wrote:
>I suspect that the problem comes from the fact that uiout still remains
>associated to the normal gdb_stdout.  This is the only way I can
>understand why most messages are suppressed but not these.
>
>I am completely uncompetent in the UI area, so someone else can
>probably better confirm or infirm my deductions.

Just in case, you're waiting for feedback from me, I have to tell you
that I don't know much about the ui stuff either.

cgf

Re: [RFC/RFA] Add hardware watchpoint support for cygwin target.

[Pierre Muller]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1282 bytes --]

At 09:12 29/11/2001 , vous avez écrit:

>On Wed, 28 Nov 2001, Christopher Faylor wrote:
>
> > It seems like the described behavior would be annoying indeed.  It
> > would be nice to fix this.
>
>I second that.

More info:

  The unwanted output comes from an ui_out_text call
inside mention when a new DLL is loaded

   After some debugging, I think that the problem
arrises at some hidden stops (causes by events like DLL loading)
that do call to new_symfile_objfile,
that does call breakpoint_re_set
that finally calls breakpoint_re_set_one for each enabled
break or watch point.

What is strange is that this new_symfile_objfile call
is encapsulated in  safe_symbol_file_add
that does temporarily disable all output by
change gdb_stdout and gdb_stderr into 
dummy versions.

   I suspect that the problem comes from the fact that uiout 
still remains associated to the normal gdb_stdout.
   This is the only way I can understand why most messages are suppressed
but not these.

    I am completely uncompetent in the UI area,
so someone else can probably better confirm or infirm 
my deductions.




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

Re: [RFC/RFA] Add hardware watchpoint support for cygwin target.

[Eli Zaretskii]

On Wed, 28 Nov 2001, Christopher Faylor wrote:

> It seems like the described behavior would be annoying indeed.  It
> would be nice to fix this.

I second that.

In my experience, having watchpoints fire when they shouldn't renders
them almost unusable.  A watchpoint is a kind of a ``silver bullet'':
it is supposed to reveal bugs that cannot be reasonably caught by any
other means, mainly when some code overwrites locations it shouldn't.
They are also very useful when studying complex programs, when you
want to find out which code modifies some variable.

In both of these situations, if a watchpoint triggers when the
variable isn't written, you cannot trust such a watchpoint, because
you have no way of verifying independently whether the watchpoint
should have triggered or not.  (Comparing the old and new values is
not a reliable way to find out whether the address was or wasn't
written to.)

So I'd suggest some more R&D here.  For example, can anyone see if the
debugger which comes with the Visual Studio does TRT with watchpoints?
If it does, it means there is a way, albeit undocumented (so what else
is new in Redmond-land?) to set a watchpoint and have it break on
writes only.

Re: [RFC/RFA] Add hardware watchpoint support for cygwin target.

[Pierre Muller]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2381 bytes --]

At 09:12 29/11/2001 , Eli Zaretskii a écrit:

>On Wed, 28 Nov 2001, Christopher Faylor wrote:
>
> > It seems like the described behavior would be annoying indeed.  It
> > would be nice to fix this.
>
>I second that.
>
>In my experience, having watchpoints fire when they shouldn't renders
>them almost unusable.  A watchpoint is a kind of a ``silver bullet'':
>it is supposed to reveal bugs that cannot be reasonably caught by any
>other means, mainly when some code overwrites locations it shouldn't.
>They are also very useful when studying complex programs, when you
>want to find out which code modifies some variable.

I think that I was not clear enough:
   Setting a watchpoint on cygwin target with my patch does NOT 
generate any unwanted stop.
   But it does generate unwanted output.
This ouptut is generated by DLL_LOAD events.
The problem that this load event does call new_symfile_objfile function,
that does call breakpoint_re_set
that finally calls breakpoint_re_set_one for each enabled
break or watch point.
   But breakpoint_re_set_one systematically calls mention
which in turn generates the unwanted uotput.


>In both of these situations, if a watchpoint triggers when the
>variable isn't written, you cannot trust such a watchpoint, because
>you have no way of verifying independently whether the watchpoint
>should have triggered or not.  (Comparing the old and new values is
>not a reliable way to find out whether the address was or wasn't
>written to.)
>
>So I'd suggest some more R&D here.  For example, can anyone see if the
>debugger which comes with the Visual Studio does TRT with watchpoints?
>If it does, it means there is a way, albeit undocumented (so what else
>is new in Redmond-land?) to set a watchpoint and have it break on
>writes only.

   As I said above, my first conclusions were wrong,
debug register generates exceptions only for write instructions if
a "normal" watchpoint is set. 
I was just misled by the fact that I saw several 
   Hardware watchpoint: string_of_the_expression_that_I_was_watching
that appeared at the loading of the executable.

   There are no false debug stops, there are only 
unwanted gdb output generated by DDL LOAD events.


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

Re: [RFC/RFA] Add hardware watchpoint support for cygwin target.

[Pierre Muller]

At 09:12 29/11/2001 , Eli Zaretskii a écrit:

>On Wed, 28 Nov 2001, Christopher Faylor wrote:
>
> > It seems like the described behavior would be annoying indeed.  It
> > would be nice to fix this.
>
>I second that.
>
>In my experience, having watchpoints fire when they shouldn't renders
>them almost unusable.  A watchpoint is a kind of a ``silver bullet'':
>it is supposed to reveal bugs that cannot be reasonably caught by any
>other means, mainly when some code overwrites locations it shouldn't.
>They are also very useful when studying complex programs, when you
>want to find out which code modifies some variable.

I think that I was not clear enough:
   Setting a watchpoint on cygwin target with my patch does NOT 
generate any unwanted stop.
   But it does generate unwanted output.
This ouptut is generated by DLL_LOAD events.
The problem that this load event does call new_symfile_objfile function,
that does call breakpoint_re_set
that finally calls breakpoint_re_set_one for each enabled
break or watch point.
   But breakpoint_re_set_one systematically calls mention
which in turn generates the unwanted uotput.


>In both of these situations, if a watchpoint triggers when the
>variable isn't written, you cannot trust such a watchpoint, because
>you have no way of verifying independently whether the watchpoint
>should have triggered or not.  (Comparing the old and new values is
>not a reliable way to find out whether the address was or wasn't
>written to.)
>
>So I'd suggest some more R&D here.  For example, can anyone see if the
>debugger which comes with the Visual Studio does TRT with watchpoints?
>If it does, it means there is a way, albeit undocumented (so what else
>is new in Redmond-land?) to set a watchpoint and have it break on
>writes only.

   As I said above, my first conclusions were wrong,
debug register generates exceptions only for write instructions if
a "normal" watchpoint is set. 
I was just misled by the fact that I saw several 
   Hardware watchpoint: string_of_the_expression_that_I_was_watching
that appeared at the loading of the executable.

   There are no false debug stops, there are only 
unwanted gdb output generated by DDL LOAD events.


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

Re: [RFC/RFA] Add hardware watchpoint support for cygwin target.

[Christopher Faylor]

On Wed, Nov 28, 2001 at 08:13:12PM +0200, Eli Zaretskii wrote:
>> Date: Wed, 28 Nov 2001 18:44:44 +0100
>> From: Pierre Muller <muller@cerbere.u-strasbg.fr>
>> 
>> But te are some annoying things,
>> the most annoying is that an exception seems to be generated
>> on read access of the watched area even if you only set a normal
>> watchpoint (which should use a write-only debug feature).
>
>So you are saying that watch, rwatch, and awatch all yield the same
>behavior?
>
>Are you sure that you pass the watchpoint information correctly to
>the OS?  For example, is the format of DR7 as the OS wants it
>identical to what GDB uses?  The layout of bits in dr_control_mirror
>follows Intel documentation, but the OS might want those bits in a
>different format (that's what the corresponding DPMI call does, for
>example).  I don't have Windows docs, so I cannot check this.
>
>> > /* Get the value of the DR6 debug status register from the inferior.
>> >    Here we just return the value stored in D_REGS, as we've got it
>> >    from the last go32_wait call.  */
>
>I believe you didn't really mean ``go32_wait'' here ;-)

I'd like some clarification on this before I can accept the patch.  It
seems like the described behavior would be annoying indeed.  It would
be nice to fix this.

Re: [RFC/RFA] Add hardware watchpoint support for cygwin target.

[Pierre Muller]

At 20:30 28/11/2001 , Christopher Faylor a écrit:
>On Wed, Nov 28, 2001 at 08:13:12PM +0200, Eli Zaretskii wrote:
> >> Date: Wed, 28 Nov 2001 18:44:44 +0100
> >> From: Pierre Muller <muller@cerbere.u-strasbg.fr>
> >> 
> >> But te are some annoying things,
> >> the most annoying is that an exception seems to be generated
> >> on read access of the watched area even if you only set a normal
> >> watchpoint (which should use a write-only debug feature).
> >
> >So you are saying that watch, rwatch, and awatch all yield the same
> >behavior?
> >
> >Are you sure that you pass the watchpoint information correctly to
> >the OS?  For example, is the format of DR7 as the OS wants it
> >identical to what GDB uses?  The layout of bits in dr_control_mirror
> >follows Intel documentation, but the OS might want those bits in a
> >different format (that's what the corresponding DPMI call does, for
> >example).  I don't have Windows docs, so I cannot check this.
> >
> >> > /* Get the value of the DR6 debug status register from the inferior.
> >> >    Here we just return the value stored in D_REGS, as we've got it
> >> >    from the last go32_wait call.  */
> >
> >I believe you didn't really mean ``go32_wait'' here ;-)
>
>I'd like some clarification on this before I can accept the patch.  It
>seems like the described behavior would be annoying indeed.  It would
>be nice to fix this.

  I now think that the thread following this patch clearly shows that
there is no cygwin specific bug related to the handling of 
i386 hardware registers in win32-nat.c.

   There are twhree problems related to watchpoints, but none is cygwin specific.

One is related to the absence of removal of the watchpoints at program
exit or kill, which explains the failures observed for the go32 target.
This bug is almost completely hidden in my patch because of the 
DLL loading that does clean the values of the previous run and 
set the watchpoint begin value correctly.

  The second bug arises on the linux target because of both 
the absence of watchpoint removal at exit and lack of calling i386_cleanup_dregs
which results in the fact that the debug registers are not set correctly
to inferior on the second run with the same watchpoints.
That second bug is also not visible on cygwin target as 
that target does call i386_cleanup_dregs.

  The last problem is the unwanted gdb output, but here again,
this is NOT a cygwin specific bug, as it is also present for linux target.
   Each time a DLL (cygwin target) or a shared lib (linux target) is 
loaded, all breakpoints are re-setted, and the mention function does
generate an output even if the output have been disabled by gdb_stdout and gdb_stderr.
This is probably an UI_OUT specific problem, but as its again not cygwin specific
I don't think that there is any reason to delay approval.




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

Re: [RFC/RFA] Add hardware watchpoint support for cygwin target.

[Andrew Cagney]

>  The last problem is the unwanted gdb output, but here again,
> this is NOT a cygwin specific bug, as it is also present for linux target.
>    Each time a DLL (cygwin target) or a shared lib (linux target) is 
> loaded, all breakpoints are re-setted, and the mention function does
> generate an output even if the output have been disabled by gdb_stdout and gdb_stderr.
> This is probably an UI_OUT specific problem, but as its again not cygwin specific
> I don't think that there is any reason to delay approval.

A note on ui-out.  Typically stray output problem come from code that 
still uses printf().  Sorting that out is a separate problem.

enjoy,
Andrew

Re: [RFC/RFA] Add hardware watchpoint support for cygwin target.

[Christopher Faylor]

On Thu, Dec 06, 2001 at 09:37:23AM +0100, Pierre Muller wrote:
>This is probably an UI_OUT specific problem, but as its again not
>cygwin specific I don't think that there is any reason to delay
>approval.

Have we seen independent corroboration of the bugs that you reported?

cgf

Re: [RFC/RFA] Add hardware watchpoint support for cygwin target.

[Pierre Muller]

At 01:58 08/12/2001 , Christopher Faylor a écrit:
>On Thu, Dec 06, 2001 at 09:37:23AM +0100, Pierre Muller wrote:
> >This is probably an UI_OUT specific problem, but as its again not
> >cygwin specific I don't think that there is any reason to delay
> >approval.
>
>Have we seen independent corroboration of the bugs that you reported?

I am not sure I understand your question.

   Do you ask if someone else saw this unwanted 'Hardware watchpoint : ....'
messages. I don't know, nobody explicitly did respond to the messages
by saying that this is indeed the case, but the way to reproduce them
is quite straight forward:

   Compile gdb 5.1 from FSF sources,
run
   ./gdb ./gdb 
from the build/gdb dir.
add a hardware watchpoint at gdb_stdlog for instance with
'top-gdb>watch gdb_stdlog'
and run simply with
'run'

This gave me several print out of  the 'Hardware watchpoint'
besides the correct one.
(Probably one for each shared lib loaded,
but I didn't check this).

  Andrew acknowledged that this was a separate problem in 
http://sources.redhat.com/ml/gdb-patches/2001-12/msg00178.html




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

Re: [RFC/RFA] Add hardware watchpoint support for cygwin target.

[Christopher Faylor]

On Mon, Dec 10, 2001 at 11:21:48AM +0100, Pierre Muller wrote:
>At 01:58 08/12/2001 , Christopher Faylor a ?crit:
>>On Thu, Dec 06, 2001 at 09:37:23AM +0100, Pierre Muller wrote:
>> >This is probably an UI_OUT specific problem, but as its again not
>> >cygwin specific I don't think that there is any reason to delay
>> >approval.
>>
>>Have we seen independent corroboration of the bugs that you reported?
>
>I am not sure I understand your question.
>
>   Do you ask if someone else saw this unwanted 'Hardware watchpoint : ....'
>messages. I don't know, nobody explicitly did respond to the messages
>by saying that this is indeed the case, but the way to reproduce them
>is quite straight forward:

That's sort of what I'm asking, yes.  It would be nice if the maintainers
for the ports that you tested were to confirm or deny the behavior that
you've noticed.

cgf

Re: [RFC/RFA] Add hardware watchpoint support for cygwin target.

[Eli Zaretskii]

On Mon, 10 Dec 2001, Christopher Faylor wrote:

> It would be nice if the maintainers for the ports that you tested
> were to confirm or deny the behavior that you've noticed.

I thought the problem Pierre was raising in the message you replied to
(messages coming out of GDB, supposedly when shared libraries,
a.k.a. DLLs, were loaded) was specific to the Cygwin port.  Did I miss
something?

Re: [RFC/RFA] Add hardware watchpoint support for cygwin target.

[Pierre Muller]

At 09:14 11/12/2001 , Eli Zaretskii a écrit:

>On Mon, 10 Dec 2001, Christopher Faylor wrote:
>
> > It would be nice if the maintainers for the ports that you tested
> > were to confirm or deny the behavior that you've noticed.
>
>I thought the problem Pierre was raising in the message you replied to
>(messages coming out of GDB, supposedly when shared libraries,
>a.k.a. DLLs, were loaded) was specific to the Cygwin port.  Did I miss
>something?

Yes, because I saw the same behavior for i386 linux port of GDB 5.1 release.


See
http://sources.redhat.com/ml/gdb-patches/2001-12/msg00268.html

for a simple test to reproduce the behavior on that target.


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

Re: [RFC/RFA] Add hardware watchpoint support for cygwin target.

[muller]

At 20:13 28/11/01 +0200, Eli Zaretskii wrote:
>> Date: Wed, 28 Nov 2001 18:44:44 +0100
>> From: Pierre Muller <muller@cerbere.u-strasbg.fr>
>> 
>> But te are some annoying things,
>> the most annoying is that an exception seems to be generated
>> on read access of the watched area even if you only set a normal
>> watchpoint (which should use a write-only debug feature).
>
>So you are saying that watch, rwatch, and awatch all yield the same
>behavior?
>
>Are you sure that you pass the watchpoint information correctly to
>the OS?  For example, is the format of DR7 as the OS wants it
>identical to what GDB uses?  The layout of bits in dr_control_mirror
>follows Intel documentation, but the OS might want those bits in a
>different format (that's what the corresponding DPMI call does, for
>example).  I don't have Windows docs, so I cannot check this.

There is nothing available, (or at least I didn't find anything)
about i386 debug registers for the win32 API.
If you look at my patch, you will see that I simply do a straight
copy of the content of the i386-nat mirror registers into 
the DrX registers of the CONTEXT structure.
 
>> > /* Get the value of the DR6 debug status register from the inferior.
>> >    Here we just return the value stored in D_REGS, as we've got it
>> >    from the last go32_wait call.  */
>
>I believe you didn't really mean ``go32_wait'' here ;-)

 Yes, as you probably noticed... it directly taken from the go32-nat code...
I will fix this!

 Concerning Christopher's worry as to 
if this is a bug in the implementation or
in the OS handling, I have no idea, and its the reason
of the RFC...
  After some debugging, I think that the problem
arrises at some hidden stops (causes by events like DLL loading)
that do call to new_symfile_objfile,
that does call breakpoint_re_set
that finally calls breakpoint_re_set_one for each enabled
break or watch point.
And here appears the unwanted GDB message...

Does this mean that uiout should be temporarily redirected toward nul device
as is already done for other functions within cygwin native code?
Or should it really be considered as an error in breakpoint...

Re: [RFC/RFA] Add hardware watchpoint support for cygwin target.

[muller]

At 20:13 28/11/01 +0200, Eli Zaretskii wrote:
>> Date: Wed, 28 Nov 2001 18:44:44 +0100
>> From: Pierre Muller <muller@cerbere.u-strasbg.fr>
>> 
>> But te are some annoying things,
>> the most annoying is that an exception seems to be generated
>> on read access of the watched area even if you only set a normal
>> watchpoint (which should use a write-only debug feature).
>
>So you are saying that watch, rwatch, and awatch all yield the same
>behavior?
>
>Are you sure that you pass the watchpoint information correctly to
>the OS?  For example, is the format of DR7 as the OS wants it
>identical to what GDB uses?  The layout of bits in dr_control_mirror
>follows Intel documentation, but the OS might want those bits in a
>different format (that's what the corresponding DPMI call does, for
>example).  I don't have Windows docs, so I cannot check this.

There is nothing available, (or at least I didn't find anything)
about i386 debug registers for the win32 API.
If you look at my patch, you will see that I simply do a straight
copy of the content of the i386-nat mirror registers into 
the DrX registers of the CONTEXT structure.
 
>> > /* Get the value of the DR6 debug status register from the inferior.
>> >    Here we just return the value stored in D_REGS, as we've got it
>> >    from the last go32_wait call.  */
>
>I believe you didn't really mean ``go32_wait'' here ;-)

 Yes, as you probably noticed... it directly taken from the go32-nat code...
I will fix this!

 Concerning Christopher's worry as to 
if this is a bug in the implementation or
in the OS handling, I have no idea, and its the reason
of the RFC...
  After some debugging, I think that the problem
arrises at some hidden stops (causes by events like DLL loading)
that do call to new_symfile_objfile,
that does call breakpoint_re_set
that finally calls breakpoint_re_set_one for each enabled
break or watch point.
And here appears the unwanted GDB message...

Does this mean that uiout should be temporarily redirected toward nul device
as is already done for other functions within cygwin native code?
Or should it really be considered as an error in breakpoint...