[PATCH V2 0/2] AMD64, Prologue: Recognize stack decrementation as prologue operation.

[PATCH V2 0/2] AMD64, Prologue: Recognize stack decrementation as prologue operation.

[Bernhard Heckel]

- Added selftest
- Added comments according to review

Bernhard Heckel (2):
  AMD64, Prologue: Recognize stack decrementation as prologue operation.
  Prologue: Add selftests to x64/x32 architecture.

 gdb/amd64-tdep.c | 320 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 312 insertions(+), 8 deletions(-)

-- 
2.7.1.339.g0233b80

[PATCH V2 1/2] AMD64, Prologue: Recognize stack decrementation as prologue operation.

[Bernhard Heckel]

GCC, ICC and Clang decrement stack pointer within the prologue
sequence in order to reserve memory for local variables.
Recognize this subtraction to stop at the very end of the
prologue.

2016-12-16  Bernhard Heckel  <bernhard.heckel@intel.com>

gdb/Changelog:
	* amd64-tdep.c (amd64_analyze_prologue): Recognize stack decrementation
	  as prologue operation.

---
 gdb/amd64-tdep.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index a3a1fde..cbcddcb 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2261,11 +2261,19 @@ amd64_x32_analyze_stack_align (CORE_ADDR pc, CORE_ADDR current_pc,
 
       pushq %rbp        0x55
       movq %rsp, %rbp   0x48 0x89 0xe5 (or 0x48 0x8b 0xec)
+      in addition, functions containing local variables
+	sub imm8, %rsp   0x48 0x83 0xec
+      or
+	sub imm32, %rsp   0x48 0x81 0xec
 
    or (for the X32 ABI):
 
       pushq %rbp        0x55
       movl %esp, %ebp   0x89 0xe5 (or 0x8b 0xec)
+      in addition, functions containing local variables
+	sub imm8, %esp   0x83 0xec
+      or
+	sub imm32, %esp   0x81 0xec
 
    Any function that doesn't start with one of these sequences will be
    assumed to have no prologue and thus no valid frame pointer in
@@ -2283,6 +2291,12 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
   /* Ditto for movl %esp, %ebp.  */
   static const gdb_byte mov_esp_ebp_1[2] = { 0x89, 0xe5 };
   static const gdb_byte mov_esp_ebp_2[2] = { 0x8b, 0xec };
+  /* Ditto for subtraction on the stack pointer.  */
+  static const gdb_byte sub_rsp_imm8[3] = { 0x48, 0x83, 0xec };
+  static const gdb_byte sub_rsp_imm32[3] = { 0x48, 0x81, 0xec };
+  /* Ditto for subtraction on the stack pointer.  */
+  static const gdb_byte sub_esp_imm8[2] = { 0x83, 0xec };
+  static const gdb_byte sub_esp_imm32[2] = { 0x81, 0xec };
 
   gdb_byte buf[3];
   gdb_byte op;
@@ -2316,6 +2330,18 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
 	{
 	  /* OK, we actually have a frame.  */
 	  cache->frameless_p = 0;
+
+	  /* GCC, ICC and Clang do subtraction on the stack pointer
+	     to reserve memory for local variables.
+	     Two common variants exist to do so.  */
+	  read_code (pc + 4, buf, 3);
+	  if (memcmp (buf, sub_rsp_imm8, 3) == 0)
+	    /* Operand is 1 byte.  */
+	    return pc + 8;
+	  else if (memcmp (buf, sub_rsp_imm32, 3) == 0)
+	    /* Operand is 4 bytes.  */
+	    return pc + 11;
+
 	  return pc + 4;
 	}
 
@@ -2327,6 +2353,18 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
 	    {
 	      /* OK, we actually have a frame.  */
 	      cache->frameless_p = 0;
+
+	      /* GCC, ICC and Clang do subtraction on the stack pointer
+		 to reserve memory for local variables.
+		 Two common variants exist to do so.  */
+	      read_code (pc + 3, buf, 2);
+	      if (memcmp (buf, sub_esp_imm8, 2) == 0)
+		/* Operand is 1 byte.  */
+		return pc + 6;
+	      else if (memcmp (buf, sub_esp_imm32, 2) == 0)
+		/* Operand is 4 bytes.  */
+		return pc + 9;
+
 	      return pc + 3;
 	    }
 	}
-- 
2.7.1.339.g0233b80

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

