From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cagney To: Mark Salter Cc: gdb-patches@sources.redhat.com Subject: Re: remote watchpoint support Date: Mon, 06 Nov 2000 04:08:00 -0000 Message-id: <3A069DB3.85902ACD@cygnus.com> References: <200011012131.eA1LVa514840@deneb.localdomain> X-SW-Source: 2000-11/msg00054.html Mark Salter wrote: > > Ok. Here's a patch that allows a remote target to pass watchpoint > information back to gdb using the T packet. It also fixes the T > packet handling to ignore other optional info which may not be > recognized. This changes the behavior to match the documentation. > > While testing this, I discovered that GDB tries to read data from > the watched address before the watchpoints are removed. This causes > another watchpoint exception so the stub returns an error response > to the read memory request. > > --Mark > > Index: ChangeLog > =================================================================== > RCS file: /cvs/src/src/gdb/ChangeLog,v > retrieving revision 1.744 > diff -p -r1.744 ChangeLog > *** ChangeLog 2000/10/31 19:35:03 1.744 > --- ChangeLog 2000/11/01 21:06:26 > *************** > *** 1,3 **** > --- 1,10 ---- > + 2000-11-01 Mark Salter > + > + * remote.c (remote_wait): Add support to extract optional > + watchpoint information from T-packet. Ignore unrecognized > + optional info in T-packet. > + (remote_async_wait): Ditto. > + FYI, changelog entries should be separated out and not included in the diff. > 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? I'm also trying to understand how remote_stopped_data_address() et.al. gets called. Andrew > *************** 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: > --- 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. Andrew >From ac131313@cygnus.com Mon Nov 06 05:13:00 2000 From: Andrew Cagney To: Stephane Carrez Cc: gdb-patches@sourceware.cygnus.com Subject: Re: [PATCH]: Fixes for pseudo regs support Date: Mon, 06 Nov 2000 05:13:00 -0000 Message-id: <3A06ACF1.AA32DF9C@cygnus.com> References: <39A5B4A7.8261D654@worldnet.fr> X-SW-Source: 2000-11/msg00055.html Content-length: 4443 Stephane Carrez wrote: > > Hi! > > Andrew Cagney wrote: > > Could I encourage you to persue this alternative. It isn't that I > > have something against extra multi-arch entries (ok I do but not in this > > case :-) but rather that the problem you're describing (registers living > > in memory space) is far more common than you might think. Off hand, I > > can think of at least three targets that have an identical problem. In > > those cases people ended up shoving hacks into either/and SIM and the > > target stub :-(. > > Ok. > > After a close look of the problem, it's not that big or complex. > > I assume that: > - Machine dependent parts need not be changed: they must know how > to handle pseudo registers (well, only the sh does that). > > - The remote, monitor, sim stubs need not be changed because they > only deal with real hard registers. They are not aware at all > of the existence of pseudo registers. > > Now, what remains is in fact the following places: > > - The stabs reader performs checks about the validity of the register > number. A pseudo register can be referenced in stabs. > > - The frame_info structure must save the pseudo registers. > The pseudo register (which lies in memory) can be saved by GCC > at entry point of a function. A pseudo register can't lie in memory as it is constructed from one or more real registers. The frame info code needs to be able to construct, on the fly, a pseudo register from a combination of both saved registers and real registers. Lets consider a hypothetical architecture with N real registers (r[0]..r[N]) and M pseudo registers (p0..pN). Each pseudo register is contructed using something from all the real registers vis: p[i] == pseudo (i, r[0], r[1], ...) When the program stops, the inner most stack frame is being examined. Determining a pseudo value is easy as all the real real registers are available - their values can be obtained directly from the register buffer. When you start to move up and down between frames, however, things get messy. Lets look at a typical stack frame. main() calls outer() outer() prologue saves r[1] being used by main() on stack calls middle() middle() prologue saves r[2] being used by outer() on stack calls inner() inner() prologue saves r[1] being used by middle() on stack somewhere in inner's body. If the above program halted, inner() would be the currently selected frame and the register buffer would contain the current registers. When middle()'s frame is selected, all registers except r[1] are in the register buffer. r[1] is found in inner()'s stack frame. When outer()'s frame is selected, all registers except r[1] and r[2] are in the register buffer. r[1] is found in inner()s frame. r[2] is found in middle()'s frame. Or to put it another way, something like: real_register (frame, regnr) for (f = next_inner (frame); f != NULL; f = next_inner (f)) if (REGNR in f->saved_registers) return (saved value of REGNR in f->saved_registers) return value from register buffer (At this point, if you're looking at the code that handles info frame and looking puzzled, yes, that code is wrong. It searches in the wrong direction and has been ever since it was first written. The SPARC window code is ok - it should be possible to merge register windows into this model but I'm leaving that as an exercise for the reader :-) So? Well the fun beings when you go to compute a pseudo register given a frame. In our example, when outer()'s frame is selected. A pseudo for that frame should be computed using: p[i] for frame == pseudo (i, real_register (frame, 0), real_register (frame, 1), ...); which expanding ends up with: == pseudo (i, r[0], saved value of r[1] from inner() frame, saved value of r[2] from middle() frame, ....); Right now this just doesn't happen. Instead: pseudo (i, r[0], r[1], r[2], ...) is computed and then only if you're lucky. enjoy, Andrew PS1: I think everything to do with fetching a register should be parameterized with the frame it is being fetched from. GDB currently has one bunch of functions for accessing the registers in the register buffer and a second set of functions for accessing the registers given a frame. I can't see the difference. PS2: I think the low level register functions should return a ``struct value''.