From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark Salter To: ac131313@cygnus.com Cc: gdb-patches@sources.redhat.com Subject: Re: remote watchpoint support Date: Mon, 06 Nov 2000 05:51:00 -0000 Message-id: <200011061353.eA6Drmi30385@deneb.localdomain> References: <200011012131.eA1LVa514840@deneb.localdomain> <3A069DB3.85902ACD@cygnus.com> X-SW-Source: 2000-11/msg00056.html >>>>> Andrew Cagney writes: > FYI, changelog entries should be separated out and not included in the > diff. Yup. I think I did that the last time, but had a relapse of expediency this time. >> Index: remote.c >> =================================================================== >> RCS file: /cvs/src/src/gdb/remote.c,v >> retrieving revision 1.26 >> diff -p -r1.26 remote.c >> *** remote.c 2000/10/23 22:49:28 1.26 >> --- remote.c 2000/11/01 21:06:29 >> *************** void _initialize_remote (void); >> *** 204,209 **** >> --- 204,216 ---- >> >> /* */ >> >> + /* This is set to the data address of the access causing the target >> + * to stop for a watchpoint. */ >> + static CORE_ADDR remote_watch_data_address; >> + >> + /* This is non-zero if taregt stopped for a watchpoint. */ >> + static int remote_stopped_by_watchpoint_p; >> + > I've strong reservations about the addition of these two ``global'' > variables. Is it possible to modify things higher up so that their > addition isn't required? Without a doubt. It would just take time to sort everything out to make sure existing support isn't broken. > I'm also trying to understand how remote_stopped_data_address() et.al. > gets called. Good luck with that. ;-) Its all part of bpstat_stop_status(). The inferior stops and gdb tries to see if the user needs to informed, etc. bpstat_stop_status() walks through the bp list and for write-only wathcpoints it reads the watched memory to see if changed. For read-only and access watchpoints, it calls target_stopped_data_address() which is a macro that usually gets defined in a tm-xxx.h file. If we add the remote_stopped_by_watchpoint() and remote_stopped_data_address functions, then the tm-xxx.h additions for remote watchpoint suppport are pretty simple: /* Add HW watchpoint support */ #define TARGET_HAS_HARDWARE_WATCHPOINTS #define TARGET_HW_WATCH_LIMIT 1 #define TARGET_CAN_USE_HARDWARE_WATCHPOINT(type, cnt, ot) \ xxx_check_watch_resources (type, cnt, 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. */ /* Comment out. On this target, the memory access is issued before the watchpoint triggers.*/ /*#define HAVE_STEPPABLE_WATCHPOINT 1*/ #define STOPPED_BY_WATCHPOINT(W) \ remote_stopped_by_watchpoint() /* Use these macros for watchpoint insertion/deletion. */ #define target_insert_watchpoint(addr, len, type) \ remote_insert_watchpoint (addr, len, type) #define target_remove_watchpoint(addr, len, type) \ remote_remove_watchpoint (addr, len, type) /* Use this to get the data address that triggered a watchpoint */ #define target_stopped_data_address() \ remote_stopped_data_address() extern int xxx_check_watch_resources (int type, int cnt, int ot); >> *************** struct gdb_ext_thread_info >> *** 963,969 **** >> >> #define BUF_THREAD_ID_SIZE (OPAQUETHREADBYTES*2) >> >> ! char *unpack_varlen_hex (char *buff, int *result); >> >> static char *unpack_nibble (char *buf, int *val); >> >> --- 970,976 ---- >> >> #define BUF_THREAD_ID_SIZE (OPAQUETHREADBYTES*2) >> >> ! char *unpack_varlen_hex (char *buff, ULONGEST *result); >> >> static char *unpack_nibble (char *buf, int *val); >> > The ChangeLog doesn't mention this change? Can I suggest this be > submitted separatly with the tweek that follows: Oops. >> --- 2657,2682 ---- >> p, buf); >> if (strncmp ((const char *) p, "thread", p1 - p) == 0) >> { > ULONGEST thread_num; >> ! p_temp = unpack_varlen_hex (++p1, &ul); >> ! thread_num = ul; >> record_currthread (thread_num); >> p = (unsigned char *) p_temp; >> } > Suggest just making threadnum a LONGEST and being done with it. Ok. --Mark