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

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

[Serge CHATROUX]

[-- Attachment #1: Type: text/plain, Size: 2502 bytes --]

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

    gdb/configure --with-x=no --with-python=/c/DATA/tmp/build/MINGW32_NT-5.1/install-python 

Python for windows
------------------

I get the python 2.6.x from http://www.python.org/download/

I have to make some modifications in Python distribution because header files and libraries directories are not the same between Linux and Windows Python distribution.
In order to fit gdb sources (Linux-based), I modify the Python distribution:

- Go to python windows install directory
- Create directory include\python2.6
- Move files from include to include\python2.6
- Copy file libs\libpython26.a to lib directory and rename it to libpython2.6.a

In the future, it could be useful that GDB sources (configure, Makefile.in) fit also Python  Windows-specific packaging.

GDB modifications
-----------------

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

- Add some tests to disable Python scripting if Python is not available on the host at runtime. 
  I detect if the PYTHONHOME variable is set. This enhancement is maybe not useful for everybody.
- I have to modify some gdb python files because the files 'python-function.c', python-cmd.c (...) define some 'static PyTypeObject'.
  The field 'tp_new' of these static variables is statically initialized with the python 'PyType_GenericNew' function. 
  In Windows port of Python, this function is declared as a dllimport one and cannot be copied at compilation time in a static variable.
  For these files, I modified 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' (...). 
- I also modified python-config.py to get python shared library path when Python is compiled in shared library mode. Otherwise the shared library is taken from Linux distribution, not from the Python given by --with-python flag.
- When sourcing a Python script using 'source file.py' command, we cannot use 'PyRun_SimpleFile' because Python may not be compiled using the same MSVCRT DLL than GDB, so the FILE* stream will not be known in this DLL.
  This generates an error when the MSVCRT will try to lock the file handle.

I hope that this patch will be useful and that I followed the gdb coding rules.

Regards

[-- Attachment #2: gdb.python-for-mingw.diff --]
[-- Type: application/octet-stream, Size: 15076 bytes --]

diff -cp python-gdb-7.2/py-cmd.c python-gdb/py-cmd.c
*** python-gdb-7.2/py-cmd.c	Fri Sep 10 09:27:40 2010
--- python-gdb/py-cmd.c	Fri Sep 10 11:08:00 2010
*************** gdbpy_initialize_commands (void)
*** 526,531 ****
--- 526,541 ----
    if (PyType_Ready (&cmdpy_object_type) < 0)
      return;
  
+ #ifdef __MINGW32__
+   /* 
+ 	 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'.
+      The field 'tp_new' of these static variables statically initialized with the python 'PyType_GenericNew' function. 
+      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.
+      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'.
+   */
+   cmdpy_object_type.tp_new = PyType_GenericNew;
+ #endif
+ 
    /* Note: alias and user are special; pseudo appears to be unused,
       and there is no reason to expose tui or xdb, I think.  */
    if (PyModule_AddIntConstant (gdb_module, "COMMAND_NONE", no_class) < 0
*************** static PyTypeObject cmdpy_object_type =
*** 610,616 ****
--- 620,630 ----
    0,				  /* tp_dictoffset */
    cmdpy_init,			  /* tp_init */
    0,				  /* tp_alloc */
+ #ifdef __MINGW32__
+   0				  /* tp_new */
+ #else
    PyType_GenericNew		  /* tp_new */
+ #endif
  };
  
  \f
diff -cp python-gdb-7.2/py-frame.c python-gdb/py-frame.c
*** python-gdb-7.2/py-frame.c	Fri Sep 10 09:27:40 2010
--- python-gdb/py-frame.c	Fri Sep 10 11:08:00 2010
*************** gdbpy_initialize_frames (void)
*** 568,573 ****
--- 568,583 ----
    if (PyType_Ready (&frame_object_type) < 0)
      return;
  
+ #ifdef __MINGW32__
+   /* 
+      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'.
+      The field 'tp_new' of these static variables statically initialized with the python 'PyType_GenericNew' function. 
+      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.
+      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'.
+   */
+   frame_object_type.tp_new = PyType_GenericNew;
+ #endif
+ 
    /* Note: These would probably be best exposed as class attributes of Frame,
       but I don't know how to do it except by messing with the type's dictionary.
       That seems too messy.  */
*************** static PyTypeObject frame_object_type = 
*** 672,676 ****
--- 682,690 ----
    0,				  /* tp_dictoffset */
    0,				  /* tp_init */
    0,				  /* tp_alloc */
+ #ifdef __MINGW32__
+   0				  /* tp_new */
+ #else
    PyType_GenericNew		  /* tp_new */
+ #endif
  };
diff -cp python-gdb-7.2/py-function.c python-gdb/py-function.c
*** python-gdb-7.2/py-function.c	Fri Sep 10 09:27:40 2010
--- python-gdb/py-function.c	Fri Sep 10 11:08:00 2010
*************** gdbpy_initialize_functions (void)
*** 130,135 ****
--- 130,145 ----
    if (PyType_Ready (&fnpy_object_type) < 0)
      return;
  
+ #ifdef __MINGW32__
+   /* 
+      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'.
+      The field 'tp_new' of these static variables statically initialized with the python 'PyType_GenericNew' function. 
+      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.
+      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'.
+   */
+   fnpy_object_type.tp_new = PyType_GenericNew;
+ #endif
+ 
    Py_INCREF (&fnpy_object_type);
    PyModule_AddObject (gdb_module, "Function", (PyObject *) &fnpy_object_type);
  }
