Re: [PATCH] Add autoload-breakpoints [7/7] autoload-breakpoints doc

[Hui Zhu <hui_zhu@mentor.com>]

Hi Eli,

Thanks for you review.

On 03/17/12 18:43, Eli Zaretskii wrote:
>> Date: Sat, 17 Mar 2012 16:53:19 +0800
>> From: Hui Zhu<hui_zhu@mentor.com>
>> CC: Stan Shebs<stan_shebs@mentor.com>
>>
>> This patch add the doc for the autoload-breakpoints.
>
> Thanks.
>
>> +If the remote stub support, @value{GDBN} can get autoload-breakpoints from
>> +remote stub.
>
>   @value{GDBN} can receive the information about autoload-breakpoints
>   from the remote stub, if the stub supports that.
>
> Also, as we don't mention autoload-breakpoints anywhere else in the
> manual, we need to explain here what these breakpoints are and why
> controlling them is useful.

What about:
Some stub support set breakpoints with itself, with this function, GDB 
can handle the breakpoint that set by the stub better.


>
>> +@item set breakpoint autoload query
>> +If this option is query (the default), @value{GDBN} will query to user
>                       ^^^^^
> @samp{query}
>
>> +how to handle the autoload-breakpints when @value{GDBN} connect to the stub.
>                                ^^^^^^^^^^                   ^^^^^^^
> A typo.  Also, "connects".
>
>> +@item set breakpoint autoload merge
>> +If this option is merge, the autoload-breakpoints of @value{GDBN}
>                       ^^^^^
> @samp{merge}
>
>> +and stub will merge together when @value{GDBN} connect to stub.
>
> I don't understand what it means to "merge" breakpoints.

It means that merge together.  Keep both the autoload-breakpoints in the 
GDB and the stub.

>
>> +@item set breakpoint autoload gdb
>> +If this option is gdb, the autoload-breakpoints of stub will be removed
>                       ^^^
> @samp{gdb}
>
>> +when GDB connect to stub.
>          ^^^
> @value{GDBN}
>
>> +@item set breakpoint autoload stub
>> +If this option is stub, the autoload-breakpoints of GDB will be removed
>                       ^^^^                              ^^^
> @samp{stub} and @value{GDBN}
>
>> +@node Autoload-breakpoints Format
>> +@section Autoload-breakpoints Format
>> +@cindex Autoload-breakpoints Format
>> +
>> +@subsection Autoload-breakpoints base format
>
> Please don't add subsections without a node (and a menu in the parent
> section).  Such subsections cannot be navigated with on-line Info
> commands.

Should I change it to:
@node Autoload-breakpoints base format
@subsection Autoload-breakpoints base format
@cindex Autoload-breakpoints base format

