Re: [gdbserver/patch] Z packet support

Re: [gdbserver/patch] Z packet support

[Paul Schlie]

In the spirit of the present structure of the file, might it make
more sense to actually add decode_Z_packet (... ) to remote-utils.c,
along with possibly the thread stuff, and use it correspondingly in
server.c, before it gets out of hand?

And while at it, move CORE_ADDR tweak server.h to wherever it likely
belongs? (and/or redefine it to void* if more appropriate)?

 /* FIXME: This should probably be autoconf'd for.  It's an integer type at
   least the size of a (void *).  */
  typedef long long CORE_ADDR;


And remove redundant declarations from server.h, and include required
respective headers in server.c

 /* Functions from remote-utils.c */
 void decode_M_packet (char *from, CORE_ADDR * mem_addr_ptr,
              unsigned int *len_ptr, char *to);
 ...
 /* Functions from ``signals.c''.  */
 ...

??? (although I know it's easier said than done?)

Re: [gdbserver/patch] Z packet support

[Daniel Jacobowitz]

On Sun, Jan 30, 2005 at 10:32:30AM -0500, Paul Schlie wrote:
> In the spirit of the present structure of the file, might it make
> more sense to actually add decode_Z_packet (... ) to remote-utils.c,
> along with possibly the thread stuff, and use it correspondingly in
> server.c, before it gets out of hand?
> 
> And while at it, move CORE_ADDR tweak server.h to wherever it likely
> belongs? (and/or redefine it to void* if more appropriate)?
> 
>  /* FIXME: This should probably be autoconf'd for.  It's an integer type at
>    least the size of a (void *).  */
>   typedef long long CORE_ADDR;
> 
> 
> And remove redundant declarations from server.h, and include required
> respective headers in server.c
> 
>  /* Functions from remote-utils.c */
>  void decode_M_packet (char *from, CORE_ADDR * mem_addr_ptr,
>               unsigned int *len_ptr, char *to);
>  ...
>  /* Functions from ``signals.c''.  */
>  ...
> 
> ??? (although I know it's easier said than done?)

If you want to make cosmetic structural changes to gdbserver, and
can show that they actually have benefit, then feel free to file a
copyright assignment and submit patches yourself.

As it happens:
  1. I don't care either way.
  2. Please re-read the comment; it should not be void *.  It is also
     required that it be harmless for CORE_ADDR to be too large; see
     MIPS n32 vs n64.
  3. There are nowhere near enough exported functions to justify
     proliferating headers.

-- 
Daniel Jacobowitz

Re: [gdbserver/patch] Z packet support

[Paul Schlie]

> From: Daniel Jacobowitz <drow@false.org>
> If you want to make cosmetic structural changes to gdbserver, and
> can show that they actually have benefit, then feel free to file a
> copyright assignment and submit patches yourself.
> 
> As it happens:
>   1. I don't care either way.

- understood.

>   2. Please re-read the comment; it should not be void *.  It is also
>      required that it be harmless for CORE_ADDR to be too large; see
>      MIPS n32 vs n64.
[ Regarding: And while at it, move CORE_ADDR tweak server.h to wherever
  it likely belongs? (and/or redefine it to void* if more appropriate)? ]

- sorry, it simply seemed implied by your own comment on the subject:

  "CORE_ADDR is always a long long in gdbserver, so your sizeof (addr)
   probably doesn't work right for 32-bit targets.  I guess sizeof
   (void *) is always right for this, though... at least for the kinds
   of targets gdbserver supports now."

  combined with it's own FIXME comment:

  /* FIXME: This should probably be autoconf'd for.  It's an integer type
   at least the size of a (void *).  */
  typedef long long CORE_ADDR;

>   3. There are nowhere near enough exported functions to justify
>      proliferating headers.

- sorry, guess I always considered "proliferating headers" as required
  a superior alternative to the maintenance problems which proliferating
  redundant declarations otherwise creates.
 
> -- 
> Daniel Jacobowitz

Re: [gdbserver/patch] Z packet support

[Daniel Jacobowitz]

On Sun, Jan 30, 2005 at 10:57:17AM -0500, Paul Schlie wrote:
> >   2. Please re-read the comment; it should not be void *.  It is also
> >      required that it be harmless for CORE_ADDR to be too large; see
> >      MIPS n32 vs n64.
> [ Regarding: And while at it, move CORE_ADDR tweak server.h to wherever
>   it likely belongs? (and/or redefine it to void* if more appropriate)? ]
> 
> - sorry, it simply seemed implied by your own comment on the subject:
> 
>   "CORE_ADDR is always a long long in gdbserver, so your sizeof (addr)
>    probably doesn't work right for 32-bit targets.  I guess sizeof
>    (void *) is always right for this, though... at least for the kinds
>    of targets gdbserver supports now."
> 
>   combined with it's own FIXME comment:
> 
>   /* FIXME: This should probably be autoconf'd for.  It's an integer type
>    at least the size of a (void *).  */
>   typedef long long CORE_ADDR;

See the second sentence? :-)  It has to be an integer type, which void
* is not.  CORE_ADDR has to be at least as large as a target address.

When talking back to GDB, we should endeavour to use the actual size of
a target address, because sometimes 32-bit GDBs will get annoyed when
they receive 64-bit numbers.

> >   3. There are nowhere near enough exported functions to justify
> >      proliferating headers.
> 
> - sorry, guess I always considered "proliferating headers" as required
>   a superior alternative to the maintenance problems which proliferating
>   redundant declarations otherwise creates.

I guess I don't understand you.  In what way are these declarations
"redundant"?

-- 
Daniel Jacobowitz

Re: [gdbserver/patch] Z packet support

[Paul Schlie]

> From: Daniel Jacobowitz <drow@false.org>
>> On Sun, Jan 30, 2005 at 10:57:17AM -0500, Paul Schlie wrote:
>>>   2. Please re-read the comment; it should not be void *.  It is also
>>>      required that it be harmless for CORE_ADDR to be too large; see
>>>      MIPS n32 vs n64.
>> [ Regarding: And while at it, move CORE_ADDR tweak server.h to wherever
>>   it likely belongs? (and/or redefine it to void* if more appropriate)? ]
>> 
>> - sorry, it simply seemed implied by your own comment on the subject:
>> 
>>   "CORE_ADDR is always a long long in gdbserver, so your sizeof (addr)
>>    probably doesn't work right for 32-bit targets.  I guess sizeof
>>    (void *) is always right for this, though... at least for the kinds
>>    of targets gdbserver supports now."
>> 
>>   combined with it's own FIXME comment:
>> 
>>   /* FIXME: This should probably be autoconf'd for.  It's an integer type
>>    at least the size of a (void *).  */
>>   typedef long long CORE_ADDR;
> 
> See the second sentence? :-)  It has to be an integer type, which void
> * is not.  CORE_ADDR has to be at least as large as a target address.

