[RFC] gdb/riscv: Add read_description method for riscv_linux_nat_target

[RFC] gdb/riscv: Add read_description method for riscv_linux_nat_target

[Andrew Burgess]

Adds riscv_linux_nat_target::read_description method to find a
suitable target description for the native linux target we are running
on.

Currently this will supply a suitably sized set of x-registers, and
will probe the kernel to see if the f-registers are readable.  If they
are readable then we currently assume that the f-registers are the
same size as the x-registers as I don't know of a good way to probe
the f-register length.  This will obviously need fixing in future.

As of Linux 4.19 there is no ptrace support for reading the
f-registers, this should appear in 4.20, so right now we only return
target descriptions without f-registers.

gdb/ChangeLog:

	* riscv-linux-nat.c: Add 'inferior.h' and 'target-descriptions.h'
	header files.
	(riscv_linux_nat_target::read_description): New method.
---
 gdb/ChangeLog         |  6 ++++++
 gdb/riscv-linux-nat.c | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/gdb/riscv-linux-nat.c b/gdb/riscv-linux-nat.c
index d51f6e30218..f0705bc763f 100644
--- a/gdb/riscv-linux-nat.c
+++ b/gdb/riscv-linux-nat.c
@@ -21,6 +21,8 @@
 #include "gregset.h"
 #include "linux-nat.h"
 #include "riscv-tdep.h"
+#include "inferior.h"
+#include "target-descriptions.h"
 
 #include "elf/common.h"
 
@@ -34,6 +36,9 @@ public:
   /* Add our register access methods.  */
   void fetch_registers (struct regcache *regcache, int regnum) override;
   void store_registers (struct regcache *regcache, int regnum) override;
+
+  /* Read suitable target description.  */
+  const struct target_desc *read_description () override;
 };
 
 static riscv_linux_nat_target the_riscv_linux_nat_target;
@@ -155,6 +160,39 @@ fill_fpregset (const struct regcache *regcache, prfpregset_t *fpregs,
     regcache->raw_collect (RISCV_CSR_FCSR_REGNUM, &fpregs->__d.__fcsr);
 }
 
+/* Return a target description for the current target.  */
+
+const struct target_desc *
+riscv_linux_nat_target::read_description ()
+{
+  struct riscv_gdbarch_features features;
+  struct iovec iov;
+  elf_fpregset_t regs;
+  int tid;
+
+  /* Figuring out xlen is easy.  */
+  features.xlen = sizeof (elf_greg_t);
+
+  tid = inferior_ptid.lwp ();
+
+  iov.iov_base = &regs;
+  iov.iov_len = sizeof (regs);
+
+  /* Can we fetch the f-registers?  */
+  if (ptrace (PTRACE_GETREGSET, tid, NT_FPREGSET,
+	      (PTRACE_TYPE_ARG3) &iov) == -1)
+    features.flen = 0;		/* No f-registers.  */
+  else
+    {
+      /* TODO: We need a way to figure out the actual length of the
+	 f-registers.  We could have 64-bit x-registers, with 32-bit
+	 f-registers.  For now, just assumed xlen and flen match.  */
+      features.flen = features.xlen;
+    }
+
+  return riscv_create_target_description (features);
+}
+
 /* Fetch REGNUM (or all registers if REGNUM == -1) from the target
    into REGCACHE using PTRACE_GETREGSET.  */
 
-- 
2.14.5

Re: [RFC] gdb/riscv: Add read_description method for riscv_linux_nat_target

[Jim Wilson]

On Wed, Nov 28, 2018 at 2:49 PM Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
> Currently this will supply a suitably sized set of x-registers, and
> will probe the kernel to see if the f-registers are readable.  If they
> are readable then we currently assume that the f-registers are the
> same size as the x-registers as I don't know of a good way to probe
> the f-register length.  This will obviously need fixing in future.

The linux kernel currently only supports 64-bit f-registers or none,
so just assuming that they are 64-bits if they exist will work for
now.  The NT_FPREGSET ptrace call is only supported if f-registers
exist, so checking for that is fine.  My linux kernel patch to add
f-register ptrace support adds an elf_fpreg_t type, but it is in
asm/elf.h which is currently not included, and you can't use the type
if you have a 4.19 or earlier kernel.  The elf_gpreg_t definition is
coming from sys/procfs.h which duplicates the definition in asm/elf.h,
but I didn't add an elf_fpreg_t definition to sys/procfs.h, so I'm not
sure what to do about that.