>
>> +Autoload-breakpoints base format describe the operation of
>                                      ^^^^^^^^
> "describes"
>
>> +the autoload-breakpints in @value{GDBN} and the stub.
>                  ^^^^^^^^^^
> A typo.
>
> Anyway, it sounds like this describes the format of the packet, not of
> autoload-breakpoints or something similar.  Therefore, I think it
> should be part of the packet description, not a separate section and
> not in this place in the manual.
>
>> +@cindex @var{id}@samp{:}@var{command}@samp{:}@var{addr_string}@samp{:}@var{type}@samp{:}@var{ignore_num} packet
>
> This index entry is useless; please remove it.
>
>> +@table @samp
>> +@item @var{id}
>> +This is the id in hex string format of this command want to control.
>
>   This is the number, as a hex string, of the autoload-breakpoint that
>   this command wants to control.
>
>> +@item @var{command}
>> +This is command char. Include E (enable) and D (disable).
>
>   This is the command character, either @samp{E} (for ``enable'') or
>   @samp{D} (for ``disable'').
>
>> +If this autoload-breakpoint @var{id} is not exist, create one and
>> +enable or disable it.
>
>   If the autoload-breakpoint @var{id} does not exist, create one and
>   enable or disable it.
>
>>                         If it is exist, item follow it will be ignore
>> +and just disable and enable the autoload-breakpoints.
>
>   If it does exist, the following items will be ignored, and the
>   autoload-breakpoint will be enabled or disabled as specified by
>   @var{command}.
>
>> +@item @var{addr_string}
>> +If need create an autoload-breakpoint, this is the address string
>> +that encoded in hex string.
>
>   This is the address of an autoload-breakpoint to create, encoded as a
>   hex string.
>
>> +@item @var{type}
>> +If need create an autoload-breakpoint, this is the type in char
>> +include H (hardware) and S (software).
>
>   This is the type of the autoload-breakpoint to create, either
>   @samp{H} (for ``hardware'') or @samp{S} (for ``software'').
>
>> +@item @var{ignore_num}
>> +If need create an autoload-breakpoint, this is the ignore_num in hex string.
>
>   This is the ignore count of the autoload-breakpoint to create,
>   encoded as a hex string.
>
>> +@item @var{id}@samp{:}@samp{R}
>> +@cindex @var{id}@samp{:}@samp{R} packet
>
> Please remove this @cindex entry, it is useless.
>
>> +@var{id} is the id in hex string format of this command want to remove.
>
>   @var{id} is the number of the autoload-breakpoint that this command
>   wants to remove, encoded as a hex string.
>
>> +@item @var{id}@samp{:}@samp{C}@samp{:}@var{cmd_str}
>> +@cindex @var{id}@samp{:}@samp{C}@samp{:}@var{cmd_str} packet
>
> No need for this @cindex.
>
>> +This packet add @var{cmd_str} to the commands list of
>> +autoload-breakpoint @var{id}.
>
>   This packet adds commands in @var{cmd_list} to the command list of
>   the autoload-breakpoint whose number is @var{id}.
>
>>                                  If @var{cmd_str} is empty,
>> +clear commands list of autoload-breakpoint @var{id}.
>
>   If @var{cmd_str} is empty, the command list will be emptied.
>
>> +@var{cmd_str} is encoded to hex string.
>                              ^^
> "as"
>
>> +@item @var{id}@samp{:}@samp{O}@samp{:}@var{condition_str}
>> +@cindex @var{id}@samp{:}@samp{O}@samp{:}@var{condition_str} packet
>
> No need for this @cindex entry.
>
>> +This packet set @var{condition_str} as the condition of
>> +autoload-breakpoint @var{id}.
>
>   This packet sets the condition of the autoload-breakpoint @var{id} to
>   be as specified by @var{condition_str}.
>
>>                                If @var{condition_str} is empty,
>> +clear condition of autoload-breakpoint @var{id}.
>
>   If @var{condition_str} is empty, the autoload-breakpoint becomes
>   unconditional.
>
>> +@var{condition_str} is encoded to hex string.
>                                    ^^
> "as"
>
>> +@subsection Autoload-breakpints request packets
>> +@table @samp
>> +@item qBfP
>> +@itemx qBsP
>> +These packets request data in autoload-breakpoints base format
>> +about autoload-breakpints from the stub.
>> +@value{GDBN} sends @code{qBfP} to get the first piece
>> +of data, and multiple @code{qTsP} to get additional pieces.
>> +@end table
>> +
>> +@subsection Autoload-breakpints control packets
>> +@table @samp
>> +@item @samp{QBDP}@var{autoload-breakpoints base format}
>> +@value{GDBN} and the stub use this packet to control
>> +the autoload-breakpoints in the remote.  The stub will translate
>> +this packet through reportAsync Packets.
>> +@end table
>
> No need to have a separate subsection for each packet.
>
> Also, I think you need to "connect the dots" by explaining the
> connection between these packets and the format described above.  It
> is probably a good idea to start with the list of the packets and
> their short description, and then follow up with the description of
> their format.
>
> In sum, this documentation patch needs to be reworked.

OK.  I will do it according to your comments.

Thanks,
Hui