From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5706 invoked by alias); 25 Jun 2010 20:38:32 -0000 Received: (qmail 5694 invoked by uid 22791); 25 Jun 2010 20:38:30 -0000 X-SWARE-Spam-Status: No, hits=-5.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD 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; Fri, 25 Jun 2010 20:38:25 +0000 Received: from int-mx05.intmail.prod.int.phx2.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.18]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o5PKcOgo023804 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 25 Jun 2010 16:38:24 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx05.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o5PKcNPk005297; Fri, 25 Jun 2010 16:38:23 -0400 Received: from opsy.redhat.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id o5PKcMrZ030472; Fri, 25 Jun 2010 16:38:23 -0400 Received: by opsy.redhat.com (Postfix, from userid 500) id 8C3EB3792DB; Fri, 25 Jun 2010 14:38:22 -0600 (MDT) From: Tom Tromey To: Phil Muldoon Cc: gdb-patches ml Subject: Re: [python][patch] Inferior and Thread information support. References: <4BFA6E82.3070704@redhat.com> <4C1B16BF.3040000@redhat.com> <4C209107.9010800@redhat.com> Reply-To: tromey@redhat.com Date: Fri, 25 Jun 2010 20:38:00 -0000 In-Reply-To: <4C209107.9010800@redhat.com> (Phil Muldoon's message of "Tue, 22 Jun 2010 11:31:35 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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: 2010-06/txt/msg00595.txt.bz2 >>>>> "Phil" == Phil Muldoon writes: Phil> I've done this. Threads do not have the same registry mechanism yet, so Phil> I've had to keep the old observer mechanism there. But inferiors use Phil> the new registry cleanup. Phil> I've also simplified the search_memory api as requested. We now take Phil> an address, a length and a buffer only. We now only return the first Phil> match found in the buffer. (and let the user do the iteration in the Phil> scripting side). Phil> Latest patch attached. WDYT? Phil> +PyObject * Phil> +inferior_to_inferior_object (struct inferior *inferior) Phil> +{ Phil> + inferior_object *inf_obj; Phil> + Phil> + inf_obj = inferior_data (inferior, infpy_inf_data_key); Phil> + if (!inf_obj) Phil> + { Phil> + struct cleanup *cleanup; Phil> + cleanup = ensure_python_env (python_gdbarch, python_language); Phil> + Phil> + inf_obj = PyObject_New (inferior_object, &inferior_object_type); [...] Phil> + do_cleanups (cleanup); Phil> + } Phil> + else Phil> + Py_INCREF (inf_obj); Phil> + Phil> + return (PyObject *) inf_obj; Right now the reference counting is inconsistent here. Either a new reference should be returned, and the Py_INCREF always done; or it should return a borrowed reference, and this Py_INCREF removed. I prefer the borrowed reference approach. I try to document this sort of thing in the function's header comment. Phil> + cleanup = ensure_python_env (python_gdbarch, python_language); Phil> + Phil> + thread_obj = create_thread_object (tp); Phil> + if (!thread_obj) Phil> + { Phil> + warning (_("Cannot create Python InferiorThread object.")); Phil> + gdbpy_print_stack (); Phil> + do_cleanups (cleanup); I don't think there is any need for a warning here. The exception should suffice. Phil> +static PyObject * Phil> +infpy_get_was_attached (PyObject *self, void *closure) Phil> +{ Phil> + inferior_object *inf = (inferior_object *) self; Phil> + INFPY_REQUIRE_VALID (inf); You need a blank line between these two lines now. Phil> +static int Phil> +build_inferior_list (struct inferior *inf, void *arg) Phil> +{ Phil> + PyObject *list = arg; Phil> + PyObject *inferior = inferior_to_inferior_object (inf); Phil> + PyList_Append (list, inferior); Likewise. Phil> +/* Implementation of gdb.read_memory (address, length). Phil> + Returns a Python buffer object with LENGTH bytes of the inferior's Phil> + memory at ADDRESS. Both arguments are integers. */ Phil> +static PyObject * Phil> +infpy_read_memory (PyObject *self, PyObject *args) Phil> +{ Phil> + int error = 0; Phil> + CORE_ADDR addr, length; Phil> + void *buffer = NULL; Phil> + membuf_object *membuf_obj; Phil> + PyObject *addr_obj, *length_obj; Phil> + struct cleanup *cleanups; Phil> + volatile struct gdb_exception except; Phil> + Phil> + if (! PyArg_ParseTuple (args, "OO", &addr_obj, &length_obj)) Phil> + return NULL; I hate to add on, but I think all functions taking 2 or more arguments should accept keyword arguments. Phil> + cleanups = make_cleanup (null_cleanup, NULL); Phil> + Phil> + TRY_CATCH (except, RETURN_MASK_ALL) Phil> + { Phil> + if (!get_addr_from_python (addr_obj, &addr) Phil> + || !get_addr_from_python (length_obj, &length)) Phil> + { Phil> + error = 1; Phil> + break; Phil> + } Phil> + Phil> + buffer = xmalloc (length); Phil> + make_cleanup (xfree, buffer); Phil> + Phil> + read_memory (addr, buffer, length); Phil> + } Phil> + GDB_PY_HANDLE_EXCEPTION (except); GDB_PY_HANDLE_EXCEPTION returns from the function; this will leave the cleanups dangling. You have to run the cleanups before the return. Phil> +static PyObject * Phil> +infpy_write_memory (PyObject *self, PyObject *args) Phil> +{ Phil> + int buf_len, error = 0; Phil> + const char *buffer; Phil> + CORE_ADDR addr, length; Phil> + PyObject *addr_obj, *length_obj = NULL; Phil> + volatile struct gdb_exception except; Phil> + Phil> + if (! PyArg_ParseTuple (args, "Os#|O", &addr_obj, &buffer, &buf_len, Phil> + &length_obj)) Phil> + return NULL; Likewise about the keyword arguments. Phil> +/* Implementation of InferiorThread.newest_frame () -> gdb.Frame. Phil> + Returns the newest frame object. */ Phil> +PyObject * Phil> +thpy_newest_frame (PyObject *self, PyObject *args) This function should be static... But, do we want it at all? If the frame comes from some other thread, there is almost no useful operation we can do with it. That said I think the code for the function is fine. If we have some use case then I'm ok with it. Tom