We always have the option of using hwcap info, there are bits set in
there for available architecture extensions.  The hwcap support is
apparently broken in some versions of the linux kernel development
tree, but is hopefully correct in all supported releases.  There are
macros you can find in asm/hwcap.h.  We would have to figure out how
to get these values into gdb, since including asm/hwcap.h directly is
probably not wise.

Jim

Re: [RFC] gdb/riscv: Add read_description method for riscv_linux_nat_target

[Jim Wilson]

On Wed, Nov 28, 2018 at 2:49 PM Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
> Adds riscv_linux_nat_target::read_description method to find a
> suitable target description for the native linux target we are running
> on.

I tested this on my HiFive Unleashed running a 4.15 kernel with
patches for gcc, gdb, and glibc support.  It looks good.  It correctly
detects the FP register support.  I get 3 extra failures
FAIL: gdb.base/solib-display.exp: NO: continue
FAIL: gdb.base/solib-display.exp: IN: continue
FAIL: gdb.base/solib-display.exp: SEP: continue
which looks a little odd since there is no obvious connection to the
target description support, but this is repeatable so something is
going on here.  Anyways, I'm OK with this for now, we can worry about
debugging this problem later.

Jim

[PATCH 0/4] Re: gdb/riscv: Add read_description method for riscv_linux_nat_target

[Andrew Burgess]

* Jim Wilson <jimw@sifive.com> [2018-11-28 18:05:21 -0800]:

> On Wed, Nov 28, 2018 at 2:49 PM Andrew Burgess
> <andrew.burgess@embecosm.com> wrote:
> > Adds riscv_linux_nat_target::read_description method to find a
> > suitable target description for the native linux target we are running
> > on.
> 
> I tested this on my HiFive Unleashed running a 4.15 kernel with
> patches for gcc, gdb, and glibc support.  It looks good.  It correctly
> detects the FP register support.  I get 3 extra failures
> FAIL: gdb.base/solib-display.exp: NO: continue
> FAIL: gdb.base/solib-display.exp: IN: continue
> FAIL: gdb.base/solib-display.exp: SEP: continue
> which looks a little odd since there is no obvious connection to the
> target description support, but this is repeatable so something is
> going on here.  Anyways, I'm OK with this for now, we can worry about
> debugging this problem later.

Thanks for looking at this Jim.

I dug a little deeper, and now understand what is going on here.  We
(or I) currently create a new target description on every call into
`riscv_create_target_description', I (incorrectly) thought that
`riscv_gdbarch_init' would sort it all out.  However, it turns out GDB
relies on identical target descriptions being the exact same object.

