[PATCH] Improve intel IBT support

[PATCH] Improve intel IBT support

[Victor Collod]

Refactor amd64_analyze_prologue to be more linear, add i386 support for endbr32.

2020-03-12  Victor Collod  <vcollod@nvidia.com>

	* i386-tdep.c (i386_skip_endbr): add a helper function to skip endbr
	instructions.
	(i386_analyze_prologue): call i386_skip_endbr.
	* amd64-tdep.c (amd64_analyze_prologue): make the function more linear
---
 gdb/amd64-tdep.c | 76 +++++++++++++++++++++++-------------------------
 gdb/i386-tdep.c  | 19 ++++++++++++
 2 files changed, 56 insertions(+), 39 deletions(-)

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index f96a9868259..06d0fe9a194 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2374,7 +2374,6 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
 			CORE_ADDR pc, CORE_ADDR current_pc,
 			struct amd64_frame_cache *cache)
 {
-  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   /* The `endbr64` instruction.  */
   static const gdb_byte endbr64[4] = { 0xf3, 0x0f, 0x1e, 0xfa };
   /* There are two variations of movq %rsp, %rbp.  */
@@ -2384,8 +2383,7 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
   static const gdb_byte mov_esp_ebp_1[2] = { 0x89, 0xe5 };
   static const gdb_byte mov_esp_ebp_2[2] = { 0x8b, 0xec };
 
-  gdb_byte buf[3];
-  gdb_byte op;
+  gdb_byte buf[4];
 
   if (current_pc <= pc)
     return current_pc;
@@ -2395,57 +2393,57 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
   else
     pc = amd64_analyze_stack_align (pc, current_pc, cache);
 
-  op = read_code_unsigned_integer (pc, 1, byte_order);
+  read_code (pc, buf, 4);
 
   /* Check for the `endbr64` instruction, skip it if found.  */
-  if (op == endbr64[0])
+  if (memcmp (buf, endbr64, sizeof(endbr64)) == 0)
     {
-      read_code (pc + 1, buf, 3);
+      pc += sizeof(endbr64);
+      /* If that's all, return now.  */
+      if (current_pc <= pc)
+	return current_pc;
+      /* update the code buffer, as pc changed */
+      read_code (pc, buf, 1);
+    }
 
-      if (memcmp (buf, &endbr64[1], 3) == 0)
-	pc += 4;
+  /* stop right now if there's no `pushq %rbp' */
+  if (buf[0] != 0x55)
+    return pc;
 
-      op = read_code_unsigned_integer (pc, 1, byte_order);
-    }
+  /* Take into account that we've executed the `pushq %rbp' that
+     starts this instruction sequence.  */
+  cache->saved_regs[AMD64_RBP_REGNUM] = 0;
+  cache->sp_offset += 8;
 
+  pc += 1;
+
+  /* If that's all, return now.  */
   if (current_pc <= pc)
     return current_pc;
 
-  if (op == 0x55)		/* pushq %rbp */
-    {
-      /* Take into account that we've executed the `pushq %rbp' that
-         starts this instruction sequence.  */
-      cache->saved_regs[AMD64_RBP_REGNUM] = 0;
-      cache->sp_offset += 8;
+  read_code (pc, buf, 3);
 
-      /* If that's all, return now.  */
-      if (current_pc <= pc + 1)
-        return current_pc;
-
-      read_code (pc + 1, buf, 3);
+  /* Check for `movq %rsp, %rbp'.  */
+  if (memcmp (buf, mov_rsp_rbp_1, 3) == 0
+      || memcmp (buf, mov_rsp_rbp_2, 3) == 0)
+    {
+      pc += 3;
+      /* OK, we actually have a frame.  */
+      cache->frameless_p = 0;
+      return pc;
+    }
 
-      /* Check for `movq %rsp, %rbp'.  */
-      if (memcmp (buf, mov_rsp_rbp_1, 3) == 0
-	  || memcmp (buf, mov_rsp_rbp_2, 3) == 0)
+  /* For X32, also check for `movl %esp, %ebp'.  */
+  if (gdbarch_ptr_bit (gdbarch) == 32)
+    {
+      if (memcmp (buf, mov_esp_ebp_1, 2) == 0
+	  || memcmp (buf, mov_esp_ebp_2, 2) == 0)
 	{
+	  pc += 2;
 	  /* OK, we actually have a frame.  */
 	  cache->frameless_p = 0;
-	  return pc + 4;
+	  return pc;
 	}
-
-      /* For X32, also check for `movq %esp, %ebp'.  */
-      if (gdbarch_ptr_bit (gdbarch) == 32)
-	{
-	  if (memcmp (buf, mov_esp_ebp_1, 2) == 0
-	      || memcmp (buf, mov_esp_ebp_2, 2) == 0)
-	    {
-	      /* OK, we actually have a frame.  */
-	      cache->frameless_p = 0;
-	      return pc + 3;
-	    }
-	}
-
-      return pc + 1;
     }
 
   return pc;
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index e87d7f36356..f7670a7febb 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -1537,6 +1537,24 @@ struct i386_insn i386_frame_setup_skip_insns[] =
   { 0 }
 };
 
+/* Check whether PC points to an endbr32 instruction.  */
+static CORE_ADDR
+i386_skip_endbr(CORE_ADDR pc)
+{
+  static const gdb_byte endbr32[] = { 0xf3, 0x0f, 0x1e, 0xfb };
+
+  gdb_byte buf[sizeof(endbr32)];
+
+  /* Stop there if we can't read the code */
+  if (target_read_code (pc, buf, sizeof(endbr32)))
+    return pc;
+
+  /* If the instruction isn't an endbr32, stop */
+  if (memcmp (buf, endbr32, sizeof(endbr32)) != 0)
+    return pc;
+
+  return pc + sizeof(endbr32);
+}
 
 /* Check whether PC points to a no-op instruction.  */
 static CORE_ADDR
@@ -1814,6 +1832,7 @@ i386_analyze_prologue (struct gdbarch *gdbarch,
 		       CORE_ADDR pc, CORE_ADDR current_pc,
 		       struct i386_frame_cache *cache)
 {
+  pc = i386_skip_endbr (pc);
   pc = i386_skip_noop (pc);
   pc = i386_follow_jump (gdbarch, pc);
   pc = i386_analyze_struct_return (pc, current_pc, cache);
-- 
2.20.1

Re: [PATCH] Improve intel IBT support

[Victor Collod]

Hello!

I forgot to describe my current FSF CLA status: I have a past and future changes agreement.
It's my first contribution to gdb, I hope it isn't too bad!

Victor

Re: [PATCH] Improve intel IBT support

[Simon Marchi]

On 2020-06-05 7:23 p.m., Victor Collod via Gdb-patches wrote:
> Refactor amd64_analyze_prologue to be more linear, add i386 support for endbr32.

Hi Victor,

Thanks for the patch.  You mentioned this is your first submission; I see you've
used git-send-email, that's a very good start!

> 2020-03-12  Victor Collod  <vcollod@nvidia.com>
> 
> 	* i386-tdep.c (i386_skip_endbr): add a helper function to skip endbr
> 	instructions.
> 	(i386_analyze_prologue): call i386_skip_endbr.
> 	* amd64-tdep.c (amd64_analyze_prologue): make the function more linear

If I understand correctly, you are doing two orthogonal changes in this patch:

1- Change amd64_analyze_prologue to make it clearer / more readable (that's what I
   understand by "more linear")
2. Add support for skipping another instruction

If that's the case, I think that can be a two patches series, such that each patch
has only one concern.  This way, it's easier to convince ourself that each is correct.
Also, if a bug is introduced by one of the patches, it's easier to bisect and find
the culprit.

> ---
>  gdb/amd64-tdep.c | 76 +++++++++++++++++++++++-------------------------
>  gdb/i386-tdep.c  | 19 ++++++++++++
>  2 files changed, 56 insertions(+), 39 deletions(-)
> 
> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> index f96a9868259..06d0fe9a194 100644
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -2374,7 +2374,6 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
>  			CORE_ADDR pc, CORE_ADDR current_pc,
>  			struct amd64_frame_cache *cache)
>  {
> -  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>    /* The `endbr64` instruction.  */
>    static const gdb_byte endbr64[4] = { 0xf3, 0x0f, 0x1e, 0xfa };
>    /* There are two variations of movq %rsp, %rbp.  */
> @@ -2384,8 +2383,7 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
>    static const gdb_byte mov_esp_ebp_1[2] = { 0x89, 0xe5 };
>    static const gdb_byte mov_esp_ebp_2[2] = { 0x8b, 0xec };
>  
> -  gdb_byte buf[3];
> -  gdb_byte op;
> +  gdb_byte buf[4];
>  
>    if (current_pc <= pc)
>      return current_pc;
> @@ -2395,57 +2393,57 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
>    else
>      pc = amd64_analyze_stack_align (pc, current_pc, cache);
>  
> -  op = read_code_unsigned_integer (pc, 1, byte_order);
> +  read_code (pc, buf, 4);

Just guessing, but I thought that the purpose of reading just one byte here
is so that if we're right at the end of a readable memory region, we won't
read too far.  If read_code can't read the whole 4 bytes, it will throw an
exception.

>    /* Check for the `endbr64` instruction, skip it if found.  */
> -  if (op == endbr64[0])
> +  if (memcmp (buf, endbr64, sizeof(endbr64)) == 0)

Space after `sizeof`, happens a few times.

> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
> index e87d7f36356..f7670a7febb 100644
> --- a/gdb/i386-tdep.c
> +++ b/gdb/i386-tdep.c
> @@ -1537,6 +1537,24 @@ struct i386_insn i386_frame_setup_skip_insns[] =
>    { 0 }
>  };
>  
> +/* Check whether PC points to an endbr32 instruction.  */
> +static CORE_ADDR
> +i386_skip_endbr(CORE_ADDR pc)

