Re: [PATCH 1/2] Add unit test to aarch64 prologue analyzer

[Pedro Alves <palves at redhat dot com>]

On 11/29/2016 02:12 PM, Yao Qi wrote:
> We don't have an effective way to test prologue analyzer which is
> highly dependent on instruction patterns in prologue generated by
> compiler.  GDB prologue analyzer may not handle the new sequences
> generated by new compiler, or may still handle some sequences that
> generated by very old compilers which are no longer used.  The
> former is a functionality issue, while the latter is a maintenance
> issue.
> 
> The input and output of prologue analyzer is quite clear, so it
> fits for unit test.  The input is series of instructions, and the
> output are 1) where prologue end, 2) where registers are saved.
> In aarch64, they are represented in 'struct aarch64_prologue_cache'.
> 
> This patch refactors aarch64_analyze_prologue so it can read
> instructions from either real target or test harness.  In unit
> test aarch64_analyze_prologue_test, aarch64_analyze_prologue gets
> instructions we prepared in the test, as the input of prologue
> analyzer.  Then, we checked various fields in
> 'struct aarch64_prologue_cache'.
> 
> gdb:
> 
> 2016-11-28  Yao Qi  <yao.qi@linaro.org>
> 
> 	* aarch64-tdep.c: Include "selftest.h".
> 	(abstract_instruction_reader): New class.
> 	(instruction_reader): New class.
> 	(aarch64_analyze_prologue): Add new parameter reader.  Call
> 	reader.read instead of read_memory_unsigned_integer.
> 	[GDB_SELF_TEST] (instruction_reader_test): New class.
> 	(aarch64_analyze_prologue_test): New function.
> 	(_initialize_aarch64_tdep) [GDB_SELF_TEST]: Register
> 	selftests::aarch64_analyze_prologue_test.
> 	* trad-frame.c (trad_frame_cache_zalloc):
> 	(trad_frame_alloc_saved_regs): Add a new function.
> 	* trad-frame.h (trad_frame_alloc_saved_regs): Declare.
> ---
>  gdb/aarch64-tdep.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  gdb/trad-frame.c   |  21 ++++++----
>  gdb/trad-frame.h   |   1 +
>  3 files changed, 129 insertions(+), 9 deletions(-)
> 
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 6b95d7c..b10002a 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -44,6 +44,7 @@
>  #include "infcall.h"
>  #include "ax.h"
>  #include "ax-gdb.h"
> +#include "selftest.h"
>  
>  #include "aarch64-tdep.h"
>  
> @@ -195,6 +196,29 @@ show_aarch64_debug (struct ui_file *file, int from_tty,
>    fprintf_filtered (file, _("AArch64 debugging is %s.\n"), value);
>  }
>  
> +/* Abstract instruction reader.  */
> +
> +class abstract_instruction_reader
> +{
> +public:
> +  /* Read in one instruction.  */
> +  virtual ULONGEST read (CORE_ADDR memaddr, int len,
> +			 enum bfd_endian byte_order) = 0;
> +};
> +
> +/* Instruction reader from real target.  */
> +
> +class instruction_reader : public abstract_instruction_reader
> +{
> + public:
> +  instruction_reader () = default;

(As mentioned in the previous email, this just looks like
unnecessary redundancy to me; suggest just removing it.  The compiler
generates it for you.)

> +
> +  ULONGEST read (CORE_ADDR memaddr, int len, enum bfd_endian byte_order)
> +  {
> +    return read_memory_unsigned_integer (memaddr, len, byte_order);
> +  }
> +};
> +
>  /* Analyze a prologue, looking for a recognizable stack frame
>     and frame pointer.  Scan until we encounter a store that could
>     clobber the stack frame unexpectedly, or an unknown instruction.  */
> @@ -202,7 +226,8 @@ show_aarch64_debug (struct ui_file *file, int from_tty,
>  static CORE_ADDR
>  aarch64_analyze_prologue (struct gdbarch *gdbarch,
>  			  CORE_ADDR start, CORE_ADDR limit,
> -			  struct aarch64_prologue_cache *cache)
> +			  struct aarch64_prologue_cache *cache,
> +			  abstract_instruction_reader& reader)
>  {
>    enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
>    int i;
> @@ -221,7 +246,7 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,
>        uint32_t insn;
>        aarch64_inst inst;
>  
> -      insn = read_memory_unsigned_integer (start, 4, byte_order_for_code);
> +      insn = reader.read (start, 4, byte_order_for_code);
>  
>        if (aarch64_decode_insn (insn, &inst, 1) != 0)
>  	break;
> @@ -436,6 +461,89 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,
>    return start;
>  }
>  
> +static CORE_ADDR
> +aarch64_analyze_prologue (struct gdbarch *gdbarch,
> +			  CORE_ADDR start, CORE_ADDR limit,
> +			  struct aarch64_prologue_cache *cache)
> +{
> +  instruction_reader reader { };

It's more idiomatic to just do:

   instruction_reader reader;

when you want default construction.

> +
> +  return aarch64_analyze_prologue (gdbarch, start, limit, cache,
> +				   reader);
> +}
> +
> +#if GDB_SELF_TEST
> +
> +namespace selftests {
> +
> +  /* Instruction reader from manually cooked instruction sequences.  */
> +  class instruction_reader_test : public abstract_instruction_reader
> +  {
> +  public:
> +    instruction_reader_test() = default ;
> +    instruction_reader_test (std::initializer_list<uint32_t> init)
> +      : insns{init} {}

I think we should put a space before "{" -> "insns {init}".  We put
it before "(", and before "{" in all other contexts.

> +
> +    ULONGEST read (CORE_ADDR memaddr, int len, enum bfd_endian byte_order)
> +    {
> +      SELF_CHECK (len == 4);
> +      SELF_CHECK (memaddr % 4 == 0);
> +      SELF_CHECK (memaddr / 4 < insns.size());
> +
> +      return insns[memaddr / 4];
> +    }
> +
> +  private:
> +    std::vector<uint32_t> insns;

Private data members should be prefixed with "m_".

I'll note that it always itches me a bit when we do
unnecessary copying.  :-)  In this case, you always
start from an array of instructions known at compile-time,
and copy it into the vector at run time.  You could
instead create the instructions array as a separate const
array, and pass than to the reader's constructor
as parameter, which would store a pointer to the array,
instead of a deep copy.


> +  };
> +
> +static void
> +aarch64_analyze_prologue_test (void)
> +{
> +  struct gdbarch_info info;
> +
> +  gdbarch_info_init (&info);
> +  info.bfd_arch_info = bfd_scan_arch ("aarch64");
> +
> +  struct gdbarch *gdbarch = gdbarch_find_by_info (info);
> +  SELF_CHECK (gdbarch != NULL);
> +
> +  struct aarch64_prologue_cache cache;
> +  cache.saved_regs = trad_frame_alloc_saved_regs (gdbarch);
> +
> +  instruction_reader_test test {
> +      0xa9af7bfd, /* stp     x29, x30, [sp,#-272]! */
> +      0x910003fd, /* mov     x29, sp */
> +      0x97ffffe6, /* bl      0x400580 */
> +   };

Indentation looks odd here.  "0x..." should be two columns
to the right of instruction_reader_test, and "};" aligned
at the same level as "instruction_reader_test".

Thanks,
Pedro Alves