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

[Daniel Jacobowitz <drow@false.org>]

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