- fair enough, missed that; so Orjan's use of sizeof(addr), which is a
  CORE_ADDR, is basically correct; assuming the host platform's long long
  is long enough as you point out, which seems like a reasonably safe bet?

> When talking back to GDB, we should endeavour to use the actual size of
> a target address, because sometimes 32-bit GDBs will get annoyed when
> they receive 64-bit numbers.
> 
>>>   3. There are nowhere near enough exported functions to justify
>>>      proliferating headers.
>> 
>> - sorry, guess I always considered "proliferating headers" as required
>>   a superior alternative to the maintenance problems which proliferating
>>   redundant declarations otherwise creates.
> 
> I guess I don't understand you.  In what way are these declarations
> "redundant"?

(with respect to "redundant", you're likely correct in questioning my use
 of that word, as I had mistakenly assumed that many of the declarations
 in server.h, also logically appeared in their corresponding source file
 headers; as opposed to being disassociated with them.)

 where for for example in server.h:

   /* Functions from remote-utils.c */

   int putpkt (char *buf);
   ...

where putpkt implementations matching that prototype are defined in both
remote.c and remote-utils.c, but prototyped in remote.h, and server.h;
where it would have seemed simpler and more consistent to declare public
prototypes in the corresponding header files associated with a function's
implementation, which may then be included by source files as required,
thereby establishing a clear dependency, which would seem to help make
file dependency generation, etc. not to mention general clarity affecting
maintainability of the of the sources. (as just an opinion)

> -- 
> Daniel Jacobowitz

Re: [gdbserver/patch] Z packet support

[Daniel Jacobowitz]

On Sun, Jan 30, 2005 at 02:55:15PM -0500, Paul Schlie wrote:
> (with respect to "redundant", you're likely correct in questioning my use
>  of that word, as I had mistakenly assumed that many of the declarations
>  in server.h, also logically appeared in their corresponding source file
>  headers; as opposed to being disassociated with them.)
> 
>  where for for example in server.h:
> 
>    /* Functions from remote-utils.c */
> 
>    int putpkt (char *buf);
>    ...
> 
> where putpkt implementations matching that prototype are defined in both
> remote.c and remote-utils.c, but prototyped in remote.h, and server.h;
> where it would have seemed simpler and more consistent to declare public
> prototypes in the corresponding header files associated with a function's
> implementation, which may then be included by source files as required,
> thereby establishing a clear dependency, which would seem to help make
> file dependency generation, etc. not to mention general clarity affecting
> maintainability of the of the sources. (as just an opinion)

You seem to be somewhat confused about one thing, which explains your
comment better.  GDB and gdbserver are completely independent programs. 
There are a few bits of shared code, e.g. the signals/ directory, but
they are compiled separately for each.

"remote.h" and "remote.c" are part of GDB.  "server.h" and
"remote-utils.c" are is part of gdbserver.  Therefore, there is no
redundancy; the overlap in function names is basically coincidence.
server.h provides prototypes for most of the target-independent files
in gdbserver.

-- 
Daniel Jacobowitz

Re: [gdbserver/patch] Z packet support

[Orjan Friberg]

Daniel Jacobowitz wrote:
> 
> The patch is OK.  Please make sure to include a full changelog entry,
> and update the copyright years in any files you touch.

Apologies for the long turn-around time.  Here is the ChangeLog entry (patch 
unchanged; no copyright year updates were needed).

2005-05-12  Orjan Friberg  <orjanf@axis.com>

	* target.h (struct target_ops): Add insert_watchpoint,
	remove_watchpoint, stopped_by_watchpoint, stopped_data_address function
	pointers for hardware watchpoint support.
	* linux-low.h (struct linux_target_ops): Ditto.
	* linux-low.c (linux_insert_watchpoint, linux_remove_watchpoint)
	(linux_stopped_by_watchpoint, linux_stopped_data_address): New.  Add
	to linux_target_ops.
	* remote-utils.c (prepare_resume_reply): Add watchpoint information to
	reply packet.
	* server.c (main): Recognize 'Z' and 'z' packets.

Committed.

-- 
Orjan Friberg
Axis Communications

Re: [gdbserver/patch] Z packet support

[Daniel Jacobowitz]

On Tue, Feb 01, 2005 at 01:08:07PM +0100, Orjan Friberg wrote:
> Daniel Jacobowitz wrote:
> >
> >You missed the most important problem with the previous posting, I'm
> >afraid - it's still whitespace mangled.  Please don't send patches
> >using format=flowed; it's very good for text, but lousy for data.
> >Either disable it or just attach them.  I've more or less figured out
> >the algorithm by which it ate your patch, but it's pretty tedious to
> >hand-merge; could you resend?
> 
> Sure.  (Until I figure it out, I'll be attaching patches instead of 
> inlining them.  Sorry for the trouble.)
> 
> >CORE_ADDR is always a long long in gdbserver, so your sizeof (addr)
> >probably doesn't work right for 32-bit targets.  I guess sizeof (void
> >*) is always right for this, though... at least for the kinds of
> >targets gdbserver supports now.
> 
> FWIW, recurse.exp and watchpoints.exp ran fine on CRISv32 (32-bit target) 
> when a 64-bit address was sent.  I changed it nonetheless, and tweaked the 
> comment.

