From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f45.google.com (mail-wr1-f45.google.com [209.85.221.45]) by sourceware.org (Postfix) with ESMTPS id 23B8A385700B for ; Wed, 22 Jul 2020 13:10:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 23B8A385700B Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=alves.ped@gmail.com Received: by mail-wr1-f45.google.com with SMTP id q5so1855553wru.6 for ; Wed, 22 Jul 2020 06:10:22 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=E/1RXBGFNZMTKFYBioEhdbx/DG5BuywDWon1CIGtbsg=; b=L8E0Rj4jMHC7UVDeHy05GGuxE9jpypvpOa1AQpRsprjCzoy+LwqZVz3hNk/PuUIp4L 3qIcRZBI/7xkjHO9zLcA6lLXd0WFMuJLGLybXbu8DoNoAsXCVdklTzjVkRVmlixa1ud0 BenXP8kqMrvVBBWg5+vp7pvAs5qHg+5+//OKw9DGbEy0VfwH1+4Llb+0amB0UmPQDBEc kWggozHHfFQNxLBk35CVhB45eMJ3RHp+JW9V8XD4Xq6X0Ek36HTfLxVfVD2pwpFJIiA7 c1EtKe2QvNS8YptLPRZ+tntb/4pa3+KuQ1mt4Iq5JBCLCaNg8Q9e41pJM4T6h6TtOYiG C4ag== X-Gm-Message-State: AOAM531mBSU6DkOc2E1U880snew1FUD2AgJj8GAF+D4gmC409Nky6J0O IOhf+yg7R8IKG5DkzkWAcLg= X-Google-Smtp-Source: ABdhPJyhWQpV7ctvbZVYqTGE4Kzz0O3KMDVC3j6OcPxyvklWTKj+BH6O1+NIegW+cKTCV/edFBRl9A== X-Received: by 2002:adf:c986:: with SMTP id f6mr31218676wrh.168.1595423421196; Wed, 22 Jul 2020 06:10:21 -0700 (PDT) Received: from ?IPv6:2001:8a0:f91a:c400:56ee:75ff:fe8d:232b? ([2001:8a0:f91a:c400:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id c10sm15098939wro.84.2020.07.22.06.10.20 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 22 Jul 2020 06:10:20 -0700 (PDT) Subject: Re: [PATCHv2 1/2] gdb/python: Reuse gdb.RegisterDescriptor objects where possible To: Andrew Burgess , gdb-patches@sourceware.org References: Cc: Tom Tromey From: Pedro Alves Message-ID: <1eaf1e55-4e95-4fe6-7c23-400bf2168e70@palves.net> Date: Wed, 22 Jul 2020 14:10:17 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3.8 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, 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 13:10:23 -0000 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.