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

Eli Zaretskii via Gdb-patches
2023-04-04T12:01:20Z

> 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>