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)
On Nov 12, 2012, at 3:43 PM, Tom Tromey wrote:
>>>>>> "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.
Thanks!
> 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.
Yes, similar to PyVarType_INIT_HEAD (with the comment you made on that).
> 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?
Because PyModule_Create returns a new reference, while Py_InitModule returns a borrowed reference (bletch).
> 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.
Thanks, yes, and it's a memory leak if you don't. There are several exits from infpy_search_memory unfortunately. Attached is a new diff for py-inferior.c.
>
> 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.
Or Py_TYPE (self)->tp_free (self) -- less change from the existing code. That argument doesn't need a cast, the signature is tp_free (void *).
> 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
Will do.
> Paul> + wchar_t *progname_copy;
>
> Can we really assume a working wchar_t?
Yes, you'd expect a configure check or the like. But the header files for Python reference that type without any checks that I can see. Similarly mbstowcs(). It looks like you can't built Python 3 if those aren't defined (which makes some sense -- how else could you build a program that uses Unicode for all its strings?).
>
> 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.
Ok. That code was adapted from Python 3 code which does it this way. The existing code in python.c calls a whole string of API calls (like PyModule_AddStringConstant) without checking the error status from any of them. Should I add those, with the failure action being to disable Python support in GDB?
> 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?
It adds _gdb to the known builtin (linked in) modules, so that the subsequent "import _gdb" will work.
> Paul> +#endif
> Paul> #endif /* HAVE_PYTHON */
>
> A blank line between these two would be nice.
Ok.
>
> Tom
Index: py-inferior.c
===================================================================
RCS file: /cvs/src/src/gdb/python/py-inferior.c,v
retrieving revision 1.25
diff -u -r1.25 py-inferior.c
--- py-inferior.c 22 Aug 2012 15:17:21 -0000 1.25
+++ py-inferior.c 12 Nov 2012 21:03:41 -0000
@@ -454,9 +454,14 @@
membuf_obj->addr = addr;
membuf_obj->length = length;
+#ifdef IS_PY3K
+ result = PyMemoryView_FromObject ((PyObject *) membuf_obj);
+#else
result = PyBuffer_FromReadWriteObject ((PyObject *) membuf_obj, 0,
Py_END_OF_BUFFER);
+#endif
Py_DECREF (membuf_obj);
+
return result;
}
@@ -476,12 +481,22 @@
PyObject *addr_obj, *length_obj = NULL;
volatile struct gdb_exception except;
static char *keywords[] = { "address", "buffer", "length", NULL };
+#ifdef IS_PY3K
+ Py_buffer pybuf;
+ if (! PyArg_ParseTupleAndKeywords (args, kw, "Os*|O", keywords,
+ &addr_obj, &pybuf,
+ &length_obj))
+ return NULL;
+ buffer = pybuf.buf;
+ buf_len = pybuf.len;
+#else
if (! PyArg_ParseTupleAndKeywords (args, kw, "Os#|O", keywords,
&addr_obj, &buffer, &buf_len,
&length_obj))
return NULL;
+#endif
TRY_CATCH (except, RETURN_MASK_ALL)
{
@@ -502,6 +517,10 @@
}
GDB_PY_HANDLE_EXCEPTION (except);
+#ifdef IS_PY3K
+ PyBuffer_Release (pybuf);
+#endif
+
if (error)
return NULL;
@@ -528,6 +547,23 @@
pulongest (membuf_obj->length));
}
+#ifdef IS_PY3K
+static int
+get_buffer (PyObject *self, Py_buffer *buf, int flags)
+{
+ membuf_object *membuf_obj = (membuf_object *) self;
+ int ret;
+
+ ret = PyBuffer_FillInfo (buf, self, membuf_obj->buffer,
+ membuf_obj->length, 0,
+ PyBUF_CONTIG);
+ buf->format = "c";
+
+ return ret;
+}
+
+#else
+
static Py_ssize_t
get_read_buffer (PyObject *self, Py_ssize_t segment, void **ptrptr)
{
@@ -572,6 +608,8 @@
return ret;
}
+#endif /* IS_PY3K */
+
/* Implementation of
gdb.search_memory (address, length, pattern). ADDRESS is the
address to start the search. LENGTH specifies the scope of the
@@ -585,17 +623,41 @@
{
CORE_ADDR start_addr, length;
static char *keywords[] = { "address", "length", "pattern", NULL };
- PyObject *pattern, *start_addr_obj, *length_obj;
+ PyObject *start_addr_obj, *length_obj;
volatile struct gdb_exception except;
Py_ssize_t pattern_size;
const void *buffer;
CORE_ADDR found_addr;
int found = 0;
+#ifdef IS_PY3K
+ Py_buffer pybuf;
- if (! PyArg_ParseTupleAndKeywords (args, kw, "OOO", keywords,
+ if (! PyArg_ParseTupleAndKeywords (args, kw, "OOs*", keywords,
&start_addr_obj, &length_obj,
+ &pybuf))
+ return NULL;
+
+ buffer = pybuf.buf;
+ pattern_size = pybuf.len;
+#else
+ PyObject *pattern;
+
+ if (! PyArg_ParseTupleAndKeywords (args, kw, "OOO", keywords,
+ &start_addr_obj, &length_obj,
&pattern))
+ return NULL;
+
+ if (!PyObject_CheckReadBuffer (pattern))
+ {
+ PyErr_SetString (PyExc_RuntimeError,
+ _("The pattern is not a Python buffer."));
+
+ return NULL;
+ }
+
+ if (PyObject_AsReadBuffer (pattern, &buffer, &pattern_size) == -1)
return NULL;
+#endif
if (get_addr_from_python (start_addr_obj, &start_addr)
&& get_addr_from_python (length_obj, &length))
@@ -604,6 +666,10 @@
{
PyErr_SetString (PyExc_ValueError,
_("Search range is empty."));
+
+#ifdef IS_PY3K
+ PyBuffer_Release (pybuf);
+#endif
return NULL;
}
/* Watch for overflows. */
@@ -613,23 +679,15 @@
PyErr_SetString (PyExc_ValueError,
_("The search range is too large."));
+#ifdef IS_PY3K
+ PyBuffer_Release (pybuf);
+#endif
return NULL;
}
}
else
return NULL;
- if (!PyObject_CheckReadBuffer (pattern))
- {
- PyErr_SetString (PyExc_RuntimeError,
- _("The pattern is not a Python buffer."));
-
- return NULL;
- }
-
- if (PyObject_AsReadBuffer (pattern, &buffer, &pattern_size) == -1)
- return NULL;
-
TRY_CATCH (except, RETURN_MASK_ALL)
{
found = target_search_memory (start_addr, length,
@@ -638,6 +696,10 @@
}
GDB_PY_HANDLE_EXCEPTION (except);
+#ifdef IS_PY3K
+ PyBuffer_Release (pybuf);
+#endif
+
if (found)
return PyLong_FromLong (found_addr);
else
@@ -777,8 +839,7 @@
static PyTypeObject inferior_object_type =
{
- PyObject_HEAD_INIT (NULL)
- 0, /* ob_size */
+ PyVarObject_HEAD_INIT (NULL, 0)
"gdb.Inferior", /* tp_name */
sizeof (inferior_object), /* tp_basicsize */
0, /* tp_itemsize */
@@ -817,6 +878,13 @@
0 /* tp_alloc */
};
+#ifdef IS_PY3K
+static PyBufferProcs buffer_procs = {
+ get_buffer
+};
+
+#else
+
/* Python doesn't provide a decent way to get compatibility here. */
#if HAVE_LIBPYTHON2_4
#define CHARBUFFERPROC_NAME getcharbufferproc
@@ -832,10 +900,10 @@
Python 2.5. */
(CHARBUFFERPROC_NAME) get_char_buffer
};
+#endif /* IS_PY3K */
static PyTypeObject membuf_object_type = {
- PyObject_HEAD_INIT (NULL)
- 0, /*ob_size*/
+ PyVarObject_HEAD_INIT (NULL, 0)
"gdb.Membuf", /*tp_name*/
sizeof (membuf_object), /*tp_basicsize*/
0, /*tp_itemsize*/