[PATCH 2/2] [AArch64] Recognize STR instruction in prologue

[PATCH 2/2] [AArch64] Recognize STR instruction in prologue

[Yao Qi]

This patch teaches GDB AArch64 backend to recognize STR instructions
in prologue, like 'str x19, [sp, #-48]!' or 'str w0, [sp, #44]'.
The unit test is added too.

gdb:

2016-11-28  Yao Qi  <yao.qi@linaro.org>

	* aarch64-tdep.c (aarch64_analyze_prologue): Recognize STR
	instruction.
	(aarch64_analyze_prologue_test): More tests.
---
 gdb/aarch64-tdep.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index b10002a..fc68b8a 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -397,6 +397,35 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,
 	    regs[rn] = pv_add_constant (regs[rn], imm);
 
 	}
+      else if ((inst.opcode->iclass == ldst_imm9 /* Signed immediate.  */
+		|| (inst.opcode->iclass == ldst_pos /* Unsigned immediate.  */
+		    && (inst.opcode->op == OP_STR_POS
+			|| inst.opcode->op == OP_STRF_POS)))
+	       && inst.operands[1].addr.base_regno == AARCH64_SP_REGNUM
+	       && strcmp ("str", inst.opcode->name) == 0)
+	{
+	  /* STR (immediate) */
+	  unsigned int rt = inst.operands[0].reg.regno;
+	  int32_t imm = inst.operands[1].addr.offset.imm;
+	  unsigned rn = inst.operands[1].addr.base_regno;
+	  int is64
+	    = (aarch64_get_qualifier_esize (inst.operands[0].qualifier) == 8);
+	  gdb_assert (inst.operands[0].type == AARCH64_OPND_Rt
+		      || inst.operands[0].type == AARCH64_OPND_Ft);
+
+	  if (inst.operands[0].type == AARCH64_OPND_Ft)
+	    {
+	      /* Only bottom 64-bit of each V register (D register) need
+		 to be preserved.  */
+	      gdb_assert (inst.operands[0].qualifier == AARCH64_OPND_QLF_S_D);
+	      rt += AARCH64_X_REGISTER_COUNT;
+	    }
+
+	  pv_area_store (stack, pv_add_constant (regs[rn], imm),
+			 is64 ? 8 : 4, regs[rt]);
+	  if (inst.operands[1].addr.writeback)
+	    regs[rn] = pv_add_constant (regs[rn], imm);
+	}
       else if (inst.opcode->iclass == testbranch)
 	{
 	  /* Stop analysis on branch.  */
@@ -540,6 +569,45 @@ aarch64_analyze_prologue_test (void)
 
       SELF_CHECK (cache.saved_regs[i + regnum + AARCH64_D0_REGNUM].addr == -1);
     }
+
+  /* Second test.  */
+  cache.saved_regs = trad_frame_alloc_saved_regs (gdbarch);
+
+  instruction_reader_test test2 {
+      0xf81d0ff3, /* str	x19, [sp, #-48]! */
+      0xb9002fe0, /* str	w0, [sp, #44] */
+      0xf90013e1, /* str	x1, [sp, #32]*/
+      0xfd000fe0, /* str	d0, [sp, #24] */
+      0xaa0203f3, /* mov	x19, x2 */
+      0xf94013e0, /* ldr	x0, [sp, #32] */
+   };
+
+  end = aarch64_analyze_prologue (gdbarch, 0, 128, &cache, test2);
+
+  SELF_CHECK (end == 4 * 5);
+
+  SELF_CHECK (cache.framereg == AARCH64_SP_REGNUM);
+  SELF_CHECK (cache.framesize == 48);
+
+  for (int i = 0; i < AARCH64_X_REGISTER_COUNT; i++)
+    {
+      if (i == 1)
+	SELF_CHECK (cache.saved_regs[i].addr == -16);
+      else if (i == 19)
+	SELF_CHECK (cache.saved_regs[i].addr == -48);
+      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);
+
+      if (i == 0)
+	SELF_CHECK (cache.saved_regs[i + regnum + AARCH64_D0_REGNUM].addr == -24);
+      else
+	SELF_CHECK (cache.saved_regs[i + regnum + AARCH64_D0_REGNUM].addr == -1);
+    }
 }
 }
 #endif /* GDB_SELF_TEST */
-- 
1.9.1

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

[Yao Qi]

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

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

[Antoine Tremblay]

Yao Qi writes:

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

Could we use the default constructor here? If it's kept.

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

Very nit, but there's a space before ';'

Also I wonder if we need to specify the default constructor explicitly ?
Is there a rationale for it?

It's never used too, unless you apply my previous comment.

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

[Yao Qi]

On Tue, Nov 29, 2016 at 09:57:46AM -0500, Antoine Tremblay wrote:
> 
> Yao Qi writes:
> 
> > @@ -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 { };
> > +
> 
> Could we use the default constructor here? If it's kept.
> 

It uses the default constructor.  instruction_reader has the default
constructor.

+ public:
+  instruction_reader () = default;

Do you mean I need to remove it?

> > +  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 ;
> 
> Very nit, but there's a space before ';'
> 

Oh, yes, it should be "instruction_reader_test () = default;"

> Also I wonder if we need to specify the default constructor explicitly ?
> Is there a rationale for it?
> 
> It's never used too, unless you apply my previous comment.

The instruction_reader_test default constructor is never used.  How
about using "= delete"?

    instruction_reader_test () = delete;
    instruction_reader_test (std::initializer_list<uint32_t> init)
      : insns{init} {}

-- 
Yao (齐尧)

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

[Antoine Tremblay]

Yao Qi writes:

