From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x441.google.com (mail-wr1-x441.google.com [IPv6:2a00:1450:4864:20::441]) by sourceware.org (Postfix) with ESMTPS id B81D93893654 for ; Wed, 22 Jul 2020 14:05:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org B81D93893654 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=andrew.burgess@embecosm.com Received: by mail-wr1-x441.google.com with SMTP id a15so2031213wrh.10 for ; Wed, 22 Jul 2020 07:05:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=IWtTCcoUjpoWaACCY8sqPwtr2+GnxKKlRjNAmM3rvd0=; b=ISauDb03o0UFFn8rwykwcsnneOEuhcF3qLo/5Soi7jXD9Hfw6rxSNI08EHvKaisSVU Wd7ysZ/4OJsrXT0B4+Bv79UvaX3iUPKnr0orwbo3jUGnDJ38nnmAmalx3D7kug5YJVW2 KUu+iFEL+LZVPSNBjjj0cHNsLgfjCwKkiAtihqmLLQJwJfzrQlCRDYTS98WFi//5NXEY cGfT3w8/D9BR6g+WNyjHJ08KcFdbAzlaP9rv+444FmgXJX0SG4U2gF/tfrLf8lZp/9e5 5nfigf7VIDaTOS7cqCgaIRt6zV5EDH6qFOwpXUfotucjm28/Ns/WlguiA3n1S6I+tCby 3gsw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=IWtTCcoUjpoWaACCY8sqPwtr2+GnxKKlRjNAmM3rvd0=; b=hyuufvHsFVluPyHz9NU6PdI3U52nRDuZtTZFsUCCu8LHI8Ef5FcNXYFLFM9oQwHZ30 FPFxgluHSEn3ztnPJOonrs6jVWU1t3WT2ABpoOlJgQAj0Rv/hOtj2b6XTh8LWog/v22u Y+w/lZPmn1zbD+pyRI5J+O/icegS7Y/RmAoChu2P9OBj00j6Z57u+iCXyICgWaW21hxx kfTpz9lItq3wrtbCQsrxWsUJuSfuEkGDZ7XTu5jAuHO6omUDCNx32UMEuQvE+l1TDfu1 04lj8e80wH6eyyhEB99V7a2D9zWXQ7p0sGF0LZY+628UmQLCJeo6khUq0pRO8JgpQFlO kxPg== X-Gm-Message-State: AOAM531Q5ACtg2hz12ZvJMtOncMgdmgPo91/yP/gUKMots5zm7KVOCP8 rpR2k4J76nwV4k+2UwNsU/otS1mUmKw= X-Google-Smtp-Source: ABdhPJzuubyDyODqRlsH1j3mlKEGaploXzWrOLLJtRq8eTB/OYC4JZHqtJYfFovTBgt6koq3kGT0Bw== X-Received: by 2002:adf:cc85:: with SMTP id p5mr30837957wrj.273.1595426748720; Wed, 22 Jul 2020 07:05:48 -0700 (PDT) Received: from localhost (host86-134-151-238.range86-134.btcentralplus.com. [86.134.151.238]) by smtp.gmail.com with ESMTPSA id u17sm21619wrp.70.2020.07.22.07.05.47 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 22 Jul 2020 07:05:48 -0700 (PDT) Date: Wed, 22 Jul 2020 15:05:46 +0100 From: Andrew Burgess To: Pedro Alves Cc: gdb-patches@sourceware.org, Tom Tromey Subject: Re: [PATCHv2 1/2] gdb/python: Reuse gdb.RegisterDescriptor objects where possible Message-ID: <20200722140546.GF853475@embecosm.com> References: <1eaf1e55-4e95-4fe6-7c23-400bf2168e70@palves.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1eaf1e55-4e95-4fe6-7c23-400bf2168e70@palves.net> X-Operating-System: Linux/5.6.15-200.fc31.x86_64 (x86_64) X-Uptime: 15:04:15 up 3 days, 23:18, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-10.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Wed, 22 Jul 2020 14:05:51 -0000 * Pedro Alves [2020-07-22 14:10:17 +0100]: > Hi Andrew, > > Some nits below. > > On 7/14/20 6:14 PM, Andrew Burgess wrote: > > > > > +/* Token to access per-gdbarch data related to register descriptors. */ > > +static struct gdbarch_data *gdbpy_register_object_data = NULL; > > + > > /* Structure for iterator over register descriptors. */ > > typedef struct { > > PyObject_HEAD > > @@ -81,6 +84,17 @@ typedef struct { > > extern PyTypeObject reggroup_object_type > > CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("reggroup_object"); > > > > +/* Associates a vector of gdb.RegisterDescriptor objects with GDBARCH as > > + gdbarch_data via the gdbarch post init registration mechanism > > + (gdbarch_data_register_post_init). */ > > + > > +static void * > > +gdbpy_register_object_data_init (struct gdbarch *gdbarch) > > +{ > > + std::vector> *vec = new (std::vector>); > > + return (void *) vec; > > This could just be: > > return new std::vector>; > > > > +} > > + > > /* Create a new gdb.RegisterGroup object wrapping REGGROUP. */ > > > > static PyObject * > > @@ -117,20 +131,38 @@ gdbpy_reggroup_name (PyObject *self, void *closure) > > return gdbpy_reggroup_to_string (self); > > } > > > > -/* Create an return a new gdb.RegisterDescriptor object. */ > > -static PyObject * > > -gdbpy_new_register_descriptor (struct gdbarch *gdbarch, > > +/* Return a gdb.RegisterDescriptor object for REGNUM from GDBARCH. For > > + each REGNUM (in GDBARCH) only one descriptor is ever created, which is > > + then cached on the GDBARCH. */ > > + > > +static gdbpy_ref<> > > +gdbpy_get_register_descriptor (struct gdbarch *gdbarch, > > int regnum) > > { > > - /* Create a new object and fill in its details. */ > > - register_descriptor_object *reg > > - = PyObject_New (register_descriptor_object, > > - ®ister_descriptor_object_type); > > - if (reg == NULL) > > - return NULL; > > - reg->regnum = regnum; > > - reg->gdbarch = gdbarch; > > - return (PyObject *) reg; > > + auto vec = (std::vector> *) gdbarch_data > > + (gdbarch, gdbpy_register_object_data); > > Wrap the rhs in parens so the second line is properly aligned, > per the GNU standard's "so that emacs aligns it automatically" > rule: > > auto vec = ((std::vector> *) gdbarch_data > (gdbarch, gdbpy_register_object_data)); > > Or you could put the = in the next line, so that it aligns > without the parens: > > auto vec > = (std::vector> *) gdbarch_data (gdbarch, > gdbpy_register_object_data); > > Note, I think it's more idiomatic to write the * for pointers: > > auto *vec = .... > > But since vec can't be NULL, I'd write a reference instead: > > auto &vec > = *(std::vector> *) gdbarch_data (gdbarch, > gdbpy_register_object_data); > > > + > > + /* Ensure that we have enough entries in the vector. */ > > + if (vec->size () <= regnum) > > + vec->resize ((regnum + 1), nullptr); > > + > > + /* If we don't already have a descriptor for REGNUM in GDBARCH then > > + create one now. */ > > + if (vec->at (regnum) == nullptr) > > Is there a reason you're using at? Was it to avoid > > (*vec)[regnum] > > or was it really about out-of-range errors? > > If such as exception were thrown, it would be a logic > bug in GDB. And note that we don't catch it anywhere, so > it would bring down GDB. > > There's a school of thought that claims that at should not > really exist, and I agree with it. :-) > > If you go the reference route, then you can write the natural: > > if (vec[regnum] = nullptr) > > > + { > > + gdbpy_ref reg > > + (PyObject_New (register_descriptor_object, > > + ®ister_descriptor_object_type)); > > + if (reg == NULL) > > + return NULL; > > + reg->regnum = regnum; > > + reg->gdbarch = gdbarch; > > + vec->at (regnum) = gdbpy_ref<> ((PyObject *) reg.release ()); > > + } > > + > > + /* Grab the register descriptor from the vector, the reference count is > > + automatically incremented thanks to gdbpy_ref. */ > > + return vec->at (regnum); > > Ditto in these two other spots. Pedro, Thanks for your feedback. As this patch was already merged, the patch below applies on top of current master to address these issues. If I don't hear anything I'll apply this in a couple of days. Thanks, Andrew --- commit ed3d57aca93bfe3e0eaa5888486b9fc84d06a0c6 Author: Andrew Burgess Date: Wed Jul 22 14:57:55 2020 +0100 gdb/python: Use reference not pointer in py-registers.c Pedro's review comments arrived after I'd already committed this change: commit f7306dac19c502232f766c3881313857915f330d Date: Tue Jul 7 15:00:30 2020 +0100 gdb/python: Reuse gdb.RegisterDescriptor objects where possible See: https://sourceware.org/pipermail/gdb-patches/2020-July/170726.html There should be no user visible changes after this commit. gdb/ChangeLog: * python/py-registers.c (gdbpy_register_object_data_init): Remove redundant local variable. (gdbpy_get_register_descriptor): Extract descriptor vector as a reference, not pointer, update code accordingly. diff --git a/gdb/python/py-registers.c b/gdb/python/py-registers.c index 9396498cc34..f64ca3c401b 100644 --- a/gdb/python/py-registers.c +++ b/gdb/python/py-registers.c @@ -92,8 +92,7 @@ extern PyTypeObject reggroup_object_type static void * gdbpy_register_object_data_init (struct gdbarch *gdbarch) { - std::vector> *vec = new (std::vector>); - return (void *) vec; + return new std::vector>; } /* Return a gdb.RegisterGroup object wrapping REGGROUP. The register @@ -158,16 +157,17 @@ static gdbpy_ref<> gdbpy_get_register_descriptor (struct gdbarch *gdbarch, int regnum) { - auto vec = (std::vector> *) gdbarch_data - (gdbarch, gdbpy_register_object_data); + auto &vec + = *(std::vector> *) gdbarch_data (gdbarch, + gdbpy_register_object_data); /* Ensure that we have enough entries in the vector. */ - if (vec->size () <= regnum) - vec->resize ((regnum + 1), nullptr); + if (vec.size () <= regnum) + vec.resize ((regnum + 1), nullptr); /* If we don't already have a descriptor for REGNUM in GDBARCH then create one now. */ - if (vec->at (regnum) == nullptr) + if (vec[regnum] == nullptr) { gdbpy_ref reg (PyObject_New (register_descriptor_object, @@ -176,12 +176,12 @@ gdbpy_get_register_descriptor (struct gdbarch *gdbarch, return NULL; reg->regnum = regnum; reg->gdbarch = gdbarch; - vec->at (regnum) = gdbpy_ref<> ((PyObject *) reg.release ()); + vec[regnum] = gdbpy_ref<> ((PyObject *) reg.release ()); } /* Grab the register descriptor from the vector, the reference count is automatically incremented thanks to gdbpy_ref. */ - return vec->at (regnum); + return vec[regnum]; } /* Convert the register descriptor to a string. */