Re: Proposal: convert function definitions to prototyped form

Re: Proposal: convert function definitions to prototyped form

[David Taylor]

    Date: Fri, 2 Jun 2000 00:50:19 -0700
    From: Kevin Buettner <kevinb@cygnus.com>
[...]
	2) ``protoize'' fails to convert functions disabled by ifdefs for
	   the given platform.  OTOH, on some other platform(s), these
	   functions might not be disabled and would be converted.  E.g,
	   in breakpoint.c, on my GNU/Linux/x86 machine, protoize failed
	   to convert create_longjmp_breakpoint() which is protected by an
	   "#ifdef GET_LONGJMP_TARGET".

With any solution, either you're going to have to check the results or
you're going to have to hand convert some of the stuff.  It's a
tradeoff.  From what you say below, your script also requires some
later hand conversions.

Does your script produce substantially less that needs to be hand
converted?

	3) ``protoize'' screws up on PTR types.  E.g, in breakpoint.c, it
	   converted

		static int
		cover_target_enable_exception_callback (arg)
		     PTR arg;

	   to

		static int
		cover_target_enable_exception_callback (__builtin_va_list arg)

I consider this to be a more serious problem than the other complaints
you have.

	4) ``protoize'' does not reformat long argument lists.  The lists
	   end up entirely contained on one line.

So... adopt the same solution for it that you chose to adopt for your
script -- run indent on the output!

    For more information on protoize, see the "Running protoize" page:

	http://gcc.gnu.org/onlinedocs/gcc_2.html#SEC48

    and the "Caveats of using protoize" page:

	http://gcc.gnu.org/onlinedocs/gcc_7.html#SEC135

I've used protoize before with good results.  It was a fairly
substantial project, though not as big as gdb.

    Two of my goals in creating the scripts for the PARAMS purging
    activities was that the scripts should be 1) easy to invoke and 2)
    require no hand editing of the output when done.  I.e, you shouldn't
    have to edit any of the files that these scripts touch in order to fix

I trust that "3)" is:

    "be conservative; not convert something if it can't be sure
    of getting it right".

I'd much rather hand convert more of the code than have it make a
subtle but incorrect change.

    errors so that you can build again.  OTOH, the script may leave
    certain portions of the file alone that could possibly have be
    converted had the script been a little smarter.  The things that the
    script fails to convert *will* have to be fixed later on (in order to
    complete the cleanup activity), either by another script, or by hand. 
    For the PARAMS purging activity, I have spent a fair amount of time
    examining the diffs to make sure that this is indeed the case.  (And
    I intend to do the same for the activity in this proposal.)

Good.

    The reason that it is so important to avoid any hand edits is that we
    want people who have local repositories or checked out source trees to
    be able to run these conversion scripts against them so that merges
    will be less painful.  (Even easy.)

Agreed.

    With that said, keeping in mind the problems I noted above, I conclude
    that ``protoize'' is not the appropriate tool for us to use to convert
    function definitions in the gdb sources to their prototyped form.

I only consider 3) -- screwing up on PTR types -- to be serious; the
others seem minor enough.
[...]

    Finally, I should note that I've done a test build on GNU/Linux/x86
    and had no build problems, nor did I see any regressions when I ran
    the testsuite.

    The fix-decls script is below.  I'd be interested in finding out if
    anyone else has a favorite script for doing this sort of thing.  Other
    comments welcome too.

I'd be tempted to do a build before running your script; stash away the
object files; run the script; do another build; compare the object
files...

I consider the lack of (prototyped) function declarations to be more
serious "problem" than the use of old style function definitions in
old code.  I'd like to see:

. declarations near the top of every file (i.e., before the first
  function definition) for every static function in the file.

. a declaration in an included header file for every non static
  function definition and for every non static function used.

. changes to the default configuration to "encourage" developers to
  include the above declarations.

Re: Proposal: convert function definitions to prototyped form

[Kevin Buettner]

On Jun 2, 11:39am, David Taylor wrote:

> With any solution, either you're going to have to check the results or
> you're going to have to hand convert some of the stuff.  It's a
> tradeoff.  From what you say below, your script also requires some
> later hand conversions.

Yes.  But, it's conservative in that it won't break your builds.
(Or at least that's the intent.)

> Does your script produce substantially less that needs to be hand
> converted?

I only ran protoize on breakpoint.c to see what the problems were. 
This was the file that required the most conversion, however, so
hopefully it's a good representative sample.  For this file, protoize
converted 126 function declarations and fix-decls converted 138.  In
addition, not counting reformatting, the protoized version of
breakpoint.c would require 5 hand edits in order to fix problems
introduced in the conversion.  The fix-decls version requires 0 hand
edits.  After conversion, the protoized version of breakpoint.c
still had 15 functions which would need to be converted by hand (or
some other tool) whereas the fix-decls version had only 3.  Here's
a table to summarizing the above:

                        breakpoint.c conversion
                                              | protoize | fix-decls |
----------------------------------------------+----------+-----------+
Functions converted                           |   126    |   138     |
----------------------------------------------+----------+-----------+
Hand edits needed afterwards to fix problems  |          |           |
in the conversion process                     |     5    |     0     |
----------------------------------------------+----------+-----------+
Functions remaining to be converted           |    15    |     3     |
----------------------------------------------+----------+-----------+
Total number of hand edits required           |    20    |     3     |
----------------------------------------------+----------+-----------+

Recall that over all of the gdb sources, fix-decls converted slightly
over 5200 declarations.  I believe that there are only around 140 left
to consider.  This is somewhat higher than I would like and I may be
able to reduce it somewhat by making the script a little bit smarter. 
But I also want to be certain that no errors are introduced in the
process. 

I should note that there are many settings where protoize is a
completely acceptable (and probably better) tool.  With gdb, however,
we have a lot of developers scattered across the globe who need to be
able to deterministically apply the same transformations to their
sources in order to make merges and checkins easier.

With protoize, I think it's going be be difficult to guarantee
determinism since the results will vary depending upon which platform
you use to do the transformation.

> 	3) ``protoize'' screws up on PTR types.  E.g, in breakpoint.c, it
> 	   converted
> 
> 		static int
> 		cover_target_enable_exception_callback (arg)
> 		     PTR arg;
> 
> 	   to
> 
> 		static int
> 		cover_target_enable_exception_callback (__builtin_va_list arg)
> 
> I consider this to be a more serious problem than the other complaints
> you have.

I think the setup issue is important too.  (I don't know if it's more
serious though.)  As I mentioned in my first point (which is no longer
quoted), you have to do a configure in the source directory above gdb
in order to to properly run protoize or else it complains that it
can't find certain header files.  Also, you need to make sure that the
bfd header files are generated before you start.  None of these
problems are insurmountable, but to do things safely with protoize,
and in order to avoid polluting your sources with files that belong in
a build tree, it would be necessary for a script using protoize to
make a complete copy of the source tree somewhere else in order to do
the work.

> 	4) ``protoize'' does not reformat long argument lists.  The lists
> 	   end up entirely contained on one line.
> 
> So... adopt the same solution for it that you chose to adopt for your
> script -- run indent on the output!

Granted.  A suitably imaginative script could identify just the lines
that protoize touched and reformat those.  I don't think we want to
run indent on entire files however, at least not for this activity.

>     For more information on protoize, see the "Running protoize" page:
> 
> 	http://gcc.gnu.org/onlinedocs/gcc_2.html#SEC48
> 
>     and the "Caveats of using protoize" page:
> 
> 	http://gcc.gnu.org/onlinedocs/gcc_7.html#SEC135
> 
> I've used protoize before with good results.  It was a fairly
> substantial project, though not as big as gdb.

Okay.  Out of curiousity, did the project in question have a large
number of active developers?  (This doesn't necessarily matter, but
I think it's probably easier to manage this kind of thing when only
a handful of developers are affected.)

>     Two of my goals in creating the scripts for the PARAMS purging
>     activities was that the scripts should be 1) easy to invoke and 2)
>     require no hand editing of the output when done.  I.e, you shouldn't
>     have to edit any of the files that these scripts touch in order to fix
> 
> I trust that "3)" is:
> 
>     "be conservative; not convert something if it can't be sure
>     of getting it right".

You're right.  I should have mentioned this.

> I'd much rather hand convert more of the code than have it make a
> subtle but incorrect change.

I agree completely.

[...]

>     Finally, I should note that I've done a test build on GNU/Linux/x86
>     and had no build problems, nor did I see any regressions when I ran
>     the testsuite.
> 
>     The fix-decls script is below.  I'd be interested in finding out if
>     anyone else has a favorite script for doing this sort of thing.  Other
>     comments welcome too.
> 
> I'd be tempted to do a build before running your script; stash away the
> object files; run the script; do another build; compare the object
> files...

Good idea.  I'll have to see what gcc does to compare object files.
(I don't think a simple cmp works for ELF files.)

> I consider the lack of (prototyped) function declarations to be more
> serious "problem" than the use of old style function definitions in
> old code.  I'd like to see:
> 
> . declarations near the top of every file (i.e., before the first
>   function definition) for every static function in the file.
> 
> . a declaration in an included header file for every non static
>   function definition and for every non static function used.
> 
> . changes to the default configuration to "encourage" developers to
>   include the above declarations.

I don't disagree, but I think that adding prototypes for everything
should be a separate activity.  The question is whether it should
occur before the activity covered by my proposal.  And if it should
occur before, once again, we'll need to find a way to help automate
the process because if we attempt to do it incrementally by hand
over a period of time, it is likely that it'll never get done.

Kevin

Re: Proposal: convert function definitions to prototyped form

[Eli Zaretskii]

> Date: Fri, 2 Jun 2000 12:10:42 -0700
> From: Kevin Buettner <kevinb@cygnus.com>
>
> > I've used protoize before with good results.  It was a fairly
> > substantial project, though not as big as gdb.
> 
> Okay.  Out of curiousity, did the project in question have a large
> number of active developers?

I think the real problem is not the number of developers, but the
number of different configurations, and also different data types and
functions concealed behind macros.  `protoize' needs everything to be
explicit, so it is not easy to run it on multi-platform project that
uses macros to hide system dependencies (since you want the same
macros back in the reformatted sources).

This is a disadvantage of `protoize'.  Its significant advantage is
that its output is *always* correct, because it takes the info ``from
the horse's mouth'': the compiler itself.  In contrast, a script is
simply a text-processing tool: it really doesn't understand the
semantics of the source.  In fact, it doesn't really understand the
syntax very well.

So with a script, we will always need a verification tool that can be
trusted to find any potential bugs introduced by reformatting.

> > I'd be tempted to do a build before running your script; stash away the
> > object files; run the script; do another build; compare the object
> > files...
> 
> Good idea.  I'll have to see what gcc does to compare object files.
> (I don't think a simple cmp works for ELF files.)

Comparing object files generally doesn't work.  COFF (at least the
variety used by DJGPP) is another case: if I compile the same source
twice in a row, I get different object files (IIRC, the time stamp is
recorded inside).

One method I can suggest is to compile the source without
optimizations, then run "objdump --disassemble" and compare the output
of `objdump' for the two source files (original and reformatted).  (I
suggest to disable optimizations because it's possible that ANSI
source allows the compiler to produce more optimal code that the K&R
source.)

Note that, since you need to compile the source to make sure
reformatting didn't screw up anything, you essentially get the same
problems you had with `protoize', albeit through the back door: you
need to build for all supported configurations to make sure nothing's
broken.

Re: Proposal: convert function definitions to prototyped form

[Kevin Buettner]

On Jun 3,  6:58am, Eli Zaretskii wrote:

> So with a script, we will always need a verification tool that can be
> trusted to find any potential bugs introduced by reformatting.

Right.

That's why I wrote check-decls (see 
    http://sourceware.cygnus.com/ml/gdb/2000-06/msg00028.html
) which takes the result of comparing (via diff -u) the original
sources with the protoized sources and produces a C source file in
which the portions from the protoized sources are used to construct
prototypes and the portions from the original sources are used to
construct (potentially?) corresponding function definitions.  We can
then invoke the C compiler (gcc -c -Wall) on the result and see what
kinds of warnings and errors are produced.