> On Tue, Nov 29, 2016 at 09:57:46AM -0500, Antoine Tremblay wrote:
>> 
>> Yao Qi writes:
>> 
>> > @@ -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 { };
>> > +
>> 
>> Could we use the default constructor here? If it's kept.
>> 
>
> It uses the default constructor.  instruction_reader has the default
> constructor.
>
> + public:
> +  instruction_reader () = default;
>
> Do you mean I need to remove it?
>

No, sorry I need to improve on C++11 I thought you needed a
std::initializer_list to init with {} like in the _test class but it
it works with the default constructor.

>> > +  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 ;
>> 
>> Very nit, but there's a space before ';'
>> 
>
> Oh, yes, it should be "instruction_reader_test () = default;"
>
>> Also I wonder if we need to specify the default constructor explicitly ?
>> Is there a rationale for it?
>> 
>> It's never used too, unless you apply my previous comment.
>
> The instruction_reader_test default constructor is never used.  How
> about using "= delete"?
>
>     instruction_reader_test () = delete;
>     instruction_reader_test (std::initializer_list<uint32_t> init)
>       : insns{init} {}

Yes that would be more appropriate if we're going to specify that.

I just wrote a patch with a C++ class and did not include explicit
default constructors do you think we should make it a code convention to
explicitly specify their existence or non-existence (=default, =delete) ?

I could not find mention of that in GCC's C++ conventions...

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

[Yao Qi]

On Wed, Nov 30, 2016 at 06:53:08AM -0500, Antoine Tremblay wrote:
> >> Also I wonder if we need to specify the default constructor explicitly ?
> >> Is there a rationale for it?
> >> 
> >> It's never used too, unless you apply my previous comment.
> >
> > The instruction_reader_test default constructor is never used.  How
> > about using "= delete"?
> >
> >     instruction_reader_test () = delete;
> >     instruction_reader_test (std::initializer_list<uint32_t> init)
> >       : insns{init} {}
> 
> Yes that would be more appropriate if we're going to specify that.
> 
> I just wrote a patch with a C++ class and did not include explicit
> default constructors do you think we should make it a code convention to
> explicitly specify their existence or non-existence (=default, =delete) ?

If you don't want default constructor to be used, "=delete" is useful,
IMO, which tells compiler not to generate the default constructor.  The
intention is quite clear that I don't want you to use the default
constructor.

Using "=default" is not that clear.  I personally prefer to write code
in an explicit way, so I prefer putting "=default" at the end.

> 
> I could not find mention of that in GCC's C++ conventions...

IMO, using "=default" is a personal programming habit, so it is
reasonable not to mention it in C++ code conventions.

-- 
Yao (齐尧)

From 0929e7f3819388c262e7ac8da157464361bb7787 Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Mon, 28 Nov 2016 10:40:59 +0000
Subject: [PATCH] Add unit test to aarch64 prologue analyzer

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.

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 6b95d7c..3838393 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;
+
+  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
+  {
+  public:
+    instruction_reader_test () = delete;
+    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;
+  };
+
+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 */
+   };
+
+  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.  */

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

[Antoine Tremblay]

Yao Qi writes:

> On Wed, Nov 30, 2016 at 06:53:08AM -0500, Antoine Tremblay wrote:
>> >> Also I wonder if we need to specify the default constructor explicitly ?
>> >> Is there a rationale for it?
>> >> 
>> >> It's never used too, unless you apply my previous comment.
>> >
>> > The instruction_reader_test default constructor is never used.  How
>> > about using "= delete"?
>> >
>> >     instruction_reader_test () = delete;
>> >     instruction_reader_test (std::initializer_list<uint32_t> init)
>> >       : insns{init} {}
>> 
>> Yes that would be more appropriate if we're going to specify that.
>> 
>> I just wrote a patch with a C++ class and did not include explicit
>> default constructors do you think we should make it a code convention to
>> explicitly specify their existence or non-existence (=default, =delete) ?
>
> If you don't want default constructor to be used, "=delete" is useful,
> IMO, which tells compiler not to generate the default constructor.  The
> intention is quite clear that I don't want you to use the default
> constructor.
>

OK.

> Using "=default" is not that clear.  I personally prefer to write code
> in an explicit way, so I prefer putting "=default" at the end.
>
>> 
>> I could not find mention of that in GCC's C++ conventions...
>
> IMO, using "=default" is a personal programming habit, so it is
> reasonable not to mention it in C++ code conventions.

OK thanks for the clarification.

The patch LGTM.

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

[Pedro Alves]

On 11/30/2016 04:34 PM, Yao Qi wrote:
> On Wed, Nov 30, 2016 at 06:53:08AM -0500, Antoine Tremblay wrote:
>>>> Also I wonder if we need to specify the default constructor explicitly ?
>>>> Is there a rationale for it?
>>>>
>>>> It's never used too, unless you apply my previous comment.
>>>
>>> The instruction_reader_test default constructor is never used.  How
>>> about using "= delete"?
>>>
>>>     instruction_reader_test () = delete;
>>>     instruction_reader_test (std::initializer_list<uint32_t> init)
>>>       : insns{init} {}
>>
>> Yes that would be more appropriate if we're going to specify that.
>>
>> I just wrote a patch with a C++ class and did not include explicit
>> default constructors do you think we should make it a code convention to
>> explicitly specify their existence or non-existence (=default, =delete) ?
> 
> If you don't want default constructor to be used, "=delete" is useful,
> IMO, which tells compiler not to generate the default constructor.  The
> intention is quite clear that I don't want you to use the default
> constructor.

Note that if the class has a user-defined constructor, then the
default ctor is not generated at all.  So you don't need to delete
the default one then.  If I see a "=delete" in such case (as above),
I'll scratch my head wondering why the redundancy is there in the
first place, and maybe if so inclined that day remove the unnecessary
code as an obvious cleanup patch.  :-)

