Re: Patch to add breakpoint extension to remote protocol

Re: Patch to add breakpoint extension to remote protocol

[Andrew Cagney]

Hello,

"J.T. Conklin" wrote:

> The enclosed patch adds insert and remove breakpoint commands to the
> GDB remote protocol.  The protocol changes are as I described in the
> messages I sent to the gdb list over the last few weeks.

Here, finally (:-( ) is some feedback.

Please take a look at each of the issues raised below (you'll notice that I've only commented
on how remote.c implements the protocol).

> I created a X86 embedded target configuration (embed.mt / tm-embed.h)
> that inherits from the sysv config, but then adds the definitions for
> hardware watchpoints and breakpoints.  I believe that the other X86
> embeded targets (i386-coff and i386-elf) should also use the embed
> config, but I have not make that change since I am unable to test it.

Perhaphs separate this out into a second patch once the remote.c changes are in.


> 1998-12-15  J.T. Conklin  <jtc@redbacknetworks.com>
>
>         * remote.c (stub_supports_B): New variable, set to 1 if stub
>         supports the new insert and remove breakpoint commands.
>         (remote_insert_breakpoint, remote_remove_breakpoint): If
>         stub_supports_B is set, attempt to use use the insert and remove
>         breakpoint commands.  If we receive a empty response, the stub
>         does not support the new commands.  In that case, stub_supports_B
>         is reset and breakpoints will be inserted and removed by writing
>         breakpoint insns as before.
>         (remote_insert_watchpoint, remote_remove_watchpoint,
>         remote_insert_hw_watchpoint, remote_remove_hw_watchpoint): New
>         functions, present if TARGET_HAS_HARDWARE_WATCHPOINTS.
>         (remote_open_1): Set stub_supports_B to 1.

Re: int stub_supports_B:


remote.c (for stub_supports_P and now stub_supports_B) assumes that any rejected P packet
should be interpreted as as an indication that the P (B) packet is no longer supported.  This
assumption is wrong.  A target that accepted an initial P (B) packet should continue to accept
such packets and we really should ensure that this occures.  (A change of mind is an error and
should result in an abort and not a fallback to some other mechanism).

Could I encourage you to take the oportunity to investigate this.  An excert of a patch I've
been playing with is below (please ignore the stub_supports_P -> stub_supports_P_packet
rename):

*** remote.c    1998/10/02 19:54:44     1.162
--- remote.c    1999/02/16 05:12:23
*************** static int remote_address_size;
*** 362,371 ****

  static int remote_register_buf_size = 0;

! /* Should we try the 'P' request?  If this is set to one when the stub
!    doesn't support 'P', the only consequence is some unnecessary traffic.  */
! static int stub_supports_P = 1;

  /* These are pointers to hook functions that may be set in order to
     modify resume/wait behavior for a particular architecture.  */

--- 362,384 ----

  static int remote_register_buf_size = 0;

! /* Some stubs do not support all packet types. For such packets, they
!    are initially marked as SUPPORT_UNKNOWN.  Upon first use, depending
!    on the response of the remote stub that is changed to either
!    SUPPORTED or UNSUPPORTED. It is important that once used
!    (especially for B packets) that once used they packet continues to
!    be used. */

+ enum packet_support { PACKET_UNSUPPORTED = 0, PACKET_SUPPORT_UNKNOWN,
+ PACKET_SUPPORTED };
+
+ /* Should we use the 'P' packet? */
+ enum packet_support stub_supports_P_packet;
+
+ /* Should we use the B packet? */
+ enum packet_support stub_supports_B_packet;
+
+
  /* These are pointers to hook functions that may be set in order to
     modify resume/wait behavior for a particular architecture.  */

*************** device is attached to the remote system
*** 633,642 ****
      }
    push_target (target);       /* Switch to using remote target now */

!   /* Start out by trying the 'P' request to set registers.  We set this each
!      time that we open a new target so that if the user switches from one
!      stub to another, we can (if the target is closed and reopened) cope.  */
!   stub_supports_P = 1;

    general_thread = -2;
    cont_thread = -2;
--- 646,657 ----
      }
    push_target (target);       /* Switch to using remote target now */

!   /* For optionally supported packets, start out not assuming
!      anything.  We set this each time that we open a new target so
!      that if the user switches from one stub to another, we can (if
!      the target is closed and reopened) cope.  */
!   stub_supports_P_packet = PACKET_SUPPORT_UNKNOWN;
!   stub_supports_B_packet = PACKET_SUPPORT_UNKNOWN;

    general_thread = -2;
    cont_thread = -2;
*************** remote_store_registers (regno)
*** 1091,1097 ****

    set_thread (inferior_pid, 1);

!   if (regno >= 0 && stub_supports_P)
      {
        /* Try storing a single register.  */
        char *regp;
--- 1106,1112 ----

    set_thread (inferior_pid, 1);

!   if (regno >= 0 && stub_supports_P_packet != PACKET_UNSUPPORTED)
      {
        /* Try storing a single register.  */
        char *regp;
*************** remote_store_registers (regno)
*** 1106,1121 ****
        }
        *p = '\0';
        remote_send (buf);
        if (buf[0] != '\0')
        {
          /* The stub understands the 'P' request.  We are done.  */
          return;
        }
-
        /* The stub does not support the 'P' request.  Use 'G' instead,
         and don't try using 'P' in the future (it will just waste our
         time).  */
!       stub_supports_P = 0;
      }

    buf[0] = 'G';