Space before parenthesis.

Simon

[PATCH v2 0/2] Improve intel IBT support

[Victor Collod]

Thanks for your review!

Victor Collod (2):
  Add i386 support for endbr skipping
  Refactor amd64_analyze_prologue

 gdb/amd64-tdep.c | 93 ++++++++++++++++++++++++++----------------------
 gdb/i386-tdep.c  | 19 ++++++++++
 2 files changed, 69 insertions(+), 43 deletions(-)

-- 
2.20.1

[PATCH v2 1/2] Add i386 support for endbr skipping

[Victor Collod]

2020-06-11  Victor Collod  <vcollod@nvidia.com>

	* i386-tdep.c (i386_skip_endbr): Add a helper function to skip endbr.
	(i386_analyze_prologue): Call i386_skip_endbr.
---
 gdb/i386-tdep.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 9b905c1996a..263a3fd452e 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -1537,6 +1537,24 @@ struct i386_insn i386_frame_setup_skip_insns[] =
   { 0 }
 };
 
+/* Check whether PC points to an endbr32 instruction.  */
+static CORE_ADDR
+i386_skip_endbr (CORE_ADDR pc)
+{
+  static const gdb_byte endbr32[] = { 0xf3, 0x0f, 0x1e, 0xfb };
+
+  gdb_byte buf[sizeof (endbr32)];
+
+  /* Stop there if we can't read the code */
+  if (target_read_code (pc, buf, sizeof (endbr32)))
+    return pc;
+
+  /* If the instruction isn't an endbr32, stop */
+  if (memcmp (buf, endbr32, sizeof (endbr32)) != 0)
+    return pc;
+
+  return pc + sizeof (endbr32);
+}
 
 /* Check whether PC points to a no-op instruction.  */
 static CORE_ADDR
@@ -1814,6 +1832,7 @@ i386_analyze_prologue (struct gdbarch *gdbarch,
 		       CORE_ADDR pc, CORE_ADDR current_pc,
 		       struct i386_frame_cache *cache)
 {
+  pc = i386_skip_endbr (pc);
   pc = i386_skip_noop (pc);
   pc = i386_follow_jump (gdbarch, pc);
   pc = i386_analyze_struct_return (pc, current_pc, cache);
-- 
2.20.1

[PATCH v2 2/2] Refactor amd64_analyze_prologue

[Victor Collod]

* merge op and the buf array, which were both used for storing code
* invert conditions to avoid long nested ifs
* use target_read_code instead of read_code to gracefully handle errors
* `if (current_pc <= pc)' felt backwards, as current_pc doesn't change,
  and the test could be described as "stop if pc went past current_pc"

2020-06-11  Victor Collod  <vcollod@nvidia.com>

	* amd64-tdep.c (amd64_analyze_prologue): Make the function more readable.
---
 gdb/amd64-tdep.c | 93 ++++++++++++++++++++++++++----------------------
 1 file changed, 50 insertions(+), 43 deletions(-)

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 0ce9fbc2997..6c1a4a138de 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2374,7 +2374,6 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
 			CORE_ADDR pc, CORE_ADDR current_pc,
 			struct amd64_frame_cache *cache)
 {
-  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   /* The `endbr64` instruction.  */
   static const gdb_byte endbr64[4] = { 0xf3, 0x0f, 0x1e, 0xfa };
   /* There are two variations of movq %rsp, %rbp.  */
@@ -2384,10 +2383,10 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
   static const gdb_byte mov_esp_ebp_1[2] = { 0x89, 0xe5 };
   static const gdb_byte mov_esp_ebp_2[2] = { 0x8b, 0xec };
 
-  gdb_byte buf[3];
-  gdb_byte op;
+  gdb_byte buf[4];
 
-  if (current_pc <= pc)
+  /* Analysis must not go past current_pc */
+  if (pc >= current_pc)
     return current_pc;
 
   if (gdbarch_ptr_bit (gdbarch) == 32)
@@ -2395,57 +2394,65 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
   else
     pc = amd64_analyze_stack_align (pc, current_pc, cache);
 
-  op = read_code_unsigned_integer (pc, 1, byte_order);
-
-  /* Check for the `endbr64` instruction, skip it if found.  */
-  if (op == endbr64[0])
+  /* Try to read enough bytes to check for `endbr64'  */
+  if (target_read_code (pc, buf, 4) != 0)
+    {
+      /* If it fails, read just enough data for `pushq %rbp' */
+      if (target_read_code (pc, buf, 1) != 0)
+	return pc;
+    }
+  /* If reading succeeded, check for the `endbr64' instruction and skip it if found.  */
+  else if (memcmp (buf, endbr64, sizeof (endbr64)) == 0)
     {
-      read_code (pc + 1, buf, 3);
+      pc += sizeof (endbr64);
+      /* If we went past the allowed bound, stop */
+      if (pc >= current_pc)
+	return current_pc;
+      /* update the code buffer, as pc changed */
+      if (target_read_code (pc, buf, 1) != 0)
+	return pc;
+    }
 
-      if (memcmp (buf, &endbr64[1], 3) == 0)
-	pc += 4;
+  /* stop right now if there's no `pushq %rbp' */
+  if (buf[0] != 0x55)
+    return pc;
 
-      op = read_code_unsigned_integer (pc, 1, byte_order);
-    }
+  /* Take into account that we've executed the `pushq %rbp' that
+     starts this instruction sequence.  */
+  cache->saved_regs[AMD64_RBP_REGNUM] = 0;
+  cache->sp_offset += 8;
 
-  if (current_pc <= pc)
-    return current_pc;
+  pc += 1;
 