[Bernhard Heckel]

2016-12-16  Bernhard Heckel  <bernhard.heckel@intel.com>

gdb/Changelog:
	* amd64-tdep.c: Include "selftest.h".
	(abstract_code_reader): New class.
	(code_reader): New class.
	(amd64_analyze_prologue): Add new parameter reader.  Call
	reader.read instead of read_code.
	[GDB_SELF_TEST] (code_reader_test): New class.
	(get_prologue_tests_x64): New function.
	(get_prologue_tests_x32): New function.
	(get_prologue_tests): New function.
	(amd64_analyze_prologue_test): New function.
	(_initialize_amd64_tdep) [GDB_SELF_TEST]: Register
	selftests::amd64_analyze_prologue_test.

---
 gdb/amd64-tdep.c | 286 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 276 insertions(+), 10 deletions(-)

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index cbcddcb..67ecdaeb6 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -54,6 +54,7 @@
 
 #include "ax.h"
 #include "ax-gdb.h"
+#include "selftest.h"
 
 /* Note that the AMD64 architecture was previously known as x86-64.
    The latter is (forever) engraved into the canonical system name as
@@ -2253,6 +2254,35 @@ amd64_x32_analyze_stack_align (CORE_ADDR pc, CORE_ADDR current_pc,
   return std::min (pc + offset + 2, current_pc);
 }
 
+/* Abstract code reader.  */
+
+class abstract_code_reader
+{
+protected:
+  bool target_memory = 0;
+public:
+  /* Read in one byte.  */
+  virtual void read (CORE_ADDR memaddr, gdb_byte *buffer, ssize_t len) = 0;
+
+  bool is_target_memory (void) { return target_memory;};
+};
+
+/* Code reader from real target.  */
+
+class code_reader : public abstract_code_reader
+{
+public:
+  code_reader (void)
+  {
+    target_memory = 1;
+  }
+
+  void read (CORE_ADDR memaddr, gdb_byte *buffer, ssize_t len)
+  {
+    read_code (memaddr, buffer, len);
+  }
+};
+
 /* Do a limited analysis of the prologue at PC and update CACHE
    accordingly.  Bail out early if CURRENT_PC is reached.  Return the
    address where the analysis stopped.
@@ -2282,7 +2312,8 @@ amd64_x32_analyze_stack_align (CORE_ADDR pc, CORE_ADDR current_pc,
 static CORE_ADDR
 amd64_analyze_prologue (struct gdbarch *gdbarch,
 			CORE_ADDR pc, CORE_ADDR current_pc,
-			struct amd64_frame_cache *cache)
+			struct amd64_frame_cache *cache,
+			abstract_code_reader& reader)
 {
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   /* There are two variations of movq %rsp, %rbp.  */
@@ -2304,12 +2335,15 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
   if (current_pc <= pc)
     return current_pc;
 
