Remote watchpoint support for FRV

Remote watchpoint support for FRV

[Ulrich Weigand]

Hello,

the tm-frv.h header file overrides a number of watchpoint related
target macros, in particular:

#define STOPPED_BY_WATCHPOINT(W) \
   ((W).kind == TARGET_WAITKIND_STOPPED \
   && (W).value.sig == TARGET_SIGNAL_TRAP \
   && frv_have_stopped_data_address())

Note that FRV is a remote-only target, so this will affect and
modifiy the behaviour of the remote.c watchpoint handling.

Apparently the intent is to use the standard remote protocol
mechanisms to *set* watchpoints, but change the way to test
whether a watchpoint was hit.  Usually, remote.c would rely
on the remote stub to detect this condition and report it
via a "watch" stop reason in the stop reply packet.

However, due to the tm-frv.h override, for FRV targets GDB
will actually perform the check on the host side (by reading
control registers via the normal register access protocol).


To get rid of the TM header file, I can see two options:

- Simply remove support for this way of handling remote 
  watchpoints.  Existing FRV remote stubs will need to be
  changed to use the standard "watch" stop reason code.

- If we think we cannot break existing FRV stubs, I'd
  suggest to add new gdbarch callbacks that will be used
  by the remote target to allow architecture-specific
  overrides of the remote watchpoint mechanism, e.g.:
    
     gdbarch_remote_stopped_by_watchpoint
     gdbarch_remote_stopped_data_address
     gdbarch_remote_check_watch_resources


Any opinions on this?  The first option would obviously be
the easiest to implement in GDB, but I'm not sure what extent
FRV remote stubs are currently in use ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: Remote watchpoint support for FRV

[Daniel Jacobowitz]

On Fri, Oct 05, 2007 at 03:47:53PM +0200, Ulrich Weigand wrote:
> Hello,
> 
> the tm-frv.h header file overrides a number of watchpoint related
> target macros, in particular:
> 
> #define STOPPED_BY_WATCHPOINT(W) \
>    ((W).kind == TARGET_WAITKIND_STOPPED \
>    && (W).value.sig == TARGET_SIGNAL_TRAP \
>    && frv_have_stopped_data_address())

Kevin, you were the last person to work on the FRV target (far as I
can tell); do you know anything about this?

Ulrich, there's a third option, but I don't know if it's practical.
Can we somehow specialize the remote target based on the architecture?
It's not the first time I've thought it would be useful.  But we
already have the target adjusting the architecture and adding
something in the other direction might be awkward.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Remote watchpoint support for FRV

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> On Fri, Oct 05, 2007 at 03:47:53PM +0200, Ulrich Weigand wrote:
> > Hello,
> > 
> > the tm-frv.h header file overrides a number of watchpoint related
> > target macros, in particular:
> > 
> > #define STOPPED_BY_WATCHPOINT(W) \
> >    ((W).kind == TARGET_WAITKIND_STOPPED \
> >    && (W).value.sig == TARGET_SIGNAL_TRAP \
> >    && frv_have_stopped_data_address())
> 
> Kevin, you were the last person to work on the FRV target (far as I
> can tell); do you know anything about this?

Any updates on this?  I've had another look at this, and it appears
to me that this wasn't actually functional in mainline GDB anyway:
Up until your recent breakpoint.c change, watchpoints were active 
only if either HAVE_STEPPABLE_WATCHPOINT, HAVE_NONSTEPPABLE_WATCHPOINT,
or HAVE_CONTINUABLE_WATIPOINT were defined.

In this particular case, the tm file doesn't define any of these (the
#define HAVE_STEPPABLE_WATCHPOINT is commented out), an nm file doesn't
apply, the remote target does not define to_have_continuable_watchpoint,
and frv-tdep does not use set_gdbarch_nonsteppable_watchpoint.

Thus it would appear that the STOPPED_BY_WATCHPOINT and 
target_stopped_data_address macros defined in the tm-frv.h header
file aren't actually ever called.  (Since the recent watchpoint change,
they *are* active, but probably not correctly as common code now assumes
continuable watchpoint behaviour).

Note that the HAVE_STEPPABLE_WATCHPOINT macro is commented out since the
very first version of tm-frv.h checked into the GDB CVS, so I'm not sure
what to think of that ...  Did this ever work?

If it actually didn't ever work, I'd say we should just remove the tm
file now; if somebody wants to get remote watchpoint support on frv
working, they can (and should) use the existing mechanism.

