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

Message ID 20200624012857.31849-2-vcollod@nvidia.com
State New
Headers show
Series
  • Improve intel IBT support
Related show

Commit Message

Mike Frysinger via Gdb-patches June 24, 2020, 1:28 a.m.
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%)

-- 
2.20.1

Comments

Simon Marchi Aug. 6, 2020, 1:57 p.m. | #1
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

Patch

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
 }