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

[Thiago Jung Bauermann <bauerman@br.ibm.com>]

Daniel Jacobowitz wrote:

> 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.

Actually I prefer to keep it this way. I am following the existing
convention used to build other GDB components which live in their own
subdirectories (cli/, mi/ and tui/).

Since by the end of this patch series there will be 13 Python-related .c
and .h files, I prefer to put them in their own subdirectory. IMHO having a
flat source tree, with all source files lumped together, is more confusing
than an object file named differently (but not much) than its source file.
 
>> @@ -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.

Indeed. I just tried saving face by reproducing the problem which prompted
me to add python/python.c there in the first place, but couldn't. I don't
remember why I needed it at the time.

>> +# 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.

I looked into using python-config, and found a problem: the CFLAGS it
provides is just a copy of all the CFLAGS used when building the Python
interpreter itself, including options which would make the python-related
objects be compiled differently from the rest of GDB. For instance, in my
system it includes -O2 (what if want to compile GDB with -O0?). In another
system it also includes -fPIC.

There's an open python ticket about this: http://bugs.python.org/issue3290

So I don't know yet how to solve the problem. :-/
Even if they fix it upstream, we want to support the broken systems out
there.

Perhaps get the python-config output and filter out options from a
blacklist?

>> +  /* 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?

I like the idea, will play with it...

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

... and provide a separate patch later.

>> +/* 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.

You mean having #ifdefs inside _initialize_python itself?

> This patch is enough for a first NEWS entry.
> 
> Otherwise OK :-)

Awesome. Now working in a respin of the patch...
-- 
[]'s
Thiago Jung Bauermann
Software Engineer
IBM Linux Technology Center