Re: [PATCH] Program Breakpoints

[Ross Morley <ross@tensilica.com>]

Thanks Eli for your valuable comments. I did indeed miss a bunch of "GDB"s,
and double periods after sentences. More in-line...


Eli Zaretskii wrote:

>>Date: Fri, 17 Apr 2009 18:32:59 -0700
>>From: Ross Morley <ross@tensilica.com>
>>CC: Maxim Grigoriev <maxim@tensilica.com>
>>
>>The user manual has been updated. I will update the internals
>>manual after this patch has been (possibly changed and) accepted.
>>    
>>
>
>Thank you.  I have a few comments for the documentation patch:
>
>  
>
>>+Some programs may contain embedded break or trap instructions that are
>>+unknown to GDB. These are called @dfn{program breakpoints} because they 
>>    
>>
>              ^^^
>"@value{GDBN}".
>
>  
>
>>+belong to the program itself. If encountered, a target may stop execution 
>>+and report this to GDB along with how much to increment the PC to step 
>>+over it (a target is not required to do this).
>>    
>>
>
>Please don't talk about a "target" in the user's manual: this
>terminology is unknown to them.  Please talk about the "inferior" or
>the "program being debugged".
>  
>

You're quite right that I should discuss the target less in the user manual
and more in in the internals manual (which I intend to update once this 
patch
has been reviewed and any changes are made). However I'm quite used to 
seeing
the term "target" in the user manual, so am a bit confused on that. I 
did wish
to convey that not all targets (inferiors) will support this feature 
(perhaps
I should just omit the parenthetical part above). Note that in the remote
stop-reply section the term "target" is used this way in several places.

I will incorparate all of your feedback.

Thanks,
Ross

>  
>
>>+@item program-breakpoint
>>+A program breakpoint (trap instruction unknown to GDB) was reached.
>>    
>>
>                                                     ^^^
>"@value{GDBN}"
>
>  
>
>>@@ -26723,6 +26731,12 @@
>> @value{GDBN} should use @samp{qXfer:libraries:read} to fetch a new
>> list of loaded libraries.  @var{r} is ignored.
>> 
>>+@item trap
>>+The packet indicates that a target-specific break or trap instruction 
>>+was hit. @var{r} is the size of the instruction if the PC is pointing
>>+to it, else 0 (for example if the hardware already incremented the PC).
>>+@var{r} is ignored if the instruction was planted by @value{GDBN}.
>>+
>> The packet indicates that the target cannot continue replaying 
>>    
>>
>
>Something strange happened with this hunk.  In the current CVS, the
>manual in this part looks like this:
>
>    @cindex shared library events, remote reply
>    @item library
>    The packet indicates that the loaded libraries have changed.
>    @value{GDBN} should use @samp{qXfer:libraries:read} to fetch a new
>    list of loaded libraries.  @var{r} is ignored.
>
>    @cindex replay log events, remote reply
>    @item replaylog
>    The packet indicates that the target cannot continue replaying
>    logged execution events, because it has reached the end (or the
>    beginning when executing backward) of the log.  The value of @var{r}
>    will be either @samp{begin} or @samp{end}.  @xref{Reverse Execution},
>    for more information.
>
>So it seems to me that you are plugging your part in the middle of
>another @item, but then where's the "@item replaylog" line and the one
>preceding it, with @cindex"?  What am I missing?
>
>Finally, please make sure every period that ends a sentence has 2
>spaces, not 1, after it.
>
>  
>
>>--- gdb/NEWS	31 Mar 2009 20:21:06 -0000	1.305
>>+++ gdb/NEWS	17 Apr 2009 18:55:25 -0000
>>@@ -3,6 +3,10 @@
>> 
>> *** Changes since GDB 6.8
>> 
>>+* GDB now supports handling (embedded) program break or trap instructions
>>+that are unknown to GDB. These are called program breakpoints because they
>>+belong to the program itself.
>>    
>>
>
>This is good, but I suggest to add a sentence telling how would GDB
>manifest these program breakpoints.  Also, "program breakpoints"
>should be in quotes, as you are introducing a new term.
>
>Thanks.
>
>  
>