I was going to implement the matching bits for i386, but never found
the time.  No point in my holding up your work any longer.  I am still
not clear on how this will handle threads but it sounds like CRIS
kernel bugs mean that that doesn't matter to you; it's irrelevant if
you've only got one copy of the debug registers.  I may come back to
revisit the names of the functions if anyone ever implements hardware
breakpoint support.

The patch is OK.  Please make sure to include a full changelog entry,
and update the copyright years in any files you touch.


-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [gdbserver/patch] Z packet support

[Orjan Friberg]

Orjan Friberg wrote:
> Daniel Jacobowitz wrote:
> 
>>
>> You missed the most important problem with the previous posting, I'm
>> afraid - it's still whitespace mangled.  Please don't send patches
>> using format=flowed; it's very good for text, but lousy for data.
>> Either disable it or just attach them.  I've more or less figured out
>> the algorithm by which it ate your patch, but it's pretty tedious to
>> hand-merge; could you resend?
> 
> 
> Sure.  (Until I figure it out, I'll be attaching patches instead of 
> inlining them.  Sorry for the trouble.)

Two week ping.

-- 
Orjan Friberg
Axis Communications

Re: [gdbserver/patch] Z packet support

[Orjan Friberg]

[-- Attachment #1: Type: text/plain, Size: 965 bytes --]

Daniel Jacobowitz wrote:
> 
> You missed the most important problem with the previous posting, I'm
> afraid - it's still whitespace mangled.  Please don't send patches
> using format=flowed; it's very good for text, but lousy for data.
> Either disable it or just attach them.  I've more or less figured out
> the algorithm by which it ate your patch, but it's pretty tedious to
> hand-merge; could you resend?

Sure.  (Until I figure it out, I'll be attaching patches instead of inlining 
them.  Sorry for the trouble.)

> CORE_ADDR is always a long long in gdbserver, so your sizeof (addr)
> probably doesn't work right for 32-bit targets.  I guess sizeof (void
> *) is always right for this, though... at least for the kinds of
> targets gdbserver supports now.

FWIW, recurse.exp and watchpoints.exp ran fine on CRISv32 (32-bit target) when a 
64-bit address was sent.  I changed it nonetheless, and tweaked the comment.

-- 
Orjan Friberg
Axis Communications


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 6691 bytes --]

Index: target.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/target.h,v
retrieving revision 1.11
diff -u -p -r1.11 target.h
--- target.h	5 Mar 2004 03:43:19 -0000	1.11
+++ target.h	1 Feb 2005 12:02:58 -0000
@@ -133,6 +133,27 @@ struct target_ops
      Read LEN bytes at OFFSET into a buffer at MYADDR.  */
 
   int (*read_auxv) (CORE_ADDR offset, char *myaddr, unsigned int len);
+
+  /* Insert and remove a hardware watchpoint.
+     Returns 0 on success, -1 on failure and 1 on unsupported.  
+     The type is coded as follows:
+       2 = write watchpoint
+       3 = read watchpoint
+       4 = access watchpoint
+  */
+
+  int (*insert_watchpoint) (char type, CORE_ADDR addr, int len);
+  int (*remove_watchpoint) (char type, CORE_ADDR addr, int len);
+
+  /* Returns 1 if target was stopped due to a watchpoint hit, 0 otherwise.  */
+
+  int (*stopped_by_watchpoint) (void);
+
+  /* Returns the address associated with the watchpoint that hit, if any;  
+     returns 0 otherwise.  */
+
+  CORE_ADDR (*stopped_data_address) (void);
+
 };
 
 extern struct target_ops *the_target;
Index: linux-low.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-low.h,v
retrieving revision 1.7
diff -u -p -r1.7 linux-low.h
--- linux-low.h	31 Jan 2004 22:19:32 -0000	1.7
+++ linux-low.h	1 Feb 2005 12:02:58 -0000
@@ -57,6 +57,13 @@ struct linux_target_ops
 
   int decr_pc_after_break;
   int (*breakpoint_at) (CORE_ADDR pc);
+
+  /* Watchpoint related functions.  See target.h for comments.  */
+  int (*insert_watchpoint) (char type, CORE_ADDR addr, int len);
+  int (*remove_watchpoint) (char type, CORE_ADDR addr, int len);
+  int (*stopped_by_watchpoint) (void);
+  CORE_ADDR (*stopped_data_address) (void);
+
 };
 
 extern struct linux_target_ops the_low_target;
Index: linux-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-low.c,v
retrieving revision 1.33
diff -u -p -r1.33 linux-low.c
--- linux-low.c	16 Oct 2004 17:42:00 -0000	1.33
+++ linux-low.c	1 Feb 2005 12:02:58 -0000
@@ -1466,7 +1466,47 @@ linux_read_auxv (CORE_ADDR offset, char 
   return n;
 }
 
-\f
+/* These watchpoint related wrapper functions simply pass on the function call
+   if the target has registered a corresponding function.  */
+
+static int
+linux_insert_watchpoint (char type, CORE_ADDR addr, int len)
+{
+  if (the_low_target.insert_watchpoint != NULL)
+    return the_low_target.insert_watchpoint (type, addr, len);
+  else
+    /* Unsupported (see target.h).  */
+    return 1;
+}
+
+static int
+linux_remove_watchpoint (char type, CORE_ADDR addr, int len)
+{
+  if (the_low_target.remove_watchpoint != NULL)
+    return the_low_target.remove_watchpoint (type, addr, len);
+  else
+    /* Unsupported (see target.h).  */
+    return 1;
+}
+
+static int
+linux_stopped_by_watchpoint (void)
+{
+  if (the_low_target.stopped_by_watchpoint != NULL)
+    return the_low_target.stopped_by_watchpoint ();
+  else
+    return 0;
+}
+
+static CORE_ADDR
+linux_stopped_data_address (void)
+{
+  if (the_low_target.stopped_data_address != NULL)
+    return the_low_target.stopped_data_address ();
+  else
+    return 0;
+}
+
 static struct target_ops linux_target_ops = {
   linux_create_inferior,
   linux_attach,
@@ -1482,6 +1522,10 @@ static struct target_ops linux_target_op
   linux_look_up_symbols,
   linux_send_signal,
   linux_read_auxv,
+  linux_insert_watchpoint,
+  linux_remove_watchpoint,
+  linux_stopped_by_watchpoint,
+  linux_stopped_data_address,
 };
 
 static void
