Re: [RFA] Re: [RFC][patch 1/9] initial Python support

Daniel Jacobowitz
2008-07-18T19:50:33Z

On Wed, Jul 16, 2008 at 07:00:20PM -0300, Thiago Jung Bauermann wrote:
> +#
> +# python sub directory definitons
> +#
> +SUBDIR_PYTHON_OBS = \
> + python-utils.o
> +SUBDIR_PYTHON_SRCS = \
> + python/python.c \
> + python/utils.c

Can we call it python-utils.c or something else, instead? IMO having
different naming for the source file and object is very confusing.

> @@ -1283,7 +1303,11 @@ test-cp-name-parser$(EXEEXT): test-cp-name-parser.o $(LIBIBERTY)
> # duplicates. Files in the gdb/ directory can end up appearing in
> # COMMON_OBS (as a .o file) and CONFIG_SRCS (as a .c file).
>
> -INIT_FILES = $(COMMON_OBS) $(TSOBS) $(CONFIG_SRCS)
> +# NOTE: bauermann/2008-06-15: python.c needs to be explicitly included
> +# because the generation of init.c does not work for .c files which
> +# result in differently named objects (i.e., python/python -> python.o).
> +
> +INIT_FILES = $(COMMON_OBS) $(TSOBS) $(CONFIG_SRCS) python/python.c

This comment doesn't make sense. python/python.c is already included
in INIT_FILES, because of CONFIG_SRCS.

> +# Flags needed to compile Python code (taken from python-config --cflags)
> +PYTHON_CFLAGS=-fno-strict-aliasing -DNDEBUG -fwrapv

The output of python-config will vary by platform (that's the point of
having a helper program). So, I'm afraid we have to run python-config
if we need the cflags. Or else we could add these flags
unconditionally, if they work with the current compiler. Consider
non-GCC.

> @@ -538,6 +560,17 @@ execute_control_command (struct command_line *cmd)
> return ret;
> }
>
> +/* Like execute_control_command, but first set
> + suppress_next_print_command_trace. */
> +
> +enum command_control_type
> +execute_control_command_suppressed (struct command_line *cmd)
> +{
> + suppress_next_print_command_trace = 1;
> + return execute_control_command (cmd);
> +}
> +
> +
> /* "while" command support. Executes a body of statements while the
> loop condition is nonzero. */
>

This is an unclear name - suppressed what? Maybe
execute_control_command_untraced is more accurate.

> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 2920179..b1419a7 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -165,6 +165,7 @@ software in general. We will miss him.
> * 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.
>
> * GDB Bugs:: Reporting bugs in @value{GDBN}
>

We should ask Eli to look at the manual.

> +@kindex set python-stack
> +By default, @value{GDBN} will print a stack trace when an error occurs
> +in a Python script. This can be controlled using @code{set python-stack}.

Please expand this; see e.g. Print Settings for the usual way we
document commands. In particular it should say what the values
are/mean.

Also, perhaps this should use a prefix command, like "set python
print-stack"?

There should probably be an explicit table item for the "python"
command, too.

> +/* Python 2.4 doesn't include stdint.h soon enough to get {u,}intptr_t
> + needed by pyport.h. */
> +#ifdef HAVE_STDINT_H
> +#include <stdint.h>
> +#endif

You can include stdint.h unconditionally now; gnulib provides it.

> +/* /usr/include/features.h on linux systems will define _POSIX_C_SOURCE
> + if it sees _GNU_SOURCE (which config.h will define).
> + pyconfig.h defines _POSIX_C_SOURCE to a different value than
> + /usr/include/features.h does causing compilation to fail.
> + To work around this, undef _POSIX_C_SOURCE before we include Python.h. */
> +#undef _POSIX_C_SOURCE

Odd. For me, pyconfig.h and features.h define it to the same thing.
Anyway, this is gross but acceptable.

> +static PyMethodDef GdbMethods[] =
> +{
> + { "execute", execute_gdb_command, METH_VARARGS,
> + "Execute a gdb command" },
> + { "show", get_show_variable, METH_VARARGS,
> + "Return a gdb setting's value" },
> +
> + { "write", gdbpy_write, METH_VARARGS,
> + "Write a string using gdb's filtered stream." },
> + { "flush", gdbpy_flush, METH_NOARGS,
> + "Flush gdb's filtered stdout stream." },
> +
> + {NULL, NULL, 0, NULL}
> +};

I think that these are now "gdb commands", effectively. So, sorry,
but there's a bit more documentation you get to write :-) No new
undocumented public interfaces.

> + /* Create a couple objects which are used for Python's stdout and
> + stderr. */
> + PyRun_SimpleString ("\
> +import sys\n\
> +class GdbOutputFile:\n\
> + def close(self):\n\
> + # Do nothing.\n\
> + return None\n\
> +\n\
> + def isatty(self):\n\
> + return False\n\
> +\n\
> + def write(self, s):\n\
> + gdb.write(s)\n\
> +\n\
> + def writelines(self, iterable):\n\
> + for line in iterable:\n\
> + self.write(line)\n\
> +\n\
> + def flush(self):\n\
> + gdb.flush()\n\
> +\n\
> +sys.stderr = GdbOutputFile()\n\
> +sys.stdout = GdbOutputFile()\n\

This is OK for now, but it is likely to grow. There's code to convert
a text file into a C string, in features/ (used for XML files); maybe
we should internalize a couple of Python source files that way?
Alternatively we could install Python source files to a known path in
the build / install directories; that lets vendors precompile the
files for faster startup.

Anyway, that's a future note, not a current problem.

> +/* Initialize dummy Python code. */
> +
> +void
> +_initialize_python (void)
> +{
> + add_com ("python", class_obscure, python_command, _("\
> +Evaluate a Python command.\n\
> +\n\
> +Python scripting is not supported in this copy of GDB.\n\
> +This command is only a placeholder."));
> +
> + add_setshow_boolean_cmd ("python-stack", class_maintenance,
> + &gdbpy_should_print_stack, _("\
> +Set Python stack printing."), _("\
> +Show Python stack printing."), _("\
> +Enables printing of Python stack traces."),
> + NULL, NULL,
> + &maintenance_set_cmdlist,
> + &maintenance_show_cmdlist);
> +
> +}

If the list of python configuration variables grows, this will get out
of hand; I suggest sharing the init code regardless of HAVE_PYTHON.

This patch is enough for a first NEWS entry.

Otherwise OK :-)

--
Daniel Jacobowitz
CodeSourcery