(deleted functions participate in overload resolution, so there's
a difference from the method not being defined, but I don't believe
that's what you were going for here.)

Thanks,
Pedro Alves

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

[Pedro Alves]

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

Re: [PATCH 2/2] [AArch64] Recognize STR instruction in prologue

[Pedro Alves]

On 11/29/2016 02:12 PM, Yao Qi wrote:
> This patch teaches GDB AArch64 backend to recognize STR instructions
> in prologue, like 'str x19, [sp, #-48]!' or 'str w0, [sp, #44]'.
> The unit test is added too.
> 
> gdb:
> 
> 2016-11-28  Yao Qi  <yao.qi@linaro.org>
> 
> 	* aarch64-tdep.c (aarch64_analyze_prologue): Recognize STR
> 	instruction.
> 	(aarch64_analyze_prologue_test): More tests.
> ---
>  gdb/aarch64-tdep.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
> 
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index b10002a..fc68b8a 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -397,6 +397,35 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,
>  	    regs[rn] = pv_add_constant (regs[rn], imm);
>  
>  	}
> +      else if ((inst.opcode->iclass == ldst_imm9 /* Signed immediate.  */
> +		|| (inst.opcode->iclass == ldst_pos /* Unsigned immediate.  */
> +		    && (inst.opcode->op == OP_STR_POS
> +			|| inst.opcode->op == OP_STRF_POS)))
> +	       && inst.operands[1].addr.base_regno == AARCH64_SP_REGNUM
> +	       && strcmp ("str", inst.opcode->name) == 0)
> +	{
> +	  /* STR (immediate) */
> +	  unsigned int rt = inst.operands[0].reg.regno;
> +	  int32_t imm = inst.operands[1].addr.offset.imm;
> +	  unsigned rn = inst.operands[1].addr.base_regno;
> +	  int is64
> +	    = (aarch64_get_qualifier_esize (inst.operands[0].qualifier) == 8);
> +	  gdb_assert (inst.operands[0].type == AARCH64_OPND_Rt
> +		      || inst.operands[0].type == AARCH64_OPND_Ft);
> +
> +	  if (inst.operands[0].type == AARCH64_OPND_Ft)
> +	    {
> +	      /* Only bottom 64-bit of each V register (D register) need
> +		 to be preserved.  */
> +	      gdb_assert (inst.operands[0].qualifier == AARCH64_OPND_QLF_S_D);
> +	      rt += AARCH64_X_REGISTER_COUNT;
> +	    }
> +
> +	  pv_area_store (stack, pv_add_constant (regs[rn], imm),
> +			 is64 ? 8 : 4, regs[rt]);
> +	  if (inst.operands[1].addr.writeback)
> +	    regs[rn] = pv_add_constant (regs[rn], imm);
> +	}
>        else if (inst.opcode->iclass == testbranch)
>  	{
>  	  /* Stop analysis on branch.  */
> @@ -540,6 +569,45 @@ aarch64_analyze_prologue_test (void)
>  
>        SELF_CHECK (cache.saved_regs[i + regnum + AARCH64_D0_REGNUM].addr == -1);
>      }
> +
> +  /* Second test.  */

I'd suggest putting each test in its own scope, to avoid accidental
local variable reuse (or unintentionally reusing internal state of
internal variables).  Like

 /* Second test.  */
 {
   /* test code here */
 }

Also, I think you should expand that "second test" comment.  It should
inform the reader about what is important about the test.  In this
case, it should probably say something about "STR".

Likewise for the first test.

> +  cache.saved_regs = trad_frame_alloc_saved_regs (gdbarch);
> +
> +  instruction_reader_test test2 {
> +      0xf81d0ff3, /* str	x19, [sp, #-48]! */
> +      0xb9002fe0, /* str	w0, [sp, #44] */
> +      0xf90013e1, /* str	x1, [sp, #32]*/
> +      0xfd000fe0, /* str	d0, [sp, #24] */
> +      0xaa0203f3, /* mov	x19, x2 */
> +      0xf94013e0, /* ldr	x0, [sp, #32] */
> +   };

Indentation looks odd here too.

Thanks,
Pedro Alves

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

[Pedro Alves]

On 11/30/2016 06:29 PM, Pedro Alves wrote:

> 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.

I applied the series locally and gave this a go, to
show what I meant.  See below.

You'd indent the body of the new scopes, of course.
I didn't do that just to keep the illustration readable.
Move each test to a separate function would be another option.

From 9ec53dbaa9d4b463849a92b56c579b728eee2830 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 30 Nov 2016 17:31:27 +0000
Subject: [PATCH] selftest

---
 gdb/aarch64-tdep.c | 60 +++++++++++++++++++++++++++++++-----------------------
 1 file changed, 34 insertions(+), 26 deletions(-)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 8754eb0..96f9b34 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -211,8 +211,6 @@ public:
 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);
@@ -495,7 +493,7 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,
 			  CORE_ADDR start, CORE_ADDR limit,
 			  struct aarch64_prologue_cache *cache)
 {
-  instruction_reader reader { };
+  instruction_reader reader;
 
   return aarch64_analyze_prologue (gdbarch, start, limit, cache,
 				   reader);
@@ -509,21 +507,24 @@ namespace selftests {
   class instruction_reader_test : public abstract_instruction_reader
   {
   public:
-    instruction_reader_test () = delete;
-    instruction_reader_test (std::initializer_list<uint32_t> init)
-      : insns{init} {}
+
+    template<size_t SIZE>
+    instruction_reader_test (const uint32_t (&insns)[SIZE])
+      : m_insns (insns), m_insns_size (SIZE)
+    {}
 
     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());
+      SELF_CHECK (memaddr / 4 < m_insns_size);
 
-      return insns[memaddr / 4];
+      return m_insns[memaddr / 4];
     }
 
   private:
-    std::vector<uint32_t> insns;
+    const uint32_t *m_insns;
+    size_t m_insns_size;
   };
 
 static void
