From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gproxy4-pub.mail.unifiedlayer.com (gproxy4-pub.mail.unifiedlayer.com [69.89.23.142]) by sourceware.org (Postfix) with ESMTPS id 5ADC63857B84 for ; Thu, 2 Jun 2022 16:08:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5ADC63857B84 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=tromey.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=tromey.com Received: from cmgw14.mail.unifiedlayer.com (unknown [10.0.90.129]) by progateway6.mail.pro1.eigbox.com (Postfix) with ESMTP id 6C90F100415E5 for ; Thu, 2 Jun 2022 16:08:57 +0000 (UTC) Received: from box5379.bluehost.com ([162.241.216.53]) by cmsmtp with ESMTP id wnNYnNoWAhWk0wnNZn1y68; Thu, 02 Jun 2022 16:08:57 +0000 X-Authority-Reason: nr=8 X-Authority-Analysis: v=2.4 cv=IrfbzJzg c=1 sm=1 tr=0 ts=6298e099 a=ApxJNpeYhEAb1aAlGBBbmA==:117 a=ApxJNpeYhEAb1aAlGBBbmA==:17 a=dLZJa+xiwSxG16/P+YVxDGlgEgI=:19 a=JPEYwPQDsx4A:10:nop_rcvd_month_year a=Qbun_eYptAEA:10:endurance_base64_authed_username_1 a=CCpqsmhAAAAA:8 a=ySvUFj2anlcB6IWix88A:9 a=ul9cdbp4aOFLsgKbc677:22 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tromey.com; s=default; h=Content-Type:MIME-Version:Message-ID:In-Reply-To:Date:References :Subject:Cc:To:From:Sender:Reply-To:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=LiYXWVBwli6AFm1/JnyV6KA10sTSHicmaD+FugHX0G4=; b=Ti+K81zww5k/jd+P3Dubafa8o5 RphQoaJDW7w6W9lyOh+fuqgTQUP96bWv9pABOGEsFP2NaOgx9V+0tNu/7yGfQb2/WR9Y/7oS6F5uA k3XQglUBS7DjNl3UQF8Fbkb5G; Received: from 71-211-167-178.hlrn.qwest.net ([71.211.167.178]:55870 helo=murgatroyd) by box5379.bluehost.com with esmtpsa (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1nwnNX-0040x6-NS; Thu, 02 Jun 2022 10:08:55 -0600 From: Tom Tromey To: Simon Farre via Gdb-patches Subject: Re: [PATCH v6] gdb/python: Add BreakpointLocation type References: <20220531173806.1213510-1-simon.farre.cx@gmail.com> X-Attribution: Tom Date: Thu, 02 Jun 2022 10:08:54 -0600 In-Reply-To: <20220531173806.1213510-1-simon.farre.cx@gmail.com> (Simon Farre via Gdb-patches's message of "Tue, 31 May 2022 19:38:06 +0200") Message-ID: <87k09z2gkp.fsf@tromey.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - box5379.bluehost.com X-AntiAbuse: Original Domain - sourceware.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - tromey.com X-BWhitelist: no X-Source-IP: 71.211.167.178 X-Source-L: No X-Exim-ID: 1nwnNX-0040x6-NS X-Source: X-Source-Args: X-Source-Dir: X-Source-Sender: 71-211-167-178.hlrn.qwest.net (murgatroyd) [71.211.167.178]:55870 X-Source-Auth: tom+tromey.com X-Email-Count: 1 X-Source-Cap: ZWx5bnJvYmk7ZWx5bnJvYmk7Ym94NTM3OS5ibHVlaG9zdC5jb20= X-Local-Domain: yes X-Spam-Status: No, score=-3023.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, JMQ_SPF_NEUTRAL, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 02 Jun 2022 16:09:00 -0000 >>>>> Simon Farre via Gdb-patches writes: > PR python/18385 > v6: > This version addresses the points Pedro gave in review to this patch. Thanks for the patch. > +extern PyTypeObject breakpoint_location_object_type > + CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("breakpoint_location_object"); > + > +struct gdbpy_breakpoint_location_object > +{ > + PyObject_HEAD > + > + /* The gdb breakpoint location object. */ > + bp_location *bp_loc; > + > + /* Location's breakpoint owner. */ > + gdbpy_breakpoint_object *owner; It would probably be good to mention in these comments that these are owning references. > +/* Python function to get the breakpoint locations of an owner breakpoint. */ > + > +static PyObject* > +bppy_get_locations (PyObject *self, void *closure) > +{ > + using py_bploc_t = gdbpy_breakpoint_location_object; > + auto *self_bp = (gdbpy_breakpoint_object *) self; > + BPPY_REQUIRE_VALID (self_bp); > + > + gdbpy_ref<> list (PyList_New (0)); This is missing a null check. The function can just return null if this fails. > + for (bp_location *loc : self_bp->bp->locations ()) > + { > + gdbpy_ref py_bploc (PyObject_New (py_bploc_t, > + &breakpoint_location_object_type)); I think this needs a null check. > + bp_location_ref_ptr ref = bp_location_ref_ptr::new_reference (loc); > + py_bploc->owner = self_bp; > + > + /* The location takes a reference to the owner breakpoint. Decrements > + when they are de-allocated in bplocpy_dealloc */ > + Py_INCREF (self); I think it would be clearer to move the incref above the assignment to 'owner' and group these together. That way it's more obvious (IMO) that the ownership is being transferred to py_bloc. > + if (PyList_Append (list.get (), (PyObject *) py_bploc.get ()) != 0) > + { > + return nullptr; > + } You can remove the braces here. > + py_bploc->bp_loc = ref.release (); It would be fine to hoist this up before the PyList_Append as well. I suppose it doesn't really matter but I was wondering why one assignment is done before and one after. > + return PyLong_FromLong (self->bp_loc->address); This should use gdb_py_object_from_ulongest. > +static PyObject * > +bplocpy_get_source_location (PyObject *py_self, void *closure) > +{ > + auto *self = (gdbpy_breakpoint_location_object *) py_self; > + BPPY_REQUIRE_VALID (self->owner); > + BPLOCPY_REQUIRE_VALID (self->owner, self); > + if (self->bp_loc->symtab) > + { > + gdbpy_ref<> tup (PyTuple_New (2)); I think this needs a null check. > + /* symtab->filename is never NULL. */ > + gdbpy_ref<> filename > + = host_string_to_python_string (self->bp_loc->symtab->filename); Same here. > + > + if (PyTuple_SetItem (tup.get (), 0, filename.release ()) == -1 > + || PyTuple_SetItem (tup.get (), 1, > + PyLong_FromLong (self->bp_loc->line_number)) == -1) Normally gdb uses the gdb_py wrappers instead of PyLong_FromLong, to avoid problems with size and/or signed-ness. > +static PyObject * > +bplocpy_get_thread_groups (PyObject *py_self, void *closure) > +{ > + auto *self = (gdbpy_breakpoint_location_object *) py_self; > + BPPY_REQUIRE_VALID (self->owner); > + BPLOCPY_REQUIRE_VALID (self->owner, self); > + gdbpy_ref<> list (PyList_New (0)); Null check. > + > + for (inferior *inf : all_inferiors ()) > + { > + if (inf->pspace == self->bp_loc->pspace) > + if (PyList_Append (list.get (), PyLong_FromLong (inf->num)) != 0) gdb_py_object_from_longest > + return nullptr; > + } > + return PyList_AsTuple (list.get ()); This seems fine but it seems like it would also be fine to just return the list here. > +/* De-allocation function to be called for the Python object. */ > + > +static void > +bplocpy_dealloc (PyObject *py_self) > +{ > + auto *self = (gdbpy_breakpoint_location_object *) py_self; > + bp_location_ref_policy::decref (self->bp_loc); > + Py_DECREF (self->owner); I wonder if Py_XDECREF is needed here, like if there can ever be a case where 'owner' isn't set. What happens if you try to create one of these objects manually? Also it's not too normal, I think, to directly refer to the refcount policy classes. Anyway the issue here is if bp_loc can be null, the policy class will cause a crash (whereas ref_ptr ensures this can't happen). > +/* Require that BREAKPOINT and LOCATION->OWNER are the same; throw a Python > + exception if they are not. */ > +#define BPLOCPY_REQUIRE_VALID(Breakpoint, Location) \ > + do { \ > + if ((Breakpoint)->bp != (Location)->bp_loc->owner) \ > + return PyErr_Format (PyExc_RuntimeError, \ > + _("Breakpoint location is invalid.")); \ > + } while (0) > + > +/* Require that BREAKPOINT and LOCATION->OWNER are the same; throw a Python > + exception if they are not. This macro is for use in setter functions. */ > +#define BPLOCPY_SET_REQUIRE_VALID(Breakpoint, Location) \ Do these macros need to be in python-internal.h? Seems like they'd be fine in the .c file. > +# Set breakpoint at end and make sure we run past the still enabled locations, > +gdb_breakpoint 32 > +gdb_continue_to_breakpoint "end of main" ".*32.*" > \ No newline at end of file Should be a newline. Tom