Re: Remote watchpoint support.

[Eli Zaretskii <eliz@delorie.com>]

> From: jtc@redback.com (J.T. Conklin)
> Date: 30 Oct 2000 12:39:53 -0800
> 
> One thing I dislike about it is that the debug agent never explicitly
> tells GDB that a watchpoint has been hit, but rather it is implicitly
> coded in the return value of the query.

I agree that this is not a good design.

> The second is the return value of 0 if the target stopped for another
> reason.  I (now) know that this is the current behavior of the target_
> stopped_data_address() macro, but it use the one address that I think
> might be very important to watch on targets where the zero page isn't
> unmapped (in which case a reference would cause a exception/trap).

I would actually be very happy if we could change the interface of
target_stopped_data_address so that zero would not be treated
specially.  One problem with the current design is that I cannot catch
NULL pointer dereferences on Windows, where the null page cannot be
unmapped from the address space of DJGPP programs.  This prevents me
from finding such bugs by setting a watchpoint at address 0.

> While it might not be possible to fix in GDB's internals for some
> time

What would it take to change that?  Perhaps it would be a good idea to
have the list of the obstacles, in case someone might try to negotiate
them one by one.
From eliz@delorie.com Tue Oct 31 01:09:00 2000
From: Eli Zaretskii <eliz@delorie.com>
To: john@Calva.COM
Cc: Peter.Schauer@Regent.E-Technik.TU-Muenchen.DE, gdb-patches@sources.redhat.com
Subject: Re: Patch for gdb5.0; enable hardware watchpoints on UnixWare
Date: Tue, 31 Oct 2000 01:09:00 -0000
Message-id: <200010310906.EAA21817@indy.delorie.com>
References: <NCBBLMGKIKDGJMEOMNMEEEDMHEAA.john@Calva.COM>
X-SW-Source: 2000-10/msg00294.html
Content-length: 1008

> From: "John Hughes" <john@Calva.COM>
> Date: Mon, 30 Oct 2000 17:11:58 +0100
> 
> > But all the i386v4-nat.c code has to be conditionalized on 
> > UNIXWARE (or better yet, moved to a new i386sco-nat.c file), as
> > i386v4-nat.c is used by x86 Solaris as well, and Solaris uses page
> > faulting instead of debug registers for watchpoints, and would not
> > compile with your patches installed.
> 
> Aha.  Here's a fixed version of the patch; the i386v4-nat.c changes
> are conditional on UNIXWARE.

Note that I'm working on a unified watchpoint implementation for all
ia32 targets, based on the code written for the DJGPP port (see
go32-nat.c).  This implementation will allow to watch large areas (up
to 16 bytes) and debug register sharing via reference counts (required
for watching overlapping areas) on all ia32 platforms.