Index: remote-utils.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/remote-utils.c,v
retrieving revision 1.22
diff -u -p -r1.22 remote-utils.c
--- remote-utils.c	16 Oct 2004 17:42:00 -0000	1.22
+++ remote-utils.c	1 Feb 2005 12:02:58 -0000
@@ -639,6 +639,28 @@ prepare_resume_reply (char *buf, char st
   if (status == 'T')
     {
       const char **regp = gdbserver_expedite_regs;
+
+      if (the_target->stopped_by_watchpoint != NULL
+	  && (*the_target->stopped_by_watchpoint) ())
+	{
+	  CORE_ADDR addr;
+	  int i;
+
+	  strncpy (buf, "watch:", 6);
+	  buf += 6;
+
+	  addr = (*the_target->stopped_data_address) ();
+
+	  /* Convert each byte of the address into two hexadecimal chars.
+	     Note that we take sizeof (void *) instead of sizeof (addr);
+	     this is to avoid sending a 64-bit address to a 32-bit GDB.  */
+	  for (i = sizeof (void *) * 2; i > 0; i--)
+	    {
+	      *buf++ = tohex ((addr >> (i - 1) * 4) & 0xf);
+	    }
+	  *buf++ = ';';
+	}
+
       while (*regp)
 	{
 	  buf = outreg (find_regno (*regp), buf);
Index: server.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/server.c,v
retrieving revision 1.22
diff -u -p -r1.22 server.c
--- server.c	5 Mar 2004 03:44:27 -0000	1.22
+++ server.c	1 Feb 2005 12:02:58 -0000
@@ -508,6 +508,66 @@ main (int argc, char *argv[])
 	      signal = mywait (&status, 1);
 	      prepare_resume_reply (own_buf, status, signal);
 	      break;
+	    case 'Z':
+	      {
+		char *lenptr;
+		char *dataptr;
+		CORE_ADDR addr = strtoul (&own_buf[3], &lenptr, 16);
+		int len = strtol (lenptr + 1, &dataptr, 16);
+		char type = own_buf[1];
+
+		if (the_target->insert_watchpoint == NULL
+		    || (type < '2' || type > '4'))
+		  {
+		    /* No watchpoint support or not a watchpoint command;
+		       unrecognized either way.  */
+		    own_buf[0] = '\0';
+		  }
+		else
+		  {
+		    int res;
+
+		    res = (*the_target->insert_watchpoint) (type, addr, len);
+		    if (res == 0)
+		      write_ok (own_buf);
+		    else if (res == 1)
+		      /* Unsupported.  */
+		      own_buf[0] = '\0';
+		    else
+		      write_enn (own_buf);
+		  }
+		break;
+	      }
+	    case 'z':
+	      {
+		char *lenptr;
+		char *dataptr;
+		CORE_ADDR addr = strtoul (&own_buf[3], &lenptr, 16);
+		int len = strtol (lenptr + 1, &dataptr, 16);
+		char type = own_buf[1];
+
+		if (the_target->remove_watchpoint == NULL
+		    || (type < '2' || type > '4'))
+		  {
+		    /* No watchpoint support or not a watchpoint command;
+		       unrecognized either way.  */
+		    own_buf[0] = '\0';
+		  }
+		else
+		  {
+		    int res;
+
+		    res = (*the_target->remove_watchpoint) (type, addr, len);
+		    if (res == 0)
+		      write_ok (own_buf);
+		    else if (res == 1)
+		      /* Unsupported.  */
+		      own_buf[0] = '\0';
+		    else
+		      write_enn (own_buf);
+		  }
+		break;
+	      }
 	    case 'k':
 	      fprintf (stderr, "Killing inferior\n");
 	      kill_inferior ();

Re: [gdbserver/patch] Z packet support

[Daniel Jacobowitz]

On Wed, Jan 12, 2005 at 02:34:33PM +0100, Orjan Friberg wrote:
> Daniel Jacobowitz wrote:
> >
> >I'm fine with the approach.  The implementation needs a bit of work,
> >though.
> 
> There's a new ChangeLog entry (date change only) and patch at the end.  A 
> couple of comments to what you wrote first through:

You missed the most important problem with the previous posting, I'm
afraid - it's still whitespace mangled.  Please don't send patches
using format=flowed; it's very good for text, but lousy for data.
Either disable it or just attach them.  I've more or less figured out
the algorithm by which it ate your patch, but it's pretty tedious to
hand-merge; could you resend?

There's still some formatting glitches, but they're minor, I can just
correct as I edit.  I still want to add x86 support before approving
this so that I can test it.  The patch itself looks right, except:

> >>+	  addr = (*the_target->stopped_data_address) ();
> >>+
> >>+	  *buf++ = tohex ((addr >> 28) & 0xf);
> >>+	  *buf++ = tohex ((addr >> 24) & 0xf);
> >>+	  *buf++ = tohex ((addr >> 20) & 0xf);
> >>+	  *buf++ = tohex ((addr >> 16) & 0xf);
> >>+
> >>+	  *buf++ = tohex ((addr >> 12) & 0xf);
> >>+	  *buf++ = tohex ((addr >> 8) & 0xf);
> >>+	  *buf++ = tohex ((addr >> 4) & 0xf);
> >>+	  *buf++ = tohex (addr & 0xf);
> >>+
> >>+	  *buf++ = ';';
> >>+	}
> >
> >
> >This is broken on 64-bit targets.  This will require a certain amount
> >of shuffling to figure out the proper size of an address.
> 
> Living comfortably in a 32-bit world, I may have missed what you mean by "a 
> certain amount of shuffling".  I imagined a simple sizeof (addr) would do.

CORE_ADDR is always a long long in gdbserver, so your sizeof (addr)
probably doesn't work right for 32-bit targets.  I guess sizeof (void
*) is always right for this, though... at least for the kinds of
targets gdbserver supports now.

> >  - Some of the z/Z commands are breakpoints, not watchpoints.  Please
> >    only invoke watchpoint methods for watchpoint commands.
> 
> I assume you mean that I should exclude memory breakpoint and hardware 
> breakpoint.  Anyway, that's what I did.

Yep.

-- 
Daniel Jacobowitz

Re: [gdbserver/patch] Z packet support

[Orjan Friberg]

Daniel Jacobowitz wrote:
> 
> I'm fine with the approach.  The implementation needs a bit of work,
> though.

There's a new ChangeLog entry (date change only) and patch at the end.  A couple 
of comments to what you wrote first through:

>>+	  addr = (*the_target->stopped_data_address) ();
>>+
>>+	  *buf++ = tohex ((addr >> 28) & 0xf);
>>+	  *buf++ = tohex ((addr >> 24) & 0xf);
>>+	  *buf++ = tohex ((addr >> 20) & 0xf);
>>+	  *buf++ = tohex ((addr >> 16) & 0xf);
>>+
>>+	  *buf++ = tohex ((addr >> 12) & 0xf);
>>+	  *buf++ = tohex ((addr >> 8) & 0xf);
>>+	  *buf++ = tohex ((addr >> 4) & 0xf);
>>+	  *buf++ = tohex (addr & 0xf);
>>+
>>+	  *buf++ = ';';
>>+	}
> 
> 
> This is broken on 64-bit targets.  This will require a certain amount
> of shuffling to figure out the proper size of an address.

