Re: GDB 7.2 - Patch proposal for the use of GDB/Python scripts on MinGW

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Serge" == Serge CHATROUX <serge.chatroux@st.com> writes:

Thank you for doing this.

Serge> I compile gdb 7.2 using MINGW tools and I enable the Python
Serge> scripting support by giving the path to a Python for Windows
Serge> install directory.

Do you have a copyright assignment in place for gdb?

If not, email me and we can get the process started.

Serge> I also modify the gdb/python sources to solve the following issues:

It is generally better if you send separate patches for each thing.
Also, we like submissions to include a ChangeLog entry... see
src/gdb/CONTRIBUTE for various other things.

Serge> + #ifdef __MINGW32__
Serge> +   /* 
Serge> + 	 I have to modify some gdb python files because the files 'python-function.c', python-cmd.c and python-frame.c define some 'static PyTypeObject'.
Serge> +      The field 'tp_new' of these static variables statically initialized with the python 'PyType_GenericNew' function. 
Serge> +      In Windows port of Python, this function is declared as a dllimport function and can not be copied at compilation time in a static variable.
Serge> +      For these 3 files, I modify the source code to initialize the 'tp_new' fields to '0' in the static variables and to affect the proper 'PyType_GenericNew' value in the 'gdbpy_initialize_commands', 'gdbpy_initialize_frames' and 'gdbpy_initialize_functions'.
Serge> +   */
Serge> +   cmdpy_object_type.tp_new = PyType_GenericNew;
Serge> + #endif

Just make your patch do this unconditionally on all hosts, with a little
(one line -- not as long as what you have above) comment explaining
where it is needed.  I think that is clearer than using #ifdef all over.

This change could be one patch, it could go in very quickly, because I
think there is only one way to do this, and so it could bypass the
paperwork requirements.

Serge> + /* Do not enable python scripting if the PYTHONHOME variable is undefined */
Serge> + int havePython = 1;

Should be static.  Also GDB doesn't use mixed case like that, so it
needs a different name.

Serge> + int have_python()

Space before the open paren.  This change is needed throughout your
patch.

Also it should read `(void)', not `()'.

Serge> *************** eval_python_from_control_command (struct
Serge> *** 147,152 ****
Serge> --- 154,165 ----
Serge>     char *script;
Serge>     struct cleanup *cleanup;
  
Serge> +   /* Do not enable python scripting if the PYTHONHOME variable is undefined */
Serge> +   if (havePython == 0)

There should not be a way to get here if python is disabled, so I think
this could just be an assertion.

Serge> *************** python_command (char *arg, int from_tty)
[...]
Serge> + #ifdef HAVE_PYTHON
Serge> +   if (havePython == 0)
Serge> +     {

I don't think this #ifdef is needed; actually, this seems to be true of
all the tests of HAVE_PYTHON in the patch.

There are 2 copies of python_command, and the one you are modifying is
already in an #ifdef HAVE_PYTHON

Serge> +       while (arg && *arg && isspace (*arg))
Serge> + 	++arg;
Serge> +       if (arg && *arg)
Serge> + 	error (_("Python scripting is not supported when the PYTHONHOME variable is undefined."));
Serge> +       else
Serge> + 	{
Serge> + 	  struct command_line *l = get_command_line (python_control, "");
Serge> + 	  struct cleanup *cleanups = make_cleanup_free_command_lines (&l);
Serge> + 	  execute_control_command_untraced (l);
Serge> + 	  do_cleanups (cleanups);
Serge> + 	}
Serge> +       return;
Serge> +     }
Serge> + #endif /* HAVE_PYTHON */

Also I don't think this needs to be duplicated.
If !havePython, then both forms of the python command should simply call
error.

Serge> *************** source_python_script (FILE *stream, cons
[...]  
Serge> + #ifdef HAVE_PYTHON

Likewise.

Serge> +   /* Do not enable python scripting if the PYTHONHOME variable is undefined */
Serge> +   if (havePython == 0)
Serge> +     {
Serge> +       error (_("Python scripting is not supported when the PYTHONHOME variable is undefined."));
Serge> +       return;

`error' calls longjmp -- it is an exception facility written in C.
So, you don't need this `return' here.

Serge> + #ifdef __MINGW32__
Serge> +   {
Serge> +     /* We can not use 'PyRun_SimpleFile' because Python may not be compiled using the same MSVCRT DLL than GDB
Serge> +        so the FILE* stream will not be known in this DLL. This generate an error when the MSVCRT will try to lock the file handle. */

These lines are too long.  In GDB we wrap at column 79 or so; the GNU
coding standards cover this.

Serge> +     PyObject* PyFileObject = PyFile_FromString( (char*) file, "r"); 

The spacing is different from our standard.  It should look like:

    PyObject *file_object = PyFile_FromString ((char *) file, "r");

Perhaps this code and some surrounding code should be refactored so that
we can just avoid FILE* and use the same code path on all hosts.

Serge> +   /* Do not enable python scripting if the PYTHONHOME variable is undefined */
Serge> + #ifdef HAVE_PYTHON
Serge> +   if (getenv( "PYTHONHOME") == NULL)
Serge> +     {

This doesn't seem correct to me.

It definitely isn't ok for the Linux distro case.  There, Python comes
with the system and basically nobody sets PYTHONHOME.

It isn't clear to me that it is always correct even on MinGW.  Couldn't
someone ship a pre-built GDB plus a pre-built Python in the same tree,
and expect it to work properly?

What is the failure mode here?  GDB finds the python DLL but still
somehow doesn't work?

Tom