*************** static PyTypeObject fnpy_object_type =
*** 176,180 ****
--- 186,194 ----
    0,				  /* tp_dictoffset */
    fnpy_init,			  /* tp_init */
    0,				  /* tp_alloc */
+ #ifdef __MINGW32__
+   0				  /* tp_new */
+ #else
    PyType_GenericNew		  /* tp_new */
+ #endif
  };
diff -cp python-gdb-7.2/py-inferior.c python-gdb/py-inferior.c
*** python-gdb-7.2/py-inferior.c	Fri Sep 10 09:27:40 2010
--- python-gdb/py-inferior.c	Fri Sep 10 11:08:00 2010
*************** gdbpy_initialize_inferior (void)
*** 593,598 ****
--- 593,608 ----
    if (PyType_Ready (&membuf_object_type) < 0)
      return;
  
+ #ifdef __MINGW32__
+   /* 
+      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'.
+      The field 'tp_new' of these static variables statically initialized with the python 'PyType_GenericNew' function. 
+      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.
+      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'.
+   */
+   membuf_object_type.tp_new = PyType_GenericNew;
+ #endif
+ 
    Py_INCREF (&membuf_object_type);
    PyModule_AddObject (gdb_module, "Membuf", (PyObject *)
  		      &membuf_object_type);
*************** static PyTypeObject membuf_object_type =
*** 724,728 ****
--- 734,742 ----
    0,				  /* tp_dictoffset */
    0,				  /* tp_init */
    0,				  /* tp_alloc */
+ #ifdef __MINGW32__
+   0				  /* tp_new */
+ #else
    PyType_GenericNew		  /* tp_new */
+ #endif
  };
diff -cp python-gdb-7.2/py-param.c python-gdb/py-param.c
*** python-gdb-7.2/py-param.c	Fri Sep 10 09:27:40 2010
--- python-gdb/py-param.c	Fri Sep 10 11:08:00 2010
*************** gdbpy_initialize_parameters (void)
*** 555,560 ****
--- 555,570 ----
    if (PyType_Ready (&parmpy_object_type) < 0)
      return;
  
+ #ifdef __MINGW32__
+   /* 
+      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'.
+      The field 'tp_new' of these static variables statically initialized with the python 'PyType_GenericNew' function. 
+      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.
+      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'.
+   */
+   parmpy_object_type.tp_new = PyType_GenericNew;
+ #endif
+ 
    set_doc_cst = PyString_FromString ("set_doc");
    if (! set_doc_cst)
      return;