E.g, in an earlier (than the one I posted) version of fix-decls,
I hadn't yet handled comma separated parameter lists and so the
following:

    foo (a, b, c)
         int a;
         char *b, *c;

was getting transformed into

    foo (int a, int b, char *b, *c)

This type of mistake would've been quickly caught by check-decls +
gcc.  (As it was, I caught it myself because I was looking for it.)

Also, since my fix-decls script merely looks for patterns which
appear to be function definitions, it was finding

if (overload_debug)
{

in find_overload_match() in valops.c and turning this into

if (int overload_debug)
{

(Note that the ``if'' is at the beginning of the line in this function.)

I found this one when I did a test build, but check-decls + the C
compiler would have caught this one too.  (fix-decls was quickly
changed so that it no longer gets tripped up by this code.)  Also, note
that check-decls would've caught this mistake even if the the
construct in question had appeared in some #if 0'd code whereas doing
a build wouldn't have.

I think it could still happen that something might slip by that won't
work in the real code, but now that I've written check-decls, I think
it is much, much less likely.

Kevin

Re: Proposal: convert function definitions to prototyped form

[Eli Zaretskii]

> Date: Sat, 3 Jun 2000 10:50:39 -0700
> From: Kevin Buettner <kevinb@cygnus.com>
>
> That's why I wrote check-decls (see 
>     http://sourceware.cygnus.com/ml/gdb/2000-06/msg00028.html
> ) which takes the result of comparing (via diff -u) the original
> sources with the protoized sources and produces a C source file in
> which the portions from the protoized sources are used to construct
> prototypes and the portions from the original sources are used to
> construct (potentially?) corresponding function definitions.  We can
> then invoke the C compiler (gcc -c -Wall) on the result and see what
> kinds of warnings and errors are produced.

I saw that script, but I don't like the idea to depend on a human to
judge what GCC warnings are okay to ignore and what aren't.  I'd
prefer an automated tool that would give a definite yes/no answer, if
that's possible.

Re: Proposal: convert function definitions to prototyped form

[Kevin Buettner]

On Jun 3,  2:37pm, Eli Zaretskii wrote:

> > Date: Sat, 3 Jun 2000 10:50:39 -0700
> > From: Kevin Buettner <kevinb@cygnus.com>
> >
> > That's why I wrote check-decls (see 
> >     http://sourceware.cygnus.com/ml/gdb/2000-06/msg00028.html
> > ) which takes the result of comparing (via diff -u) the original
> > sources with the protoized sources and produces a C source file in
> > which the portions from the protoized sources are used to construct
> > prototypes and the portions from the original sources are used to
> > construct (potentially?) corresponding function definitions.  We can
> > then invoke the C compiler (gcc -c -Wall) on the result and see what
> > kinds of warnings and errors are produced.
> 
> I saw that script, but I don't like the idea to depend on a human to
> judge what GCC warnings are okay to ignore and what aren't.  I'd
> prefer an automated tool that would give a definite yes/no answer, if
> that's possible.

I see your point.

If I do as Andrew suggests and prevent fix-decls from looking at the
*-share directories, that will prevent the arg_type conflict which
is responsible for the errors.  That leaves only the exit() warning
from standalone.c to worry about.  I can probably devise a solution
which will prevent that warning from occurring as well.

Anyway... I will see if I can arrange it so that any error or warning
indicates a problem and no errors or warnings indicates that all's
well.

Kevin

Re: Proposal: convert function definitions to prototyped form

[Andrew Cagney]

Kevin Buettner wrote:
> 
> On Jun 3,  6:58am, Eli Zaretskii wrote:
> 
> > So with a script, we will always need a verification tool that can be
> > trusted to find any potential bugs introduced by reformatting.
> 
> Right.
> 
> That's why I wrote check-decls (see
>     http://sourceware.cygnus.com/ml/gdb/2000-06/msg00028.html
> ) which takes the result of comparing (via diff -u) the original
> sources with the protoized sources and produces a C source file in
> which the portions from the protoized sources are used to construct
> prototypes and the portions from the original sources are used to
> construct (potentially?) corresponding function definitions.  We can
> then invoke the C compiler (gcc -c -Wall) on the result and see what
> kinds of warnings and errors are produced.

Rather than -Wall (badly missnamed) I'd suggest several more carefully
selected flags such as: -Wimplict -Wreturn-type -Wstrict-prototypes
-Wmissing-declarations.  No need to make life difficult for your self
:-)

	Andrew

Re: gdb doesn't work very well with dynamic linked binaries

[James Cownie]

Mark Kettenis wrote :-
  I'm not sure whether the debug registers are
  per-thread or per-VM-space in Linux.  I'll probably need to look into
  the kernel source.

To save you the time, they are per-thread, just like all the 
other process' registers.

They are conceptually saved and restored on process scheduling
events (which for linuxthreads is the same thing as thread 
scheduling events, since linuxthreads _are_ processes as far as
the scheduler is concerned). 

