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

[Andrew Cagney <ac131313@cygnus.com>]

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