@@ -537,17 +538,19 @@ aarch64_analyze_prologue_test (void)
   struct gdbarch *gdbarch = gdbarch_find_by_info (info);
   SELF_CHECK (gdbarch != NULL);
 
+  /* Say something more descriptive here?  */
+  {
   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 */
-   };
+  static const uint32_t insns[] = {
+    0xa9af7bfd, /* stp     x29, x30, [sp,#-272]! */
+    0x910003fd, /* mov     x29, sp */
+    0x97ffffe6, /* bl      0x400580 */
+  };
+  instruction_reader_test reader (insns);
 
-  CORE_ADDR end = aarch64_analyze_prologue (gdbarch, 0, 128,
-					    &cache, test);
+  CORE_ADDR end = aarch64_analyze_prologue (gdbarch, 0, 128, &cache, reader);
   SELF_CHECK (end == 4 * 2);
 
   SELF_CHECK (cache.framereg == AARCH64_FP_REGNUM);
@@ -569,20 +572,24 @@ aarch64_analyze_prologue_test (void)
 
       SELF_CHECK (cache.saved_regs[i + regnum + AARCH64_D0_REGNUM].addr == -1);
     }
+  }
 
-  /* Second test.  */
+  /* Second test.  Say something more descriptive here?  */
+  {
+  struct aarch64_prologue_cache cache;
   cache.saved_regs = trad_frame_alloc_saved_regs (gdbarch);
 
-  instruction_reader_test test2 {
-      0xf81d0ff3, /* str	x19, [sp, #-48]! */
-      0xb9002fe0, /* str	w0, [sp, #44] */
-      0xf90013e1, /* str	x1, [sp, #32]*/
-      0xfd000fe0, /* str	d0, [sp, #24] */
-      0xaa0203f3, /* mov	x19, x2 */
-      0xf94013e0, /* ldr	x0, [sp, #32] */
-   };
+  const uint32_t insns[] = {
+    0xf81d0ff3, /* str	x19, [sp, #-48]! */
+    0xb9002fe0, /* str	w0, [sp, #44] */
+    0xf90013e1, /* str	x1, [sp, #32]*/
+    0xfd000fe0, /* str	d0, [sp, #24] */
+    0xaa0203f3, /* mov	x19, x2 */
+    0xf94013e0, /* ldr	x0, [sp, #32] */
+  };
+  instruction_reader_test reader (insns);
 
-  end = aarch64_analyze_prologue (gdbarch, 0, 128, &cache, test2);
+  CORE_ADDR end = aarch64_analyze_prologue (gdbarch, 0, 128, &cache, reader);
 
   SELF_CHECK (end == 4 * 5);
 
@@ -608,6 +615,7 @@ aarch64_analyze_prologue_test (void)
       else
 	SELF_CHECK (cache.saved_regs[i + regnum + AARCH64_D0_REGNUM].addr == -1);
     }
+  }
 }
 }
 #endif /* GDB_SELF_TEST */
-- 
2.5.5

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

[Luis Machado]

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.

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

[Yao Qi]

v2: Address review comments from Antoine, Pedro, and Luis.

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>
	    Pedro Alves  <palves@redhat.com>

	* 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 | 122 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 gdb/trad-frame.c   |  21 ++++++---
 gdb/trad-frame.h   |   1 +
 3 files changed, 135 insertions(+), 9 deletions(-)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 6b95d7c..c8a69a8 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,27 @@ 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:
+  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 +224,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 +244,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 +459,97 @@ 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
+  {
+  public:
+    template<size_t SIZE>
+    instruction_reader_test (const uint32_t (&insns)[SIZE])
+      : m_insns (insns), m_insns_size (SIZE)
+    {}
+
+    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 < m_insns_size);
+
+      return m_insns[memaddr / 4];
+    }
+
+  private:
+    const uint32_t *m_insns;
+    size_t m_insns_size;
+  };
+
+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);
+
+  /* Test the simple prologue in which frame pointer is used.  */
+  {
+    struct aarch64_prologue_cache cache;
+    cache.saved_regs = trad_frame_alloc_saved_regs (gdbarch);
+
+    static const uint32_t insns[] = {
+      0xa9af7bfd, /* stp     x29, x30, [sp,#-272]! */
+      0x910003fd, /* mov     x29, sp */
+      0x97ffffe6, /* bl      0x400580 */
+    };
+    instruction_reader_test reader (insns);
+
+    CORE_ADDR end = aarch64_analyze_prologue (gdbarch, 0, 128, &cache, reader);
+    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 +2978,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.  */
-- 
1.9.1

[PATCH 2/2 v2] [AArch64] Recognize STR instruction in prologue

[Yao Qi]

This patch teaches GDB AArch64 backend to recognize STR instructions
in prologue, like 'str x19, [sp, #-48]!' or 'str w0, [sp, #44]'.
The unit test is added too.

gdb:

2016-11-28  Yao Qi  <yao.qi@linaro.org>
	    Pedro Alves  <palves@redhat.com>

	* aarch64-tdep.c (aarch64_analyze_prologue): Recognize STR
	instruction.
	(aarch64_analyze_prologue_test): More tests.
---
 gdb/aarch64-tdep.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index c8a69a8..45dd5e4 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -395,6 +395,35 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,
 	    regs[rn] = pv_add_constant (regs[rn], imm);
 
 	}