The bug I mentioned previously is exactly that they're getting
cleared by the kernel and then not getting restored on return
to user space, leaving them wrong until the next reschedule :-(

-- 
-- Jim
James Cownie
jcownie@etnus.com
Etnus, Inc.
Phone +44 117 9071438

Re: gdb doesn't work very well with dynamic linked binaries

[Mark Kettenis]

   Date: Thu, 7 Sep 2000 04:55:38 -0400
   From: James Cownie <jcownie@etnus.com>

   Mark Kettenis wrote :-
     I'm not sure whether the debug registers are
     per-thread or per-VM-space in Linux.  I'll probably need to look into
     the kernel source.

   To save you the time, they are per-thread, just like all the 
   other process' registers.

Thanks, that was what I suspected from looking at the code.

   They are conceptually saved and restored on process scheduling
   events (which for linuxthreads is the same thing as thread 
   scheduling events, since linuxthreads _are_ processes as far as
   the scheduler is concerned).

Yep.  This means that getting HW watchpoints working for
multi-threaded processes is a bit difficult, since GDB expects them to
be process-wide.  So any HW watchpoints will have to be inserted in
*all* threads, not just one as we do now.

Eli, this probably means that adding the debugging registers to GDB's
register cache isn't a good idea.  Something more specialized is
needed, i.e. a native-dependent interface that updates the address and
control register in all threads.  This might implicate that keeping
the actual HW watchpoint support a native-only thing is a good idea.

   The bug I mentioned previously is exactly that they're getting
   cleared by the kernel and then not getting restored on return
   to user space, leaving them wrong until the next reschedule :-(

I think I understand the problems now.  It basically means that one
cannot reliably watch area's that are somehow used in system calls.

I suspect that Linux isn't the only kernel with this bug.  AFAICS
FreeBSD also simply disables any (user-space) watchpoints triggered
from within the kernel.  I don't know what the various x86 System V's
(Solaris, SCO Open Server 5) do, but I wouldn't be surprised if it is
broken there too.

Mark

Re: gdb doesn't work very well with dynamic linked binaries

[Eli Zaretskii]

>   Date: Thu, 7 Sep 2000 12:09:50 +0200 (MET DST)
>   From: Mark Kettenis <kettenis@wins.uva.nl>
>
>   Yep.  This means that getting HW watchpoints working for
>   multi-threaded processes is a bit difficult, since GDB expects them to
>   be process-wide.  So any HW watchpoints will have to be inserted in
>   *all* threads, not just one as we do now.
>
>   Eli, this probably means that adding the debugging registers to GDB's
>   register cache isn't a good idea.  Something more specialized is
>   needed, i.e. a native-dependent interface that updates the address and
>   control register in all threads.  This might implicate that keeping
>   the actual HW watchpoint support a native-only thing is a good idea.

Why ``native-dependent'' and not ``target-dependent''?  Won't the same
problem affect any multithreaded ia32 target?  Or am I missing
something?

In any case, would a special array of debug registers be an okay
solution?  The elements of that array will be set by
ia32_insert_watchpoint and ia32_remove_watchpoint (to be written), and
target-dependent subroutines which resume the inferior and get control
when the inferior is stopped will access that array to pass the
registers to the debuggee.

Re: gdb doesn't work very well with dynamic linked binaries

[Mark Kettenis]

   Date: Thu, 7 Sep 2000 11:00:32 -0400 (EDT)
   From: Eli Zaretskii <eliz@delorie.com>
   CC: jcownie@etnus.com, gdb@sources.redhat.com
   Content-Type: text
   Content-Length: 1217

   >   Date: Thu, 7 Sep 2000 12:09:50 +0200 (MET DST)
   >   From: Mark Kettenis <kettenis@wins.uva.nl>
   >
   >   Yep.  This means that getting HW watchpoints working for
   >   multi-threaded processes is a bit difficult, since GDB expects them to
   >   be process-wide.  So any HW watchpoints will have to be inserted in
   >   *all* threads, not just one as we do now.
   >
   >   Eli, this probably means that adding the debugging registers to GDB's
   >   register cache isn't a good idea.  Something more specialized is
   >   needed, i.e. a native-dependent interface that updates the address and
   >   control register in all threads.  This might implicate that keeping
   >   the actual HW watchpoint support a native-only thing is a good idea.

   Why ``native-dependent'' and not ``target-dependent''?  Won't the same
   problem affect any multithreaded ia32 target?  Or am I missing
   something?

Well, we already have a special packet in the remote protocol for
setting watchpoints, so in principle it isn't necessary to be
``target-dependant''.

The problems are likely to be different for the other multi-threaded
systems.  Linux is kinda weird in that respect.

   In any case, would a special array of debug registers be an okay
   solution?  The elements of that array will be set by
   ia32_insert_watchpoint and ia32_remove_watchpoint (to be written), and
   target-dependent subroutines which resume the inferior and get control
   when the inferior is stopped will access that array to pass the
   registers to the debuggee.

I think it is important to realize that the address and control
registers should be per-process, whereas the status register should be
per-thread.  On Linux, the code to actually pass the registers to the
debuggee should probably live in the (currently native-only)
threads-module.

Oh, and please use an i386_ prefix for the function names instead of
ia32_ for consistency :-).

Mark

Re: gdb doesn't work very well with dynamic linked binaries

[Peter.Schauer]

>    The bug I mentioned previously is exactly that they're getting
>    cleared by the kernel and then not getting restored on return
>    to user space, leaving them wrong until the next reschedule :-(
> 
> I think I understand the problems now.  It basically means that one
> cannot reliably watch area's that are somehow used in system calls.
> 
> I suspect that Linux isn't the only kernel with this bug.  AFAICS
> FreeBSD also simply disables any (user-space) watchpoints triggered
> from within the kernel.  I don't know what the various x86 System V's
> (Solaris, SCO Open Server 5) do, but I wouldn't be surprised if it is
> broken there too.

AFAIK Solaris never provided access to the x86 debug registers.
Starting with Solaris 2.6, support for hardware watchpoints is provided
via procfs (protect pages which contain watchpoint areas, examine address
on pagefault, cause breakpoint if address is within watchpoint area).

Perhaps this scheme (which allows for an arbitrary number of watchpoints)
could be adopted by the Linux kernel.

Debug register access might be valuable for embedded targets, but the
generic x86 target should not assume availability of debug register access.

-- 
Peter Schauer			pes@regent.e-technik.tu-muenchen.de

[RFC] Unified watchpoints for x86 platforms

[Eli Zaretskii]

[Yes, I know: it's been a looong time since I promised to make this
happen.  Still, it's better late than never ;-)]

I started working on the unified support for hardware-assisted
breakpoints and watchpoints on x86 platforms (see TODO).  Since I
don't feel I know enough about all the aspects of this on any platform
but DJGPP, I thought I'd better get the framework agreed to before I
start coding.

Here's the API I suggest for use by higher-level GDB code:

  (Note: I'm not good at inventing names, so please suggest better
  ones if you want.)

  int i386_hwbp_insert (int pid, CORE_ADDR addr, int len, int kind);

  This function inserts a breakpoint or watchpoint to watch memory
  region starting at address ADDR whose length is LEN bytes.  The
  watchpoint will watch said memory region for accesses whose type
  is defined by KIND:

    HW_READ	     break if the region is accessed for reading[1]
    HW_WRITE	     break if the region is accessed for writing
    HW_ACCESS	     break if the region is accessed for either
		     reading or writing
    HW_IO_ACCESS     same as HW_ACCESS type, but for I/O read/write
		     access[2]
    HW_EXECUTE       instruction execution breakpoint

  The function returns 0 upon success, else -1.

  Notes:
  [1] Since x86 doesn't support read data watchpoints, HW_READ will
      actually be implemented as a read/write watchpoint, and relies
      on higher-level GDB code to distinguish between reads and
      writes.  The infrastructure to support this is already in place
      in breakpoint.c, since GDB 5.0.

  [2] I/O watchpoints are not currently supported (AFAIK) by GDB on
      any x86 platform.  I can provide the code to handle it, but do
      people think we should define a command to access this feature?
      If so, should we provide separate read, write, and access types
      of watchpoints, or a single access type (the only one supported
      by x86's debug registers) is enough?

      Note that I/O watchpoints require that the DE (debug extensions)
      flag in the CR4 register be set.  I don't know what platforms
      set it and under what circumstances.

  int i386_hwbp_remove (int pid, CORE_ADDR addr, int len, int kind);

  This function removes a breakpoint of watchpoint at address ADDR
  which watches a memory region of LEN bytes and whose type is given
  by KIND.  It returns 0 upon success, else -1.

  int i386_hwbp_region_ok (CORE_ADDR addr, int len);

  This function tests whether a memory region of LEN bytes starting at
  ADDR can be watched with debug registers.  It returns 1 if the
  region can be watched, 0 otherwise.

  int i386_hwbp_stopped_by_watchpoint (int pid);

  This function returns the address of a breakpoint or watchpoint
  which could have stopped the debuggee.  If no watchpoint triggered,
  it returns 0.

To actually insert and remove breakpoints and watchpoints, I need
low-level system-dependent functions.  Here's the API I suggest for
this low-levwl layer.  (These are macros so that targets could define
them on their nm-*.h files.  On a typical Unix or GNU/Linux system,
each of these macros will call `ptrace' with suitable arguments.)

  void HWBP_SET_ADDR (int pid, int dr_num, CORE_ADDR addr);

  This macro sets the debug register DR_NUM in the inferior to watch
  the address ADDR.  DR_NUM can be in the range [0..3].

  void HWBP_SET_CONTROL (int pid, unsigned int val);

  This macro sets the DR7 debug control register in the inferior to
  the value VAL.

  unsigned int HWBP_GET_STATUS (int pid);

  This macro returns the value of the DR6 debug status register from
  the inferior.

  In the discussion we had back in September, Mark said that the
  status register should be per thread.  Does that mean that we need
  an additional argument (int tid?) to pass to HWBP_GET_STATUS?  If
  so, how will this argument get into the i386_hwbp_* functions which
  will call these macros?

  Or maybe the target end can figure out the thread id by itself with
  some TIDGET(pid) magic?


Comments?  Suggestions?  Flames?

Re: [RFC] Unified watchpoints for x86 platforms

[H . J . Lu]

On Sat, Feb 10, 2001 at 10:33:52AM -0500, Eli Zaretskii wrote:
>   unsigned int HWBP_GET_STATUS (int pid);
> 
>   This macro returns the value of the DR6 debug status register from
>   the inferior.
> 
>   In the discussion we had back in September, Mark said that the
>   status register should be per thread.  Does that mean that we need
>   an additional argument (int tid?) to pass to HWBP_GET_STATUS?  If
>   so, how will this argument get into the i386_hwbp_* functions which
>   will call these macros?

What is the difference between pid and tid in this case? Can we derive
tid from pid?

> 
>   Or maybe the target end can figure out the thread id by itself with
>   some TIDGET(pid) magic?
> 
> 
> Comments?  Suggestions?  Flames?

I haven't looked at it in detail. I guess Linux can live with the one
everyone agrees up on. If it turns out it is not true, I hope we can
still change it :-(. Unfortunately, I don't have the time to try it
out now. However, I will give it a try when the OS independent part is
checked in and noone is working on Linux.

Thanks.


-- 
H.J. Lu (hjl@valinux.com)

Re: [RFC] Unified watchpoints for x86 platforms

[Eli Zaretskii]

> Date: Sat, 10 Feb 2001 10:18:37 -0800
> From: "H . J . Lu" <hjl@valinux.com>
> 
> On Sat, Feb 10, 2001 at 10:33:52AM -0500, Eli Zaretskii wrote:
> >   unsigned int HWBP_GET_STATUS (int pid);
> > 
> >   This macro returns the value of the DR6 debug status register from
> >   the inferior.
> > 
> >   In the discussion we had back in September, Mark said that the
> >   status register should be per thread.  Does that mean that we need
> >   an additional argument (int tid?) to pass to HWBP_GET_STATUS?  If
> >   so, how will this argument get into the i386_hwbp_* functions which
> >   will call these macros?
> 
> What is the difference between pid and tid in this case? Can we derive
> tid from pid?

That's what I'd like to know as well.  Anyone?

Re: [RFC] Unified watchpoints for x86 platforms

[Eli Zaretskii]

Ping!

No one posted any approvals or disprovals of this design.  Do I take
the silence as a sign of agreement and start coding?
 
> I started working on the unified support for hardware-assisted
> breakpoints and watchpoints on x86 platforms (see TODO).  Since I
> don't feel I know enough about all the aspects of this on any platform
> but DJGPP, I thought I'd better get the framework agreed to before I
> start coding.
> 
> Here's the API I suggest for use by higher-level GDB code:
> 
>   (Note: I'm not good at inventing names, so please suggest better
>   ones if you want.)
> 
>   int i386_hwbp_insert (int pid, CORE_ADDR addr, int len, int kind);
> 
>   This function inserts a breakpoint or watchpoint to watch memory
>   region starting at address ADDR whose length is LEN bytes.  The
>   watchpoint will watch said memory region for accesses whose type
>   is defined by KIND:
> 
>     HW_READ	     break if the region is accessed for reading[1]
>     HW_WRITE	     break if the region is accessed for writing
>     HW_ACCESS	     break if the region is accessed for either
> 		     reading or writing
>     HW_IO_ACCESS     same as HW_ACCESS type, but for I/O read/write
> 		     access[2]
>     HW_EXECUTE       instruction execution breakpoint
> 
>   The function returns 0 upon success, else -1.
> 
>   Notes:
>   [1] Since x86 doesn't support read data watchpoints, HW_READ will
>       actually be implemented as a read/write watchpoint, and relies
>       on higher-level GDB code to distinguish between reads and
>       writes.  The infrastructure to support this is already in place
>       in breakpoint.c, since GDB 5.0.
> 
>   [2] I/O watchpoints are not currently supported (AFAIK) by GDB on
>       any x86 platform.  I can provide the code to handle it, but do
>       people think we should define a command to access this feature?
>       If so, should we provide separate read, write, and access types
>       of watchpoints, or a single access type (the only one supported
>       by x86's debug registers) is enough?
> 
>       Note that I/O watchpoints require that the DE (debug extensions)
>       flag in the CR4 register be set.  I don't know what platforms
>       set it and under what circumstances.
> 
>   int i386_hwbp_remove (int pid, CORE_ADDR addr, int len, int kind);
> 
>   This function removes a breakpoint of watchpoint at address ADDR
>   which watches a memory region of LEN bytes and whose type is given
>   by KIND.  It returns 0 upon success, else -1.
> 
>   int i386_hwbp_region_ok (CORE_ADDR addr, int len);
> 
>   This function tests whether a memory region of LEN bytes starting at
>   ADDR can be watched with debug registers.  It returns 1 if the
>   region can be watched, 0 otherwise.
> 
>   int i386_hwbp_stopped_by_watchpoint (int pid);
> 
>   This function returns the address of a breakpoint or watchpoint
>   which could have stopped the debuggee.  If no watchpoint triggered,
>   it returns 0.
> 
> To actually insert and remove breakpoints and watchpoints, I need
> low-level system-dependent functions.  Here's the API I suggest for
> this low-levwl layer.  (These are macros so that targets could define
> them on their nm-*.h files.  On a typical Unix or GNU/Linux system,
> each of these macros will call `ptrace' with suitable arguments.)
> 
>   void HWBP_SET_ADDR (int pid, int dr_num, CORE_ADDR addr);
> 
>   This macro sets the debug register DR_NUM in the inferior to watch
>   the address ADDR.  DR_NUM can be in the range [0..3].
> 
>   void HWBP_SET_CONTROL (int pid, unsigned int val);
> 
>   This macro sets the DR7 debug control register in the inferior to
>   the value VAL.
> 
>   unsigned int HWBP_GET_STATUS (int pid);
> 
>   This macro returns the value of the DR6 debug status register from
>   the inferior.
> 
>   In the discussion we had back in September, Mark said that the
>   status register should be per thread.  Does that mean that we need
>   an additional argument (int tid?) to pass to HWBP_GET_STATUS?  If
>   so, how will this argument get into the i386_hwbp_* functions which
>   will call these macros?
> 
>   Or maybe the target end can figure out the thread id by itself with
>   some TIDGET(pid) magic?
> 
> 
> Comments?  Suggestions?  Flames?
> 

Re: [RFC] Unified watchpoints for x86 platforms

[Mark Kettenis]

Eli Zaretskii <eliz@is.elta.co.il> writes:

> Ping!
> 
> No one posted any approvals or disprovals of this design.  Do I take
> the silence as a sign of agreement and start coding?

Sorry for not responding earlier.  In general, your proposal looks
fine, but I think it is best to export functions similar to GDB's
target_* functions:

int i386_remove_watchpoint (CORE_ADDR addr, int len,
			    enum target_hw_bp_type type);
int i386_insert_watchpoint (CORE_ADDR addr, int len,
                            enum target_hw_bp_type type);

int i386_insert_hw_breakpoint (CORE_ADDR addr, void *shadow);
int i386_remove_hw_breakpoint (CORE_ADDR addr, void *shadow);

etc.

Of course you can implement those on top of the functions mentioned below.

> > I started working on the unified support for hardware-assisted
> > breakpoints and watchpoints on x86 platforms (see TODO).  Since I
> > don't feel I know enough about all the aspects of this on any platform
> > but DJGPP, I thought I'd better get the framework agreed to before I
> > start coding.
> > 
> > Here's the API I suggest for use by higher-level GDB code:
> > 
> >   (Note: I'm not good at inventing names, so please suggest better
> >   ones if you want.)
> > 
> >   int i386_hwbp_insert (int pid, CORE_ADDR addr, int len, int kind);

Is there any particular reason why you need the PID argument?  AFAICS
it will always be equal to INFERIOR_PID, so I think we can do without
it.  This is also true for the other i386_hwbp_* functions you're
proposing.

> > 
> >   This function inserts a breakpoint or watchpoint to watch memory
> >   region starting at address ADDR whose length is LEN bytes.  The
> >   watchpoint will watch said memory region for accesses whose type
> >   is defined by KIND:
> > 
> >     HW_READ	     break if the region is accessed for reading[1]
> >     HW_WRITE	     break if the region is accessed for writing
> >     HW_ACCESS	     break if the region is accessed for either
> > 		     reading or writing
> >     HW_IO_ACCESS     same as HW_ACCESS type, but for I/O read/write
> > 		     access[2]
> >     HW_EXECUTE       instruction execution breakpoint

Please consider using an enum instead of an int.  It looks as if GDB's
convention is to use lower-case names for enum values.

> >   The function returns 0 upon success, else -1.
> > 
> >   Notes:
> >   [1] Since x86 doesn't support read data watchpoints, HW_READ will
> >       actually be implemented as a read/write watchpoint, and relies
> >       on higher-level GDB code to distinguish between reads and
> >       writes.  The infrastructure to support this is already in place
> >       in breakpoint.c, since GDB 5.0.

Sounds OK.

> >   [2] I/O watchpoints are not currently supported (AFAIK) by GDB on
> >       any x86 platform.  I can provide the code to handle it, but do
> >       people think we should define a command to access this feature?
> >       If so, should we provide separate read, write, and access types
> >       of watchpoints, or a single access type (the only one supported
> >       by x86's debug registers) is enough?

I think this can be added later if people want it.

> >       Note that I/O watchpoints require that the DE (debug extensions)
> >       flag in the CR4 register be set.  I don't know what platforms
> >       set it and under what circumstances.

I don't think you can do this in any of the x86 Unixoid systems.

> > 
> >   int i386_hwbp_remove (int pid, CORE_ADDR addr, int len, int kind);
> > 
> >   This function removes a breakpoint of watchpoint at address ADDR
> >   which watches a memory region of LEN bytes and whose type is given
> >   by KIND.  It returns 0 upon success, else -1.
> > 
> >   int i386_hwbp_region_ok (CORE_ADDR addr, int len);
> > 
> >   This function tests whether a memory region of LEN bytes starting at
> >   ADDR can be watched with debug registers.  It returns 1 if the
> >   region can be watched, 0 otherwise.
> > 
> >   int i386_hwbp_stopped_by_watchpoint (int pid);
> > 
> >   This function returns the address of a breakpoint or watchpoint
> >   which could have stopped the debuggee.  If no watchpoint triggered,
> >   it returns 0.
> > 
> > To actually insert and remove breakpoints and watchpoints, I need
> > low-level system-dependent functions.  Here's the API I suggest for
> > this low-levwl layer.  (These are macros so that targets could define
> > them on their nm-*.h files.  On a typical Unix or GNU/Linux system,
> > each of these macros will call `ptrace' with suitable arguments.)
> > 
> >   void HWBP_SET_ADDR (int pid, int dr_num, CORE_ADDR addr);
> > 
> >   This macro sets the debug register DR_NUM in the inferior to watch
> >   the address ADDR.  DR_NUM can be in the range [0..3].
> > 
> >   void HWBP_SET_CONTROL (int pid, unsigned int val);
> > 
> >   This macro sets the DR7 debug control register in the inferior to
> >   the value VAL.
> > 
> >   unsigned int HWBP_GET_STATUS (int pid);
> > 
> >   This macro returns the value of the DR6 debug status register from
> >   the inferior.

Why not have simply long I386_GET_DR(int regnum) and I386_SET_DR(int
regnum, long value) and let the system-dependent decide how to map the
debug register number ([0..7], as used in the Intel documentation)
into whatever is needed?

> >   In the discussion we had back in September, Mark said that the
> >   status register should be per thread.  Does that mean that we need
> >   an additional argument (int tid?) to pass to HWBP_GET_STATUS?  If
> >   so, how will this argument get into the i386_hwbp_* functions which
> >   will call these macros?

I don't think an additional argument is needed.  When calling
HWBP_GET_STATUS, it is the current thread that has encountered a trap,
and INFERIOR_PID should be set appropriately.

> >   Or maybe the target end can figure out the thread id by itself with
> >   some TIDGET(pid) magic?

Indeed.

Mark

Re: [RFC] Unified watchpoints for x86 platforms

[H . J . Lu]

On Thu, Feb 15, 2001 at 01:46:27PM +0200, Eli Zaretskii wrote:
> Ping!
> 
> No one posted any approvals or disprovals of this design.  Do I take
> the silence as a sign of agreement and start coding?
>  

Why not?


H.J.

Re: [RFC] Unified watchpoints for x86 platforms

[Eli Zaretskii]

> From: Mark Kettenis <kettenis@wins.uva.nl>
> Date: 15 Feb 2001 17:17:17 +0100
> 
> In general, your proposal looks
> fine, but I think it is best to export functions similar to GDB's
> target_* functions:
> 
> int i386_remove_watchpoint (CORE_ADDR addr, int len,
> 			    enum target_hw_bp_type type);
> int i386_insert_watchpoint (CORE_ADDR addr, int len,
>                             enum target_hw_bp_type type);
> 
> int i386_insert_hw_breakpoint (CORE_ADDR addr, void *shadow);
> int i386_remove_hw_breakpoint (CORE_ADDR addr, void *shadow);

I didn't want to use those names because they are taken by
i386v-nat.c.  If we use these names, we will have to modify
i386v-nat.c, which is used by many x86 platforms.  I didn't want such
a profound effect; some platform maintainers might not want the new
interface, and will be less than happy to rewrite their code for no
good reason.

> > >   int i386_hwbp_insert (int pid, CORE_ADDR addr, int len, int kind);
>
> Is there any particular reason why you need the PID argument?

Just the existing interfaces.  DJGPP obviously doesn't use that
argument.

> > >     HW_READ	     break if the region is accessed for reading[1]
> > >     HW_WRITE	     break if the region is accessed for writing
> > >     HW_ACCESS	     break if the region is accessed for either
> > > 		     reading or writing
> > >     HW_IO_ACCESS     same as HW_ACCESS type, but for I/O read/write
> > > 		     access[2]
> > >     HW_EXECUTE       instruction execution breakpoint
> 
> Please consider using an enum instead of an int.  It looks as if GDB's
> convention is to use lower-case names for enum values.

I intended to use an enum.  I believe the upper-case names were just a
coincidence.

> > >   void HWBP_SET_ADDR (int pid, int dr_num, CORE_ADDR addr);
> > > 
> > >   This macro sets the debug register DR_NUM in the inferior to watch
> > >   the address ADDR.  DR_NUM can be in the range [0..3].
> > > 
> > >   void HWBP_SET_CONTROL (int pid, unsigned int val);
> > > 
> > >   This macro sets the DR7 debug control register in the inferior to
> > >   the value VAL.
> > > 
> > >   unsigned int HWBP_GET_STATUS (int pid);
> > > 
> > >   This macro returns the value of the DR6 debug status register from
> > >   the inferior.
> 
> Why not have simply long I386_GET_DR(int regnum) and I386_SET_DR(int
> regnum, long value) and let the system-dependent decide how to map the
> debug register number ([0..7], as used in the Intel documentation)
> into whatever is needed?

Why bother the target end to do that?  They will all do the same, and
the code I'll write knows exactly when it needs what register.

In addition, I386_GET_DR might mislead someone into thinking that it
actually gets the value of any DRi from the inferior.  That was not my
intention: I don't need to fetch any debug register except DR6.

Thanks for the feedback.

I think I will start coding this weekend, so if someone has more
input, please hurry ;-)

Re: [RFC] Unified watchpoints for x86 platforms

[Mark Kettenis]

   Date: Thu, 15 Feb 2001 12:32:03 -0500 (EST)
   From: Eli Zaretskii <eliz@delorie.com>

   > From: Mark Kettenis <kettenis@wins.uva.nl>
   > Date: 15 Feb 2001 17:17:17 +0100
   > 
   > In general, your proposal looks
   > fine, but I think it is best to export functions similar to GDB's
   > target_* functions:
   > 
   > int i386_remove_watchpoint (CORE_ADDR addr, int len,
   > 			    enum target_hw_bp_type type);
   > int i386_insert_watchpoint (CORE_ADDR addr, int len,
   >                             enum target_hw_bp_type type);
   > 
   > int i386_insert_hw_breakpoint (CORE_ADDR addr, void *shadow);
   > int i386_remove_hw_breakpoint (CORE_ADDR addr, void *shadow);

   I didn't want to use those names because they are taken by
   i386v-nat.c.  If we use these names, we will have to modify
   i386v-nat.c, which is used by many x86 platforms.  I didn't want such
   a profound effect; some platform maintainers might not want the new
   interface, and will be less than happy to rewrite their code for no
   good reason.

Ah, but that isn't an issue here.  Targets using the old code from
i386v-nat.c won't use the generic code and vice versa.  Moreover, I
think I'll re-implement the stuff in i386v-nat.c with the help of the
generic code.  And as a last resort we can always rename the i386_*
functions in i386v-nat.c into i386v_*.  And really the only platforms
that actually use that code Linux and SCO Open Server 5.

So please use the names I suggest above.  I think they're much clearer.

   > > >   int i386_hwbp_insert (int pid, CORE_ADDR addr, int len, int kind);
   >
   > Is there any particular reason why you need the PID argument?

   Just the existing interfaces.  DJGPP obviously doesn't use that
   argument.

Linux and SCO Open Server don't need it either.  So please drop it.

   > > >     HW_READ	     break if the region is accessed for reading[1]
   > > >     HW_WRITE	     break if the region is accessed for writing
   > > >     HW_ACCESS	     break if the region is accessed for either
   > > > 		     reading or writing
   > > >     HW_IO_ACCESS     same as HW_ACCESS type, but for I/O read/write
   > > > 		     access[2]
   > > >     HW_EXECUTE       instruction execution breakpoint
   > 
   > Please consider using an enum instead of an int.  It looks as if GDB's
   > convention is to use lower-case names for enum values.

   I intended to use an enum.  I believe the upper-case names were just a
   coincidence.

See Andrews reply on that.

   > > >   void HWBP_SET_ADDR (int pid, int dr_num, CORE_ADDR addr);
   > > > 
   > > >   This macro sets the debug register DR_NUM in the inferior to watch
   > > >   the address ADDR.  DR_NUM can be in the range [0..3].
   > > > 
   > > >   void HWBP_SET_CONTROL (int pid, unsigned int val);
   > > > 
   > > >   This macro sets the DR7 debug control register in the inferior to
   > > >   the value VAL.
   > > > 
   > > >   unsigned int HWBP_GET_STATUS (int pid);
   > > > 
   > > >   This macro returns the value of the DR6 debug status register from
   > > >   the inferior.
   > 
   > Why not have simply long I386_GET_DR(int regnum) and I386_SET_DR(int
   > regnum, long value) and let the system-dependent decide how to map the
   > debug register number ([0..7], as used in the Intel documentation)
   > into whatever is needed?

   Why bother the target end to do that?  They will all do the same, and
   the code I'll write knows exactly when it needs what register.

It would make the implementation on Linux easier :-).  But the
difference isn't really that big.

   In addition, I386_GET_DR might mislead someone into thinking that it
   actually gets the value of any DRi from the inferior.  That was not my
   intention: I don't need to fetch any debug register except DR6.

Good point.

Mark

Re: [RFC] Unified watchpoints for x86 platforms

[Kevin Buettner]

On Feb 15,  5:17pm, Mark Kettenis wrote:

> > > I started working on the unified support for hardware-assisted
> > > breakpoints and watchpoints on x86 platforms (see TODO).  Since I
> > > don't feel I know enough about all the aspects of this on any platform
> > > but DJGPP, I thought I'd better get the framework agreed to before I
> > > start coding.
> > > 
> > > Here's the API I suggest for use by higher-level GDB code:
> > > 
> > >   (Note: I'm not good at inventing names, so please suggest better
> > >   ones if you want.)
> > > 
> > >   int i386_hwbp_insert (int pid, CORE_ADDR addr, int len, int kind);
> 
> Is there any particular reason why you need the PID argument?  AFAICS
> it will always be equal to INFERIOR_PID, so I think we can do without
> it.  This is also true for the other i386_hwbp_* functions you're
> proposing.

I think it'd be better to not rely on ``inferior_pid''.  I would
rather see the explicitly passed.  There will come a day when GDB
is able to debug more than one process at a time and to perpetuate
reliance on inferior pid would be short sighted.

> > >   In the discussion we had back in September, Mark said that the
> > >   status register should be per thread.  Does that mean that we need
> > >   an additional argument (int tid?) to pass to HWBP_GET_STATUS?  If
> > >   so, how will this argument get into the i386_hwbp_* functions which
> > >   will call these macros?
> 
> I don't think an additional argument is needed.  When calling
> HWBP_GET_STATUS, it is the current thread that has encountered a trap,
> and INFERIOR_PID should be set appropriately.
> 
> > >   Or maybe the target end can figure out the thread id by itself with
> > >   some TIDGET(pid) magic?

Hopefully I'll find time to merge my pid/tid/lwp patch sometime soon.
When this occurs, you'll be able to extract the thread id from what is
now the pid argument.

I have read the rest of Eli's proposal as well as Mark's comments and
I agree with the rest of Mark's remarks.

Kevin

Re: [RFC] Unified watchpoints for x86 platforms

[Eli Zaretskii]

> Date: Thu, 15 Feb 2001 11:41:35 -0700
> From: Kevin Buettner <kevinb@cygnus.com>
> > 
> > Is there any particular reason why you need the PID argument?  AFAICS
> > it will always be equal to INFERIOR_PID, so I think we can do without
> > it.  This is also true for the other i386_hwbp_* functions you're
> > proposing.
> 
> I think it'd be better to not rely on ``inferior_pid''.  I would
> rather see the explicitly passed.  There will come a day when GDB
> is able to debug more than one process at a time and to perpetuate
> reliance on inferior pid would be short sighted.

I have two opposite opinions here.  We need to resolve this somehow.

> I have read the rest of Eli's proposal as well as Mark's comments and
> I agree with the rest of Mark's remarks.

Thanks for the feedback.

Re: [RFC] Unified watchpoints for x86 platforms

[Eli Zaretskii]

> Date: Thu, 15 Feb 2001 19:33:40 +0100
> From: Mark Kettenis <kettenis@wins.uva.nl>
> 
> So please use the names I suggest above.

Will do.

> I think they're much clearer.

Yes, I prefer them as well.

>    > Why not have simply long I386_GET_DR(int regnum) and I386_SET_DR(int
>    > regnum, long value) and let the system-dependent decide how to map the
>    > debug register number ([0..7], as used in the Intel documentation)
>    > into whatever is needed?
> 
>    Why bother the target end to do that?  They will all do the same, and
>    the code I'll write knows exactly when it needs what register.
> 
> It would make the implementation on Linux easier :-).

Could you please explain in a few words why?  Even if I don't change
the interface, I think it is important for me to know about such
subtle aspects.

What I see in i386v-nat.c is that each of these macros maps directly
into a ptrace call with appropriate arguments.  For example, here's
how DR7 (the debug control register) is set:

  ptrace (6, pid, offsetof (struct user, u_debugreg[DR_CONTROL]),
	  debug_control_mirror);

and here's how DR6, the debug status register, is fetched:

  status = ptrace (3, pid, offsetof (struct user, u_debugreg[DR_STATUS]), 0);

Even ia64-linux-nat.c does something very similar, even though it is
not one of the potential customers of what I'm about to write.

Re: [RFC] Unified watchpoints for x86 platforms

[J.T. Conklin]

>>>>> "Eli" == Eli Zaretskii <eliz@delorie.com> writes:
>> > Is there any particular reason why you need the PID argument?  AFAICS
>> > it will always be equal to INFERIOR_PID, so I think we can do without
>> > it.  This is also true for the other i386_hwbp_* functions you're
>> > proposing.
>> 
>> I think it'd be better to not rely on ``inferior_pid''.  I would
>> rather see the explicitly passed.  There will come a day when GDB
>> is able to debug more than one process at a time and to perpetuate
>> reliance on inferior pid would be short sighted.

Eli> I have two opposite opinions here.  We need to resolve this somehow.

We're going to need to pass a PID, or perhaps some new representation
of a execution context, to a lot of code functions that don't allready
have such an argument.  It is not clear to me that adding such an
argument "because it will be needed" is correct, considering that the
design has not yet started.  The truth is we don't know "what" will be
needed, so we'll have to revisit this function (among many others)
down the line anyway.


        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: [RFC] Unified watchpoints for x86 platforms

[Kevin Buettner]

On Feb 15,  2:45pm, J.T. Conklin wrote:

> Subject: Re: [RFC] Unified watchpoints for x86 platforms
> >>>>> "Eli" == Eli Zaretskii <eliz@delorie.com> writes:
> >> > Is there any particular reason why you need the PID argument?  AFAICS
> >> > it will always be equal to INFERIOR_PID, so I think we can do without
> >> > it.  This is also true for the other i386_hwbp_* functions you're
> >> > proposing.
> >> 
> >> I think it'd be better to not rely on ``inferior_pid''.  I would
> >> rather see the explicitly passed.  There will come a day when GDB
> >> is able to debug more than one process at a time and to perpetuate
> >> reliance on inferior pid would be short sighted.
> 
> Eli> I have two opposite opinions here.  We need to resolve this somehow.
> 
> We're going to need to pass a PID, or perhaps some new representation
> of a execution context, to a lot of code functions that don't allready
> have such an argument.  It is not clear to me that adding such an
> argument "because it will be needed" is correct, considering that the
> design has not yet started.  The truth is we don't know "what" will be
> needed, so we'll have to revisit this function (among many others)
> down the line anyway.

Eli,

I think the answer is to use your best judgement.  Regardless of
whether you pass the pid in or simply use inferior_pid, your new
watchpoint code is going to have to eventually be changed to use
a different representation of the execution context.  I happen to
think that it might actually change less if you pass a parameter, but
after thinking about it a bit more, I can see why someone else might
hold the opposite opinion.

Kevin

Re: [RFC] Unified watchpoints for x86 platforms

[Eli Zaretskii]

> Date: Thu, 15 Feb 2001 17:09:53 -0700
> From: Kevin Buettner <kevinb@cygnus.com>
> 
> I think the answer is to use your best judgement.  Regardless of
> whether you pass the pid in or simply use inferior_pid, your new
> watchpoint code is going to have to eventually be changed to use
> a different representation of the execution context.  I happen to
> think that it might actually change less if you pass a parameter, but
> after thinking about it a bit more, I can see why someone else might
> hold the opposite opinion.

Okay, thanks for the feedback.  I'll sleep on this and then decide.

Re: [RFC] Unified watchpoints for x86 platforms

[Eli Zaretskii]

> From: jtc@redback.com (J.T. Conklin)
> Date: 15 Feb 2001 14:45:16 -0800
> 
> We're going to need to pass a PID, or perhaps some new representation
> of a execution context, to a lot of code functions that don't allready
> have such an argument.

Sorry, I'm not sure I'm following: why do you envision we'll need to
pass the PID to functions that don't receive it today?  What
function(s) did you have in mind?

The current watchpoint implementation on i386v-nat.c, for example,
does accept PID as an argument.

Re: [RFC] Unified watchpoints for x86 platforms

[Mark Kettenis]

   From: jtc@redback.com (J.T. Conklin)
   Date: 15 Feb 2001 14:45:16 -0800

   >>>>> "Eli" == Eli Zaretskii <eliz@delorie.com> writes:
   >> > Is there any particular reason why you need the PID argument?  AFAICS
   >> > it will always be equal to INFERIOR_PID, so I think we can do without
   >> > it.  This is also true for the other i386_hwbp_* functions you're
   >> > proposing.
   >> 
   >> I think it'd be better to not rely on ``inferior_pid''.  I would
   >> rather see the explicitly passed.  There will come a day when GDB
   >> is able to debug more than one process at a time and to perpetuate
   >> reliance on inferior pid would be short sighted.

   Eli> I have two opposite opinions here.  We need to resolve this somehow.

   We're going to need to pass a PID, or perhaps some new representation
   of a execution context, to a lot of code functions that don't allready
   have such an argument.  It is not clear to me that adding such an
   argument "because it will be needed" is correct, considering that the
   design has not yet started.  The truth is we don't know "what" will be
   needed, so we'll have to revisit this function (among many others)
   down the line anyway.

I agree with J.T. here.

Mark

Re: [RFC] Unified watchpoints for x86 platforms

[Kevin Buettner]

On Feb 16,  2:29am, Eli Zaretskii wrote:

> > We're going to need to pass a PID, or perhaps some new representation
> > of a execution context, to a lot of code functions that don't allready
> > have such an argument.
> 
> Sorry, I'm not sure I'm following: why do you envision we'll need to
> pass the PID to functions that don't receive it today?  What
> function(s) did you have in mind?

The idea (I think) is to make most uses of ``inferior_pid'' go away. 
In its place will be occurrences of PIDGET (ptid) (or something along
these lines) where ptid is passed from somewhere else.  As a result,
it will very likely become necessary to pass the execution context (or
perhaps an identifier representing the execution context) as a
parameter or perhaps as a member of some larger structure.

See http://sources.redhat.com/ml/gdb-patches/2000-10/msg00014.html

This patch doesn't address the elimination of inferior_pid, but it
does take a first step towards providing a more robust execution
context identifier in the sense that it is now possible to encode a
process id, thread id, and lightweight process id in one of these
identifiers.  At the moment, we use a single int to encode the
following:

 - a process id (alone)
 - a process id and a thread id
 - a process id and an lwp id

IIRC, there are also times where the int in question ends up
containing just the thread id or just the lwp id.

Kevin

Re: [RFC] Unified watchpoints for x86 platforms

[J.T. Conklin]

>>>>> "Eli" == Eli Zaretskii <eliz@delorie.com> writes:
>> From: jtc@redback.com (J.T. Conklin)
>> Date: 15 Feb 2001 14:45:16 -0800
>> 
>> We're going to need to pass a PID, or perhaps some new representation
>> of a execution context, to a lot of code functions that don't allready
>> have such an argument.

Eli> Sorry, I'm not sure I'm following: why do you envision we'll need to
Eli> pass the PID to functions that don't receive it today?  What
Eli> function(s) did you have in mind?

I was speaking in generalities.  

But if GDB is ever going to be able to debug multiple independent
processes (perhaps with multiple threads) as has been a stated long
term goal, we are going to have to entirely revamp how a "execution
context" is represented, and how target functions know what context
they are operating on.  IMO, an inferior_pid global, and passing pids
to various functions is not enough.

        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: [RFC] Unified watchpoints for x86 platforms

[Eli Zaretskii]

While writing the code for the x86 watchpoint support, a question
popped up regarding the Global Enable and Local Enable flags in the
DR7 (Debug Control) register.

For those who might not remember what those flags are: Each
hardware-assisted breakpoint or watchpoint should be enabled by
setting bits in the DR7 register.  A watchpoint can be enabled locally
or globally, depending on which bit in DR7 is set for that watchpoint.
A watchpoint that is enabled locally will only break if it is hit in
the current task.  A watchpoint that is enabled globally will break in
any task.

Now, I understand that, by and large, x86 platforms enable watchpoints
locally, to avoid spurious traps triggered by other processes.  If
that is true, should the code I write use the Local Enable flag
unconditionally?  Or perhaps it would be useful to define an API that
enables a target to control whether the global or local enable flag is
used?

Opinions?

Re: [RFC] Unified watchpoints for x86 platforms

[Mark Kettenis]

Eli Zaretskii <eliz@delorie.com> writes:

> While writing the code for the x86 watchpoint support, a question
> popped up regarding the Global Enable and Local Enable flags in the
> DR7 (Debug Control) register.
> 
> For those who might not remember what those flags are: Each
> hardware-assisted breakpoint or watchpoint should be enabled by
> setting bits in the DR7 register.  A watchpoint can be enabled locally
> or globally, depending on which bit in DR7 is set for that watchpoint.
> A watchpoint that is enabled locally will only break if it is hit in
> the current task.  A watchpoint that is enabled globally will break in
> any task.

Makes me wonder if global watchpoints would really work if hardware
task switching isn't used.

Anyway, I guess most x86 OS'es wouldn't allow any real global
watchpoints.  For Linux it doesn't matter whether you set the local or
the global bit.

> Now, I understand that, by and large, x86 platforms enable watchpoints
> locally, to avoid spurious traps triggered by other processes.  If
> that is true, should the code I write use the Local Enable flag
> unconditionally?  Or perhaps it would be useful to define an API that
> enables a target to control whether the global or local enable flag is
> used?

In principle a GDB command to control whether to enable watchpoints
locally or globally wouldn't be a bad idea.  But I don't know any
targets where this would be useful.  So if it isn't useful for DJGPP
either, I wouldn't bother.  A comment about the issue at the right
spot would be nice though.

Mark

Re: [RFC] Unified watchpoints for x86 platforms

[Eli Zaretskii]

> From: Mark Kettenis <kettenis@wins.uva.nl>
> Date: 27 Feb 2001 12:28:43 +0100
> > 
> > For those who might not remember what those flags are: Each
> > hardware-assisted breakpoint or watchpoint should be enabled by
> > setting bits in the DR7 register.  A watchpoint can be enabled locally
> > or globally, depending on which bit in DR7 is set for that watchpoint.
> > A watchpoint that is enabled locally will only break if it is hit in
> > the current task.  A watchpoint that is enabled globally will break in
> > any task.
> 
> Makes me wonder if global watchpoints would really work if hardware
> task switching isn't used.

I guess the answer depends on how exactly is the software task
switching implemented.  Namely, does it reset the GE and LE bits in
DR7 when the task switch happens.

> In principle a GDB command to control whether to enable watchpoints
> locally or globally wouldn't be a bad idea.  But I don't know any
> targets where this would be useful.  So if it isn't useful for DJGPP
> either, I wouldn't bother.  A comment about the issue at the right
> spot would be nice though.

That was my tendency as well.  Thanks for the feedback.

Re: [RFA] Unified watchpoints for x86 platforms

[Eli Zaretskii]

This is the first cut of the code to support hardware breakpoints and
watchpoints on all x86 platforms.  It works for me in the DJGPP port.
Please critique as you see fit.

I'm still working on the doco changes for gdbint.texinfo, but I
thought there's no reason not to start the review process right away.

Note that the names of the functions and macros have changed a little
since the draft API I posted here.  Also, since Fernando said that
there are remote targets which might want to use hardware watchpoints,
I decided to put the code on i386-tdep.c.

I'd like to thank everybody who participated in discussions and
returned feedback.

2001-03-03  Eli Zaretskii  <eliz@is.elta.co.il>

	Unified support for hardware breakpoints and watchpoints on
	x86 targets:

	* config/i386/tm-i386.h (TARGET_HAS_HARDWARE_WATCHPOINTS):
	Define.
	(i386_cleanup_dregs, i386_insert_watchpoint)
	(i386_remove_watchpoint, i386_region_ok_for_watchpoint)
	(i386_stopped_by_hwbp, i386_stopped_data_address)
	(i386_insert_hw_breakpoint, i386_remove_hw_breakpoint): Declare
	prototypes.
	(TARGET_CAN_USE_HARDWARE_WATCHPOINT) 
	(TARGET_REGION_OK_FOR_HW_WATCHPOINT, HAVE_CONTINUABLE_WATCHPOINT) 
	(STOPPED_BY_WATCHPOINT, target_stopped_data_address) 
	(target_insert_watchpoint, target_remove_watchpoint) 
	(target_insert_hw_breakpoint, target_remove_hw_breakpoint): Define
	to call the appropriate i386_* functions.

	* i386-tdep.c (I386_DR_CONTROL_MASK, I386_DR_LOCAL_ENABLE) 
	(I386_DR_GLOBAL_ENABLE, I386_DR_DISABLE, I386_DR_SET_RW_LEN) 
	(I386_DR_GET_RW_LEN, I386_DR_WATCH_HIT): New macros.
	(dr_mirror, dr_status_mirror, dr_control_mirror, dr_ref_count)
	(maint_show_dr): New variables.
	(i386_cleanup_dregs, i386_show_dr, i386_length_and_rw_bits) 
	(i386_insert_aligned_watchpoint, i386_remove_aligned_watchpoint) 
	(i386_handle_nonaligned_watchpoint, i386_insert_watchpoint) 
	(i386_remove_watchpoint, i386_region_ok_for_watchpoint) 
	(i386_stopped_data_address, i386_stopped_by_hwbp) 
	(i386_insert_hw_breakpoint, i386_remove_hw_breakpoint): New
	functions.
	(_initialize_i386_tdep): Add new maint command `show-debug-regs',
	sets maint_show_dr to non-zero value and activates debugging
	print-outs in functions which insert, remove, and test
	watchpoints and hardware breakpoints.


--- gdb/i386-tdep.c~0	Thu Dec 21 22:52:58 2000
+++ gdb/i386-tdep.c	Sat Mar  3 23:18:12 2001
@@ -1,5 +1,5 @@
 /* Intel 386 target-dependent stuff.
-   Copyright (C) 1988, 1989, 1991, 1994, 1995, 1996, 1998
+   Copyright (C) 1988, 1989, 1991, 1994, 1995, 1996, 1998, 2001
    Free Software Foundation, Inc.
 
    This file is part of GDB.
@@ -858,6 +858,659 @@ i386_register_convert_to_raw (struct typ
   memcpy (to, from, FPU_REG_RAW_SIZE);
 }
 
+
+/* Support for hardware watchpoints and breakpoints using the x86
+   debug registers.
+
+   This provides several functions for inserting and removing
+   hardware-assisted breakpoints and watchpoints, testing if
+   one or more of the watchpoints triggerd and at what address,
+   checking whether a given region can be watched, etc.  See
+   the section "Exported API functions" below.
+
+   A target which wants to use these functions should define
+   several macros, such as `target_insert_watchpoint' and
+   `target_stopped_data_address', listed in target.h, to call
+   the appropriate functions below.
+
+   Each target should also provide several low-level macros
+   that will be called to insert watchpoints and hardware
+   breakpoints into the inferior, remove them, and check their
+   status.  These macros are:
+
+      I386_DR_LOW_SET_CONTROL  -- set the debug control (DR7)
+				  register to a given value
+
+      I386_DR_LOW_SET_ADDR     -- put an address into one debug
+				  register
+
+      I386_DR_LOW_RESET_ADDR   -- reset the address stored in
+				  one debug register
+
+      I386_DR_LOW_GET_STATUS   -- return the value of the debug
+				  status (DR6) register.
+
+   The functions below implement debug registers sharing by
+   reference counts, and allow to watch regions up to 16 bytes
+   long.  */
+
+#ifdef TARGET_HAS_HARDWARE_WATCHPOINTS
+
+#ifdef HAVE_PTRACE_H
+#include <ptrace.h>
+#else
+#ifdef HAVE_SYS_PTRACE_H
+#include <sys/ptrace.h>
+#endif
+#endif
+
+#ifdef HAVE_SYS_USER
+#include <sys/user.h>
+#endif
+
+/* FIXME: The following should be just "#include <sys/debugreg.h>",
+   but the the Linux 2.1.x kernel and glibc 2.0.x are not in sync;
+   including <sys/debugreg.h> will result in an error.  With luck,
+   these losers will get their act together and we can trash this hack
+   in the near future.
+
+   --jsm 1998-10-21; modified by eliz 2001-02-24.  */
+
+#ifdef HAVE_ASM_DEBUGREG_H
+#include <asm/debugreg.h>
+#else  /* !HAVE_ASM_DEBUGREG_H */
+
+#ifdef HAVE_SYS_DEBUGREG_H
+#include <sys/debugreg.h>
+#else  /* !HAVE_SYS_DEBUGREG_H */
+
+/* Provide definitions for platforms which don't have debugreg.h, such
+   as DJGPP.  As a side effect, explain what each macro means or does.  */
+
+/* Debug registers' indices.  */
+#define DR_FIRSTADDR		0  /* index of first debug address register */
+#define DR_LASTADDR		3  /* index of last debug address register */
+#define DR_STATUS		6  /* index of debug status register (DR6) */
+#define DR_CONTROL		7  /* index of debug control register (DR7) */
+
+/* DR7 Debug Control register fields.  */
+
+/* How many bits to skip in DR7 to get to R/W and LEN fields.  */
+#define DR_CONTROL_SHIFT	16
+/* How many bits in DR7 per R/W and LEN field for each watchpoint.  */
+#define DR_CONTROL_SIZE		4
+
+/* Watchpoint/breakpoint read/write fields in DR7.  */
+#define DR_RW_EXECUTE		(0x0) /* break on instruction execution */
+#define DR_RW_WRITE		(0x1) /* break on data writes */
+#define DR_RW_READ		(0x3) /* break on data reads or writes */
+
+/* Watchpoint/breakpoint length fields in DR7.  The 2-bit left shift
+   is so we could OR this with the read/write field defined above.  */
+#define DR_LEN_1		(0x0 << 2) /* 1-byte region watch or breakpt */
+#define DR_LEN_2		(0x1 << 2) /* 2-byte region watch */
+#define DR_LEN_4		(0x3 << 2) /* 4-byte region watch */
+
+/* Local and Global Enable flags in DR7.
+
+   When the Local Enable flag is set, the breakpoint/watchpoint is
+   enabled only for the current task; the processor automatically
+   clears this flag on every task switch.  When the Global Enable
+   flag is set, the breakpoint/watchpoint is enabled for all tasks;
+   the processor never clears this flag.
+
+   Currently, all watchpoint are locally enabled.  If you need to
+   enable them globally, read the comment which pertains to this in
+   i386_insert_aligned_watchpoint below.  */
+#define DR_LOCAL_ENABLE_SHIFT	0   /* extra shift to the local enable bit */
+#define DR_GLOBAL_ENABLE_SHIFT	1   /* extra shift to the global enable bit */
+#define DR_ENABLE_SIZE		2   /* 2 enable bits per debug register */
+
+/* Local and global exact breakpoint enable flags (a.k.a. slowdown
+   flags).  These are only required on i386, to allow detection of the
+   exact instruction which caused a watchpoint to break; i486 and
+   later processors do that automatically.  We set these flags for
+   back compatibility.  */
+#define DR_LOCAL_SLOWDOWN	(0x100)
+#define DR_GLOBAL_SLOWDOWN	(0x200)
+
+/* Fields reserved by Intel.  This includes the GD (General Detect
+   Enable) flag, which causes a debug exception to be generated when a
+   MOV instruction accesses one of the debug registers.
+
+   FIXME: My Intel manual says we should use 0xF800, not 0xFC00.  */
+#define DR_CONTROL_RESERVED	(0xFC00)
+
+#endif /* !HAVE_SYS_DEBUGREG_H */
+
+#endif /* !HAVE_ASM_DEBUGREG_H */
+
+/* This is here for completeness.  No platform supports this
+   functionality yet (as of Feb-2001).  Note that the DE flag in the
+   CR4 register needs to be set to support this.  */
+#ifndef DR_RW_IORW
+#define DR_RW_IORW		(0x2) /* break on I/O reads or writes */
+#endif
+
+/* Auxiliary helper macros.  */
+
+/* A value that masks all fields in DR7 that are reserved by Intel.  */
+#define I386_DR_CONTROL_MASK		(~DR_CONTROL_RESERVED)
+
+/* The I'th debug register is vacant if its Local and Global Enable
+   bits are reset in the Debug Control register.  */
+#define I386_DR_VACANT(i) \
+  ((dr_control_mirror & (3 << (DR_ENABLE_SIZE * (i)))) == 0)
+
+/* Locally enable the break/watchpoint in the I'th debug register.  */
+#define I386_DR_LOCAL_ENABLE(i) \
+  dr_control_mirror |= (1 << (DR_LOCAL_ENABLE_SHIFT + DR_ENABLE_SIZE * (i)))
+
+/* Globally enable the break/watchpoint in the I'th debug register.  */
+#define I386_DR_GLOBAL_ENABLE(i) \
+  dr_control_mirror |= (1 << (DR_GLOBAL_ENABLE_SHIFT + DR_ENABLE_SIZE * (i)))
+
+/* Disable the break/watchpoint in the I'th debug register.  */
+#define I386_DR_DISABLE(i) \
+  dr_control_mirror &= ~(3 << (DR_ENABLE_SIZE * (i)))
+
+/* Set in DR7 the RW and LEN fields for the I'th debug register.  */
+#define I386_DR_SET_RW_LEN(i,rwlen) \
+  do { \
+    dr_control_mirror &= ~(0x0f << (DR_CONTROL_SHIFT+DR_CONTROL_SIZE*(i)));   \
+    dr_control_mirror |= ((rwlen) << (DR_CONTROL_SHIFT+DR_CONTROL_SIZE*(i))); \
+  } while (0)
+
+/* Get from DR7 the RW and LEN fields for the I'th debug register.  */
+#define I386_DR_GET_RW_LEN(i) \
+  ((dr_control_mirror >> (DR_CONTROL_SHIFT + DR_CONTROL_SIZE * (i))) & 0x0f)
+
+/* Did the watchpoint whose address is in the I'th register break?  */
+#define I386_DR_WATCH_HIT(i)	(dr_status_mirror & (1 << (i)))
+
+/* A macro to loop over all debug registers.  */
+#define ALL_DEBUG_REGISTERS(i)	for (i = 0; i <= DR_LASTADDR-DR_FIRSTADDR; i++)
+
+/* This is in i386v-nat.c, so let's have it here, just in case.  */
+#if !defined (offsetof)
+#define offsetof(TYPE, MEMBER) ((unsigned long) &((TYPE *)0)->MEMBER)
+#endif
+
+/* Mirror the inferior's DRi registers.  We keep the status and
+   control registers separated because they don't hold addresses.  */
+static CORE_ADDR dr_mirror[DR_LASTADDR - DR_FIRSTADDR + 1];
+static unsigned	 dr_status_mirror, dr_control_mirror;
+
+/* Reference counts for each debug register.  */
+static int	 dr_ref_count[DR_LASTADDR - DR_FIRSTADDR + 1];
+
+/* Whether or not to print the mirrored debug registers.  */
+static int	 maint_show_dr;
+
+/* Types of operations supported by i386_handle_nonaligned_watchpoint.  */
+typedef enum { WP_INSERT, WP_REMOVE, WP_COUNT } i386_wp_op_t;
+
+/* Exported API functions.  */
+
+/* Clear the reference counts and forget everything we knew about DRi.  */
+void i386_cleanup_dregs (void);
+
+/* Insert a watchpoint to watch a memory region which starts at
+   address ADDR and whose length is LEN bytes.  Watch memory accesses
+   of the type TYPE.  Return 0 on success, -1 on failure.  */
+int  i386_insert_watchpoint (CORE_ADDR addr, int len, int type);
+
+/* Remove a watchpoint that watched the memory region which starts at
+   address ADDR, whose length is LEN bytes, and for accesses of the
+   type TYPE.  Return 0 on success, -1 on failure.  */
+int  i386_remove_watchpoint (CORE_ADDR addr, int len, int type);
+
+/* Return non-zero if we can watch a memory region that starts at
+   address ADDR and whose length is LEN bytes.  */
+int  i386_region_ok_for_watchpoint (CORE_ADDR addr, int len);
+
+/* Return non-zero if the inferior has some break/watchpoint that
+   triggered.  */
+int  i386_stopped_by_hwbp (void);
+
+/* If the inferior has some break/watchpoint that triggered, return
+   the address associated with that break/watchpoint.  Otherwise,
+   return zero.  */
+CORE_ADDR i386_stopped_data_address (void);
+
+/* Insert a hardware-assisted breakpoint at address ADDR.  SHADOW is
+   unused.  Return 0 on success, EBUSY on failure.  */
+int  i386_insert_hw_breakpoint (CORE_ADDR addr, void *shadow);
+
+/* Remove a hardware-assisted breakpoint at address ADDR.  SHADOW is
+   unused.  Return 0 on success, -1 on failure.  */
+int  i386_remove_hw_breakpoint (CORE_ADDR addr, void *shadow);
+
+/* Internal functions.  */
+
+/* Return the value of a 4-bit field for DR7 suitable for watching a
+   region of LEN bytes for accesses of type TYPE.  LEN is assumed
+   to have the value of 1, 2, or 4.  */
+static unsigned i386_length_and_rw_bits (int len, enum target_hw_bp_type type);
+
+/* Insert a watchpoint at address ADDR, which is assumed to be aligned
+   according to the length of the region to watch.  LEN_RW_BITS is the
+   value of the bit-field from DR7 which describes the length and
+   access type of the region to be watched by this watchpoint.  Return
+   0 on success, -1 on failure.  */
+static int i386_insert_aligned_watchpoint (CORE_ADDR addr,
+					   unsigned len_rw_bits);
+
+/* Remove a watchpoint at address ADDR, which is assumed to be aligned
+   according to the length of the region to watch.  LEN_RW_BITS is the
+   value of the bits from DR7 which describes the length and access
+   type of the region watched by this watchpoint.  Return 0 on
+   success, -1 on failure.  */
+static int i386_remove_aligned_watchpoint (CORE_ADDR addr,
+					   unsigned len_rw_bits);
+
+/* Insert or remove a (possibly non-aligned) watchpoint, or count the
+   number of debug registers required to watch a region at address
+   ADDR whose length is LEN for accesses of type TYPE.  Return 0 on
+   successful insertion or removal, a positive number when queried
+   about the number of registers, or -1 on failure.  If WHAT is not
+   a valid value, returns EINVAL.  */
+static int i386_handle_nonaligned_watchpoint (i386_wp_op_t what,
+					      CORE_ADDR addr, int len,
+					      enum target_hw_bp_type type);
+
+/* Implementation.  */
+
+/* Clear the reference counts and forget everything we knew about
+   the debug registers.  */
+void
+i386_cleanup_dregs (void)
+{
+  int i;
+
+  ALL_DEBUG_REGISTERS(i)
+    {
+      dr_mirror[i] = 0;
+      dr_ref_count[i] = 0;
+    }
+  dr_control_mirror = 0;
+  dr_status_mirror  = 0;
+}
+
+/* Print the values of the mirrored debug registers.
+   This is called when maint_show_dr is non-zero.  To set that
+   up, type "maint show-debug-regs" at GDB's prompt.  */
+static void
+i386_show_dr (const char *func, CORE_ADDR addr,
+	      int len, enum target_hw_bp_type type)
+{
+  int i;
+
+  puts_unfiltered (func);
+  if (addr || len)
+    printf_unfiltered (" (addr=%lx, len=%d, type=%s)",
+		       /* This code is for ia32, so casting CORE_ADDR
+			  to unsigned long should be okay.  */
+		       (unsigned long)addr, len,
+		       type == hw_write ? "data-write"
+		       : (type == hw_read ? "data-read"
+			  : (type == hw_access ? "data-read/write"
+			     : (type == hw_execute ? "instruction-execute"
+				/* FIXME: if/when I/O read/write
+				   watchpoints are supported, add them
+				   here.  */
+				: "??unknown??"))));
+  puts_unfiltered (":\n");
+  printf_unfiltered ("CONTROL (DR7): %08x          STATUS (DR6): %08x\n",
+		     dr_control_mirror, dr_status_mirror);
+  ALL_DEBUG_REGISTERS(i)
+    {
+      printf_unfiltered ("DR%d: addr=%08lx, ref.count=%d  DR%d: addr=%08lx, ref.count=%d\n",
+			 i, dr_mirror[i], dr_ref_count[i],
+			 i+1, dr_mirror[i+1], dr_ref_count[i+1]);
+      i++;
+    }
+}
+
+/* Return the value of a 4-bit field for DR7 suitable for watching a
+   region of LEN bytes for accesses of type TYPE.  LEN is assumed
+   to have the value of 1, 2, or 4.  */
+static unsigned
+i386_length_and_rw_bits (int len, enum target_hw_bp_type type)
+{
+  unsigned rw;
+
+  switch (type)
+    {
+      case hw_execute:
+	rw = DR_RW_EXECUTE;
+	break;
+      case hw_write:
+	rw = DR_RW_WRITE;
+	break;
+      case hw_read:	 /* x86 doesn't support data-read watchpoints */
+      case hw_access:
+	rw = DR_RW_READ;
+	break;
+#if 0
+      case hw_io_access: /* not yet supported */
+	rw = DR_RW_IORW;
+	break;
+#endif
+      default:
+	internal_error ("\
+Invalid hw breakpoint type %d in i386_length_and_rw_bits.\n", (int)type);
+    }
+
+  switch (len)
+    {
+      case 4:
+	return (DR_LEN_4 | rw);
+      case 2:
+	return (DR_LEN_2 | rw);
+      case 1:
+	return (DR_LEN_1 | rw);
+      default:
+	internal_error ("\
+Invalid hw breakpoint length %d in i386_length_and_rw_bits.\n", len);
+    }
+}
+
+/* Insert a watchpoint at address ADDR, which is assumed to be aligned
+   according to the length of the region to watch.  LEN_RW_BITS is the
+   value of the bits from DR7 which describes the length and access
+   type of the region to be watched by this watchpoint.  Return 0 on
+   success, -1 on failure.  */
+static int
+i386_insert_aligned_watchpoint (CORE_ADDR addr, unsigned len_rw_bits)
+{
+  int i;
+
+  /* First, look for an occupied debug register with the same address
+     and the same RW and LEN definitions.  If we find one, we can
+     reuse it for this watchpoint as well (and save a register).  */
+  ALL_DEBUG_REGISTERS(i)
+    {
+      if (!I386_DR_VACANT (i)
+	  && dr_mirror[i] == addr
+	  && I386_DR_GET_RW_LEN (i) == len_rw_bits)
+	{
+	  dr_ref_count[i]++;
+	  return 0;
+	}
+    }
+
+  /* Next, look for a vacant debug register.  */
+  ALL_DEBUG_REGISTERS(i)
+    {
+      if (I386_DR_VACANT (i))
+	break;
+    }
+
+  /* No more debug registers!  */
+  if (i > DR_LASTADDR - DR_FIRSTADDR)
+    return -1;
+
+  /* Now set up the register I to watch our region.  */
+
+  /* Record the info in our local mirrored array.  */
+  dr_mirror[i] = addr;
+  dr_ref_count[i] = 1;
+  I386_DR_SET_RW_LEN (i, len_rw_bits);
+  /* Note: we only enable the watchpoint locally, i.e. in the current
+     task.  Currently, no x86 target allows or supports global
+     watchpoints; however, if any target would want that in the
+     future, GDB should probably provide a command to control whether
+     to enable watchpoints globally or locally, and the code below
+     should use global or local enable and slow-down flags as
+     appropriate.  */
+  I386_DR_LOCAL_ENABLE (i);
+  dr_control_mirror |= DR_LOCAL_SLOWDOWN;
+  dr_control_mirror &= I386_DR_CONTROL_MASK;
+
+  /* Finally, actually pass the info to the inferior.  */
+  I386_DR_LOW_SET_CONTROL (dr_control_mirror);
+  I386_DR_LOW_SET_ADDR (i + DR_FIRSTADDR, addr);
+
+  return 0;
+}
+
+/* Remove a watchpoint at address ADDR, which is assumed to be aligned
+   according to the length of the region to watch.  LEN_RW_BITS is the
+   value of the bits from DR7 which describes the length and access
+   type of the region watched by this watchpoint.  Return 0 on
+   success, -1 on failure.  */
+static int
+i386_remove_aligned_watchpoint (CORE_ADDR addr, unsigned len_rw_bits)
+{
+  int i, retval = -1;
+
+  ALL_DEBUG_REGISTERS(i)
+    {
+      if (!I386_DR_VACANT (i)
+	  && dr_mirror[i] == addr
+	  && I386_DR_GET_RW_LEN (i) == len_rw_bits)
+	{
+	  if (--dr_ref_count[i] == 0) /* no longer in use? */
+	    {
+	      /* Reset our mirror.  */
+	      dr_mirror[i] = 0;
+	      I386_DR_DISABLE (i);
+	      /* Reset it in the inferior.  */
+	      I386_DR_LOW_RESET_ADDR (i + DR_FIRSTADDR);
+	      I386_DR_LOW_SET_CONTROL (dr_control_mirror);
+	    }
+	  retval = 0;
+	}
+    }
+
+  return retval;
+}
+
+/* Insert or remove a (possibly non-aligned) watchpoint, or count the
+   number of debug registers required to watch a region at address
+   ADDR whose length is LEN for accesses of type TYPE.  Return 0 on
+   successful insertion or removal, a positive number when queried
+   about the number of registers, or -1 on failure.  If WHAT is not
+   a valid value, returns EINVAL.  */
+static int
+i386_handle_nonaligned_watchpoint (i386_wp_op_t what, CORE_ADDR addr, int len,
+				   enum target_hw_bp_type type)
+{
+  int align;
+  int size;
+  int rv = 0, status = 0;
+
+  static int size_try_array[4][4] =
+  {
+    { 1, 1, 1, 1 },		/* trying size one */
+    { 2, 1, 2, 1 },		/* trying size two */
+    { 2, 1, 2, 1 },		/* trying size three */
+    { 4, 1, 2, 1 }		/* trying size four */
+  };
+
+  while (len > 0)
+    {
+      align = addr % 4;
+      /* Four is the maximum length an x86 debug register can watch.  */
+      size = size_try_array[len > 4 ? 3 : len - 1][align];
+      if (what == WP_COUNT)
+	/* size_try_array[] is defined so that each iteration through
+	   the loop is guaranteed to produce an address and a size
+	   that can be watched with a single debug register.  Thus,
+	   for counting the registers required to watch a region, we
+	   simply need to increment the count on each iteration.  */
+	rv++;
+      else
+	{
+	  unsigned len_rw = i386_length_and_rw_bits (size, type);
+
+	  if (what == WP_INSERT)
+	    status = i386_insert_aligned_watchpoint (addr, len_rw);
+	  else if (what == WP_REMOVE)
+	    status = i386_remove_aligned_watchpoint (addr, len_rw);
+	  else
+	    status = EINVAL;
+	  /* We keep the loop going even after a failure, because some
+	     of the other aligned watchpoints might still succeed
+	     (e.g. if they watch addresses that are already watched,
+	     in which case we just increment the reference counts of
+	     occupied debug registers).  If we break out of the loop
+	     too early, we could cause those addresses watched by
+	     other watchpoints to be disabled when breakpoint.c reacts
+	     to our failure to insert this watchpoint and tries to
+	     remove it.  */
+	  if (status)
+	    rv = status;
+	}
+      addr += size;
+      len -= size;
+    }
+  return rv;
+}
+
+/* Insert a watchpoint to watch a memory region which starts at
+   address ADDR and whose length is LEN bytes.  Watch memory accesses
+   of the type TYPE.  Return 0 on success, -1 on failure.  */
+int
+i386_insert_watchpoint (CORE_ADDR addr, int len, int type)
+{
+  int retval;
+
+  if (len == 3 || len > 4 || addr % len != 0)
+    retval = i386_handle_nonaligned_watchpoint (WP_INSERT, addr, len, type);
+  else
+    {
+      unsigned len_rw = i386_length_and_rw_bits (len, type);
+
+      retval = i386_insert_aligned_watchpoint (addr, len_rw);
+    }
+
+  if (maint_show_dr)
+    i386_show_dr ("insert_watchpoint", addr, len, type);
+
+  return retval;
+}
+
+/* Remove a watchpoint that watched the memory region which starts at
+   address ADDR, whose length is LEN bytes, and for accesses of the
+   type TYPE.  Return 0 on success, -1 on failure.  */
+int
+i386_remove_watchpoint (CORE_ADDR addr, int len, int type)
+{
+  int retval;
+
+  if (len == 3 || len > 4 || addr % len != 0)
+    retval = i386_handle_nonaligned_watchpoint (WP_REMOVE, addr, len, type);
+  else
+    {
+      unsigned len_rw = i386_length_and_rw_bits (len, type);
+
+      retval = i386_remove_aligned_watchpoint (addr, len_rw);
+    }
+
+  if (maint_show_dr)
+    i386_show_dr ("remove_watchpoint", addr, len, type);
+
+  return retval;
+}
+
+/* Return non-zero if we can watch a memory region that starts at
+   address ADDR and whose length is LEN bytes.  */
+int
+i386_region_ok_for_watchpoint (CORE_ADDR addr, int len)
+{
+  /* Compute how many aligned watchpoints we would need to cover this
+     region.  */
+  int nregs = i386_handle_nonaligned_watchpoint (WP_COUNT, addr, len,
+						 hw_write);
+
+  return nregs <= 4 ? 1 : 0;
+}
+
+/* If the inferior has some watchpoint that triggered, return the
+   address associated with that watchpoint.  Otherwise, return
+   zero.  */
+CORE_ADDR
+i386_stopped_data_address (void)
+{
+  int i;
+  CORE_ADDR ret = 0;
+
+  dr_status_mirror = I386_DR_LOW_GET_STATUS ();
+
+  ALL_DEBUG_REGISTERS(i)
+    {
+      if (I386_DR_WATCH_HIT (i)
+	  /* This second condition makes sure DRi is set up for a data
+	     watchpoint, not a hardware breakpoint.  The reason is
+	     that GDB doesn't call the target_stopped_data_address
+	     method except for data watchpoints.  In fact, if this
+	     function returns non-zero for a hardware breakpoint, GDB
+	     will become confused.  You _have_ been warned!  */
+	  && I386_DR_GET_RW_LEN (i) != 0)
+	{
+	  ret = dr_mirror[i];
+	  if (maint_show_dr)
+	    i386_show_dr ("watchpoint_hit", ret, -1, hw_write);
+	}
+    }
+  if (maint_show_dr && ret == 0)
+    i386_show_dr ("stopped_data_addr", 0, 0, hw_write);
+
+  return ret;
+}
+
+/* Return non-zero if the inferior has some break/watchpoint that
+   triggered.  */
+int
+i386_stopped_by_hwbp (void)
+{
+  int i;
+
+  dr_status_mirror = I386_DR_LOW_GET_STATUS ();
+  if (maint_show_dr)
+    i386_show_dr ("stopped_by_hwbp", 0, 0, hw_execute);
+
+  ALL_DEBUG_REGISTERS(i)
+    {
+      if (I386_DR_WATCH_HIT (i))
+	return 1;
+    }
+
+  return 0;
+}
+
+/* Insert a hardware-assisted breakpoint at address ADDR.  SHADOW is
+   unused.  Return 0 on success, EBUSY on failure.  */
+int
+i386_insert_hw_breakpoint (CORE_ADDR addr, void *shadow)
+{
+  unsigned len_rw = i386_length_and_rw_bits (1, hw_execute);
+  int retval = i386_insert_aligned_watchpoint (addr, len_rw) ? EBUSY : 0;
+
+  if (maint_show_dr)
+    i386_show_dr ("insert_hwbp", addr, 1, hw_execute);
+
+  return retval;
+}
+
+/* Remove a hardware-assisted breakpoint at address ADDR.  SHADOW is
+   unused.  Return 0 on success, -1 on failure.  */
+int
+i386_remove_hw_breakpoint (CORE_ADDR addr, void *shadow)
+{
+  unsigned len_rw = i386_length_and_rw_bits (1, hw_execute);
+  int retval = i386_remove_aligned_watchpoint (addr, len_rw);
+
+  if (maint_show_dr)
+    i386_show_dr ("remove_hwbp", addr, 1, hw_execute);
+
+  return retval;
+}
+
+#endif /* TARGET_HAS_HARDWARE_WATCHPOINTS */
+
      
 #ifdef I386V4_SIGTRAMP_SAVED_PC
 /* Get saved user PC for sigtramp from the pushed ucontext on the stack
@@ -1014,4 +1667,15 @@ and the default value is \"att\".",
      in the disassembly_flavor variable */
 
   set_disassembly_flavor ();
+
+  /* A maintenance command to enable printing the internal DRi mirror
+     variables.  */
+  add_set_cmd ("show-debug-regs", class_maintenance,
+	       var_boolean, (char *) &maint_show_dr,
+	       "\
+Set whether to show variables that mirror the x86 debug registers.\n\
+Use \"on\" to enable, \"off\" to disable.\n\
+If enabled, the debug registers values are shown when GDB inserts\n\
+or removes a hardware breakpoint or watchpoint, and when the inferior\n\
+triggers a breakpoint or watchpoint.", &maintenancelist);
 }
--- gdb/config/i386/tm-i386.h~0	Thu Jan  4 17:46:20 2001
+++ gdb/config/i386/tm-i386.h	Sat Mar  3 17:50:52 2001
@@ -1,5 +1,5 @@
 /* Macro definitions for GDB on an Intel i[345]86.
-   Copyright (C) 1995, 1996, 2000 Free Software Foundation, Inc.
+   Copyright (C) 1995, 1996, 2000, 2001 Free Software Foundation, Inc.
 
    This file is part of GDB.
 
@@ -418,4 +418,97 @@ extern void print_387_status_word (unsig
 
 #define SP_ARG0 (1 * 4)
 
+
+
+/* Hardware-assisted breakpoints and watchpoints.  */
+
+#ifdef TARGET_HAS_HARDWARE_WATCHPOINTS
+
+/* Clear the reference counts and forget everything we knew about DRi.  */
+extern void i386_cleanup_dregs (void);
+
+/* Insert a watchpoint to watch a memory region which starts at
+   address ADDR and whose length is LEN bytes.  Watch memory accesses
+   of the type TYPE.  Return 0 on success, -1 on failure.  */
+extern int  i386_insert_watchpoint (CORE_ADDR addr, int len, int type);
+
+/* Remove a watchpoint that watched the memory region which starts at
+   address ADDR, whose length is LEN bytes, and for accesses of the
+   type TYPE.  Return 0 on success, -1 on failure.  */
+extern int  i386_remove_watchpoint (CORE_ADDR addr, int len, int type);
+
+/* Return non-zero if we can watch a memory region that starts at
+   address ADDR and whose length is LEN bytes.  */
+extern int  i386_region_ok_for_watchpoint (CORE_ADDR addr, int len);
+
+/* Return non-zero if the inferior has some break/watchpoint that
+   triggered.  */
+extern int  i386_stopped_by_hwbp (void);
+
+/* If the inferior has some break/watchpoint that triggered, return
+   the address associated with that break/watchpoint.  Otherwise,
+   return zero.  */
+extern CORE_ADDR i386_stopped_data_address (void);
+
+/* Insert a hardware-assisted breakpoint at address ADDR.  SHADOW is
+   unused.  Return 0 on success, EBUSY on failure.  */
+extern int  i386_insert_hw_breakpoint (CORE_ADDR addr, void *shadow);
+
+/* Remove a hardware-assisted breakpoint at address ADDR.  SHADOW is
+   unused.  Return 0 on success, -1 on failure.  */
+extern int  i386_remove_hw_breakpoint (CORE_ADDR addr, void *shadow);
+
+/* Returns the number of hardware watchpoints of type TYPE that we can
+   set.  Value is positive if we can set CNT watchpoints, zero if
+   setting watchpoints of type TYPE is not supported, and negative if
+   CNT is more than the maximum number of watchpoints of type TYPE
+   that we can support.  TYPE is one of bp_hardware_watchpoint,
+   bp_read_watchpoint, bp_write_watchpoint, or bp_hardware_breakpoint.
+   CNT is the number of such watchpoints used so far (including this
+   one).  OTHERTYPE is non-zero if other types of watchpoints are
+   currently enabled.
+
+   We always return 1 here because we don't have enough information
+   about possible overlap of addresses that they want to watch.  As
+   an extreme example, consider the case where all the watchpoints
+   watch the same address and the same region length: then we can
+   handle a virtually unlimited number of watchpoints, due to debug
+   register sharing implemented via reference counts in i386-tdep.c.  */
+
+#define TARGET_CAN_USE_HARDWARE_WATCHPOINT(type, cnt, ot) 1
+
+/* Returns non-zero if we can use hardware watchpoints to watch a region
+   whose address is ADDR and whose length is LEN.  */
+
+#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(addr,len) \
+	i386_region_ok_for_watchpoint(addr,len)
+
+/* After a watchpoint trap, the PC points to the instruction after the
+   one that caused the trap.  Therefore we don't need to step over it.
+   But we do need to reset the status register to avoid another trap.  */
+
+#define HAVE_CONTINUABLE_WATCHPOINT
+
+#define STOPPED_BY_WATCHPOINT(W)       (i386_stopped_data_address () != 0)
+
+#define target_stopped_data_address()  i386_stopped_data_address ()
+
+/* Use these macros for watchpoint insertion/removal.  */
+
+#define target_insert_watchpoint(addr, len, type)  \
+  i386_insert_watchpoint (addr, len, type)
+
+#define target_remove_watchpoint(addr, len, type)  \
+  i386_remove_watchpoint (addr, len, type)
+
+#define target_insert_hw_breakpoint(addr, shadow)  \
+  i386_insert_hw_breakpoint(addr, shadow)
+
+#define target_remove_hw_breakpoint(addr, shadow)  \
+  i386_remove_hw_breakpoint(addr, shadow)
+
+#define DECR_PC_AFTER_HW_BREAK 0
+
+#endif /* TARGET_HAS_HARDWARE_WATCHPOINTS */
+
 #endif /* ifndef TM_I386_H */