This new set of patches does a little prep-work, and then adds a cache
so that calls to `riscv_create_target_description' with the same
feature set will return the exact same target description object.

With this done, the tests listed above now pass.

But why I hear you ask?

Well, in the test we we set up a 'display' of a function local static
variable.  When the 'display' is setup GDB evaluates the expression
and captures the block and symbol.  Then, later, when we stop we can
display the value of the symbol even though (technically) its out of
scope (as we have the block cached).

In the test in question we actually set up the display, and the
restart GDB, however, we still manage to keep displaying the static
variable as we have the block cached, and (I guess) as we are running
the same program everything is still valid (I hope).

Anyway, GDB does have one safety check built in - has the gdbarch
changed.  If it has then GDB ditches the cached block and symbol, and
tries to reevaluate the 'display' string.  However, when this happens
the function local static variable is out of scope, and the display
expression gets deleted.

Conclusion, we need to make sure we don't create new gdbarch objects
when we don't need to (well obviously) and to do this we need to make
sure that we reuse target descriptions.

This is still running through testing at my end, so far its looking
good, but I thought I'd share in case you also wanted to test.

Thanks,
Andrew


---

Andrew Burgess (4):
  gdb/riscv: Make some target description functions constant
  gdb/riscv: Add equality operators to riscv_gdb_features
  gdb/riscv: Create each unique target description only once
  gdb/riscv: Add read_description method for riscv_linux_nat_target

 gdb/ChangeLog         | 27 +++++++++++++++++++++++
 gdb/arch/riscv.c      | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 gdb/arch/riscv.h      | 15 ++++++++++++-
 gdb/riscv-linux-nat.c | 38 +++++++++++++++++++++++++++++++++
 gdb/riscv-tdep.c      |  6 ++----
 5 files changed, 139 insertions(+), 6 deletions(-)

-- 
2.14.5

[PATCH 1/4] gdb/riscv: Make some target description functions constant

[Andrew Burgess]

Makes more of the interface related to fetching target descriptions
constant.

gdb/ChangeLog:

	* arch/riscv.h (riscv_create_target_description): Make return type
	const.
	* arch/riscv.c (riscv_create_target_description): Likewise.
	* riscv-tdep.c (riscv_find_default_target_description): Likewise.
---
 gdb/ChangeLog    | 7 +++++++
 gdb/arch/riscv.c | 2 +-
 gdb/arch/riscv.h | 2 +-
 gdb/riscv-tdep.c | 2 +-
 4 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/gdb/arch/riscv.c b/gdb/arch/riscv.c
index ca2238d5d70..cb715fabb1f 100644
--- a/gdb/arch/riscv.c
+++ b/gdb/arch/riscv.c
@@ -26,7 +26,7 @@
 
 /* See arch/riscv.h.  */
 
-target_desc *
+const target_desc *
 riscv_create_target_description (struct riscv_gdbarch_features features)
 {
   target_desc *tdesc = allocate_target_description ();
diff --git a/gdb/arch/riscv.h b/gdb/arch/riscv.h
index 007944019a9..ec4d5f39525 100644
--- a/gdb/arch/riscv.h
+++ b/gdb/arch/riscv.h
@@ -58,7 +58,7 @@ struct riscv_gdbarch_features
 /* Create and return a target description that is compatible with
    FEATURES.  */
 
-target_desc *riscv_create_target_description
+const target_desc *riscv_create_target_description
 	(struct riscv_gdbarch_features features);
 
 #endif /* ARCH_RISCV_H */
diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 5965a594440..d66fe5c8793 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -2793,7 +2793,7 @@ static const struct frame_unwind riscv_frame_unwind =
    specifically the bfd object being executed, to guide the selection of a
    suitable default target description.  */
 
-static struct target_desc *
+static const struct target_desc *
 riscv_find_default_target_description (const struct gdbarch_info info)
 {
   struct riscv_gdbarch_features features;
-- 
2.14.5

[PATCH 2/4] gdb/riscv: Add equality operators to riscv_gdb_features

[Andrew Burgess]

Add '==' and '!=' operators for the struct riscv_gdb_features,
allowing a small simplification.

gdb/ChangeLog:

	* arch/riscv.h (riscv_gdb_features::operator==): New.
	(riscv_gdb_features::operator!=): New.
	* riscv-tdep.c (riscv_gdbarch_init): Make use of the inequality
	operator.
---
 gdb/ChangeLog    |  7 +++++++
 gdb/arch/riscv.h | 13 +++++++++++++
 gdb/riscv-tdep.c |  4 +---
 3 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/gdb/arch/riscv.h b/gdb/arch/riscv.h
index ec4d5f39525..be41828eff6 100644
--- a/gdb/arch/riscv.h
+++ b/gdb/arch/riscv.h
@@ -53,6 +53,19 @@ struct riscv_gdbarch_features
      this field is true then the hardware floating point abi is in use, and
      values are passed in f-registers matching the size of FLEN.  */
   bool hw_float_abi = false;
+
+  /* Equality operator.  */
+  bool operator== (const struct riscv_gdbarch_features &rhs) const
+  {
+    return (xlen == rhs.xlen && flen == rhs.flen
+	    && hw_float_abi == rhs.hw_float_abi);
+  }
+
+  /* Inequality operator.  */
+  bool operator!= (const struct riscv_gdbarch_features &rhs) const
+  {
+    return !((*this) == rhs);
+  }
 };
 
 /* Create and return a target description that is compatible with
diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index d66fe5c8793..30c777db6f2 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -3025,9 +3025,7 @@ riscv_gdbarch_init (struct gdbarch_info info,
          gdbarch.  */
       struct gdbarch_tdep *other_tdep = gdbarch_tdep (arches->gdbarch);
 
-      if (other_tdep->features.hw_float_abi != features.hw_float_abi
-          || other_tdep->features.xlen != features.xlen
-          || other_tdep->features.flen != features.flen)
+      if (other_tdep->features != features)
         continue;
 
       break;
-- 
2.14.5

[PATCH 3/4] gdb/riscv: Create each unique target description only once

[Andrew Burgess]

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.c (class riscv_target_desc_cache): New.
	(riscv_tdesc_cache): New global.
	(riscv_create_target_description): Look in the cache before
	creating a new target description.
---
 gdb/ChangeLog    |  7 +++++++
 gdb/arch/riscv.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

diff --git a/gdb/arch/riscv.c b/gdb/arch/riscv.c
index cb715fabb1f..fef0c403e7b 100644
--- a/gdb/arch/riscv.c
+++ b/gdb/arch/riscv.c
@@ -24,11 +24,65 @@
 #include "../features/riscv/32bit-fpu.c"
 #include "../features/riscv/64bit-fpu.c"
 
+/* Class used to cache previously created RISC-V target descriptions.  */
+
+class riscv_target_desc_cache
+{
+public:
+
+  /* Lookup a previously created target description for FEATURES, or
+     return nullptr if no matching target description has ever been
+     created.  */
+  const target_desc *lookup (struct riscv_gdbarch_features features)
+  {
+    for (auto p : m_tdesc_list)
+      {
+        if (p.first == features)
+          return p.second;
+      }
+
+    return nullptr;
+  }
+
+  /* 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 struct riscv_gdbarch_features,
+                    const target_desc *> feature_tdesc_pair;
+
+  /* List of all target descriptions we've previously seen.  */
+  std::vector<feature_tdesc_pair> m_tdesc_list;
+};
+
+/* Cache of previously seen target descriptions, indexed by the feature set
+   that created them.  */
+static riscv_target_desc_cache 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 target_desc *prev_tdesc = riscv_tdesc_cache.lookup (features);
+  if (prev_tdesc != nullptr)
+    return prev_tdesc;
+
+  /* Now we should create a new target description.  */
   target_desc *tdesc = allocate_target_description ();
 
 #ifndef IN_PROCESS_AGENT
@@ -65,5 +119,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.add (features, tdesc);
+
   return tdesc;
 }
-- 
2.14.5

[PATCH 4/4] gdb/riscv: Add read_description method for riscv_linux_nat_target

[Andrew Burgess]

Adds riscv_linux_nat_target::read_description method to find a
suitable target description for the native linux target we are running
on.

Currently this will supply a suitably sized set of x-registers, and
will probe the kernel to see if the f-registers are readable.  If they
are readable then we currently assume that the f-registers are the
same size as the x-registers as I don't know of a good way to probe
the f-register length.  This will obviously need fixing in future.

As of Linux 4.19 there is no ptrace support for reading the
f-registers, this should appear in 4.20, so right now we only return
target descriptions without f-registers.

gdb/ChangeLog:

	* riscv-linux-nat.c: Add 'inferior.h' and 'target-descriptions.h'
	header files.
	(riscv_linux_nat_target::read_description): New method.
---
 gdb/ChangeLog         |  6 ++++++
 gdb/riscv-linux-nat.c | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/gdb/riscv-linux-nat.c b/gdb/riscv-linux-nat.c
index d51f6e30218..f0705bc763f 100644
--- a/gdb/riscv-linux-nat.c
+++ b/gdb/riscv-linux-nat.c
@@ -21,6 +21,8 @@
 #include "gregset.h"
 #include "linux-nat.h"
 #include "riscv-tdep.h"
+#include "inferior.h"
+#include "target-descriptions.h"
 
 #include "elf/common.h"
 
@@ -34,6 +36,9 @@ public:
   /* Add our register access methods.  */
   void fetch_registers (struct regcache *regcache, int regnum) override;
   void store_registers (struct regcache *regcache, int regnum) override;
+
+  /* Read suitable target description.  */
+  const struct target_desc *read_description () override;
 };
 
 static riscv_linux_nat_target the_riscv_linux_nat_target;
@@ -155,6 +160,39 @@ fill_fpregset (const struct regcache *regcache, prfpregset_t *fpregs,
     regcache->raw_collect (RISCV_CSR_FCSR_REGNUM, &fpregs->__d.__fcsr);
 }
 
+/* Return a target description for the current target.  */
+
+const struct target_desc *
+riscv_linux_nat_target::read_description ()
+{
+  struct riscv_gdbarch_features features;
+  struct iovec iov;
+  elf_fpregset_t regs;
+  int tid;
+
+  /* Figuring out xlen is easy.  */
+  features.xlen = sizeof (elf_greg_t);
+
+  tid = inferior_ptid.lwp ();
+
+  iov.iov_base = &regs;
+  iov.iov_len = sizeof (regs);
+
+  /* Can we fetch the f-registers?  */
+  if (ptrace (PTRACE_GETREGSET, tid, NT_FPREGSET,
+	      (PTRACE_TYPE_ARG3) &iov) == -1)
+    features.flen = 0;		/* No f-registers.  */
+  else
+    {
+      /* TODO: We need a way to figure out the actual length of the
+	 f-registers.  We could have 64-bit x-registers, with 32-bit
+	 f-registers.  For now, just assumed xlen and flen match.  */
+      features.flen = features.xlen;
+    }
+
+  return riscv_create_target_description (features);
+}
+
 /* Fetch REGNUM (or all registers if REGNUM == -1) from the target
    into REGCACHE using PTRACE_GETREGSET.  */
 
-- 
2.14.5

Re: [PATCH 3/4] gdb/riscv: Create each unique target description only once

[Pedro Alves]

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 struct riscv_gdbarch_features,
> +                    const target_desc *> feature_tdesc_pair;
> +
> +  /* List of all target descriptions we've previously seen.  */
> +  std::vector<feature_tdesc_pair> 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,
Pedro Alves

Re: [PATCH 3/4] gdb/riscv: Create each unique target description only once

[Andrew Burgess]

* Pedro Alves <palves@redhat.com> [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 struct riscv_gdbarch_features,
> > +                    const target_desc *> feature_tdesc_pair;
> > +
> > +  /* List of all target descriptions we've previously seen.  */
> > +  std::vector<feature_tdesc_pair> 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.

No, the class only exists to group the lookup/update methods.  As I
only expect the list to contain 1 or 2 items in a "normal" session I
didn't invest much thought into it really.  I'll take a look at the
example you suggest and update the patch accordingly.

Thanks for the feedback,

Andrew

Re: [PATCH 4/4] gdb/riscv: Add read_description method for riscv_linux_nat_target

[Jim Wilson]

I tried the new patch set on my HiFive Unleashed and it does solve the
problem.  The gdb testsuite results on my board look good now.

Jim

Re: [PATCH 3/4] gdb/riscv: Create each unique target description only once

[Andrew Burgess]

* Pedro Alves <palves@redhat.com> [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 struct riscv_gdbarch_features,
> > +                    const target_desc *> feature_tdesc_pair;
> > +
> > +  /* List of all target descriptions we've previously seen.  */
> > +  std::vector<feature_tdesc_pair> 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 <stdlib.h>
+#include <unordered_map>
 
 #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<struct riscv_gdbarch_features,
+                          target_desc *,
+                          riscv_gdbarch_features_hasher> 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
-- 
2.14.5

Re: [PATCH 3/4] gdb/riscv: Create each unique target description only once

[Pedro Alves]

On 11/29/2018 10:32 PM, Andrew Burgess wrote:

> +/* 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

I don't think we do "east const" in gdb.  

Also, '&' is formatted like '*'.

Might as well drop the "struct" while at it (I'd do that in a number
of places).

That leaves:

   operator() (const riscv_gdbarch_features &features) const noexcept

Otherwise looks fine.  Thanks!

Pedro Alves