Re: [PATCH 15/25] [RFC] GDBserver unit test to i386_tdesc

[Pedro Alves <palves@redhat.com>]

On 06/12/2017 09:41 AM, Yao Qi wrote:
> This patch adds a unit test in GDBserver to test dynamically created
> target descriptions equal to these pre-generated ones.
> 
> gdb/gdbserver:

Are these temporary tests?  What's the plan for the pre-generated ones?

> +#if defined GDB_SELF_TEST && !defined IN_PROCESS_AGENT
> +#include "selftest.h"
> +
> +namespace selftests {
> +namespace gdbserver {

Considering that at some point we may want to wrap
all of gdbserver under "namespace gdbserver", this
namespace choice here seems like a poor choice.
s/gdbserver/tdesc_tests/ or some such?


> +static void
> +i386_tdesc_test ()
> +{
> +  const struct target_desc *tdesc = i386_get_ipa_tdesc (X86_TDESC_MMX);
> +
> +  SELF_CHECK (*tdesc == *tdesc_i386_mmx_linux);
> +  delete tdesc;
> +
> +  tdesc = i386_get_ipa_tdesc (X86_TDESC_SSE);
> +  SELF_CHECK (*tdesc == *tdesc_i386_linux);
> +  delete tdesc;
> +
> +  tdesc = i386_get_ipa_tdesc (X86_TDESC_AVX);
> +  SELF_CHECK (*tdesc == *tdesc_i386_avx_linux);
> +  delete tdesc;
> +
> +  tdesc = i386_get_ipa_tdesc (X86_TDESC_MPX);
> +  SELF_CHECK (*tdesc == *tdesc_i386_mpx_linux);
> +  delete tdesc;
> +
> +  tdesc = i386_get_ipa_tdesc (X86_TDESC_AVX_MPX);
> +  SELF_CHECK (*tdesc == *tdesc_i386_avx_mpx_linux);
> +  delete tdesc;
> +
> +  tdesc = i386_get_ipa_tdesc (X86_TDESC_AVX_AVX512);
> +  SELF_CHECK (*tdesc == *tdesc_i386_avx_avx512_linux);
> +  delete tdesc;
> +
> +  tdesc = i386_get_ipa_tdesc (X86_TDESC_AVX_MPX_AVX512_PKU);
> +  SELF_CHECK (*tdesc == *tdesc_i386_avx_mpx_avx512_pku_linux);
> +  delete tdesc;

Would something like this, based on an array, work? :

  struct 
    {
      int idx;
      const target_desc *tdesc;
    }
  tdesc_tests[] = {
      { X86_TDESC_MMX, tdesc_i386_mmx_linux },
      { X86_TDESC_SSE, tdesc_i386_linux },
      ...
  };

  for (auto &elem : tdesc_tests)
    {
      std::unique_ptr<const target_desc> tdesc (i386_get_ipa_tdesc (elem.idx));
      SELF_CHECK (*elem.tdesc == *tdesc);
    }

> diff --git a/gdb/gdbserver/tdesc.h b/gdb/gdbserver/tdesc.h
> index 2d4cbfa..e64f0b3 100644
> --- a/gdb/gdbserver/tdesc.h
> +++ b/gdb/gdbserver/tdesc.h
> @@ -19,7 +19,8 @@
>  #ifndef TDESC_H
>  #define TDESC_H
>  
> -struct reg;
> +#include "regdef.h"
> +#include <cstring>

Odd that you needed cstring?  string.h is one of the
system headers always pulled in by common/common-defs.h.

>  
>  typedef struct reg *tdesc_reg_p;
>  DEF_VEC_P(tdesc_reg_p);
> @@ -60,6 +61,45 @@ public:
>        xfree (reg);
>      VEC_free (tdesc_reg_p, reg_defs);
>    }
> +  bool operator!= (const target_desc &other) const

Same comment about "operator!=" vs "operator==" as in
a previous patch.

Thanks,
Pedro Alves