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

[Pedro Alves <palves@redhat.com>]

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