Re: remote watchpoint support

[Andrew Cagney <ac131313@cygnus.com>]

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  <msalter@redhat.com>
> +
> +       * 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 <ac131313@cygnus.com>
To: Stephane Carrez <Stephane.Carrez@worldnet.fr>
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''.