Living comfortably in a 32-bit world, I may have missed what you mean by "a 
certain amount of shuffling".  I imagined a simple sizeof (addr) would do.

>   - Some of the z/Z commands are breakpoints, not watchpoints.  Please
>     only invoke watchpoint methods for watchpoint commands.

I assume you mean that I should exclude memory breakpoint and hardware 
breakpoint.  Anyway, that's what I did.


2005-01-12  Orjan Friberg  <orjanf@axis.com>

	* target.h (struct target_ops): Add insert_watchpoint,
	remove_watchpoint, stopped_by_watchpoint, stopped_data_address function
	pointers for hardware watchpoint support.
	* linux-low.h (struct linux_target_ops): Ditto.
	* linux-low.c (linux_insert_watchpoint, linux_remove_watchpoint)
	(linux_stopped_by_watchpoint, linux_stopped_data_address): New.  Add
	to linux_target_ops.
	* remote-utils.c (prepare_resume_reply): Add watchpoint information to
	reply packet.
	* server.c (main): Recognize 'Z' and 'z' packets.


Index: target.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/target.h,v
retrieving revision 1.11
diff -u -p -r1.11 target.h
--- target.h    5 Mar 2004 03:43:19 -0000       1.11
+++ target.h    12 Jan 2005 13:23:55 -0000
@@ -133,6 +133,27 @@ struct target_ops
       Read LEN bytes at OFFSET into a buffer at MYADDR.  */

    int (*read_auxv) (CORE_ADDR offset, char *myaddr, unsigned int len);
+
+  /* Insert and remove a hardware watchpoint.
+     Returns 0 on success, -1 on failure and 1 on unsupported.
+     The type is coded as follows:
+       2 = write watchpoint
+       3 = read watchpoint
+       4 = access watchpoint
+  */
+
+  int (*insert_watchpoint) (char type, CORE_ADDR addr, int len);
+  int (*remove_watchpoint) (char type, CORE_ADDR addr, int len);
+
+  /* Returns 1 if target was stopped due to a watchpoint hit, 0 otherwise.  */
+
+  int (*stopped_by_watchpoint) (void);
+
+  /* Returns the address associated with the watchpoint that hit, if any;
+     returns 0 otherwise.  */
+
+  CORE_ADDR (*stopped_data_address) (void);
+
  };

  extern struct target_ops *the_target;
Index: linux-low.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-low.h,v
retrieving revision 1.7
diff -u -p -r1.7 linux-low.h
--- linux-low.h 31 Jan 2004 22:19:32 -0000      1.7
+++ linux-low.h 12 Jan 2005 13:23:55 -0000
@@ -57,6 +57,13 @@ struct linux_target_ops

    int decr_pc_after_break;
    int (*breakpoint_at) (CORE_ADDR pc);
+
+  /* Watchpoint related functions.  See target.h for comments.  */
+  int (*insert_watchpoint) (char type, CORE_ADDR addr, int len);
+  int (*remove_watchpoint) (char type, CORE_ADDR addr, int len);
+  int (*stopped_by_watchpoint) (void);
+  CORE_ADDR (*stopped_data_address) (void);
+
  };

  extern struct linux_target_ops the_low_target;
Index: linux-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-low.c,v
retrieving revision 1.33
diff -u -p -r1.33 linux-low.c
--- linux-low.c 16 Oct 2004 17:42:00 -0000      1.33
+++ linux-low.c 12 Jan 2005 13:23:55 -0000
@@ -1466,7 +1466,47 @@ linux_read_auxv (CORE_ADDR offset, char
    return n;
  }

-

