Re: Patch to update gdbint.texinfo [00/05]

[Eli Zaretskii <eliz at gnu dot org>]

> From: Jeremy Bennett <jeremy.bennett@embecosm.com>
> Date: Sat, 20 Sep 2008 15:01:30 +0100
> 
> As suggested by Thiago Jung Bauermann, I have taken some of the sections
> from my HOWTO on porting GDB and added them to the GDB Internals Manual.

Thanks.

However, I don't see your name in the FSF records of copyright
assignments.  If you didn't submit legal paperwork to the FSF, please
do, because your contribution is substantial to require that.  We
cannot use such large patches without legal papers.

> I'd appreciate feedback - this is my first attempt at GNU style
> documentation.

Comments below.  Please note that some of the mistakes against which I
comment are recurring elsewhere in your patch, and I didn't always
pointed out all of them.  Please be sure to look for those mistakes in
all of your contribution.

> If people wish, I'm happy to work on revising other sections as time
> permits.

Thanks!  This is really welcome.

> 2008-09-19  Jeremy Bennett  <jeremy.bennett@embecosm.com>
> 
> 	* doc/gdbint.texinfo (Initializing a New Architecture, Register
> 	Representation, Frame Interpretation, Inferior Call Setup, Adding
> 	a New Target, Porting gdb): These sections extended and updated.

A minor nit: this is against GNU coding style conventions of
formatting ChangeLog entries.  The right way is this:

	* doc/gdbint.texinfo (Initializing a New Architecture)
	(Register Representation, Frame Interpretation)
	(Inferior Call Setup, Adding a New Target, Porting gdb): These
	sections extended and updated.

That is, each line ends with a right paren.

Also, the doc/ directory has its own ChangeLog file, while
doc/gdbint.texinfo hints that you made these changes in the parent
directory's ChangeLog.

> 	* doc/images/stack_frame.{eps,pdf,png,svg,txt}: New image files for
> 	doc/gdbint.texinfo

Please state the full file names.  Abbreviating like this will defeat
grepp'ing ChangeLog files for specific file names.

> --- src/gdb/doc/gdbint.texinfo	2008-09-19 15:38:00.000000000 +0100
> +++ src-new/gdb/doc/gdbint.texinfo	2008-09-20 14:32:28.000000000 +0100
> @@ -36,6 +36,9 @@ Free Documentation License''.
>  @author Second Edition:
>  @author Stan Shebs
>  @author Cygnus Solutions
> +@author With additions by:
> +@author Jeremy Bennett
> +@author Embecosm

We don't really add each contributor to the list of authors.  E.g., my
name is not there.

> +@flushright
> +Partial revision for @value{GDBN} @value{GDBVN}
> +Embecosm, 19 September 2008
> +@end flushright

I don't think this is needed.

> +@c Much of the following section has been taken from the Open Source
> +@c application note ``Howto: Porting the GNU Debugger: Practical
> +@c Experience with the OpenRISC 1000 Architecture'', written by Jeremy
> +@c Bennett <jeremy.bennett@embecosm.com> of Embecosm. See
> +@c http://www.embecosm.com/download/ean3.html for more information.

Is this comment really needed?  It makes one wonder whether we copied
someone's text without having them sign legal papers, since someone
who reads this will not necessarily know that the named Jeremy Bennett
contributed his own HOWTO.

> +@node How an Architecture is Represented
> +@subsection How an Architecture is Represented

It is usually a good idea to have a @cindex entry at each section and
subsection, for those who use the manual as a reference.  If you were
to search the manual for the material in this subsection, what phrases
would you try looking up in the index?  Those are the phrases that
should be put here in @cindex entries.

> +@smallexample
> +void gdbarch_register (enum bfd_architecture    @var{architecture},
> +                       gdbarch_init_ftype      *@var{init_func},
> +                       gdbarch_dump_tdep_ftype *@var{tdep_dump_func});
> +@end smallexample
> +
> +The @var{architecture} enumeration will identify the unique @sc{bfd}
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@var{architecture} is not an enumeration, but a value that belongs to
the bfd_architecture enumeration.

> +to be associated with this @code{gdbarch}. The @var{init_func}
                                            ^^
Please make sure to leave 2 spaces after a period that ends a
sentence, here and everywhere else.

> +For example the function @code{_initialize_or1k_tdep} creates its
> +architecture for 32-bit OpenRISC 1000 architectures by calling.
                                                                 ^
