This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Python 3 support, part 1 (non-testsuite part)
>>>>> "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