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

[Andrew Burgess <aburgess@redhat.com>]

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