--- 1121,1139 ----
        }
        *p = '\0';
        remote_send (buf);
+       if (stub_supports_P_packet == PACKET_SUPPORTED)
+       /* should we check buf[0] for a protocol error? */
+       return;
        if (buf[0] != '\0')
        {
          /* The stub understands the 'P' request.  We are done.  */
+         stub_supports_P_packet = PACKET_SUPPORTED;
          return;
        }
        /* The stub does not support the 'P' request.  Use 'G' instead,
         and don't try using 'P' in the future (it will just waste our
         time).  */
!       stub_supports_P_packet = PACKET_UNSUPPORTED;
      }

    buf[0] = 'G';


----


> ***************
> *** 90,95 ****
> --- 90,117 ----
>                                         where only part of the data was
>                                         written).
>
> +       insert break    Bt,AA..AA[,LLLL]
> +       or watchpoint                   t is type: 0 - software breakpoint,
> +                                       1 - hardware breakpoint, 2 - write
> +                                       watchpoint, 3 - read watchpoint, 4 -
> +                                       access watchpoint;
> +                                       AA..AA is address;
> +                                       LLLL is number of bytes
> +       reply           OK              for success
> +                       ENN             for an error
> +       (not supported by all stubs).
> +
> +       remove break    bt,AA..AA[,LLLL]
> +       or watchpoint                   t is type: 0 - software breakpoint,
> +                                       1 - hardware breakpoint, 2 - write
> +                                       watchpoint, 3 - read watchpoint, 4 -
> +                                       access watchpoint;
> +                                       AA..AA is address;
> +                                       LLLL is number of bytes
> +       reply           OK              for success
> +                       ENN             for an error
> +       (not supported by all stubs).
> +
>         continue        cAA..AA         AA..AA is address to resume
>                                         If AA..AA is omitted,
>                                         resume at same address.

Several questions:

o    Must the insert/remote value (t,AAA[,LLL]) for a given breakpoint match? (I assume yes)

o    For software breakpoints, would a memory inspection of the breakpoint address return the
pre-breakpoint value? (I assume yes)


Re: sprintf (buf, "B%x,%lx,%lx", type + 2, (long) addr, len);

Have a look at remote_write_bytes() and remote_address_masked().  It handles addresses of up
to 64 bits.


Re: #ifdef TARGET_HAS_HARDWARE_WATCHPOINTS

Don't worry about #ifdefing all the hardware watchpoint code out.  I'd instead just make
certain that stub_supports_B is set to something that stops GDB trying to use it.


Re: general

