Re: [PATCH 5/5] gdb/python: extend the Python Disassembler API to allow for styling

[Eli Zaretskii <eliz@gnu.org>]

> Cc: Andrew Burgess <aburgess@redhat.com>
> Date: Tue,  4 Apr 2023 09:21:07 +0100
> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
> 
>  gdb/NEWS                               |  19 +
>  gdb/doc/python.texi                    | 313 +++++++++-
>  gdb/python/py-disasm.c                 | 818 +++++++++++++++++++++++--
>  gdb/testsuite/gdb.python/py-disasm.exp |  94 ++-
>  gdb/testsuite/gdb.python/py-disasm.py  | 158 ++++-
>  5 files changed, 1309 insertions(+), 93 deletions(-)

Thanks.

> diff --git a/gdb/NEWS b/gdb/NEWS
> index d1726842299..dceccf09c6d 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -155,6 +155,25 @@ info main
>    ** It is now no longer possible to sub-class the
>       gdb.disassembler.DisassemblerResult type.
>  
> +  ** The Disassembler API from the gdb.disassembler module has been
> +     extended to include styling support:
> +
> +     - The DisassemblerResult class can now be initialized with a list
> +       of parts.  Each part represents part of the disassembled
> +       instruction along with the associated style information.  This
> +       list of parts can be accessed with the new
> +       DisassemblerResult.parts property.
> +
> +     - New constants gdb.disassembler.STYLE_* representing all the
> +       different styles part of an instruction might have.
> +
> +     - New methods DisassembleInfo.text_part and
> +       DisassembleInfo.address_part which are used to create the new
> +       styled parts of a disassembled instruction.
> +
> +     - Changes are backwards compatible, the older API can still be
> +       used to disassemble instructions without styling.
> +
>  *** Changes in GDB 13

This part is OK.

> +@defun DisassembleInfo.text_part (@var{style}, @var{string})

Same question about @var inside @defun.

> +@defun DisassembleInfo.address_part (@var{address})
> +Create a new @code{DisassemblerAddressPart}.  @var{address} is the
> +value of the absolute address this part represents.  When @value{GDBN}
> +displays an address part it display the absolute address, and also an
> +associated symbol.

The last sentence is problematic ("When GDB dipslays [...] it
display...")  Suggest to rephrase.

> +Only one of @var{string} or @var{parts} should be used to initialize a
> +new @code{DisassemblerResult}, whichever is not used should be passed
> +the value @code{None}, or the arguments can be passed by name, and the
> +unused argument can be ignored.

This sentence is hard to parse.  Suggest to reword:

  Only one of @var{string} or @var{parts} should be used to initialize
  a new @code{DisassemblerResult}; the other one should be passed the
  value @code{None}.  Alternatively, the arguments can be passed by
  name, and the unused argument can be ignored.

> +The @var{string} argument, if not @code{None}, is a non-empty string
> +that represents the entire disassembled instruction. Building a result
                                                      ^^
Two spaces there.

> +If this instance was initialized using a single string rather than a
                                                                      ^
That "a" should be deleted.

> +The following table lists all of the disassembler styles that are
> +available.  @value{GDBN} maps these style constants onto its style
> +settings (@pxref{Output Styling}).  In some cases multiple of these
> +style constants map to the same style setting.

Suggest to reword the last sentence above:

  In some cases, several style constants produce the same style
  settings, and thus will produce the same visual effect on the
  screen.

>                                                   This could change in
> +future releases of @value{GDBN}, so care should be taken to select the
> +correct style constant to ensure correct output styling on future
> +releases of @value{GDBN}.                               ^^

"in", not "on".

> +@vindex STYLE_IMMEDIATE
> +@item gdb.disassembler.STYLE_IMMEDIATE
> +This style is used for styling numerical values within a disassembled
> +instruction when those values are not addresses, address offsets, or
> +register numbers, use @code{STYLE_ADDRESS},
> +@code{STYLE_ADDRESS_OFFSET}, or @code{STYLE_REGISTER} in those cases.

This should be rewritten as 2 separate sentences: one describing which
values use STYLE_IMMEDIATE, the other (perhaps in parentheses)
describing the alternative style constants for other kinds of values.

> +For any other numerical value within a disassembled instruction,
> +@code{STYLE_IMMEDIATE} should be used.

This reads awkward, both because it uses passive voice, and because
the verb is at the end.  Reword:

  Use the @code{STYLE_IMMEDIATE} for any other values within a
  disassembled instruction.

Also, it sounds like this just repeats the first of the two sentences
above, so perhaps it should be removed?

It might be a good idea to describe the "other" styles for values
first, and then the "immediate" style, as the catchall for the other
values.

> +referring too.  For example, this x86-64 instruction:
> +
> +@smallexample
> +call   0x401136 <foo>
> +@end smallexample
> +
> +@noindent
> +Here @code{foo} is the name of a symbol, and should be given the
> +@code{STYLE_SYMBOL} style.

Since you start a new sentence after @noindent, the "For example" part
should be reworded, perhaps leaving just "For example:".

Reviewed-By: Eli Zaretskii <eliz@gnu.org>