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

[Luis Machado <lgustavo@codesourcery.com>]

On 11/29/2016 08:12 AM, 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
> +{

There is a new line before the class declaration here, but not on some 
of the other declarations.

> +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;
> +
> +  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 { };
> +
> +  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
> +  {

No newline between class declaration and comment.

> +  public:
> +    instruction_reader_test() = default ;
> +    instruction_reader_test (std::initializer_list<uint32_t> init)
> +      : insns{init} {}
> +
> +    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;
> +  };

Should the curly braces both be on their own lines as the rest of the 
uses? I see mixed formatting in the uses of namespace we currently have. 
We should pick one and go with it.

I particularly dislike the curly brace on the same line being used to 
GNU's coding standards.

> +
> +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 */
> +   };

Same here.

> +
> +  CORE_ADDR end = aarch64_analyze_prologue (gdbarch, 0, 128,
> +					    &cache, test);
> +  SELF_CHECK (end == 4 * 2);
> +
> +  SELF_CHECK (cache.framereg == AARCH64_FP_REGNUM);
> +  SELF_CHECK (cache.framesize == 272);
> +
> +  for (int i = 0; i < AARCH64_X_REGISTER_COUNT; i++)
> +    {
> +      if (i == AARCH64_FP_REGNUM)
> +	SELF_CHECK (cache.saved_regs[i].addr == -272);
> +      else if (i == AARCH64_LR_REGNUM)
> +	SELF_CHECK (cache.saved_regs[i].addr == -264);
> +      else
> +	SELF_CHECK (cache.saved_regs[i].addr == -1);
> +    }
> +
> +  for (int i = 0; i < AARCH64_D_REGISTER_COUNT; i++)
> +    {
> +      int regnum = gdbarch_num_regs (gdbarch);
> +
> +      SELF_CHECK (cache.saved_regs[i + regnum + AARCH64_D0_REGNUM].addr == -1);
> +    }
> +}
> +}
> +#endif /* GDB_SELF_TEST */
> +
>  /* Implement the "skip_prologue" gdbarch method.  */
>
>  static CORE_ADDR
> @@ -2864,6 +2972,10 @@ When on, AArch64 specific debugging is enabled."),
>  			    NULL,
>  			    show_aarch64_debug,
>  			    &setdebuglist, &showdebuglist);
> +
> +#if GDB_SELF_TEST
> +  register_self_test (selftests::aarch64_analyze_prologue_test);
> +#endif
>  }
>
>  /* AArch64 process record-replay related structures, defines etc.  */
> diff --git a/gdb/trad-frame.c b/gdb/trad-frame.c
> index ebf19df..4430dd5 100644
> --- a/gdb/trad-frame.c
> +++ b/gdb/trad-frame.c
> @@ -43,16 +43,10 @@ trad_frame_cache_zalloc (struct frame_info *this_frame)
>    return this_trad_cache;
>  }
>
> -/* A traditional frame is unwound by analysing the function prologue
> -   and using the information gathered to track registers.  For
> -   non-optimized frames, the technique is reliable (just need to check
> -   for all potential instruction sequences).  */
> -
>  struct trad_frame_saved_reg *
> -trad_frame_alloc_saved_regs (struct frame_info *this_frame)
> +trad_frame_alloc_saved_regs (struct gdbarch *gdbarch)
>  {
>    int regnum;
> -  struct gdbarch *gdbarch = get_frame_arch (this_frame);
>    int numregs = gdbarch_num_regs (gdbarch) + gdbarch_num_pseudo_regs (gdbarch);
>    struct trad_frame_saved_reg *this_saved_regs
>      = FRAME_OBSTACK_CALLOC (numregs, struct trad_frame_saved_reg);
> @@ -65,6 +59,19 @@ trad_frame_alloc_saved_regs (struct frame_info *this_frame)
>    return this_saved_regs;
>  }
>
> +/* A traditional frame is unwound by analysing the function prologue
> +   and using the information gathered to track registers.  For
> +   non-optimized frames, the technique is reliable (just need to check
> +   for all potential instruction sequences).  */
> +
> +struct trad_frame_saved_reg *
> +trad_frame_alloc_saved_regs (struct frame_info *this_frame)
> +{
> +  struct gdbarch *gdbarch = get_frame_arch (this_frame);
> +
> +  return trad_frame_alloc_saved_regs (gdbarch);
> +}
> +
>  enum { TF_REG_VALUE = -1, TF_REG_UNKNOWN = -2 };
>
>  int
> diff --git a/gdb/trad-frame.h b/gdb/trad-frame.h
> index b8aed16..d1c24b0 100644
> --- a/gdb/trad-frame.h
> +++ b/gdb/trad-frame.h
> @@ -104,6 +104,7 @@ int trad_frame_realreg_p (struct trad_frame_saved_reg this_saved_regs[],
>
>  /* Return a freshly allocated (and initialized) trad_frame array.  */
>  struct trad_frame_saved_reg *trad_frame_alloc_saved_regs (struct frame_info *);
> +struct trad_frame_saved_reg *trad_frame_alloc_saved_regs (struct gdbarch *);
>
>  /* Given the trad_frame info, return the location of the specified
>     register.  */
>

Otherwise looks good to me.