Re: [PATCH] Program Breakpoints

[Ross Morley <ross at tensilica dot 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.
>
>