+      else if ((inst.opcode->iclass == ldst_imm9 /* Signed immediate.  */
+		|| (inst.opcode->iclass == ldst_pos /* Unsigned immediate.  */
+		    && (inst.opcode->op == OP_STR_POS
+			|| inst.opcode->op == OP_STRF_POS)))
+	       && inst.operands[1].addr.base_regno == AARCH64_SP_REGNUM
+	       && strcmp ("str", inst.opcode->name) == 0)
+	{
+	  /* STR (immediate) */
+	  unsigned int rt = inst.operands[0].reg.regno;
+	  int32_t imm = inst.operands[1].addr.offset.imm;
+	  unsigned rn = inst.operands[1].addr.base_regno;
+	  int is64
+	    = (aarch64_get_qualifier_esize (inst.operands[0].qualifier) == 8);
+	  gdb_assert (inst.operands[0].type == AARCH64_OPND_Rt
+		      || inst.operands[0].type == AARCH64_OPND_Ft);
+
+	  if (inst.operands[0].type == AARCH64_OPND_Ft)
+	    {
+	      /* Only bottom 64-bit of each V register (D register) need
+		 to be preserved.  */
+	      gdb_assert (inst.operands[0].qualifier == AARCH64_OPND_QLF_S_D);
+	      rt += AARCH64_X_REGISTER_COUNT;
+	    }
+
+	  pv_area_store (stack, pv_add_constant (regs[rn], imm),
+			 is64 ? 8 : 4, regs[rt]);
+	  if (inst.operands[1].addr.writeback)
+	    regs[rn] = pv_add_constant (regs[rn], imm);
+	}
       else if (inst.opcode->iclass == testbranch)
 	{
 	  /* Stop analysis on branch.  */
@@ -546,6 +575,52 @@ aarch64_analyze_prologue_test (void)
 		    == -1);
       }
   }
+
+  /* Test a prologue in which STR is used and frame pointer is not
+     used.  */
+  {
+    struct aarch64_prologue_cache cache;
+    cache.saved_regs = trad_frame_alloc_saved_regs (gdbarch);
+
+    const uint32_t insns[] = {
+      0xf81d0ff3, /* str	x19, [sp, #-48]! */
+      0xb9002fe0, /* str	w0, [sp, #44] */
+      0xf90013e1, /* str	x1, [sp, #32]*/
+      0xfd000fe0, /* str	d0, [sp, #24] */
+      0xaa0203f3, /* mov	x19, x2 */
+      0xf94013e0, /* ldr	x0, [sp, #32] */
+    };
+    instruction_reader_test reader (insns);
+
+    CORE_ADDR end = aarch64_analyze_prologue (gdbarch, 0, 128, &cache, reader);
+
+    SELF_CHECK (end == 4 * 5);
+
+    SELF_CHECK (cache.framereg == AARCH64_SP_REGNUM);
+    SELF_CHECK (cache.framesize == 48);
+
+    for (int i = 0; i < AARCH64_X_REGISTER_COUNT; i++)
+      {
+	if (i == 1)
+	  SELF_CHECK (cache.saved_regs[i].addr == -16);
+	else if (i == 19)
+	  SELF_CHECK (cache.saved_regs[i].addr == -48);
+	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);
+
+	if (i == 0)
+	  SELF_CHECK (cache.saved_regs[i + regnum + AARCH64_D0_REGNUM].addr
+		      == -24);
+	else
+	  SELF_CHECK (cache.saved_regs[i + regnum + AARCH64_D0_REGNUM].addr
+		      == -1);
+      }
+  }
 }
 }
 #endif /* GDB_SELF_TEST */
-- 
1.9.1

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

[Pedro Alves]

On 11/30/2016 07:30 PM, Luis Machado wrote:

> 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.

Looking at the GCC codebase, the style with trailing { on the same
line (my preferred) is prevalent:

 $ grep "^namespace " gcc/* | grep "{$" | wc -l
 282
 $ grep "^namespace " gcc/* | grep -v "{$" | wc -l
 28

That style lends itself to opening nested namespaces more
like a FQN, like:

 namespace foo { namespace bar {

With C++17, we'll be able to write:

 namespace foo::bar {

Thanks,
Pedro Alves

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

[Pedro Alves]

On 12/01/2016 11:16 AM, Yao Qi wrote:

> +#if GDB_SELF_TEST
> +
> +namespace selftests
> +{
> +
> +  /* Instruction reader from manually cooked instruction sequences.  */
> +
> +  class instruction_reader_test : public abstract_instruction_reader
> +  {

This whole class should be indented at column 0.

> +  public:
> +    template<size_t SIZE>
> +    instruction_reader_test (const uint32_t (&insns)[SIZE])
> +      : m_insns (insns), m_insns_size (SIZE)
> +    {}

Please add "explicit".  Sorry that I missed adding it myself.

Otherwise LGTM.

Thanks,
Pedro Alves

Re: [PATCH 2/2 v2] [AArch64] Recognize STR instruction in prologue

[Pedro Alves]

Hi Yao,

On 12/01/2016 11:16 AM, Yao Qi wrote:

> +      else if ((inst.opcode->iclass == ldst_imm9 /* Signed immediate.  */
> +		|| (inst.opcode->iclass == ldst_pos /* Unsigned immediate.  */
> +		    && (inst.opcode->op == OP_STR_POS
> +			|| inst.opcode->op == OP_STRF_POS)))
> +	       && inst.operands[1].addr.base_regno == AARCH64_SP_REGNUM
> +	       && strcmp ("str", inst.opcode->name) == 0)
> +	{
> +	  /* STR (immediate) */
> +	  unsigned int rt = inst.operands[0].reg.regno;
> +	  int32_t imm = inst.operands[1].addr.offset.imm;
> +	  unsigned rn = inst.operands[1].addr.base_regno;

Mixed "unsigned int" vs "unsigned" style.

> +	  int is64

"bool".

> +	    = (aarch64_get_qualifier_esize (inst.operands[0].qualifier) == 8);
> +	  gdb_assert (inst.operands[0].type == AARCH64_OPND_Rt
> +		      || inst.operands[0].type == AARCH64_OPND_Ft);
> +
> +	  if (inst.operands[0].type == AARCH64_OPND_Ft)
> +	    {
> +	      /* Only bottom 64-bit of each V register (D register) need
> +		 to be preserved.  */
> +	      gdb_assert (inst.operands[0].qualifier == AARCH64_OPND_QLF_S_D);
> +	      rt += AARCH64_X_REGISTER_COUNT;
> +	    }
> +
> +	  pv_area_store (stack, pv_add_constant (regs[rn], imm),
> +			 is64 ? 8 : 4, regs[rt]);
> +	  if (inst.operands[1].addr.writeback)
> +	    regs[rn] = pv_add_constant (regs[rn], imm);
> +	}

>        else if (inst.opcode->iclass == testbranch)
>  	{
>  	  /* Stop analysis on branch.  */
> @@ -546,6 +575,52 @@ aarch64_analyze_prologue_test (void)
>  		    == -1);
>        }
>    }
> +
> +  /* Test a prologue in which STR is used and frame pointer is not
> +     used.  */

