Re: [PATCH] Python 3 support, part 1 (non-testsuite part)

[Tom Tromey <tromey@redhat.com>]

>>>>> "Paul" ==   <Paul_Koning@Dell.com> writes:

Paul> The attached set of patches updates the Python scripting support
Paul> in GDB to add Python 3 to the existing Python 2 support.

Thanks.

First, I think this is definitely something we want to support.  And, I
think you did it the right way, by making it possible to support both
versions with the same source tree.

Paul> 1. Type objects have a PyVarObject header, and because of the
Paul> syntax changes in the underlying macros in Python 3, you need a
Paul> PyVarObject_HEAD_INIT macro to initialize those.  For the same
Paul> reason, calls to the tp_free method need to go through a PyObject
Paul> * pointer, not a type object pointer.

I think the ob_type stuff should be accessed using the Py_TYPE macro.
IIUC this is new in 2.6 or 2.7, but we can easily define it
conditionally.

Paul> The ones that need to be different between the two versions are
Paul> conditional on IS_PY3K (as suggested in the Porting Python 2 to
Paul> Python 3 manual from python.org).

Sounds good to me.

Paul> Tested on Linux with Python version 2.4, 2.6, 2.7, 3.2, and 3.3.
Paul> No regressions on any of the tests.

Paul> Ok to commit?

I have some comments on the patch, but nothing too serious.

Paul>  PyTypeObject block_object_type = {
Paul> -  PyObject_HEAD_INIT (NULL)
Paul> -  0,				  /*ob_size*/
Paul> +  PyVarObject_HEAD_INIT (NULL, 0)

All the changes to use this macro are ok.  You can put them in
separately if you are so motivated (but see the note for
python-internal.h).

Paul> @@ -44,7 +59,11 @@
Paul>  void
Paul>  gdbpy_initialize_py_events (void)
Paul>  {
Paul> +#ifdef IS_PY3K
Paul> +  gdb_py_events.module = PyModule_Create (&EventModuleDef);
Paul> +#else
Paul>    gdb_py_events.module = Py_InitModule ("events", NULL);
Paul> +#endif

I was going to suggest a convenience function here, but I see we only
have two uses, so it doesn't matter all that much to me; but...

Paul> +#ifndef IS_PY3K
Paul>    Py_INCREF (gdb_py_events.module);
Paul> +#endif

Why is this needed?

Paul> +#ifdef IS_PY3K
Paul> +  Py_buffer pybuf;
 
Paul> +  if (! PyArg_ParseTupleAndKeywords (args, kw, "Os*|O", keywords,
Paul> +				     &addr_obj, &pybuf,
Paul> +				     &length_obj))
Paul> +    return NULL;

At least in 2.7, if you use 's*' it appears you must call
PyBuffer_Release at the appropriate moment.

Paul> +++ gdb/python/py-objfile.c	12 Nov 2012 15:51:30 -0000
Paul> @@ -58,7 +58,7 @@
Paul>    objfile_object *self = (objfile_object *) o;
 
Paul>    Py_XDECREF (self->printers);
Paul> -  self->ob_type->tp_free ((PyObject *) self);
Paul> +  o->ob_type->tp_free ((PyObject *) self);
Paul>  }

One of the spots where we could write

    Py_TYPE (o)->tp_free ((PyObject *) self);

... with Py_TYPE conditionally defined in python-internal.h.

Paul> +/* Python 2.4 does not define PyVarObject_HEAD_INIT.  */
Paul> +#define PyVarObject_HEAD_INIT(type, size)       \
Paul> +    PyObject_HEAD_INIT(type) size,

According to the docs, this wasn't introduced until Python 2.5
So rather than putting it in the HAVE_LIBPYTHON2_4 section, how about
conditionally defining it like:

#ifndef PyVarObject_HEAD_INIT
#define ...
#endif

Paul> +  wchar_t *progname_copy;

Can we really assume a working wchar_t?

Paul> +  if (progsize == (size_t) -1)
Paul> +    {
Paul> +      fprintf(stderr, "Could not convert python path to string\n");
Paul> +      exit (1);
Paul> +    }

I think if Python initialization fails, we should disable Python and
keep going.  It should not be a fatal error.

Paul> +  progname_copy = PyMem_Malloc((progsize + 1) * sizeof (wchar_t));

Missing space before paren.

Paul> +  count = mbstowcs (progname_copy, progname, progsize + 1);

Another portability question here.

Paul> +  if (count == (size_t) -1) {

Wrong brace placement.

Paul> +  _PyImport_FixupBuiltin (gdb_module, "_gdb");

What does this do?

Paul> +#endif
Paul>  #endif /* HAVE_PYTHON */

A blank line between these two would be nice.

Tom