-  if (op == 0x55)		/* pushq %rbp */
-    {
-      /* Take into account that we've executed the `pushq %rbp' that
-         starts this instruction sequence.  */
-      cache->saved_regs[AMD64_RBP_REGNUM] = 0;
-      cache->sp_offset += 8;
+  /* If we went past the allowed bound, stop */
+  if (pc >= current_pc)
+    return current_pc;
 
-      /* If that's all, return now.  */
-      if (current_pc <= pc + 1)
-        return current_pc;
+  /* Try to read the code for the stack base move */
+  if (target_read_code (pc, buf, 3) != 0)
+    return pc;
 
-      read_code (pc + 1, buf, 3);
+  /* Check for `movq %rsp, %rbp'.  */
+  if (memcmp (buf, mov_rsp_rbp_1, 3) == 0
+      || memcmp (buf, mov_rsp_rbp_2, 3) == 0)
+    {
+      pc += 3;
+      /* OK, we actually have a frame.  */
+      cache->frameless_p = 0;
+      return pc;
+    }
 
-      /* Check for `movq %rsp, %rbp'.  */
-      if (memcmp (buf, mov_rsp_rbp_1, 3) == 0
-	  || memcmp (buf, mov_rsp_rbp_2, 3) == 0)
+  /* For X32, also check for `movl %esp, %ebp'.  */
+  if (gdbarch_ptr_bit (gdbarch) == 32)
+    {
+      if (memcmp (buf, mov_esp_ebp_1, 2) == 0
+	  || memcmp (buf, mov_esp_ebp_2, 2) == 0)
 	{
+	  pc += 2;
 	  /* OK, we actually have a frame.  */
 	  cache->frameless_p = 0;
-	  return pc + 4;
+	  return pc;
 	}
-
-      /* For X32, also check for `movq %esp, %ebp'.  */
-      if (gdbarch_ptr_bit (gdbarch) == 32)
-	{
-	  if (memcmp (buf, mov_esp_ebp_1, 2) == 0
-	      || memcmp (buf, mov_esp_ebp_2, 2) == 0)
-	    {
-	      /* OK, we actually have a frame.  */
-	      cache->frameless_p = 0;
-	      return pc + 3;
-	    }
-	}
-
-      return pc + 1;
     }
 
   return pc;
-- 
2.20.1

Re: [PATCH v2 1/2] Add i386 support for endbr skipping

[Simon Marchi]

On 2020-06-11 6:54 p.m., Victor Collod via Gdb-patches wrote:
> 2020-06-11  Victor Collod  <vcollod@nvidia.com>
> 
> 	* i386-tdep.c (i386_skip_endbr): Add a helper function to skip endbr.
> 	(i386_analyze_prologue): Call i386_skip_endbr.
> ---
>  gdb/i386-tdep.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
> index 9b905c1996a..263a3fd452e 100644
> --- a/gdb/i386-tdep.c
> +++ b/gdb/i386-tdep.c
> @@ -1537,6 +1537,24 @@ struct i386_insn i386_frame_setup_skip_insns[] =
>    { 0 }
>  };
>  
> +/* Check whether PC points to an endbr32 instruction.  */
> +static CORE_ADDR
> +i386_skip_endbr (CORE_ADDR pc)
> +{
> +  static const gdb_byte endbr32[] = { 0xf3, 0x0f, 0x1e, 0xfb };
> +
> +  gdb_byte buf[sizeof (endbr32)];
> +
> +  /* Stop there if we can't read the code */
> +  if (target_read_code (pc, buf, sizeof (endbr32)))
> +    return pc;
> +
> +  /* If the instruction isn't an endbr32, stop */
> +  if (memcmp (buf, endbr32, sizeof (endbr32)) != 0)
> +    return pc;
> +
> +  return pc + sizeof (endbr32);
> +}
>  
>  /* Check whether PC points to a no-op instruction.  */
>  static CORE_ADDR
> @@ -1814,6 +1832,7 @@ i386_analyze_prologue (struct gdbarch *gdbarch,
>  		       CORE_ADDR pc, CORE_ADDR current_pc,
>  		       struct i386_frame_cache *cache)
>  {
> +  pc = i386_skip_endbr (pc);
>    pc = i386_skip_noop (pc);
>    pc = i386_follow_jump (gdbarch, pc);
>    pc = i386_analyze_struct_return (pc, current_pc, cache);
> -- 
> 2.20.1

Hi Victor,

I hadn't realized that this instriction also existed in the 32 bit variant.

The patch looks fine, but is just missing a test.  You could maybe adapt
gdb.arch/amd64-prologue-skip-cf-protection.exp so that it runs on i386 as
well?  If so, I'd rename it from amd64- to i386-, like there are other tests
running on both amd64 and i386 that are prefixed i386.  It's not great, but
at least it would be consistent.

Simon

Re: [PATCH v2 2/2] Refactor amd64_analyze_prologue

[Simon Marchi]

On 2020-06-11 6:54 p.m., Victor Collod via Gdb-patches wrote:
> * merge op and the buf array, which were both used for storing code
> * invert conditions to avoid long nested ifs
> * use target_read_code instead of read_code to gracefully handle errors
> * `if (current_pc <= pc)' felt backwards, as current_pc doesn't change,
>   and the test could be described as "stop if pc went past current_pc"

I'd appreciate if you could do one patch per change here.  They'd each be
more trivial to review and ensure they are ok.

> 2020-06-11  Victor Collod  <vcollod@nvidia.com>
> 
> 	* amd64-tdep.c (amd64_analyze_prologue): Make the function more readable.
> ---
>  gdb/amd64-tdep.c | 93 ++++++++++++++++++++++++++----------------------
>  1 file changed, 50 insertions(+), 43 deletions(-)
> 
> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> index 0ce9fbc2997..6c1a4a138de 100644
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -2374,7 +2374,6 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
>  			CORE_ADDR pc, CORE_ADDR current_pc,
>  			struct amd64_frame_cache *cache)
>  {
> -  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>    /* The `endbr64` instruction.  */
>    static const gdb_byte endbr64[4] = { 0xf3, 0x0f, 0x1e, 0xfa };
>    /* There are two variations of movq %rsp, %rbp.  */
> @@ -2384,10 +2383,10 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
>    static const gdb_byte mov_esp_ebp_1[2] = { 0x89, 0xe5 };
>    static const gdb_byte mov_esp_ebp_2[2] = { 0x8b, 0xec };
>  
> -  gdb_byte buf[3];
> -  gdb_byte op;
> +  gdb_byte buf[4];
>  
> -  if (current_pc <= pc)
> +  /* Analysis must not go past current_pc */

Take the habit of starting each comment with a capital letter, finishing with a period and two
spaces (unless it's not really a sentence, like `/* no-op */`).

> +  if (pc >= current_pc)
>      return current_pc;
>  
>    if (gdbarch_ptr_bit (gdbarch) == 32)
> @@ -2395,57 +2394,65 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
>    else
>      pc = amd64_analyze_stack_align (pc, current_pc, cache);
>  
> -  op = read_code_unsigned_integer (pc, 1, byte_order);
> -
> -  /* Check for the `endbr64` instruction, skip it if found.  */
> -  if (op == endbr64[0])
> +  /* Try to read enough bytes to check for `endbr64'  */
> +  if (target_read_code (pc, buf, 4) != 0)
> +    {
> +      /* If it fails, read just enough data for `pushq %rbp' */
> +      if (target_read_code (pc, buf, 1) != 0)
> +	return pc;
> +    }
> +  /* If reading succeeded, check for the `endbr64' instruction and skip it if found.  */
> +  else if (memcmp (buf, endbr64, sizeof (endbr64)) == 0)
>      {
> -      read_code (pc + 1, buf, 3);
> +      pc += sizeof (endbr64);
> +      /* If we went past the allowed bound, stop */
> +      if (pc >= current_pc)
> +	return current_pc;

For readability, please add an empty line after return statements, when there
is another statement after it.

Thanks,

Simon

[PATCH v3 0/7] Improve intel IBT support

[Victor Collod]

Thanks again for your review!

Victor Collod (7):
  Add i386 support for endbr skipping
  amd64_analyze_prologue: swap upper bound check condition operands
  amd64_analyze_prologue: merge op and buf
  amd64_analyze_prologue: invert a condition for readability
  amd64_analyze_prologue: gradually update pc
  amd64_analyze_prologue: fix incorrect comment
  amd64_analyze_prologue: use target_read_code instead of read_code

 gdb/amd64-tdep.c                              | 94 ++++++++++---------
 gdb/i386-tdep.c                               | 19 ++++
 ...n.c => i386-prologue-skip-cf-protection.c} |  0
 ...p => i386-prologue-skip-cf-protection.exp} |  2 +-
 4 files changed, 71 insertions(+), 44 deletions(-)
 rename gdb/testsuite/gdb.arch/{amd64-prologue-skip-cf-protection.c => i386-prologue-skip-cf-protection.c} (100%)
 rename gdb/testsuite/gdb.arch/{amd64-prologue-skip-cf-protection.exp => i386-prologue-skip-cf-protection.exp} (97%)

