From patchwork Fri Feb 14 19:32:35 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Maciej W. Rozycki" X-Patchwork-Id: 38083 Received: (qmail 93877 invoked by alias); 14 Feb 2020 19:32:44 -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 93868 invoked by uid 89); 14 Feb 2020 19:32:44 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-8.4 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_2, GIT_PATCH_3, KAM_ASCII_DIVIDERS autolearn=ham version=3.3.1 spammy=aims, non-const, UD:binutils-gdb.orig, nonconst X-HELO: esa6.hgst.iphmx.com Received: from esa6.hgst.iphmx.com (HELO esa6.hgst.iphmx.com) (216.71.154.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 14 Feb 2020 19:32:41 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=wdc.com; i=@wdc.com; q=dns/txt; s=dkim.wdc.com; t=1581708762; x=1613244762; h=date:from:to:cc:subject:in-reply-to:message-id: references:mime-version; bh=oXWlPCL8o93ZOcNpBar/k1/Ar8R+zAqyT5RGpXxzhiY=; b=YigDIlcvz24By+6OaUIWKcOQm7jNzv99JbYQzxtCBm/3846saH3z4sWj E12Lks/hWAZgScxDkHVi5n/ruIroKsWI5GKz7igeuFpuql3v7bAx5ToEP PtFwkLqRVXafUKw9mvIwDz5srh7ZjQ/Q06CErWOjYgjryp2cQQB69hMfm xvpY2HxPNFOCK11pw0gUgPpC5Dcl4WR+4XnxZqZOAEX8qR4x0JXpHMXKU ItujxKebVbKU30FURitMOJwHj9yZVq7uLtvf2OYKUF6788PCQFagOTl2+ 3hDNdRdefd6gR63q7s0/XzVWCG/n6pcxXN06t5XAaF07arG6zvAn6TOyO g==; IronPort-SDR: c637nG9J8mQWsaOQxP1O6RG6cvEm2EB/AnA2YbNvNlNbyeWu6YkjIVsSbpTDqT10BqZr9JWYxe aXH/+hAvbsU5cOnFT1RQAWoX3ScYvoLOF/g3ciEiPD2t4KhAuKvVma/7VE8UACP+Sv3UMRYfj9 1NFDLIMtRvtMVvAMGf0yzy7oEgPA4IfcXFbuf1BH57rm/8SfzPe00HDfF9SlwFPEeRzPZ0Lsj4 a0vJ0JFs3yeU+xcjwS3V8V1OG0IiPQ10lPb9wgP12votMlIy9vjSWeYfs49P8vF3bGUJfDrBi9 9gk= Received: from h199-255-45-14.hgst.com (HELO uls-op-cesaep01.wdc.com) ([199.255.45.14]) by ob1.hgst.iphmx.com with ESMTP; 15 Feb 2020 03:32:40 +0800 IronPort-SDR: zeQ2lHOsvCe3UwdiyrxoY3DsJ9D2VyfTnP/JEmTzBbzLcMqWWLv5OfW40x0bJwSWDl4y4tLP61 jfQ4qlxF8rWddDx4jJsjYKx0k7fiNxS0AYmjzfx0gcb/5uvpwDUrO7S4nHQBUZi/JgYjPM1IY1 QScKvN8TFb738bZIpFKipA9EECI7Vu8fQpwQOlHeTeYJCFKhtjg8Ix8tV/RcboaYsn/zLJ78Si nuhcC/Ftl60WHVRPDApaxxLbVh3K+3w9cakiUEn27OXJRWdZB+htQMAQyPiXCCOdISufnQEFVH VgdqT7lRLTvVaKni8jv1xEPU Received: from uls-op-cesaip02.wdc.com ([10.248.3.37]) by uls-op-cesaep01.wdc.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Feb 2020 11:25:24 -0800 IronPort-SDR: +f6cVCuxW2HOIuKjf09cIPYQWDKN2FTyrUOMbWLqdMrz9i2nW9gKOHVm+ryBqbsK/wbsN0naOW /HRdkT18QEovW28AGrYnSYBJrS8oiBnZFR0ZMnMyMkT/oVAT3mfQw4N2FzNriU7G+jkdvPGmuF MxD2zWNiw1DyxhpUtfMzoDWoC4Kf83m7PXSwENPQgdc3lMMjCD3YozXlxl0Sy6eAe74mzO00Wj 2KuKi8yI2E2SxKcByLd+R2R6w9cIm0hrd+6lHu+gqA9xKPEPpGJCC28ja4ZRtjXR+6ZiOQgwgJ N3g= WDCIronportException: Internal Received: from unknown (HELO redsun52) ([10.149.66.28]) by uls-op-cesaip02.wdc.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Feb 2020 11:32:38 -0800 Date: Fri, 14 Feb 2020 19:32:35 +0000 (GMT) From: "Maciej W. Rozycki" To: gdb-patches@sourceware.org cc: Jim Wilson , Andrew Burgess , Palmer Dabbelt , Tom Tromey , guoren@kernel.org, lifang_xia@c-sky.com, yunhai_shang@c-sky.com, jiangshuai_li@c-sky.com Subject: [PATCH v7 1/2] gdb/riscv: Update API for looking up target descriptions In-Reply-To: Message-ID: References: User-Agent: Alpine 2.21 (LFD 202 2017-01-01) MIME-Version: 1.0 From: Andrew Burgess In preparation for adding the RISC-V gdbserver, this commit restructures the API for looking up target descriptions. The current API is riscv_create_target_description, which creates a target description from a riscv_gdbarch_features, but also caches the created target descriptions so that for a given features object we always get back the same target description object. This is important for GDB due to the way gdbarch objects are reused. As the same target description is always returned to GDB, and can be returned multiple times, it is returned as a const, however, the current cache actually stores a non-const target description. This is improved in this patch so that the cache holds a const target description. For gdbsever, this caching of the target descriptions is not needed, the gdbserver looks up one target description to describe the target it is actually running on and that is it. Further the gdbserver actually needs to modify the target description that is looked up, so for the gdbsever, returning a const target description is not acceptable. This commit aims to address this by creating two parallel target description APIs, on is the old riscv_create_target_description, however, this no longer performs any caching, and just creates a new target description, and returns it as non-const. The second API is riscv_lookup_target_description, this one performs the caching, and calls riscv_create_target_description to create a target description when needed. In order to make sure the correct API is used in the correct place I have guarded the code using the GDBSERVER define. For GDB the riscv_create_target_description is static, and not generally usable throughout GDB, only the lookup API is global. In gdbserver, the lookup functions, and the cache are not defined or created at all, only the riscv_create_target_description API is available. There should be no user visible changes after this commit. gdb/ChangeLog: * arch/riscv.c (struct riscv_gdbarch_features_hasher): Only define if GDBSERVER is not defined. (riscv_tdesc_cache): Likewise, also store const target_desc. (STATIC_IN_GDB): Define. (riscv_create_target_description): Update declaration with STATIC_IN_GDB. (riscv_lookup_target_description): New function, only define if GDBSERVER is not defined. * arch/riscv.h (riscv_create_target_description): Declare only when GDBSERVER is defined. (riscv_lookup_target_description): New declaration when GDBSERVER is not defined. * nat/riscv-linux-tdesc.c (riscv_linux_read_description): Rename to... (riscv_linux_read_features): ...this, and return riscv_gdbarch_features instead of target_desc. * nat/riscv-linux-tdesc.h: Include 'arch/riscv.h'. (riscv_linux_read_description): Rename to... (riscv_linux_read_features): ...this. * riscv-linux-nat.c (riscv_linux_nat_target::read_description): Update to use riscv_gdbarch_features and riscv_lookup_target_description. * riscv-tdep.c (riscv_find_default_target_description): Use riscv_lookup_target_description instead of riscv_create_target_description. --- gdb/arch/riscv.c | 69 ++++++++++++++++++++++++++------------------ gdb/arch/riscv.h | 26 ++++++++++++++-- gdb/nat/riscv-linux-tdesc.c | 8 ++--- gdb/nat/riscv-linux-tdesc.h | 7 ++-- gdb/riscv-linux-nat.c | 4 +- gdb/riscv-tdep.c | 2 - 6 files changed, 76 insertions(+), 40 deletions(-) gdb-aburgess-riscv-tdesc-api-gdbserver.diff Index: binutils-gdb/gdb/arch/riscv.c =================================================================== --- binutils-gdb.orig/gdb/arch/riscv.c +++ binutils-gdb/gdb/arch/riscv.c @@ -25,37 +25,17 @@ #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() (const riscv_gdbarch_features &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; +#ifndef GDBSERVER +#define STATIC_IN_GDB static +#else +#define STATIC_IN_GDB +#endif /* See arch/riscv.h. */ -const target_desc * -riscv_create_target_description (struct riscv_gdbarch_features features) +STATIC_IN_GDB target_desc * +riscv_create_target_description (const 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 (); @@ -93,8 +73,43 @@ riscv_create_target_description (struct else if (features.flen == 8) regnum = create_feature_riscv_64bit_fpu (tdesc, regnum); + return tdesc; +} + +#ifndef GDBSERVER + +/* Wrapper used by std::unordered_map to generate hash for feature set. */ +struct riscv_gdbarch_features_hasher +{ + std::size_t + operator() (const riscv_gdbarch_features &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_lookup_target_description (const struct riscv_gdbarch_features features) +{ + /* Lookup in the cache. */ + const auto it = riscv_tdesc_cache.find (features); + if (it != riscv_tdesc_cache.end ()) + return it->second; + + target_desc *tdesc = riscv_create_target_description (features); + /* Add to the cache. */ riscv_tdesc_cache.emplace (features, tdesc); return tdesc; } + +#endif /* !GDBSERVER */ Index: binutils-gdb/gdb/arch/riscv.h =================================================================== --- binutils-gdb.orig/gdb/arch/riscv.h +++ binutils-gdb/gdb/arch/riscv.h @@ -66,10 +66,28 @@ struct riscv_gdbarch_features } }; -/* Create and return a target description that is compatible with - FEATURES. */ +#ifdef GDBSERVER + +/* Create and return a target description that is compatible with FEATURES. + This is only used directly from the gdbserver where the created target + description is modified after it is return. */ + +target_desc *riscv_create_target_description + (const struct riscv_gdbarch_features features); + +#else + +/* Lookup an already existing target description matching FEATURES, or + create a new target description if this is the first time we have seen + FEATURES. For the same FEATURES the same target_desc is always + returned. 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 target_desc *riscv_lookup_target_description + (const struct riscv_gdbarch_features features); + +#endif /* GDBSERVER */ -const target_desc *riscv_create_target_description - (struct riscv_gdbarch_features features); #endif /* ARCH_RISCV_H */ Index: binutils-gdb/gdb/nat/riscv-linux-tdesc.c =================================================================== --- binutils-gdb.orig/gdb/nat/riscv-linux-tdesc.c +++ binutils-gdb/gdb/nat/riscv-linux-tdesc.c @@ -31,10 +31,10 @@ # define NFPREG 33 #endif -/* Determine XLEN and FLEN and return a corresponding target description. */ +/* See nat/riscv-linux-tdesc.h. */ -const struct target_desc * -riscv_linux_read_description (int tid) +struct riscv_gdbarch_features +riscv_linux_read_features (int tid) { struct riscv_gdbarch_features features; elf_fpregset_t regs; @@ -79,5 +79,5 @@ riscv_linux_read_description (int tid) break; } - return riscv_create_target_description (features); + return features; } Index: binutils-gdb/gdb/nat/riscv-linux-tdesc.h =================================================================== --- binutils-gdb.orig/gdb/nat/riscv-linux-tdesc.h +++ binutils-gdb/gdb/nat/riscv-linux-tdesc.h @@ -19,9 +19,10 @@ #ifndef NAT_RISCV_LINUX_TDESC_H #define NAT_RISCV_LINUX_TDESC_H -struct target_desc; +#include "arch/riscv.h" -/* Return a target description for the LWP identified by TID. */ -const struct target_desc *riscv_linux_read_description (int tid); +/* Determine XLEN and FLEN for the LWP identified by TID, and return a + corresponding features object. */ +struct riscv_gdbarch_features riscv_linux_read_features (int tid); #endif /* NAT_RISCV_LINUX_TDESC_H */ Index: binutils-gdb/gdb/riscv-linux-nat.c =================================================================== --- binutils-gdb.orig/gdb/riscv-linux-nat.c +++ binutils-gdb/gdb/riscv-linux-nat.c @@ -201,7 +201,9 @@ fill_fpregset (const struct regcache *re const struct target_desc * riscv_linux_nat_target::read_description () { - return riscv_linux_read_description (inferior_ptid.lwp ()); + const struct riscv_gdbarch_features features + = riscv_linux_read_features (inferior_ptid.lwp ()); + return riscv_lookup_target_description (features); } /* Fetch REGNUM (or all registers if REGNUM == -1) from the target Index: binutils-gdb/gdb/riscv-tdep.c =================================================================== --- binutils-gdb.orig/gdb/riscv-tdep.c +++ binutils-gdb/gdb/riscv-tdep.c @@ -2973,7 +2973,7 @@ riscv_find_default_target_description (c features.xlen = 8; /* Now build a target description based on the feature set. */ - return riscv_create_target_description (features); + return riscv_lookup_target_description (features); } /* All of the registers in REG_SET are checked for in FEATURE, TDESC_DATA