Re: [PATCH 5/5] gdb/python: implement the print_insn extension language hook

[Andrew Burgess <andrew.burgess@embecosm.com>]

Thanks for the feedback.  I have a couple of questions:

* Eli Zaretskii <eliz@gnu.org> [2021-10-14 10:12:45 +0300]:

> > From: Andrew Burgess <andrew.burgess@embecosm.com>
> > Date: Wed, 13 Oct 2021 22:59:10 +0100
> > 
> > diff --git a/gdb/NEWS b/gdb/NEWS
> > index d001a03145d..fd1952a2f59 100644
> > --- a/gdb/NEWS
> > +++ b/gdb/NEWS
> > @@ -32,6 +32,12 @@ maint show internal-warning backtrace
> >    internal-error, or an internal-warning.  This is on by default for
> >    internal-error and off by default for internal-warning.
> >  
> > +set style disassembly on|off
> > +show style disassembly
> > +  If GDB is compiled with Python support, and the Python pygments
> > +  module is available, then, when this setting is on, disassembler
> > +  output will have styling applied.
> 
> If this requires Python with a module that is not available by
> default, I think a general style name like "disassembly" would be
> misleading.  I suggest "pygment-disassembly" instead.

I'm not sure I agree with this.  I don't see why we'd want to leak an
implementation detail (that we're using the pygment library) in a
setting name.

Surely, the setting should reflect what the effect is within GDB, and,
as far as possible, the implementation should be hidden from the
user. I'm hoping that some of the clarifications below might make this
more palatable...

> 
> > +@item set style disassembly @samp{on|off}
> > +Enable or disable disassembly styling.  This affects whether
> > +disassembly output, such as the output of the @code{disassemble}
> > +command, is styled.  The default is @samp{on}.  Note that disassembly
> > +styling only works if styling in general is enabled, and if a source
> > +highlighting library is available to @value{GDBN}.
> > +
> > +To highlight disassembly output @value{GDBN} must be compiled with
> > +Python support, and the Python Pygments package must be available,
> 
> So what does the default ON setting mean if pygments module is not
> available, or if GDB was not compiled with Python support?

You're correct, I've reworded this to reflect what actually happens.

First, as this is all implemented in Python, if GDB is compiled
without Python support then this setting (and the underlying feature)
is just not available.

If we do have Python, but not the Pygments library, then this feature
will be off by default, and an attempt to turn it on will give an
error that informs the user that the Python Pygments library is
missing.

Finally, if all the bits are in place, then this feature is on by
default.

> > +@node Disassembly In Python
> > +@cindex Python Instruction Disassembly
> 
> Index entries should begin with a lower-case letter, so that sorting
> of the entries in the produced manual would not depend on the locale.
> 
> > +@defivar DisassembleInfo can_emit_style_escapes
> > +This is @code{True} if the output stream that the disassembler is
> > +currently printing too can support escape sequences use for colors,
>                       ^^^
> Should be "to".
> 
> > +otherwise this attribute is @code{False}.
> 
> Not sure why you are talking about escape sequences: we support
> styling with colors also on terminals without escape sequences.  Does
> this mean this feature _must_ have actual escape sequence support?

I took the name from the internal GDB functions that do the same
check.  Can you point me at the terminal that does syntax highlighting
without using escape sequences, then I can see how this hooks back
into GDB.

Maybe I should rename this function 'supports_styling'? or something
similar?

Everything else I've fixed in my local tree.

Thanks,
Andrew


> 
> > +@defmethod DisassembleInfo memory_error (offset)
> > +This method marks the @code{DisassembleInfo} as having experienced a
> > +@code{gdb.MemoryError} when trying to access memory of @var{offset}
> > +bytes from @code{DisassembleInfo.address}.
> 
> Should this text have a cross-reference to where MemoryError is
> described?
> 
> > +The optional @var{architecture} is either a string, or the value
> > +@code{None}.  If it is a string, then it should be the name of an
> > +architecture known to @value{GDBN}, as returned either from
> > +@code{gdb.Architecture.name()}
> > +(@pxref{gdbpy_architecture_name,,gdb.Architecture.name}), or from
> > +@code{gdb.architecture_names()}
> > +(@pxref{gdb_architecture_names,,gdb.architecture_names}).
> 
> Please remove the parentheses from the references to these methods.
> 
> > +@defun format_address (architecture, address)
> > +Returns @var{address} formatted as a string, in a style suitable for
> > +including in the disassembly output of an instruction, for example a
> > +formatted address might look like:
> > +
> > +@smallexample
> > +0x00001042 <symbol+16>
> > +@end smallexample
> > +
> > +@var{architecture} is a @code{gdb.Architecture} (@pxref{Architectures
> > +In Python}), which is required to format the addresses correctly.
> > +This can be obtained from @code{DisassembleInfo.architecture}.
> 
> This last paragraph should have @noindent before it, since it's a
> continuation the description of format_address.
> 
> > +After calling this function the result in @var{info} @emph{might} have
> > +been updated to include syntax highlighting escape sequences.  If
> > +syntax highlighting is disabled in @value{GDBN}, or the output stream
> > +doesn't support syntax highlighting, then this function will leave
> > +@var{info} unchanged.
> 
> I suggest a cross-reference to commands that enable syntax
> highlighting where you mention it.
> 
> > +This function should return a Python object that supports the buffer
> > +protocol, i.e. a string, an array, or the object returned from
> 
> Please add @: after i.e., to prevent TeX from typesetting that as an
> end of a sentence.
> 
> Thanks.