Runtiming hw watchpoints; Was: Split up Z packet enable/disable cmd

Runtiming hw watchpoints; Was: Split up Z packet enable/disable cmd

[Andrew Cagney]

(I'm going to think out loud...)

> 
> There are 3 new target vector entries:
> to_Z_packet_supported_p
> to_insert_watchpoint
> to_remove_watchpoint

From memory watchpoints can be implemented at two levels:

	o	GDB directly pokes watchpoint
		registers

	o	GDB asks the target to take
		care of the problem

First thing to keep in mind is that everytime this is pointed out the
discussion goes off on a religious tangent where people argue how one or
the other is the only correct solution ...  I think they both are
equally correct :-)

Something less target specific than to_Z_packet_supported_p() is going
to be needed :-)

> > PS: It doesn't fix the compile time test for remote watchpoints but it
> > does enable the code always (so that I've a better chance of not
> > breaking it :-).
> >
> Well, my patch takes care of that part. This is a good opportunity to ask what
> people think of my solution.  It is working on a beta version and a candidate
> for being submitted as a complement to Andrews patch.
> 
> I have set TARGET_CAN_USE_HARDWARE_WATCHPOINT to a function which will call
> target_Z_packet_supported_p().  remote_Z_packet_supported_p() sends a "probe"
> packet if the support is set to auto and we haven't asserted if the target
> accepts or not the Z packet (OK, I will have to make it Z4 or something).
> 
> If the Z-packet support is in auto and we have not connected to the target yet,
> we cannot access if watchpoints are allowed (if it is set to enabled or disabled
> we assume the user knows).  In this case I print:
> "Cannot set watchpoints before connecting to remote target: Z-packet support unknown."
> 
> That is the only real restriction.  As I don't meddle at all with the breakpoint.c
> code it results in a very simple fix.

I don't think this is reasonable.  I think a better model would involve:

	(gdb) watch c
	Watchpoint 1: c

	(gdb) watch b
	Watchpoint 2: b

	(gdb) target xxx

	(gdb) try-watchpoints-for-size OR run
	Watchpoint 1 hardware
	Watchpoint 2 doesn't fit

	(gdb) run

I.E. delay assigning watchpoints to hardware/software until the last
moment.  Instead of having watch_command_1()  try to select a hw/sw
watchpoint before it knows the answer, the code inserting the watchpoint
could do it and just set ->hw if it succeeded.

From memory, GDB refuses to mix hardware and software watchpoints which
should greatly simplify the implementation.

	Andrew
From eliz@delorie.com Wed Aug 02 23:02:00 2000
From: Eli Zaretskii <eliz@delorie.com>
To: ac131313@cygnus.com
Cc: fnasser@cygnus.com, gdb-patches@sourceware.cygnus.com
Subject: Re: Runtiming hw watchpoints; Was: Split up Z packet enable/disable cmd
Date: Wed, 02 Aug 2000 23:02:00 -0000
Message-id: <200008030602.CAA26491@indy.delorie.com>
References: <3987DD9A.21F2B29A@cygnus.com> <39884DFA.12DD4043@cygnus.com> <3988E54B.39646794@cygnus.com>
X-SW-Source: 2000-08/msg00060.html
Content-length: 1697

> Date: Thu, 03 Aug 2000 13:21:47 +1000
> From: Andrew Cagney <ac131313@cygnus.com>
> 
> I.E. delay assigning watchpoints to hardware/software until the last
> moment.  Instead of having watch_command_1()  try to select a hw/sw
> watchpoint before it knows the answer, the code inserting the watchpoint
> could do it and just set ->hw if it succeeded.

Yes, I think this is the only reasonable way, eventually.

The problem is that the code which is run when the user says "watch
foo" doesn't know how many other watchpoints, and of what kind, will
be needed when the inferior is resumed.  In particular, all kinds of
breakpoints with commands that set, reset, or enable watchpoints may
be lying around; a target may be able to combine several watchpoints
into one, but only under some conditions; etc.  You don't know all
that stuff until it's time to resume, and only the target code knows
the entire truth.

Automatic fallback to software watchpoints might be useful, but IMHO
it must be a user option, because in many cases, if I know that my
resources for hardware watchpoints are not enough to cover all of the
watches, I might wish to reconsider instead of letting the program to
crawl...

However, with the current machinery, it is very hard to delay the
hw/sw decision to the point where the inferior is resumed.  This is
because high-level code in GDB needs to know whether there are any
software watchpoints, to arrange for single-stepping.  Since the
decision can only be reliably made by the target-specific code, by the
time it knows that, it's too late.  We need some way of communicating
that information back to GDB's application code, before resume() and
its brethren is called.
From ac131313@cygnus.com Thu Aug 03 01:47:00 2000
From: Andrew Cagney <ac131313@cygnus.com>
To: GDB Patches <gdb-patches@sourceware.cygnus.com>
Subject: [patch] remote-mips.c param fixes
Date: Thu, 03 Aug 2000 01:47:00 -0000
Message-id: <398931B2.1FD60AA2@cygnus.com>
X-SW-Source: 2000-08/msg00061.html
Content-length: 2086

FYI,

More fallout from the K&R->ISO conversion.

	Andrew
Thu Aug  3 15:02:23 2000  Andrew Cagney  <cagney@b1.cygnus.com>

	* remote-mips.c (mips_expect, mips_expect_timeout, common_open,
 	fputs_readable): Make string pointer arguments constant.