I am trying to make this happen for v5.1, so perhaps changes like this
one should be witheld and not committed, at least until we decide that
I'm not living up to my promises.
From kettenis@wins.uva.nl Tue Oct 31 01:37:00 2000
From: Mark Kettenis <kettenis@wins.uva.nl>
To: msnyder@cygnus.com
Cc: gdb-patches@sourceware.cygnus.com, kevinb@cygnus.com
Subject: Re: [PATCH]: Extend SVR4 shlib debug changes to SH3 and other targets
Date: Tue, 31 Oct 2000 01:37:00 -0000
Message-id: <200010310937.e9V9b4604953@debye.wins.uva.nl>
References: <200010310102.RAA24876@cleaver.cygnus.com>
X-SW-Source: 2000-10/msg00295.html
Content-length: 2239

   From: Michael Snyder <msnyder@cygnus.com>
   Date: Mon, 30 Oct 2000 17:02:01 -0800 (PST)

   This change moves the definition of SVR4_SHARED_LIBS into 
   config/tm-linux.h, so that it will affect all linuxen.
   I then add a new file config/sh/tm-linux.h which includes
   config/tm-linux.h (as well as sh/tm-sh.h).  Finally I add
   target-specific definitions of fetch_link_map_offsets, so
   that shared libraries can be cross-debugged.

   2000-10-30  Michael Snyder  <msnyder@cleaver.cygnus.com>

	   * config/sh/tm-linux.h: New file.  Include generic tm-linux.h,
	   plus tm-sh.h, then define SVR4_FETCH_LINK_MAP_OFFSETS to use
	   the sh target function instead of the default link map offsets.
	   * config/sh/sh.mt: Add solib.o and solib-svr4.o to TDEPFILES.
	   Use sh/tm-linux.h instead of sh/tm-sh.h.
	   * sh-tdep.c (sh_linux_svr4_fetch_link_map_offsets):
	   New function.  Construct target-specific link map offsets.
	   * i386-linux-tdep.c (i386_linux_svr4_fetch_link_map_offsets:
	   New function.  Construct target-specific link map offsets.
	   * config/i386/tm-linux.h: Use above function instead of default.

   2000-10-30  Michael Snyder  <msnyder@cleaver.cygnus.com>

	   * config/i386/tm-linux.h: Remove definition of SVR4_SHARED_LIBS,
	   and inclusion of solib.h.  Move up into ../tm-linux.h.
	   config/tm-linux.h: Define SVR4_SHARED_LIBS, include solib.h.

Looks good to me.  One minor nit though:

   Index: config/tm-linux.h
   ===================================================================
   RCS file: /cvs/src/src/gdb/config/tm-linux.h,v
   retrieving revision 1.1.1.1
   diff -c -3 -p -r1.1.1.1 tm-linux.h
   *** tm-linux.h	1999/12/07 03:56:08	1.1.1.1
   --- tm-linux.h	2000/10/31 00:57:12
   ***************
   *** 34,36 ****
   --- 34,42 ----
     /* We need this file for the SOLIB_TRAMPOLINE stuff. */

     #include "tm-sysv4.h"
   + 
   + /* We define this if link.h is available, because with ELF we use SVR4 style
   +    shared libraries.  FIXME: move up into config/tm-linux.h?  */
   + 
   + #define SVR4_SHARED_LIBS
   + #include "solib.h"		/* Support for shared libraries. */

Please update the comment.  The FIXME can go now, and the blurb about
link.h isn't true anymore isn't it?
From john@Calva.COM Tue Oct 31 03:20:00 2000
From: "John Hughes" <john@Calva.COM>
To: "Eli Zaretskii" <eliz@is.elta.co.il>
Cc: <gdb-patches@sources.redhat.com>
Subject: RE: Patch for gdb5.0; enable hardware watchpoints on UnixWare
Date: Tue, 31 Oct 2000 03:20:00 -0000
Message-id: <NCBBLMGKIKDGJMEOMNMEMEELHEAA.john@Calva.COM>
References: <200010310906.EAA21817@indy.delorie.com>
X-SW-Source: 2000-10/msg00296.html
Content-length: 2169

Eli Zaretskii writes:
> Note that I'm working on a unified watchpoint implementation for all
> ia32 targets, based on the code written for the DJGPP port (see
> go32-nat.c).

Ah good.  I saw the stuff in TODO yesterday and was trying to work up
the courage to propose doing this myself.

> I am trying to make this happen for v5.1, so perhaps changes like this
> one should be witheld and not committed, at least until we decide that
> I'm not living up to my promises.

Oh, please ignore my patch then, could you steal the svr42mp specific
bits of it for 5.1?  (debug registers held in pr_family element of
proc status, PCSDEBUG operation to write debug registers).

Looking at version 1.6 of go32-nat.c I've got a couple of comments:

1. Why not zap the waddr arg to go32_..._watchpoint?  It's not used.

2. In go32_insert_aligned_watchpoint (and ...remove_aligned...) you
   have a "switch" to work out the len_bits for the watchpoint, then a
   loop to find an existing watchpoint that might match, then an
   "if" to check for non-aligned watchpoints.  (Which could not have
   matched in the loop).

   It seems to me this could be simplified to:

  ...
  if (addr % len)
    return go32_handle_nonaligned_watchpoint (wp_insert, addr, len, rw);

  switch (len)
  {
  case 4:
    len_bits = DR_LEN_4;
    break;
  case 2:
    len_bits = DR_LEN_2;
    break;
  case 1:
    len_bits = DR_LEN_1;
    break;
  default:
    return go32_handle_nonaligned_watchpoint (wp_insert, addr, len, rw);
  }

  /* Look for an occupied debug register with the same address and the
     same RW and LEN definitions.  If we find one, we can use it for
     this watchpoint as well (and save a register).  */
  ...

3. You're not using the DR_FIRSADDR/DR_LASTADDR defines.

4. You have a define DR_RW_READWRITE, other people (SVR3/SVR4/SVR5) seem
   to have DR_RW_READ with the same value (0x3).

5. Not everybody has DR_CONTROL_MASK.

	#define DR_CONTROL_MASK ((1<<DR_CONTROL_SIZE) - 1)

-- 
John Hughes <john@Calva.COM>, 
        CalvaEDI SA.                            Tel: +33-1-4313-3131
        66 rue du Moulin de la Pointe,          Fax: +33-1-4313-3139
        75013 PARIS.