+/* These watchpoint related wrapper functions simply pass on the function call
+   if the target has registered a corresponding function.  */
+
+static int
+linux_insert_watchpoint (char type, CORE_ADDR addr, int len)
+{
+  if (the_low_target.insert_watchpoint != NULL)
+    return the_low_target.insert_watchpoint (type, addr, len);
+  else
+    /* Unsupported (see target.h).  */
+    return 1;
+}
+
+static int
+linux_remove_watchpoint (char type, CORE_ADDR addr, int len)
+{
+  if (the_low_target.remove_watchpoint != NULL)
+    return the_low_target.remove_watchpoint (type, addr, len);
+  else
+    /* Unsupported (see target.h).  */
+    return 1;
+}
+
+static int
+linux_stopped_by_watchpoint (void)
+{
+  if (the_low_target.stopped_by_watchpoint != NULL)
+    return the_low_target.stopped_by_watchpoint ();
+  else
+    return 0;
+}
+
+static CORE_ADDR
+linux_stopped_data_address (void)
+{
+  if (the_low_target.stopped_data_address != NULL)
+    return the_low_target.stopped_data_address ();
+  else
+    return 0;
+}
+
  static struct target_ops linux_target_ops = {
    linux_create_inferior,
    linux_attach,
@@ -1482,6 +1522,10 @@ static struct target_ops linux_target_op
    linux_look_up_symbols,
    linux_send_signal,
    linux_read_auxv,
+  linux_insert_watchpoint,
+  linux_remove_watchpoint,
+  linux_stopped_by_watchpoint,
+  linux_stopped_data_address,
  };

  static void
