[PATCH] gdb: Update i386_analyze_prologue to skip endbr32

H.J. Lu
2020-09-19T00:30:01Z

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