[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.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(+)
  

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
  

* 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