[PATCHv6 1/2] gdb/riscv: Update API for looking up target descriptions
Andrew Burgess
andrew.burgess@embecosm.com
Sat Feb 8 00:41:00 GMT 2020
* Andrew Burgess <andrew.burgess@embecosm.com> [2020-02-08 00:18:42 +0000]:
> 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.
>
> Change-Id: Ia732b3bf03fedea0f94b2f73f0581ad90eb1355f
> ---
> gdb/ChangeLog | 27 ++++++++++++++++++
> 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 +-
> 7 files changed, 103 insertions(+), 40 deletions(-)
>
> diff --git a/gdb/arch/riscv.c b/gdb/arch/riscv.c
> index a3ab8a92909..a02c18b4c91 100644
> --- a/gdb/arch/riscv.c
> +++ b/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_gdbarch_features,
> - target_desc *,
> - riscv_gdbarch_features_hasher> 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 riscv_gdbarch_features features)
> 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_gdbarch_features,
> + const target_desc *,
> + riscv_gdbarch_features_hasher> 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 */
> diff --git a/gdb/arch/riscv.h b/gdb/arch/riscv.h
> index 016a5fcd2f0..d40862cbd8e 100644
> --- a/gdb/arch/riscv.h
> +++ b/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 */
> diff --git a/gdb/nat/riscv-linux-tdesc.c b/gdb/nat/riscv-linux-tdesc.c
> index 1b625cf38fc..3220725b875 100644
> --- a/gdb/nat/riscv-linux-tdesc.c
> +++ b/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;
> }
> diff --git a/gdb/nat/riscv-linux-tdesc.h b/gdb/nat/riscv-linux-tdesc.h
> index 9b57a9e99d5..e9ee64aa5d2 100644
> --- a/gdb/nat/riscv-linux-tdesc.h
> +++ b/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 */
> diff --git a/gdb/riscv-linux-nat.c b/gdb/riscv-linux-nat.c
> index 2622f1b4399..c34ecd7cd5d 100644
> --- a/gdb/riscv-linux-nat.c
> +++ b/gdb/riscv-linux-nat.c
> @@ -201,7 +201,9 @@ fill_fpregset (const struct regcache *regcache, prfpregset_t *fpregs,
> 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_feature (inferior_ptid.lwp ());
/\
Gah! There's an 's' missing ----||
Sorry!
Andrew
> + return riscv_lookup_target_description (features);
> }
>
> /* Fetch REGNUM (or all registers if REGNUM == -1) from the target
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index d585b0be5ab..bc816831941 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -2973,7 +2973,7 @@ riscv_find_default_target_description (const struct gdbarch_info info)
> 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
> --
> 2.14.5
>
More information about the Gdb-patches
mailing list