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

[Kevin Pouget <kevin.pouget@gmail.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