You want a colon here, not a period.

> +architecture it finds, or NULL if none are found. If an architecture

NULL should be in @code, since it's a C symbol.

> +is found, the initialization function can finish, returning the found
> +architecture as result.

I don't understand what does the "the initialization function can
finish" part want to say.

> +@smallexample
> +struct gdbarch_info
> +@{
> +   const struct bfd_arch_info     *@var{bfd_arch_info};
> +   int                             @var{byte_order};
> +   bfd                            *@var{abfd};
> +   struct gdbarch_tdep_info       *@var{tdep_info};
> +   enum gdb_osabi                  @var{osabi};
> +   const struct target_desc       *@var{target_desc};
> +@};

Those @var's in the struct definition should be removed.  They are
literal names of the struct fields, so @var is inappropriate here.

> +@vindex bfd_arch_info
> +@var{bfd_arch_info} holds the key details about the

Same in this description of the struct fields: please use @code, not
@var.  For example, I'd rephrase the sentence above as:

  The @code{bfd_arch_info} member holds the key details about the
  architecture.

> +@cindex @code{gdbarch}

I doubt that a general index entry such as this one is useful at this
point.  "gdbarch" is such a general notion in GDB that it is unlikely
that people who look for it will want to land in this place.  You
probably wanted a reference to "struct gdbarch".

> +When the @code{struct gdbarch} initialization function is called, not
> +all the fields are providedâonly those which can be deduced from the

Please use 3 dashes in a row here: this is what produces an em-dash in
the printed manual.  A single dash will be typeset too narrow (as a
minus sign).

> +gdbarch_info} and and any additional custom target specific
                 ^^^^^^^
A typo.

> +argument. Populating the new @code{gdbarch} should use the
> +@code{@w{set_gdbarch}} functions.

You don't need a @w when the argument does not include whitespace.
TeX will not normally hyphenate on the `_' character.

> +architecture. Many of the functions and variables are described in the
> +header file, @file{gdbarch.h}.
              ^
I think this comma is redundant.

> +@cindex @code{gdbarch_tdep}

This is the second time you define an index entry with the same text.
If you really need both, it is better to make them distinct, by
qualifying either one of them or both with the specific aspects
described under each reference.  In this case, I think only one index
entry is enough, since they both point to th same subsection.

> +it can be set to NULL.
                    ^^^^
@code{NULL}.

> -to indicate the presence of one of these "short" pointers.  E.g, if
> +to indicate the presence of one of these ``short'' pointers.  E.g, if

Thanks for the fix.  But while at that, please also fix "E.g" by
adding a second period after the `g'.

> +@c Much of the following section has been taken from the Open Source
> +@c application note ``Howto: Porting the GNU Debugger: Practical
> +@c Experience with the OpenRISC 1000 Architecture'', written by Jeremy
> +@c Bennett <jeremy.bennett@embecosm.com> of Embecosm. See
> +@c http://www.embecosm.com/download/ean3.html for more information.

Same comment as above.

> +GDB considers registers to be a set with members numbered linearly
   ^^^
@value{GDBN}.

> +from 0 upwards. The first part of that set corresponds to real
> +physical registers, the second part to any
> +``pseudo-registers''. Pseudo-registers have no independent physical

When you introduce a term, please use @dfn, not quotes, to make it
stand out.

> +For any architecture, the implementer will decide on a mapping from
> +hardware to GDB register numbers. The registers corresponding to real
> +hardware are referred to as @emph{raw} registers, the remaining registers are

Here' "raw registers" is also a new term, so should be in @dfn the
first time it is used.

> +pseudo-registers. The total register set (raw and pseudo) is called
> +the @emph{cooked} register set.

Similarly about the "cooked register set".

> +@node Functions and Variables Specifying the Register Architecture

This is too long for a node name.  Please consider shortening it (the
names of a node and its subsection do not need to be identical).

> +@deftypefn {Architecture Function} CORE_ADDR read_pc (struct regcache *@var{regcache})
> +@findex read_pc

Do you really need this @findex?  I think @deftypefn generates an
index entry automatically, doesn't it?

