From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13526 invoked by alias); 12 Nov 2012 21:37:27 -0000 Received: (qmail 13518 invoked by uid 22791); 12 Nov 2012 21:37:26 -0000 X-SWARE-Spam-Status: No, hits=-5.3 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,TW_BJ X-Spam-Check-By: sourceware.org Received: from ausc60pc101.us.dell.com (HELO ausc60pc101.us.dell.com) (143.166.85.206) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 12 Nov 2012 21:37:21 +0000 X-Loopcount0: from 10.175.216.250 From: To: CC: Subject: Re: [PATCH] Python 3 support, part 1 (non-testsuite part) Date: Mon, 12 Nov 2012 21:37:00 -0000 Message-ID: References: <87mwyma6xi.fsf@fleche.redhat.com> In-Reply-To: <87mwyma6xi.fsf@fleche.redhat.com> Content-Type: text/plain; charset="us-ascii" Content-ID: <0D7ABD947C1E654A901F111B039CA771@dell.com> Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-11/txt/msg00301.txt.bz2 On Nov 12, 2012, at 3:43 PM, Tom Tromey wrote: >>>>>> "Paul" =3D=3D writes: >=20 > Paul> The attached set of patches updates the Python scripting support > Paul> in GDB to add Python 3 to the existing Python 2 support. >=20 > Thanks. >=20 > 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. >=20 > 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). >=20 > Sounds good to me. >=20 > 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. >=20 > Paul> Ok to commit? >=20 > I have some comments on the patch, but nothing too serious. >=20 > Paul> PyTypeObject block_object_type =3D { > Paul> - PyObject_HEAD_INIT (NULL) > Paul> - 0, /*ob_size*/ > Paul> + PyVarObject_HEAD_INIT (NULL, 0) >=20 > 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). >=20 > Paul> @@ -44,7 +59,11 @@ > Paul> void > Paul> gdbpy_initialize_py_events (void) > Paul> { > Paul> +#ifdef IS_PY3K > Paul> + gdb_py_events.module =3D PyModule_Create (&EventModuleDef); > Paul> +#else > Paul> gdb_py_events.module =3D Py_InitModule ("events", NULL); > Paul> +#endif >=20 > 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... >=20 > Paul> +#ifndef IS_PY3K > Paul> Py_INCREF (gdb_py_events.module); > Paul> +#endif >=20 > Why is this needed? Because PyModule_Create returns a new reference, while Py_InitModule return= s a borrowed reference (bletch). > Paul> +#ifdef IS_PY3K > Paul> + Py_buffer pybuf; >=20 > Paul> + if (! PyArg_ParseTupleAndKeywords (args, kw, "Os*|O", keywords, > Paul> + &addr_obj, &pybuf, > Paul> + &length_obj)) > Paul> + return NULL; >=20 > 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-infe= rior.c. >=20 > Paul> +++ gdb/python/py-objfile.c 12 Nov 2012 15:51:30 -0000 > Paul> @@ -58,7 +58,7 @@ > Paul> objfile_object *self =3D (objfile_object *) o; >=20 > Paul> Py_XDECREF (self->printers); > Paul> - self->ob_type->tp_free ((PyObject *) self); > Paul> + o->ob_type->tp_free ((PyObject *) self); > Paul> } >=20 > One of the spots where we could write >=20 > Py_TYPE (o)->tp_free ((PyObject *) self); >=20 > ... with Py_TYPE conditionally defined in python-internal.h. Or Py_TYPE (self)->tp_free (self) -- less change from the existing code. T= hat 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, >=20 > 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: >=20 > #ifndef PyVarObject_HEAD_INIT > #define ... > #endif Will do. > Paul> + wchar_t *progname_copy; >=20 > 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 mb= stowcs(). 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 Unic= ode for all its strings?). >=20 > Paul> + if (progsize =3D=3D (size_t) -1) > Paul> + { > Paul> + fprintf(stderr, "Could not convert python path to string\n"); > Paul> + exit (1); > Paul> + } >=20 > 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. Sho= uld I add those, with the failure action being to disable Python support in= GDB? > Paul> + progname_copy =3D PyMem_Malloc((progsize + 1) * sizeof (wchar_t)= ); >=20 > Missing space before paren. >=20 > Paul> + count =3D mbstowcs (progname_copy, progname, progsize + 1); >=20 > Another portability question here. >=20 > Paul> + if (count =3D=3D (size_t) -1) { >=20 > Wrong brace placement. >=20 > Paul> + _PyImport_FixupBuiltin (gdb_module, "_gdb"); >=20 > What does this do? It adds _gdb to the known builtin (linked in) modules, so that the subseque= nt "import _gdb" will work. > Paul> +#endif > Paul> #endif /* HAVE_PYTHON */ >=20 > A blank line between these two would be nice. Ok. >=20 > Tom Index: py-inferior.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D 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 =3D addr; membuf_obj->length =3D length; =20 +#ifdef IS_PY3K + result =3D PyMemoryView_FromObject ((PyObject *) membuf_obj); +#else result =3D PyBuffer_FromReadWriteObject ((PyObject *) membuf_obj, 0, Py_END_OF_BUFFER); +#endif Py_DECREF (membuf_obj); + return result; } =20 @@ -476,12 +481,22 @@ PyObject *addr_obj, *length_obj =3D NULL; volatile struct gdb_exception except; static char *keywords[] =3D { "address", "buffer", "length", NULL }; +#ifdef IS_PY3K + Py_buffer pybuf; =20 + if (! PyArg_ParseTupleAndKeywords (args, kw, "Os*|O", keywords, + &addr_obj, &pybuf, + &length_obj)) + return NULL; =20 + buffer =3D pybuf.buf; + buf_len =3D pybuf.len; +#else if (! PyArg_ParseTupleAndKeywords (args, kw, "Os#|O", keywords, &addr_obj, &buffer, &buf_len, &length_obj)) return NULL; +#endif =20 TRY_CATCH (except, RETURN_MASK_ALL) { @@ -502,6 +517,10 @@ } GDB_PY_HANDLE_EXCEPTION (except); =20 +#ifdef IS_PY3K + PyBuffer_Release (pybuf); +#endif + if (error) return NULL; =20 @@ -528,6 +547,23 @@ pulongest (membuf_obj->length)); } =20 +#ifdef IS_PY3K +static int +get_buffer (PyObject *self, Py_buffer *buf, int flags) +{ + membuf_object *membuf_obj =3D (membuf_object *) self; + int ret; +=20=20 + ret =3D PyBuffer_FillInfo (buf, self, membuf_obj->buffer, + membuf_obj->length, 0,=20 + PyBUF_CONTIG); + buf->format =3D "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; } =20 +#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[] =3D { "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 =3D 0; +#ifdef IS_PY3K + Py_buffer pybuf; =20 - if (! PyArg_ParseTupleAndKeywords (args, kw, "OOO", keywords, + if (! PyArg_ParseTupleAndKeywords (args, kw, "OOs*", keywords, &start_addr_obj, &length_obj, + &pybuf)) + return NULL; + + buffer =3D pybuf.buf; + pattern_size =3D pybuf.len; +#else + PyObject *pattern; +=20=20 + 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) =3D=3D -1) return NULL; +#endif =20 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.")); =20 +#ifdef IS_PY3K + PyBuffer_Release (pybuf); +#endif return NULL; } } else return NULL; =20 - if (!PyObject_CheckReadBuffer (pattern)) - { - PyErr_SetString (PyExc_RuntimeError, - _("The pattern is not a Python buffer.")); - - return NULL; - } - - if (PyObject_AsReadBuffer (pattern, &buffer, &pattern_size) =3D=3D -1) - return NULL; - TRY_CATCH (except, RETURN_MASK_ALL) { found =3D target_search_memory (start_addr, length, @@ -638,6 +696,10 @@ } GDB_PY_HANDLE_EXCEPTION (except); =20 +#ifdef IS_PY3K + PyBuffer_Release (pybuf); +#endif + if (found) return PyLong_FromLong (found_addr); else @@ -777,8 +839,7 @@ =20 static PyTypeObject inferior_object_type =3D { - 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 */ }; =20 +#ifdef IS_PY3K +static PyBufferProcs buffer_procs =3D { + 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 */ =20 static PyTypeObject membuf_object_type =3D { - PyObject_HEAD_INIT (NULL) - 0, /*ob_size*/ + PyVarObject_HEAD_INIT (NULL, 0) "gdb.Membuf", /*tp_name*/ sizeof (membuf_object), /*tp_basicsize*/ 0, /*tp_itemsize*/