*************** static PyTypeObject parmpy_object_type =
*** 617,621 ****
--- 627,635 ----
    0,				  /* tp_dictoffset */
    parmpy_init,			  /* tp_init */
    0,				  /* tp_alloc */
+ #ifdef __MINGW32__
+   0				  /* tp_new */
+ #else
    PyType_GenericNew		  /* tp_new */
+ #endif
  };
diff -cp python-gdb-7.2/py-prettyprint.c python-gdb/py-prettyprint.c
*** python-gdb-7.2/py-prettyprint.c	Fri Sep 10 09:27:40 2010
--- python-gdb/py-prettyprint.c	Fri Sep 10 11:08:00 2010
*************** apply_val_pretty_printer (struct type *t
*** 617,622 ****
--- 617,627 ----
    struct cleanup *cleanups;
    int result = 0;
    int is_py_none = 0;
+ 
+   /* Do not enable python scripting if the PYTHONHOME variable is undefined */
+   if (have_python() == 0)
+     return 0;
+ 
    cleanups = ensure_python_env (gdbarch, language);
  
    /* Instantiate the printer.  */
diff -cp python-gdb-7.2/py-value.c python-gdb/py-value.c
*** python-gdb-7.2/py-value.c	Fri Sep 10 09:27:40 2010
--- python-gdb/py-value.c	Fri Sep 10 11:08:00 2010
*************** preserve_python_values (struct objfile *
*** 158,163 ****
--- 158,167 ----
  {
    value_object *iter;
  
+   /* Do not enable python scripting if the PYTHONHOME variable is undefined */
+   if (have_python() == 0)
+     return;
+ 
    for (iter = values_in_python; iter; iter = iter->next)
      preserve_one_value (iter->value, objfile, copied_types);
  }
diff -cp python-gdb-7.2/python-config.py python-gdb/python-config.py
*** python-gdb-7.2/python-config.py	Fri Sep 10 09:27:40 2010
--- python-gdb/python-config.py	Fri Sep 10 11:08:00 2010
*************** elif opt in ('--libs', '--ldflags'):
*** 50,54 ****
--- 50,59 ----
      # shared library in prefix/lib/.
      if opt == '--ldflags' and not getvar('Py_ENABLE_SHARED'):
          libs.insert(0, '-L' + getvar('LIBPL'))
+ 
+     # Get python shared library path
+     if opt == '--ldflags' and getvar('Py_ENABLE_SHARED'):
+         libs.insert(0, '-L' + getvar('LIBDIR'))        
+         
      print ' '.join(libs)
  
diff -cp python-gdb-7.2/python-internal.h python-gdb/python-internal.h
*** python-gdb-7.2/python-internal.h	Fri Sep 10 09:27:40 2010
--- python-gdb/python-internal.h	Fri Sep 10 11:08:00 2010
***************
*** 26,31 ****
--- 26,34 ----
     needed by pyport.h.  */
  #include <stdint.h>
  
+ /* Do not enable python scripting if the PYTHONHOME variable is undefined */
+ int have_python();
+ 
  /* /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
diff -cp python-gdb-7.2/python.c python-gdb/python.c
*** python-gdb-7.2/python.c	Fri Sep 10 09:27:40 2010
--- python-gdb/python.c	Fri Sep 10 11:08:00 2010
*************** PyObject *gdbpy_enabled_cst;
*** 61,66 ****
--- 61,73 ----
  /* The GdbError exception.  */
  PyObject *gdbpy_gdberror_exc;
  
+ /* Do not enable python scripting if the PYTHONHOME variable is undefined */
+ int havePython = 1;
+ int have_python()
+ {
+   return havePython;
+ }
+ 
  /* Architecture and language to be used in callbacks from
     the Python interpreter.  */
  struct gdbarch *python_gdbarch;
*************** eval_python_from_control_command (struct
*** 147,152 ****
--- 154,165 ----
    char *script;
    struct cleanup *cleanup;
  
+   /* Do not enable python scripting if the PYTHONHOME variable is undefined */
+   if (havePython == 0)
+     {
+       return;
+     }
+ 
    if (cmd->body_count != 1)
      error (_("Invalid \"python\" block structure."));
  
*************** python_command (char *arg, int from_tty)
*** 171,176 ****
--- 184,209 ----
  {
    struct cleanup *cleanup;
  
+   /* Do not enable python scripting if the PYTHONHOME variable is undefined */
+ #ifdef HAVE_PYTHON
+   if (havePython == 0)
+     {
+       while (arg && *arg && isspace (*arg))
+ 	++arg;
+       if (arg && *arg)
+ 	error (_("Python scripting is not supported when the PYTHONHOME variable is undefined."));
+       else
+ 	{
+ 	  struct command_line *l = get_command_line (python_control, "");
+ 	  struct cleanup *cleanups = make_cleanup_free_command_lines (&l);
+ 	  execute_control_command_untraced (l);
+ 	  do_cleanups (cleanups);
+ 	}
+       return;
+     }
+ #endif /* HAVE_PYTHON */
+ 
+ 
    cleanup = ensure_python_env (get_current_arch (), current_language);
    while (arg && *arg && isspace (*arg))
      ++arg;
*************** source_python_script (FILE *stream, cons
*** 403,413 ****
--- 436,467 ----
  {
    struct cleanup *cleanup;
  
+ #ifdef HAVE_PYTHON
+   /* Do not enable python scripting if the PYTHONHOME variable is undefined */
+   if (havePython == 0)
+     {
+       error (_("Python scripting is not supported when the PYTHONHOME variable is undefined."));
+       return;
+     }
+ #endif /* HAVE_PYTHON */
+ 
    cleanup = ensure_python_env (get_current_arch (), current_language);
  
+ #ifdef __MINGW32__
+   {
+     /* We can not use 'PyRun_SimpleFile' because Python may not be compiled using the same MSVCRT DLL than GDB
+        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. */
+     PyObject* PyFileObject = PyFile_FromString( (char*) file, "r"); 
+     if (PyFileObject == NULL)
+       error (_("Could not source file %s."), file);
+     PyRun_SimpleFile(PyFile_AsFile(PyFileObject), file); 
+     Py_DECREF(PyFileObject);
+   }
+ #else
    /* Note: If an exception occurs python will print the traceback and
       clear the error indicator.  */
    PyRun_SimpleFile (stream, file);
+ #endif
  
    do_cleanups (cleanup);
  }
*************** source_python_script_for_objfile (struct
*** 516,524 ****
--- 570,590 ----
    cleanups = ensure_python_env (get_objfile_arch (objfile), current_language);
    gdbpy_current_objfile = objfile;
  
+ #ifdef __MINGW32__
+   {
+     /* We can not use 'PyRun_SimpleFile' because Python may not be compiled using the same MSVCRT DLL than GDB
+        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. */
+     PyObject* PyFileObject = PyFile_FromString( (char*) file, "r"); 
+     if (PyFileObject == NULL)
+       error (_("Could not source file %s."), file);
+     PyRun_SimpleFile(PyFile_AsFile(PyFileObject), file); 
+     Py_DECREF(PyFileObject);
+   }
+ #else
    /* Note: If an exception occurs python will print the traceback and
       clear the error indicator.  */
    PyRun_SimpleFile (stream, file);
+ #endif
  
    do_cleanups (cleanups);
    gdbpy_current_objfile = NULL;
*************** extern initialize_file_ftype _initialize
*** 633,638 ****
--- 699,719 ----
  void
  _initialize_python (void)
  {
+   /* Do not enable python scripting if the PYTHONHOME variable is undefined */
+ #ifdef HAVE_PYTHON
+   if (getenv( "PYTHONHOME") == NULL)
+     {
+        havePython = 0;
+        add_com ("python", class_obscure, python_command,
+ 		_("\
+ Evaluate a Python command.\n\
+ \n\
+ Python scripting is not supported when the PYTHONHOME variable is undefined.")
+ 		);
+     }
+   else
+ #endif /* HAVE_PYTHON */
+ 
    add_com ("python", class_obscure, python_command,
  #ifdef HAVE_PYTHON
  	   _("\
*************** Enables or disables printing of Python s
*** 683,688 ****
--- 764,778 ----
    Py_SetProgramName (concat (ldirname (python_libdir), SLASH_STRING, "bin",
  			     SLASH_STRING, "python", NULL));
  #endif
+ 
+   /* Do not enable python scripting if the PYTHONHOME variable is undefined */
+ #ifdef HAVE_PYTHON
+   if (havePython == 0)
+     {
+       return;
+     }
+ #endif /* HAVE_PYTHON */
+ 
  
    Py_Initialize ();
    PyEval_InitThreads ();

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

[Tom Tromey]

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

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

[Daniel Jacobowitz]

On Fri, Sep 10, 2010 at 03:25:45PM +0200, Serge CHATROUX wrote:
> I compile gdb 7.2 using MINGW tools and I enable the Python
> scripting support by giving the path to a Python for Windows install
> directory.

FYI, we've been building GDB for MinGW with Python support for six
months, and haven't encountered any of these problems.

I'm not surprised by the naming problems since I think our build
system rearranges the files.  But I am surprised that you had to
modify the GDB sources.

> I also modify the gdb/python sources to solve the following issues:
> 
> - Add some tests to disable Python scripting if Python is not available on the host at runtime. 
>   I detect if the PYTHONHOME variable is set. This enhancement is
>   maybe not useful for everybody.

What are you really detecting missing here?  The Python DLL must be
present; we link directly to it so GDB would not start if it wasn't.

> - I have to modify some gdb python files because the files 'python-function.c', python-cmd.c (...) define some 'static PyTypeObject'.
>   The field 'tp_new' of these static variables is statically initialized with the python 'PyType_GenericNew' function. 
>   In Windows port of Python, this function is declared as a dllimport one and cannot be copied at compilation time in a static variable.
>   For these files, I modified 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' (...). 

This is the one that particularly puzzles me, we didn't have this problem.

> - When sourcing a Python script using 'source file.py' command, we cannot use 'PyRun_SimpleFile' because Python may not be compiled using the same MSVCRT DLL than GDB, so the FILE* stream will not be known in this DLL.
>   This generates an error when the MSVCRT will try to lock the file handle.

We did encounter this one; we solved it by making sure that GDB used
the same MSVCRT DLL.  But that solution is definitely not for
everyone, and I'd be happy enough to see GDB avoid this problem.



-- 
Daniel Jacobowitz
CodeSourcery

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

[Christophe Lyon]

On 10.09.2010 17:39, Tom Tromey wrote:
>>>>>> "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?
>

Yes, we do have a copyright assignment for our whole group.

Christophe.

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

[Serge CHATROUX]

> Do you have a copyright assignment in place for gdb?
> If not, email me and we can get the process started.

Yes (I work with Christophe Lyon).

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

I will split the patch into severals.
I also replied (directly) to Daniel Jacobowitz comments and wait its feedback.

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

I can reduce the comment size.
I do not know how to avoid the '#if __MINGW32__' because this modification is not needed for other systems (Linux, cygwin).
It seems that Daniel Jacobowitz succeed in compiling gdb over MINGW without this modification.

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

OK.

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

It could be great. I don't know the name of the Python scripting support maintainers. It could be great to have their feedback.

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

I will remove this part of the patch ($PYTHONHOME detection...), it is not relevant for a standard version.
I set this feature to solve an issue that I had on Linux:
- I compiled Python support without the --enabled-shared support. 
- At gdb compilation time, the Python library was embed (linked with libpython2.6.a not the shared library).
When I use the gdb program on a Linux host on which Python is not installed or without defined PYTHONPATH variable, I got the following message:
	gdb
	'import site' failed; use -v for traceback
	GNU gdb (GDB) 7.2
	Copyright (C) 2010 Free Software Foundation, Inc.
By checking the PYTHONHOME variable, I tried to check that the user set a valid Python environment. 


Regards

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

[Tom Tromey]

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

Serge> +   cmdpy_object_type.tp_new = PyType_GenericNew; #endif

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

Serge> I do not know how to avoid the '#if __MINGW32__' because this
Serge> modification is not needed for other systems (Linux, cygwin).

You can just use the MinGW code everywhere.

Serge> It seems that Daniel Jacobowitz succeed in compiling gdb over
Serge> MINGW without this modification.

Yeah, I don't know about that.  But, from
http://docs.python.org/extending/newtypes.html:

    We’d like to just assign this to the tp_new slot, but we can’t, for
    portability sake, On some platforms or compilers, we can’t statically
    initialize a structure member with a function defined in another C
    module, so, instead, we’ll assign the tp_new slot in the module
    initialization function just before calling PyType_Ready():

So it seems your change might be needed at least sometimes.

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

Serge> It could be great. I don't know the name of the Python scripting
Serge> support maintainers. It could be great to have their feedback.

If you want to do it, it is fine.

Serge> I set this feature to solve an issue that I had on Linux:
Serge> - I compiled Python support without the --enabled-shared support. 

Ok.

How does Python usually work in this setup?

Serge> 	gdb
Serge> 	'import site' failed; use -v for traceback

It seems that we could catch this error and disable the python support
at that time.

This error is very misleading because "gdb -v" won't actually help.
Ugh.

Tom

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

[Serge CHATROUX]

I confirm that the modification to avoid the affectation 'cmdpy_object_type.tp_new = PyType_GenericNew' is not supported by gcc version 3.4.2 (mingw-special) but is now supported by newer compiler (above a 4.3-based, I tried with gcc 4.5.0).

Thank you for the following information:

> Yeah, I don't know about that.  But, from
> http://docs.python.org/extending/newtypes.html:
>
>     We’d like to just assign this to the tp_new slot, but we can’t, for
>     portability sake, On some platforms or compilers, we can’t statically
>     initialize a structure member with a function defined in another C
>     module, so, instead, we’ll assign the tp_new slot in the module
>     initialization function just before calling PyType_Ready():
>
> So it seems your change might be needed at least sometimes.

I look at the Python documentation and I check that some standard modules of Python (sqlite) are conformed  to this rule but other don't (bz2, datetime).
I have to rewrite my patch because the affectation should be done before PyType_Ready() and I did it after!

> Serge> It could be great. I don't know the name of the Python scripting
> Serge> support maintainers. It could be great to have their feedback.
>
>  If you want to do it, it is fine.

I look at the initial list of developer that provides 'python.c' file:
	Vladimir Prus  <vladimir@codesourcery.com>
	Tom Tromey  <tromey@redhat.com>
	Thiago Jung Bauermann  <bauerman@br.ibm.com>
	Doug Evans  <dje@google.com>
I think that you and other maintainers will check my modifications carefully.

> Serge> I set this feature to solve an issue that I had on Linux:
> Serge> - I compiled Python support without the --enabled-shared support. 
>
> Ok.
>
> How does Python usually work in this setup?

Instead of embed the libpython.a, gdb look for the libpython2.6.so.1.0. 
I tried to compile with python --enabled-shared support because a user tells me that he have some issue in its environment.

> Serge> 	gdb
> Serge> 	'import site' failed; use -v for traceback
>
> It seems that we could catch this error and disable the python support at that time.
>
> This error is very misleading because "gdb -v" won't actually help.
> Ugh.

I think that this issue occurs in the '_initialize_python' function that executes a simple script to create a couple objects which are used for Python's stdout and stderr.
For instance, it occurs if I set the PYTHONHOME variable to an invalid value.

In order to submit a new corrected patch, should I start a new mail thread or do I continue to use this mail thread?

Regards

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

[Tom Tromey]

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

Serge> In order to submit a new corrected patch, should I start a new
Serge> mail thread or do I continue to use this mail thread?

I think it is slightly clearer to start a new thread for new patches
(referring back to the older thread by URL if needed to explain some
context); but really either way is ok with me.

Tom