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

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: gdb doesn't work very well with dynamic linked binaries

[James Cownie]

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

There are two slightly separate issues here 
1) If the kernel triggers a watchpoint do you get to see it
2) If the kernel triggers a watchpoint does that break 
   subsequent user level watchpoints.

The ideal behaviour, of course, would be that you saw the watchpoint
whether or not it was triggered by the kernel, and that didn't
break anything. (I believe this is what 2.0 kernels achieved).

The current (2.2.17) Linux behaviour is that 
1) The kernel triggered watchpoint is not reported to the debugger
*AND*
2) After the watchpoint is triggered by the kernel _all_ watchpoints
   in the thread are disabled until it gets rescheduled. (So some
   arbitrary number of user level watchpoint hits can be missed).

My patch would remove the second property, but would still leave
us ignorant about watchpoint hits from the kernel. 

So, with my kernel patch all watchpoint hits from user space are
reported, but watchpoint hits from kernel space are ignored. (Therefore
reading over a watched area doesn't get reported).

The ideal patch would also report kernel watchpoint hits back to the
process. Unfortunately I don't see any "free" way of doing that. 
(By which I mean a way which doesn't add code to the system call
return path whether or not debugging is enabled).

The current bug was introduced in the move from 2.0 to 2.2 because
the system call return path was optimised to remove the restoration
of the debug registers (which is fine as long as the kernel doesn't
change them, unfortunately it does).

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

It all depends on whether these OSes restore the debug registers in
the system call return path. If so changing them in the kernel is 
OK.

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

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

[James Cownie]

This threads seems really to have morphed to be about using the
hardware watchpoint registers on IA32.

So :-

Note that there are a couple of linux kernel bugs in 2.2 (one of which
was fixed in 2.2.17) which affect the use of the debug registers.

The first bug (which was fixed in 2.2.17) was that the debug status
register wasn't being saved where it could be seen by ptrace, so if
you were relying on looking at the hardware status register you were
stuffed.

The second (which isn't yet fixed) I described with a test case in a
previous mail here. ( http://sources.redhat.com/ml/gdb/2000-08/msg00072.html )
Basically the kernel ignores all hardware
watchpoints from the point where one of them is triggered inside the
kernel until a reschedule. This means many watchpoints at user level
can be silently skipped :-(

AFAICS both bugs remain in 2.4.0-test7.

-- Jim 

James Cownie	<jcownie@etnus.com>
Etnus, LLC.     +44 117 9071438
http://www.etnus.com



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

James,

Thanks for reporting these issues to the GDB lists, and sorry for not
getting back to you earlier.

It's unfortunate that these bugs exist in the Linux kernel, especially
if Linux 2.2.17 contains a partial fix, but Linux 2.4 doesn't.
However, I don't think I can be much of a help here.  I'm not very
familiar with the relevant parts of the kernel so I don't feel I can
advocate your patch for you (which doesn't mean that it isn't right,
just that I don't know enough about it).

The only advice I can offer is: keep trying.  If you have a tested
patch for Linux 2.4, send it directly to Linus, and if the patch
doesn't appear in the next (test)-release, send it again.  Repeat this
until the patch has been integrated or has been rejected.

As for the unresolved issues: try sending a message to the
linux-kernel mailing list (linux-kernel@vger.kernel.org) again.  It
might be that the people interested in fixing the problems simply
missed it, or were too busy when you first sent it.

Mark

gdb doesn't work very well with dynamic linked binaries

[H . J . Lu]

On Mon, Sep 04, 2000 at 11:47:13AM +0100, Joern Rennecke wrote:
> > It is too bad that not many gcc developers using --enable-shared to
> > configure gcc under ia32. See
> 
> Well, on Linux, gdb fails to restart a cc1 / cc1plus executable that is
> statically linked, which makes debugging very tedious.
> So I always patch my Makefile to use static linking.

That is a very annoying bug in gdb 5.0. When I set a breakpoint in
the shared library, I have to disable them before restart. Also it is
very hard to debug ld-linux.so.2:

# gdb ld-linux.so.2



H.J.

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

[Mark Kettenis]

   Date: Mon, 4 Sep 2000 08:49:34 -0700
   From: "H . J . Lu" <hjl@lucon.org>

   On Mon, Sep 04, 2000 at 11:47:13AM +0100, Joern Rennecke wrote:
   > > It is too bad that not many gcc developers using --enable-shared to
   > > configure gcc under ia32. See
   > 
   > Well, on Linux, gdb fails to restart a cc1 / cc1plus executable that is
   > statically linked, which makes debugging very tedious.
   > So I always patch my Makefile to use static linking.

   That is a very annoying bug in gdb 5.0. When I set a breakpoint in
   the shared library, I have to disable them before restart.

Getting this bug fixed is one of the release criteria for GDB 5.1.
All I have now is a hack that works around the problems, see the GDB
TODO file for more info.

   Also it is very hard to debug ld-linux.so.2:

   # gdb ld-linux.so.2

AFAIK this never worked.  Patches to make it work are certainly
welcome.  Uli mentioned he submitted some patches, but I cannot find
any trace of them.  Perhaps you re-submit them Uli?

Mark

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

[Daniel Berlin]

> 
> AFAIK this never worked.  Patches to make it work are certainly
> welcome.  Uli mentioned he submitted some patches, but I cannot find
> any trace of them.  Perhaps you re-submit them Uli?

If they were the same ones he sent me, i forwarded them along to various
gdb people, but the consensus was that it didn't actually fix the real
problem.
--Dan
> > Mark
> 

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

[Ulrich Drepper]

Daniel Berlin <dan@cgsoftware.com> writes:

> If they were the same ones he sent me,

They are

> i forwarded them along to various gdb people, but the consensus was
> that it didn't actually fix the real problem.

Well, then fix it correctly.  I'm using the patches for years without
experiencing negative side effects.  Only with them is it possible to
debug ld.so.

-- 
---------------.                          ,-.   1325 Chesapeake Terrace
Ulrich Drepper  \    ,-------------------'   \  Sunnyvale, CA 94089 USA
Red Hat          `--' drepper at redhat.com   `------------------------

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

[Kevin Buettner]

On Sep 4, 11:16am, Ulrich Drepper wrote:

> > i forwarded them along to various gdb people, but the consensus was
> > that it didn't actually fix the real problem.
> 
> Well, then fix it correctly.  I'm using the patches for years without
> experiencing negative side effects.  Only with them is it possible to
> debug ld.so.

I took a look at these patches in late July in the hope that they
would fix some problems that I was seeing (on a not-to-be-named
platform) with relocating the main executable.  Below is a portion of
a message that I sent to one of the internal Red Hat lists concerning
Uli's solib.c patches.  In order to make sense of some of my comments,
it helps to know that I needed the read-only and read/write sections
to be relocated by different amounts.  This isn't terribly relevant
for the discussion at hand, but I think that any solution we come
up with needs to handle this case.  (My comments immediately preceding
Uli's patch *are* relevant though.)

......................................................................

I tried them and they didn't work for me.  There are several problems
with these patches for my situation:

    1)  The exec_bfd isn't marked DYNAMIC.  (But the OS is relocating
	it anyway; according to the ABI, this is okay.) Anyway, since
	it isn't marked DYNAMIC, Uli's code for relocating the symbols
	doesn't get a chance to run.

    2)  The stop_pc when this code is hit is at the _start symbol in
        the dynamic linker.  But I want to relocate the main executable
	whose _start symbol hasn't been hit yet.

    3)  Even if the preceding two problems could be reconciled, the
        .text and .data sections need to be relocated by different
	amounts.

[...]

I'm going to back out Uli's patch from my sandbox.  It didn't build
cleanly in my sandbox, so I'm posting below a cleaned up version which
does build.  We'll need to incorporate something like this into gdb at
some point.  Before we do though, I'd like to understand why the
changes to breakpoint.c are necessary.  Also, we need to consider the
situation where exec_bfd is not the dynamic linker, but is marked
DYNAMIC for some reason.  If this should happen, Uli's code will get
hit and the symbols in exec_bfd / symfile_objfile will get improperly
relocated.

Index: solib.c
===================================================================
RCS file: /cvs/cvsfiles/devo/gdb/solib.c,v
retrieving revision 1.146
diff -u -p -r1.146 solib.c
--- solib.c	2000/05/28 01:25:33	1.146
+++ solib.c	2000/07/26 17:50:36
@@ -54,6 +54,7 @@
 #include "environ.h"
 #include "language.h"
 #include "gdbcmd.h"
+#include "objfiles.h"
 
 #define MAX_PATH_SIZE 512	/* FIXME: Should be dynamic */
 
@@ -1984,6 +1985,39 @@ solib_create_inferior_hook ()
     {
       warning ("shared library handler failed to enable breakpoint");
       return;
+    }
+
+  if ((bfd_get_file_flags (exec_bfd) & DYNAMIC) != 0
+      && bfd_get_start_address (exec_bfd) != stop_pc)
+    {
+      /* We have to relocate the debug information.  */
+      static CORE_ADDR last_displacement;
+      CORE_ADDR displacement = stop_pc - exec_bfd->start_address;
+
+      if (last_displacement != displacement)
+	{
+	  CORE_ADDR correction = displacement - last_displacement;
+	  struct section_offsets *new_offsets;
+	  int i;
+
+	  new_offsets = alloca (symfile_objfile->num_sections
+				* sizeof (*new_offsets));
+
+	  for (i = 0; i < symfile_objfile->num_sections; ++i)
+	    ANOFFSET (new_offsets, i) =
+	      ANOFFSET (symfile_objfile->section_offsets, i);
+
+	  ANOFFSET (new_offsets, SECT_OFF_TEXT (symfile_objfile)) += displacement;
+	  ANOFFSET (new_offsets, SECT_OFF_DATA (symfile_objfile)) += displacement;
+	  ANOFFSET (new_offsets, SECT_OFF_BSS (symfile_objfile)) += displacement;
+	  ANOFFSET (new_offsets, SECT_OFF_RODATA (symfile_objfile)) += displacement;
+
+	  objfile_relocate (symfile_objfile, new_offsets);
+	  breakpoint_re_set ();
+
+	  /* Remember the current displacement.  */
+	  last_displacement = displacement;
+	}
     }
 
 #if !defined(SVR4_SHARED_LIBS) || defined(_SCO_DS)
Index: breakpoint.c
===================================================================
RCS file: /cvs/cvsfiles/devo/gdb/breakpoint.c,v
retrieving revision 1.294
diff -u -p -r1.294 breakpoint.c
--- breakpoint.c	2000/06/04 00:35:16	1.294
+++ breakpoint.c	2000/07/26 17:50:44
@@ -7108,6 +7108,7 @@ breakpoint_re_set_one (bint)
       return 0;
     case bp_breakpoint:
     case bp_hardware_breakpoint:
+    case bp_shlib_event:
     case bp_catch_load:
     case bp_catch_unload:
       if (b->addr_string == NULL)
@@ -7246,10 +7247,6 @@ breakpoint_re_set_one (bint)
 
       /* This breakpoint is special, it's set up when the inferior
          starts and we really don't want to touch it.  */
-    case bp_shlib_event:
-
-      /* Like bp_shlib_event, this breakpoint type is special.
-	 Once it is set up, we do not want to touch it.  */
     case bp_thread_event:
 
       /* Keep temporary breakpoints, which can be encountered when we step

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

[H . J . Lu]

On Mon, Sep 04, 2000 at 07:51:53PM +0200, Mark Kettenis wrote:
>    Date: Mon, 4 Sep 2000 08:49:34 -0700
>    From: "H . J . Lu" <hjl@lucon.org>
> 
>    On Mon, Sep 04, 2000 at 11:47:13AM +0100, Joern Rennecke wrote:
>    > > It is too bad that not many gcc developers using --enable-shared to
>    > > configure gcc under ia32. See
>    > 
>    > Well, on Linux, gdb fails to restart a cc1 / cc1plus executable that is
>    > statically linked, which makes debugging very tedious.
>    > So I always patch my Makefile to use static linking.
> 
>    That is a very annoying bug in gdb 5.0. When I set a breakpoint in
>    the shared library, I have to disable them before restart.
> 
> Getting this bug fixed is one of the release criteria for GDB 5.1.
> All I have now is a hack that works around the problems, see the GDB
> TODO file for more info.
> 

How about the hardware watchpoints on ia32? I mean

1. Delete hardware watchpoints to free hardware debug registers. Set 4
hardware watchpoints. Then delete/disable one hardware watchpoint. Set
another hardware watchpoint. Can gdb free a hardware debug register
when I delete/disable the hardware watchpoint which uses it?
2. Watch for different values on a viariable with one hardware debug
register. That is do

(gdb) watch foobar == 1
(gdb) watch foobar == 2
(gdb) watch foobar == 3
(gdb) watch foobar == 4
(gdb) watch foobar == 5

only using one hardware debug register.

I have reported them long before 5.0 was released. But at least #1
still doesn't work right in 5.0 under Linux/ia32.

Thanks.

H.J.

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

[Eli Zaretskii]

> 
> Date: Mon, 4 Sep 2000 16:44:58 -0700
> From: "H . J . Lu" <hjl@lucon.org>
> 
> 1. Delete hardware watchpoints to free hardware debug registers. Set 4
> hardware watchpoints. Then delete/disable one hardware watchpoint. Set
> another hardware watchpoint. Can gdb free a hardware debug register
> when I delete/disable the hardware watchpoint which uses it?
> 2. Watch for different values on a viariable with one hardware debug
> register. That is do
> 
> (gdb) watch foobar == 1
> (gdb) watch foobar == 2
> (gdb) watch foobar == 3
> (gdb) watch foobar == 4
> (gdb) watch foobar == 5
> 
> only using one hardware debug register.
> 
> I have reported them long before 5.0 was released. But at least #1
> still doesn't work right in 5.0 under Linux/ia32.

These are not GDB/ia32 issues per se: the above features are all
implemented in the DJGPP port of GDB and work in v5.0.  Every
x86-based target should be able to lift the relevant parts of
go32-nat.c and use them almost verbatim.  You get debug register
sharing through reference counts, and the ability to watch large
regions (up to 16 bytes) using multiple registers.  (The required
infrastructure in high-level GDB application code, mostly in
breakpoint.c, is also working since v5.0.)

What is missing is something that we discussed here some time ago: a
unified handling for debug registers common for ALL ia32 targets.  If
you want to get this done before 5.1 is out, I'm for it.  I said in
the past that I'm willing to volunteer to pull the code out of
go32-nat.c and generalize it as appropriate, as the first step towards
this goal.  Provided that it's decided to do that for 5.1, of course
(otherwise, I have too many other important things to do ;-).

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

[H . J . Lu]

On Tue, Sep 05, 2000 at 01:48:56AM -0400, Eli Zaretskii wrote:
> > 
> > Date: Mon, 4 Sep 2000 16:44:58 -0700
> > From: "H . J . Lu" <hjl@lucon.org>
> > 
> > 1. Delete hardware watchpoints to free hardware debug registers. Set 4
> > hardware watchpoints. Then delete/disable one hardware watchpoint. Set
> > another hardware watchpoint. Can gdb free a hardware debug register
> > when I delete/disable the hardware watchpoint which uses it?
> > 2. Watch for different values on a viariable with one hardware debug
> > register. That is do
> > 
> > (gdb) watch foobar == 1
> > (gdb) watch foobar == 2
> > (gdb) watch foobar == 3
> > (gdb) watch foobar == 4
> > (gdb) watch foobar == 5
> > 
> > only using one hardware debug register.
> > 
> > I have reported them long before 5.0 was released. But at least #1
> > still doesn't work right in 5.0 under Linux/ia32.
> 
> These are not GDB/ia32 issues per se: the above features are all
> implemented in the DJGPP port of GDB and work in v5.0.  Every
> x86-based target should be able to lift the relevant parts of
> go32-nat.c and use them almost verbatim.  You get debug register
> sharing through reference counts, and the ability to watch large
> regions (up to 16 bytes) using multiple registers.  (The required
> infrastructure in high-level GDB application code, mostly in
> breakpoint.c, is also working since v5.0.)
> 
> What is missing is something that we discussed here some time ago: a
> unified handling for debug registers common for ALL ia32 targets.  If
> you want to get this done before 5.1 is out, I'm for it.  I said in
> the past that I'm willing to volunteer to pull the code out of
> go32-nat.c and generalize it as appropriate, as the first step towards
> this goal.  Provided that it's decided to do that for 5.1, of course
> (otherwise, I have too many other important things to do ;-).
> 

If you can generalize it for ia32, I will implement it for Linux/ia32.
If it won't be fixed in 5.1, I will follow your hints and implement a
Linux only solution when it happens to me again. It is just one of
those things which makes me to roll my own stuff. Hardware watchpoints
have been known to be broken on Linux/ia32 for a long time and nothing
has been done to it.


H.J.

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

[Eli Zaretskii]

>   Date: Mon, 4 Sep 2000 23:32:22 -0700
>   From: "H . J . Lu" <hjl@lucon.org>
>
>   If you can generalize it for ia32, I will implement it for Linux/ia32.

The code on go32-nat.c manipulates an array which represents the ia32
debug registers, including the status and control registers.  It
leaves it to resume() and its subroutines on the target end to
actually insert the watchpoints when the inferior is resumed and
remove them when the inferior stops and control is passed to GDB.

If this model suits most or all ia32 targets, pulling the code from
go32-nat.c into a separate module (probably, as part of i386-nat.c)
would be very easy for me.  If not, I'd ask the relevant maintainers
to tell what provisions should I make for other platforms to fit in.

>   If it won't be fixed in 5.1, I will follow your hints and implement a
>   Linux only solution when it happens to me again.

I can do this Very Soon (tm) provided that I hear a GO from The Powers
That Be.  Andrew?  Stan?  What say you?

>   Hardware watchpoints
>   have been known to be broken on Linux/ia32 for a long time and nothing
>   has been done to it.

That's not 100% true: a few important patches related to watchpoints
went into mainstream sources (mainly in breakpoint.c) in preparation
for v5.0.  This is the infrastructure I was talking about in my
previous message; without those patches the watchpoint support in
go32-nat.c could not work reliably.  (IIRC, a major part of these
patches were resubmitted by me and approved by Michael Snyder as a
result of a discussion in which you participated.)

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

[Mark Kettenis]

   Date: Tue, 5 Sep 2000 06:35:52 -0400 (EDT)
   From: Eli Zaretskii <eliz@delorie.com>

   >   Date: Mon, 4 Sep 2000 23:32:22 -0700
   >   From: "H . J . Lu" <hjl@lucon.org>
   >
   >   If you can generalize it for ia32, I will implement it for Linux/ia32.

   The code on go32-nat.c manipulates an array which represents the ia32
   debug registers, including the status and control registers.  It
   leaves it to resume() and its subroutines on the target end to
   actually insert the watchpoints when the inferior is resumed and
   remove them when the inferior stops and control is passed to GDB.

   If this model suits most or all ia32 targets, pulling the code from
   go32-nat.c into a separate module (probably, as part of i386-nat.c)
   would be very easy for me.  If not, I'd ask the relevant maintainers
   to tell what provisions should I make for other platforms to fit in.

I looked into the the hardware watchpoint support some time ago, and
actually started coding, seperating the generic bits out of the go32
code.  I was thinking of having some sort of register cache for the
debugging registers but didn't immedeately see the right solution to
do that.  Perhaps they should simply be added to the register cache?
I also couldn't directly see how the hardware watchpoint supports
interacts with multiple threads, especially in presence of the nifty
way the go32 code maps multiple waitchpoints on a single debugging
register.

In principle I see no reason why x86 hardware waitchpoints couldn't be
implemented by i386-tdep.c though.

   >   If it won't be fixed in 5.1, I will follow your hints and implement a
   >   Linux only solution when it happens to me again.

A generic x86 solution would be preferable, but a clean, well
documented Linux-only solution is certainly welcome.

   I can do this Very Soon (tm) provided that I hear a GO from The Powers
   That Be.  Andrew?  Stan?  What say you?

   >   Hardware watchpoints
   >   have been known to be broken on Linux/ia32 for a long time and nothing
   >   has been done to it.

Apparently nobody cares enough.  It isn't at the top of my priority
list so if nobody else contributes the necessary code, it probably
won't happen in the near future.

Mark

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

[Eli Zaretskii]

> Date: Tue, 5 Sep 2000 15:33:54 +0200 (MET DST)
> From: Mark Kettenis <kettenis@wins.uva.nl>
> 
> I was thinking of having some sort of register cache for the
> debugging registers but didn't immedeately see the right solution to
> do that.  Perhaps they should simply be added to the register cache?

I think all we need is to store the debug registers somewhere.  They
need to be accessed by (1) the x86-dependent code which is called by
GDB's application level to insert and remove watchpoints; (2) the
target-specific code which actually calls ptrace or the equivalent
syscall to pass the values into the inferior's registers before
resuming it, and set bits after the inferior stops to indicate which
watchpoint(s) triggered; and (3) by stopped_by_watchpoint's target
end.

It's possible that the register cache is as good place as any to store
DRi, even though they slightly differ from the rest of the registers.

> I also couldn't directly see how the hardware watchpoint supports
> interacts with multiple threads

Sorry, I'm not sure I see the problem.  Could you please elaborate?
(I'm afraid I don't know much about Linux threads.)

> especially in presence of the nifty way the go32 code maps multiple
> waitchpoints on a single debugging register.

I don't think this should matter.  The reference counts are just a
means to know which register is used and which isn't.  As far as the
higher-level GDB code is concerned, what's important is (a) which
address, if any, triggered a watchpoint; and (b) which thread caused
the watchpoint to trigger.

However, if I'm missing the point, and there's some additional
infrastructure that is required to make this work with multiple
threads, please tell what is, or might be, missing.

> Apparently nobody cares enough.  It isn't at the top of my priority
> list so if nobody else contributes the necessary code, it probably
> won't happen in the near future.

If v5.1 freeze date is far away, and if the current code in go32-nat.c
is good enough to be used by other x86 platforms, I might take the
silence as a sign of agreement, given the fact that making it happen
is easy... ;-)

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

[Mark Kettenis]

   Date: Tue, 5 Sep 2000 11:46:48 -0400 (EDT)
   From: Eli Zaretskii <eliz@delorie.com>

   > Date: Tue, 5 Sep 2000 15:33:54 +0200 (MET DST)
   > From: Mark Kettenis <kettenis@wins.uva.nl>
   > 
   > I was thinking of having some sort of register cache for the
   > debugging registers but didn't immedeately see the right solution to
   > do that.  Perhaps they should simply be added to the register cache?

   I think all we need is to store the debug registers somewhere.  They
   need to be accessed by (1) the x86-dependent code which is called by
   GDB's application level to insert and remove watchpoints; (2) the
   target-specific code which actually calls ptrace or the equivalent
   syscall to pass the values into the inferior's registers before
   resuming it, and set bits after the inferior stops to indicate which
   watchpoint(s) triggered; and (3) by stopped_by_watchpoint's target
   end.

   It's possible that the register cache is as good place as any to store
   DRi, even though they slightly differ from the rest of the registers.

Yep, and that's where I got distracted by more urgent problems :-(.

   > I also couldn't directly see how the hardware watchpoint supports
   > interacts with multiple threads

   Sorry, I'm not sure I see the problem.  Could you please elaborate?
   (I'm afraid I don't know much about Linux threads.)

Me neither :-(.  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.

   > especially in presence of the nifty way the go32 code maps multiple
   > waitchpoints on a single debugging register.

   I don't think this should matter.  The reference counts are just a
   means to know which register is used and which isn't.  As far as the
   higher-level GDB code is concerned, what's important is (a) which
   address, if any, triggered a watchpoint; and (b) which thread caused
   the watchpoint to trigger.

If you can set the debug registers per-thread, we might need a
reference count for every thread.  If the debug registers are
per-VM-space there might be a potential problem with finding out the
right triggering address for the right thread.

   However, if I'm missing the point, and there's some additional
   infrastructure that is required to make this work with multiple
   threads, please tell what is, or might be, missing.

It's something that needs to be investigated.  But since DJGPP doesn't
seem to support multiple threads I certainly don't expect you to do
that :-).

Mark

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

[Eli Zaretskii]

> Date: Wed, 6 Sep 2000 02:06:50 +0200
> From: Mark Kettenis <kettenis@wins.uva.nl>
> 
> If you can set the debug registers per-thread, we might need a
> reference count for every thread.

Oh, I see.  I assumed that the debug registers are per VM; I should
have known better.

In that case, adding the debug registers to the register cache is
probably the way to go (since the other registers are per thread,
right?).  And the functions which manipulate the debug registers
should accept an array of registers as their argument--this should
allow any specific target to DTRT easily.

Does this sound as a good plan?

> But since DJGPP doesn't seem to support multiple threads I certainly
> don't expect you to do that :-).

Threading is possible with DJGPP, but the DJGPP port of GDB doesn't
have any special support for debugging such programs.

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

[H . J . Lu]

On Tue, Sep 05, 2000 at 03:33:54PM +0200, Mark Kettenis wrote:
> 
>    >   If it won't be fixed in 5.1, I will follow your hints and implement a
>    >   Linux only solution when it happens to me again.
> 
> A generic x86 solution would be preferable, but a clean, well
> documented Linux-only solution is certainly welcome.

Given do it clean, do it fast and make it to work, you can only
pick 2 if I am the only one to do it. Since I don't have much time
to do it, I have to pick do it fast.

> 
>    I can do this Very Soon (tm) provided that I hear a GO from The Powers
>    That Be.  Andrew?  Stan?  What say you?
> 
>    >   Hardware watchpoints
>    >   have been known to be broken on Linux/ia32 for a long time and nothing
>    >   has been done to it.
> 
> Apparently nobody cares enough.  It isn't at the top of my priority
> list so if nobody else contributes the necessary code, it probably
> won't happen in the near future.

It has to be fixed in 5.1, one way or the other. The worst case is
I will make my kludge available to the Linux community.


H.J.

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

[Stan Shebs]

"H . J . Lu" wrote:
> 
> On Tue, Sep 05, 2000 at 03:33:54PM +0200, Mark Kettenis wrote:
> >
> >    >   If it won't be fixed in 5.1, I will follow your hints and implement a
> >    >   Linux only solution when it happens to me again.
> >
> > A generic x86 solution would be preferable, but a clean, well
> > documented Linux-only solution is certainly welcome.
> 
> Given do it clean, do it fast and make it to work, you can only
> pick 2 if I am the only one to do it. Since I don't have much time
> to do it, I have to pick do it fast.

I feel like I should say something here - doing things fast for GNU/Linux
has been a continuing source of problems.  All this stuff is going
to take the same amount of time in the end, whether we kludge now and
rewrite over and over later, or do it right the first time.  GDB Linux
thread hacking has been going on for a couple years, and it's still not
quite done; if I had known it was going to take this long, I would
have taken a harder line about accepting the original expedient version.

If you don't think that you're getting enough time to do things right,
then you should take it up with your management.  Since VA Linux thinks
highly enough of you to feature your picture prominently in their Linux
World booth :-), you should have enough pull to say "this is how long
it needs to take".  If you supply me with names, I will be happy to
take it up with them myself too - VA Linux' business depends heavily on
its reputation for good Linux engineering, and I doubt they want to
become known as the company that is pushing hacky forked versions of GDB
out into the world.

Stan

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

[H . J . Lu]

On Tue, Sep 05, 2000 at 06:23:31PM -0700, Stan Shebs wrote:
> 
> If you don't think that you're getting enough time to do things right,
> then you should take it up with your management.  Since VA Linux thinks

The gdb work has nothing to do with my job at VA :-). I don't even
know they are aware of it. I only work on gdb because I don't want to
wait for a few hours when debugging the code nor start all over when
I use up all hardware registers. If it is a must-fix for gdb 5.1, I
am willing to pitch in. If noone else wants to spend time on it, I
will come up with some kludge for myself.

BTW, it has been a long standing problem for gdb. It seems that I
am the only one who thinks gdb sucks under Linux/ia32 and is willing
to do something about it.


H.J.

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

[Eli Zaretskii]

> Date: Tue, 5 Sep 2000 18:33:12 -0700
> From: "H . J . Lu" <hjl@lucon.org>
>
> I only work on gdb because I don't want to wait for a few hours when
> debugging the code nor start all over when I use up all hardware
> registers.

Same here.  I debug development versions of Emacs as a matter of
routine, and find that to be impossible in many ``interesting'' cases
without watchpoints.  That was my motivation to fix them, including
the ability to watch struct members and array elements.

> If it is a must-fix for gdb 5.1, I am willing to pitch in. If noone
> else wants to spend time on it, I will come up with some kludge for
> myself.

I will try to pull the relevant code out of go32-nat.c and put it on
i386-tdep.c in the next few weeks.

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

[H . J . Lu]

On Tue, Sep 05, 2000 at 06:35:52AM -0400, Eli Zaretskii wrote:
> >   Date: Mon, 4 Sep 2000 23:32:22 -0700
> >   From: "H . J . Lu" <hjl@lucon.org>
> >
> >   If you can generalize it for ia32, I will implement it for Linux/ia32.
> 
> The code on go32-nat.c manipulates an array which represents the ia32
> debug registers, including the status and control registers.  It
> leaves it to resume() and its subroutines on the target end to
> actually insert the watchpoints when the inferior is resumed and
> remove them when the inferior stops and control is passed to GDB.
> 
> If this model suits most or all ia32 targets, pulling the code from
> go32-nat.c into a separate module (probably, as part of i386-nat.c)
> would be very easy for me.  If not, I'd ask the relevant maintainers
> to tell what provisions should I make for other platforms to fit in.
> 
> >   If it won't be fixed in 5.1, I will follow your hints and implement a
> >   Linux only solution when it happens to me again.
> 
> I can do this Very Soon (tm) provided that I hear a GO from The Powers
> That Be.  Andrew?  Stan?  What say you?

I will vote for fixing it in 5.1. I will do the Linux work since I
brought it up. It has been broken for too long. There is no excuse
not to fix it given that at least 2 people are willing to do it.

> 
> >   Hardware watchpoints
> >   have been known to be broken on Linux/ia32 for a long time and nothing
> >   has been done to it.
> 
> That's not 100% true: a few important patches related to watchpoints
> went into mainstream sources (mainly in breakpoint.c) in preparation

Sorry for the misunderstanding. Thanks for your work on breakpoints.
It is just that hardware watchpoints under Linux/ia32 are as broken
as before :-).


H.J.

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

[Stan Shebs]

Eli Zaretskii wrote:
> 
> >   Date: Mon, 4 Sep 2000 23:32:22 -0700
> >   From: "H . J . Lu" <hjl@lucon.org>
> >
> >   If you can generalize it for ia32, I will implement it for Linux/ia32.
> 
> The code on go32-nat.c manipulates an array which represents the ia32
> debug registers, including the status and control registers.  It
> leaves it to resume() and its subroutines on the target end to
> actually insert the watchpoints when the inferior is resumed and
> remove them when the inferior stops and control is passed to GDB.
> 
> If this model suits most or all ia32 targets, pulling the code from
> go32-nat.c into a separate module (probably, as part of i386-nat.c)
> would be very easy for me.  If not, I'd ask the relevant maintainers
> to tell what provisions should I make for other platforms to fit in.
> 
> >   If it won't be fixed in 5.1, I will follow your hints and implement a
> >   Linux only solution when it happens to me again.
> 
> I can do this Very Soon (tm) provided that I hear a GO from The Powers
> That Be.  Andrew?  Stan?  What say you?

Uh, is there any reason not to?  We tell people that GDB can support
h/w watchpoints, seems like we ought to deliver it on our most popular
platforms.  Perhaps I could be evil and insist on adding a testsuite test
that would take 24 hours to run if h/w watchpoints don't work... think
that would help motivate anyone? :-)

Stan

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

[H . J . Lu]

On Tue, Sep 05, 2000 at 06:01:39PM -0700, Stan Shebs wrote:
> > 
> > I can do this Very Soon (tm) provided that I hear a GO from The Powers
> > That Be.  Andrew?  Stan?  What say you?
> 
> Uh, is there any reason not to?  We tell people that GDB can support
> h/w watchpoints, seems like we ought to deliver it on our most popular
> platforms.  Perhaps I could be evil and insist on adding a testsuite test
> that would take 24 hours to run if h/w watchpoints don't work... think
> that would help motivate anyone? :-)
> 

How about this patch? As I said, I will work on Linux since I use this
feature a lot.

Thanks.


H.J.
----
2000-09-05  H.J. Lu  <hjl@gnu.org>

	* TODO: Add hardware watchpint problems on x86 OSes for 5.1.

--- TODO	Thu Aug 10 15:23:13 2000
+++ /tmp/TODO	Tue Sep  5 20:41:08 2000
@@ -12,6 +12,26 @@ cycle.  People hope to have these proble
 
 --
 
+Hardware watchpint problems on x86 OSes, including Linux:
+
+1. Delete/disable hardware watchpoints should free hardware debug
+registers. 
+2. Watch for different values on a viariable with one hardware debug
+register.
+
+According to Eli Zaretskii <eliz@delorie.com>:
+
+These are not GDB/ia32 issues per se: the above features are all
+implemented in the DJGPP port of GDB and work in v5.0.  Every
+x86-based target should be able to lift the relevant parts of
+go32-nat.c and use them almost verbatim.  You get debug register
+sharing through reference counts, and the ability to watch large
+regions (up to 16 bytes) using multiple registers.  (The required
+infrastructure in high-level GDB application code, mostly in
+breakpoint.c, is also working since v5.0.)
+
+--
+
 RFD: infrun.c: No bpstat_stop_status call after proceed over break?
 http://sourceware.cygnus.com/ml/gdb-patches/2000-q1/msg00665.html
 

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

[Eli Zaretskii]

> Date: Tue, 5 Sep 2000 20:45:19 -0700
> From: "H . J . Lu" <hjl@lucon.org>
> 
> How about this patch? As I said, I will work on Linux since I use this
> feature a lot.
> 
> Thanks.
> 
> 
> H.J.
> ----
> 2000-09-05  H.J. Lu  <hjl@gnu.org>
> 
> 	* TODO: Add hardware watchpint problems on x86 OSes for 5.1.

Fine with me.

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

[Andrew Cagney]

FYI,

I'm going to apply this patch but as part of the 5.2 criteria.  If it is
fixed in 5.1 then it is a bonus.

	enjoy,
		Andrew


"H . J . Lu" wrote:
> 
> On Tue, Sep 05, 2000 at 06:01:39PM -0700, Stan Shebs wrote:
> > >
> > > I can do this Very Soon (tm) provided that I hear a GO from The Powers
> > > That Be.  Andrew?  Stan?  What say you?
> >
> > Uh, is there any reason not to?  We tell people that GDB can support
> > h/w watchpoints, seems like we ought to deliver it on our most popular
> > platforms.  Perhaps I could be evil and insist on adding a testsuite test
> > that would take 24 hours to run if h/w watchpoints don't work... think
> > that would help motivate anyone? :-)
> >
> 
> How about this patch? As I said, I will work on Linux since I use this
> feature a lot.
> 
> Thanks.
> 
> H.J.
> ----
> 2000-09-05  H.J. Lu  <hjl@gnu.org>
> 
>         * TODO: Add hardware watchpint problems on x86 OSes for 5.1.
> 
> --- TODO        Thu Aug 10 15:23:13 2000
> +++ /tmp/TODO   Tue Sep  5 20:41:08 2000
> @@ -12,6 +12,26 @@ cycle.  People hope to have these proble
> 
>  --
> 
> +Hardware watchpint problems on x86 OSes, including Linux:
> +
> +1. Delete/disable hardware watchpoints should free hardware debug
> +registers.
> +2. Watch for different values on a viariable with one hardware debug
> +register.
> +
> +According to Eli Zaretskii <eliz@delorie.com>:
> +
> +These are not GDB/ia32 issues per se: the above features are all
> +implemented in the DJGPP port of GDB and work in v5.0.  Every
> +x86-based target should be able to lift the relevant parts of
> +go32-nat.c and use them almost verbatim.  You get debug register
> +sharing through reference counts, and the ability to watch large
> +regions (up to 16 bytes) using multiple registers.  (The required
> +infrastructure in high-level GDB application code, mostly in
> +breakpoint.c, is also working since v5.0.)
> +
> +--
> +
>  RFD: infrun.c: No bpstat_stop_status call after proceed over break?
>  http://sourceware.cygnus.com/ml/gdb-patches/2000-q1/msg00665.html
>

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

[Eli Zaretskii]

> Date: Tue, 05 Sep 2000 18:01:39 -0700
> From: Stan Shebs <shebs@apple.com>
> > 
> > I can do this Very Soon (tm) provided that I hear a GO from The Powers
> > That Be.  Andrew?  Stan?  What say you?
> 
> Uh, is there any reason not to?  We tell people that GDB can support
> h/w watchpoints, seems like we ought to deliver it on our most popular
> platforms.

Agreed.  I think that having watchpoint support in good shape is
important, even if initially it won't work well with multithreaded
programs.

> Perhaps I could be evil and insist on adding a testsuite test
> that would take 24 hours to run if h/w watchpoints don't work... think
> that would help motivate anyone? :-)

I think I now have all the motivation I need ;-)

I'll be away for about two weeks starting this weekend, but I'll try
to do my part of this soon after I return.

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