(I'm probably rambling)  Should there be a `set remote*' command to control the use of the B
and P packets?  Just in case we find that we really do/don't want GDB using either of these
protocol extensions?

Sorry this has taken so long,

        Andrew

Re: Patch to add breakpoint extension to remote protocol

[J.T. Conklin]

>>>>> "cagney" == Andrew Cagney <ac131313@cygnus.com> writes:
>> The enclosed patch adds insert and remove breakpoint commands to
>> the GDB remote protocol.  The protocol changes are as I described
>> in the messages I sent to the gdb list over the last few weeks.

cagney> Here, finally (:-( ) is some feedback.

Thanks,

cagney> Please take a look at each of the issues raised below (you'll
cagney> notice that I've only commented on how remote.c implements the
cagney> protocol).

Unfortunately, that's the easy part.  I'm having reservations about
adding statefullness to the remote stub given the weaknesses in the
protocol.  More on this at the end of this message.

>> I created a X86 embedded target configuration (embed.mt /
>> tm-embed.h) that inherits from the sysv config, but then adds the
>> definitions for hardware watchpoints and breakpoints.  I believe
>> that the other X86 embeded targets (i386-coff and i386-elf) should
>> also use the embed config, but I have not make that change since I
>> am unable to test it.

cagney> Perhaphs separate this out into a second patch once the
cagney> remote.c changes are in.

I agree, this can wait until there is reason that sysv and embedded
configurations diverge.  If nothing else turns up that requires the
new config, it can wait until hardware watchpoint support is
integrated.

cagney> Re: int stub_supports_B:
cagney> remote.c (for stub_supports_P and now stub_supports_B) assumes
cagney> that any rejected P packet should be interpreted as as an
cagney> indication that the P (B) packet is no longer supported.  This
cagney> assumption is wrong.  A target that accepted an initial P (B)
cagney> packet should continue to accept such packets and we really
cagney> should ensure that this occures.  (A change of mind is an
cagney> error and should result in an abort and not a fallback to some
cagney> other mechanism).

I tend to agree with this.  A stub should be expected to support the
calls it provides at all times.  A call can fail (ie. return E...),
but should not go un-recognized.


cagney> Could I encourage you to take the oportunity to investigate
cagney> this.  An excert of a patch I've been playing with is below
cagney> (please ignore the stub_supports_P -> stub_supports_P_packet
cagney> rename):

Are you asking me to integrate this into my patch?  Do you want a
patch with only the stub_supports_P_packet changes first?

cagney> Several questions:

cagney> o Must the insert/remote value (t,AAA[,LLL]) for a given
cagney> breakpoint match? (I assume yes)

Yes.  

My original implementation returned a cookie when a breakpoint was
inserted.  The cookie was then used to identify the breakpoint when it
was removed.  However, this added a bunch of hair to both the stub and
GDB as both sides were required to maintain a data structure recording
the relationship between cookies and type/address/length tuples.  And 
since GDB manages watch/breakpoints as type/address/length internally
anyway, it seemed easiest to lose the extra layer of abstraction the
cookie provided and use type/address/length directly.

cagney> o For software breakpoints, would a memory inspection of the
cagney> breakpoint address return the pre-breakpoint value? (I assume
cagney> yes)

Yes.

In my implementation, the stub remove breakpoints/restores memory just
after it obtains control, and it inserts breakpoints just before it
restores control to the program.

cagney> Re: sprintf (buf, "B%x,%lx,%lx", type + 2, (long) addr, len);

cagney> Have a look at remote_write_bytes() and
cagney> remote_address_masked().  It handles addresses of up to 64
cagney> bits.

Will do.

cagney> Re: general
cagney> (I'm probably rambling) Should there be a `set remote*'
cagney> command to control the use of the B and P packets?  Just in
cagney> case we find that we really do/don't want GDB using either of
cagney> these protocol extensions?

Although new variables that control whether B and P packets should be
used wouldn't be difficult to add, I can't see a situation that would
require them given a robust protocol (which we have) and implement-
ation (which GDB has, and any stub should provide).



Back to the statefullness issue I mentioned earlier.  

Although I'm currently using GDB over fairly reliable serial links;
there is nothing that guarentees that a reply will be munged and a
command (like insert breakpoint) be executed twice.  There doesn't
seem to be a solution to this problem other than to add sequence
numbers to the protocol.  Unfortunately, the current specification of
sequence numbers in the protocol is inadequate to protect against
duplicated packets.

This, plus changes like adding thread stuff into the 'q' escape
instead of making threads `first class objects' in the protocol;
adding 'X' for binary memory write instead of a true binary packet
mode; etc. has me thinking that it's time to re-design the remote
protocol from the ground up.

Here are some characteristics I think a new protocol/implementation
should contain, in no particular order.

* Able to sit on top of both datagram and stream based transport layers.
  The least common denominator being a small datagram.  Protocol should
  be able negotiate a larger packet size to minimize overhead for bulk
  transfers.

  ? transport layer should handle providing framing, quoting, etc. to
    provide an 8 bit clean path?

  ? transport layer independent from protocol

* Supports ascii and binary modes (Perhaps ascii is no longer needed;
  I don't think many people are computing checksums by hand.  And if
  transport layer provides 8 bit clean path...)

* Sequence numbers to handle duplicate and lost packets.

  ? Or is that something we require of the transport layer?

* As lightweight as possible.  Should be able to execute with only the
  equivalent of g/G, m/M and s/c commands.

  ? The current protocol/stub allows remote debugging support to be added
    to very small target systems.  A new protocol shouldn't preclude running
    on those same systems.

* New stub should be as easy to integrate as the old one.  It's pretty
  trival to integrate the stub into an existing target.

* Provide a stub library that contains the guts of the protocol
  engine that's linked with CPU/target specific routines.  We have a half 
  dozen stubs that are distributed with GDB, where not even the "common"
  code is exactly the same.  In usenet, there are periodic calls for
  additional stubs (ppc, arm, etc.) that are not distributed with GDB.  A
  library might be able to address both of these problems.

* threads as first class objects.  You should be able to put a breakpoint
  in a thread, and not have the stub return unless that thread hits the
  breakpoint (as opposed to the current scheme where GDB regains control
  each time the breakpoint is hit, and then it verifies what thread is
  running).

* Stub can be queried to determine what CPU varient is used on the
  target, so that GDB can reconfigure itself automatically.

* Defined error cases.  Currently there is 'EXX', but 'XX' isn't defined
  to mean anything.

* Richer exception / signal reporting.  I find it particularly annoying
  that all CPU exceptions are mapped into the POSIX signals.  It makes 
  it difficult to distinguish between different types of exception.  I
  realize that using CPU exception types within GDB would require more
  changes, perhaps more than can be hoped for now.  But the protocol
  should return the real exception and have GDB translate it into a
  POSIX signal value, that would allow GDB to be enhanced later.


An IP based transport would be relatively easy.  For serial lines, we
could do a fake `SL/IP + IP + UDP' transport small enough to fit in a
stub, while targets that have their own networking capability could 
use UDP directly (over whatever media is supported).

One consequence of separating the transport from the protocol would be
that there needs to be a way to integrate different transport layers
into GDB, and the user would need a way to specify the new remote
protocol with whatever transport is 


Required function calls:
	* get target information (CPU, FPU, etc.)
	* set memory
	* get memory
	* set register(s)
	* get register(s)
	* continue
	* step (on cpu's that support single step)

* Optional function calls
	* insert watch/breakpoint
	* remove watch/breakpoint

* Optional thread related calls
	* get current thread
	* get thread info
	* get thread list
	* set context 
		+ Are future commands interpreted relative to the 
		  entire system, or a particular thread.

* Optional high level memory manipulation calls
	* zero memory region
	* move memory region
	* checksum memory region


Sorry for rambling, but I'd be interested in your thoughts nonetheless.

	--jtc

-- 
J.T. Conklin
RedBack Networks

Re: Patch to add breakpoint extension to remote protocol

[J.T. Conklin]

>>>>> "cagney" == Andrew Cagney <ac131313@cygnus.com> writes:
>> The enclosed patch adds insert and remove breakpoint commands to
>> the GDB remote protocol.  The protocol changes are as I described
>> in the messages I sent to the gdb list over the last few weeks.

cagney> Here, finally (:-( ) is some feedback.

Thanks,

cagney> Please take a look at each of the issues raised below (you'll
cagney> notice that I've only commented on how remote.c implements the
cagney> protocol).

Unfortunately, that's the easy part.  I'm having reservations about
adding statefullness to the remote stub given the weaknesses in the
protocol.  More on this at the end of this message.

>> I created a X86 embedded target configuration (embed.mt /
>> tm-embed.h) that inherits from the sysv config, but then adds the
>> definitions for hardware watchpoints and breakpoints.  I believe
>> that the other X86 embeded targets (i386-coff and i386-elf) should
>> also use the embed config, but I have not make that change since I
>> am unable to test it.

cagney> Perhaphs separate this out into a second patch once the
cagney> remote.c changes are in.

I agree, this can wait until there is reason that sysv and embedded
configurations diverge.  If nothing else turns up that requires the
new config, it can wait until hardware watchpoint support is
integrated.

cagney> Re: int stub_supports_B:
cagney> remote.c (for stub_supports_P and now stub_supports_B) assumes
cagney> that any rejected P packet should be interpreted as as an
cagney> indication that the P (B) packet is no longer supported.  This
cagney> assumption is wrong.  A target that accepted an initial P (B)
cagney> packet should continue to accept such packets and we really
cagney> should ensure that this occures.  (A change of mind is an
cagney> error and should result in an abort and not a fallback to some
cagney> other mechanism).

I tend to agree with this.  A stub should be expected to support the
calls it provides at all times.  A call can fail (ie. return E...),
but should not go un-recognized.


cagney> Could I encourage you to take the oportunity to investigate
cagney> this.  An excert of a patch I've been playing with is below
cagney> (please ignore the stub_supports_P -> stub_supports_P_packet
cagney> rename):

Are you asking me to integrate this into my patch?  Do you want a
patch with only the stub_supports_P_packet changes first?

cagney> Several questions:

cagney> o Must the insert/remote value (t,AAA[,LLL]) for a given
cagney> breakpoint match? (I assume yes)

Yes.  

My original implementation returned a cookie when a breakpoint was
inserted.  The cookie was then used to identify the breakpoint when it
was removed.  However, this added a bunch of hair to both the stub and
GDB as both sides were required to maintain a data structure recording
the relationship between cookies and type/address/length tuples.  And 
since GDB manages watch/breakpoints as type/address/length internally
anyway, it seemed easiest to lose the extra layer of abstraction the
cookie provided and use type/address/length directly.

cagney> o For software breakpoints, would a memory inspection of the
cagney> breakpoint address return the pre-breakpoint value? (I assume
cagney> yes)

Yes.

In my implementation, the stub remove breakpoints/restores memory just
after it obtains control, and it inserts breakpoints just before it
restores control to the program.

cagney> Re: sprintf (buf, "B%x,%lx,%lx", type + 2, (long) addr, len);

cagney> Have a look at remote_write_bytes() and
cagney> remote_address_masked().  It handles addresses of up to 64
cagney> bits.

Will do.

cagney> Re: general
cagney> (I'm probably rambling) Should there be a `set remote*'
cagney> command to control the use of the B and P packets?  Just in
cagney> case we find that we really do/don't want GDB using either of
cagney> these protocol extensions?

Although new variables that control whether B and P packets should be
used wouldn't be difficult to add, I can't see a situation that would
require them given a robust protocol (which we have) and implement-
ation (which GDB has, and any stub should provide).



Back to the statefullness issue I mentioned earlier.  

Although I'm currently using GDB over fairly reliable serial links;
there is nothing that guarentees that a reply will be munged and a
command (like insert breakpoint) be executed twice.  There doesn't
seem to be a solution to this problem other than to add sequence
numbers to the protocol.  Unfortunately, the current specification of
sequence numbers in the protocol is inadequate to protect against
duplicated packets.

This, plus changes like adding thread stuff into the 'q' escape
instead of making threads `first class objects' in the protocol;
adding 'X' for binary memory write instead of a true binary packet
mode; etc. has me thinking that it's time to re-design the remote
protocol from the ground up.

Here are some characteristics I think a new protocol/implementation
should contain, in no particular order.

* Able to sit on top of both datagram and stream based transport layers.
  The least common denominator being a small datagram.  Protocol should
  be able negotiate a larger packet size to minimize overhead for bulk
  transfers.

  ? transport layer should handle providing framing, quoting, etc. to
    provide an 8 bit clean path?

  ? transport layer independent from protocol

* Supports ascii and binary modes (Perhaps ascii is no longer needed;
  I don't think many people are computing checksums by hand.  And if
  transport layer provides 8 bit clean path...)

* Sequence numbers to handle duplicate and lost packets.

  ? Or is that something we require of the transport layer?

* As lightweight as possible.  Should be able to execute with only the
  equivalent of g/G, m/M and s/c commands.

  ? The current protocol/stub allows remote debugging support to be added
    to very small target systems.  A new protocol shouldn't preclude running
    on those same systems.

* New stub should be as easy to integrate as the old one.  It's pretty
  trival to integrate the stub into an existing target.

* Provide a stub library that contains the guts of the protocol
  engine that's linked with CPU/target specific routines.  We have a half 
  dozen stubs that are distributed with GDB, where not even the "common"
  code is exactly the same.  In usenet, there are periodic calls for
  additional stubs (ppc, arm, etc.) that are not distributed with GDB.  A
  library might be able to address both of these problems.

* threads as first class objects.  You should be able to put a breakpoint
  in a thread, and not have the stub return unless that thread hits the
  breakpoint (as opposed to the current scheme where GDB regains control
  each time the breakpoint is hit, and then it verifies what thread is
  running).

* Stub can be queried to determine what CPU varient is used on the
  target, so that GDB can reconfigure itself automatically.

* Defined error cases.  Currently there is 'EXX', but 'XX' isn't defined
  to mean anything.

* Richer exception / signal reporting.  I find it particularly annoying
  that all CPU exceptions are mapped into the POSIX signals.  It makes 
  it difficult to distinguish between different types of exception.  I
  realize that using CPU exception types within GDB would require more
  changes, perhaps more than can be hoped for now.  But the protocol
  should return the real exception and have GDB translate it into a
  POSIX signal value, that would allow GDB to be enhanced later.


An IP based transport would be relatively easy.  For serial lines, we
could do a fake `SL/IP + IP + UDP' transport small enough to fit in a
stub, while targets that have their own networking capability could 
use UDP directly (over whatever media is supported).

One consequence of separating the transport from the protocol would be
that there needs to be a way to integrate different transport layers
into GDB, and the user would need a way to specify the new remote
protocol with whatever transport is 


Required function calls:
	* get target information (CPU, FPU, etc.)
	* set memory
	* get memory
	* set register(s)
	* get register(s)
	* continue
	* step (on cpu's that support single step)

* Optional function calls
	* insert watch/breakpoint
	* remove watch/breakpoint

* Optional thread related calls
	* get current thread
	* get thread info
	* get thread list
	* set context 
		+ Are future commands interpreted relative to the 
		  entire system, or a particular thread.

* Optional high level memory manipulation calls
	* zero memory region
	* move memory region
	* checksum memory region


Sorry for rambling, but I'd be interested in your thoughts nonetheless.

	--jtc

-- 
J.T. Conklin
RedBack Networks

Re: Patch to add breakpoint extension to remote protocol

[Andrew Cagney]

"J.T. Conklin" wrote:

> cagney> Could I encourage you to take the oportunity to investigate
> cagney> this.  An excert of a patch I've been playing with is below
> cagney> (please ignore the stub_supports_P -> stub_supports_P_packet
> cagney> rename):
>
> Are you asking me to integrate this into my patch?  Do you want a
> patch with only the stub_supports_P_packet changes first?

I'm not worried.  If you're able test/debug both P and B changes and submit a
singe patch I'll be more than grateful.

> cagney> Re: sprintf (buf, "B%x,%lx,%lx", type + 2, (long) addr, len);
>
> cagney> Have a look at remote_write_bytes() and
> cagney> remote_address_masked().  It handles addresses of up to 64
> cagney> bits.
>
> Will do.
>
> cagney> Re: general
> cagney> (I'm probably rambling) Should there be a `set remote*'
> cagney> command to control the use of the B and P packets?  Just in
> cagney> case we find that we really do/don't want GDB using either of
> cagney> these protocol extensions?
>
> Although new variables that control whether B and P packets should be
> used wouldn't be difficult to add, I can't see a situation that would
> require them given a robust protocol (which we have) and implement-
> ation (which GDB has, and any stub should provide).

True.  I can however see the situtation where GDB encounters a target that has
a broken implementation which we don't want to use.


> Back to the statefullness issue I mentioned earlier.
>
> Although I'm currently using GDB over fairly reliable serial links;
> there is nothing that guarentees that a reply will be munged and a
> command (like insert breakpoint) be executed twice.  There doesn't
> seem to be a solution to this problem other than to add sequence
> numbers to the protocol.  Unfortunately, the current specification of
> sequence numbers in the protocol is inadequate to protect against
> duplicated packets.

Yes, the current protocol makes the assumption that the only transmition error
that can occure is data overrun (and an ack ("+" or "-") can't overrun a
buffer.  Suprisingly this assumption holds up very well.

Should the semantics for the set/clear breakpoint commands include an
idenpotent clause?  I can't see any problems with doing this.  (It won't help
other commands such as RUN but it does at least tie this one down better).

> This, plus changes like adding thread stuff into the 'q' escape
> instead of making threads `first class objects' in the protocol;
> adding 'X' for binary memory write instead of a true binary packet
> mode; etc. has me thinking that it's time to re-design the remote
> protocol from the ground up.

Just FYI, I believe that part of the rationale behind the X packet was:

    o    only memory transfers impose a significant overhead

    o    it is strongly preferable to have a protocol based on ASCII (snooping,
debugging...)


> Here are some characteristics I think a new protocol/implementation
> should contain, in no particular order.
>
> * Able to sit on top of both datagram and stream based transport layers.
>   The least common denominator being a small datagram.  Protocol should
>   be able negotiate a larger packet size to minimize overhead for bulk
>   transfers.

Yes, this is an important problem.

> Sorry for rambling, but I'd be interested in your thoughts nonetheless.

My memory from the last time this was discussed is that there were two schools
of thought (aren't there always :-):

    o    The existing GDB protocol works.
        Lets just evolve it a little, spac filler here, sand paper there, ...

    o    The existing protocol should be frozen and we should start again from
scratch.
        What we need is a new protocol for a new century, ...

Technically the latter has much merit.  I think most older GDB engineers
reconise each of the problems you identified.
If it was me doing the work, however, I'd actually prefer the former!  Mainly
because I think that there are other areas of GDB that could do with more
immediate attention :-(

    enjoy,

        Andrew

Re: Patch to add breakpoint extension to remote protocol

[Andrew Cagney]

"J.T. Conklin" wrote:

> cagney> Could I encourage you to take the oportunity to investigate
> cagney> this.  An excert of a patch I've been playing with is below
> cagney> (please ignore the stub_supports_P -> stub_supports_P_packet
> cagney> rename):
>
> Are you asking me to integrate this into my patch?  Do you want a
> patch with only the stub_supports_P_packet changes first?

I'm not worried.  If you're able test/debug both P and B changes and submit a
singe patch I'll be more than grateful.

> cagney> Re: sprintf (buf, "B%x,%lx,%lx", type + 2, (long) addr, len);
>
> cagney> Have a look at remote_write_bytes() and
> cagney> remote_address_masked().  It handles addresses of up to 64
> cagney> bits.
>
> Will do.
>
> cagney> Re: general
> cagney> (I'm probably rambling) Should there be a `set remote*'
> cagney> command to control the use of the B and P packets?  Just in
> cagney> case we find that we really do/don't want GDB using either of
> cagney> these protocol extensions?
>
> Although new variables that control whether B and P packets should be
> used wouldn't be difficult to add, I can't see a situation that would
> require them given a robust protocol (which we have) and implement-
> ation (which GDB has, and any stub should provide).

True.  I can however see the situtation where GDB encounters a target that has
a broken implementation which we don't want to use.


> Back to the statefullness issue I mentioned earlier.
>
> Although I'm currently using GDB over fairly reliable serial links;
> there is nothing that guarentees that a reply will be munged and a
> command (like insert breakpoint) be executed twice.  There doesn't
> seem to be a solution to this problem other than to add sequence
> numbers to the protocol.  Unfortunately, the current specification of
> sequence numbers in the protocol is inadequate to protect against
> duplicated packets.

Yes, the current protocol makes the assumption that the only transmition error
that can occure is data overrun (and an ack ("+" or "-") can't overrun a
buffer.  Suprisingly this assumption holds up very well.

Should the semantics for the set/clear breakpoint commands include an
idenpotent clause?  I can't see any problems with doing this.  (It won't help
other commands such as RUN but it does at least tie this one down better).

> This, plus changes like adding thread stuff into the 'q' escape
> instead of making threads `first class objects' in the protocol;
> adding 'X' for binary memory write instead of a true binary packet
> mode; etc. has me thinking that it's time to re-design the remote
> protocol from the ground up.

Just FYI, I believe that part of the rationale behind the X packet was:

    o    only memory transfers impose a significant overhead

    o    it is strongly preferable to have a protocol based on ASCII (snooping,
debugging...)


> Here are some characteristics I think a new protocol/implementation
> should contain, in no particular order.
>
> * Able to sit on top of both datagram and stream based transport layers.
>   The least common denominator being a small datagram.  Protocol should
>   be able negotiate a larger packet size to minimize overhead for bulk
>   transfers.

Yes, this is an important problem.

> Sorry for rambling, but I'd be interested in your thoughts nonetheless.

My memory from the last time this was discussed is that there were two schools
of thought (aren't there always :-):

    o    The existing GDB protocol works.
        Lets just evolve it a little, spac filler here, sand paper there, ...

    o    The existing protocol should be frozen and we should start again from
scratch.
        What we need is a new protocol for a new century, ...

Technically the latter has much merit.  I think most older GDB engineers
reconise each of the problems you identified.
If it was me doing the work, however, I'd actually prefer the former!  Mainly
because I think that there are other areas of GDB that could do with more
immediate attention :-(

    enjoy,

        Andrew

Patch to add breakpoint extension to remote protocol

[J.T. Conklin]

The enclosed patch adds insert and remove breakpoint commands to the
GDB remote protocol.  The protocol changes are as I described in the
messages I sent to the gdb list over the last few weeks.

Since the i386-aout config directly used the i386-sysv configuration,
I created a X86 embedded target configuration (embed.mt / tm-embed.h)
that inherits from the sysv config, but then adds the definitions for
hardware watchpoints and breakpoints.  I believe that the other X86
embeded targets (i386-coff and i386-elf) should also use the embed
config, but I have not make that change since I am unable to test it.

	--jtc

1998-12-15  J.T. Conklin  <jtc@redbacknetworks.com>

	* remote.c (stub_supports_B): New variable, set to 1 if stub
	supports the new insert and remove breakpoint commands.
	(remote_insert_breakpoint, remote_remove_breakpoint): If
	stub_supports_B is set, attempt to use use the insert and remove
	breakpoint commands.  If we receive a empty response, the stub
	does not support the new commands.  In that case, stub_supports_B
	is reset and breakpoints will be inserted and removed by writing
	breakpoint insns as before.
	(remote_insert_watchpoint, remote_remove_watchpoint,
	remote_insert_hw_watchpoint, remote_remove_hw_watchpoint): New
	functions, present if TARGET_HAS_HARDWARE_WATCHPOINTS.
	(remote_open_1): Set stub_supports_B to 1.

	* config/i386/embed.mt: New file.
	* config/i386/tm-embed.h: New file.
	* configure.tgt: Changed i[3456]86-*-aout* to use embed.mt.

Index: tools-src/gdb/gdb/configure.tgt
diff -c tools-src/gdb/gdb/configure.tgt:1.1.1.1 tools-src/gdb/gdb/configure.tgt:1.1.1.1.2.1
*** tools-src/gdb/gdb/configure.tgt:1.1.1.1	Wed Dec  2 16:05:09 1998
--- tools-src/gdb/gdb/configure.tgt	Wed Dec  9 15:09:53 1998
***************
*** 83,89 ****
  i[3456]86-sequent-sysv4*) gdb_target=ptx4 ;;
  i[3456]86-sequent-sysv*) gdb_target=ptx ;;
  i[3456]86-ncr-*)	gdb_target=ncr3000 ;;
! i[3456]86-*-aout*)	gdb_target=i386aout ;;
  i[3456]86-*-coff*)	gdb_target=i386v ;;
  i[3456]86-*-elf*)	gdb_target=i386v ;;
  i[3456]86-*-aix*)	gdb_target=i386aix ;;
--- 83,89 ----
  i[3456]86-sequent-sysv4*) gdb_target=ptx4 ;;
  i[3456]86-sequent-sysv*) gdb_target=ptx ;;
  i[3456]86-ncr-*)	gdb_target=ncr3000 ;;
! i[3456]86-*-aout*)	gdb_target=embed ;;
  i[3456]86-*-coff*)	gdb_target=i386v ;;
  i[3456]86-*-elf*)	gdb_target=i386v ;;
  i[3456]86-*-aix*)	gdb_target=i386aix ;;
Index: tools-src/gdb/gdb/remote.c
diff -c tools-src/gdb/gdb/remote.c:1.1.1.1 tools-src/gdb/gdb/remote.c:1.1.1.1.2.3
*** tools-src/gdb/gdb/remote.c:1.1.1.1	Wed Dec  2 16:05:20 1998
--- tools-src/gdb/gdb/remote.c	Tue Dec 15 15:46:37 1998
***************
*** 90,95 ****
--- 90,117 ----
  					where only part of the data was
  					written).
  
+ 	insert break	Bt,AA..AA[,LLLL]
+ 	or watchpoint			t is type: 0 - software breakpoint, 
+ 					1 - hardware breakpoint, 2 - write
+ 					watchpoint, 3 - read watchpoint, 4 -
+ 					access watchpoint;
+ 					AA..AA is address;
+ 					LLLL is number of bytes
+ 	reply		OK		for success
+ 			ENN		for an error
+ 	(not supported by all stubs).
+ 					
+ 	remove break	bt,AA..AA[,LLLL]
+ 	or watchpoint			t is type: 0 - software breakpoint, 
+ 					1 - hardware breakpoint, 2 - write
+ 					watchpoint, 3 - read watchpoint, 4 -
+ 					access watchpoint;
+ 					AA..AA is address;
+ 					LLLL is number of bytes
+ 	reply		OK		for success
+ 			ENN		for an error
+ 	(not supported by all stubs).
+ 
  	continue	cAA..AA		AA..AA is address to resume
  					If AA..AA is omitted,
  					resume at same address.
***************
*** 387,392 ****
--- 409,419 ----
  
  static int remote_register_buf_size = 0;
  
+ /* Should we try the 'b'/'B' requests?  If this is set to one when the
+    stub doesn't support 'b'/'B', the only consequence is some
+    unecessary traffic. */
+ static int stub_supports_B = 1;
+ 
  /* Should we try the 'P' request?  If this is set to one when the stub
     doesn't support 'P', the only consequence is some unnecessary traffic.  */
  static int stub_supports_P = 1;
***************
*** 1678,1686 ****
       so we use this function to call back into the thread module and
       register the thread vector and its contained functions. */
    bind_target_thread_vector(&remote_thread_vec);
!   /* Start out by trying the 'P' request to set registers.  We set this each
!      time that we open a new target so that if the user switches from one
!      stub to another, we can (if the target is closed and reopened) cope.  */
    stub_supports_P = 1;
  
    general_thread = -2;
--- 1705,1717 ----
       so we use this function to call back into the thread module and
       register the thread vector and its contained functions. */
    bind_target_thread_vector(&remote_thread_vec);
! 
!   /* Start out by trying the 'B' request for stub-managed breakpoints
!      and the 'P' request to set registers.  We set these each time
!      that we open a new target so that if the user switches from one
!      stub to another, we can (if the target is closed and reopened)
!      cope.  */
!   stub_supports_B = 1;
    stub_supports_P = 1;
  
    general_thread = -2;
***************
*** 3012,3022 ****
       CORE_ADDR addr;
       char *contents_cache;
  {
  #ifdef REMOTE_BREAKPOINT
    int val;
  
!   val = target_read_memory (addr, contents_cache, sizeof big_break_insn);
  
    if (val == 0)
      {
        if (TARGET_BYTE_ORDER == BIG_ENDIAN)
--- 3043,3068 ----
       CORE_ADDR addr;
       char *contents_cache;
  {
+   char buf[PBUFSIZ];
  #ifdef REMOTE_BREAKPOINT
    int val;
+ #endif
  
!   if (stub_supports_B) 
!     {
!       sprintf (buf, "B0,%lx", (long) addr);
!       putpkt (buf);
!       getpkt (buf, 0);
! 
!       if (buf[0] != '\0') 
! 	return (buf[0] == 'E');
  
+       /* The stub does not support the 'B' request. */
+       stub_supports_B = 0;
+     }
+ 
+ #ifdef REMOTE_BREAKPOINT
+   val = target_read_memory (addr, contents_cache, sizeof big_break_insn);
    if (val == 0)
      {
        if (TARGET_BYTE_ORDER == BIG_ENDIAN)
***************
*** 3038,3049 ****
--- 3084,3179 ----
       CORE_ADDR addr;
       char *contents_cache;
  {
+   char buf[PBUFSIZ];
+ 
+   if (stub_supports_B)
+     {
+       sprintf (buf, "b0,%lx", (long) addr);
+       putpkt (buf);
+       getpkt (buf, 0);
+ 
+       return (buf[0] == 'E');
+     }
+ 
  #ifdef REMOTE_BREAKPOINT
    return target_write_memory (addr, contents_cache, sizeof big_break_insn);
  #else
    return memory_remove_breakpoint (addr, contents_cache);
  #endif /* REMOTE_BREAKPOINT */
  }
+ 
+ #ifdef TARGET_HAS_HARDWARE_WATCHPOINTS
+ int
+ remote_insert_watchpoint (addr, len, type)
+      CORE_ADDR addr;
+      int len;
+      int type;
+ {
+   char buf[PBUFSIZ];
+   
+   sprintf (buf, "B%x,%lx,%lx", type + 2, (long) addr, len);
+   putpkt (buf);
+   getpkt (buf, 0);
+ 
+   if (buf[0] == '\0' || buf [0] == 'E')
+     return -1;
+ 
+   return 0;
+ }
+ 
+ int
+ remote_remove_watchpoint (addr, len, type)
+      CORE_ADDR addr;
+      int len;
+      int type;
+ {
+   char buf[PBUFSIZ];
+   
+   sprintf (buf, "b%x,%lx,%lx", type + 2, (long) addr, len);
+   putpkt (buf);
+   getpkt (buf, 0);
+ 
+   if (buf[0] == '\0' || buf [0] == 'E')
+     return -1;
+ 
+   return 0;
+ }
+ 
+ int
+ remote_insert_hw_breakpoint (addr, len)
+      CORE_ADDR addr;
+      int len;
+ {
+   char buf[PBUFSIZ];
+ 
+   sprintf (buf, "B1,%lx", (long) addr);
+   putpkt (buf);
+   getpkt (buf, 0);
+ 
+   if (buf[0] == '\0' || buf [0] == 'E')
+     return -1;
+ 
+   return 0;
+ }
+ 
+ int 
+ remote_remove_hw_breakpoint (addr, len)
+      CORE_ADDR addr;
+      int len;
+ {
+   char buf[PBUFSIZ];
+ 
+   sprintf (buf, "b1,%lx", (long) addr);
+   putpkt(buf);
+   getpkt (buf, 0);
+   
+   if (buf[0] == '\0' || buf [0] == 'E')
+     return -1;
+ 
+   return 0;
+ }
+ #endif
+ 
  
  /* Some targets are only capable of doing downloads, and afterwards they switch
     to the remote serial protocol.  This function provides a clean way to get
Index: tools-src/gdb/gdb/config/i386/embed.mt
diff -c /dev/null tools-src/gdb/gdb/config/i386/embed.mt:1.1.2.1
*** /dev/null	Tue Dec 15 16:15:14 1998
--- tools-src/gdb/gdb/config/i386/embed.mt	Wed Dec  9 15:10:00 1998
***************
*** 0 ****
--- 1,3 ----
+ # Target: Intel 386 embedded target
+ TDEPFILES= i386-tdep.o
+ TM_FILE= tm-embed.h
Index: tools-src/gdb/gdb/config/i386/tm-embed.h
diff -c /dev/null tools-src/gdb/gdb/config/i386/tm-embed.h:1.1.2.1
*** /dev/null	Tue Dec 15 16:15:15 1998
--- tools-src/gdb/gdb/config/i386/tm-embed.h	Wed Dec  9 15:10:01 1998
***************
*** 0 ****
--- 1,47 ----
+ /* Macro definitions for i386, embedded targets.
+    Copyright 1998 Free Software Foundation, Inc.
+ 
+ This file is part of GDB.
+ 
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 2 of the License, or
+ (at your option) any later version.
+ 
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ GNU General Public License for more details.
+ 
+ You should have received a copy of the GNU General Public License
+ along with this program; if not, write to the Free Software
+ Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.  */
+ 
+ #ifndef TM_EMBED_H
+ #define TM_EMBED_H 1
+ 
+ #include "i386/tm-i386v.h"
+ 
+ /* Remote protocol can use hardware debugging registers */
+ #define TARGET_HAS_HARDWARE_WATCHPOINTS
+ 
+ #define TARGET_CAN_USE_HARDWARE_WATCHPOINT(type, cnt, ot) 1
+ 
+ /* After a watchpoint trap, the PC points to the instruction after the
+    one that caused the trap.  Therefore we don't need to step over it.
+    But we do need to reset the status register to avoid another trap. */
+ #define HAVE_CONTINUABLE_WATCHPOINT
+ 
+ #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)
+ 
+ #define target_insert_hw_breakpoint(addr, len) \
+ 	remote_insert_hw_breakpoint(addr, len)
+ 
+ #define target_remove_hw_breakpoint(addr, len) \
+ 	remote_remove_hw_breakpoint(addr, len)
+ 
+ #endif /* ifndef TM_EMBED_H */