This is the mail archive of the gdb-patches@sourceware.cygnus.com mailing list for the GDB project. See the GDB home page for more information.
| Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
|---|---|---|
| Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
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