Re: [patch] gdbserver: Add support for Z0/Z1 packets

[Aleksandar Ristovski <aristovski at qnx dot com>]

Pedro Alves wrote:
> On Wednesday 24 June 2009 19:50:44, Aleksandar Ristovski wrote:
> > Pedro Alves wrote:
> > > On Tuesday 23 June 2009 16:17:58, Aleksandar Ristovski wrote:
> > > > Pedro Alves wrote:
> > > > > On Monday 22 June 2009 20:38:50, Aleksandar Ristovski wrote:
> > > > >
> > > > > > > Z0 and Z1 breakpoints also take a 'len' argument, just
> > > > > > > like Z2-Z4.  You should also pass those down.
> > > > > > >
> > > > > > > But, Let's take a step back --- why not just rename the
> > > > > > > insert_watchpoint|remove_watchpoint functions to insert_point,remove_point,
> > > > > > > and relax the type checks in server.c:
> > > > > > But either way is fine with me - just let me know.
> > > > > I'd prefer the approach I suggested, and worry about splitting
> > > > > the breakpoints from watchpoints API if/when we actually need it.
> > > > Ok, then that version is committed.
> > > Well, we had never seen "that" version
> > Ok, to rectify this I am attaching two versions: one if I 
> > revert the changes I committed and the other is diff to what 
> > is in now.
> >
> >  > ... and you bypassed the "rename" suggestion...
> >
> > I did not do any renaming - I think it is not terribly 
> > confusing since both in target.h comment and server.c 'Z' 
> > case it is made very clear that it handles both breakpoints 
> > and watchpoints (i.e. I don't find it any clearer if it was 
> > called "insert_point"... it would still require reading the 
> > comment in target.h)
>
>
> Urgh, I was just about to press the send button when I saw
> this message of yours.  This version corrects a few troubles
> with the previous commit (see ChangeLog) (one of them I still
> see in your new patch) and I've tested it on x86_64-linux.
> Aleksandar, please, please, do run the testsuite (and state
> that you have) when posting patches.  E.g., we could have caught
> the vKill issue that Pierre fixed when we made only linux
> report multi-process (the testsuite runs in "target remote"
> mode, hence with multi-process off most of the way).

And just another small note:

+	    require_running (own_buf);
+	    if (insert && the_target->insert_point != NULL)
+	      res = (*the_target->insert_point) (type, addr, len);
+	    else if (!insert && the_target->remove_point != NULL)
+	      res = (*the_target->remove_point) (type, addr, len);
+	    break;

They should either both be present or none. In the gdb 
document, there is implementation note that reads:

Implementation notes: A remote target shall return an empty 
string for an un-recognized breakpoint or watchpoint packet 
type. A remote target shall support either both or neither 
of a given `Ztype...' and `ztype...' packet pair. To
avoid potential problems with duplicate packets, the 
operations should be imple-mented in an idempotent way.

So, I would make it something like I proposed (if either is 
NULL, it's unsupported - also makes a clear statement to new 
target implementors).


Thanks
--
Aleksandar Ristovski
QNX Software Systems