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

[Eli Zaretskii <eliz@delorie.com>]

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