Re: [RFA][patch 1/9] Yet another respin of the patch with initial Python support

Eli Zaretskii
2008-07-26T13:07:00Z

> From: Thiago Jung Bauermann <bauerman@br.ibm.com>
> Cc: Tom Tromey <tromey@redhat.com>,
> gdb-patches ml <gdb-patches@sourceware.org>,
> Eli Zaretskii <eliz@gnu.org>
> Date: Mon, 21 Jul 2008 12:26:09 -0300
>
> This is today's version of the patch. :-)
> It addresses the latest review comments, and is good to go AFAIK.
>
> Eli,
>
> What do you think of this documentation?

Thanks; comments below.

Do I understand correctly that I need not review the doco patches you
posted prior to this? If there's something else I should review,
please point to the corresponding messages.

> +* Python scripting
> +
> + GDB now has support for scripting using Python. Whether this is
> + available is determined at configure time; you can explicitly
> + disable the support using --without-python.

Is --without-python a configure-time switch or a run-time switch? If
the former, I don't think it should be mentioned here. If the latter,
we should explicitly say it's a run-time switch.

> +python [code]
> + Invoke python code.

Please say a few word about the form of [code]. Is it a string? if
so, should it be in quotes? Or maybe it's the name of a file with
Python code? Or something else? These all are questions that went
through my head when I was reading this NEWS entry.

> + else if (p1 - p == 6 && !strncmp (p, "python", 6))

Can we avoid literal constants such as 6 here, and instead make it
dependent on the string "python"?

> else if (p1 - p == 10 && !strncmp (p, "loop_break", 10))

Ditto.

> * Emacs:: Using @value{GDBN} under @sc{gnu} Emacs
> * GDB/MI:: @value{GDBN}'s Machine Interface.
> * Annotations:: @value{GDBN}'s annotation interface.
> +* Python:: Scripting @value{GDBN} using Python.

I think this should not be a separate chapter, but rather a section of
the "Canned Sequences of Commands" chapter.

> +@node Python
> +@chapter Scripting @value{GDBN} using Python
> +@cindex Python

First, @cindex entries should begin with lower-case letters, to
produce deterministic results when index entries are sorted.

Second, I think "python scripting" will be a better index entry here,
especially since the GDB manual joins all indices into one, and you
define later an index entry "python" for `python' the command..

Third, I'd add another @cindex entry "scripting with Python".

> +You can script @value{GDBN} using the @url{http://www.python.org/,
> +Python programming language}.

Please use @uref, not @url here. The latter has become a synonym of
the former only in the latest versions of Texinfo, so let's not force
users upgrade their Texinfo just for that.

>+ This feature is available only if
> +@value{GDBN} was configured using @code{--with-python}.

Please use @option for switches, not @code.

> +@node Python Commands
> +@section Python Commands

This needs index entries; please add them.

> +By default, @value{GDBN} will print a stack trace when an error occurs
> +in a Python script. This can be controlled using @code{maint set python
> +print-stack}. This variable takes a boolean value; if @code{on}, the
> +default, then Python stack printing is enabled; if @code{off}, then
> +Python stack printing is disabled.

I'd rephrase this slightly ("set python print-stack" is not a
variable):

By default, @value{GDBN} prints a stack trace when an error occurs
in a Python script. This can be controlled using @code{maint set
python print-stack}: if @code{on}, the default, Python stack
printing is enabled; if @code{off}, it is disabled.

> +@node Python API
> +@section Python API

This also needs index entries.

> +@value{GDBN} exposes a number of features to Python scripts.

This should end with a colon, and the list of the features should be a
@table.

> +When executing a @code{python} command, uncaught Python exceptions are
^^^
"the @code{python} command"

> +In particular, a @value{GDBN} @code{QUIT} exception is translated to a
> +Python @code{KeyboardInterrupt} exception, and other @value{GDBN}
> +exceptions are translated to Python @code{RuntimeError}s.

I think this is nowhere nearly as clear as it should be. Even to a C
hacker, "QUIT exception" is not sufficiently self-explanatory. Do you
mean SIGQUIT? then please say so. Will Python RuntimeError clear
enough to casual GDB users who want to use Python scripting, but are
not experienced Python programmers? I'm not sure. And what about
stating explicitly which exceptions map to which Python RuntimeError.

Finally, "GDB exception" is itself a term that is not defined anywhere
in the manual. Strictly speaking, GDB being a C program does not have
any exception at all.

This all needs to be explained, if we want Python scripting to be
useful for writing complex GDB extensions (which is the raison d'Ãªtre
of this feature in the first place).

> +At startup, @value{GDBN} overrides Python's @code{sys.stdout} and
> +@code{sys.stderr} to print using @value{GDBN}'s output-paging streams.
> +A Python program which outputs to one of these streams may have its
> +output interrupted by the user (@pxref{Screen Size}). In this
> +situation, a Python @code{KeyboardInterrupt} exception is thrown.

Is there something here that whoever writes the Python program should
know to behave correctly? Does she need, for example, catch
KeyboardInterrupt and do something in the handler? If so, we should
state here what's needed.

> +@value{GDBN} introduces a new Python module, named @code{gdb}. All
> +methods and classes added by @value{GDBN} are placed in this module.

OK, but what does this mean for whoever writes Python extensions for
GDB? While at that, how about explaining why a command FOO is indexed
as gdb.FOO?

> +@defun execute @var{command}

You should not give a @var markup to arguments in @defun; @defun does
that itself.

> +Evaluate @var{command}, a string, as a @value{GDBN} CLI command.
> +Exceptions are translated as noted above. If no error occurs, this
> +function will return @code{None}.

The last sentence begs a question: what happens if errors _do_ occur?

> +@defun show @var{variable}

Please remove @var.

> +Find the value of a @value{GDBN} setting.

"find" or "display"? If the former, why the name is "show"?

> + @var{variable} is a string
> +naming the setting to look up; @var{variable} may contain spaces if the
> +variable has a multi-part name. For example, @samp{print object} is a
> +valid variable name.

So what signals the end of the name? Also, what exactly is a
"multi-part name"?

> +If the named variable does not exist, this function throws an exception.

Any specific exception? if so, please name it.

> +Otherwise, the variable's value is converted to a Python value of the
> +appropriate type, and returned.

Sounds like it's indeed "find the value" or "return the value", not
"show". How about renaming the command to be more mnemonic?

> +@defun write @var{string}

@var again.

> +Print a string to @value{GDBN}'s paginated standard output stream.
> +Ordinarily your program should not call this function directly; simply
> +print to @code{sys.stdout} as usual.

So why is this command available, and why document it?

> +@defun flush
> +Flush @value{GDBN}'s paginated standard output stream. Ordinarily your
> +program should not call this function directly; simply flush
> +@code{sys.stdout} as usual.

Ditto.

Thanks again for working on this.