Thanks for the new comments.  This helps.

> +  {
> +    struct aarch64_prologue_cache cache;
> +    cache.saved_regs = trad_frame_alloc_saved_regs (gdbarch);
> +
> +    const uint32_t insns[] = {

"static const".  Sorry, my fault.

Othewrise, code-style-wise LGTM.  Thanks much for updating.

I've not payed attention to Aarch64-specifics, TBC.  I just
assume you got those right.  :-)

Thanks,
Pedro Alves

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

[Yao Qi]

On Thu, Dec 01, 2016 at 12:57:45PM +0000, Pedro Alves wrote:
> On 12/01/2016 11:16 AM, Yao Qi wrote:
> 
> > +#if GDB_SELF_TEST
> > +
> > +namespace selftests
> > +{
> > +
> > +  /* Instruction reader from manually cooked instruction sequences.  */
> > +
> > +  class instruction_reader_test : public abstract_instruction_reader
> > +  {
> 
> This whole class should be indented at column 0.
> 

Hi Pedro,
That is how emacs indents my code (at column 2).  Even I start emacs
with -q, it still indents that way.

-- 
Yao (齐尧)

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

[Yao Qi]

On Thu, Dec 01, 2016 at 03:21:15PM +0000, Yao Qi wrote:
> On Thu, Dec 01, 2016 at 12:57:45PM +0000, Pedro Alves wrote:
> > On 12/01/2016 11:16 AM, Yao Qi wrote:
> > 
> > > +#if GDB_SELF_TEST
> > > +
> > > +namespace selftests
> > > +{
> > > +
> > > +  /* Instruction reader from manually cooked instruction sequences.  */
> > > +
> > > +  class instruction_reader_test : public abstract_instruction_reader
> > > +  {
> > 
> > This whole class should be indented at column 0.
> > 
> 
> Hi Pedro,
> That is how emacs indents my code (at column 2).  Even I start emacs
> with -q, it still indents that way.
> 

I managed to get emacs indent at column 0 with the following changes in
gdb/.dir-locals.el.

--- a/gdb/.dir-locals.el
+++ b/gdb/.dir-locals.el
@@ -19,6 +19,12 @@
              (tcl-continued-indent-level . 4)
              (indent-tabs-mode . t)))
  (nil . ((bug-reference-url-format . "http://sourceware.org/bugzilla/show_bug.cgi?id=%s")))
- (c-mode . ((c-file-style . "GNU")
-           (indent-tabs-mode . t)))
+ (c-mode . (
+           (c-file-style . "GNU")
+           (mode . c++)
+           (indent-tabs-mode . t)
+           (c-offsets-alist . (
+                               (innamespace . 0)
+                               ))
+           ))
 )

-- 
Yao (齐尧)

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

[Pedro Alves]

On 12/01/2016 04:04 PM, Yao Qi wrote:
> On Thu, Dec 01, 2016 at 03:21:15PM +0000, Yao Qi wrote:
>> On Thu, Dec 01, 2016 at 12:57:45PM +0000, Pedro Alves wrote:
>>> On 12/01/2016 11:16 AM, Yao Qi wrote:
>>>
>>>> +#if GDB_SELF_TEST
>>>> +
>>>> +namespace selftests
>>>> +{
>>>> +
>>>> +  /* Instruction reader from manually cooked instruction sequences.  */
>>>> +
>>>> +  class instruction_reader_test : public abstract_instruction_reader
>>>> +  {
>>>
>>> This whole class should be indented at column 0.
>>>
>>
>> Hi Pedro,
>> That is how emacs indents my code (at column 2).  Even I start emacs
>> with -q, it still indents that way.
>>
> 
> I managed to get emacs indent at column 0 with the following changes in
> gdb/.dir-locals.el.

That unfortunately somehow makes emacs indent members of classes
(even if not in a namespace) at column 0, like:

struct foo
{
int a;
};

Do you get that too?

BTW, the don't-indent-body-of-namespace guideline is here:

  https://gcc.gnu.org/codingconventions.html#Namespace_Form

and it does look like GCC generally follows it.

(The '{'-on-same-line rule is there too.)

Thanks,
Pedro Alves

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

[Yao Qi]

On Thu, Dec 01, 2016 at 06:05:30PM +0000, Pedro Alves wrote:
> > 
> > I managed to get emacs indent at column 0 with the following changes in
> > gdb/.dir-locals.el.
> 
> That unfortunately somehow makes emacs indent members of classes
> (even if not in a namespace) at column 0, like:
> 
> struct foo
> {
> int a;
> };
> 
> Do you get that too?
> 

Yes, :(

> BTW, the don't-indent-body-of-namespace guideline is here:
> 
>   https://gcc.gnu.org/codingconventions.html#Namespace_Form
> 
> and it does look like GCC generally follows it.

I still can't get me emacs indent code inside namespace that way, but
I manually fixed it.  I'll ask gcc people how do they set emacs.

> 
> (The '{'-on-same-line rule is there too.)
> 

Fixed.  Patch is pushed in.

-- 
Yao (齐尧)

From 4d9a9006139d1ceea787cdda871dff8943e493f0 Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Fri, 2 Dec 2016 09:37:30 +0000
Subject: [PATCH] Add unit test to aarch64 prologue analyzer

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-12-02  Yao Qi  <yao.qi@linaro.org>
	    Pedro Alves  <palves@redhat.com>

	* 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.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 620b582..b4dd117 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,19 @@
+2016-12-02  Yao Qi  <yao.qi@linaro.org>
+	    Pedro Alves  <palves@redhat.com>
+
+	* 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.
+
 2016-12-01  Simon Marchi  <simon.marchi@polymtl.ca>
 
 	* ui-out.c (enum ui_out_table_state): Move to class
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 6b95d7c..576ee70 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,27 @@ 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:
+  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 +224,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 +244,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 +459,96 @@ 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
+{
+public:
+  template<size_t SIZE>
+  explicit instruction_reader_test (const uint32_t (&insns)[SIZE])
+  : m_insns (insns), m_insns_size (SIZE)
+  {}
+
+  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 < m_insns_size);
+
+    return m_insns[memaddr / 4];
+  }
+
+private:
+  const uint32_t *m_insns;
+  size_t m_insns_size;
+};
+
+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);
+
+  /* Test the simple prologue in which frame pointer is used.  */
+  {
+    struct aarch64_prologue_cache cache;
+    cache.saved_regs = trad_frame_alloc_saved_regs (gdbarch);
+
+    static const uint32_t insns[] = {
+      0xa9af7bfd, /* stp     x29, x30, [sp,#-272]! */
+      0x910003fd, /* mov     x29, sp */
+      0x97ffffe6, /* bl      0x400580 */
+    };
+    instruction_reader_test reader (insns);
+
+    CORE_ADDR end = aarch64_analyze_prologue (gdbarch, 0, 128, &cache, reader);
+    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);
+      }
+  }
+}
+} // namespace selftests
+#endif /* GDB_SELF_TEST */
+
 /* Implement the "skip_prologue" gdbarch method.  */
 
 static CORE_ADDR