-- 
2.20.1

[PATCH v3 1/7] Add i386 support for endbr skipping

[Victor Collod]

2020-06-11  Victor Collod  <vcollod@nvidia.com>

gdb/ChangeLog:

	* i386-tdep.c (i386_skip_endbr): Add a helper function to skip endbr.
	(i386_analyze_prologue): Call i386_skip_endbr.

gdb/testsuite/ChangeLog:

	* gdb.arch/amd64-prologue-skip-cf-protection.exp: Make the test
	compatible with i386, and move it to...
	* gdb.arch/i386-prologue-skip-cf-protection.exp: ... here.
	* gdb.arch/amd64-prologue-skip-cf-protection.c: Move to...
	* gdb.arch/i386-prologue-skip-cf-protection.c: ... here.
---
 gdb/i386-tdep.c                               | 19 +++++++++++++++++++
 ...n.c => i386-prologue-skip-cf-protection.c} |  0
 ...p => i386-prologue-skip-cf-protection.exp} |  2 +-
 3 files changed, 20 insertions(+), 1 deletion(-)
 rename gdb/testsuite/gdb.arch/{amd64-prologue-skip-cf-protection.c => i386-prologue-skip-cf-protection.c} (100%)
 rename gdb/testsuite/gdb.arch/{amd64-prologue-skip-cf-protection.exp => i386-prologue-skip-cf-protection.exp} (97%)

diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 9b905c1996a..263a3fd452e 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -1537,6 +1537,24 @@ struct i386_insn i386_frame_setup_skip_insns[] =
   { 0 }
 };
 
+/* Check whether PC points to an endbr32 instruction.  */
+static CORE_ADDR
+i386_skip_endbr (CORE_ADDR pc)
+{
+  static const gdb_byte endbr32[] = { 0xf3, 0x0f, 0x1e, 0xfb };
+
+  gdb_byte buf[sizeof (endbr32)];
+
+  /* Stop there if we can't read the code */
+  if (target_read_code (pc, buf, sizeof (endbr32)))
+    return pc;
+
+  /* If the instruction isn't an endbr32, stop */
+  if (memcmp (buf, endbr32, sizeof (endbr32)) != 0)
+    return pc;
+
+  return pc + sizeof (endbr32);
+}
 
 /* Check whether PC points to a no-op instruction.  */
 static CORE_ADDR
