From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7342 invoked by alias); 12 Nov 2012 20:43:32 -0000 Received: (qmail 7334 invoked by uid 22791); 12 Nov 2012 20:43:32 -0000 X-SWARE-Spam-Status: No, hits=-6.6 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_BJ X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 12 Nov 2012 20:43:25 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qACKhNDH015888 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 12 Nov 2012 15:43:23 -0500 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id qACKhLgG011550 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Mon, 12 Nov 2012 15:43:22 -0500 From: Tom Tromey To: Cc: Subject: Re: [PATCH] Python 3 support, part 1 (non-testsuite part) References: Date: Mon, 12 Nov 2012 20:43:00 -0000 In-Reply-To: (Paul Koning's message of "Mon, 12 Nov 2012 16:06:11 +0000") Message-ID: <87mwyma6xi.fsf@fleche.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain 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/msg00298.txt.bz2 >>>>> "Paul" == 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