@@ -2864,6 +2977,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.  */

Re: [PATCH 2/2 v2] [AArch64] Recognize STR instruction in prologue

[Yao Qi]

On Thu, Dec 01, 2016 at 01:07:21PM +0000, Pedro Alves wrote:
> Hi Yao,
> 
> On 12/01/2016 11:16 AM, Yao Qi wrote:
> 
> > +      else if ((inst.opcode->iclass == ldst_imm9 /* Signed immediate.  */
> > +		|| (inst.opcode->iclass == ldst_pos /* Unsigned immediate.  */
> > +		    && (inst.opcode->op == OP_STR_POS
> > +			|| inst.opcode->op == OP_STRF_POS)))
> > +	       && inst.operands[1].addr.base_regno == AARCH64_SP_REGNUM
> > +	       && strcmp ("str", inst.opcode->name) == 0)
> > +	{
> > +	  /* STR (immediate) */
> > +	  unsigned int rt = inst.operands[0].reg.regno;
> > +	  int32_t imm = inst.operands[1].addr.offset.imm;
> > +	  unsigned rn = inst.operands[1].addr.base_regno;
> 
> Mixed "unsigned int" vs "unsigned" style.
> 

Fixed.

> > +	  int is64
> 
> "bool".

Fixed.

> 
> > +  {
> > +    struct aarch64_prologue_cache cache;
> > +    cache.saved_regs = trad_frame_alloc_saved_regs (gdbarch);
> > +
> > +    const uint32_t insns[] = {
> 
> "static const".  Sorry, my fault.
> 

Fixed.

> Othewrise, code-style-wise LGTM.  Thanks much for updating.
> 
 
Patch below is pushed in.

-- 
Yao (齐尧)

From 432ec0814b01a93b88eddf13092ea6abef34652d Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Fri, 2 Dec 2016 09:37:30 +0000
Subject: [PATCH] [AArch64] Recognize STR instruction in prologue

This patch teaches GDB AArch64 backend to recognize STR instructions
in prologue, like 'str x19, [sp, #-48]!' or 'str w0, [sp, #44]'.
The unit test is added too.

gdb:

2016-12-02  Yao Qi  <yao.qi@linaro.org>
	    Pedro Alves  <palves@redhat.com>

	* aarch64-tdep.c (aarch64_analyze_prologue): Recognize STR
	instruction.
	(aarch64_analyze_prologue_test): More tests.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index b4dd117..72eeea4 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,6 +1,13 @@
 2016-12-02  Yao Qi  <yao.qi@linaro.org>
 	    Pedro Alves  <palves@redhat.com>
 
+	* aarch64-tdep.c (aarch64_analyze_prologue): Recognize STR
+	instruction.
+	(aarch64_analyze_prologue_test): More tests.
+
+2016-12-02  Yao Qi  <yao.qi@linaro.org>
+	    Pedro Alves  <palves@redhat.com>
+
 	* aarch64-tdep.c: Include "selftest.h".
 	(abstract_instruction_reader): New class.
 	(instruction_reader): New class.
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 576ee70..590dcf6 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -395,6 +395,35 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,
 	    regs[rn] = pv_add_constant (regs[rn], imm);
 
 	}
+      else if ((inst.opcode->iclass == ldst_imm9 /* Signed immediate.  */
+		|| (inst.opcode->iclass == ldst_pos /* Unsigned immediate.  */
+		    && (inst.opcode->op == OP_STR_POS
+			|| inst.opcode->op == OP_STRF_POS)))
+	       && inst.operands[1].addr.base_regno == AARCH64_SP_REGNUM
+	       && strcmp ("str", inst.opcode->name) == 0)
+	{
+	  /* STR (immediate) */
+	  unsigned int rt = inst.operands[0].reg.regno;
+	  int32_t imm = inst.operands[1].addr.offset.imm;
+	  unsigned int rn = inst.operands[1].addr.base_regno;
+	  bool is64
+	    = (aarch64_get_qualifier_esize (inst.operands[0].qualifier) == 8);
+	  gdb_assert (inst.operands[0].type == AARCH64_OPND_Rt
+		      || inst.operands[0].type == AARCH64_OPND_Ft);
+
+	  if (inst.operands[0].type == AARCH64_OPND_Ft)
+	    {
+	      /* Only bottom 64-bit of each V register (D register) need
+		 to be preserved.  */
+	      gdb_assert (inst.operands[0].qualifier == AARCH64_OPND_QLF_S_D);
+	      rt += AARCH64_X_REGISTER_COUNT;
+	    }
+
+	  pv_area_store (stack, pv_add_constant (regs[rn], imm),
+			 is64 ? 8 : 4, regs[rt]);
+	  if (inst.operands[1].addr.writeback)
+	    regs[rn] = pv_add_constant (regs[rn], imm);
+	}
       else if (inst.opcode->iclass == testbranch)
 	{
 	  /* Stop analysis on branch.  */
@@ -545,6 +574,52 @@ aarch64_analyze_prologue_test (void)
 		    == -1);
       }
   }