Index: remote-mips.c
===================================================================
RCS file: /cvs/cvsfiles/devo/gdb/remote-mips.c,v
retrieving revision 2.105
diff -p -r2.105 remote-mips.c
*** remote-mips.c	2000/07/30 01:49:39	2.105
--- remote-mips.c	2000/08/03 07:56:52
*************** fputc_readable (int ch, struct ui_file *
*** 526,532 ****
     ^x notation or in hex.  */
  
  static void
! fputs_readable (char *string, struct ui_file *file)
  {
    int c;
  
--- 526,532 ----
     ^x notation or in hex.  */
  
  static void
! fputs_readable (const char *string, struct ui_file *file)
  {
    int c;
  
*************** fputs_readable (char *string, struct ui_
*** 540,548 ****
   */
  
  int
! mips_expect_timeout (char *string, int timeout)
  {
!   char *p = string;
  
    if (remote_debug)
      {
--- 540,548 ----
   */
  
  int
! mips_expect_timeout (const char *string, int timeout)
  {
!   const char *p = string;
  
    if (remote_debug)
      {
*************** mips_expect_timeout (char *string, int t
*** 596,602 ****
   */
  
  int
! mips_expect (char *string)
  {
    return mips_expect_timeout (string, 2);
  }
--- 596,602 ----
   */
  
  int
! mips_expect (const char *string)
  {
    return mips_expect_timeout (string, 2);
  }
*************** mips_initialize (void)
*** 1499,1505 ****
  /* Open a connection to the remote board.  */
  static void
  common_open (struct target_ops *ops, char *name, int from_tty,
! 	     enum mips_monitor_type new_monitor, char *new_monitor_prompt)
  {
    char *ptype;
    char *serial_port_name;
--- 1499,1506 ----
  /* Open a connection to the remote board.  */
  static void
  common_open (struct target_ops *ops, char *name, int from_tty,
! 	     enum mips_monitor_type new_monitor,
! 	     const char *new_monitor_prompt)
  {
    char *ptype;
    char *serial_port_name;
From taylor@cygnus.com Thu Aug 03 07:45:00 2000
From: David Taylor <taylor@cygnus.com>
To: Jimmy Guo <guo@cup.hp.com>
Cc: gdb-patches@sourceware.cygnus.com
Subject: Re: [PATCH] language support: case sensitivity 
Date: Thu, 03 Aug 2000 07:45:00 -0000
Message-id: <200008031433.KAA04437@texas.cygnus.com>
References: <Pine.LNX.4.10.10008020947550.12072-100000@hpcll168.cup.hp.com>
X-SW-Source: 2000-08/msg00062.html
Content-length: 2019

    Date: Wed, 2 Aug 2000 09:55:23 -0700 (PDT)
    From: Jimmy Guo <guo@cup.hp.com>

    I don't quite understand yet the need for having a new show command
    routine ... command.c (do_setshow_command) seems to be the one that
    handle the normal output of settings.  Also, show_language_command
    has similar setup as show_case_command ().  Do you have an example on
    the kind of behavior you'd like to see but not supported currently by
    this patch?

    - Jimmy Guo, guo@cup.hp.com

You added two commands:

  set case-sensitivity
  show case-sensitivity

And if the user types "show case-sensitivity", nothing will print
unless the sensitivity differs from that of the current language.  I
feel that if the user *ASKED* to be shown the case sensitivity, then
the user should be *SHOWN* the case sensitivity.  If the user
explicitily asks what it is, then it should not be silent.

So, you could have the command "show case-sensitivity" print something like:

  Case sensitivity is on.
  warning: current language, fortran, is case insensitive.

Where the first line is printed by a new function (to be called when
the user types "show case-sensitivity"), and the second line is
printed by the function (renamed) that currently implements "show
case-sensitivity".

[...]
    >Your new function show_case_command plays double duty -- it is both
    >invoked by other functions / commands and it is invoked by the user in
    >response to the 'show case' command.  And while it is quite reasonable
    >for it to be silent when it is *NOT* invoked by the user, it should
    >not be silent when it is invoked by the user.
    >
    >My suggestion is to define two functions:
    >
    >. one, the new show_case_command, which is never silent -- it always
    >tells you the setting.
    >
    >. the other, the current show_case_command, prints a warning if
    >appropriate and is called by the current callers of show_case_command.
    >
    >Otherwise it looks fine to me.  Thanks for submitting this.

Re: Runtiming hw watchpoints; Was: Split up Z packet enable/disable cmd

[Fernando Nasser]

Thanks, that is a very good summary.

Let me comment...


Eli Zaretskii wrote:
> 
> > Date: Thu, 03 Aug 2000 13:21:47 +1000
> > From: Andrew Cagney <ac131313@cygnus.com>
> >
> > I.E. delay assigning watchpoints to hardware/software until the last
> > moment.  Instead of having watch_command_1()  try to select a hw/sw
> > watchpoint before it knows the answer, the code inserting the watchpoint
> > could do it and just set ->hw if it succeeded.
> 
> Yes, I think this is the only reasonable way, eventually.
> 

Delaying the decision for watchpoints is hard because the software implementation
is something not atomic, much different from the sw breakpoint which is basically
alter a memory position.  The software implementation of watchpoints includes
an arbitrary number of single-steps and processing on inferior event.

Note that once the user realizes his/her target do have hardware support for 
watchpoints he/she can either:
1) Use target first, before inserting watchpoints
2) Add "set harware-watchpoints enable" to her/his .gdbinit file

Nothing that the manual or a good error message text can't fix.


> The problem is that the code which is run when the user says "watch
> foo" doesn't know how many other watchpoints, and of what kind, will
> be needed when the inferior is resumed.  In particular, all kinds of
> breakpoints with commands that set, reset, or enable watchpoints may
> be lying around; a target may be able to combine several watchpoints
> into one, but only under some conditions; etc.  You don't know all
> that stuff until it's time to resume, and only the target code knows
> the entire truth.
> 

The code in breakpoints does collect the necessary information and pass it
as arguments to the macro.  We just have to write the appropriate code for
each architecture.  The check is part architecture dependent and part target
dependent.  My implementation allow for both checks.

The macro (is it already at gdbarch?  If not it should) invokes the architecture
dependent code (in i386-tdep.c in my case) and that uses the target vector above
to check the target dependent parts.

I believe we always should go through these two layers, in that order, for things
that are both arch and target dependent.


> Automatic fallback to software watchpoints might be useful, but IMHO
> it must be a user option, because in many cases, if I know that my
> resources for hardware watchpoints are not enough to cover all of the
> watches, I might wish to reconsider instead of letting the program to
> crawl...
> 

set hardware-watchpoints disable   (the default will be auto)

> However, with the current machinery, it is very hard to delay the
> hw/sw decision to the point where the inferior is resumed.  This is
> because high-level code in GDB needs to know whether there are any
> software watchpoints, to arrange for single-stepping.  Since the
> decision can only be reliably made by the target-specific code, by the
> time it knows that, it's too late.  We need some way of communicating
> that information back to GDB's application code, before resume() and
> its brethren is called.

Exactly.  Unless we want the watchpoint support to remain in the TODO list
for a very long time we must set for a initial solution that does not require
a huge amount of code rewriting.  In this case I would bet it would need a
major architectural change.




-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@cygnus.com
2323 Yonge Street, Suite #300           Tel:  416-482-2661 ext. 311
Toronto, Ontario   M4P 2C9              Fax:  416-482-6299
From guo@cup.hp.com Thu Aug 03 10:03:00 2000
From: Jimmy Guo <guo@cup.hp.com>
To: David Taylor <taylor@cygnus.com>
Cc: gdb-patches@sourceware.cygnus.com
Subject: Re: [PATCH] language support: case sensitivity 
Date: Thu, 03 Aug 2000 10:03:00 -0000
Message-id: <Pine.LNX.4.10.10008030953070.17962-100000@hpcll168.cup.hp.com>
References: <200008031433.KAA04437@texas.cygnus.com>
X-SW-Source: 2000-08/msg00064.html
Content-length: 3031

Actually the two commands added are:
	 set case-sensitive (on/off/auto)
	 show case-sensitive

It's not correct that nothing would be print if case sensitivity is set
same as the language default and if the user issues the show command.
As I said, do_setshow_command () in command.c handles that part.
Looking at the 'set/show language' implementation, there's nothing
different between the two -- one would expect that if this patch needs a
different command to support user query, then show_language would too.

Just to show this, here's what I got from GDB on a C application:

(gdb) show case-sensitive
Case sensitivity in name search is "auto; currently on".
(gdb) set case-sensitive on
(gdb) show case-sensitive
Case sensitivity in name search is "on".
(gdb) show language
The current source language is "auto; currently c".
(gdb) set language c
(gdb) show language
The current source language is "c".

- Jimmy

On Thu, 3 Aug 2000, David Taylor wrote:

>    Date: Wed, 2 Aug 2000 09:55:23 -0700 (PDT)
>    From: Jimmy Guo <guo@cup.hp.com>
>
>    I don't quite understand yet the need for having a new show command
>    routine ... command.c (do_setshow_command) seems to be the one that
>    handle the normal output of settings.  Also, show_language_command
>    has similar setup as show_case_command ().  Do you have an example on
>    the kind of behavior you'd like to see but not supported currently by
>    this patch?
>
>    - Jimmy Guo, guo@cup.hp.com
>
>You added two commands:
>
>  set case-sensitivity
>  show case-sensitivity
>
>And if the user types "show case-sensitivity", nothing will print
>unless the sensitivity differs from that of the current language.  I
>feel that if the user *ASKED* to be shown the case sensitivity, then
>the user should be *SHOWN* the case sensitivity.  If the user
>explicitily asks what it is, then it should not be silent.
>
>So, you could have the command "show case-sensitivity" print something like:
>
>  Case sensitivity is on.
>  warning: current language, fortran, is case insensitive.
>
>Where the first line is printed by a new function (to be called when
>the user types "show case-sensitivity"), and the second line is
>printed by the function (renamed) that currently implements "show
>case-sensitivity".
>
>[...]
>    >Your new function show_case_command plays double duty -- it is both
>    >invoked by other functions / commands and it is invoked by the user in
>    >response to the 'show case' command.  And while it is quite reasonable
>    >for it to be silent when it is *NOT* invoked by the user, it should
>    >not be silent when it is invoked by the user.
>    >
>    >My suggestion is to define two functions:
>    >
>    >. one, the new show_case_command, which is never silent -- it always
>    >tells you the setting.
>    >
>    >. the other, the current show_case_command, prints a warning if
>    >appropriate and is called by the current callers of show_case_command.
>    >
>    >Otherwise it looks fine to me.  Thanks for submitting this.
>
>

Re: Runtiming hw watchpoints; Was: Split up Z packet enable/disable cmd

[Eli Zaretskii]

> Date: Fri, 04 Aug 2000 13:51:05 +0000
> From: Fernando Nasser <fnasser@cygnus.com>
> 
> > >       i = hw_watchpoint_used_count (bp_type, &other_type_used);
> > >       target_resources_ok =
> > > !       TARGET_CAN_USE_HARDWARE_WATCHPOINT (bp_type, j, i + mem_cnt,
> > >                                             other_type_used);
> > 
> > The problem here is that the mere _count_ of the used up break- and
> > watch-points is not enough to return an accurate answer to the
> > question asked by TARGET_CAN_USE_HARDWARE_WATCHPOINT.  At least in the
> > DJGPP case (and I believe for every other x86 target, too), I need to
> > know the addresses and the size of watched regions of all watchpoints,
> > and also the addresses of all hardware-assisted breakpoints.  If
> > TARGET_CAN_USE_HARDWARE_WATCHPOINT will get that info, it could be
> > implemented so as to return a meaningful result.
> 
> That is taken care of in breakpoints.c also.  The function can_use_hardware_watchpoint()
> checks how many hardware registers will be necessary for implementing a watchpoint.
> It may need fix or parametrization, but it is in there.

can_use_hardware_watchpoint could do this if it gets the info about all
the watchpoints and hardware breakpoints defined previously.
Currently, it only gets information about one single watchpoint at a
time, and that is not enough, at least on x86.

Alternatively, we could request the target to remember all watchpoints
that GDB intends to insert when it resumes the debuggee, but that
calls for a change in the API, since currently
can_use_hardware_watchpoint does not guarantee the watchpoint will
actually be inserted.

> You see, nobody even know that these things are there because the macros/functions to 
> do the actual check for most targets did not get implemented.

I completely agree.  It took me quite a while, at the time, to
untangle the logic of the decisions made by
can_use_hardware_watchpoint and TARGET_CAN_USE_HARDWARE_WATCHPOINT,
and then get them right by introducing
TARGET_REGION_OK_FOR_HW_WATCHPOINT (before this, GDB only tested the
size of the region with TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT).

One problem with that code is that, originally, it was written for
Sparc Lite (I think) where the hardware watchpoints are handled in a
very different way.
From eliz@delorie.com Fri Aug 04 11:11:00 2000
From: Eli Zaretskii <eliz@delorie.com>
To: robert.hoehne@gmx.net
Cc: gdb-patches@sourceware.cygnus.com
Subject: Re: Initialize the correct cwd for the child
Date: Fri, 04 Aug 2000 11:11:00 -0000
Message-id: <200008041811.OAA28514@indy.delorie.com>
References: <200008041341.PAA15581@robby.dittmannsdorf.de>
X-SW-Source: 2000-08/msg00101.html
Content-length: 693

> From: Robert Hoehne <robert.hoehne@gmx.net>
> Date: Fri, 4 Aug 2000 15:43:26 +0200
> 
> On DJGPP, the child's working directory have to be remembered.
> In the current implementation this is done once, when gdb is started.
> But this is wrong, since the user can change it with the gdb-command
> 
> cd <some-directory>
> 
> and when running the child, it is expected, that it get this directory as
> its starting directory. So I made a patch for this.

Thanks for the patch.  However, I wonder: what would be the cwd of an
inferior on Unix, after the following commands:

	 gdb foo
	 cd bar
	 run

I think the DJGPP port should, as much as possible, behave the same
way as GDB does on Unix.
From msnyder@redhat.com Fri Aug 04 11:13:00 2000
From: Michael Snyder <msnyder@redhat.com>
To: Eli Zaretskii <eliz@is.elta.co.il>
Cc: robert.hoehne@gmx.net, gdb-patches@sourceware.cygnus.com
Subject: Re: Initialize the correct cwd for the child
Date: Fri, 04 Aug 2000 11:13:00 -0000
Message-id: <398B07C5.10E3@redhat.com>
References: <200008041341.PAA15581@robby.dittmannsdorf.de> <200008041811.OAA28514@indy.delorie.com>
X-SW-Source: 2000-08/msg00102.html
Content-length: 874

Eli Zaretskii wrote:
> 
> > From: Robert Hoehne <robert.hoehne@gmx.net>
> > Date: Fri, 4 Aug 2000 15:43:26 +0200
> >
> > On DJGPP, the child's working directory have to be remembered.
> > In the current implementation this is done once, when gdb is started.
> > But this is wrong, since the user can change it with the gdb-command
> >
> > cd <some-directory>
> >
> > and when running the child, it is expected, that it get this directory as
> > its starting directory. So I made a patch for this.
> 
> Thanks for the patch.  However, I wonder: what would be the cwd of an
> inferior on Unix, after the following commands:
> 
>          gdb foo
>          cd bar
>          run
> 
> I think the DJGPP port should, as much as possible, behave the same
> way as GDB does on Unix.

I'm pretty sure the cwd would be bar, so the patch
would make djgpp behave more like unix does.
From kevinb@cygnus.com Fri Aug 04 11:43:00 2000
From: Kevin Buettner <kevinb@cygnus.com>
To: Elena Zannoni <ezannoni@cygnus.com>
Cc: gdb-patches@sourceware.cygnus.com
Subject: Re: [PATCH RFA]: Fix hash table bugs in minsyms.c
Date: Fri, 04 Aug 2000 11:43:00 -0000
Message-id: <1000804184305.ZM5237@ocotillo.lan>
References: <1000804003817.ZM3200@ocotillo.lan> <14730.64814.976208.581246@kwikemart.cygnus.com> <ezannoni@cygnus.com>
X-SW-Source: 2000-08/msg00103.html
Content-length: 166

On Aug 4,  1:28pm, Elena Zannoni wrote:

> Kevin Buettner writes:
>  > I request approval for committing the patch below.
> 
> Go ahead, it makes sense. 

Committed.
From fnasser@cygnus.com Fri Aug 04 12:12:00 2000
From: Fernando Nasser <fnasser@cygnus.com>
To: Eli Zaretskii <eliz@is.elta.co.il>
Cc: ac131313@cygnus.com, gdb-patches@sourceware.cygnus.com
Subject: Re: Runtiming hw watchpoints; Was: Split up Z packet enable/disable cmd
Date: Fri, 04 Aug 2000 12:12:00 -0000
Message-id: <398B1590.27E611B8@cygnus.com>
References: <3987DD9A.21F2B29A@cygnus.com> <39884DFA.12DD4043@cygnus.com> <3988E54B.39646794@cygnus.com> <200008030602.CAA26491@indy.delorie.com> <39898EA6.4683E9F2@cygnus.com> <200008031820.OAA27128@indy.delorie.com> <3989C2A4.C5322586@cygnus.com> <200008040922.FAA28046@indy.delorie.com> <398ACA49.8C933672@cygnus.com> <200008041810.OAA28510@indy.delorie.com>
X-SW-Source: 2000-08/msg00104.html
Content-length: 1578

Eli Zaretskii wrote:
> 
> t could do this if it gets the info about all
> the watchpoints and hardware breakpoints defined previously.
> Currently, it only gets information about one single watchpoint at a
> time, and that is not enough, at least on x86.
> 
You are absolutely right.  I missed that.  We can add a field in the breakpoint
structure and store the number of hw registers used by a hw watchpoint.
Then we can fix hw_watchpoint_used_count() to add them up.
Does it sound right?


> I completely agree.  It took me quite a while, at the time, to
> untangle the logic of the decisions made by
> can_use_hardware_watchpoint and TARGET_CAN_USE_HARDWARE_WATCHPOINT,
> and then get them right by introducing
> TARGET_REGION_OK_FOR_HW_WATCHPOINT (before this, GDB only tested the
> size of the region with TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT).
> 
> One problem with that code is that, originally, it was written for
> Sparc Lite (I think) where the hardware watchpoints are handled in a
> very different way.

Right again.  We will probably have to fix things a bit to make them more
generic.  But I guess this will only happen if we try to use it for good.
No pain, no gain :-)

I am merging my Pentium III changes to the current sourceware code.
I will retest things and post some patches as soon as I get them to
work again.  We can start from that.

-- 
Fernando Nasser
Red Hat - Toronto                       E-Mail:  fnasser@cygnus.com
2323 Yonge Street, Suite #300           Tel:  416-482-2661 ext. 311
Toronto, Ontario   M4P 2C9              Fax:  416-482-6299
From kevinb@cygnus.com Fri Aug 04 14:16:00 2000
From: Kevin Buettner <kevinb@cygnus.com>
To: gdb-patches@sourceware.cygnus.com
Subject: [PATCH RFA] Fixup SYMBOL_SECTION for objfiles_relocate()
Date: Fri, 04 Aug 2000 14:16:00 -0000
Message-id: <1000804211627.ZM5495@ocotillo.lan>
X-SW-Source: 2000-08/msg00105.html
Content-length: 5756

I request permission to commit the patch below.

The OS for the project that I'm working on relocates the read/write
and the read-only sections by different amounts.  Thus it is critical
that objfile_relocate() actually have correct section indices during
the relocation process.

 I also discovered that for some symbol sym, SYMBOL_SECTION(sym) was
not providing the correct section index.  In fact, it was almost
always 0.  The same was true for partial symbols (upon which the same
macro magically works) as well.

The reason is that (at least for DWARF2) the SYMBOL_SECTION field
(section) was simply getting initialized to 0.  Furthermore, no
attempt was made to set things right later on.

The interesting thing is that SYMBOL_BFD_SECTION has exactly same
problem, but we have some code which attempts to set things right at
various points along the way.  The relevant functions are
fixup_symbol_section() and fixup_psymbol_section().  It seems to me
that we ought to be setting SYMBOL_SECTION at the same time that we're
setting BFD_SECTION and that is what the second hunk in the diff for
symtab.c is doing below.  [Thanks to Elena for bringing this to my
attention.]

Even once this is done, there is no guarantee that the symbols
and/or partial symbols will have been fixed up (with respect to
SYMBOL_SECTION) by the time that objfile_relocate() has been called.
So... now objfile_relocate() calls fixup_symbol_section() and
fixup_psymbol_section() as appropriate.

I'm not convinced that fixing these fields up at various random places
in the code where we need to access these fields is the right approach
to solving this problem.  It seems to me that it would be better to
attempt to set them correctly at the time that (or perhaps slightly
after) the symbol (or partial symbol) is created.  I made an attempt
at doing this, but I could not get it to work.  (Perhaps the minimal
symbols weren't available yet?) In any event, this is probably an
issue that the symbol table maintainers should consider in the future.
(grep for fixup_symbol_section() and look at all the places that
it's used.)

	* symtab.h (fixup_psymbol_section): Declare.
	* symtab.c (fixup_psymbol_section): Make extern.
	(fixup_section): Fix up section as well as bfd_section.
	* objfiles.c (objfile_relocate): Call fixup_symbol_section
	or fixup_psymbol_section before attempting to access
	the SYMBOL_SECTION component of a symbol or partial symbol.

Index: objfiles.c
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.c,v
retrieving revision 1.7
diff -u -p -r1.7 objfiles.c
--- objfiles.c	2000/07/30 01:48:26	1.7
+++ objfiles.c	2000/08/04 20:46:02
@@ -564,6 +564,9 @@ objfile_relocate (struct objfile *objfil
 	  for (j = 0; j < BLOCK_NSYMS (b); ++j)
 	    {
 	      struct symbol *sym = BLOCK_SYM (b, j);
+
+	      fixup_symbol_section (sym, objfile);
+
 	      /* The RS6000 code from which this was taken skipped
 	         any symbols in STRUCT_NAMESPACE or UNDEF_NAMESPACE.
 	         But I'm leaving out that test, on the theory that
@@ -606,15 +609,21 @@ objfile_relocate (struct objfile *objfil
     for (psym = objfile->global_psymbols.list;
 	 psym < objfile->global_psymbols.next;
 	 psym++)
-      if (SYMBOL_SECTION (*psym) >= 0)
-	SYMBOL_VALUE_ADDRESS (*psym) += ANOFFSET (delta,
-						  SYMBOL_SECTION (*psym));
+      {
+	fixup_psymbol_section (*psym, objfile);
+	if (SYMBOL_SECTION (*psym) >= 0)
+	  SYMBOL_VALUE_ADDRESS (*psym) += ANOFFSET (delta,
+						    SYMBOL_SECTION (*psym));
+      }
     for (psym = objfile->static_psymbols.list;
 	 psym < objfile->static_psymbols.next;
 	 psym++)
-      if (SYMBOL_SECTION (*psym) >= 0)
-	SYMBOL_VALUE_ADDRESS (*psym) += ANOFFSET (delta,
-						  SYMBOL_SECTION (*psym));
+      {
+	fixup_psymbol_section (*psym, objfile);
+	if (SYMBOL_SECTION (*psym) >= 0)
+	  SYMBOL_VALUE_ADDRESS (*psym) += ANOFFSET (delta,
+						    SYMBOL_SECTION (*psym));
+      }
   }
 
   {
Index: symtab.c
===================================================================
RCS file: /cvs/src/src/gdb/symtab.c,v
retrieving revision 1.10
diff -u -p -r1.10 symtab.c
--- symtab.c	2000/07/30 01:48:27	1.10
+++ symtab.c	2000/08/04 20:46:07
@@ -81,10 +81,6 @@ static struct partial_symbol *lookup_par
 						     const char *, int,
 						     namespace_enum);
 
-static struct partial_symbol *fixup_psymbol_section (struct
-						     partial_symbol *,
-						     struct objfile *);
-
 static struct symtab *lookup_symtab_1 (char *);
 
 static void cplusplus_hint (char *);
@@ -520,7 +516,10 @@ fixup_section (struct general_symbol_inf
   msym = lookup_minimal_symbol (ginfo->name, NULL, objfile);
 
   if (msym)
-    ginfo->bfd_section = SYMBOL_BFD_SECTION (msym);
+    {
+      ginfo->bfd_section = SYMBOL_BFD_SECTION (msym);
+      ginfo->section = SYMBOL_SECTION (msym);
+    }
 }
 
 struct symbol *
@@ -537,7 +536,7 @@ fixup_symbol_section (struct symbol *sym
   return sym;
 }
 
-static struct partial_symbol *
+struct partial_symbol *
 fixup_psymbol_section (struct partial_symbol *psym, struct objfile *objfile)
 {
   if (!psym)
Index: symtab.h
===================================================================
RCS file: /cvs/src/src/gdb/symtab.h,v
retrieving revision 1.11
diff -u -p -r1.11 symtab.h
--- symtab.h	2000/06/05 20:49:53	1.11
+++ symtab.h	2000/08/04 20:46:09
@@ -1414,6 +1414,10 @@ extern int in_prologue (CORE_ADDR pc, CO
 extern struct symbol *fixup_symbol_section (struct symbol *,
 					    struct objfile *);
 
+extern struct partial_symbol *fixup_psymbol_section (struct partial_symbol
+						     *psym,
+						     struct objfile *objfile);
+
 /* Symbol searching */
 
 /* When using search_symbols, a list of the following structs is returned.

Re: Runtiming hw watchpoints; Was: Split up Z packet enable/disable cmd

[Eli Zaretskii]

> Date: Fri, 04 Aug 2000 15:12:16 -0400
> From: Fernando Nasser <fnasser@cygnus.com>
> 
> Eli Zaretskii wrote:
> > 
> > t could do this if it gets the info about all
> > the watchpoints and hardware breakpoints defined previously.
> > Currently, it only gets information about one single watchpoint at a
> > time, and that is not enough, at least on x86.
> > 
> You are absolutely right.  I missed that.  We can add a field in the breakpoint
> structure and store the number of hw registers used by a hw watchpoint.
> Then we can fix hw_watchpoint_used_count() to add them up.
> Does it sound right?

This would be a huge step in the right direction, but it is still not
enough.

Imagine a situation where a user sets 2 different watchpoints at the
same address.  (This might make sense if these watchpoints have
different conditions, for example.)  DJGPP currently can insert such
watchpoints using a single debug register, by implementing reference
counts for debug registers.  I believe that eventually, other x86
platforms will use a similar technique, since the number of debug
registers is so small.  (We had a discussion some time ago where
everybody agreed that all x86 targets should share the code which
handles watchpoints.)

In situations like this, it is not enough to know how many debug
registers are already taken up.  You need to know everything about
those other watchpoints.  (In the x86 case, you need to know only the
type of the watchpoint, and the address and the size of the watched
region, but other platforms might want to know more.)

So I think we have two alternatives to solving this:

  - Make can_use_hardware_watchpoint loop over all the watchpoints
    before you ask it about the additional one, so that the target end
    could get the entire picture.

  - Invent a new API call whereby the target will add and remove the
    info about the watchpoints and breakpoints to some internal
    storage at the target end.

The latter seems like a cleaner solution, and also has a benefit that
it might remove the need for inserting the breakpoints and watchpoints
one by one when resuming: you could simply tell the target to insert
all active ones in one go.  But it does mean more extensive changes...

> But I guess this will only happen if we try to use it for good.
> No pain, no gain :-)

Sure.  Personally, I use watchpoints a lot, and would be glad to see
any improvements in their handling.  The current code in go32-nat.c
works well enough for me, but it is messy and full of pitfalls.  I
will welcome any changes in higher-level APIs that would make this
simpler and cleaner.

> I am merging my Pentium III changes to the current sourceware code.
> I will retest things and post some patches as soon as I get them to
> work again.  We can start from that.

Great!
From eliz@delorie.com Fri Aug 04 23:58:00 2000
From: Eli Zaretskii <eliz@delorie.com>
To: kevinb@cygnus.com
Cc: gdb-patches@sourceware.cygnus.com
Subject: Re: [PATCH RFC] Protoize ch-exp.c, core-regset.c
Date: Fri, 04 Aug 2000 23:58:00 -0000
Message-id: <200008050658.CAA29260@indy.delorie.com>
References: <1000803190941.ZM2697@ocotillo.lan> <200008040931.FAA28059@indy.delorie.com> <1000804151730.ZM4041@ocotillo.lan> <200008041810.OAA28507@indy.delorie.com> <1000805000138.ZM5802@ocotillo.lan>
X-SW-Source: 2000-08/msg00121.html
Content-length: 759

> Date: Fri, 4 Aug 2000 17:01:38 -0700
> From: Kevin Buettner <kevinb@cygnus.com>
>  
>  #if 0
> +/* Parse the name of an option string.  If ALLOW_ALL is 1, ALL is
> +   allowed as a postfix. */
> +
>  static tree
> -parse_opt_name_string (allow_all)
> -     int allow_all;		/* 1 if ALL is allowed as a postfix */
> +parse_opt_name_string (int allow_all)

Err... now that I actually *look* at ch-exp.c, it seems that this
function should be described simply as "Parse name_string."  I cannot
figure out what does the "opt" part stand for; perhaps it hints at the
optional ALLOW_ALL parameter.

Sorry, I should have told that the description I showed in my previous
message was meant to be an example, not suggest that this is the
actual function description.
From geoffk@cygnus.com Sat Aug 05 01:15:00 2000
From: Geoff Keating <geoffk@cygnus.com>
To: guo@cup.hp.com
Cc: gdb-patches@sourceware.cygnus.com
Subject: Re: recent dejagnu changes
Date: Sat, 05 Aug 2000 01:15:00 -0000
Message-id: <200008050810.BAA11524@localhost.cygnus.com>
References: <Pine.LNX.4.10.10008042318560.26633-100000@hpcll168.cup.hp.com>
X-SW-Source: 2000-08/msg00122.html
Content-length: 792

> Date: Fri, 4 Aug 2000 23:33:47 -0700 (PDT)
> From: Jimmy Guo <guo@cup.hp.com>

> The problem is that I thought a dejagnu test tree contains
> <tool>.<something>/ as a proper suite name, anywhere.  However apparently
> the GCC suite just requires the top level suite name be such.

I think, in the absence of documentation, you have to assume that the
original code was correct and the way you're using it was wrong :-(.

> Actually, the dir_to_run and cmdline_dir_to_run change is OK ... that
> allows specification of a list of test suites, instead of one at a time.

Could you post these as a separate patch, and test them separately?
They were committed together, and it's difficult to know which part
of the commit belongs with which change.

-- 
- Geoffrey Keating <geoffk@cygnus.com>
From kettenis@wins.uva.nl Sat Aug 05 01:54:00 2000
From: Mark Kettenis <kettenis@wins.uva.nl>
To: kevinb@cygnus.com
Cc: gdb-patches@sourceware.cygnus.com
Subject: Re: [PATCH RFC] Protoize ch-exp.c, core-regset.c
Date: Sat, 05 Aug 2000 01:54:00 -0000
Message-id: <200008050854.e758sNh00394@delius.kettenis.local>
References: <1000803190941.ZM2697@ocotillo.lan> <200008040931.FAA28059@indy.delorie.com> <1000804151730.ZM4041@ocotillo.lan> <200008041810.OAA28507@indy.delorie.com> <1000805000138.ZM5802@ocotillo.lan>
X-SW-Source: 2000-08/msg00123.html
Content-length: 967

   Date: Fri, 4 Aug 2000 17:01:38 -0700
   From: Kevin Buettner <kevinb@cygnus.com>

   Index: core-regset.c
   ===================================================================
   RCS file: /cvs/src/src/gdb/core-regset.c,v
   retrieving revision 1.5
   diff -u -r1.5 core-regset.c
   --- core-regset.c	2000/07/30 01:48:25	1.5
   +++ core-regset.c	2000/08/04 23:35:40
   @@ -72,7 +72,8 @@
       Read the values of either the general register set (WHICH equals 0)
       or the floating point register set (WHICH equals 2) from the core
       file data (pointed to by CORE_REG_SECT), and update gdb's idea of
   -   their current values.  The CORE_REG_SIZE parameter is ignored.
   +   their current values.  The CORE_REG_SIZE and REG_ADDR parameters are
   +   ignored.

       NOTES

If you look at the actual code, you'll see that CORE_REG_SIZE isn't
really ignored, but is used to check if the structure size matches the
amount of data in the core file.

Mark
From fnasser@cygnus.com Sat Aug 05 09:29:00 2000
From: Fernando Nasser <fnasser@cygnus.com>
To: Geoff Keating <geoffk@cygnus.com>
Cc: guo@cup.hp.com, gdb-patches@sourceware.cygnus.com
Subject: Re: recent dejagnu changes
Date: Sat, 05 Aug 2000 09:29:00 -0000
Message-id: <398C3E91.D0EDB934@cygnus.com>
References: <Pine.LNX.4.10.10008042318560.26633-100000@hpcll168.cup.hp.com> <200008050810.BAA11524@localhost.cygnus.com>
X-SW-Source: 2000-08/msg00124.html
Content-length: 910

Geoff Keating wrote:
> 
> > Actually, the dir_to_run and cmdline_dir_to_run change is OK ... that
> > allows specification of a list of test suites, instead of one at a time.
> 
We do want to retain this functionality.

> Could you post these as a separate patch, and test them separately?
> They were committed together, and it's difficult to know which part
> of the commit belongs with which change.
> 
Better than that would be if Jimmy submitted a patch commenting out (with the
explanation why) the code that he believes is the offending one.  If that passes
the gcc testsuite tests we commit it.

I wonder if there isn't a way to accomodate both the gcc tree structure and the HP one...

-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@cygnus.com
2323 Yonge Street, Suite #300           Tel:  416-482-2661 ext. 311
Toronto, Ontario   M4P 2C9              Fax:  416-482-6299
From kevinb@cygnus.com Sat Aug 05 11:01:00 2000
From: Kevin Buettner <kevinb@cygnus.com>
To: gdb-patches@sourceware.cygnus.com
Subject: Re: [PATCH RFC] Protoize ch-exp.c, core-regset.c
Date: Sat, 05 Aug 2000 11:01:00 -0000
Message-id: <1000805180139.ZM7354@ocotillo.lan>
References: <1000803190941.ZM2697@ocotillo.lan> <200008040931.FAA28059@indy.delorie.com> <1000804151730.ZM4041@ocotillo.lan> <200008041810.OAA28507@indy.delorie.com> <1000805000138.ZM5802@ocotillo.lan> <kevinb@cygnus.com>
X-SW-Source: 2000-08/msg00125.html
Content-length: 2348

Let's try this (yet) again...

The previous versions of this patch at

    http://sources.redhat.com/ml/gdb-patches/2000-08/msg00073.html
and
    http://sources.redhat.com/ml/gdb-patches/2000-08/msg00113.html

are withdrawn.

Thanks to Mark Kettenis and Eli Zaretskii for pointing out problems
with the previous patch.

	* ch-exp.c (parse_opt_name_string): Protoize.  [Thanks to Eli
	Zaretskii for the prefatory comment.]
	* core-regset.c (fetch_core_registers): Protoize; revise
	comment.

Index: ch-exp.c
===================================================================
RCS file: /cvs/src/src/gdb/ch-exp.c,v
retrieving revision 1.4
diff -u -r1.4 ch-exp.c
--- ch-exp.c	2000/07/30 01:48:24	1.4
+++ ch-exp.c	2000/08/05 17:56:35
@@ -301,9 +301,10 @@
 }
 
 #if 0
+/* Parse a name string.  If ALLOW_ALL is 1, ALL is allowed as a postfix. */
+
 static tree
-parse_opt_name_string (allow_all)
-     int allow_all;		/* 1 if ALL is allowed as a postfix */
+parse_opt_name_string (int allow_all)
 {
   int token = PEEK_TOKEN ();
   tree name;
Index: core-regset.c
===================================================================
RCS file: /cvs/src/src/gdb/core-regset.c,v
retrieving revision 1.5
diff -u -r1.5 core-regset.c
--- core-regset.c	2000/07/30 01:48:25	1.5
+++ core-regset.c	2000/08/05 17:56:37
@@ -72,20 +72,16 @@
    Read the values of either the general register set (WHICH equals 0)
    or the floating point register set (WHICH equals 2) from the core
    file data (pointed to by CORE_REG_SECT), and update gdb's idea of
-   their current values.  The CORE_REG_SIZE parameter is ignored.
+   their current values.  The CORE_REG_SIZE parameter is compared to
+   the size of the gregset or fpgregset structures (as appropriate) to
+   validate the size of the structure from the core file.  The
+   REG_ADDR parameter is ignored.
 
-   NOTES
-
-   Use the indicated sizes to validate the gregset and fpregset
-   structures.
  */
 
 static void
-fetch_core_registers (core_reg_sect, core_reg_size, which, reg_addr)
-     char *core_reg_sect;
-     unsigned core_reg_size;
-     int which;
-     CORE_ADDR reg_addr;	/* Unused in this version */
+fetch_core_registers (char *core_reg_sect, unsigned core_reg_size, int which,
+		      CORE_ADDR reg_addr)
 {
 #if defined (HAVE_GREGSET_T) && defined (HAVE_FPREGSET_T)
   gregset_t gregset;