@@ -1814,6 +1832,7 @@ i386_analyze_prologue (struct gdbarch *gdbarch,
 		       CORE_ADDR pc, CORE_ADDR current_pc,
 		       struct i386_frame_cache *cache)
 {
+  pc = i386_skip_endbr (pc);
   pc = i386_skip_noop (pc);
   pc = i386_follow_jump (gdbarch, pc);
   pc = i386_analyze_struct_return (pc, current_pc, cache);
diff --git a/gdb/testsuite/gdb.arch/amd64-prologue-skip-cf-protection.c b/gdb/testsuite/gdb.arch/i386-prologue-skip-cf-protection.c
similarity index 100%
rename from gdb/testsuite/gdb.arch/amd64-prologue-skip-cf-protection.c
rename to gdb/testsuite/gdb.arch/i386-prologue-skip-cf-protection.c
diff --git a/gdb/testsuite/gdb.arch/amd64-prologue-skip-cf-protection.exp b/gdb/testsuite/gdb.arch/i386-prologue-skip-cf-protection.exp
similarity index 97%
rename from gdb/testsuite/gdb.arch/amd64-prologue-skip-cf-protection.exp
rename to gdb/testsuite/gdb.arch/i386-prologue-skip-cf-protection.exp
index 3c51fd30352..612c6edf9f1 100644
--- a/gdb/testsuite/gdb.arch/amd64-prologue-skip-cf-protection.exp
+++ b/gdb/testsuite/gdb.arch/i386-prologue-skip-cf-protection.exp
@@ -22,7 +22,7 @@
 standard_testfile .c
 set binfile ${binfile}
 
-if { ![istarget x86_64-*-* ] || ![is_lp64_target] } {
+if { ![istarget x86_64-*-*] && ![istarget i?86-*-*] } {
     verbose "Skipping ${testfile}."
     return
 }
-- 
2.20.1

[PATCH v3 2/7] amd64_analyze_prologue: swap upper bound check condition operands

[Victor Collod]

`if (current_pc <= pc)' felt backwards, as current_pc doesn't change,
and the test could be described as "stop if pc went past current_pc".

2020-06-23  Victor Collod  <vcollod@nvidia.com>

	* amd64-tdep.c (amd64_analyze_prologue): Swap upper bound check
	condition operands.
---
 gdb/amd64-tdep.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 0ce9fbc2997..ff12cb874b8 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2387,7 +2387,8 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
   gdb_byte buf[3];
   gdb_byte op;
 
-  if (current_pc <= pc)
+  /* Analysis must not go past current_pc.  */
+  if (pc >= current_pc)
     return current_pc;
 
   if (gdbarch_ptr_bit (gdbarch) == 32)
@@ -2408,7 +2409,8 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
       op = read_code_unsigned_integer (pc, 1, byte_order);
     }
 
-  if (current_pc <= pc)
+  /* If we went past the allowed bound, stop.  */
+  if (pc >= current_pc)
     return current_pc;
 
   if (op == 0x55)		/* pushq %rbp */
@@ -2418,8 +2420,8 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
       cache->saved_regs[AMD64_RBP_REGNUM] = 0;
       cache->sp_offset += 8;
 
-      /* If that's all, return now.  */
-      if (current_pc <= pc + 1)
+      /* If we went past the allowed bound, stop.  */
+      if (pc + 1 >= current_pc)
         return current_pc;
 
       read_code (pc + 1, buf, 3);
-- 
2.20.1

[PATCH v3 3/7] amd64_analyze_prologue: merge op and buf

[Victor Collod]

Both variables were used to store function code.

2020-06-23  Victor Collod  <vcollod@nvidia.com>

	* amd64-tdep.c (amd64_analyze_prologue): Merge op and buf.
---
 gdb/amd64-tdep.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index ff12cb874b8..c1a9b553e20 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2374,7 +2374,6 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
 			CORE_ADDR pc, CORE_ADDR current_pc,
 			struct amd64_frame_cache *cache)
 {
-  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   /* The `endbr64` instruction.  */
   static const gdb_byte endbr64[4] = { 0xf3, 0x0f, 0x1e, 0xfa };
   /* There are two variations of movq %rsp, %rbp.  */
@@ -2384,8 +2383,7 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
   static const gdb_byte mov_esp_ebp_1[2] = { 0x89, 0xe5 };
   static const gdb_byte mov_esp_ebp_2[2] = { 0x8b, 0xec };
 
-  gdb_byte buf[3];
-  gdb_byte op;
+  gdb_byte buf[4];
 
   /* Analysis must not go past current_pc.  */
   if (pc >= current_pc)
@@ -2396,24 +2394,20 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
   else
     pc = amd64_analyze_stack_align (pc, current_pc, cache);
 
-  op = read_code_unsigned_integer (pc, 1, byte_order);
-
-  /* Check for the `endbr64` instruction, skip it if found.  */
-  if (op == endbr64[0])
+  read_code (pc, buf, 4);
+  /* Check for the `endbr64' instruction and skip it if found.  */
+  if (memcmp (buf, endbr64, sizeof (endbr64)) == 0)
     {
-      read_code (pc + 1, buf, 3);
-
-      if (memcmp (buf, &endbr64[1], 3) == 0)
-	pc += 4;
+      pc += sizeof (endbr64);
+      /* If we went past the allowed bound, stop.  */
+      if (pc >= current_pc)
+	return current_pc;
 
-      op = read_code_unsigned_integer (pc, 1, byte_order);
+      /* Update the code buffer, as pc changed.  */
+      read_code (pc, buf, 1);
     }
 
-  /* If we went past the allowed bound, stop.  */
-  if (pc >= current_pc)
-    return current_pc;
-
-  if (op == 0x55)		/* pushq %rbp */
+  if (buf[0] == 0x55)           /* pushq %rbp */
     {
       /* Take into account that we've executed the `pushq %rbp' that
          starts this instruction sequence.  */
-- 
2.20.1

[PATCH v3 4/7] amd64_analyze_prologue: invert a condition for readability

[Victor Collod]

You can use git diff --ignore-space-at-eol -b -w --ignore-blank-lines
to make the patch clearer.

2020-06-23  Victor Collod  <vcollod@nvidia.com>

	* amd64-tdep.c (amd64_analyze_prologue): Invert a condition for readability.
---
 gdb/amd64-tdep.c | 57 ++++++++++++++++++++++++------------------------
 1 file changed, 28 insertions(+), 29 deletions(-)

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index c1a9b553e20..17b02706e54 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2407,44 +2407,43 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
       read_code (pc, buf, 1);
     }
 
-  if (buf[0] == 0x55)           /* pushq %rbp */
-    {
-      /* Take into account that we've executed the `pushq %rbp' that
-         starts this instruction sequence.  */
-      cache->saved_regs[AMD64_RBP_REGNUM] = 0;
-      cache->sp_offset += 8;
+  /* Stop right now if there's no `pushq %rbp'.  */
+  if (buf[0] != 0x55)
+    return pc;
 
-      /* If we went past the allowed bound, stop.  */
-      if (pc + 1 >= current_pc)
-        return current_pc;
+  /* Take into account that we've executed the `pushq %rbp' that
+     starts this instruction sequence.  */
+  cache->saved_regs[AMD64_RBP_REGNUM] = 0;
+  cache->sp_offset += 8;
+
+  /* If we went past the allowed bound, stop.  */
+  if (pc + 1 >= current_pc)
+    return current_pc;
+
+  read_code (pc + 1, buf, 3);
 
-      read_code (pc + 1, buf, 3);
+  /* Check for `movq %rsp, %rbp'.  */
+  if (memcmp (buf, mov_rsp_rbp_1, 3) == 0
+      || memcmp (buf, mov_rsp_rbp_2, 3) == 0)
+    {
+      /* OK, we actually have a frame.  */
+      cache->frameless_p = 0;
+      return pc + 4;
+    }
 
-      /* Check for `movq %rsp, %rbp'.  */
-      if (memcmp (buf, mov_rsp_rbp_1, 3) == 0
-	  || memcmp (buf, mov_rsp_rbp_2, 3) == 0)
+  /* For X32, also check for `movq %esp, %ebp'.  */
+  if (gdbarch_ptr_bit (gdbarch) == 32)
+    {
+      if (memcmp (buf, mov_esp_ebp_1, 2) == 0
+	  || memcmp (buf, mov_esp_ebp_2, 2) == 0)
 	{
 	  /* OK, we actually have a frame.  */
 	  cache->frameless_p = 0;
-	  return pc + 4;
-	}
-
-      /* For X32, also check for `movq %esp, %ebp'.  */
-      if (gdbarch_ptr_bit (gdbarch) == 32)
-	{
-	  if (memcmp (buf, mov_esp_ebp_1, 2) == 0
-	      || memcmp (buf, mov_esp_ebp_2, 2) == 0)
-	    {
-	      /* OK, we actually have a frame.  */
-	      cache->frameless_p = 0;
-	      return pc + 3;
-	    }
+	  return pc + 3;
 	}
-
-      return pc + 1;
     }
 
-  return pc;
+  return pc + 1;
 }
 
 /* Work around false termination of prologue - GCC PR debug/48827.
-- 
2.20.1

[PATCH v3 5/7] amd64_analyze_prologue: gradually update pc

[Victor Collod]

It makes the function easier to read, as you don't have to remember
what's the current offset from pc.

2020-06-23  Victor Collod  <vcollod@nvidia.com>

	* amd64-tdep.c (amd64_analyze_prologue): Gradually update pc.
---
 gdb/amd64-tdep.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 17b02706e54..5c3ad505784 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2416,19 +2416,22 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
   cache->saved_regs[AMD64_RBP_REGNUM] = 0;
   cache->sp_offset += 8;
 
+  pc += 1;
+
   /* If we went past the allowed bound, stop.  */
-  if (pc + 1 >= current_pc)
+  if (pc >= current_pc)
     return current_pc;
 
-  read_code (pc + 1, buf, 3);
+  read_code (pc, buf, 3);
 
   /* Check for `movq %rsp, %rbp'.  */
   if (memcmp (buf, mov_rsp_rbp_1, 3) == 0
       || memcmp (buf, mov_rsp_rbp_2, 3) == 0)
     {
+      pc += 3;
       /* OK, we actually have a frame.  */
       cache->frameless_p = 0;
-      return pc + 4;
+      return pc;
     }
 
   /* For X32, also check for `movq %esp, %ebp'.  */
@@ -2437,13 +2440,14 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
       if (memcmp (buf, mov_esp_ebp_1, 2) == 0
 	  || memcmp (buf, mov_esp_ebp_2, 2) == 0)
 	{
+	  pc += 2;
 	  /* OK, we actually have a frame.  */
 	  cache->frameless_p = 0;
-	  return pc + 3;
+	  return pc;
 	}
     }
 
-  return pc + 1;
+  return pc;
 }
 
 /* Work around false termination of prologue - GCC PR debug/48827.
-- 
2.20.1

[PATCH v3 6/7] amd64_analyze_prologue: fix incorrect comment

[Victor Collod]

The width of the instruction didn't match the size of its operands.

2020-06-23  Victor Collod  <vcollod@nvidia.com>

	* amd64-tdep.c (amd64_analyze_prologue): Fix incorrect comment.
---
 gdb/amd64-tdep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 5c3ad505784..901733cf443 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2434,7 +2434,7 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
       return pc;
     }
 
-  /* For X32, also check for `movq %esp, %ebp'.  */
+  /* For X32, also check for `movl %esp, %ebp'.  */
   if (gdbarch_ptr_bit (gdbarch) == 32)
     {
       if (memcmp (buf, mov_esp_ebp_1, 2) == 0
-- 
2.20.1

[PATCH v3 7/7] amd64_analyze_prologue: use target_read_code instead of read_code

[Victor Collod]

Using target_read_code enables gracefuly handling error cases.

2020-06-23  Victor Collod  <vcollod@nvidia.com>

	* amd64-tdep.c (amd64_analyze_prologue): Use target_read_code
	instead of read_code.
---
 gdb/amd64-tdep.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 901733cf443..7f70c1d0d8d 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2394,9 +2394,15 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
   else
     pc = amd64_analyze_stack_align (pc, current_pc, cache);
 
-  read_code (pc, buf, 4);
-  /* Check for the `endbr64' instruction and skip it if found.  */
-  if (memcmp (buf, endbr64, sizeof (endbr64)) == 0)
+  /* Try to read enough bytes to check for `endbr64'.  */
+  if (target_read_code (pc, buf, 4) != 0)
+    {
+      /* If it fails, read just enough data for `pushq %rbp'.  */
+      if (target_read_code (pc, buf, 1) != 0)
+	return pc;
+    }
+  /* If reading succeeded, check for the `endbr64' instruction and skip it if found.  */
+  else if (memcmp (buf, endbr64, sizeof (endbr64)) == 0)
     {
       pc += sizeof (endbr64);
       /* If we went past the allowed bound, stop.  */
@@ -2404,7 +2410,8 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
 	return current_pc;
 
       /* Update the code buffer, as pc changed.  */
-      read_code (pc, buf, 1);
+      if (target_read_code (pc, buf, 1) != 0)
+	return pc;
     }
 
   /* Stop right now if there's no `pushq %rbp'.  */
@@ -2422,7 +2429,9 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
   if (pc >= current_pc)
     return current_pc;
 
-  read_code (pc, buf, 3);
+  /* Try to read the code for the stack base move.  */
+  if (target_read_code (pc, buf, 3) != 0)
+    return pc;
 
   /* Check for `movq %rsp, %rbp'.  */
   if (memcmp (buf, mov_rsp_rbp_1, 3) == 0
-- 
2.20.1

Re: [PATCH v3 0/7] Improve intel IBT support

[Victor Collod]

Hello,

Can someone have a look?

Thanks!
________________________________
From: Victor Collod <vcollod@nvidia.com>
Sent: Tuesday, June 23, 2020 6:28 PM
To: gdb-patches@sourceware.org <gdb-patches@sourceware.org>
Cc: Victor Collod <vcollod@nvidia.com>
Subject: [PATCH v3 0/7] Improve intel IBT support

Thanks again for your review!

Victor Collod (7):
  Add i386 support for endbr skipping
  amd64_analyze_prologue: swap upper bound check condition operands
  amd64_analyze_prologue: merge op and buf
  amd64_analyze_prologue: invert a condition for readability
  amd64_analyze_prologue: gradually update pc
  amd64_analyze_prologue: fix incorrect comment
  amd64_analyze_prologue: use target_read_code instead of read_code

 gdb/amd64-tdep.c                              | 94 ++++++++++---------
 gdb/i386-tdep.c                               | 19 ++++
 ...n.c => i386-prologue-skip-cf-protection.c} |  0
 ...p => i386-prologue-skip-cf-protection.exp} |  2 +-
 4 files changed, 71 insertions(+), 44 deletions(-)
 rename gdb/testsuite/gdb.arch/{amd64-prologue-skip-cf-protection.c => i386-prologue-skip-cf-protection.c} (100%)
 rename gdb/testsuite/gdb.arch/{amd64-prologue-skip-cf-protection.exp => i386-prologue-skip-cf-protection.exp} (97%)

--
2.20.1

Re: [PATCH v3 1/7] Add i386 support for endbr skipping

[Simon Marchi]

On 2020-06-23 9:28 p.m., Victor Collod via Gdb-patches wrote:
> 2020-06-11  Victor Collod  <vcollod@nvidia.com>
> 

Please write a commit message that explains the change.  Imagine that you are
talking to somebody already somewhat knowledgeable about the subject matter, but
who doesn't know what you are working on or the problem you are trying to fix
(this will be the case for most people trying to understand this patch in the
future, if they git-blame this code).  So you don't to go in details explaining
what prologue skipping is, for example, but you need to explain what triggered
you to write this patch.  What didn't work, what's the bug, what is the impact
of the bug, how do you fix it?  And since it's relevant to this patch, how do
modify / improve the testsuite to make sure this gets tested?

Since this was already explained in commit ac4a4f1cd7dc ("gdb: handle endbr64
instruction in amd64_analyze_prologue"), you can always refer to this commit
and say that you are fixing the same bug, but for i386 instead of amd64.

When referring to another commit, always include both the sha1 and the subject/title.

The Linux kernel way of doing it [1] is fine.

[1] https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html

> gdb/ChangeLog:
> 
> 	* i386-tdep.c (i386_skip_endbr): Add a helper function to skip endbr.
> 	(i386_analyze_prologue): Call i386_skip_endbr.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.arch/amd64-prologue-skip-cf-protection.exp: Make the test
> 	compatible with i386, and move it to...
> 	* gdb.arch/i386-prologue-skip-cf-protection.exp: ... here.
> 	* gdb.arch/amd64-prologue-skip-cf-protection.c: Move to...
> 	* gdb.arch/i386-prologue-skip-cf-protection.c: ... here.
> ---
>  gdb/i386-tdep.c                               | 19 +++++++++++++++++++
>  ...n.c => i386-prologue-skip-cf-protection.c} |  0
>  ...p => i386-prologue-skip-cf-protection.exp} |  2 +-
>  3 files changed, 20 insertions(+), 1 deletion(-)
>  rename gdb/testsuite/gdb.arch/{amd64-prologue-skip-cf-protection.c => i386-prologue-skip-cf-protection.c} (100%)
>  rename gdb/testsuite/gdb.arch/{amd64-prologue-skip-cf-protection.exp => i386-prologue-skip-cf-protection.exp} (97%)
> 
> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
> index 9b905c1996a..263a3fd452e 100644
> --- a/gdb/i386-tdep.c
> +++ b/gdb/i386-tdep.c
> @@ -1537,6 +1537,24 @@ struct i386_insn i386_frame_setup_skip_insns[] =
>    { 0 }
>  };
>  
> +/* Check whether PC points to an endbr32 instruction.  */
> +static CORE_ADDR
> +i386_skip_endbr (CORE_ADDR pc)
> +{
> +  static const gdb_byte endbr32[] = { 0xf3, 0x0f, 0x1e, 0xfb };
> +
> +  gdb_byte buf[sizeof (endbr32)];
> +
> +  /* Stop there if we can't read the code */
> +  if (target_read_code (pc, buf, sizeof (endbr32)))

Compare explicitly with `!= 0`.

In the test, please update the comment on top of the file.  Where it talks about the endbr64
instruction, it should now say: `endbr32` / `endbr64`.

Simon

Re: [PATCH v3 2/7] amd64_analyze_prologue: swap upper bound check condition operands

[Simon Marchi]

On 2020-06-23 9:28 p.m., Victor Collod via Gdb-patches wrote:
> `if (current_pc <= pc)' felt backwards, as current_pc doesn't change,
> and the test could be described as "stop if pc went past current_pc".

This LGTM.

Simon

Re: [PATCH v3 3/7] amd64_analyze_prologue: merge op and buf

[Simon Marchi]

On 2020-06-23 9:28 p.m., Victor Collod via Gdb-patches wrote:
> Both variables were used to store function code.

I expressed concerns earlier that the code might be written in this way on purpose,
to make sure we don't read too much, in case we are close to the end of a readable
region.  I don't know if this is something that can actually happen in pracptice,
but still tt makes sense to me that we read a single byte, check what it is, and
read more if needed.  Any performance concerns should be taken care of by some cache
at the lower level (each call to read_code won't necessarily cause a target read to
happen).


> 2020-06-23  Victor Collod  <vcollod@nvidia.com>
> 
> 	* amd64-tdep.c (amd64_analyze_prologue): Merge op and buf.
> ---
>  gdb/amd64-tdep.c | 28 +++++++++++-----------------
>  1 file changed, 11 insertions(+), 17 deletions(-)
> 
> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> index ff12cb874b8..c1a9b553e20 100644
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -2374,7 +2374,6 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
>  			CORE_ADDR pc, CORE_ADDR current_pc,
>  			struct amd64_frame_cache *cache)
>  {
> -  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>    /* The `endbr64` instruction.  */
>    static const gdb_byte endbr64[4] = { 0xf3, 0x0f, 0x1e, 0xfa };
>    /* There are two variations of movq %rsp, %rbp.  */
> @@ -2384,8 +2383,7 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
>    static const gdb_byte mov_esp_ebp_1[2] = { 0x89, 0xe5 };
>    static const gdb_byte mov_esp_ebp_2[2] = { 0x8b, 0xec };
>  
> -  gdb_byte buf[3];
> -  gdb_byte op;
> +  gdb_byte buf[4];
>  
>    /* Analysis must not go past current_pc.  */
>    if (pc >= current_pc)
> @@ -2396,24 +2394,20 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
>    else
>      pc = amd64_analyze_stack_align (pc, current_pc, cache);
>  
> -  op = read_code_unsigned_integer (pc, 1, byte_order);
> -
> -  /* Check for the `endbr64` instruction, skip it if found.  */
> -  if (op == endbr64[0])
> +  read_code (pc, buf, 4);
> +  /* Check for the `endbr64' instruction and skip it if found.  */
> +  if (memcmp (buf, endbr64, sizeof (endbr64)) == 0)
>      {
> -      read_code (pc + 1, buf, 3);
> -
> -      if (memcmp (buf, &endbr64[1], 3) == 0)
> -	pc += 4;
> +      pc += sizeof (endbr64);
> +      /* If we went past the allowed bound, stop.  */
> +      if (pc >= current_pc)
> +	return current_pc;
>  
> -      op = read_code_unsigned_integer (pc, 1, byte_order);
> +      /* Update the code buffer, as pc changed.  */
> +      read_code (pc, buf, 1);
>      }
>  
> -  /* If we went past the allowed bound, stop.  */
> -  if (pc >= current_pc)
> -    return current_pc;
> -
> -  if (op == 0x55)		/* pushq %rbp */
> +  if (buf[0] == 0x55)           /* pushq %rbp */
>      {
>        /* Take into account that we've executed the `pushq %rbp' that
>           starts this instruction sequence.  */

If it's ok to read 4 bytes from the start, then why not merge remove the
following read (not shown in the snippet):

    read_code (pc + 1, buf, 3);

and make the read above (which happens if the endbr64 instruction is found) read
4 bytes directly?

Simon

Re: [PATCH v3 4/7] amd64_analyze_prologue: invert a condition for readability

[Simon Marchi]

On 2020-06-23 9:28 p.m., Victor Collod via Gdb-patches wrote:
> You can use git diff --ignore-space-at-eol -b -w --ignore-blank-lines
> to make the patch clearer.

This LGTM.

Simon

Re: [PATCH v3 5/7] amd64_analyze_prologue: gradually update pc

[Simon Marchi]

On 2020-06-23 9:28 p.m., Victor Collod via Gdb-patches wrote:
> It makes the function easier to read, as you don't have to remember
> what's the current offset from pc.

I agree, this LGTM.

Simon

Re: [PATCH v3 7/7] amd64_analyze_prologue: use target_read_code instead of read_code

[Simon Marchi]

On 2020-06-23 9:28 p.m., Victor Collod via Gdb-patches wrote:
> Using target_read_code enables gracefuly handling error cases.

Can you expand a bit why you think it's more graceful?

> 
> 2020-06-23  Victor Collod  <vcollod@nvidia.com>
> 
> 	* amd64-tdep.c (amd64_analyze_prologue): Use target_read_code
> 	instead of read_code.
> ---
>  gdb/amd64-tdep.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> index 901733cf443..7f70c1d0d8d 100644
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -2394,9 +2394,15 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
>    else
>      pc = amd64_analyze_stack_align (pc, current_pc, cache);
>  
> -  read_code (pc, buf, 4);
> -  /* Check for the `endbr64' instruction and skip it if found.  */
> -  if (memcmp (buf, endbr64, sizeof (endbr64)) == 0)
> +  /* Try to read enough bytes to check for `endbr64'.  */
> +  if (target_read_code (pc, buf, 4) != 0)
> +    {
> +      /* If it fails, read just enough data for `pushq %rbp'.  */
> +      if (target_read_code (pc, buf, 1) != 0)
> +	return pc;
> +    }
> +  /* If reading succeeded, check for the `endbr64' instruction and skip it if found.  */
> +  else if (memcmp (buf, endbr64, sizeof (endbr64)) == 0)

Personally, I find this "try to read 4 else try to read 1" approach less clear
and less intuitive than the "read 1 byte and read more if needed approach".

Simon

Re: [PATCH v3 6/7] amd64_analyze_prologue: fix incorrect comment

[Simon Marchi]

On 2020-06-23 9:28 p.m., Victor Collod via Gdb-patches wrote:
> The width of the instruction didn't match the size of its operands.
> 
> 2020-06-23  Victor Collod  <vcollod@nvidia.com>
> 
> 	* amd64-tdep.c (amd64_analyze_prologue): Fix incorrect comment.
> ---
>  gdb/amd64-tdep.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> index 5c3ad505784..901733cf443 100644
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -2434,7 +2434,7 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
>        return pc;
>      }
>  
> -  /* For X32, also check for `movq %esp, %ebp'.  */
> +  /* For X32, also check for `movl %esp, %ebp'.  */
>    if (gdbarch_ptr_bit (gdbarch) == 32)
>      {
>        if (memcmp (buf, mov_esp_ebp_1, 2) == 0
> -- 
> 2.20.1
> 

Thanks, I pushed this fix right away since it's obvious.

Simon

[PATCH] gdb: Update i386_analyze_prologue to skip endbr32

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 3300 bytes --]

On Thu, Aug 6, 2020 at 6:59 AM Simon Marchi <simark@simark.ca> wrote:
>
> On 2020-06-23 9:28 p.m., Victor Collod via Gdb-patches wrote:
> > 2020-06-11  Victor Collod  <vcollod@nvidia.com>
> >
>
> Please write a commit message that explains the change.  Imagine that you are
> talking to somebody already somewhat knowledgeable about the subject matter, but
> who doesn't know what you are working on or the problem you are trying to fix
> (this will be the case for most people trying to understand this patch in the
> future, if they git-blame this code).  So you don't to go in details explaining
> what prologue skipping is, for example, but you need to explain what triggered
> you to write this patch.  What didn't work, what's the bug, what is the impact
> of the bug, how do you fix it?  And since it's relevant to this patch, how do
> modify / improve the testsuite to make sure this gets tested?
>
> Since this was already explained in commit ac4a4f1cd7dc ("gdb: handle endbr64
> instruction in amd64_analyze_prologue"), you can always refer to this commit
> and say that you are fixing the same bug, but for i386 instead of amd64.
>
> When referring to another commit, always include both the sha1 and the subject/title.
>
> The Linux kernel way of doing it [1] is fine.
>
> [1] https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html
>
> > gdb/ChangeLog:
> >
> >       * i386-tdep.c (i386_skip_endbr): Add a helper function to skip endbr.
> >       (i386_analyze_prologue): Call i386_skip_endbr.
> >
> > gdb/testsuite/ChangeLog:
> >
> >       * gdb.arch/amd64-prologue-skip-cf-protection.exp: Make the test
> >       compatible with i386, and move it to...
> >       * gdb.arch/i386-prologue-skip-cf-protection.exp: ... here.
> >       * gdb.arch/amd64-prologue-skip-cf-protection.c: Move to...
> >       * gdb.arch/i386-prologue-skip-cf-protection.c: ... here.
> > ---
> >  gdb/i386-tdep.c                               | 19 +++++++++++++++++++
> >  ...n.c => i386-prologue-skip-cf-protection.c} |  0
> >  ...p => i386-prologue-skip-cf-protection.exp} |  2 +-
> >  3 files changed, 20 insertions(+), 1 deletion(-)
> >  rename gdb/testsuite/gdb.arch/{amd64-prologue-skip-cf-protection.c => i386-prologue-skip-cf-protection.c} (100%)
> >  rename gdb/testsuite/gdb.arch/{amd64-prologue-skip-cf-protection.exp => i386-prologue-skip-cf-protection.exp} (97%)
> >
> > diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
> > index 9b905c1996a..263a3fd452e 100644
> > --- a/gdb/i386-tdep.c
> > +++ b/gdb/i386-tdep.c
> > @@ -1537,6 +1537,24 @@ struct i386_insn i386_frame_setup_skip_insns[] =
> >    { 0 }
> >  };
> >
> > +/* Check whether PC points to an endbr32 instruction.  */
> > +static CORE_ADDR
> > +i386_skip_endbr (CORE_ADDR pc)
> > +{
> > +  static const gdb_byte endbr32[] = { 0xf3, 0x0f, 0x1e, 0xfb };
> > +
> > +  gdb_byte buf[sizeof (endbr32)];
> > +
> > +  /* Stop there if we can't read the code */
> > +  if (target_read_code (pc, buf, sizeof (endbr32)))
>
> Compare explicitly with `!= 0`.
>
> In the test, please update the comment on top of the file.  Where it talks about the endbr64
> instruction, it should now say: `endbr32` / `endbr64`.
>

I updated Victor's patch to fix:

https://sourceware.org/bugzilla/show_bug.cgi?id=26635

OK for master?

Thanks.

-- 
H.J.

[-- Attachment #2: 0001-gdb-Update-i386_analyze_prologue-to-skip-endbr32.patch --]
[-- Type: text/x-patch, Size: 4881 bytes --]

From 95a7715a0e085cbbaff33d9291647d5249a55b76 Mon Sep 17 00:00:00 2001
From: Victor Collod <vcollod@nvidia.com>
Date: Tue, 23 Jun 2020 18:28:51 -0700
Subject: [PATCH] gdb: Update i386_analyze_prologue to skip endbr32

With -m32 -fcf-protection, GCC generates an `endbr32` instruction at the
function entry:

[hjl@gnu-cfl-2 gdb]$ cat /tmp/x.c
int
main(void)
{
  return 0;
}
[hjl@gnu-cfl-2 gdb]$ gcc -g -fcf-protection /tmp/x.c -m32
(gdb) b main
Breakpoint 1 at 0x8049176: file /tmp/x.c, line 3.
(gdb) r
Breakpoint 1, main () at /tmp/x.c:3
3	{
(gdb) disass
Dump of assembler code for function main:
=> 0x08049176 <+0>:	endbr32
   0x0804917a <+4>:	push   %ebp
   0x0804917b <+5>:	mov    %esp,%ebp
   0x0804917d <+7>:	mov    $0x0,%eax
   0x08049182 <+12>:	pop    %ebp
   0x08049183 <+13>:	ret
End of assembler dump.
(gdb)

Update i386_analyze_prologue to skip `endbr32`:

(gdb) b main
Breakpoint 1 at 0x804917d: file /tmp/x.c, line 4.
(gdb) r
Breakpoint 1, main () at /tmp/x.c:4
4	  return 0;
(gdb) disass
Dump of assembler code for function main:
   0x08049176 <+0>:	endbr32
   0x0804917a <+4>:	push   %ebp
   0x0804917b <+5>:	mov    %esp,%ebp
=> 0x0804917d <+7>:	mov    $0x0,%eax
   0x08049182 <+12>:	pop    %ebp
   0x08049183 <+13>:	ret
End of assembler dump.
(gdb)

Tested with

$ make check RUNTESTFLAGS="--target_board='unix{-m32,}' i386-prologue-skip-cf-protection.exp"

on Fedora 32/x86-64.

2020-0X-YY  Victor Collod  <vcollod@nvidia.com>

gdb/ChangeLog:

	PR gdb/26635
	* i386-tdep.c (i386_skip_endbr): Add a helper function to skip endbr.
	(i386_analyze_prologue): Call i386_skip_endbr.

gdb/testsuite/ChangeLog:

	PR gdb/26635
	* gdb.arch/amd64-prologue-skip-cf-protection.exp: Make the test
	compatible with i386, and move it to...
	* gdb.arch/i386-prologue-skip-cf-protection.exp: ... here.
	* gdb.arch/amd64-prologue-skip-cf-protection.c: Move to...
	* gdb.arch/i386-prologue-skip-cf-protection.c: ... here.
---
 gdb/i386-tdep.c                               | 19 +++++++++++++++++++
 ...n.c => i386-prologue-skip-cf-protection.c} |  0
 ...p => i386-prologue-skip-cf-protection.exp} |  6 +++---
 3 files changed, 22 insertions(+), 3 deletions(-)
 rename gdb/testsuite/gdb.arch/{amd64-prologue-skip-cf-protection.c => i386-prologue-skip-cf-protection.c} (100%)
 rename gdb/testsuite/gdb.arch/{amd64-prologue-skip-cf-protection.exp => i386-prologue-skip-cf-protection.exp} (90%)

diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 95cfe5b820e..b485f0b296a 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -1538,6 +1538,24 @@ struct i386_insn i386_frame_setup_skip_insns[] =
   { 0 }
 };
 
