Re: Patch to add breakpoint extension to remote protocol

[jtc@redbacknetworks.com (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