-  if (gdbarch_ptr_bit (gdbarch) == 32)
-    pc = amd64_x32_analyze_stack_align (pc, current_pc, cache);
-  else
-    pc = amd64_analyze_stack_align (pc, current_pc, cache);
-
-  op = read_code_unsigned_integer (pc, 1, byte_order);
+  /* For selftests, we don't analyze stack alignment.  */
+  if (reader.is_target_memory ())
+    {
+      if (gdbarch_ptr_bit (gdbarch) == 32)
+	pc = amd64_x32_analyze_stack_align (pc, current_pc, cache);
+      else
+	pc = amd64_analyze_stack_align (pc, current_pc, cache);
+    }
+  reader.read (pc, &op, 1);
 
   if (op == 0x55)		/* pushq %rbp */
     {
@@ -2322,7 +2356,7 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
       if (current_pc <= pc + 1)
         return current_pc;
 
-      read_code (pc + 1, buf, 3);
+      reader.read (pc + 1, buf, 3);
 
       /* Check for `movq %rsp, %rbp'.  */
       if (memcmp (buf, mov_rsp_rbp_1, 3) == 0
@@ -2334,7 +2368,7 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
 	  /* GCC, ICC and Clang do subtraction on the stack pointer
 	     to reserve memory for local variables.
 	     Two common variants exist to do so.  */
-	  read_code (pc + 4, buf, 3);
+	  reader.read (pc + 4, buf, 3);
 	  if (memcmp (buf, sub_rsp_imm8, 3) == 0)
 	    /* Operand is 1 byte.  */
 	    return pc + 8;
@@ -2357,7 +2391,7 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
 	      /* GCC, ICC and Clang do subtraction on the stack pointer
 		 to reserve memory for local variables.
 		 Two common variants exist to do so.  */
-	      read_code (pc + 3, buf, 2);
+	      reader.read (pc + 3, buf, 2);
 	      if (memcmp (buf, sub_esp_imm8, 2) == 0)
 		/* Operand is 1 byte.  */
 		return pc + 6;
@@ -2375,6 +2409,234 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
   return pc;
 }
 
+static CORE_ADDR
+amd64_analyze_prologue (struct gdbarch *gdbarch,
+			CORE_ADDR pc, CORE_ADDR current_pc,
+			struct amd64_frame_cache *cache)
+{
+  code_reader reader;
+
+  return amd64_analyze_prologue (gdbarch, pc, current_pc, cache, reader);
+}
+
+#if GDB_SELF_TEST
+
+namespace selftests {
+
+/* Code reader from manually created instruction sequences.  */
+
+class code_reader_test : public abstract_code_reader
+{
+private:
+  const std::vector<gdb_byte> memory;
+
+public:
+  explicit code_reader_test (const std::vector<gdb_byte> &memory)
+  : memory (memory)
+  {
+    /* Nothing to do.  */
+  }
+
+  void read (CORE_ADDR memaddr, gdb_byte *buffer, ssize_t len)
+  {
+    SELF_CHECK ((memaddr + len) <= memory.size ());
+    memcpy (buffer, memory.data () + memaddr, len);
+  }
+};
+
+struct prologue_test_t {
+  amd64_frame_cache exp_cache;
+  CORE_ADDR exp_pc;
+  std::vector<gdb_byte> memory;
+};
+
+/* Returns x64 test pattern and expected results.  */
+
+static std::vector<struct prologue_test_t> get_prologue_tests_x64 (void)
+{
+  std::vector<struct prologue_test_t> prologue_tests;
+  struct prologue_test_t prologue_test;
+
+  /* Negative test.  */
+  prologue_test.exp_pc = 0;
+  prologue_test.memory = {0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90,
+      0x90, 0x90, 0x90, 0x90, 0x90}; /* nop */
+  amd64_init_frame_cache (&prologue_test.exp_cache);
+  prologue_test.exp_cache.frameless_p = 1;
+  prologue_tests.push_back (prologue_test);
+
+  prologue_test.exp_pc = 4;
+  prologue_test.memory = {0x55, /* push %rbp */
+      0x48, 0x89, 0xe5,         /* mov  %rsp, %rbp */
+      0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90}; /* nop */
+  amd64_init_frame_cache (&prologue_test.exp_cache);
+  prologue_test.exp_cache.frameless_p = 0;
+  prologue_tests.push_back (prologue_test);
+
+  prologue_test.exp_pc = 4;
+  prologue_test.memory = {0x55, /* push %rbp */
+      0x48, 0x8b, 0xec,         /* mov  %rsp, %rbp */
+      0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90}; /* nop */
+  amd64_init_frame_cache (&prologue_test.exp_cache);
+  prologue_test.exp_cache.frameless_p = 0;
+  prologue_tests.push_back (prologue_test);
+
+  prologue_test.exp_pc = 8;
+  prologue_test.memory = {0x55, /* push %rbp */
+      0x48, 0x89, 0xe5,         /* mov  %rsp, %rbp */
+      0x48, 0x83, 0xec, 0x10,   /* sub  imm8, %rsp */
+      0x90, 0x90, 0x90, 0x90, 0x90, 0x90}; /* nop */
+  amd64_init_frame_cache (&prologue_test.exp_cache);
+  prologue_test.exp_cache.frameless_p = 0;
+  prologue_tests.push_back (prologue_test);
+
+  prologue_test.exp_pc = 11;
+  prologue_test.memory = {0x55, /* push %rbp */
+      0x48, 0x89, 0xe5,         /* mov  %rsp, %rbp */
+      0x48, 0x81, 0xec, 0x10, 0x00, 0x00, 0x00, /* sub  imm32, %rsp */
+      0x90, 0x90, 0x90}; /* nop */
+  amd64_init_frame_cache (&prologue_test.exp_cache);
+  prologue_test.exp_cache.frameless_p = 0;
+  prologue_tests.push_back (prologue_test);
+
+  prologue_test.exp_pc = 8;
+  prologue_test.memory = {0x55, /* push %rbp */
+      0x48, 0x8b, 0xec,         /* mov  %rsp, %rbp */
+      0x48, 0x83, 0xec, 0x10,   /* sub  imm8, %rsp */
+      0x90, 0x90, 0x90, 0x90, 0x90, 0x90}; /* nop */
+  amd64_init_frame_cache (&prologue_test.exp_cache);
+  prologue_test.exp_cache.frameless_p = 0;
+  prologue_tests.push_back (prologue_test);
+
+  prologue_test.exp_pc = 11;
+  prologue_test.memory = {0x55, /* push %rbp */
+      0x48, 0x8b, 0xec,         /* mov  %rsp, %rbp */
+      0x48, 0x81, 0xec, 0x10, 0x00, 0x00, 0x00, /* sub  imm32, %rsp */
+      0x90, 0x90, 0x90}; /* nop */
+  amd64_init_frame_cache (&prologue_test.exp_cache);
+  prologue_test.exp_cache.frameless_p = 0;
+  prologue_tests.push_back (prologue_test);
+
+  return prologue_tests;
+}
+
+/* Returns x32 test pattern and expected results.  */
+
+static std::vector<struct prologue_test_t> get_prologue_tests_x32 (void)
+{
+  std::vector<struct prologue_test_t> prologue_tests;
+  struct prologue_test_t prologue_test;
+
+  /* Negative test.  */
+  prologue_test.exp_pc = 0;
+  prologue_test.memory = {0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90,
+      0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90}; /* nop */
+  amd64_init_frame_cache (&prologue_test.exp_cache);
+  prologue_test.exp_cache.frameless_p = 1;
+  prologue_tests.push_back (prologue_test);
+
+  prologue_test.exp_pc = 3;
+  prologue_test.memory = {0x55, /* push %ebp */
+      0x89, 0xe5,               /* mov  %esp, %ebp */
+      0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90}; /* nop */
+  amd64_init_frame_cache (&prologue_test.exp_cache);
+  prologue_test.exp_cache.frameless_p = 0;
+  prologue_tests.push_back (prologue_test);
+
+  prologue_test.exp_pc = 3;
+  prologue_test.memory = {0x55, /* push %ebp */
+      0x8b, 0xec,               /* mov  %esp, %ebp */
+      0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90, 0x90}; /* nop */
+  amd64_init_frame_cache (&prologue_test.exp_cache);
+  prologue_test.exp_cache.frameless_p = 0;
+  prologue_tests.push_back (prologue_test);
+
+  prologue_test.exp_pc = 6;
+  prologue_test.memory = {0x55, /* push %ebp */
+      0x89, 0xe5,               /* mov  %esp, %ebp */
+      0x83, 0xec, 0x10,         /* sub  imm8, %esp */
+      0x90, 0x90, 0x90, 0x90, 0x90, 0x90}; /* nop */
+  amd64_init_frame_cache (&prologue_test.exp_cache);
+  prologue_test.exp_cache.frameless_p = 0;
+  prologue_tests.push_back (prologue_test);
+
+  prologue_test.exp_pc = 9;
+  prologue_test.memory = {0x55, /* push %ebp */
+      0x89, 0xe5,               /* mov  %esp, %ebp */
+      0x81, 0xec, 0x10,	0x00, 0x00, 0x00, /* sub  imm32, %esp */
+      0x90, 0x90, 0x90}; /* nop */
+  amd64_init_frame_cache (&prologue_test.exp_cache);
+  prologue_test.exp_cache.frameless_p = 0;
+  prologue_tests.push_back (prologue_test);
+
+  prologue_test.exp_pc = 6;
+  prologue_test.memory = {0x55, /* push %ebp */
+      0x8b, 0xec,               /* mov  %esp, %ebp */
+      0x83, 0xec, 0x10,         /* sub  imm8, %esp */
+      0x90, 0x90, 0x90, 0x90, 0x90, 0x90}; /* nop */
+  amd64_init_frame_cache (&prologue_test.exp_cache);
+  prologue_test.exp_cache.frameless_p = 0;
+  prologue_tests.push_back (prologue_test);
+
+  prologue_test.exp_pc = 9;
+  prologue_test.memory = {0x55,  /* push %ebp */
+      0x8b, 0xec,                /* mov  %esp, %ebp */
+      0x81, 0xec, 0x10,	0x00, 0x00, 0x00, /* sub  imm32, %esp */
+      0x90, 0x90, 0x90}; /* nop */
+  amd64_init_frame_cache (&prologue_test.exp_cache);
+  prologue_test.exp_cache.frameless_p = 0;
+  prologue_tests.push_back (prologue_test);
+
+  return prologue_tests;
+}
+
+/* Returns test pattern and expected results.  */
+
+static std::vector<struct prologue_test_t> get_prologue_tests (const std::string arch)
+{
+  SELF_CHECK (arch == "i386:x86-64"
+	      || arch == "i386:x64-32");
+
+  if (arch == "i386:x86-64")
+    return get_prologue_tests_x64 ();
+  else
+    return get_prologue_tests_x32 ();
+}
+
+static void
+amd64_analyze_prologue_test (void)
+{
+  const std::string arch_string[] = {"i386:x86-64", "i386:x64-32"};
+
+  for (const auto& current_arch : arch_string)
+    {
+      struct gdbarch_info info;
+      gdbarch_info_init (&info);
+      info.bfd_arch_info = bfd_scan_arch (current_arch.c_str ());
+      struct gdbarch *gdbarch = gdbarch_find_by_info (info);
+      SELF_CHECK (gdbarch != NULL);
+
+      const std::vector<struct prologue_test_t> prologue_tests =
+	  get_prologue_tests (current_arch);
+
+      for (auto& prologue_test : prologue_tests)
+	{
+	  code_reader_test reader (prologue_test.memory);
+	  struct amd64_frame_cache cache;
+	  amd64_init_frame_cache (&cache);
+
+	  CORE_ADDR pc = amd64_analyze_prologue (gdbarch, 0, 128, &cache,
+						 reader);
+
+	  SELF_CHECK (pc == prologue_test.exp_pc);
+	  SELF_CHECK (cache.frameless_p == prologue_test.exp_cache.frameless_p);
+	}
+    }
+}
+} // namespace selftests
+#endif /* GDB_SELF_TEST */
+
+
 /* Work around false termination of prologue - GCC PR debug/48827.
 
    START_PC is the first instruction of a function, PC is its minimal already
@@ -3256,6 +3518,10 @@ _initialize_amd64_tdep (void)
   initialize_tdesc_x32 ();
   initialize_tdesc_x32_avx ();
   initialize_tdesc_x32_avx512 ();
+
+#if GDB_SELF_TEST
+  register_self_test (selftests::amd64_analyze_prologue_test);
+#endif
 }
 \f
 
-- 
2.7.1.339.g0233b80

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

[Yao Qi]

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

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

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

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

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

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

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

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

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

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

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

Why don't you test other fields in amd64_frame_cache?

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

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

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

You can use .emplace_back to avoid copying.

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

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

-- 
Yao 

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

[Yao Qi]

On 16-12-16 14:58:36, Bernhard Heckel wrote:
> +
> +class code_reader : public abstract_code_reader
> +{
> +public:
> +  code_reader (void)
> +  {
> +    target_memory = 1;
> +  }
> +
> +  void read (CORE_ADDR memaddr, gdb_byte *buffer, ssize_t len)
> +  {
> +    read_code (memaddr, buffer, len);

Can we use target_read_code here?  It doesn't throw an error if it
can't read.  Method 'read' becomes

int read (CORE_ADDR memaddr, gdb_byte *buffer, ssize_t len)

so that we may even share more code with disassembly.  See Pedro's
POC patch.

-- 
Yao (齐尧)

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

[Bernhard Heckel]

On 20/12/2016 10:20, Yao Qi wrote:
> On 16-12-16 14:58:36, Bernhard Heckel wrote:
>> +
>> +class code_reader : public abstract_code_reader
>> +{
>> +public:
>> +  code_reader (void)
>> +  {
>> +    target_memory = 1;
>> +  }
>> +
>> +  void read (CORE_ADDR memaddr, gdb_byte *buffer, ssize_t len)
>> +  {
>> +    read_code (memaddr, buffer, len);
> Can we use target_read_code here?  It doesn't throw an error if it
> can't read.  Method 'read' becomes
>
> int read (CORE_ADDR memaddr, gdb_byte *buffer, ssize_t len)
>
> so that we may even share more code with disassembly.  See Pedro's
> POC patch.
>
Can you point me to the Pedro's patch?


Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928