> +Read the program counter. The default value is NULL (no function
                                                  ^^^^
@code{NULL}

> +cache, @var{regcache}. @xref{Register Cacheing, , Register Cacheing}.

"caching", not "cacheing".

> +@deftypefn {Architecture Function} void write_pc (struct regcache *@var{regcache}, CORE_ADDR @var{val})
> +@findex write_pc

Again, I think this @findex is redundant.

> +Write @var{val} to the program counter. The default value is NULL (no
> +function available). If the program counter is just an ordinary
> +register, it can be specified in @code{struct gdbarch} instead (see
> +@code{pc_regnum} below) and it will be written using the standard
> +routines to access registers. This function need only be specified if
> +the program counter is not an ordinary register.
> +
> +Any register information can be obtained using the supplied register
> +cache, @var{regcache}. @xref{Register Cacheing, , Register Cacheing}.

The description of read_pc and write_pc is almost identical, so I'd
suggest having one description for both.

> +@deftypefn {Architecture Function} void pseudo_register_read (struct gdbarch *@var{gdbarch}, struct regcache *@var{regcache}, int @var{regnum}, const gdb_byte *@var{buf})

Similarly for pseudo_register_read and pseudo_register_write.

> +@deftypevr {Architecture Variable} int sp_regnum
> +@vindex sp_regnum

Again, a redundant index entry.

> +register), which map to the strings ``gpr00'' through ``gpr31'', ``pc'' and

I think it is better to format C strings as @code{"gpr00"}.

> +@quotation Note

It is better to use @footnote for such notes.

> +Define this function to print out one or all of the registers for the
> +@value{GDBN} @b{info registers} command. The default value is the

Commands should be in @kbd or in @code.  Using physical markup such as
@b is discouraged, because it hints that you know something about the
physical shape of what @kbd and @code produce.

> +each register should be printed. Define a proprietary version of this
                                             ^^^^^^^^^^^
You mean "your own", not proprietary in the legal sense, right?

> +The registers should show their values in the frame specified by
> +@var{frame}. If @var{regnum} is -1 and @var{all} is zero, then all the
> +``main'' registers should be shown, otherwise only the value of the

What are "main" registers?  I don't think you explain that anywhere in
the text.

> +The default, function @code{@w{default_print_registers_info}}, prints
   ^^^^^^^^^^^
"By default" is better.

> +The @var{gdbarch} and @var{file} and @var{frame} arguments have the
                     ^^^
Replace this "and" with a comma.

> +same meaning as in the @code{print_registers_info} function above. The
> +string @var{args} contains any supplementary arguments to the @b{info
> +float} command.

@kbd{info float}

> +registers for the @value{GDBN} @b{info vector} command
                                  ^^
@kbd

>  Some architectures can represent a data object in a register using a
> -form that is different to the objects more normal memory representation.
> -For example:
> +form that is different to the objects more normal memory
> +representation.  For example:

Something is wrong here: "different to the objects more normal memory
representation" doesn't sound right.

> +@quotation Note
> +@code{gdbarch_register_to_value} and @code{gdbarch_value_to_register}
>  take their @var{reg} and @var{type} arguments in different orders.
> +@end quotation

Please don't put anything after @quotation on the same line.  Strictly
speaking, @quotation (as many any other similar @-command in Texinfo)
should appear on a line by themselves; a Texinfo translator may decide
to ignore anything after them.

> +@node Register Cacheing

"caching"

> +variables for a function invocation is known as the @emph{stack frame}
                                                       ^^^^^
@dfn

> +Almost all architectures have one register dedicated to point to the
> +end of the stack (the @emph{stack pointer}). Many have a second register
> +which points to the start of the currently active stack frame (the
> +@emph{frame pointer}). The specific arrangements for an architecture are
> +a key part of the ABI.

"stack point" and "frame pointer" should be in @dfn.

> +  if( 0 == n ) @{
> +    return 1;
> +  @}
> +  else @{
> +    return n * fact( n - 1 );
> +  @}

This is not the GNU style of formatting C code.  Please use the GNU
style.

> +@quotation Note

Not on the same line.  Actually, @footnote is better here.

> +@emph{THIS} frame is the frame currently under consideration.

THIS, NEXT, PREVIOUS should be in @dfn the first time they are used.

> +of the call to fact (0)). It is always numbered frame #0.
                  ^^^^^^^^
This should be in @code.

> +The @emph{base} of a frame is the address immediately before the start of
       ^^^^^
@dfn

> +the @emph{NEXT} frame. For a falling stack this will be the lowest

Isn't "push-down stack" the more widely used terminology?