+
+  /* Test a prologue in which STR is used and frame pointer is not
+     used.  */
+  {
+    struct aarch64_prologue_cache cache;
+    cache.saved_regs = trad_frame_alloc_saved_regs (gdbarch);
+
+    static const uint32_t insns[] = {
+      0xf81d0ff3, /* str	x19, [sp, #-48]! */
+      0xb9002fe0, /* str	w0, [sp, #44] */
+      0xf90013e1, /* str	x1, [sp, #32]*/
+      0xfd000fe0, /* str	d0, [sp, #24] */
+      0xaa0203f3, /* mov	x19, x2 */
+      0xf94013e0, /* ldr	x0, [sp, #32] */
+    };
+    instruction_reader_test reader (insns);
+
+    CORE_ADDR end = aarch64_analyze_prologue (gdbarch, 0, 128, &cache, reader);
+
+    SELF_CHECK (end == 4 * 5);
+
+    SELF_CHECK (cache.framereg == AARCH64_SP_REGNUM);
+    SELF_CHECK (cache.framesize == 48);
+
+    for (int i = 0; i < AARCH64_X_REGISTER_COUNT; i++)
+      {
+	if (i == 1)
+	  SELF_CHECK (cache.saved_regs[i].addr == -16);
+	else if (i == 19)
+	  SELF_CHECK (cache.saved_regs[i].addr == -48);
+	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);
+
+	if (i == 0)
+	  SELF_CHECK (cache.saved_regs[i + regnum + AARCH64_D0_REGNUM].addr
+		      == -24);
+	else
+	  SELF_CHECK (cache.saved_regs[i + regnum + AARCH64_D0_REGNUM].addr
+		      == -1);
+      }
+  }
 }
 } // namespace selftests
 #endif /* GDB_SELF_TEST */

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

[Pedro Alves]

On 12/02/2016 09:40 AM, Yao Qi wrote:
> On Thu, Dec 01, 2016 at 06:05:30PM +0000, Pedro Alves wrote:
>>>
>>> I managed to get emacs indent at column 0 with the following changes in
>>> gdb/.dir-locals.el.
>>
>> That unfortunately somehow makes emacs indent members of classes
>> (even if not in a namespace) at column 0, like:
>>
>> struct foo
>> {
>> int a;
>> };
>>
>> Do you get that too?
>>
> 
> Yes, :(
> 
>> BTW, the don't-indent-body-of-namespace guideline is here:
>>
>>   https://gcc.gnu.org/codingconventions.html#Namespace_Form
>>
>> and it does look like GCC generally follows it.
> 
> I still can't get me emacs indent code inside namespace that way, but
> I manually fixed it.  I'll ask gcc people how do they set emacs.

I have a custom c-mode on my ~/.emacs (predating gdb/.dir-locals.el),
and adding the namespace tweak there works correctly.  Eh.
Like this:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
(defun gdb-c-mode ()
  "C mode with adjusted defaults for use with GDB."
  (interactive)
  (c++-mode)
  (c-set-style "gnu")
  (setq tab-width 8)
  (setq indent-tabs-mode 1)
  (setq c-basic-offset 2)
  (c-set-offset 'innamespace 0)
  )
(setq auto-mode-alist (cons '(".*/gdb.*/.*\\.[ch]$" . gdb-c-mode)
			    auto-mode-alist))
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

So if .dir-locals.el can't work for some reason, we could put
something like that in gdb/gdb-code-style.el.

Thanks,
Pedro Alves