+/* Check whether PC points to an endbr32 instruction.  */
+static CORE_ADDR
+i386_skip_endbr (CORE_ADDR pc)
+{
+  static const gdb_byte endbr32[] = { 0xf3, 0x0f, 0x1e, 0xfb };
+
+  gdb_byte buf[sizeof (endbr32)];
+
+  /* Stop there if we can't read the code */
+  if (target_read_code (pc, buf, sizeof (endbr32)))
+    return pc;
+
+  /* If the instruction isn't an endbr32, stop */
+  if (memcmp (buf, endbr32, sizeof (endbr32)) != 0)
+    return pc;
+
+  return pc + sizeof (endbr32);
+}
 
 /* Check whether PC points to a no-op instruction.  */
 static CORE_ADDR
@@ -1815,6 +1833,7 @@ i386_analyze_prologue (struct gdbarch *gdbarch,
 		       CORE_ADDR pc, CORE_ADDR current_pc,
 		       struct i386_frame_cache *cache)
 {
+  pc = i386_skip_endbr (pc);
   pc = i386_skip_noop (pc);
   pc = i386_follow_jump (gdbarch, pc);
   pc = i386_analyze_struct_return (pc, current_pc, cache);
diff --git a/gdb/testsuite/gdb.arch/amd64-prologue-skip-cf-protection.c b/gdb/testsuite/gdb.arch/i386-prologue-skip-cf-protection.c
similarity index 100%
rename from gdb/testsuite/gdb.arch/amd64-prologue-skip-cf-protection.c
rename to gdb/testsuite/gdb.arch/i386-prologue-skip-cf-protection.c
diff --git a/gdb/testsuite/gdb.arch/amd64-prologue-skip-cf-protection.exp b/gdb/testsuite/gdb.arch/i386-prologue-skip-cf-protection.exp
similarity index 90%
rename from gdb/testsuite/gdb.arch/amd64-prologue-skip-cf-protection.exp
rename to gdb/testsuite/gdb.arch/i386-prologue-skip-cf-protection.exp
index 3c51fd30352..9ba64f9c375 100644
--- a/gdb/testsuite/gdb.arch/amd64-prologue-skip-cf-protection.exp
+++ b/gdb/testsuite/gdb.arch/i386-prologue-skip-cf-protection.exp
@@ -16,13 +16,13 @@
 # Test skipping a prologue that was generated with gcc's -fcf-protection=full
 # (control flow protection) option.
 #
-# This option places an `endbr64` instruction at the start of all functions,
-# which can interfere with prologue analysis.
+# This option places an `endbr32`/`endbr64` instruction at the start of
+# all functions, which can interfere with prologue analysis.
 
 standard_testfile .c
 set binfile ${binfile}
 
-if { ![istarget x86_64-*-* ] || ![is_lp64_target] } {
+if { ![istarget x86_64-*-*] && ![istarget i?86-*-*] } {
     verbose "Skipping ${testfile}."
     return
 }
-- 
2.26.2

Re: [PATCH] gdb: Update i386_analyze_prologue to skip endbr32

[Simon Marchi]

On 2020-09-18 8:29 p.m., H.J. Lu wrote:
> I updated Victor's patch to fix:
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=26635
>
> OK for master?
>
> Thanks.

This is OK, thanks for writing the commit message.

Simon