Re: [PATCH v3 1/3] arc: Add ARCv2 XML target along with refactoring

[Shahab Vahedi <shahab.vahedi@gmail.com>]

Hi Simon,

Thank you a lot for going through all the changes. I've taken most
of your remarks in. There are a few of them that need answering.
Below, you will find them.

If you think the current state of code is satisfactory, I will go
ahead and merge them. With one note: there might come another
patch on top of this if the output of "AddressSanitizer" shows some
leak. Please see my question for the "AddressSanitizer" below.

On Tue, Jul 14, 2020 at 10:52:12PM -0400, Simon Marchi wrote:
> Hi Shahab,
> 
> > +/* Common construction code for ARC_GDBARCH_FEATURES struct.  If there
> > +   is no ABFD, then a FEATURE with default values is returned.  */
> > +void arc_gdbarch_features_init (arc_gdbarch_features &features,
> > +				const bfd *abfd, const unsigned long mach)
> 
> Since this function is only used in this file, can it be static?

The other patch [1] in this series makes use of this function.

[1] [PATCH v3 3/3] arc: Add GNU/Linux support for ARC
https://sourceware.org/pipermail/gdb-patches/2020-July/170429.html
> Could this function return a arc_gdbarch_features by value instead?  Then,
> you could add a constructor to arc_gdbarch_features.  You wouldn't need
> the "A 0 indicates an uninitialised value" comment on reg_size, as the
> object could never be in an uninitialised state.

Those were my thoughts as well. I tried something like that, but I failed
in the end:

Since the "arc_gdbarch_features" is introduced in "gdb/arch/arc.h", I
tried implementing the constructor in "gdb/arch/arc.c". Because the
constructor had to make use of "struct bfd", "bfd_get_flavour ()",
"elf_header ()", "EFCLASS*", etc. the following headers had to be used in
"gdb/arch/arc.c":

  #include "defs.h"
  #include "arch-utils.h"
  #include "elf-bfd.h"

This would build OK for gdb targets, but when compiling "arc.o" for
gdbserver target, I ended up with something like:

  (paraphrasing)
  Error: defs.h should not be included while building gdbserver.
  (I am not sure if the error was indeed about "defs.h")

And removing/if-def-guarding that header file was opening other cans of
worms. Eventually, I gave up and went for the current approach.

> > +/* Wrapper used by std::unordered_map to generate hash for features set.  */
> > +struct arc_gdbarch_features_hasher
> > +{
> > +  std::size_t
> > +  operator() (const arc_gdbarch_features &features) const noexcept
> > +  {
> > +    return features.hash ();
> > +  }
> > +};
> > +
> > +/* Cache of previously created target descriptions, indexed by the hash
> > +   of the features set used to create them.  */
> > +static std::unordered_map<arc_gdbarch_features,
> > +			  const target_desc *,
> > +			  arc_gdbarch_features_hasher> arc_tdesc_cache;
> 
> Should this map hold unique_ptr of target_desc?  I understand that they
> are never really deallocated anyway, but just to do it right and avoid
> adding more leaks to the final AddressSanitizer report shown when you exit
> GDB (and you have it build with AddressSanitizer)?

No, I haven't built it with AddressSanitizer. Could you point me in the right
direction how I can use it?
> > +  /* Used by std::unordered_map to hash the feature sets.  The hash is
> > +     calculated in the manner below:
> > +     REG_SIZE |  ISA
> > +      5-bits  | 4-bits  */
> > +
> > +  std::size_t hash () const noexcept
> > +  {
> > +    std::size_t val = ((reg_size & 0x1f) << 8 | (isa & 0xf) << 0);
> > +    return val;
> 
> I don't know much about hashing, but I don't think makes for a very good hash
> function.  It outputs similar hashes for similar inputs, and does not use the
> range of possible hash values uniformly.

Indeed it does not output hashes uniformly but I believe it is unique. The
meaningful (and future proof) values for "reg_size" are {4, 8, 16} _bytes_.
Hence, the "0x1f" mask should suffice. Having 15 versions for ISA should be
enough as well. Having these conditions in mind, I don't think there is any
collusion while hashing such inputs.
> It would be more appropriate and typical to use std::hash on each member and
> then combine the hashes.  Doing a quick search I found that Boost offers
> something exactly for that called hash_combine:
> 
> https://www.boost.org/doc/libs/1_55_0/doc/html/hash/combine.html

Thanks for this. I didn't know about such functionality. As you said
though, we can't use it. Personally, I think it would be an overkill
here. We can get away with a simple "shift" and "or". Please read my
response above for my reasoning.
> 
> Unfortunately, there is nothing similar in the standard library.  But
> 
> A bit off-topic: intuitively, I would have thought that it would have been
> sufficient to just add the hash values together.  Anyone knows why this
> wouldn't be a good idea?

So far, possible values are:

  reg_size : {4, 8, 16}
  ISA: {1, 2, ... 15}

A simple addition would produce the same result for these legally valid
inputs:

reg_size=8, ISA=2   --->  hash = 10
reg_size=4, ISA=6   --->  hash = 10


Shahab