Re: [PATCH][gdb/python] Add interface to access minimal_symbols

[Eli Zaretskii <eliz@gnu.org>]

> Date: Thu, 4 Oct 2018 23:11:17 +0200
> From: Tom de Vries <tdevries@suse.de>
> 
> [ Submitted earlier here (
> https://sourceware.org/ml/gdb-patches/2016-02/msg00124.html ). ]
> 
> This patch adds a new gdb.MinSymbol object to export the minimal_symbol
> interface.

Thanks.

> Build and reg-tested on x86_64-linux.
> 
> OK for trunk?

I have a couple of comments for the documentation part.

I think this warrants a NEWS entry.

> +@value{GDBN} represents every variable, function and type as an
> +entry in a symbol table.  @xref{Symbols, ,Examining the Symbol Table}.
> +Typical symbols like functions, variables, etc are represented by
> +gdb.Symbol objects in Python.  Some symbols are defined with less

"gdb.Symbol" should be in @code.

> +@findex gdb.lookup_minimal_symbol
> +@defun gdb.lookup_minimal_symbol (name @r{[}, sfile@r{]}, objfile@r{[})

There's no need to use @findex for methods you introduce with @defun,
since the latter automatically creates an @findex entry.

> +This function searches for a minimal symbol by name.
> +The search scope can be restricted by the sfile and objfile arguments.
                                             ^^^^^     ^^^^^^^
Argument names in text should have the @var markup, like you did below:

> +The optional @var{sfile} argument restricts the search to the source file
> +in which the minimal symbol was defined.
> +The @var{sfile} argument must be a string.  The optional @var{objfile}
> +restricts the search to the objfile that contains the minimal symbol.

> +@defvar MinSymbol.print_name
> +The name of the symbol in a form suitable for output.  This is either
> +@code{name} or @code{linkage_name}, depending on whether the user
> +asked @value{GDBN} to display demangled or mangled names.

When you refer to other GDB features, it is generally a good idea to
have a hyperlink there.  In this case, I believe you refer to the
command "set print demangle", described in "Print Settings", so please
use @pxref or @xref to add a cross-reference there.

> +@defvar MinSymbol.filename
> +The file name of the source file where the minimal symbol is defined.  This
> +value may represent filenames used internally by the compiler rather
> +than a viewable/editable source file.

The last sentence sounds a bit mysterious to me: what are "filenames
used internally by the compiler"?  Maybe an example will clarify that.

> +@defvar MinSymbol.section
> +The name of the binary section containing this minimal symbol.

I would suggest to use "section in the object file"; "binary" might be
misinterpreted to mean that the section itself is binary.

> +@vindex MINSYMBOL_TYPE_SLOT_GOT_PLT
> +@item gdb.MINSYMBOL_TYPE_SLOT_GOT_PLT
> +This type represents GOT for .plt sections.

I don't believe we have "GOT" mentioned anywhere else in the manual,
so (1) please use @acronym{GOT}, and (2) please tell in the text what
this acronym stands for.

> +@vindex MINSYMBOL_TYPE_FILE_DATA
> +@item gdb.MINSYMBOL_TYPE_FILE_DATA
> +This type represents the static version of gdb.MINSYMBOL_TYPE_DATA.
> +
> +@vindex MINSYMBOL_TYPE_FILE_BSS
> +@item gdb.MINSYMBOL_TYPE_FILE_BSS
> +This type represents the static version of gdb.MINSYMBOL_TYPE_BSS.
> +@end vtable

The 2 "gdb.*" symbols above should be in @code.

The documentation part is approved with the above gotchas fixed.