Re: Patch to add breakpoint extension to remote protocol

[Andrew Cagney <ac131313@cygnus.com>]

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