Re: Make the "python" command resemble the standard Python interpreter

[Kevin Pouget <kevin dot pouget at gmail dot com>]

On Wed, Jan 11, 2012 at 1:18 AM, Khoo Yit Phang <khooyp@cs.umd.edu> wrote:
> Hi,
>
> I'd like to contribute a patch to improve the "python" command by making it resemble the standard Python interpreter in behavior.
>
> For "python" with arguments, this prints the results of expressions, e.g., "python 1 + 2" will print "3" (previously, it will not print anything).
>
> For "python" without arguments, this uses Python's built-in interactive loop (PyRun_InteractiveLoop) so that individual statements/expressions are immediately evaluated and results of expressions are printed. It also hooks GDB's readline wrappers (command_line_input) to Python so that line editing and history editing works. Additionally, Python's standard readline module is stubbed out because it conflicts with GDB's use of readline.
>
> This patch depends on a patch to handle SIGINT that I previously submitted (http://sourceware.org/bugzilla/show_bug.cgi?id=13265) to handle SIGINT in the interactive loop.
>
> Thanks!
>
> Yit
> January 10, 2012
>

Hello,

it sounds interesting indeed,
I dropped below a few comments.  I'm not a maintainer, so that's
nothing more than suggestions


> gdb/ChangeLog:
>
> 2012-01-10 ?Khoo Yit Phang <khooyp@cs.umd.edu>
>
> Make the "python" command resemble the standard Python interpreter.
> * Makefile.in (SUBDIR_PYTHON_OBS): Add py-gdb-readline.o.
> (SUBDIR_PYTHON_SRCS): Add python/py-gdb-readline.c.
> (py-gdb-readline.o): Add rule to compile python/py-gdb-readline.c.
> * python/py-gdb-readline.c: New file.
> * python/python-internal.h (gdbpy_initialize_gdb_readline): New
> prototype.
> * python/python.c (eval_python_command): New function.
> (eval_python_from_control_command): Call eval_python_command instead
> of PyRun_SimpleString.
> (python_command): For "python" with arguments, call
> eval_python_command. ?For "python" without arguments, call
> PyRun_InteractiveLoop if from_tty is true, other call
> execute_control_command_untraced.
>
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -270,6 +270,7 @@
> #
> SUBDIR_PYTHON_OBS = \
> python.o \
> + py-gdb-readline.o \
> py-auto-load.o \
> py-block.o \
> py-bpevent.o \
> @@ -302,6 +303,7 @@
>
> SUBDIR_PYTHON_SRCS = \
> python/python.c \
> + python/py-gdb-readline.c \
> python/py-auto-load.c \
> python/py-block.c \
> python/py-bpevent.c \

name should be kept in alphabetical order, please. The same applies
for the entries below.

> @@ -2019,6 +2021,10 @@
> $(COMPILE) $(PYTHON_CFLAGS) $(srcdir)/python/python.c
> $(POSTCOMPILE)
>
> +py-gdb-readline.o: $(srcdir)/python/py-gdb-readline.c
> + $(COMPILE) $(PYTHON_CFLAGS) $(srcdir)/python/py-gdb-readline.c
> + $(POSTCOMPILE)
> +
> py-auto-load.o: $(srcdir)/python/py-auto-load.c
> $(COMPILE) $(PYTHON_CFLAGS) $(srcdir)/python/py-auto-load.c
> $(POSTCOMPILE)
> diff --git a/gdb/python/py-gdb-readline.c b/gdb/python/py-gdb-readline.c
> new file mode 100644
> --- /dev/null
> +++ b/gdb/python/py-gdb-readline.c
> @@ -0,0 +1,168 @@
> +/* Readline support for Python.
> +
> + ? Copyright (C) 2012 Free Software Foundation, Inc.
> +
> + ? This file is part of GDB.
> +
> + ? This program is free software; you can redistribute it and/or modify
> + ? it under the terms of the GNU General Public License as published by
> + ? the Free Software Foundation; either version 3 of the License, or
> + ? (at your option) any later version.
> +
> + ? This program is distributed in the hope that it will be useful,
> + ? but WITHOUT ANY WARRANTY; without even the implied warranty of
> + ? MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. ?See the
> + ? GNU General Public License for more details.
> +
> + ? You should have received a copy of the GNU General Public License
> + ? along with this program. ?If not, see <http://www.gnu.org/licenses/>. ?*/
> +
> +#include "defs.h"
> +#include "python-internal.h"
> +#include "../exceptions.h"
> +#include "../top.h"

according to other py-*.c files, you don't need the '../'

> +#include <stddef.h>
> +#include <string.h>
> +
> +/* Python's readline module conflicts with GDB's use of readline
> + ? since readline is not reentrant. To prevent conflicts, a dummy
> + ? readline module is set up which simply fails for any readline
> + ? method. */

Two spaces after the dots "reentrant.  To prevent", "method.  */", and
the same in further comments

> +/* TODO: Ideally, this module should implement a reentrant wrapper
> + ? to readline, so that the readline module can be used without
> + ? conflicting with GDB. */

I think TODO will not be accepted in the final patch.

> +static PyObject *
> +unimplemented_command (PyObject *self, PyObject *args, PyObject *kw)
> +{
> + ?PyErr_SetString (PyExc_NotImplementedError,
> + ? "readline module disabled under GDB");
> + ?return NULL;
> +}
> +
> +#define UNIMPLEMENTED(cmd) \
> + ?{ cmd, (PyCFunction)unimplemented_command, \
> + ? ?METH_VARARGS | METH_KEYWORDS, "" }

space after the '('. The same applies below, and for all the function calls:
> function (...);
> (cast) var;

> +static PyMethodDef GdbReadlineMethods[] =
> +{
> + ?/* readline module methods as of Python 2.7.2. */
> + ?UNIMPLEMENTED("add_history"),
> + ?UNIMPLEMENTED("clear_history"),
> + ?UNIMPLEMENTED("get_begidx"),
> + ?UNIMPLEMENTED("get_completer"),
> + ?UNIMPLEMENTED("get_completer_delims"),
> + ?UNIMPLEMENTED("get_completion_type"),
> + ?UNIMPLEMENTED("get_current_history_length"),
> + ?UNIMPLEMENTED("get_endidx"),
> + ?UNIMPLEMENTED("get_history_item"),
> + ?UNIMPLEMENTED("get_history_length"),
> + ?UNIMPLEMENTED("get_line_buffer"),
> + ?UNIMPLEMENTED("insert_text"),
> + ?UNIMPLEMENTED("parse_and_bind"),
> + ?UNIMPLEMENTED("read_history_file"),
> + ?UNIMPLEMENTED("read_init_file"),
> + ?UNIMPLEMENTED("redisplay"),
> + ?UNIMPLEMENTED("remove_history_item"),
> + ?UNIMPLEMENTED("replace_history_item"),
> + ?UNIMPLEMENTED("set_completer"),
> + ?UNIMPLEMENTED("set_completer_delims"),
> + ?UNIMPLEMENTED("set_completion_display_matches_hook"),
> + ?UNIMPLEMENTED("set_history_length"),
> + ?UNIMPLEMENTED("set_pre_input_hook"),
> + ?UNIMPLEMENTED("set_startup_hook"),
> + ?UNIMPLEMENTED("write_history_file"),
> + ?{ NULL, NULL, 0, NULL }
> +};
> +
> +/* Readline function suitable for PyOS_ReadlineFunctionPointer, which
> + ? is used for Python's interactive parser and raw_input. In both
> + ? cases, sys_stdin and sys_stdout are always stdin and stdout
> + ? respectively, as far as I can tell; they are ignored and
> + ? command_line_input is used instead. */
> +
> +static char *
> +gdbpy_readline_wrapper (FILE *sys_stdin, FILE *sys_stdout,
> + char *prompt)

I think that 'char *prompt' whould be aligned with FILE *sys_stdin'

> +{
> + ?int n;
> + ?char *p = NULL, *p_start, *p_end, *q;
> + ?volatile struct gdb_exception except;
> +
> + ?TRY_CATCH (except, RETURN_MASK_ALL)
> + ? ?{
> + ? ? ?struct cleanup *cleanup = gdbpy_suspend_sigint_handler ();

new line between declarations and the code

> + ? ? ?p = command_line_input (prompt, 0, "python");
> + ? ? ?do_cleanups (cleanup);
> + ? ?}

I'm not sure about that, but isn't the clean up supposed to be
executed even if an exception is thrown? it seems not to be the case
here

> + ?/* Detect Ctrl-C and treat as KeyboardInterrupt. */
> + ?if (except.reason == RETURN_QUIT)
> + ? ?return NULL;
> +
> + ?/* Handle errors by raising Python exceptions. */
> + ?if (except.reason < 0)
> + ? ?{
> + ? ? ?/* The thread state is nulled during gdbpy_readline_wrapper,
> + with the original value saved in the following undocumented
> + variable (see Python's Parser/myreadline.c and
> + Modules/readline.c). */

comment lines should be aligned with "The thread" (or maybe my tabs
are not displayed properly)

> + ? ? ?PyEval_RestoreThread (_PyOS_ReadlineTState);
> + ? ? ?gdbpy_convert_exception (except);
> + ? ? ?PyEval_SaveThread ();
> + ? ? ?return NULL;
> + ? ?}
> +
> + ?/* Detect EOF (Ctrl-D). */
> + ?if (p == NULL)
> + ? ?{
> + ? ? ?q = PyMem_Malloc (1);
> + ? ? ?if (q != NULL)
> + q[0] = '\0';
> + ? ? ?return q;
> + ? ?}
> +
> + ?n = strlen (p);
> +
> + ?/* Detect "end" like process_next_line in cli/cli-script.c. */
> + ?/* Strip trailing whitespace. ?*/
> + ?p_end = p + n;
> + ?while (p_end > p && (p_end[-1] == ' ' || p_end[-1] == '\t'))
> + ? ?p_end--;
> +
> + ?/* Strip leading whitespace. ?*/
> + ?p_start = p;
> + ?while (p_start < p_end && (*p_start == ' ' || *p_start == '\t'))
> + ? ?p_start++;

maybe clu-utils.h remove_trailing_whitespace and skip_spaces already
do the same?

> + ?/* Treat "end" as EOF. */
> + ?if (p_end - p_start == 3 && !strncmp (p_start, "end", 3))
> + ? ?{
> + ? ? ?q = PyMem_Malloc (1);
> + ? ? ?if (q != NULL)
> + q[0] = '\0';
> + ? ? ?return q;
> + ? ?}
> +
> + ?/* Copy the line to Python and return. */
> + ?q = PyMem_Malloc (n + 2);
> + ?if (q != NULL)
> + ? ?{
> + ? ? ?strncpy (q, p, n);
> + ? ? ?q[n] = '\n';
> + ? ? ?q[n + 1] = '\0';
> + ? ?}
> + ?return q;
> +}
> +
> +void
> +gdbpy_initialize_gdb_readline (void)

all functions should have a comment, but I'm not sure whether it
should be in the C or H file

> +{
> + ?Py_InitModule3 ("readline", GdbReadlineMethods,
> + ?"GDB-provided dummy readline module to prevent conflicts with the standard readline module.");

This line is too long, should be < 80 chars

> +
> + ?PyOS_ReadlineFunctionPointer = gdbpy_readline_wrapper;
> +}
> +
> diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
> --- a/gdb/python/python-internal.h
> +++ b/gdb/python/python-internal.h
> @@ -234,6 +234,7 @@
> struct symtab_and_line *sal_object_to_symtab_and_line (PyObject *obj);
> struct frame_info *frame_object_to_frame_info (PyObject *frame_obj);
>
> +void gdbpy_initialize_gdb_readline (void);
> void gdbpy_initialize_auto_load (void);
> void gdbpy_initialize_values (void);
> void gdbpy_initialize_frames (void);
> diff --git a/gdb/python/python.c b/gdb/python/python.c
> --- a/gdb/python/python.c
> +++ b/gdb/python/python.c
> @@ -279,6 +279,43 @@
> ? return script;
> }
>
> +/* Evaluate a Python command like PyRun_SimpleString, but uses
> + ? Py_single_input which prints the result of expressions if the input
> + ? is from tty, and Py_file_input otherwise. */
> +
> +static void
> +eval_python_command (const char *command, int from_tty)
> +{
> + ?struct cleanup *cleanup;
> + ?PyObject *m, *d, *v;
> +
> + ?cleanup = ensure_python_env (get_current_arch (), current_language);
> +
> + ?m = PyImport_AddModule ("__main__");
> + ?if (m == NULL)
> + ? ?error (_("Error while executing Python code."));
> +
> + ?d = PyModule_GetDict (m);
> + ?v = PyRun_StringFlags (command,
> + from_tty ? Py_single_input : Py_file_input,
> + d, d, NULL);
> + ?if (v == NULL)
> + ? ?{
> + ? ? ?int interrupt = PyErr_ExceptionMatches (PyExc_KeyboardInterrupt);
blank line

> + ? ? ?PyErr_Print ();
> + ? ? ?if (! interrupt)
> + error (_("Error while executing Python code."));
> + ? ?}
> + ?else
> + ? ?{
> + ? ? ?Py_DECREF (v);
> + ? ? ?if (Py_FlushLine ())
> + PyErr_Clear ();
> + ? ?}
> +
> + ?do_cleanups (cleanup);
> +}
> +
> /* Take a command line structure representing a 'python' command, and
> ? ?evaluate its body using the Python interpreter. ?*/
>
> @@ -292,13 +329,10 @@
> ? if (cmd->body_count != 1)
> ? ? error (_("Invalid \"python\" block structure."));
>
> - ?cleanup = ensure_python_env (get_current_arch (), current_language);
> + ?script = compute_python_string (cmd->body_list[0]);
> + ?cleanup = make_cleanup (xfree, script);
>
> - ?script = compute_python_string (cmd->body_list[0]);
> - ?ret = PyRun_SimpleString (script);
> - ?xfree (script);
> - ?if (ret)
> - ? ?error (_("Error while executing Python code."));
> + ?eval_python_command (script, 0);
>
> ? do_cleanups (cleanup);
> }
> @@ -316,18 +350,26 @@
> ? while (arg && *arg && isspace (*arg))
> ? ? ++arg;
> ? if (arg && *arg)
> - ? ?{
> - ? ? ?ensure_python_env (get_current_arch (), current_language);
> -
> - ? ? ?if (PyRun_SimpleString (arg))
> - error (_("Error while executing Python code."));
> - ? ?}
> + ? ?eval_python_command (arg, from_tty);
> ? else
> ? ? {
> - ? ? ?struct command_line *l = get_command_line (python_control, "");
> -
> - ? ? ?make_cleanup_free_command_lines (&l);
> - ? ? ?execute_control_command_untraced (l);
> + ? ? ?if (from_tty)
> + {
> + ?int err;
> + ?ensure_python_env (get_current_arch (), current_language);
> + ?err = PyRun_InteractiveLoop(instream, "<stdin>");
> + ?dont_repeat ();
> + ?if (err && ! PyErr_ExceptionMatches (PyExc_KeyboardInterrupt))
> + ? ?error(_("Error while executing Python code."));
> + }
> + ? ? ?else
> + {
> + ?/* TODO: perhaps PyRun_FileExFlags can be used, which would
> + ? ? simplify cli/cli-script.c. */
> + ?struct command_line *l = get_command_line (python_control, "");
> + ?make_cleanup_free_command_lines (&l);
> + ?execute_control_command_untraced (l);
> + }
> ? ? }
>
> ? do_cleanups (cleanup);
> @@ -1280,6 +1322,7 @@
> ? gdbpy_gdberror_exc = PyErr_NewException ("gdb.GdbError", NULL, NULL);
> ? PyModule_AddObject (gdb_module, "GdbError", gdbpy_gdberror_exc);
>
> + ?gdbpy_initialize_gdb_readline ();
> ? gdbpy_initialize_auto_load ();
> ? gdbpy_initialize_values ();
> ? gdbpy_initialize_frames ();

and you can maybe add some regression tests, if there's a way to test
it and update the 'python' documentation


Cordially,

Kevin