> Ulrich, there's a third option, but I don't know if it's practical.
> Can we somehow specialize the remote target based on the architecture?
> It's not the first time I've thought it would be useful.  But we
> already have the target adjusting the architecture and adding
> something in the other direction might be awkward.

I agree this would probably be a bit awkward ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: Remote watchpoint support for FRV

[Daniel Jacobowitz]

On Sun, Oct 14, 2007 at 09:55:11PM +0200, Ulrich Weigand wrote:
> Thus it would appear that the STOPPED_BY_WATCHPOINT and 
> target_stopped_data_address macros defined in the tm-frv.h header
> file aren't actually ever called.  (Since the recent watchpoint change,
> they *are* active, but probably not correctly as common code now assumes
> continuable watchpoint behaviour).
> 
> Note that the HAVE_STEPPABLE_WATCHPOINT macro is commented out since the
> very first version of tm-frv.h checked into the GDB CVS, so I'm not sure
> what to think of that ...  Did this ever work?
> 
> If it actually didn't ever work, I'd say we should just remove the tm
> file now; if somebody wants to get remote watchpoint support on frv
> working, they can (and should) use the existing mechanism.

Probably the submitter meant to finish up watchpoint support later.

At this stage, I agree with you that we should simply remove the file.

-- 
Daniel Jacobowitz
CodeSourcery

[commit] Re: Remote watchpoint support for FRV

[Ulrich Weigand]

Daniel Jacobowitz wrote:

> At this stage, I agree with you that we should simply remove the file.

OK; I've committed the following patch.

Thanks,
Ulrich


ChangeLog:

	* config/frv/frv.mt (DEPRECATED_TM_FILE): Remove.
	* config/frv-tm-frv.h: Delete file.


diff -urNp gdb-orig/gdb/config/frv/frv.mt gdb-head/gdb/config/frv/frv.mt
--- gdb-orig/gdb/config/frv/frv.mt	2007-10-06 15:39:15.000000000 +0200
+++ gdb-head/gdb/config/frv/frv.mt	2007-10-06 15:41:42.000000000 +0200
@@ -1,5 +1,4 @@
 # Target: Fujitsu FRV processor
 TDEPFILES= frv-tdep.o frv-linux-tdep.o solib.o solib-frv.o corelow.o
-DEPRECATED_TM_FILE= tm-frv.h
 SIM_OBS = remote-sim.o
 SIM = ../sim/frv/libsim.a
diff -urNp gdb-orig/gdb/config/frv/tm-frv.h gdb-head/gdb/config/frv/tm-frv.h
--- gdb-orig/gdb/config/frv/tm-frv.h	2007-08-28 23:30:21.000000000 +0200
+++ gdb-head/gdb/config/frv/tm-frv.h	1970-01-01 01:00:00.000000000 +0100
@@ -1,40 +0,0 @@
-/* Target definitions for the Fujitsu FR-V, for GDB, the GNU Debugger.
-   Copyright 2000, 2004, 2007 Free Software Foundation, Inc.
-
-   This file is part of GDB.
-
-   This program is free software; you can redistribute it and/or modify
-   it under the terms of the GNU General Public License as published by
-   the Free Software Foundation; either version 3 of the License, or
-   (at your option) any later version.
-
-   This program is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-   GNU General Public License for more details.
-
-   You should have received a copy of the GNU General Public License
-   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
-
-/* This target uses an architecture vector for most architecture methods.  */
-
-#define TARGET_CAN_USE_HARDWARE_WATCHPOINT(type, cnt, ot) \
-	frv_check_watch_resources (type, cnt, ot)
-extern int frv_check_watch_resources (int type, int cnt, int ot);
-
-/* When a hardware watchpoint fires off the PC will be left at the
-   instruction which caused the watchpoint.  It will be necessary for
-   GDB to step over the watchpoint. */
-
-/*#define HAVE_STEPPABLE_WATCHPOINT 1*/
-
-#define STOPPED_BY_WATCHPOINT(W) \
-   ((W).kind == TARGET_WAITKIND_STOPPED \
-   && (W).value.sig == TARGET_SIGNAL_TRAP \
-   && frv_have_stopped_data_address())
-extern int frv_have_stopped_data_address(void);
-
-/* Use these macros for watchpoint insertion/deletion.  */
-#define target_stopped_data_address(target, x) frv_stopped_data_address(x)
-extern int frv_stopped_data_address(CORE_ADDR *addr_p);
-


-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com