> +@emph{unwinding}. The @value{GDBN} functions involved in this typically
    ^^^^
@dfn

> +should go in struct frame_info is called @emph{sniffing}. The functions
                                            ^^^^^
@dfn

> +issue about addressing the innermost frameâit has no @emph{NEXT}
                                             ^
You want --- here.

> +Some ABIs reserve space beyond the end of the stack for use by leaf
> +functions without prologue or epilogue or by exception handlers (for
> +example the OpenRISC 1000).
> +
> +This is known as a @emph{red zone} (AMD terminology). The @sc{amd64} (nee
> +x86-64) @sc{abi} documentation refers to the @emph{red zone} when

Please use consistently either "ABI" or "sc{abi}", everywhere.

> +The default value is 0.  Set this field if the architecture has such a
> +red zone. The value must be aligned (see @code{frame_align} above).

What do you mean by "value must be aligned"?  Do you mean rounded up?

> +in the @emph{PREVIOUS} frame (i.e. the frame of the function that called

Please always put "@:" after a period that is followed by whitespace,
but does not end a sentence.  This will tell TeX not to typeset that
whitespace as separating two sentences.

> +@emph{THIS} one). This is commonly referred to as the return address.
                                                         ^^^^^^^^^^^^^^
@dfn{return address}.

> +(@pxref{All About Stack Frames, , All About Stack Frames} for how
                                                            ^
A comma is needed here, or makeinfo will complain.

> +This function is given a pointer to @emph{THIS} stack frame (@pxref{All
> +About Stack Frames, , All About Stack Frames} for how frames are
                                                ^
Missing comma.

> +@node Analysing Stacks---Frame Sniffers
> +@subsection Analysing Stacks---Frame Sniffers

Please use US English spelling: "analyzing".

> +When a program stops, @value{GDBN} needs to construct the chain of
> +struct @code{@w{frame_info}} representing the state of the stack using
> +appropriate @emph{sniffers}.

No need to @emph a word "sniffer" in too many places.  Once it is
established as a term you defined and use, there's no need to make it
stand out further.

> +be required and a sniffer may be suitable for more than one struct
> +gdbarch.

@code{struct gdbarch}.

> +@code{@w{frame_unwind_append_sniffer}} is used to add a new sniffer to

No need for a @w here.

> +@code{@w{frame_base_append_sniffer}} is used to add a new sniffer

Nor here.

> +@code{@w{frame_base_set_default}} is used to specify the default base

Nor here.

> +@code{@w{frame_unwind_append_sniffer)}} returns a structure specifying

Nor here.

> +struct frame_unwind
> +@{
> +   enum frame_type            @var{type};
> +   frame_this_id_ftype       *@var{this_id};
> +   frame_prev_register_ftype *@var{prev_register};
> +   const struct frame_data   *@var{unwind_data};
> +   frame_sniffer_ftype       *@var{sniffer};
> +   frame_prev_pc_ftype       *@var{prev_pc};
> +   frame_dealloc_cache_ftype *@var{dealloc_cache};
> +@};

No need for these @var's again.  (Also in the following text.)

> +@item
> +@var{prev_pc} determines the program counter for @emph{THIS}
> +frame. Only needed if the program counter is not an ordinary register
> +(@pxref{Functions and Variables Specifying the Register Architecture,
> +, Functions and Variables Specifying the Register Architecture})

Missing period at the end of this sentence.

> +The frame base sniffer is much simpler. It is a @code{@w{struct
> +frame_base}}, which refers to the corresponding @code{@w{frame_unwind}}

No need for the second @w.

> +@node About Dummy Frames
> +@subsection About Dummy Frames

Please add a suitable @cindex entry here.

> +breakpoint, commands like @b{backtrace} work correctly.

@kbd{backtrace}

> +Build and test. If desired, lobby the @value{GDBN} steering group to

"@value{GDBN}" is not right here: this @value trick exists so that a
GDB manual could be easily produced for a version of GDB whose name is
different, e.g., "os1k-gdb".  By contrast, the name of the steering
group does not change!  So you need to use a literal GDB here.

> +Add a description of the new architecture to the ``Configuration
> +Specific Information'' section in the main @var{GDBN} user guide
> +(@cite{Debugging with @value{GDBN}} found in the file
> +@file{gdb/doc/gdb.texinfo}).

Please use a @pxref here, so that a hyper-link will be produced in the
manual.

Thanks again for working on this.