[PATCH] gdb: Update i386_analyze_prologue to skip endbr32

["H.J. Lu" <hjl.tools@gmail.com>]

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