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

[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: 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: [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: 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

[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

[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

[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

[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

[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]

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 */

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

[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

[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

[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: 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

Re: Proposal: convert function definitions to prototyped form

[David Taylor]

    Date: Fri, 2 Jun 2000 12:10:42 -0700
    From: Kevin Buettner <kevinb@cygnus.com>

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

    >       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

Sorry, I didn't consider it relevant to what I was replying to.

    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.

I don't consider the setup issue to be a big issue.  It requires some
time, but it doesn't require much human time.  Configure in the source
tree, then build.  Setup done.

During the commit phase, you don't commit anything that is new -- you
only commit files that existed prior to doing the configure.  No
separate tree needed.

    >       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.

Since Stan previously ran indent on all of the files, re-running it
*should* produce no change for stuff that hasn't changed.  Should
produce no change.  I'm not saying it won't.  If it changes its own
output when presented with it as input, I would consider that a bug.

    > 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.)

It was at a former company.  There were a small number of active
developers all within the company.  The total project was around
200-250K lines of code.  There was a great deal less use of #if within
the sources than there is in GDB...

[...]
    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.

I agree that it should be a separate activity.

I think that your script is a good thing.  And due to the amount of
hand editing that appears to be necessary with protoize vs the amount
that appears to be necessary with your script, I think we should use
your script.

Ultimately, though, since you're doing the work, you get to select the
tool.  So long as it is fairly reliable and reasonably quick (quick in
terms of *your time*, not in terms of *elapsed time*), it doesn't
ultimately matter to me what tool you use.  (Which is not to say that
I am not interested in knowing what tool you use -- I am interested.)

Like I said, I think your script is a good thing.  And I look forward
to you using it to protoize the bulk of the GDB sources.

    Kevin

David

Proposal: convert function definitions to prototyped form

[Kevin Buettner]

As many of you know, I'm in the midst of purging the use of ``PARAMS''
in prototyped function declarations from the gdb sources.  After this
activity is concluded, I'd like to begin converting function
definitions whose parameters are declared using the traditional C
style to the ISO C prototyped form.  I.e, I'd like to convert
functions of the form

    int
    foo (a, b, c)
         int a;
         char *b;
         int *c;
    {
      ...
    }

to

    int foo (int a, char *b, int *c)
    {
      ...
    }

To this end, I've been working on a script to automate most of this
conversion.  I've appended a work-in-progress version to the end of
this message and will discuss it in some detail further below.

But first...  it's been pointed out to me that the ``protoize''
program is supposed to do the same thing.  I gave ``protoize'' a whirl
and noticed the following problems:

    1) A substantial amount of initial setup is involved.  Only in
       the simplest cases could you just feed a list of files to
       work on to protoize and expect it to work okay.  In particular,
       for gdb, I found it necessary to do a configure *in the source
       directory* so that xm.h, tm.h, and nm.h are set up.  I also
       found it necessary to build in bfd so that certain header
       files are created.

       The reason all of this is necessary is because protoize invokes
       gcc to learn about the program.  As a consequence, it is also
       important to know the set of -I switches that one normally feeds
       to gcc to do a build.

    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".

    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)

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

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

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
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.)

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.)

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.

Now, I'll tell you about the fix-decls script (see below).

The fix-decls script is run via "fix-decls <dirname>" where <dirname>
is the name of the directory to (recursively) fix.  It only touches
the .c files, with the exception of gnu-regex.c and the testsuite
files.  After it converts a traditional C style declaration to a
prototyped one, it invokes GNU indent (on the function declaration
only) to make sure that our indentation standards are still adhered
to.  There is no gratuitous reformatting of any other code.

Here are a list of things that it won't handle (at the moment,
anyway):

    - declarations with comments in them
    - function pointers
    - array declarations
    - malformed declarations (e.g, look at m88k_skip_prologue in m88k-tdep.c)

Functions with traditional C declarations with any of the above in
them will be left alone.  (And need to be converted by hand at some
point.)  But this isn't so bad.  When I ran the script over the gdb
sources, it converted 5,204 function definitions.  After throwing
out gnu-regex.c (which needs to track glibc), all of the stuff in
the testsuite, and everything in tui (which has ifdefs around each
and every function declaration), there are only 143 cases left to
examine by hand.  I've sampled these and many of them are unhandled
due to having comments following a parameter's type declaration.
If I could figure out what to do about comments, we'd have even
fewer cases to fix up by hand.

Here are some of the more interesting cases that it does handle:

Missing int in declaration (i960-tdep.c):

 CORE_ADDR
-frame_args_address (fi, must_be_correct)
-     struct frame_info *fi;
+frame_args_address (struct frame_info *fi, int must_be_correct)
 {

Comma separated parameter lists (remote-mm.c):

 static void
-init_target_mm (tstart, tend, dstart, dend, entry, ms_size, rs_size, arg_start)
-     ADDR32 tstart, tend, dstart, dend, entry;
-     INT32 ms_size, rs_size;
-     ADDR32 arg_start;
+init_target_mm (ADDR32 tstart, ADDR32 tend, ADDR32 dstart, ADDR32 dend,
+		ADDR32 entry, INT32 ms_size, INT32 rs_size, ADDR32 arg_start)
 {

Comma separated list with differing number of stars on the parameter
names (sparc-tdep.c):

 static branch_type
-isbranch (instruction, addr, target)
-     long instruction;
-     CORE_ADDR addr, *target;
+isbranch (long instruction, CORE_ADDR addr, CORE_ADDR * target)
 {

And another one (command.c):

 struct cmd_list_element *
-lookup_cmd_1 (text, clist, result_list, ignore_help_classes)
-     char **text;
-     struct cmd_list_element *clist, **result_list;
-     int ignore_help_classes;
+lookup_cmd_1 (char **text, struct cmd_list_element *clist,
+	      struct cmd_list_element **result_list, int ignore_help_classes)
 {

Out of order comma separated parameter list - note positions of ``from''
and ``to'' (gdbserver/remote-utils.c):

 void
-decode_M_packet (from, mem_addr_ptr, len_ptr, to)
-     char *from, *to;
-     CORE_ADDR *mem_addr_ptr;
-     unsigned int *len_ptr;
+decode_M_packet (char *from, CORE_ADDR * mem_addr_ptr, unsigned int *len_ptr,
+		 char *to)
 {

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.

--- fix-decls ---
#!/usr/bin/perl -w

use File::Find;
use FileHandle;
use IPC::Open3;
use English;

my ($root) = @ARGV;

if (!defined($root)) {
    die "Usage: $0 root\n";
}

@ARGV = ();

find(
    sub { 
	if ($_ eq 'testsuite') {
	    $File::Find::prune = 1;
	} elsif (-f && -T && /\.c$/ && $_ ne "gnu-regex.c") {
	    push @ARGV, $File::Find::name;
	}
    },
    $root
);

#$INPLACE_EDIT = '.bak';
$INPLACE_EDIT = '';
undef $/;			# slurp entire files

while (<>) {
    s/
	^			# line start
	( 
	  \w+			# function name
	)
	(
	  \s*			# spaces
	  \(			# left paren
	  \s*			# spaces
	)
	(
	  (?:\w+\s*,\s*)*	# 1 thru N-1 parameter names
	  \w+			# last parameter name
	)
	(
	  \s*			# spaces
	  \)			# right paren
	)
	(
	  (?:
	    [^;{}()]+;		# trad C parameter decl
	  )*
	)
	(
	  \s*			# spaces
	)
	(?=^{)			# lookahead to make sure we see a
				# right curly brace at the beginning
				# of the line
    /
	fix_decl($1, $2, $3, $4, $5, $6);
    /smgex;

    s/
	^			# line start
	( 
	  \w+			# function name
	)
	\s*			# spaces
	\(			# left paren
	\s*			# spaces
	\)			# right paren
	(?=\s*^{)		# lookahead to make sure we see a
				# right curly brace at the beginning
				# of the line
     /$1 (void)/smgx;

    print;
}

sub fix_decl {
    my ($funcname, $lparen, $params, $rparen, $decls, $spaces) = @_;
    my %h = ();
    my ($param, $decl);

    # Define $bailstr to be the original function declaration.  We
    # return it when we see something which doesn't make sense.
    my $bailstr = $funcname . $lparen . $params . $rparen . $decls . $spaces;

    if ($funcname =~ /^(do|while|if)$/) {
	# 'if', 'do', and 'while' are not function names
	# find_overload_match() in valops.c contains an if statement
	# which is confused as a function if we don't have this test.
	return $bailstr;
    }

    foreach $param (split /\s*,\s*/, $params) {
	if (defined $h{$param} || $param eq 'void') {
	    # Bail; either param has already been encountered or
	    # it's void in which case the decl in question is already
	    # ISO C.
	    return $bailstr;
	}
	$h{$param} = "int $param";	# Default
    }

    $decls =~ s/\s*;\Z//;		# remove final semicolon
    			
    foreach $decl (split /\s*;\s*/, $decls) {
	my ($type, $dparams) = 
	    $decl =~ /^				# beginning of string
	              (.*?)			# type
	              (				# dparams...
		        (?:
			  \**			#  stars
			  \s*			#  spaces
			  \w+			#  identifier
			  \s*			#  spaces
			  ,			#  comma
			  \s*			#  spaces
			)*			# any number of the above
			\**			#  stars
			\s*			#  spaces
			\w+			#  identifier
		      )
		      $				# end of string
		     /sx;
	return $bailstr			if !defined $type || !defined $dparams;
	$type =~ s/\A\s+//;		# nuke leading spaces
	$type =~ s/\s+\Z//;		# nuke trailing spaces
	return $bailstr			if $type eq '';
					# Bail if no type
	foreach $param (split /\s*,\s*/, $dparams) {

	    my ($stars, $stripped_param) = 
		$param =~ /(\**)\s*(\w+)/;

	    if (!defined($stripped_param) || !defined $h{$stripped_param}) {
		# Either we couldn't find the parameter or else
		# the parameter wasn't found in the parameter list
		return $bailstr;
	    }
	    $h{$stripped_param} = "$type $param";
	}
    }

    my $newparams = join(', ', map { $h{$_} } split(/\s*,\s*/, $params));

    my $newdecl = reindent("$funcname ($newparams)\n{\n}\n");
    $newdecl =~ s/{\n}//;
    return $newdecl;
}


sub reindent {
    my ($decl, $line_length) = @_;
    $line_length = 80		unless defined $line_length;
    my ($rfh, $wfh, $efh) = (FileHandle->new, FileHandle->new,
					      FileHandle->new);
    my $pid = open3($wfh, $rfh, $efh, "indent -l$line_length");
    $rfh->input_record_separator(undef);
    $efh->input_record_separator(undef);
    $wfh->print($decl);
    $wfh->close();
    my $replacement = <$rfh>;
    $rfh->close();
    my $errstr = <$efh>;
    $efh->close();
    waitpid $pid, 0;
    $replacement =~ s#\n$##;
    if ($errstr ne "") {
	print STDERR "Check $ARGV...\n$errstr\nInput:$decl\nOutput:$replacement\n\n"
    }
    $replacement;
}
--- end fix-decls ---

Re: Proposal: convert function definitions to prototyped form

[Andrew Cagney]

Kevin Buettner wrote:
> 
> As many of you know, I'm in the midst of purging the use of ``PARAMS''
> in prototyped function declarations from the gdb sources.  After this
> activity is concluded, I'd like to begin converting function
> definitions whose parameters are declared using the traditional C
> style to the ISO C prototyped form.  I.e, I'd like to convert
> functions of the form
> 
>     int
>     foo (a, b, c)
>          int a;
>          char *b;
>          int *c;
>     {
>       ...
>     }
> 
> to
> 
>     int foo (int a, char *b, int *c)
>     {
>       ...
>     }

(you mean
	int
	foo (int a, char *b, int *c)
:-)

The obvious thing to note is that by moving to pure ISO-C we eliminate
the type promotion problems.  For instance:

	void foo (enum e e);
	void
	foo (e)
		enum e e;
	{
	}

is not well defined and many compilers reject it.  Plenty of similar
examples (involving char, short, and the like exist).

It is one less issue that people need to worry about.

The only concern I have is, given the slightly more complex nature of
the script (compared to PARAMS) there is a possibility that the
conversion re-orders or re-types the argument list.  With that in mind,
should a pre-cursor to this be to least have prototypes for all
(global?) functions (-Wmissing-prototypes?) or only do the conversion
when there is a prototype visible?

	Andrew

PS: You may want to add gdb/*-share to the list of directories to avoid.

Re: Proposal: convert function definitions to prototyped form

[Kevin Buettner]

On Jun 2,  7:42pm, Andrew Cagney wrote:

> The only concern I have is, given the slightly more complex nature of
> the script (compared to PARAMS) there is a possibility that the
> conversion re-orders or re-types the argument list.  With that in mind,
> should a pre-cursor to this be to least have prototypes for all
> (global?) functions (-Wmissing-prototypes?) or only do the conversion
> when there is a prototype visible?

I've given this matter a lot of thought.

I agree that it would be desirable to have prototypes for all
functions.  Unfortunately, while it is easy to generate prototypes,
it's not so easy to know where to stick them.  Also, even if we had
prototypes in place, there's no guarantee that we'd catch the errors
after a few builds because I think there's some code in gdb (though I
don't know how much) that never gets built!  (Due to ifdefs and
near obsolete ports.)

What we really need is a method for vetting all of the changes
immediately after running the script.  I.e, we need to make sure that
the conversion does no reordering or retyping of any argument list.
Also, we need to make sure that the rewritten function declaration
is syntactically correct.

While examining the diffs (made with the -u switch) an idea occurred
to me.  Consider the following example:

    diff -ur ../../orig/gdb-sourceware/wrapper.c ./wrapper.c
    --- ../../orig/gdb-sourceware/wrapper.c	Sat May 27 17:10:27 2000
    +++ ./wrapper.c	Thu Jun  1 23:33:16 2000
    @@ -57,11 +57,8 @@
     static int wrap_parse_and_eval_type (char *);
     
     int
    -gdb_parse_exp_1 (stringptr, block, comma, expression)
    -     char **stringptr;
    -     struct block *block;
    -     int comma;
    -     struct expression **expression;
    +gdb_parse_exp_1 (char **stringptr, struct block *block, int comma,
    +		 struct expression **expression)
     {
       struct gdb_wrapper_arguments args;
       args.args[0].pointer = stringptr;

In the above diff, the lines prepended with `-' represent the original
K&R definition.  And the lines prepended with `+' represent the
transformed code.  Moreover, the diff is extremely regular in this
respect.  So...

If you take the lines which begin with `+', prepend the type on the
line before the `-' lines and tack a semicolon onto the end, you end
up with a prototype declaration.  And, if you take the lines beginning
with `-', again tack the type onto the front and put a function body
underneath it, you have a K&R style (traditional) function definition.

Now if you put these into a file with the prototype first and the K&R
definition later on, you can run "gcc -Wall" on it to see if any
warnings are produced.  Obviously, if we get warnings, we need to look
closer to see if something went wrong with the fix-decls conversion.

Of course, there are other details to consider, like making sure that
all of the types, structs, unions, and enums are declared.  Also,
in a source tree as big as gdb, we'll likely wind up with a number
of functions with the same name, so some method of disambiguating
these will be necessary.  And then of course, there's the matter
of no declared return type and other oddments.

I've written a script called ``check-decls'' which performs these
transformations on the diff output.  When I run it on the above diff,
it produces the following output (indented by four spaces by me for
readability)

    struct block { int f0; };
    struct expression { int f1; };
    #define INLINE
    #define private
    #define CONST const
    #define NORETURN
    void init___ (void *);
    int gdb_parse_exp_1 (char **stringptr, struct block *block, int comma,
		     struct expression **expression);

    int
    gdb_parse_exp_1 (stringptr, block, comma, expression)
	 char **stringptr;
	 struct block *block;
	 int comma;
	 struct expression **expression;
    {
      int ret;
      init___ (&ret);
      return ret;
    }

    void
    use___ (void)
    {
    }

The use___ () function isn't interesting in this example, but it would
be if there had been a static declaration.

Here's what happens when I run it on *all* of the diffs:

ocotillo:ptests$ ./check-decls <declsdiff >prog.c
ocotillo:ptests$ wc prog.c
  50235  112228  960827 prog.c
ocotillo:ptests$ gcc -c -Wall prog.c
prog.c: In function `exit':
prog.c:39303: warning: function declared `noreturn' has a `return' statement
prog.c: At top level:
prog.c:45903: parse error before `arg_type'
prog.c: In function `value_primitive_field':
prog.c:45907: declaration for parameter `arg_type' but no such parameter
prog.c:45906: declaration for parameter `fieldno' but no such parameter
prog.c:45905: declaration for parameter `offset' but no such parameter
prog.c:45904: declaration for parameter `arg1' but no such parameter
prog.c:45908: argument `arg_type' doesn't match prototype
prog.c:5886: prototype declaration
prog.c:45908: argument `arg1' doesn't match prototype
prog.c:5886: prototype declaration

The `exit' warning is due to the fact that there's a declaration and
definition of exit() from standalone.c.  It is of no concern.

The error following this warning looks more serious.  Here's the declaration
and the definition of the function involved:

    value_ptr value_primitive_field (register value_ptr arg1, int offset,
			   register int fieldno, register struct type *arg_type);

    value_ptr
    value_primitive_field (arg1, offset, fieldno, arg_type)
	 register value_ptr arg1;
	 int offset;
	 register int fieldno;
	 register struct type *arg_type;
    {
      value_ptr ret;
      init___ (&ret);
      return ret;
    }

I looked at this for a long, long time and didn't see anything wrong.
Finally, I realized that arg_type was a type from a different file.
(Which is one of the problems with throwing everything into one big
pot.)  Anyway, here's the type that the script declared:

    typedef struct t44 { int f44; } arg_type;

And here's the (transformed) definition which caused it to be defined:

    bool_t
    xdr_arg_type(xdrs, objp)
	    XDR *xdrs;
	    arg_type *objp;
    {
      bool_t ret;
      init___ (&ret);
      return ret;
    }

So it turns out that it's nothing to worry about.

And that's it.  There are no other warnings or errors.  Which means
that the transformation was successful and didn't mess up any of
the parameter types.

The check-decls script is below.  One might argue that it is about as
complex as the fix-decls script.  This is true, but the code which
actually extracts the `-' and `+' lines is fairly simple.  Also, after
being extracted, there are no transformations made to these lines
aside from appending ___<num> to the function name if the script
detects that the function name has already been seen.  Most
importantly, the parameter lists are not rewritten in any way.

Most of the complexity is in the analysis and generation of the
type, struct, enum, and union declarations.  But uniqueness of
these is easy to verify.  Plus, if something is screwed up, the
compiler complains.

--- check-decls ---
#!/usr/bin/perl -w

# Feed this script a unidiff after running fix-decls and it generates
# (on stdout) a program which may be used to test the validity of the
# conversion.  Just run the result through gcc -Wall and if it
# generates any warnings, there's a problem...

undef $/;		# slurp mode
my $diff = <>;		# read entire diff in $diff;

my $decls = '';
my $defns = '';

my %userstructs = ();
my %userenums = ();
my %usertypes = ();
my %funcnames = ();
my $funcname_gensym = 0;		# for names that clash
my @needuse;

while ($diff =~
	/ (
	    ^ 				# beginning of line
	    [^\n]+			# everything til the end of line
	  )
	  \n				# newline
	  (
	    (?:
	      ^				#   beginning of line
	      -				#   minus sign
	      (?: \n			#   either just a newline
		|			#     -- or -- 
	          [^-\n]		#   any character but minus and newline
	          [^\n]*		#   the rest of the line
	          \n    		#   including the newline
	      )
	    )+				# one or more of the above
	  )
	  (
	    (?:
	      ^				#   beginning of line
	      \+			#   plus sign
	      [^+]			#   any character but plus
	      [^\n]*			#   the rest of the line
	      \n			#   including the newline
	    )+				# one or more of the above
	  )
	                                                           /mgx) {
    my ($rettype, $traddecl, $isodecl) = ($1, $2, $3);

    # Remove leading diff character from the lines extracted
    foreach ($rettype, $traddecl, $isodecl) {
	s/^.//mg;
    }

    # Find type names in parameter list
    my $parmdecls = $traddecl;
    $parmdecls =~ s/^\w+\s*\([^)]*\)//;
    foreach my $parm (split /\s*;\s*/, $parmdecls) {
	$parm =~ s/\s*\**\w+(,|$).*$//;
	analyze_type($parm);
    }

    # Resolve collisions between function name (either due to statics
    # or due to the names being in different branches of an ifdef)
    my ($funcname) = $traddecl =~ /^(\w+)/;
    if (defined $funcnames{$funcname}) {
	foreach ($traddecl, $isodecl) {
	    s/\b$funcname\b/${funcname}___$funcname_gensym/;
	}
	$funcname .= "___$funcname_gensym";
	$funcname_gensym++;
    }
    $funcnames{$funcname} = $funcname;

    # Nuke comments in the return type
    $rettype =~ s#/\*.*?\*/##g;

    # Nuke partial comment in return type
    $rettype =~ s#^.*?\*/##;

    # Eliminate ``CALLBACK'' from return type
    $rettype =~ s/\bCALLBACK\b//;

    # Eliminate ``extern'' from return type
    $rettype =~ s/\bextern\b//;

    # Eliminate leading and trailing spaces from return type
    $rettype =~ s/^\s*//;
    $rettype =~ s/\s*$//;

    if (($rettype =~ /^#/) || ($rettype eq '')) {
	# preprocessor line or empty string
	$rettype = 'int';
    } elsif ($rettype eq "static") {
	$rettype = 'static int';
    } elsif ($rettype eq "private") {
	$rettype = 'static int';
    } else {
	analyze_type($rettype);
    }

    $isodecl =~ s/\n\Z/;\n/;

    $decls .= "$rettype $isodecl";
    if ($rettype =~ /\bvoid$/) {
	$defns .= "$rettype\n$traddecl\{\n}\n\n";
    } else {
	$defns .= "$rettype\n$traddecl\{\n  $rettype ret;\n"
	       .  "  init___ (&ret);\n  return ret;\n}\n\n";
    }

    if ($rettype =~/\bstatic\b/) {
	push @needuse, $funcname;
    }
}


my $typeidx = 0;

foreach $key (sort keys %usertypes) {
    print "typedef struct t$typeidx { int f$typeidx; } $key;\n";
    $typeidx++;
}

foreach $key (sort keys %userstructs) {
    print "$key { int f$typeidx; };\n";
    $typeidx++;
}

foreach $key (sort keys %userenums) {
    print "$key { e$typeidx };\n";
    $typeidx++;
}

print "#define INLINE\n";
print "#define private\n";
print "#define CONST const\n";
print "#define NORETURN\n";
print "void init___ (void *);\n";

print $decls;
print "\n";
print $defns;

print "void\nuse___ (void)\n{\n";
foreach (@needuse) {
    print "  init___ ($_);\n";
}
print "}\n";

sub analyze_type {
    my ($parm) = @_;
    $parm =~ s/\s*\**\s*$//;
    my $type;
    if ($parm =~ /\b(struct|union)\b/) {
	$parm =~ s/\A.*\b(struct|union)\b/$1/s;
	$parm =~ s/\s*\**\s*\Z//s;
	$userstructs{$parm} = $parm;
    } elsif ($parm =~ /\b(enum)\b/) {
	$parm =~ s/\A.*\b(enum)\b/$1/s;
	$parm =~ s/\s*\**\s*\Z//s;
	$userenums{$parm} = $parm;
    } elsif ((($type) = $parm =~ /(\w+)$/)
	&& ($type !~ /^(int|char|long|short|unsigned|double
			   |register|void|const|static)$/x)) {
	$usertypes{$type} = $type;
    }
}
--- end check-decls ---

Re: Proposal: convert function definitions to prototyped form

[Kevin Buettner]

On Jun 2,  7:42pm, Andrew Cagney wrote:

> PS: You may want to add gdb/*-share to the list of directories to avoid.

I can certainly do this, but I'd like to know why they shouldn't be
converted...

Re: Proposal: convert function definitions to prototyped form

[Andrew Cagney]

Kevin Buettner wrote:
> 
> On Jun 2,  7:42pm, Andrew Cagney wrote:
> 
> > PS: You may want to add gdb/*-share to the list of directories to avoid.
> 
> I can certainly do this, but I'd like to know why they shouldn't be
> converted...

Some of the *-share files are based on code from third parties.  We
should probably try to avoid munging that code - it will make it harder
for us merge back patches.  rdi-share comes to mind.

As you note, the TUI which will be cleaned up, can be handed separatly.

	Andrew

Re: Proposal: convert function definitions to prototyped form

[Mark Kettenis]

   Date: Fri, 2 Jun 2000 00:50:19 -0700
   From: Kevin Buettner <kevinb@cygnus.com>

   Comma separated list with differing number of stars on the parameter
   names (sparc-tdep.c):

    static branch_type
   -isbranch (instruction, addr, target)
   -     long instruction;
   -     CORE_ADDR addr, *target;
   +isbranch (long instruction, CORE_ADDR addr, CORE_ADDR * target)
    {

I guess you should tweak it some more such that it outputs

   CORE_ADDR *target

instead of

   CORE_ADDR * target

(note the spurious space between * and target).

Mark

Re: Proposal: convert function definitions to prototyped form

[Kevin Buettner]

On Jun 2,  2:26pm, Mark Kettenis wrote:

>    Date: Fri, 2 Jun 2000 00:50:19 -0700
>    From: Kevin Buettner <kevinb@cygnus.com>
> 
>    Comma separated list with differing number of stars on the parameter
>    names (sparc-tdep.c):
> 
>     static branch_type
>    -isbranch (instruction, addr, target)
>    -     long instruction;
>    -     CORE_ADDR addr, *target;
>    +isbranch (long instruction, CORE_ADDR addr, CORE_ADDR * target)
>     {
> 
> I guess you should tweak it some more such that it outputs
> 
>    CORE_ADDR *target
> 
> instead of
> 
>    CORE_ADDR * target
> 
> (note the spurious space between * and target).

I noticed that.  The space was put there by ``indent''.  I would very
much like to get rid of that space and it would be easy to make the
perl script postprocess the ``indent'' output.  But in doing so, we
(obviously) generate different output than that of ``indent''.

I suppose the other solution is to fix indent.  :-)

FYI, I'm using GNU indent 2.2.5.

Kevin

Re: Proposal: convert function definitions to prototyped form

[J.T. Conklin]

>>>>> "Kevin" == Kevin Buettner <kevinb@cygnus.com> writes:
Kevin> I noticed that.  The space was put there by ``indent''.  I
Kevin> would very much like to get rid of that space and it would be
Kevin> easy to make the perl script postprocess the ``indent'' output.
Kevin> But in doing so, we (obviously) generate different output than
Kevin> that of ``indent''.
Kevin>
Kevin> I suppose the other solution is to fix indent.  :-)

You can tell indent about all the types defined by typedef with -T
option, and then it won't add the extra space.  It shouldn't be too
difficult to identify all the types.  

It might be useful for us to maintain an indent.pro file that has
these definitions so that additional runs of indent don't add back
the space.

        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: Proposal: convert function definitions to prototyped form

[Andrew Cagney]

"J.T. Conklin" wrote:
> 
> >>>>> "Kevin" == Kevin Buettner <kevinb@cygnus.com> writes:
> Kevin> I noticed that.  The space was put there by ``indent''.  I
> Kevin> would very much like to get rid of that space and it would be
> Kevin> easy to make the perl script postprocess the ``indent'' output.
> Kevin> But in doing so, we (obviously) generate different output than
> Kevin> that of ``indent''.
> Kevin>
> Kevin> I suppose the other solution is to fix indent.  :-)
> 
> You can tell indent about all the types defined by typedef with -T
> option, and then it won't add the extra space.  It shouldn't be too
> difficult to identify all the types.
> 
> It might be useful for us to maintain an indent.pro file that has
> these definitions so that additional runs of indent don't add back
> the space.

Given that people are currently editing headers to remove spaces by hand
this sounds like a good idea.  One thing, how does one pass
``indent.pro'' rather than have it pick up ``.indent.pro''?  I suspect
it can be maintained largely by hand (please not another perl script to
maintain it - we can't make perl a requirement on developers :-)

	enjoy,
		Andrew

Re: Proposal: convert function definitions to prototyped form

[J.T. Conklin]

>>>>> "Andrew" == Andrew Cagney <ac131313@cygnus.com> writes:
Andrew> Given that people are currently editing headers to remove
Andrew> spaces by hand this sounds like a good idea.  One thing, how
Andrew> does one pass ``indent.pro'' rather than have it pick up
Andrew> ``.indent.pro''?  I suspect it can be maintained largely by
Andrew> hand (please not another perl script to maintain it - we can't
Andrew> make perl a requirement on developers :-)

For what we want to do, indent needs an option to disable processing
the .indent.pro files located in the current or home directories, and
another to process an arbitrary file.  I examined the latest revision
of the indent sources, 2.2.5, and I can not find an option to process
a file containing options.

It appears that indent is being actively maintained.  Perhaps someone
with a bit of time on their hands can work with the indent maintainer
to get such an option in a future revesion.

        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: Proposal: convert function definitions to prototyped form

[Eric Bachalo]

"J.T. Conklin" wrote:
> 
> >>>>> "Kevin" == Kevin Buettner <kevinb@cygnus.com> writes:
> Kevin> I noticed that.  The space was put there by ``indent''.  I
> Kevin> would very much like to get rid of that space and it would be
> Kevin> easy to make the perl script postprocess the ``indent'' output.
> Kevin> But in doing so, we (obviously) generate different output than
> Kevin> that of ``indent''.
> Kevin>
> Kevin> I suppose the other solution is to fix indent.  :-)
> 
> You can tell indent about all the types defined by typedef with -T
> option, and then it won't add the extra space.  It shouldn't be too
> difficult to identify all the types.
> 
> It might be useful for us to maintain an indent.pro file that has
> these definitions so that additional runs of indent don't add back
> the space.

I believe etags does a fairly good job a finding typedefs.  I would be a
fairly simple script to parse the TAGS file to make a list of all typedefs.  I
have a python script for this.  Not sure how ported etags and python are for
all the hosts that compile GDB though. 

- Eric Bachalo

Re: Proposal: convert function definitions to prototyped form

[Andrew Cagney]

Eric Bachalo wrote:
> 
> "J.T. Conklin" wrote:
> >
> > >>>>> "Kevin" == Kevin Buettner <kevinb@cygnus.com> writes:
> > Kevin> I noticed that.  The space was put there by ``indent''.  I
> > Kevin> would very much like to get rid of that space and it would be
> > Kevin> easy to make the perl script postprocess the ``indent'' output.
> > Kevin> But in doing so, we (obviously) generate different output than
> > Kevin> that of ``indent''.
> > Kevin>
> > Kevin> I suppose the other solution is to fix indent.  :-)
> >
> > You can tell indent about all the types defined by typedef with -T
> > option, and then it won't add the extra space.  It shouldn't be too
> > difficult to identify all the types.
> >
> > It might be useful for us to maintain an indent.pro file that has
> > these definitions so that additional runs of indent don't add back
> > the space.
> 
> I believe etags does a fairly good job a finding typedefs.  I would be a
> fairly simple script to parse the TAGS file to make a list of all typedefs.  I
> have a python script for this.  Not sure how ported etags and python are for
> all the hosts that compile GDB though.

As a guide, check the list in:

http://www.gnu.ai.mit.edu/prep/standards_45.html

	Andrew

Re: Proposal: convert function definitions to prototyped form

[Daniel Berlin]

> > I believe etags does a fairly good job a finding typedefs.  I would be a
> > fairly simple script to parse the TAGS file to make a list of all typedefs.  I
> > have a python script for this.  Not sure how ported etags and python are for
> > all the hosts that compile GDB though.
> 
> As a guide, check the list in:
> 
> http://www.gnu.ai.mit.edu/prep/standards_45.html
> 
> 	Andrew
> 
Funny you guys bring up the number of types in GDB.
According to the debug info, there are >82,000.
How exactly are we counting them, again?

Re: Proposal: convert function definitions to prototyped form

[Stan Shebs]

Daniel Berlin wrote:
> 
> Funny you guys bring up the number of types in GDB.
> According to the debug info, there are >82,000.
> How exactly are we counting them, again?

Since GDB sources are around 400K lines or so, it's highly unlikely
that there is a type definition for every five lines in the
C sources.  You're seeing duplicates in the debug info, presumably.

Stan

Re: Proposal: convert function definitions to prototyped form

[Daniel Berlin]

Before my patches to eliminate duplicate dwarf2 info, it said there were
120k. 
So something isn't adding up here. 
I find it hard to believe we duplicate that much info.
It's pretty much absurd.
--Dan

On Mon, 12 Jun 2000, Stan Shebs wrote:

> Daniel Berlin wrote:
> > 
> > Funny you guys bring up the number of types in GDB.
> > According to the debug info, there are >82,000.
> > How exactly are we counting them, again?
> 
> Since GDB sources are around 400K lines or so, it's highly unlikely
> that there is a type definition for every five lines in the
> C sources.  You're seeing duplicates in the debug info, presumably.
> 
> Stan
> 

Re: Proposal: convert function definitions to prototyped form

[Anatoly Vorobey]

On Mon, Jun 12, 2000 at 06:23:27PM -0700, Daniel Berlin wrote:
> Before my patches to eliminate duplicate dwarf2 info, it said there were
> 120k. 
> So something isn't adding up here. 

How do you eliminate them? The vast majority of types in debug info
are duplicate header file types coming from different source files.
Reducing this should cut the number much more drastically than
from 120k to 80k. I'd say by factor of 8 at the very least. 

My code which reads stabs info says that there are 52276 types (not
trying to remove duplicates) defined
in gdb (not including libbfd, libiberty and libopcodes; including them
gives another 13000 or so) and of them 886 unique type *names*. This
gives a lower bound, obviously, since names may be reused or may denote
different types even coming from the same header due to different defines --
but it's also more useful than the raw number since it doesn't count the
multitude of unnamed automatically generated types which are pointers, 
aliases, forward references, etc.

That would mean around one type name per 400 lines of code. Since almost all
type declarations are in header files which are about 1/10 of all sources
in size, it looks reasonable. 

-- 
Anatoly Vorobey,
mellon@pobox.com http://pobox.com/~mellon/
"Angels can fly because they take themselves lightly" - G.K.Chesterton

Re: Proposal: convert function definitions to prototyped form

[Daniel Berlin]

On Tue, 13 Jun 2000, Anatoly Vorobey wrote:

> On Mon, Jun 12, 2000 at 06:23:27PM -0700, Daniel Berlin wrote:
> > Before my patches to eliminate duplicate dwarf2 info, it said there were
> > 120k. 
> > So something isn't adding up here. 
> 
> How do you eliminate them? The vast majority of types in debug info
> are duplicate header file types coming from different source files.
> Reducing this should cut the number much more drastically than
> from 120k to 80k. I'd say by factor of 8 at the very least. 

Easy.
I have a simple hash table, 4096 entries right now.
before we read a type die, we see if the name of the type is in the hash
table (C++ ODR lets me, nobody with a C program has complained they have
the same type in multiple translation units that isn't really the same
type. I suspect it would break other stuff anyway), if it is, we use the
type in the hash table.
Otherwise, we enter it into the hash table.
If it's in the hash table, but the name doesn't match, i have some code
(not in gdb 5.0, because it's
noisy and only for profiling, obviously) to tell me it's a collision.
No collisions found.

Theoretically, this should remove all duplicate type info being read in if
there aren't any collisions.

So something else is making them, most likely lookup_pointer_type or
thereabouts.

I guess part of the problem is that we can't free types.

But i still think we generate way too many types.

> 
> That would mean around one type name per 400 lines of code. Since almost all
> type declarations are in header files which are about 1/10 of all sources
> in size, it looks reasonable. 
> 
> -- 
> Anatoly Vorobey,
> mellon@pobox.com http://pobox.com/~mellon/
> "Angels can fly because they take themselves lightly" - G.K.Chesterton
> 

Re: Proposal: convert function definitions to prototyped form

[Kevin Buettner]

On Jun 12,  4:30pm, Eric Bachalo wrote:

> > You can tell indent about all the types defined by typedef with -T
> > option, and then it won't add the extra space.  It shouldn't be too
> > difficult to identify all the types.
> > 
> > It might be useful for us to maintain an indent.pro file that has
> > these definitions so that additional runs of indent don't add back
> > the space.
> 
> I believe etags does a fairly good job a finding typedefs.  I would be a
> fairly simple script to parse the TAGS file to make a list of all typedefs.  I
> have a python script for this.  Not sure how ported etags and python are for
> all the hosts that compile GDB though. 

What you suggest would work so long as you also run etags over the
system header files.

For the task at hand, however, the check-decls script has already
identified the necessary types.  (Recall that check-decls is run on
the "diff -u" output after doing a fix-decls.  It constructs a program
which is then compiled with "gcc -Wall" to check the validity of the
protoization performed by fix-decls.  In the process of constructing
the validating program, it must first identify all of the types
requiring typedefs.)

Here is the list of types found by check-decls:

    ADDR32, B_TYPE, COMMON_ENTRY_PTR, CORE_ADDR, CPUSpace, DCACHE,
    DIE_REF, DOUBLEST, EXTR, EventRecord, FDR, FILE, HWND, INSN_WORD,
    INT32, LONG, LONGEST, LPARAM, LRESULT, PDR, PTR, PTRACE_ARG3_TYPE,
    PXDB_header_ptr, Point, Ptrace_return, RDB_EVENT, REGISTER_TYPE,
    RgnHandle, Rptrace, SAVED_BF_PTR, SAVED_F77_COMMON_PTR,
    SAVED_FUNCTION, SYMR, TTRACE_ARG_TYPE, UINT, ULONGEST, WAITTYPE,
    WPARAM, WindowPtr, XDR, YYSTYPE, alpha_extra_func_info_t,
    arg_array, arg_one, arg_type, arg_value, argsin, asection,
    attach_continue_t, bfd, bfd_arch_info_type, bfd_byte,
    bfd_signed_vma, bfd_vma, bool_t, boolean, boolean_t, bpstat,
    branch_type, catch_errors_ftype, catch_fork_kind, cma__t_int_tcb,
    disassemble_info, dld_cache_t, dnttpointer, dst_rec_ptr_t,
    dst_sec, dst_sect_ref_t, dst_type_t, file_ptr, fltset_t,
    fpregset_t, func_call, gdb_client_data, gdb_fpregset_t,
    gdb_gregset_t, gdb_thread_t, gdb_threadref, gregset_t,
    host_callback, insertion_state_t, insn_type, kern_return_t,
    lwpid_t, mach_msg_header_t, mach_msg_id_t, mach_msg_type_name_t,
    mach_port_mscount_t, mach_port_t, memory_page_t, memxferfunc,
    mips_extra_func_info_t, namespace_enum, off_t, pid_t,
    port_chain_t, process_state_t, procinfo, quick_file_entry,
    quick_module_entry, quick_procedure_entry, return_mask,
    rmt_thread_action, sec_ptr, serial_t, serial_ttystate, sigset_t,
    size_t, sltpointer, stepping_mode_t, sysset_t, t_inst, task_t,
    td_err_e, td_thr_state_e, td_thr_type_e, td_thragent_t,
    td_thrhandle_t, thread_array_t, thread_info, thread_t, threadinfo,
    threadref, time_t, tree, ttevents_t, ttreq_t, ttstate_t, ttwopt_t,
    u_long, ui_file_delete_ftype, ui_file_flush_ftype,
    ui_file_fputs_ftype, ui_file_isatty_ftype, ui_file_put_ftype,
    ui_file_rewind_ftype, va_list, value_ptr, and xdrproc_t

Note that there are likely typedefs for types in the gdb sources
missing from the above list, but these were not necessary for doing
the fix-decls conversion.  Also, there are some in the above list
which are defined in system header files.  But that's okay because
indent still needs to know about them in order to do a good job of
formatting.

Re: Proposal: convert function definitions to prototyped form

[Eli Zaretskii]

> Date: Mon, 12 Jun 2000 16:30:44 -0700
> From: Eric Bachalo <ebachalo@redhat.com>
>
> Not sure how ported etags and python are for all the hosts that
> compile GDB though.

What platforms cause your concern?

Re: Proposal: convert function definitions to prototyped form

[Andrew Cagney]

Kevin Buettner wrote:
> 
> As many of you know, I'm in the midst of purging the use of ``PARAMS''
> in prototyped function declarations from the gdb sources.  After this
> activity is concluded, I'd like to begin converting function
> definitions whose parameters are declared using the traditional C
> style to the ISO C prototyped form.  I.e, I'd like to convert
> functions of the form

Something to consider with the timing.  There are Pascal, ObjectiveC.*
and random other Apple files pending.  It might be good to wait until
the buil of the work is in the repository so that we reduce the number
of contributors / maintainers that get hit for six by this one :-)

Unlike params, this one will really hurt people with private diffs.

	Andrew

Re: Proposal: convert function definitions to prototyped form

[Andrew Cagney]

Kevin Buettner wrote:
> 
> As many of you know, I'm in the midst of purging the use of ``PARAMS''
> in prototyped function declarations from the gdb sources.  After this
> activity is concluded, I'd like to begin converting function
> definitions whose parameters are declared using the traditional C
> style to the ISO C prototyped form.  I.e, I'd like to convert
> functions of the form

Kevin,

Would a fair summary of this be that you've encountered the following
road blocks:

	o	indent.pro

		So that indent gives better results


	o	a drain of some of the backlog of
		patches (pascal I believe is now
		going in).

		Apple and HP?

No one has objected to the principal (well not on gdb-patches :-) and
the tool would be based on perl rather than the the gcc thing.

Once those blockages are cleared, it can be scheduled and can go
through?

	Andrew

Re: Proposal: convert function definitions to prototyped form

[Kevin Buettner]

On Jun 13, 11:09am, Andrew Cagney wrote:

> Kevin Buettner wrote:
> > 
> > As many of you know, I'm in the midst of purging the use of ``PARAMS''
> > in prototyped function declarations from the gdb sources.  After this
> > activity is concluded, I'd like to begin converting function
> > definitions whose parameters are declared using the traditional C
> > style to the ISO C prototyped form.  I.e, I'd like to convert
> > functions of the form
> 
> Kevin,
> 
> Would a fair summary of this be that you've encountered the following
> road blocks:
> 
> 	o	indent.pro
> 
> 		So that indent gives better results

I don't think this is a serious road block.  While it would be nice to
be able to specify an indent.pro file (e.g., "indent --options-file
gdbtypes.pro"), it's not absolutely necessary for the task at hand.

As I noted in my reply to Eric, I already know the complete list of
types which indent has to know about in order to complete the
protoization task.  It is easy to put this list in the script which
does the conversion.  I.e,

	@typelist = qw(ADDR32 B_TYPE ... value_ptr xdrproc_t);
	$indentoptions = '-T ' . join(' -T ', @typelist);

> 	o	a drain of some of the backlog of
> 		patches (pascal I believe is now
> 		going in).
> 
> 		Apple and HP?

I think the patch backlog is the real road block.

However, even for this road block, the only reason for delaying the
protoization activity is to make it easier on the people doing the
patch integration.  Perhaps setting a date for the protoization
activity would help motivate the patch integrators to clear some of
the backlog?

> No one has objected to the principal (well not on gdb-patches :-) and
> the tool would be based on perl rather than the the gcc thing.
> 
> Once those blockages are cleared, it can be scheduled and can go
> through?

I still have a little over a week to go on the PARAMS elimination
activity, so any time after then is good for me.  How does midnight
GMT of Sunday July 9 sound?  That's about four weeks away.  Is that
enough time for the patch integrators to clear the patch backlog?