Index: remote-utils.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/remote-utils.c,v
retrieving revision 1.22
diff -u -p -r1.22 remote-utils.c
--- remote-utils.c      16 Oct 2004 17:42:00 -0000      1.22
+++ remote-utils.c      12 Jan 2005 13:23:56 -0000
@@ -639,6 +639,26 @@ prepare_resume_reply (char *buf, char st
    if (status == 'T')
      {
        const char **regp = gdbserver_expedite_regs;
+
+      if (the_target->stopped_by_watchpoint != NULL
+         && (*the_target->stopped_by_watchpoint) ())
+       {
+         CORE_ADDR addr;
+         int i;
+
+         strncpy (buf, "watch:", 6);
+         buf += 6;
+
+         addr = (*the_target->stopped_data_address) ();
+
+         /* Convert each byte of the address into two hexadecimal chars.  */
+         for (i = sizeof (addr) * 2; i > 0; i--)
+           {
+             *buf++ = tohex ((addr >> (i - 1) * 4) & 0xf);
+           }
+         *buf++ = ';';
+       }
+
        while (*regp)
         {
           buf = outreg (find_regno (*regp), buf);
Index: server.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/server.c,v
retrieving revision 1.22
diff -u -p -r1.22 server.c
--- server.c    5 Mar 2004 03:44:27 -0000       1.22
+++ server.c    12 Jan 2005 13:23:56 -0000
@@ -508,6 +508,66 @@ main (int argc, char *argv[])
               signal = mywait (&status, 1);
               prepare_resume_reply (own_buf, status, signal);
               break;
+           case 'Z':
+             {
+               char *lenptr;
+               char *dataptr;
+               CORE_ADDR addr = strtoul (&own_buf[3], &lenptr, 16);
+               int len = strtol (lenptr + 1, &dataptr, 16);
+               char type = own_buf[1];
+
+               if (the_target->insert_watchpoint == NULL
+                   || (type < '2' || type > '4'))
+                 {
+                   /* No watchpoint support or not a watchpoint command;
+                      unrecognized either way.  */
+                   own_buf[0] = '\0';
+                 }
+               else
+                 {
+                   int res;
+
+                   res = (*the_target->insert_watchpoint) (type, addr, len);
+                   if (res == 0)
+                     write_ok (own_buf);
+                   else if (res == 1)
+                     /* Unsupported.  */
+                     own_buf[0] = '\0';
+                   else
+                     write_enn (own_buf);
+                 }
+               break;
+             }
+           case 'z':
+             {
+               char *lenptr;
+               char *dataptr;
+               CORE_ADDR addr = strtoul (&own_buf[3], &lenptr, 16);
+               int len = strtol (lenptr + 1, &dataptr, 16);
+               char type = own_buf[1];
+
+               if (the_target->remove_watchpoint == NULL
+                   || (type < '2' || type > '4'))
+                 {
+                   /* No watchpoint support or not a watchpoint command;
+                      unrecognized either way.  */
+                   own_buf[0] = '\0';
+                 }
+               else
+                 {
+                   int res;
+
+                   res = (*the_target->remove_watchpoint) (type, addr, len);
+                   if (res == 0)
+                     write_ok (own_buf);
+                   else if (res == 1)
+                     /* Unsupported.  */
+                     own_buf[0] = '\0';
+                   else
+                     write_enn (own_buf);
+                 }
+               break;
+             }
             case 'k':
               fprintf (stderr, "Killing inferior\n");
               kill_inferior ();

-- 
Orjan Friberg
Axis Communications

Re: [gdbserver/patch] Z packet support

[Orjan Friberg]

Daniel Jacobowitz wrote:
> 
> I'm fine with the approach.  The implementation needs a bit of work,
> though.

Thanks for your comments (I agree with all of them); I will produce a new patch 
shortly (I thought I had the GNU coding standard hard-wired into my brain by now 
but obviously I didn't).

-- 
Orjan Friberg
Axis Communications

Re: [gdbserver/patch] Z packet support

[Daniel Jacobowitz]

On Wed, Dec 01, 2004 at 04:12:43PM +0100, Orjan Friberg wrote:
> A while ago there was a brief discussion concerning Z packet support in the 
> gdbserver (starting at 
> http://sources.redhat.com/ml/gdb-patches/2004-05/msg00706.html) where it 
> was suggested that maybe the watchpoint code should be shared with GDB.
> 
> Well, I implemented Z packet support in the most straight-forward way ever: 
> separate from GDB (and I have an upcoming CRISv32 port which would use it). 
> I don't know if the various watchpoint functions need to change to 
> accommodate other architectures (if memory serves me correctly there were 
> some issues with the s390 on the host side regarding "stopped data 
> address").

I'm fine with the approach.  The implementation needs a bit of work,
though.

First of all, your mailer has mangled the patch.  A lot of leading
whitespace is messed up.  Could you fix that?  I'd like to be able to
play with this for i386 before approving it.

Secondly (presumably a separate problem), you've lost a lot of
whitespace before open parentheses.

> +  /* Watchpoint related functions.  */
> +  int (*insert_watchpoint)(char type, CORE_ADDR addr, int len);
> +  int (*remove_watchpoint)(char type, CORE_ADDR addr, int len);
> +  int (*stopped_by_watchpoint) (void);
> +  CORE_ADDR (*stopped_data_address) (void);

More detailed comments, please - at least for the target.h copy.  For
instance, what are the return values?

> +static int linux_insert_watchpoint (char type, CORE_ADDR addr, int len)
> +{
> +  if (the_low_target.insert_watchpoint != NULL)
> +    return the_low_target.insert_watchpoint (type, addr, len);
> +  else
> +    return -1;
> +}

Formatting; function names start in column 0.  Also, this block of
wrapper functions could use at least one comment.

> Index: remote-utils.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/remote-utils.c,v
> retrieving revision 1.22
> diff -u -p -r1.22 remote-utils.c
> --- remote-utils.c	16 Oct 2004 17:42:00 -0000	1.22
> +++ remote-utils.c	1 Dec 2004 14:49:02 -0000
> @@ -639,6 +639,30 @@ prepare_resume_reply (char *buf, char st
>    if (status == 'T')
>      {
>        const char **regp = gdbserver_expedite_regs;
> +
> +      if (*the_target->stopped_by_watchpoint != NULL

I'm surprised that compiles.... the * there is spurious.

> +	  && (*the_target->stopped_by_watchpoint) ())
> +	{
> +	  CORE_ADDR addr;
> +
> +	  strncpy (buf, "watch:", 6);
> +	  buf += 6;
> +
> +	  addr = (*the_target->stopped_data_address) ();
> +
> +	  *buf++ = tohex ((addr >> 28) & 0xf);
> +	  *buf++ = tohex ((addr >> 24) & 0xf);
> +	  *buf++ = tohex ((addr >> 20) & 0xf);
> +	  *buf++ = tohex ((addr >> 16) & 0xf);
> +
> +	  *buf++ = tohex ((addr >> 12) & 0xf);
> +	  *buf++ = tohex ((addr >> 8) & 0xf);
> +	  *buf++ = tohex ((addr >> 4) & 0xf);
> +	  *buf++ = tohex (addr & 0xf);
> +
> +	  *buf++ = ';';
> +	}

This is broken on 64-bit targets.  This will require a certain amount
of shuffling to figure out the proper size of an address.

> +	    case 'Z':
> +	      {
> +		char *lenptr;
> +		char *dataptr;
> +		CORE_ADDR addr = strtoul(&own_buf[3], &lenptr, 16);
> +		int len = strtol(lenptr + 1, &dataptr, 16);
> +		char type = own_buf[1];
> +
> +		set_desired_inferior (0);
> +		if (the_target->insert_watchpoint != NULL
> +		    && ((*the_target->insert_watchpoint) (type, addr, len)
> +			== 0))
> +		  write_ok (own_buf);
> +		else
> +		  own_buf[0] = '\0';
> +		break;
> +	      }

There's two problems here:

  - Some of the z/Z commands are breakpoints, not watchpoints.  Please
    only invoke watchpoint methods for watchpoint commands.

  - An empty string is supposed to represent an unrecognized "type". 
    We should distinguish error from unsupported.

Why the call to set_desired_inferior?  This should presumably affect
all threads, since that's how GDB behaves (or is supposed to). 
Most likely that should be handled in the linux-low wrappers.

-- 
Daniel Jacobowitz

[gdbserver/patch] Z packet support

[Orjan Friberg]

A while ago there was a brief discussion concerning Z packet support in the 
gdbserver (starting at 
http://sources.redhat.com/ml/gdb-patches/2004-05/msg00706.html) where it was 
suggested that maybe the watchpoint code should be shared with GDB.

Well, I implemented Z packet support in the most straight-forward way ever: 
separate from GDB (and I have an upcoming CRISv32 port which would use it).  I 
don't know if the various watchpoint functions need to change to accommodate 
other architectures (if memory serves me correctly there were some issues with 
the s390 on the host side regarding "stopped data address").


2004-12-01  Orjan Friberg  <orjanf@axis.com>

	* target.h (struct target_ops): Add insert_watchpoint,
	remove_watchpoint, stopped_by_watchpoint, stopped_data_address function
	pointers for hardware watchpoint support.
	* linux-low.h (struct linux_target_ops): Ditto.
	* linux-low.c (linux_insert_watchpoint, linux_remove_watchpoint)
	(linux_stopped_by_watchpoint, linux_stopped_data_address): New.  Add
	to linux_target_ops.
	* remote-utils.c (prepare_resume_reply): Add watchpoint information to
	reply packet.
	* server.c (main): Recognize 'Z' and 'z' packets.


Index: target.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/target.h,v
retrieving revision 1.11
diff -u -p -r1.11 target.h
--- target.h	5 Mar 2004 03:43:19 -0000	1.11
+++ target.h	1 Dec 2004 14:49:02 -0000
@@ -133,6 +133,13 @@ struct target_ops
       Read LEN bytes at OFFSET into a buffer at MYADDR.  */

    int (*read_auxv) (CORE_ADDR offset, char *myaddr, unsigned int len);
+
+  /* Watchpoint related functions.  */
+  int (*insert_watchpoint)(char type, CORE_ADDR addr, int len);
+  int (*remove_watchpoint)(char type, CORE_ADDR addr, int len);
+  int (*stopped_by_watchpoint) (void);
+  CORE_ADDR (*stopped_data_address) (void);
+
  };

  extern struct target_ops *the_target;
Index: linux-low.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-low.h,v
retrieving revision 1.7
diff -u -p -r1.7 linux-low.h
--- linux-low.h	31 Jan 2004 22:19:32 -0000	1.7
+++ linux-low.h	1 Dec 2004 14:49:02 -0000
@@ -57,6 +57,13 @@ struct linux_target_ops

    int decr_pc_after_break;
    int (*breakpoint_at) (CORE_ADDR pc);
+
+  /* Watchpoint related functions.  */
+  int (*insert_watchpoint) (char type, CORE_ADDR addr, int len);
+  int (*remove_watchpoint) (char type, CORE_ADDR addr, int len);
+  int (*stopped_by_watchpoint) (void);
+  CORE_ADDR (*stopped_data_address) (void);
+
  };

  extern struct linux_target_ops the_low_target;
Index: linux-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-low.c,v
retrieving revision 1.33
diff -u -p -r1.33 linux-low.c
--- linux-low.c	16 Oct 2004 17:42:00 -0000	1.33
+++ linux-low.c	1 Dec 2004 14:49:02 -0000
@@ -1466,7 +1466,38 @@ linux_read_auxv (CORE_ADDR offset, char
    return n;
  }

-\f
+static int linux_insert_watchpoint (char type, CORE_ADDR addr, int len)
+{
+  if (the_low_target.insert_watchpoint != NULL)
+    return the_low_target.insert_watchpoint (type, addr, len);
+  else
+    return -1;
+}
+
+static int linux_remove_watchpoint (char type, CORE_ADDR addr, int len)
+{
+  if (the_low_target.remove_watchpoint != NULL)
+    return the_low_target.remove_watchpoint (type, addr, len);
+  else
+    return -1;
+}
+
+static int linux_stopped_by_watchpoint (void)
+{
+  if (the_low_target.stopped_by_watchpoint != NULL)
+    return the_low_target.stopped_by_watchpoint ();
+  else
+    return 0;
+}
+
+static CORE_ADDR linux_stopped_data_address (void)
+{
+  if (the_low_target.stopped_data_address != NULL)
+    return the_low_target.stopped_data_address ();
+  else
+    return 0;
+}
+
  static struct target_ops linux_target_ops = {
    linux_create_inferior,
    linux_attach,
@@ -1482,6 +1513,10 @@ static struct target_ops linux_target_op
    linux_look_up_symbols,
    linux_send_signal,
    linux_read_auxv,
+  linux_insert_watchpoint,
+  linux_remove_watchpoint,
+  linux_stopped_by_watchpoint,
+  linux_stopped_data_address,
  };

  static void
Index: remote-utils.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/remote-utils.c,v
retrieving revision 1.22
diff -u -p -r1.22 remote-utils.c
--- remote-utils.c	16 Oct 2004 17:42:00 -0000	1.22
+++ remote-utils.c	1 Dec 2004 14:49:02 -0000
@@ -639,6 +639,30 @@ prepare_resume_reply (char *buf, char st
    if (status == 'T')
      {
        const char **regp = gdbserver_expedite_regs;
+
+      if (*the_target->stopped_by_watchpoint != NULL
+	  && (*the_target->stopped_by_watchpoint) ())
+	{
+	  CORE_ADDR addr;
+
+	  strncpy (buf, "watch:", 6);
+	  buf += 6;
+
+	  addr = (*the_target->stopped_data_address) ();
+
+	  *buf++ = tohex ((addr >> 28) & 0xf);
+	  *buf++ = tohex ((addr >> 24) & 0xf);
+	  *buf++ = tohex ((addr >> 20) & 0xf);
+	  *buf++ = tohex ((addr >> 16) & 0xf);
+
+	  *buf++ = tohex ((addr >> 12) & 0xf);
+	  *buf++ = tohex ((addr >> 8) & 0xf);
+	  *buf++ = tohex ((addr >> 4) & 0xf);
+	  *buf++ = tohex (addr & 0xf);
+
+	  *buf++ = ';';
+	}
+
        while (*regp)
  	{
  	  buf = outreg (find_regno (*regp), buf);
Index: server.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/server.c,v
retrieving revision 1.22
diff -u -p -r1.22 server.c
--- server.c	5 Mar 2004 03:44:27 -0000	1.22
+++ server.c	1 Dec 2004 14:49:02 -0000
@@ -508,6 +508,41 @@ main (int argc, char *argv[])
  	      signal = mywait (&status, 1);
  	      prepare_resume_reply (own_buf, status, signal);
  	      break;
+	    case 'Z':
+	      {
+		char *lenptr;
+		char *dataptr;
+		CORE_ADDR addr = strtoul(&own_buf[3], &lenptr, 16);
+		int len = strtol(lenptr + 1, &dataptr, 16);
+		char type = own_buf[1];
+
+		set_desired_inferior (0);
+		if (the_target->insert_watchpoint != NULL
+		    && ((*the_target->insert_watchpoint) (type, addr, len)
+			== 0))
+		  write_ok (own_buf);
+		else
+		  own_buf[0] = '\0';
+		break;
+	      }
+	    case 'z':
+	      {
+		char *lenptr;
+		char *dataptr;
+		CORE_ADDR addr = strtoul(&own_buf[3], &lenptr, 16);
+		int len = strtol(lenptr + 1, &dataptr, 16);
+		char type = own_buf[1];
+
+		set_desired_inferior (0);
+
+		if (the_target->remove_watchpoint != NULL
+		    && ((*the_target->remove_watchpoint) (type, addr, len)
+			== 0))
+		  write_ok (own_buf);
+		else
+		  own_buf[0] = '\0';
+		break;
+	      }
  	    case 'k':
  	      fprintf (stderr, "Killing inferior\n");
  	      kill_inferior ();

-- 
Orjan Friberg
Axis Communications