From patchwork Thu Nov 29 22:32:27 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 30406 Received: (qmail 116753 invoked by alias); 29 Nov 2018 22:32:40 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 116707 invoked by uid 89); 29 Nov 2018 22:32:34 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-wr1-f65.google.com Received: from mail-wr1-f65.google.com (HELO mail-wr1-f65.google.com) (209.85.221.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 29 Nov 2018 22:32:31 +0000 Received: by mail-wr1-f65.google.com with SMTP id t27so3474373wra.6 for ; Thu, 29 Nov 2018 14:32:31 -0800 (PST) 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:user-agent; bh=2hFQ9pWbIYMG1wsA1bUlsRWq3LBnA8FUkLhfQO7H1VE=; b=S2DxlVsURB/bIWpVA1PqQgHp6Z2P7Mp03lWb/bXNzuaG8CPIR3NjWfDn5E9e65j0Xt t+zrc5LR0zyFRHKKrSb+wavqVtyrCRoog6Z9JADJ3d1D9EaMFAgLi6n4hyvDtjKVUyyf rVsUvM5IJsQEnVoJQiUR47SXapH4H0Baa8pxEKY6JInzWIUXGekiaKPtNlfkqR1yYnP9 6qhk22jWetLQeBOvxIB/S/Y4fuGsfOtqBnw1wwfFLHkH33D/qbREqWHO00PQ7gx4Duem KiNzWlatCzYDr33dzluVZ03vvI3Y80gUknahtOA7H6JEraicOkUFKSxe/BEg5MQLSJ3k +4dA== Return-Path: Received: from localhost (host86-156-236-171.range86-156.btcentralplus.com. [86.156.236.171]) by smtp.gmail.com with ESMTPSA id v133sm4558319wmd.4.2018.11.29.14.32.28 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 29 Nov 2018 14:32:28 -0800 (PST) Date: Thu, 29 Nov 2018 22:32:27 +0000 From: Andrew Burgess To: Pedro Alves Cc: gdb-patches@sourceware.org, jimw@sifive.com, palmer@sifive.com, jhb@FreeBSD.org Subject: Re: [PATCH 3/4] gdb/riscv: Create each unique target description only once Message-ID: <20181129223227.GI18841@embecosm.com> References: <84ce42bd83dc3d67bd312c4c8772171dd5bf1182.1543509416.git.andrew.burgess@embecosm.com> <6277aa2c-c64e-e7d2-07d9-444ce80ef170@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <6277aa2c-c64e-e7d2-07d9-444ce80ef170@redhat.com> X-Fortune: "Our vision is to speed up time, eventually eliminating it." -- Alex Schure X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes * Pedro Alves [2018-11-29 18:12:29 +0000]: > On 11/29/2018 04:48 PM, Andrew Burgess wrote: > > > + > > + /* Add TDESC into the cache, a target description created to match > > + FEATURES. */ > > + void add (const struct riscv_gdbarch_features features, > > + const target_desc *tdesc) > > + { > > + feature_tdesc_pair p (features, tdesc); > > + m_tdesc_list.push_back (p); > > + } > > + > > +private: > > + > > + /* Map from a feature set to the corresponding target description. */ > > + typedef std::pair > + const target_desc *> feature_tdesc_pair; > > + > > + /* List of all target descriptions we've previously seen. */ > > + std::vector m_tdesc_list; > > +}; > > Did you consider an unordered_map instead of this whole class here? > See xml-tdesc.c's xml_cache. If there's a reason the custom data > structure is preferred, I think that warrants a comment. Thanks for the feedback. Below is a revised version using std::unordered_map. Thanks, Andrew --- [PATCH 3/4] gdb/riscv: Create each unique target description only once GDB relies on the fact that if two target descriptions have the same contents, then they will be the same object instance (having the same address). One place where this is a requirement is in GDBARCH_LIST_LOOKUP_BY_INFO which is used to find previously created gdbarch objects. In GDBARCH_LIST_LOOKUP_BY_INFO a pointer comparison is made on the gdbarch's target description, if the pointers are different then it is assumed the gdbarches have different, non-compatible target descriptions. Previously we would create duplicate target descriptions in the belief that RISCV_GDBARCH_INIT would spot this duplication and discard the second instance. However, this was incorrect, and instead we ended up creating duplicate gdbarch objects. With this commit every unique feature set will create one and only one target description, the feature set and resulting target description is then cached so that the same target description object can be returned later. Many other target avoid this problem by creating a small number of named target descriptions, and returning one of these. However, we currently have 8 possible target descriptions (32 vs 64 bit for x-reg and f-reg, and h/w or s/w float abi) and creating each of these just to avoid a dynamic cache seems pointless. gdb/ChangeLog: * arch/riscv.h (riscv_gdbarch_features::hash): New method. * arch/riscv.c (struct riscv_gdbarch_features_hasher): New. (riscv_tdesc_cache): New global. (riscv_create_target_description): Look in the cache before creating a new target description. --- gdb/ChangeLog | 8 ++++++++ gdb/arch/riscv.c | 31 +++++++++++++++++++++++++++++++ gdb/arch/riscv.h | 9 +++++++++ 3 files changed, 48 insertions(+) diff --git a/gdb/arch/riscv.c b/gdb/arch/riscv.c index cb715fabb1f..94f07646607 100644 --- a/gdb/arch/riscv.c +++ b/gdb/arch/riscv.c @@ -18,17 +18,45 @@ #include "common-defs.h" #include "riscv.h" #include +#include #include "../features/riscv/32bit-cpu.c" #include "../features/riscv/64bit-cpu.c" #include "../features/riscv/32bit-fpu.c" #include "../features/riscv/64bit-fpu.c" +/* Wrapper used by std::unordered_map to generate hash for feature set. */ +struct riscv_gdbarch_features_hasher +{ + std::size_t + operator() (struct riscv_gdbarch_features const& features) const noexcept + { + return features.hash (); + } +}; + +/* Cache of previously seen target descriptions, indexed by the feature set + that created them. */ +static std::unordered_map riscv_tdesc_cache; + /* See arch/riscv.h. */ const target_desc * riscv_create_target_description (struct riscv_gdbarch_features features) { + /* Have we seen this feature set before? If we have return the same + target description. GDB expects that if two target descriptions are + the same (in content terms) then they will actually be the same + instance. This is important when trying to lookup gdbarch objects as + GDBARCH_LIST_LOOKUP_BY_INFO performs a pointer comparison on target + descriptions to find candidate gdbarch objects. */ + const auto it = riscv_tdesc_cache.find (features); + if (it != riscv_tdesc_cache.end ()) + return it->second; + + /* Now we should create a new target description. */ target_desc *tdesc = allocate_target_description (); #ifndef IN_PROCESS_AGENT @@ -65,5 +93,8 @@ riscv_create_target_description (struct riscv_gdbarch_features features) else if (features.flen == 8) regnum = create_feature_riscv_64bit_fpu (tdesc, regnum); + /* Add to the cache. */ + riscv_tdesc_cache.emplace (features, tdesc); + return tdesc; } diff --git a/gdb/arch/riscv.h b/gdb/arch/riscv.h index be41828eff6..79f5ec622d4 100644 --- a/gdb/arch/riscv.h +++ b/gdb/arch/riscv.h @@ -66,6 +66,15 @@ struct riscv_gdbarch_features { return !((*this) == rhs); } + + /* Used by std::unordered_map to hash feature sets. */ + std::size_t hash () const noexcept + { + std::size_t val = ((xlen & 0x1f) << 6 + | (flen & 0x1f) << 1 + | (hw_float_abi ? 1 : 0)); + return val; + } }; /* Create and return a target description that is compatible with