Re: [PATCH V2 2/2] Prologue: Add selftests to x64/x32 architecture.

[Yao Qi <qiyaoltc at gmail dot com>]

On 16-12-16 14:58:36, Bernhard Heckel wrote:
> +/* Abstract code reader.  */
> +
> +class abstract_code_reader
> +{
> +protected:
> +  bool target_memory = 0;

We don't need this field, see comments below,

> +public:
> +  /* Read in one byte.  */

/* Read LEN bytes from MEMADDR to BUFFER.  */

> +  virtual void read (CORE_ADDR memaddr, gdb_byte *buffer, ssize_t len) = 0;
> +
> +  bool is_target_memory (void) { return target_memory;};
> +};
> +
> +/* Code reader from real target.  */
> +
> +class code_reader : public abstract_code_reader
> +{
> +public:
> +  code_reader (void)
> +  {
> +    target_memory = 1;
> +  }
> +
> +  void read (CORE_ADDR memaddr, gdb_byte *buffer, ssize_t len)
> +  {
> +    read_code (memaddr, buffer, len);
> +  }
> +};
> +
>  /* Do a limited analysis of the prologue at PC and update CACHE
>     accordingly.  Bail out early if CURRENT_PC is reached.  Return the
>     address where the analysis stopped.
> @@ -2282,7 +2312,8 @@ amd64_x32_analyze_stack_align (CORE_ADDR pc, CORE_ADDR current_pc,
>  static CORE_ADDR
>  amd64_analyze_prologue (struct gdbarch *gdbarch,
>  			CORE_ADDR pc, CORE_ADDR current_pc,
> -			struct amd64_frame_cache *cache)
> +			struct amd64_frame_cache *cache,
> +			abstract_code_reader& reader)
>  {
>    enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>    /* There are two variations of movq %rsp, %rbp.  */
> @@ -2304,12 +2335,15 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
>    if (current_pc <= pc)
>      return current_pc;
>  
> -  if (gdbarch_ptr_bit (gdbarch) == 32)
> -    pc = amd64_x32_analyze_stack_align (pc, current_pc, cache);
> -  else
> -    pc = amd64_analyze_stack_align (pc, current_pc, cache);
> -
> -  op = read_code_unsigned_integer (pc, 1, byte_order);
> +  /* For selftests, we don't analyze stack alignment.  */

Why don't you pass reader to amd64{x32,}_x32_analyze_stack_align and
analyze the stack alignment as well?...

> +  if (reader.is_target_memory ())
> +    {
> +      if (gdbarch_ptr_bit (gdbarch) == 32)
> +	pc = amd64_x32_analyze_stack_align (pc, current_pc, cache);
> +      else
> +	pc = amd64_analyze_stack_align (pc, current_pc, cache);
> +    }

In this way, we don't need such change.

> +#if GDB_SELF_TEST
> +
> +namespace selftests {
> +
> +/* Code reader from manually created instruction sequences.  */
> +
> +class code_reader_test : public abstract_code_reader
> +{
> +private:
> +  const std::vector<gdb_byte> memory;

std::vector<gdb_byte> is a little heavy, and there are a lot of copies
in the tests below.  Why don't you follow class instruction_reader_test
in aarch64-tdep.c?  You can have a field gdb_byte *m_data in class.

> +
> +public:
> +  explicit code_reader_test (const std::vector<gdb_byte> &memory)
> +  : memory (memory)
> +  {
> +    /* Nothing to do.  */
> +  }
> +
> +  void read (CORE_ADDR memaddr, gdb_byte *buffer, ssize_t len)
> +  {
> +    SELF_CHECK ((memaddr + len) <= memory.size ());
> +    memcpy (buffer, memory.data () + memaddr, len);
> +  }
> +};
> +
> +struct prologue_test_t {
> +  amd64_frame_cache exp_cache;

Why don't you test other fields in amd64_frame_cache?

> +  CORE_ADDR exp_pc;
> +  std::vector<gdb_byte> memory;
> +};
> +
> +/* Returns x64 test pattern and expected results.  */
> +
> +static std::vector<struct prologue_test_t> get_prologue_tests_x64 (void)
> +{
> +  std::vector<struct prologue_test_t> prologue_tests;
> +  struct prologue_test_t prologue_test;
> +
> +  /* Negative test.  */
> +  prologue_test.exp_pc = 0;
> +  prologue_test.memory = {0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90,
> +      0x90, 0x90, 0x90, 0x90, 0x90}; /* nop */
> +  amd64_init_frame_cache (&prologue_test.exp_cache);
> +  prologue_test.exp_cache.frameless_p = 1;
> +  prologue_tests.push_back (prologue_test);
> +
> +  prologue_test.exp_pc = 4;
> +  prologue_test.memory = {0x55, /* push %rbp */
> +      0x48, 0x89, 0xe5,         /* mov  %rsp, %rbp */
> +      0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90}; /* nop */

I don't think compiler can generate prologue with some many 'nop'.
I hope the test case is the real code sequences generated by compiler.
prologue_test.memory should have all prologue instructions plus one
non-prologue instruction.  Test can check the analysis stops at the last
instruction (non-prologue one).

> +  amd64_init_frame_cache (&prologue_test.exp_cache);
> +  prologue_test.exp_cache.frameless_p = 0;
> +  prologue_tests.push_back (prologue_test);
> +

You can use .emplace_back to avoid copying.

> +
> +/* Returns test pattern and expected results.  */
> +
> +static std::vector<struct prologue_test_t> get_prologue_tests (const std::string arch)
> +{
> +  SELF_CHECK (arch == "i386:x86-64"
> +	      || arch == "i386:x64-32");
> +
> +  if (arch == "i386:x86-64")
> +    return get_prologue_tests_x64 ();
> +  else
> +    return get_prologue_tests_x32 ();
> +}
> +
> +static void
> +amd64_analyze_prologue_test (void)
> +{
> +  const std::string arch_string[] = {"i386:x86-64", "i386:x64-32"};
> +

Alternatively, you can have two test functions,
amd64_analyze_prologue_test, and amd64_x32_analyze_prologue_test.

-- 
Yao