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