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

Andrew Burgess via Gdb-patches
2023-04-28T23:12:00Z

Eli Zaretskii <eliz@gnu.org> writes:

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

Eli,

Thanks for the feedback. I've included all these fixes in the updated
patch that I replied to Tom with.

And now with no excessive @var uses :)

Thanks,
Andrew